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 <[email protected]>
> Cc: Jeff Moyer <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Dave Chinner <[email protected]>
> Cc: "Darrick J. Wong" <[email protected]>
> Cc: Ross Zwisler <[email protected]>
> Cc: Jeff Layton <[email protected]>
> Cc: "J. Bruce Fields" <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> 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.
--
Dave Chinner
[email protected]
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
