diff mbox series

[4/5] xfs: make attr removal an explicit operation

Message ID 171323026654.250975.17998254398908556664.stgit@frogsfrogsfrogs (mailing list archive)
State Superseded
Headers show
Series [1/5] xfs: remove XFS_DA_OP_REMOVE | expand

Commit Message

Darrick J. Wong April 16, 2024, 1:22 a.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

Parent pointers match attrs on name+value, unlike everything else which
matches on only the name.  Therefore, we cannot keep using the heuristic
that !value means remove.  Make this an explicit operation code.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_attr.c |   19 ++++++++++---------
 fs/xfs/libxfs/xfs_attr.h |    1 +
 fs/xfs/xfs_acl.c         |    3 ++-
 fs/xfs/xfs_ioctl.c       |    7 +++++--
 fs/xfs/xfs_xattr.c       |    7 +++++--
 5 files changed, 23 insertions(+), 14 deletions(-)

Comments

Christoph Hellwig April 16, 2024, 5:12 a.m. UTC | #1 
> +	if (op != XFS_ATTRUPDATE_REMOVE || xfs_inode_hasattr(dp)) {
>  		error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK,
>  				XFS_IEXT_ATTR_MANIP_CNT(rmt_blks));
>  		if (error == -EFBIG)

No need to change this in the current patch, but checking for a remove
on an inodes without attrs just to skip the extent count upgrade
and not fail the whole operation is a little silly :)

> @@ -203,7 +203,8 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  		xfs_acl_to_disk(args.value, acl);
>  	}
>  
> -	error = xfs_attr_change(&args, XFS_ATTRUPDATE_UPSERT);
> +	error = xfs_attr_change(&args, args.value ? XFS_ATTRUPDATE_UPSERT :
> +						    XFS_ATTRUPDATE_REMOVE);

Given that we have a conditional for removing vs setting right above
i'd clean this up a bit:

		xfs_acl_to_disk(args.value, acl);
		error = xfs_attr_change(&args, XFS_ATTRUPDATE_UPSERT)
		kvfree(args.value);
	} else {
		error = xfs_attr_change(&args, XFS_ATTRUPDATE_REMOVE)
		/*
		 * If the attribute didn't exist to start with that's fine.
		 */
		if (error == -ENOATTR)
			error = 0;
  	}

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Darrick J. Wong April 16, 2024, 5:31 p.m. UTC | #2 
On Mon, Apr 15, 2024 at 10:12:12PM -0700, Christoph Hellwig wrote:
> > +	if (op != XFS_ATTRUPDATE_REMOVE || xfs_inode_hasattr(dp)) {
> >  		error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK,
> >  				XFS_IEXT_ATTR_MANIP_CNT(rmt_blks));
> >  		if (error == -EFBIG)
> 
> No need to change this in the current patch, but checking for a remove
> on an inodes without attrs just to skip the extent count upgrade
> and not fail the whole operation is a little silly :)

Yeah, I don't think there's a point to checking @op here.  The only
code we need here is skipping the overflow checks if !xfs_inode_hasattr.
Separate patch.

> > @@ -203,7 +203,8 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> >  		xfs_acl_to_disk(args.value, acl);
> >  	}
> >  
> > -	error = xfs_attr_change(&args, XFS_ATTRUPDATE_UPSERT);
> > +	error = xfs_attr_change(&args, args.value ? XFS_ATTRUPDATE_UPSERT :
> > +						    XFS_ATTRUPDATE_REMOVE);
> 
> Given that we have a conditional for removing vs setting right above
> i'd clean this up a bit:
> 
> 		xfs_acl_to_disk(args.value, acl);
> 		error = xfs_attr_change(&args, XFS_ATTRUPDATE_UPSERT)
> 		kvfree(args.value);
> 	} else {
> 		error = xfs_attr_change(&args, XFS_ATTRUPDATE_REMOVE)
> 		/*
> 		 * If the attribute didn't exist to start with that's fine.
> 		 */
> 		if (error == -ENOATTR)
> 			error = 0;
>   	}

Done.

> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks!

--D
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index b04e09143869d..f8f7445b063c0 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -916,10 +916,6 @@  xfs_attr_defer_add(
 	trace_xfs_attr_defer_add(new->xattri_dela_state, args->dp);
 }
 
-/*
- * Note: If args->value is NULL the attribute will be removed, just like the
- * Linux ->setattr API.
- */
 int
 xfs_attr_set(
 	struct xfs_da_args	*args,
@@ -955,7 +951,10 @@  xfs_attr_set(
 	args->op_flags = XFS_DA_OP_OKNOENT |
 					(args->op_flags & XFS_DA_OP_LOGGED);
 
-	if (args->value) {
+	switch (op) {
+	case XFS_ATTRUPDATE_UPSERT:
+	case XFS_ATTRUPDATE_CREATE:
+	case XFS_ATTRUPDATE_REPLACE:
 		XFS_STATS_INC(mp, xs_attr_set);
 		args->total = xfs_attr_calc_size(args, &local);
 
@@ -975,9 +974,11 @@  xfs_attr_set(
 
 		if (!local)
 			rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
-	} else {
+		break;
+	case XFS_ATTRUPDATE_REMOVE:
 		XFS_STATS_INC(mp, xs_attr_remove);
 		rmt_blks = xfs_attr3_rmt_blocks(mp, XFS_XATTR_SIZE_MAX);
+		break;
 	}
 
 	/*
@@ -989,7 +990,7 @@  xfs_attr_set(
 	if (error)
 		return error;
 
-	if (args->value || xfs_inode_hasattr(dp)) {
+	if (op != XFS_ATTRUPDATE_REMOVE || xfs_inode_hasattr(dp)) {
 		error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK,
 				XFS_IEXT_ATTR_MANIP_CNT(rmt_blks));
 		if (error == -EFBIG)
@@ -1002,7 +1003,7 @@  xfs_attr_set(
 	error = xfs_attr_lookup(args);
 	switch (error) {
 	case -EEXIST:
-		if (!args->value) {
+		if (op == XFS_ATTRUPDATE_REMOVE) {
 			/* if no value, we are performing a remove operation */
 			xfs_attr_defer_add(args, XFS_ATTRI_OP_FLAGS_REMOVE);
 			break;
@@ -1015,7 +1016,7 @@  xfs_attr_set(
 		break;
 	case -ENOATTR:
 		/* Can't remove what isn't there. */
-		if (!args->value)
+		if (op == XFS_ATTRUPDATE_REMOVE)
 			goto out_trans_cancel;
 
 		/* Pure replace fails if no existing attr to replace. */
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index 02dca538f6b91..c8005f52102ad 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -546,6 +546,7 @@  int xfs_attr_get_ilocked(struct xfs_da_args *args);
 int xfs_attr_get(struct xfs_da_args *args);
 
 enum xfs_attr_update {
+	XFS_ATTRUPDATE_REMOVE,	/* remove attr */
 	XFS_ATTRUPDATE_UPSERT,	/* set value, replace any existing attr */
 	XFS_ATTRUPDATE_CREATE,	/* set value, fail if attr already exists */
 	XFS_ATTRUPDATE_REPLACE,	/* set value, fail if attr does not exist */
diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index ea3ae0acff096..48e93b14b4ab8 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -203,7 +203,8 @@  __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		xfs_acl_to_disk(args.value, acl);
 	}
 
-	error = xfs_attr_change(&args, XFS_ATTRUPDATE_UPSERT);
+	error = xfs_attr_change(&args, args.value ? XFS_ATTRUPDATE_UPSERT :
+						    XFS_ATTRUPDATE_REMOVE);
 	kvfree(args.value);
 
 	/*
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 9cf3f8d16c055..66133b4fc20f7 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -363,8 +363,11 @@  xfs_attr_filter(
 
 static inline enum xfs_attr_update
 xfs_xattr_flags(
-	u32			ioc_flags)
+	u32			ioc_flags,
+	void			*value)
 {
+	if (!value)
+		return XFS_ATTRUPDATE_REMOVE;
 	if (ioc_flags & XFS_IOC_ATTR_CREATE)
 		return XFS_ATTRUPDATE_CREATE;
 	if (ioc_flags & XFS_IOC_ATTR_REPLACE)
@@ -526,7 +529,7 @@  xfs_attrmulti_attr_set(
 		args.valuelen = len;
 	}
 
-	error = xfs_attr_change(&args, xfs_xattr_flags(flags));
+	error = xfs_attr_change(&args, xfs_xattr_flags(flags, args.value));
 	if (!error && (flags & XFS_IOC_ATTR_ROOT))
 		xfs_forget_acl(inode, name);
 	kfree(args.value);
diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
index c2d17def7d9d1..0cbb93cf2869c 100644
--- a/fs/xfs/xfs_xattr.c
+++ b/fs/xfs/xfs_xattr.c
@@ -118,8 +118,11 @@  xfs_xattr_get(const struct xattr_handler *handler, struct dentry *unused,
 
 static inline enum xfs_attr_update
 xfs_xattr_flags_to_op(
-	int		flags)
+	int		flags,
+	const void	*value)
 {
+	if (!value)
+		return XFS_ATTRUPDATE_REMOVE;
 	if (flags & XATTR_CREATE)
 		return XFS_ATTRUPDATE_CREATE;
 	if (flags & XATTR_REPLACE)
@@ -143,7 +146,7 @@  xfs_xattr_set(const struct xattr_handler *handler,
 	};
 	int			error;
 
-	error = xfs_attr_change(&args, xfs_xattr_flags_to_op(flags));
+	error = xfs_attr_change(&args, xfs_xattr_flags_to_op(flags, value));
 	if (!error && (handler->flags & XFS_ATTR_ROOT))
 		xfs_forget_acl(inode, name);
 	return error;