Re: [PATCH 15/29] xfs: replace ATTR_KERNOTIME with XFS_DA_OP_NOTIME

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-xfs@vger.kernel.org,
	Allison Collins <allison.henderson@oracle.com>
Subject: Re: [PATCH 15/29] xfs: replace ATTR_KERNOTIME with XFS_DA_OP_NOTIME
Date: Tue, 21 Jan 2020 10:20:51 -0800	[thread overview]
Message-ID: <20200121182051.GO8247@magnolia> (raw)
In-Reply-To: <20200114081051.297488-16-hch@lst.de>

On Tue, Jan 14, 2020 at 09:10:37AM +0100, Christoph Hellwig wrote:
> op_flags with the XFS_DA_OP_* flags is the usual place for in-kernel
> only flags, so move the notime flag there.

Whenever we're done removing the kernel-internal flags from
xfs_da_args.flags, I think it's worth amending the comments for @flags
and @op_flags noting that the first one is for argument flags from
userspace and the second one is for internal flags.

Otherwise this looks good,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_attr.c  | 4 ++--
>  fs/xfs/libxfs/xfs_attr.h  | 8 +-------
>  fs/xfs/libxfs/xfs_types.h | 2 ++
>  fs/xfs/scrub/attr.c       | 2 +-
>  fs/xfs/xfs_ioctl.c        | 1 -
>  5 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index c362dbab1a6a..fce4fd4a3370 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -186,7 +186,7 @@ xfs_attr_try_sf_addname(
>  	 * Commit the shortform mods, and we're done.
>  	 * NOTE: this is also the error path (EEXIST, etc).
>  	 */
> -	if (!error && (args->flags & ATTR_KERNOTIME) == 0)
> +	if (!error && !(args->op_flags & XFS_DA_OP_NOTIME))
>  		xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
>  
>  	if (mp->m_flags & XFS_MOUNT_WSYNC)
> @@ -385,7 +385,7 @@ xfs_attr_set(
>  	if (mp->m_flags & XFS_MOUNT_WSYNC)
>  		xfs_trans_set_sync(args->trans);
>  
> -	if ((args->flags & ATTR_KERNOTIME) == 0)
> +	if (!(args->op_flags & XFS_DA_OP_NOTIME))
>  		xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
>  
>  	/*
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index a6de050675c9..0f369399effd 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -33,19 +33,13 @@ struct xfs_attr_list_context;
>  #define ATTR_CREATE	0x0010	/* pure create: fail if attr already exists */
>  #define ATTR_REPLACE	0x0020	/* pure set: fail if attr does not exist */
>  
> -#define ATTR_KERNOTIME	0x1000	/* [kernel] don't update inode timestamps */
> -
> -#define ATTR_KERNEL_FLAGS \
> -	(ATTR_KERNOTIME)
> -
>  #define XFS_ATTR_FLAGS \
>  	{ ATTR_DONTFOLLOW, 	"DONTFOLLOW" }, \
>  	{ ATTR_ROOT,		"ROOT" }, \
>  	{ ATTR_TRUST,		"TRUST" }, \
>  	{ ATTR_SECURE,		"SECURE" }, \
>  	{ ATTR_CREATE,		"CREATE" }, \
> -	{ ATTR_REPLACE,		"REPLACE" }, \
> -	{ ATTR_KERNOTIME,	"KERNOTIME" }
> +	{ ATTR_REPLACE,		"REPLACE" }
>  
>  /*
>   * The maximum size (into the kernel or returned from the kernel) of an
> diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
> index 3379ebc0c7c5..1594325d7742 100644
> --- a/fs/xfs/libxfs/xfs_types.h
> +++ b/fs/xfs/libxfs/xfs_types.h
> @@ -223,6 +223,7 @@ typedef struct xfs_da_args {
>  #define XFS_DA_OP_ADDNAME	0x0004	/* this is an add operation */
>  #define XFS_DA_OP_OKNOENT	0x0008	/* lookup/add op, ENOENT ok, else die */
>  #define XFS_DA_OP_CILOOKUP	0x0010	/* lookup to return CI name if found */
> +#define XFS_DA_OP_NOTIME	0x0020	/* don't update inode timestamps */
>  #define XFS_DA_OP_INCOMPLETE	0x0040	/* lookup INCOMPLETE attr keys */
>  
>  #define XFS_DA_OP_FLAGS \
> @@ -231,6 +232,7 @@ typedef struct xfs_da_args {
>  	{ XFS_DA_OP_ADDNAME,	"ADDNAME" }, \
>  	{ XFS_DA_OP_OKNOENT,	"OKNOENT" }, \
>  	{ XFS_DA_OP_CILOOKUP,	"CILOOKUP" }, \
> +	{ XFS_DA_OP_NOTIME,	"NOTIME" }, \
>  	{ XFS_DA_OP_INCOMPLETE,	"INCOMPLETE" }
>  
>  /*
> diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
> index f983c2b969e0..05537627211d 100644
> --- a/fs/xfs/scrub/attr.c
> +++ b/fs/xfs/scrub/attr.c
> @@ -147,7 +147,7 @@ xchk_xattr_listent(
>  		return;
>  	}
>  
> -	args.flags = ATTR_KERNOTIME;
> +	args.op_flags = XFS_DA_OP_NOTIME;
>  	if (flags & XFS_ATTR_ROOT)
>  		args.flags |= ATTR_ROOT;
>  	else if (flags & XFS_ATTR_SECURE)
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 75b8fa7da1c9..9306cefa9e61 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -434,7 +434,6 @@ xfs_ioc_attrmulti_one(
>  
>  	if ((flags & ATTR_ROOT) && (flags & ATTR_SECURE))
>  		return -EINVAL;
> -	flags &= ~ATTR_KERNEL_FLAGS;
>  
>  	name = strndup_user(uname, MAXNAMELEN);
>  	if (IS_ERR(name))
> -- 
> 2.24.1
> 