diff mbox series

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

Message ID 156174693300.1557952.1660572699951099381.stgit@magnolia (mailing list archive)
State New, archived
Headers show
Series xfs: further FSSETXATTR cleanups | expand

Commit Message

Darrick J. Wong June 28, 2019, 6:35 p.m. UTC 
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(-)

Comments

Allison Henderson July 7, 2019, 10:29 p.m. UTC | #1 
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;
>   }
>
Darrick J. Wong July 8, 2019, 4:41 p.m. UTC | #2 
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;
> >   }
> >
diff mbox series

Patch

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;
 }