Re: [PATCH v5 10/11] xfs: prepare xfs_break_layouts() for another layout type

[Christoph Hellwig]

>  
> +int
> +xfs_break_layouts(
> + struct inode*inode,
> + uint*iolock,
> + unsigned long   flags)
> +{
> + struct xfs_inode*ip = XFS_I(inode);
> + uintiolock_assert = 0;
> + int ret = 0;
> +
> + if (flags & XFS_BREAK_REMOTE)
> + iolock_assert |= XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL;
> + if (flags & XFS_BREAK_MAPS)
> + iolock_assert |= XFS_MMAPLOCK_EXCL;
> +
> + ASSERT(xfs_isilocked(ip, iolock_assert));
> +
> + if (flags & XFS_BREAK_REMOTE)
> + ret = xfs_break_leased_layouts(inode, iolock);
> + return ret;

This just looks weird as hell.  We already pass in what to drop/reacquire
in the iolock argument.  I don't think we need another argument controlled
by the same callers to assert it.

> @@ -768,7 +790,7 @@ xfs_file_fallocate(
>   struct xfs_inode*ip = XFS_I(inode);
>   longerror;
>   enum xfs_prealloc_flags flags = 0;
> - uintiolock = XFS_IOLOCK_EXCL;
> + uintiolock = XFS_IOLOCK_EXCL|XFS_MMAPLOCK_EXCL;

This is a behavior change that should not be in a patch titled
"prepare xfs_break_layouts() for another layout type" but in one
explicitly changing this and documenting why.

In summary:  I think this should be replaced with a patch that
allows xfs_break_layouts to be called with the mmap lock held, and
change the callers that want the mmap lock to pass it with a good
explanation, and we should get rid of the XFS_BREAK_* flags here.
(need to check the next patch if there is any other good reason for
them to be added later, though).

[PATCH v5 10/11] xfs: prepare xfs_break_layouts() for another layout type

[Dan Williams]

When xfs is operating as the back-end of a pNFS block server, it prevents
collisions between local and remote operations by requiring a lease to
be held for remotely accessed blocks. Local filesystem operations break
those leases before writing or mutating the extent map of the file.

A similar mechanism is needed to prevent operations on pinned dax
mappings, like device-DMA, from colliding with extent unmap operations.

XFS_BREAK_REMOTE and XFS_BREAK_MAPS are introduced as flags to control
the layouts that need to be broken by xfs_break_layouts(). While
XFS_BREAK_REMOTE is invoked in all calls to the new xfs_break_layouts(),
XFS_BREAK_MAPS only needs to specified when extents may be unmapped,
i.e. xfs_file_fallocate() and xfs_ioc_space(). XFS_BREAK_MAPS also
imposes the additional locking constraint of breaking (awaiting) pinned
dax mappings while holding XFS_MMAPLOCK_EXCL.

There is a small functional change in this rework. For the cases where
XFS_BREAK_MAPS is specified to xfs_break_layouts(), the
XFS_MMAPLOCK_EXCL ilock is held over the break_layouts() loop in
xfs_break_leased_layouts().

Cc: "Darrick J. Wong" 
Cc: Ross Zwisler 
Reported-by: Dave Chinner 
Reported-by: Christoph Hellwig 
Signed-off-by: Dan Williams 
---
 fs/xfs/xfs_file.c  |   32 ++--
 fs/xfs/xfs_inode.h |9 +
 fs/xfs/xfs_ioctl.c |9 +++--
 fs/xfs/xfs_iops.c  |   12 +++-
 fs/xfs/xfs_pnfs.c  |8 +++-
 fs/xfs/xfs_pnfs.h  |4 ++--
 6 files changed, 50 insertions(+), 24 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 9ea08326f876..f914f0628dc2 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -350,7 +350,7 @@ xfs_file_aio_write_checks(
if (error <= 0)
return error;
 
-   error = xfs_break_layouts(inode, iolock);
+   error = xfs_break_layouts(inode, iolock, XFS_BREAK_REMOTE);
if (error)
return error;
 
@@ -752,6 +752,28 @@ xfs_file_write_iter(
return ret;
 }
 
+int
+xfs_break_layouts(
+   struct inode*inode,
+   uint*iolock,
+   unsigned long   flags)
+{
+   struct xfs_inode*ip = XFS_I(inode);
+   uintiolock_assert = 0;
+   int ret = 0;
+
+   if (flags & XFS_BREAK_REMOTE)
+   iolock_assert |= XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL;
+   if (flags & XFS_BREAK_MAPS)
+   iolock_assert |= XFS_MMAPLOCK_EXCL;
+
+   ASSERT(xfs_isilocked(ip, iolock_assert));
+
+   if (flags & XFS_BREAK_REMOTE)
+   ret = xfs_break_leased_layouts(inode, iolock);
+   return ret;
+}
+
 #defineXFS_FALLOC_FL_SUPPORTED 
\
(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE |   \
 FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_ZERO_RANGE |  \
@@ -768,7 +790,7 @@ xfs_file_fallocate(
struct xfs_inode*ip = XFS_I(inode);
longerror;
enum xfs_prealloc_flags flags = 0;
-   uintiolock = XFS_IOLOCK_EXCL;
+   uintiolock = XFS_IOLOCK_EXCL|XFS_MMAPLOCK_EXCL;
loff_t  new_size = 0;
booldo_file_insert = false;
 
@@ -778,13 +800,11 @@ xfs_file_fallocate(
return -EOPNOTSUPP;
 
xfs_ilock(ip, iolock);
-   error = xfs_break_layouts(inode, &iolock);
+   error = xfs_break_layouts(inode, &iolock,
+   XFS_BREAK_REMOTE | XFS_BREAK_MAPS);
if (error)
goto out_unlock;
 
-   xfs_ilock(ip, XFS_MMAPLOCK_EXCL);
-   iolock |= XFS_MMAPLOCK_EXCL;
-
if (mode & FALLOC_FL_PUNCH_HOLE) {
error = xfs_free_file_space(ip, offset, len);
if (error)
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 3e8dc990d41c..9b73ceb09cb1 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -379,6 +379,12 @@ static inline void xfs_ifunlock(struct xfs_inode *ip)
>> XFS_ILOCK_SHIFT)
 
 /*
+ * Flags for layout breaks
+ */
+#define XFS_BREAK_REMOTE (1<<0) /* break remote layout leases */
+#define XFS_BREAK_MAPS   (1<<1) /* break local direct (dax) mappings */
+
+/*
  * For multiple groups support: if S_ISGID bit is set in the parent
  * directory, group of new file is set to that of the parent, and
  * new subdirectory gets S_ISGID bit from parent.
@@ -447,6 +453,9 @@ int xfs_zero_eof(struct xfs_inode *ip, xfs_off_t offset,
 xfs_fsize_t isize, bool *did_zeroing);
 intxfs_zero_range(struct xfs_inode *ip, xfs_off_t pos, xfs_off_t count,
bool *did_zero);
+intxfs_break_layouts(struct inode *inode, uint *iolock,
+   unsigned long flags);
+
 
 /* from xfs_iops.c */
 extern void xfs_setup_inode(struct xfs_inode *ip);
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index