Re: [PATCH 16/30] xfs: replace ATTR_KERNOTIME with XFS_DA_OP_NOTIME

From: Chandan Rajendra <chandan@linux.ibm.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-xfs@vger.kernel.org,
	Allison Collins <allison.henderson@oracle.com>,
	"Darrick J . Wong" <darrick.wong@oracle.com>
Subject: Re: [PATCH 16/30] xfs: replace ATTR_KERNOTIME with XFS_DA_OP_NOTIME
Date: Sat, 08 Feb 2020 17:05:25 +0530	[thread overview]
Message-ID: <9195667.EHZHh7sMLN@localhost.localdomain> (raw)
In-Reply-To: <20200129170310.51370-17-hch@lst.de>

On Wednesday, January 29, 2020 10:32 PM 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.
>

The changes look good to me,
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  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 1382e51ef85e..3b1db2afb104 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)
> @@ -389,7 +389,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 2da22595f828..dd1cb8c50518 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -436,7 +436,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))
> 


-- 
chandan