From: Dave Chinner <firstname.lastname@example.org> To: Ira Weiny <email@example.com> Cc: firstname.lastname@example.org, Alexander Viro <email@example.com>, "Darrick J. Wong" <firstname.lastname@example.org>, Dan Williams <email@example.com>, Christoph Hellwig <firstname.lastname@example.org>, "Theodore Y. Ts'o" <email@example.com>, Jan Kara <firstname.lastname@example.org>, email@example.com, firstname.lastname@example.org, email@example.com Subject: Re: [PATCH 5/5] fs/xfs: Allow toggle of physical DAX flag Date: Tue, 22 Oct 2019 10:46:04 +1100 [thread overview] Message-ID: <20191021234604.GB2681@dread.disaster.area> (raw) In-Reply-To: <20191021224931.GA25526@iweiny-DESK2.sc.intel.com> On Mon, Oct 21, 2019 at 03:49:31PM -0700, Ira Weiny wrote: > On Mon, Oct 21, 2019 at 11:45:36AM +1100, Dave Chinner wrote: > > On Sun, Oct 20, 2019 at 08:59:35AM -0700, firstname.lastname@example.org wrote: > > > @@ -1232,12 +1233,10 @@ xfs_diflags_to_linux( > > > inode->i_flags |= S_NOATIME; > > > else > > > inode->i_flags &= ~S_NOATIME; > > > -#if 0 /* disabled until the flag switching races are sorted out */ > > > if (xflags & FS_XFLAG_DAX) > > > inode->i_flags |= S_DAX; > > > else > > > inode->i_flags &= ~S_DAX; > > > -#endif > > > > This code has bit-rotted. See xfs_setup_iops(), where we now have a > > different inode->i_mapping->a_ops for DAX inodes. > > :-( > > > > > That, fundamentally, is the issue here - it's not setting/clearing > > the DAX flag that is the issue, it's doing a swap of the > > mapping->a_ops while there may be other code using that ops > > structure. > > > > IOWs, if there is any code anywhere in the kernel that > > calls an address space op without holding one of the three locks we > > hold here (i_rwsem, MMAPLOCK, ILOCK) then it can race with the swap > > of the address space operations. > > > > By limiting the address space swap to file sizes of zero, we rule > > out the page fault path (mmap of a zero length file segv's with an > > access beyond EOF on the first read/write page fault, right?). > > Yes I checked that and thought we were safe here... > > > However, other aops callers that might run unlocked and do the wrong > > thing if the aops pointer is swapped between check of the aop method > > existing and actually calling it even if the file size is zero? > > > > A quick look shows that FIBMAP (ioctl_fibmap())) looks susceptible > > to such a race condition with the current definitions of the XFS DAX > > aops. I'm guessing there will be others, but I haven't looked > > further than this... > > I'll check for others and think on what to do about this. ext4 will have the > same problem I think. :-( > > I don't suppose using a single a_ops for both DAX and non-DAX is palatable? IMO, no. It means we have to check IS_DAX() in every aops, and replicate a bunch of callouts to generic code. i.e this sort of thing: if (aops->method) return aops->method(...) /* do something else */ results in us having to replicate that logic as something like: if (!IS_DAX) return filesystem_aops_method() /* do something else */ Indeed, the calling code may well do the wrong thing if we have methods defined just to add IS_DAX() checks to avoid using that functionality because the caller now thinks that functionality is supported when in fact it isn't. So it seems to me like an even bigger can of worms to try to use a single aops structure for vastly different functionality.... Cheers, Dave. -- Dave Chinner email@example.com
next prev parent reply other threads:[~2019-10-21 23:46 UTC|newest] Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-10-20 15:59 [PATCH 0/5] Enable per-file/directory DAX operations ira.weiny 2019-10-20 15:59 ` [PATCH 1/5] fs/stat: Define DAX statx attribute ira.weiny 2019-10-22 11:32 ` Boaz Harrosh 2019-10-22 16:51 ` Ira Weiny 2019-10-20 15:59 ` [PATCH 2/5] fs/xfs: Isolate the physical DAX flag from effective ira.weiny 2019-10-21 0:26 ` Dave Chinner 2019-10-21 17:40 ` Ira Weiny 2019-10-20 15:59 ` [PATCH 3/5] fs/xfs: Separate functionality of xfs_inode_supports_dax() ira.weiny 2019-10-20 15:59 ` [PATCH 4/5] fs/xfs: Clean up DAX support check ira.weiny 2019-10-20 15:59 ` [PATCH 5/5] fs/xfs: Allow toggle of physical DAX flag ira.weiny 2019-10-21 0:45 ` Dave Chinner 2019-10-21 22:49 ` Ira Weiny 2019-10-21 23:46 ` Dave Chinner [this message] 2019-11-08 13:12 ` Jan Kara 2019-11-08 13:46 ` Jan Kara 2019-11-08 19:36 ` Ira Weiny 2019-11-11 16:07 ` Jan Kara 2019-11-11 23:54 ` Ira Weiny 2019-10-22 11:21 ` [PATCH 0/5] Enable per-file/directory DAX operations Boaz Harrosh 2019-10-23 13:09 ` Boaz Harrosh 2019-10-23 22:13 ` Dave Chinner 2019-10-24 2:31 ` Boaz Harrosh 2019-10-24 7:34 ` Dave Chinner 2019-10-24 14:05 ` Boaz Harrosh 2019-10-24 21:35 ` Dave Chinner 2019-10-24 23:29 ` Boaz Harrosh 2019-10-25 0:36 ` Dave Chinner 2019-10-25 1:15 ` Boaz Harrosh 2019-10-25 20:49 ` Ira Weiny 2019-10-27 22:10 ` Dave Chinner 2019-10-31 16:17 ` Ira Weiny 2019-11-01 22:47 ` Dave Chinner 2019-11-02 4:25 ` Dan Williams
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191021234604.GB2681@dread.disaster.area \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH 5/5] fs/xfs: Allow toggle of physical DAX flag' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.