[PATCH] xfs: fix xfs_init_attr_trans not handling explicit operation codes

[PATCH] xfs: fix xfs_init_attr_trans not handling explicit operation codes

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

When we were converting the attr code to use an explicit operation code
instead of keying off of attr->value being null, we forgot to change the
code that initializes the transaction reservation.  Split the function
into two helpers that handle the !remove and remove cases, then fix both
callsites to handle this correctly.

Fixes: c27411d4c640 ("xfs: make attr removal an explicit operation")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_attr.c |   40 +++++++++++++++++++++++-----------------
 fs/xfs/libxfs/xfs_attr.h |    6 ++++--
 fs/xfs/xfs_attr_item.c   |   15 +++++++++++++--
 3 files changed, 40 insertions(+), 21 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 33f1d032ef138..302b0cf95528a 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -339,26 +339,31 @@ xfs_attr_calc_size(
 	return nblks;
 }
 
-/* Initialize transaction reservation for attr operations */
-void
-xfs_init_attr_trans(
+/* Initialize transaction reservation for an xattr set/replace/upsert */
+unsigned int
+xfs_attr_init_set_trans(
 	struct xfs_da_args	*args,
-	struct xfs_trans_res	*tres,
-	unsigned int		*total)
+	struct xfs_trans_res	*tres)
 {
 	struct xfs_mount	*mp = args->dp->i_mount;
 
-	if (args->value) {
-		tres->tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
-				 M_RES(mp)->tr_attrsetrt.tr_logres *
-				 args->total;
-		tres->tr_logcount = XFS_ATTRSET_LOG_COUNT;
+	tres->tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
+			  M_RES(mp)->tr_attrsetrt.tr_logres * args->total;
+	tres->tr_logcount = XFS_ATTRSET_LOG_COUNT;
 		tres->tr_logflags = XFS_TRANS_PERM_LOG_RES;
-		*total = args->total;
-	} else {
-		*tres = M_RES(mp)->tr_attrrm;
-		*total = XFS_ATTRRM_SPACE_RES(mp);
-	}
+	return args->total;
+}
+
+/* Initialize transaction reservation for an xattr remove */
+unsigned int
+xfs_attr_init_remove_trans(
+	struct xfs_da_args	*args,
+	struct xfs_trans_res	*tres)
+{
+	struct xfs_mount	*mp = args->dp->i_mount;
+
+	*tres = M_RES(mp)->tr_attrrm;
+	return XFS_ATTRRM_SPACE_RES(mp);
 }
 
 /*
@@ -1021,7 +1026,7 @@ xfs_attr_set(
 	struct xfs_trans_res	tres;
 	int			error, local;
 	int			rmt_blks = 0;
-	unsigned int		total;
+	unsigned int		total = 0;
 
 	ASSERT(!args->trans);
 
@@ -1049,10 +1054,12 @@ xfs_attr_set(
 		if (!local)
 			rmt_blks = xfs_attr3_rmt_blocks(mp, args->attr_filter,
 					args->valuelen);
+		total = xfs_attr_init_set_trans(args, &tres);
 		break;
 	case XFS_ATTRUPDATE_REMOVE:
 		XFS_STATS_INC(mp, xs_attr_remove);
 		rmt_blks = xfs_attr3_max_rmt_blocks(mp);
+		total = xfs_attr_init_remove_trans(args, &tres);
 		break;
 	}
 
@@ -1060,7 +1067,6 @@ xfs_attr_set(
 	 * Root fork attributes can use reserved data blocks for this
 	 * operation if necessary
 	 */
-	xfs_init_attr_trans(args, &tres, &total);
 	error = xfs_trans_alloc_inode(dp, &tres, total, 0, rsvd, &args->trans);
 	if (error)
 		return error;
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index 088cb7b301680..616b0b5a0f11c 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -565,8 +565,10 @@ bool xfs_attr_check_namespace(unsigned int attr_flags);
 bool xfs_attr_namecheck(unsigned int attr_flags, const void *name,
 		size_t length);
 int xfs_attr_calc_size(struct xfs_da_args *args, int *local);
-void xfs_init_attr_trans(struct xfs_da_args *args, struct xfs_trans_res *tres,
-			 unsigned int *total);
+unsigned int xfs_attr_init_set_trans(struct xfs_da_args *args,
+		struct xfs_trans_res *tres);
+unsigned int xfs_attr_init_remove_trans(struct xfs_da_args *args,
+		struct xfs_trans_res *tres);
 
 /*
  * Check to see if the attr should be upgraded from non-existent or shortform to
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 2b10ac4c5fce2..cff051ccafa43 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -746,7 +746,7 @@ xfs_attr_recover_work(
 	struct xfs_attri_log_format	*attrp;
 	struct xfs_attri_log_nameval	*nv = attrip->attri_nameval;
 	int				error;
-	int				total;
+	unsigned int			total = 0;
 
 	/*
 	 * First check the validity of the attr described by the ATTRI.  If any
@@ -763,7 +763,18 @@ xfs_attr_recover_work(
 		return PTR_ERR(attr);
 	args = attr->xattri_da_args;
 
-	xfs_init_attr_trans(args, &resv, &total);
+	switch (xfs_attr_intent_op(attr)) {
+	case XFS_ATTRI_OP_FLAGS_PPTR_SET:
+	case XFS_ATTRI_OP_FLAGS_PPTR_REPLACE:
+	case XFS_ATTRI_OP_FLAGS_SET:
+	case XFS_ATTRI_OP_FLAGS_REPLACE:
+		total = xfs_attr_init_set_trans(args, &resv);
+		break;
+	case XFS_ATTRI_OP_FLAGS_PPTR_REMOVE:
+	case XFS_ATTRI_OP_FLAGS_REMOVE:
+		total = xfs_attr_init_remove_trans(args, &resv);
+		break;
+	}
 	resv = xlog_recover_resv(&resv);
 	error = xfs_trans_alloc(mp, &resv, total, 0, XFS_TRANS_RESERVE, &tp);
 	if (error)

Re: [PATCH] xfs: fix xfs_init_attr_trans not handling explicit operation codes

[Christoph Hellwig]

On Mon, May 20, 2024 at 06:03:38PM -0700, Darrick J. Wong wrote:
> +	tres->tr_logcount = XFS_ATTRSET_LOG_COUNT;
>  		tres->tr_logflags = XFS_TRANS_PERM_LOG_RES;
> +	return args->total;

Seems like indentation is off for the XFS_TRANS_PERM_LOG_RES
assignment?

Also wju does this need to return args->total vs just handling it
in the caller? 

> +/* Initialize transaction reservation for an xattr remove */
> +unsigned int
> +xfs_attr_init_remove_trans(
> +	struct xfs_da_args	*args,
> +	struct xfs_trans_res	*tres)
> +{
> +	struct xfs_mount	*mp = args->dp->i_mount;
> +
> +	*tres = M_RES(mp)->tr_attrrm;
> +	return XFS_ATTRRM_SPACE_RES(mp);

And do we even need this helper vs open coding it like we do in
most transaction allocations?

Re: [PATCH] xfs: fix xfs_init_attr_trans not handling explicit operation codes

[Darrick J. Wong]

On Tue, May 21, 2024 at 06:51:07AM -0700, Christoph Hellwig wrote:
> On Mon, May 20, 2024 at 06:03:38PM -0700, Darrick J. Wong wrote:
> > +	tres->tr_logcount = XFS_ATTRSET_LOG_COUNT;
> >  		tres->tr_logflags = XFS_TRANS_PERM_LOG_RES;
> > +	return args->total;
> 
> Seems like indentation is off for the XFS_TRANS_PERM_LOG_RES
> assignment?

Oops.  I think I could shorten this to:

/* Initialize transaction reservation for an xattr set/replace/upsert */
inline struct xfs_trans_res
xfs_attr_set_resv(
	const struct xfs_da_args	*args)
{
	struct xfs_mount		*mp = args->dp->i_mount;
	struct xfs_trans_res		ret = {
		.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
			    M_RES(mp)->tr_attrsetrt.tr_logres * args->total,
		.tr_logcount		= XFS_ATTRSET_LOG_COUNT,
		.tr_logflags		= XFS_TRANS_PERM_LOG_RES,
	};

	return ret;
}

> Also wju does this need to return args->total vs just handling it
> in the caller? 

I'm not sure, @total for the remove action used to be in xfs_attr_set
prior to the creation of xfs_attr_recover_work.

> > +/* Initialize transaction reservation for an xattr remove */
> > +unsigned int
> > +xfs_attr_init_remove_trans(
> > +	struct xfs_da_args	*args,
> > +	struct xfs_trans_res	*tres)
> > +{
> > +	struct xfs_mount	*mp = args->dp->i_mount;
> > +
> > +	*tres = M_RES(mp)->tr_attrrm;
> > +	return XFS_ATTRRM_SPACE_RES(mp);
> 
> And do we even need this helper vs open coding it like we do in
> most transaction allocations?

Not really.

--D

[PATCH v2] xfs: fix xfs_init_attr_trans not handling explicit operation codes

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

When we were converting the attr code to use an explicit operation code
instead of keying off of attr->value being null, we forgot to change the
code that initializes the transaction reservation.  Split the function
into two helpers that handle the !remove and remove cases, then fix both
callsites to handle this correctly.

Fixes: c27411d4c640 ("xfs: make attr removal an explicit operation")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v2: remove the xfs_init_attr_remove_trans function, fix indenting
---
 fs/xfs/libxfs/xfs_attr.c |   37 +++++++++++++++++--------------------
 fs/xfs/libxfs/xfs_attr.h |    3 +--
 fs/xfs/xfs_attr_item.c   |   17 +++++++++++++++--
 3 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 33f1d032ef138..82f601cd6833a 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -339,26 +339,20 @@ xfs_attr_calc_size(
 	return nblks;
 }
 
-/* Initialize transaction reservation for attr operations */
-void
-xfs_init_attr_trans(
-	struct xfs_da_args	*args,
-	struct xfs_trans_res	*tres,
-	unsigned int		*total)
+/* Initialize transaction reservation for an xattr set/replace/upsert */
+inline struct xfs_trans_res
+xfs_attr_set_resv(
+	const struct xfs_da_args	*args)
 {
-	struct xfs_mount	*mp = args->dp->i_mount;
+	struct xfs_mount		*mp = args->dp->i_mount;
+	struct xfs_trans_res		ret = {
+		.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
+			    M_RES(mp)->tr_attrsetrt.tr_logres * args->total,
+		.tr_logcount		= XFS_ATTRSET_LOG_COUNT,
+		.tr_logflags		= XFS_TRANS_PERM_LOG_RES,
+	};
 
-	if (args->value) {
-		tres->tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
-				 M_RES(mp)->tr_attrsetrt.tr_logres *
-				 args->total;
-		tres->tr_logcount = XFS_ATTRSET_LOG_COUNT;
-		tres->tr_logflags = XFS_TRANS_PERM_LOG_RES;
-		*total = args->total;
-	} else {
-		*tres = M_RES(mp)->tr_attrrm;
-		*total = XFS_ATTRRM_SPACE_RES(mp);
-	}
+	return ret;
 }
 
 /*
@@ -1021,7 +1015,7 @@ xfs_attr_set(
 	struct xfs_trans_res	tres;
 	int			error, local;
 	int			rmt_blks = 0;
-	unsigned int		total;
+	unsigned int		total = 0;
 
 	ASSERT(!args->trans);
 
@@ -1049,10 +1043,14 @@ xfs_attr_set(
 		if (!local)
 			rmt_blks = xfs_attr3_rmt_blocks(mp, args->attr_filter,
 					args->valuelen);
+		tres = xfs_attr_set_resv(args);
+		total = args->total;
 		break;
 	case XFS_ATTRUPDATE_REMOVE:
 		XFS_STATS_INC(mp, xs_attr_remove);
 		rmt_blks = xfs_attr3_max_rmt_blocks(mp);
+		tres = M_RES(mp)->tr_attrrm;
+		total = XFS_ATTRRM_SPACE_RES(mp);
 		break;
 	}
 
@@ -1060,7 +1058,6 @@ xfs_attr_set(
 	 * Root fork attributes can use reserved data blocks for this
 	 * operation if necessary
 	 */
-	xfs_init_attr_trans(args, &tres, &total);
 	error = xfs_trans_alloc_inode(dp, &tres, total, 0, rsvd, &args->trans);
 	if (error)
 		return error;
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index 088cb7b301680..0e51d0723f9aa 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -565,8 +565,7 @@ bool xfs_attr_check_namespace(unsigned int attr_flags);
 bool xfs_attr_namecheck(unsigned int attr_flags, const void *name,
 		size_t length);
 int xfs_attr_calc_size(struct xfs_da_args *args, int *local);
-void xfs_init_attr_trans(struct xfs_da_args *args, struct xfs_trans_res *tres,
-			 unsigned int *total);
+struct xfs_trans_res xfs_attr_set_resv(const struct xfs_da_args *args);
 
 /*
  * Check to see if the attr should be upgraded from non-existent or shortform to
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 2b10ac4c5fce2..f683b7a9323f1 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -746,7 +746,7 @@ xfs_attr_recover_work(
 	struct xfs_attri_log_format	*attrp;
 	struct xfs_attri_log_nameval	*nv = attrip->attri_nameval;
 	int				error;
-	int				total;
+	unsigned int			total = 0;
 
 	/*
 	 * First check the validity of the attr described by the ATTRI.  If any
@@ -763,7 +763,20 @@ xfs_attr_recover_work(
 		return PTR_ERR(attr);
 	args = attr->xattri_da_args;
 
-	xfs_init_attr_trans(args, &resv, &total);
+	switch (xfs_attr_intent_op(attr)) {
+	case XFS_ATTRI_OP_FLAGS_PPTR_SET:
+	case XFS_ATTRI_OP_FLAGS_PPTR_REPLACE:
+	case XFS_ATTRI_OP_FLAGS_SET:
+	case XFS_ATTRI_OP_FLAGS_REPLACE:
+		resv = xfs_attr_set_resv(args);
+		total = args->total;
+		break;
+	case XFS_ATTRI_OP_FLAGS_PPTR_REMOVE:
+	case XFS_ATTRI_OP_FLAGS_REMOVE:
+		resv = M_RES(mp)->tr_attrrm;
+		total = XFS_ATTRRM_SPACE_RES(mp);
+		break;
+	}
 	resv = xlog_recover_resv(&resv);
 	error = xfs_trans_alloc(mp, &resv, total, 0, XFS_TRANS_RESERVE, &tp);
 	if (error)

Re: [PATCH v2] xfs: fix xfs_init_attr_trans not handling explicit operation codes

[Christoph Hellwig]

Looks good:

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