From: Jan Kara <email@example.com> To: Ira Weiny <firstname.lastname@example.org> Cc: Dave Chinner <email@example.com>, 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: Fri, 8 Nov 2019 14:12:38 +0100 [thread overview] Message-ID: <20191108131238.GK20863@quack2.suse.cz> (raw) In-Reply-To: <20191021224931.GA25526@iweiny-DESK2.sc.intel.com> On Mon 21-10-19 15:49:31, 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: > > 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. :-( Just as a datapoint, ext4 is bold and sets inode->i_mapping->a_ops on existing inodes when switching journal data flag and so far it has not blown up. What we did to deal with issues Dave describes is that we introduced percpu rw-semaphore guarding switching of aops and then inside problematic functions redirect callbacks in the right direction under this semaphore. Somewhat ugly but it seems to work. Honza -- Jan Kara <email@example.com> SUSE Labs, CR
next prev parent reply other threads:[~2019-11-08 13:12 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 2019-11-08 13:12 ` Jan Kara [this message] 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=20191108131238.GK20863@quack2.suse.cz \ --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.