| Message ID | 150732933283.22363.570426117546397495.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 06, 2017 at 03:35:32PM -0700, Dan Williams wrote: > When ->iomap_begin() sees this count being non-zero and determines that > the block map of the file needs to be modified to satisfy the I/O > request it will instead return an error. This is needed for MAP_DIRECT > where, due to locking constraints, we can't rely on xfs_break_layouts() > to protect against allocating write-faults either from the process that > setup the MAP_DIRECT mapping nor other processes that have the file > mapped. xfs_break_layouts() requires XFS_IOLOCK which is problematic to > mix with the XFS_MMAPLOCK in the fault path. > > Cc: Jan Kara <jack@suse.cz> > Cc: Jeff Moyer <jmoyer@redhat.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Dave Chinner <david@fromorbit.com> > Cc: "Darrick J. Wong" <darrick.wong@oracle.com> > Cc: Ross Zwisler <ross.zwisler@linux.intel.com> > Cc: Jeff Layton <jlayton@poochiereds.net> > Cc: "J. Bruce Fields" <bfields@fieldses.org> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > fs/xfs/xfs_iomap.c | 9 +++++++++ > include/linux/fs.h | 31 +++++++++++++++++++++++++++++++ > 2 files changed, 40 insertions(+) > > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c > index a1909bc064e9..6816f8ebbdcf 100644 > --- a/fs/xfs/xfs_iomap.c > +++ b/fs/xfs/xfs_iomap.c > @@ -1053,6 +1053,15 @@ xfs_file_iomap_begin( > goto out_unlock; > } > /* > + * If a file has MAP_DIRECT mappings disable block map > + * updates. This should only effect mmap write faults as > + * other paths are protected by an FL_LAYOUT lease. > + */ > + if (i_mapdcount_read(inode)) { > + error = -ETXTBSY; > + goto out_unlock; > + } That looks really fragile. For one, it's going to miss modifications to reflinked files altogether. Ignoring that, however, I don't want to have to care one bit about the internals of the MAP_DIRECT implementation in the filesystem code. Hide it behind something with an obvious name that returns the appropriate error and the filesystem code becomes self documenting: if ((flags & IOMAP_WRITE) && imap_needs_alloc(inode, &imap, nimaps)) { ..... error = iomap_can_allocate(inode); if (error) goto out_unlock; Then you can put all the MAP_DIRECT stuff and the comments explaining what is does inside the generic function that determines if we are allowed to allocate on that inode or not. > + /* > * We cap the maximum length we map here to MAX_WRITEBACK_PAGES > * pages to keep the chunks of work done where somewhat symmetric > * with the work writeback does. This is a completely arbitrary > diff --git a/include/linux/fs.h b/include/linux/fs.h > index c2b9bf3dc4e9..f83871b188ff 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -642,6 +642,9 @@ struct inode { > atomic_t i_count; > atomic_t i_dio_count; > atomic_t i_writecount; > +#ifdef CONFIG_FS_DAX > + atomic_t i_mapdcount; /* count of MAP_DIRECT vmas */ > +#endif Is there any way to avoid growing the struct inode for this? Cheers, Dave.
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c index a1909bc064e9..6816f8ebbdcf 100644 --- a/fs/xfs/xfs_iomap.c +++ b/fs/xfs/xfs_iomap.c @@ -1053,6 +1053,15 @@ xfs_file_iomap_begin( goto out_unlock; } /* + * If a file has MAP_DIRECT mappings disable block map + * updates. This should only effect mmap write faults as + * other paths are protected by an FL_LAYOUT lease. + */ + if (i_mapdcount_read(inode)) { + error = -ETXTBSY; + goto out_unlock; + } + /* * We cap the maximum length we map here to MAX_WRITEBACK_PAGES * pages to keep the chunks of work done where somewhat symmetric * with the work writeback does. This is a completely arbitrary diff --git a/include/linux/fs.h b/include/linux/fs.h index c2b9bf3dc4e9..f83871b188ff 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -642,6 +642,9 @@ struct inode { atomic_t i_count; atomic_t i_dio_count; atomic_t i_writecount; +#ifdef CONFIG_FS_DAX + atomic_t i_mapdcount; /* count of MAP_DIRECT vmas */ +#endif #ifdef CONFIG_IMA atomic_t i_readcount; /* struct files open RO */ #endif @@ -2784,6 +2787,34 @@ static inline void i_readcount_inc(struct inode *inode) return; } #endif + +#ifdef CONFIG_FS_DAX +static inline void i_mapdcount_dec(struct inode *inode) +{ + BUG_ON(!atomic_read(&inode->i_mapdcount)); + atomic_dec(&inode->i_mapdcount); +} +static inline void i_mapdcount_inc(struct inode *inode) +{ + atomic_inc(&inode->i_mapdcount); +} +static inline int i_mapdcount_read(struct inode *inode) +{ + return atomic_read(&inode->i_mapdcount); +} +#else +static inline void i_mapdcount_dec(struct inode *inode) +{ +} +static inline void i_mapdcount_inc(struct inode *inode) +{ +} +static inline int i_mapdcount_read(struct inode *inode) +{ + return 0; +} +#endif + extern int do_pipe_flags(int *, int); #define __kernel_read_file_id(id) \
When ->iomap_begin() sees this count being non-zero and determines that the block map of the file needs to be modified to satisfy the I/O request it will instead return an error. This is needed for MAP_DIRECT where, due to locking constraints, we can't rely on xfs_break_layouts() to protect against allocating write-faults either from the process that setup the MAP_DIRECT mapping nor other processes that have the file mapped. xfs_break_layouts() requires XFS_IOLOCK which is problematic to mix with the XFS_MMAPLOCK in the fault path. Cc: Jan Kara <jack@suse.cz> Cc: Jeff Moyer <jmoyer@redhat.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Dave Chinner <david@fromorbit.com> Cc: "Darrick J. Wong" <darrick.wong@oracle.com> Cc: Ross Zwisler <ross.zwisler@linux.intel.com> Cc: Jeff Layton <jlayton@poochiereds.net> Cc: "J. Bruce Fields" <bfields@fieldses.org> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- fs/xfs/xfs_iomap.c | 9 +++++++++ include/linux/fs.h | 31 +++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+) -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html