[PATCH 1/3] xfs: refactor setflags to use setattr code directly

[PATCH 1/3] xfs: refactor setflags to use setattr code directly

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Refactor the SETFLAGS implementation to use the SETXATTR code directly
instead of partially constructing a struct fsxattr and calling bits and
pieces of the setxattr code.  This reduces code size with no functional
change.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_ioctl.c |   48 +++---------------------------------------------
 1 file changed, 3 insertions(+), 45 deletions(-)


diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 723550c8a2e4..7a9076867a30 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1491,11 +1491,8 @@ xfs_ioc_setxflags(
 	struct file		*filp,
 	void			__user *arg)
 {
-	struct xfs_trans	*tp;
 	struct fsxattr		fa;
-	struct fsxattr		old_fa;
 	unsigned int		flags;
-	int			join_flags = 0;
 	int			error;
 
 	if (copy_from_user(&flags, arg, sizeof(flags)))
@@ -1506,52 +1503,13 @@ xfs_ioc_setxflags(
 		      FS_SYNC_FL))
 		return -EOPNOTSUPP;
 
-	fa.fsx_xflags = xfs_merge_ioc_xflags(flags, xfs_ip2xflags(ip));
+	xfs_fill_fsxattr(ip, false, &fa);
+	fa.fsx_xflags = xfs_merge_ioc_xflags(flags, fa.fsx_xflags);
 
 	error = mnt_want_write_file(filp);
 	if (error)
 		return error;
-
-	error = xfs_ioctl_setattr_drain_writes(ip, &fa, &join_flags);
-	if (error) {
-		xfs_iunlock(ip, join_flags);
-		goto out_drop_write;
-	}
-
-	/*
-	 * Changing DAX config may require inode locking for mapping
-	 * invalidation. These need to be held all the way to transaction commit
-	 * or cancel time, so need to be passed through to
-	 * xfs_ioctl_setattr_get_trans() so it can apply them to the join call
-	 * appropriately.
-	 */
-	error = xfs_ioctl_setattr_dax_invalidate(ip, &fa, &join_flags);
-	if (error) {
-		xfs_iunlock(ip, join_flags);
-		goto out_drop_write;
-	}
-
-	tp = xfs_ioctl_setattr_get_trans(ip, join_flags);
-	if (IS_ERR(tp)) {
-		error = PTR_ERR(tp);
-		goto out_drop_write;
-	}
-
-	xfs_fill_fsxattr(ip, false, &old_fa);
-	error = vfs_ioc_fssetxattr_check(VFS_I(ip), &old_fa, &fa);
-	if (error) {
-		xfs_trans_cancel(tp);
-		goto out_drop_write;
-	}
-
-	error = xfs_ioctl_setattr_xflags(tp, ip, &fa);
-	if (error) {
-		xfs_trans_cancel(tp);
-		goto out_drop_write;
-	}
-
-	error = xfs_trans_commit(tp);
-out_drop_write:
+	error = xfs_ioctl_setattr(ip, &fa);
 	mnt_drop_write_file(filp);
 	return error;
 }

[PATCH v2 0/3] xfs: further FSSETXATTR cleanups

[Darrick J. Wong]

Hi all,

Now that we've refactored a lot of the FSSETXATTR parameter checking
code into generic functions, shorten the XFS FS_IOC_[GS]ETFLAGS
implementations by making them wrappers around FS_IOC_FS[GS]ETXATTR.
Finally, kill all the useless support code that prepared the memory
manager to change the S_DAX flag since none of it works anyway.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This has been lightly tested with fstests.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=setxattr-cleanups

[PATCH 1/3] xfs: refactor setflags to use setattr code directly

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Refactor the SETFLAGS implementation to use the SETXATTR code directly
instead of partially constructing a struct fsxattr and calling bits and
pieces of the setxattr code.  This reduces code size with no functional
change.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_ioctl.c |   48 +++---------------------------------------------
 1 file changed, 3 insertions(+), 45 deletions(-)


diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 552f18554c48..6f55cd7eb34f 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1490,11 +1490,8 @@ xfs_ioc_setxflags(
 	struct file		*filp,
 	void			__user *arg)
 {
-	struct xfs_trans	*tp;
 	struct fsxattr		fa;
-	struct fsxattr		old_fa;
 	unsigned int		flags;
-	int			join_flags = 0;
 	int			error;
 
 	if (copy_from_user(&flags, arg, sizeof(flags)))
@@ -1505,52 +1502,13 @@ xfs_ioc_setxflags(
 		      FS_SYNC_FL))
 		return -EOPNOTSUPP;
 
-	fa.fsx_xflags = xfs_merge_ioc_xflags(flags, xfs_ip2xflags(ip));
+	xfs_fill_fsxattr(ip, false, &fa);
+	fa.fsx_xflags = xfs_merge_ioc_xflags(flags, fa.fsx_xflags);
 
 	error = mnt_want_write_file(filp);
 	if (error)
 		return error;
-
-	error = xfs_ioctl_setattr_drain_writes(ip, &fa, &join_flags);
-	if (error) {
-		xfs_iunlock(ip, join_flags);
-		goto out_drop_write;
-	}
-
-	/*
-	 * Changing DAX config may require inode locking for mapping
-	 * invalidation. These need to be held all the way to transaction commit
-	 * or cancel time, so need to be passed through to
-	 * xfs_ioctl_setattr_get_trans() so it can apply them to the join call
-	 * appropriately.
-	 */
-	error = xfs_ioctl_setattr_dax_invalidate(ip, &fa, &join_flags);
-	if (error) {
-		xfs_iunlock(ip, join_flags);
-		goto out_drop_write;
-	}
-
-	tp = xfs_ioctl_setattr_get_trans(ip, join_flags);
-	if (IS_ERR(tp)) {
-		error = PTR_ERR(tp);
-		goto out_drop_write;
-	}
-
-	xfs_fill_fsxattr(ip, false, &old_fa);
-	error = vfs_ioc_fssetxattr_check(VFS_I(ip), &old_fa, &fa);
-	if (error) {
-		xfs_trans_cancel(tp);
-		goto out_drop_write;
-	}
-
-	error = xfs_ioctl_setattr_xflags(tp, ip, &fa);
-	if (error) {
-		xfs_trans_cancel(tp);
-		goto out_drop_write;
-	}
-
-	error = xfs_trans_commit(tp);
-out_drop_write:
+	error = xfs_ioctl_setattr(ip, &fa);
 	mnt_drop_write_file(filp);
 	return error;
 }

[PATCH 2/3] xfs: clean up xfs_merge_ioc_xflags

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Clean up the calling convention since we're editing the fsxattr struct
anyway.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_ioctl.c |   32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)


diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 6f55cd7eb34f..d2526d9070d2 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -829,35 +829,31 @@ xfs_ioc_ag_geometry(
  * Linux extended inode flags interface.
  */
 
-STATIC unsigned int
+static inline void
 xfs_merge_ioc_xflags(
-	unsigned int	flags,
-	unsigned int	start)
+	struct fsxattr	*fa,
+	unsigned int	flags)
 {
-	unsigned int	xflags = start;
-
 	if (flags & FS_IMMUTABLE_FL)
-		xflags |= FS_XFLAG_IMMUTABLE;
+		fa->fsx_xflags |= FS_XFLAG_IMMUTABLE;
 	else
-		xflags &= ~FS_XFLAG_IMMUTABLE;
+		fa->fsx_xflags &= ~FS_XFLAG_IMMUTABLE;
 	if (flags & FS_APPEND_FL)
-		xflags |= FS_XFLAG_APPEND;
+		fa->fsx_xflags |= FS_XFLAG_APPEND;
 	else
-		xflags &= ~FS_XFLAG_APPEND;
+		fa->fsx_xflags &= ~FS_XFLAG_APPEND;
 	if (flags & FS_SYNC_FL)
-		xflags |= FS_XFLAG_SYNC;
+		fa->fsx_xflags |= FS_XFLAG_SYNC;
 	else
-		xflags &= ~FS_XFLAG_SYNC;
+		fa->fsx_xflags &= ~FS_XFLAG_SYNC;
 	if (flags & FS_NOATIME_FL)
-		xflags |= FS_XFLAG_NOATIME;
+		fa->fsx_xflags |= FS_XFLAG_NOATIME;
 	else
-		xflags &= ~FS_XFLAG_NOATIME;
+		fa->fsx_xflags &= ~FS_XFLAG_NOATIME;
 	if (flags & FS_NODUMP_FL)
-		xflags |= FS_XFLAG_NODUMP;
+		fa->fsx_xflags |= FS_XFLAG_NODUMP;
 	else
-		xflags &= ~FS_XFLAG_NODUMP;
-
-	return xflags;
+		fa->fsx_xflags &= ~FS_XFLAG_NODUMP;
 }
 
 STATIC unsigned int
@@ -1503,7 +1499,7 @@ xfs_ioc_setxflags(
 		return -EOPNOTSUPP;
 
 	xfs_fill_fsxattr(ip, false, &fa);
-	fa.fsx_xflags = xfs_merge_ioc_xflags(flags, fa.fsx_xflags);
+	xfs_merge_ioc_xflags(&fa, flags);
 
 	error = mnt_want_write_file(filp);
 	if (error)

[PATCH 3/3] xfs: make the dax inode flag advisory

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

We have no ability to change S_DAX on an inode, which means that this
flag is purely advisory.  Remove all the (broken) code that tried to
change the state since it's cluttering up the code for no good reason.
If the kernel ever gains the ability to change S_DAX on the fly we can
always add this back.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_ioctl.c |   76 ----------------------------------------------------
 1 file changed, 76 deletions(-)


diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index d2526d9070d2..dda681698555 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1001,12 +1001,6 @@ 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
 }
 
 static int
@@ -1077,63 +1071,6 @@ xfs_ioctl_setattr_drain_writes(
 	return inode_drain_writes(inode);
 }
 
-/*
- * If we are changing DAX flags, we have to ensure the file is clean and any
- * cached objects in the address space are invalidated and removed. This
- * requires us to lock out other IO and page faults similar to a truncate
- * operation. The locks need to be held until the transaction has been committed
- * so that the cache invalidation is atomic with respect to the DAX flag
- * manipulation.
- *
- * The caller must use @join_flags to release the locks which are held on @ip
- * regardless of return value.
- */
-static int
-xfs_ioctl_setattr_dax_invalidate(
-	struct xfs_inode	*ip,
-	struct fsxattr		*fa,
-	int			*join_flags)
-{
-	struct inode		*inode = VFS_I(ip);
-	struct super_block	*sb = inode->i_sb;
-	int			error;
-
-	/*
-	 * It is only valid to set the DAX flag on regular files and
-	 * directories on filesystems where the block size is equal to the page
-	 * size. On directories it serves as an inherited hint so we don't
-	 * have to check the device for dax support or flush pagecache.
-	 */
-	if (fa->fsx_xflags & FS_XFLAG_DAX) {
-		if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)))
-			return -EINVAL;
-		if (S_ISREG(inode->i_mode) &&
-		    !bdev_dax_supported(xfs_find_bdev_for_inode(VFS_I(ip)),
-				sb->s_blocksize))
-			return -EINVAL;
-	}
-
-	/* If the DAX state is not changing, we have nothing to do here. */
-	if ((fa->fsx_xflags & FS_XFLAG_DAX) && IS_DAX(inode))
-		return 0;
-	if (!(fa->fsx_xflags & FS_XFLAG_DAX) && !IS_DAX(inode))
-		return 0;
-
-	if (S_ISDIR(inode->i_mode))
-		return 0;
-
-	/* lock, flush and invalidate mapping in preparation for flag change */
-	if (*join_flags == 0) {
-		*join_flags = XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL;
-		xfs_ilock(ip, *join_flags);
-		error = filemap_write_and_wait(inode->i_mapping);
-		if (error)
-			return error;
-	}
-
-	return invalidate_inode_pages2(inode->i_mapping);
-}
-
 /*
  * Set up the transaction structure for the setattr operation, checking that we
  * have permission to do so. On success, return a clean transaction and the
@@ -1346,19 +1283,6 @@ xfs_ioctl_setattr(
 		goto error_free_dquots;
 	}
 
-	/*
-	 * Changing DAX config may require inode locking for mapping
-	 * invalidation. These need to be held all the way to transaction commit
-	 * or cancel time, so need to be passed through to
-	 * xfs_ioctl_setattr_get_trans() so it can apply them to the join call
-	 * appropriately.
-	 */
-	code = xfs_ioctl_setattr_dax_invalidate(ip, fa, &join_flags);
-	if (code) {
-		xfs_iunlock(ip, join_flags);
-		goto error_free_dquots;
-	}
-
 	tp = xfs_ioctl_setattr_get_trans(ip, join_flags);
 	if (IS_ERR(tp)) {
 		code = PTR_ERR(tp);

Re: [PATCH 1/3] xfs: refactor setflags to use setattr code directly

[Allison Collins]

On 6/28/19 11:35 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Refactor the SETFLAGS implementation to use the SETXATTR code directly
> instead of partially constructing a struct fsxattr and calling bits and
> pieces of the setxattr code.  This reduces code size with no functional
> change.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/xfs_ioctl.c |   48 +++---------------------------------------------
>   1 file changed, 3 insertions(+), 45 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 552f18554c48..6f55cd7eb34f 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1490,11 +1490,8 @@ xfs_ioc_setxflags(
>   	struct file		*filp,
>   	void			__user *arg)
>   {
> -	struct xfs_trans	*tp;
>   	struct fsxattr		fa;
> -	struct fsxattr		old_fa;
>   	unsigned int		flags;
> -	int			join_flags = 0;
>   	int			error;
>   
>   	if (copy_from_user(&flags, arg, sizeof(flags)))
> @@ -1505,52 +1502,13 @@ xfs_ioc_setxflags(
>   		      FS_SYNC_FL))
>   		return -EOPNOTSUPP;
>   
> -	fa.fsx_xflags = xfs_merge_ioc_xflags(flags, xfs_ip2xflags(ip));
> +	xfs_fill_fsxattr(ip, false, &fa);

While reviewing this patch, it looks like xfs_fill_fsxattr comes in with 
a different set?  Not sure if you meant to stack them that way.  I may 
come back to this patch later if there is a dependency.  Or maybe it 
might make sense to move this patch into the set it depends on?

Allison

> +	fa.fsx_xflags = xfs_merge_ioc_xflags(flags, fa.fsx_xflags);
>   
>   	error = mnt_want_write_file(filp);
>   	if (error)
>   		return error;
> -
> -	error = xfs_ioctl_setattr_drain_writes(ip, &fa, &join_flags);
> -	if (error) {
> -		xfs_iunlock(ip, join_flags);
> -		goto out_drop_write;
> -	}
> -
> -	/*
> -	 * Changing DAX config may require inode locking for mapping
> -	 * invalidation. These need to be held all the way to transaction commit
> -	 * or cancel time, so need to be passed through to
> -	 * xfs_ioctl_setattr_get_trans() so it can apply them to the join call
> -	 * appropriately.
> -	 */
> -	error = xfs_ioctl_setattr_dax_invalidate(ip, &fa, &join_flags);
> -	if (error) {
> -		xfs_iunlock(ip, join_flags);
> -		goto out_drop_write;
> -	}
> -
> -	tp = xfs_ioctl_setattr_get_trans(ip, join_flags);
> -	if (IS_ERR(tp)) {
> -		error = PTR_ERR(tp);
> -		goto out_drop_write;
> -	}
> -
> -	xfs_fill_fsxattr(ip, false, &old_fa);
> -	error = vfs_ioc_fssetxattr_check(VFS_I(ip), &old_fa, &fa);
> -	if (error) {
> -		xfs_trans_cancel(tp);
> -		goto out_drop_write;
> -	}
> -
> -	error = xfs_ioctl_setattr_xflags(tp, ip, &fa);
> -	if (error) {
> -		xfs_trans_cancel(tp);
> -		goto out_drop_write;
> -	}
> -
> -	error = xfs_trans_commit(tp);
> -out_drop_write:
> +	error = xfs_ioctl_setattr(ip, &fa);
>   	mnt_drop_write_file(filp);
>   	return error;
>   }
> 

Re: [PATCH 2/3] xfs: clean up xfs_merge_ioc_xflags

[Allison Collins]

On 6/28/19 11:35 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Clean up the calling convention since we're editing the fsxattr struct
> anyway.
> 
This one looks ok.  You can add my review:
Reviewed-by: Allison Collins <allison.henderson@oracle.com>

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/xfs_ioctl.c |   32 ++++++++++++++------------------
>   1 file changed, 14 insertions(+), 18 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 6f55cd7eb34f..d2526d9070d2 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -829,35 +829,31 @@ xfs_ioc_ag_geometry(
>    * Linux extended inode flags interface.
>    */
>   
> -STATIC unsigned int
> +static inline void
>   xfs_merge_ioc_xflags(
> -	unsigned int	flags,
> -	unsigned int	start)
> +	struct fsxattr	*fa,
> +	unsigned int	flags)
>   {
> -	unsigned int	xflags = start;
> -
>   	if (flags & FS_IMMUTABLE_FL)
> -		xflags |= FS_XFLAG_IMMUTABLE;
> +		fa->fsx_xflags |= FS_XFLAG_IMMUTABLE;
>   	else
> -		xflags &= ~FS_XFLAG_IMMUTABLE;
> +		fa->fsx_xflags &= ~FS_XFLAG_IMMUTABLE;
>   	if (flags & FS_APPEND_FL)
> -		xflags |= FS_XFLAG_APPEND;
> +		fa->fsx_xflags |= FS_XFLAG_APPEND;
>   	else
> -		xflags &= ~FS_XFLAG_APPEND;
> +		fa->fsx_xflags &= ~FS_XFLAG_APPEND;
>   	if (flags & FS_SYNC_FL)
> -		xflags |= FS_XFLAG_SYNC;
> +		fa->fsx_xflags |= FS_XFLAG_SYNC;
>   	else
> -		xflags &= ~FS_XFLAG_SYNC;
> +		fa->fsx_xflags &= ~FS_XFLAG_SYNC;
>   	if (flags & FS_NOATIME_FL)
> -		xflags |= FS_XFLAG_NOATIME;
> +		fa->fsx_xflags |= FS_XFLAG_NOATIME;
>   	else
> -		xflags &= ~FS_XFLAG_NOATIME;
> +		fa->fsx_xflags &= ~FS_XFLAG_NOATIME;
>   	if (flags & FS_NODUMP_FL)
> -		xflags |= FS_XFLAG_NODUMP;
> +		fa->fsx_xflags |= FS_XFLAG_NODUMP;
>   	else
> -		xflags &= ~FS_XFLAG_NODUMP;
> -
> -	return xflags;
> +		fa->fsx_xflags &= ~FS_XFLAG_NODUMP;
>   }
>   
>   STATIC unsigned int
> @@ -1503,7 +1499,7 @@ xfs_ioc_setxflags(
>   		return -EOPNOTSUPP;
>   
>   	xfs_fill_fsxattr(ip, false, &fa);
> -	fa.fsx_xflags = xfs_merge_ioc_xflags(flags, fa.fsx_xflags);
> +	xfs_merge_ioc_xflags(&fa, flags);
>   
>   	error = mnt_want_write_file(filp);
>   	if (error)
> 

Re: [PATCH 3/3] xfs: make the dax inode flag advisory

[Allison Collins]

On 6/28/19 11:35 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> We have no ability to change S_DAX on an inode, which means that this
> flag is purely advisory.  Remove all the (broken) code that tried to
> change the state since it's cluttering up the code for no good reason.
> If the kernel ever gains the ability to change S_DAX on the fly we can
> always add this back.
> 
Looks ok to me.
Reviewed-by: Allison Collins <allison.henderson@oracle.com>

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/xfs_ioctl.c |   76 ----------------------------------------------------
>   1 file changed, 76 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index d2526d9070d2..dda681698555 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1001,12 +1001,6 @@ 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
>   }
>   
>   static int
> @@ -1077,63 +1071,6 @@ xfs_ioctl_setattr_drain_writes(
>   	return inode_drain_writes(inode);
>   }
>   
> -/*
> - * If we are changing DAX flags, we have to ensure the file is clean and any
> - * cached objects in the address space are invalidated and removed. This
> - * requires us to lock out other IO and page faults similar to a truncate
> - * operation. The locks need to be held until the transaction has been committed
> - * so that the cache invalidation is atomic with respect to the DAX flag
> - * manipulation.
> - *
> - * The caller must use @join_flags to release the locks which are held on @ip
> - * regardless of return value.
> - */
> -static int
> -xfs_ioctl_setattr_dax_invalidate(
> -	struct xfs_inode	*ip,
> -	struct fsxattr		*fa,
> -	int			*join_flags)
> -{
> -	struct inode		*inode = VFS_I(ip);
> -	struct super_block	*sb = inode->i_sb;
> -	int			error;
> -
> -	/*
> -	 * It is only valid to set the DAX flag on regular files and
> -	 * directories on filesystems where the block size is equal to the page
> -	 * size. On directories it serves as an inherited hint so we don't
> -	 * have to check the device for dax support or flush pagecache.
> -	 */
> -	if (fa->fsx_xflags & FS_XFLAG_DAX) {
> -		if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)))
> -			return -EINVAL;
> -		if (S_ISREG(inode->i_mode) &&
> -		    !bdev_dax_supported(xfs_find_bdev_for_inode(VFS_I(ip)),
> -				sb->s_blocksize))
> -			return -EINVAL;
> -	}
> -
> -	/* If the DAX state is not changing, we have nothing to do here. */
> -	if ((fa->fsx_xflags & FS_XFLAG_DAX) && IS_DAX(inode))
> -		return 0;
> -	if (!(fa->fsx_xflags & FS_XFLAG_DAX) && !IS_DAX(inode))
> -		return 0;
> -
> -	if (S_ISDIR(inode->i_mode))
> -		return 0;
> -
> -	/* lock, flush and invalidate mapping in preparation for flag change */
> -	if (*join_flags == 0) {
> -		*join_flags = XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL;
> -		xfs_ilock(ip, *join_flags);
> -		error = filemap_write_and_wait(inode->i_mapping);
> -		if (error)
> -			return error;
> -	}
> -
> -	return invalidate_inode_pages2(inode->i_mapping);
> -}
> -
>   /*
>    * Set up the transaction structure for the setattr operation, checking that we
>    * have permission to do so. On success, return a clean transaction and the
> @@ -1346,19 +1283,6 @@ xfs_ioctl_setattr(
>   		goto error_free_dquots;
>   	}
>   
> -	/*
> -	 * Changing DAX config may require inode locking for mapping
> -	 * invalidation. These need to be held all the way to transaction commit
> -	 * or cancel time, so need to be passed through to
> -	 * xfs_ioctl_setattr_get_trans() so it can apply them to the join call
> -	 * appropriately.
> -	 */
> -	code = xfs_ioctl_setattr_dax_invalidate(ip, fa, &join_flags);
> -	if (code) {
> -		xfs_iunlock(ip, join_flags);
> -		goto error_free_dquots;
> -	}
> -
>   	tp = xfs_ioctl_setattr_get_trans(ip, join_flags);
>   	if (IS_ERR(tp)) {
>   		code = PTR_ERR(tp);
> 

Re: [PATCH 1/3] xfs: refactor setflags to use setattr code directly

[Darrick J. Wong]

On Sun, Jul 07, 2019 at 03:29:42PM -0700, Allison Collins wrote:
> 
> 
> On 6/28/19 11:35 AM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Refactor the SETFLAGS implementation to use the SETXATTR code directly
> > instead of partially constructing a struct fsxattr and calling bits and
> > pieces of the setxattr code.  This reduces code size with no functional
> > change.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >   fs/xfs/xfs_ioctl.c |   48 +++---------------------------------------------
> >   1 file changed, 3 insertions(+), 45 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 552f18554c48..6f55cd7eb34f 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1490,11 +1490,8 @@ xfs_ioc_setxflags(
> >   	struct file		*filp,
> >   	void			__user *arg)
> >   {
> > -	struct xfs_trans	*tp;
> >   	struct fsxattr		fa;
> > -	struct fsxattr		old_fa;
> >   	unsigned int		flags;
> > -	int			join_flags = 0;
> >   	int			error;
> >   	if (copy_from_user(&flags, arg, sizeof(flags)))
> > @@ -1505,52 +1502,13 @@ xfs_ioc_setxflags(
> >   		      FS_SYNC_FL))
> >   		return -EOPNOTSUPP;
> > -	fa.fsx_xflags = xfs_merge_ioc_xflags(flags, xfs_ip2xflags(ip));
> > +	xfs_fill_fsxattr(ip, false, &fa);
> 
> While reviewing this patch, it looks like xfs_fill_fsxattr comes in with a
> different set?  Not sure if you meant to stack them that way.  I may come
> back to this patch later if there is a dependency.  Or maybe it might make
> sense to move this patch into the set it depends on?

This series depends on the two that were posted immediately before it,
though I admit the cover letters don't really make that explicit.

--D

> Allison
> 
> > +	fa.fsx_xflags = xfs_merge_ioc_xflags(flags, fa.fsx_xflags);
> >   	error = mnt_want_write_file(filp);
> >   	if (error)
> >   		return error;
> > -
> > -	error = xfs_ioctl_setattr_drain_writes(ip, &fa, &join_flags);
> > -	if (error) {
> > -		xfs_iunlock(ip, join_flags);
> > -		goto out_drop_write;
> > -	}
> > -
> > -	/*
> > -	 * Changing DAX config may require inode locking for mapping
> > -	 * invalidation. These need to be held all the way to transaction commit
> > -	 * or cancel time, so need to be passed through to
> > -	 * xfs_ioctl_setattr_get_trans() so it can apply them to the join call
> > -	 * appropriately.
> > -	 */
> > -	error = xfs_ioctl_setattr_dax_invalidate(ip, &fa, &join_flags);
> > -	if (error) {
> > -		xfs_iunlock(ip, join_flags);
> > -		goto out_drop_write;
> > -	}
> > -
> > -	tp = xfs_ioctl_setattr_get_trans(ip, join_flags);
> > -	if (IS_ERR(tp)) {
> > -		error = PTR_ERR(tp);
> > -		goto out_drop_write;
> > -	}
> > -
> > -	xfs_fill_fsxattr(ip, false, &old_fa);
> > -	error = vfs_ioc_fssetxattr_check(VFS_I(ip), &old_fa, &fa);
> > -	if (error) {
> > -		xfs_trans_cancel(tp);
> > -		goto out_drop_write;
> > -	}
> > -
> > -	error = xfs_ioctl_setattr_xflags(tp, ip, &fa);
> > -	if (error) {
> > -		xfs_trans_cancel(tp);
> > -		goto out_drop_write;
> > -	}
> > -
> > -	error = xfs_trans_commit(tp);
> > -out_drop_write:
> > +	error = xfs_ioctl_setattr(ip, &fa);
> >   	mnt_drop_write_file(filp);
> >   	return error;
> >   }
> >