pass ops directly to the xfs_defer helpers

pass ops directly to the xfs_defer helpers

[Christoph Hellwig]

Hi all,

this little series cleans up the defer mechanism to directly pass the
ops instead a type enum that is used to index a global table.

Diffstat:
 libxfs/xfs_alloc.c       |    4 +-
 libxfs/xfs_attr.c        |   92 +++++++++++------------------------------------
 libxfs/xfs_bmap.c        |    2 -
 libxfs/xfs_defer.c       |   63 ++++++++++----------------------
 libxfs/xfs_defer.h       |   25 +++---------
 libxfs/xfs_log_recover.h |    3 +
 libxfs/xfs_refcount.c    |    2 -
 libxfs/xfs_rmap.c        |    2 -
 xfs_attr_item.c          |   69 +++++++++++++++++------------------
 xfs_bmap_item.c          |    3 +
 xfs_extfree_item.c       |    4 +-
 xfs_log_recover.c        |    4 +-
 xfs_refcount_item.c      |    3 +
 xfs_rmap_item.c          |    3 +
 xfs_trace.h              |   18 ++++-----
 15 files changed, 110 insertions(+), 187 deletions(-)

[PATCH 1/5] xfs: consolidate the xfs_attr_defer_* helpers

[Christoph Hellwig]

Consolidate the xfs_attr_defer_* helpers into a single xfs_attr_defer_add
one that picks the right dela_state based on the passed in operation.
Also move to a single trace point as the actual operation is visible
through the flags in the delta_state passed to the trace point.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_attr.c | 90 ++++++++++------------------------------
 fs/xfs/xfs_trace.h       |  2 -
 2 files changed, 21 insertions(+), 71 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index e28d93d232de49..4fed0c87a968ab 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -880,11 +880,10 @@ xfs_attr_lookup(
 	return error;
 }
 
-static int
-xfs_attr_intent_init(
+static void
+xfs_attr_defer_add(
 	struct xfs_da_args	*args,
-	unsigned int		op_flags,	/* op flag (set or remove) */
-	struct xfs_attr_intent	**attr)		/* new xfs_attr_intent */
+	unsigned int		op_flags)
 {
 
 	struct xfs_attr_intent	*new;
@@ -893,66 +892,22 @@ xfs_attr_intent_init(
 	new->xattri_op_flags = op_flags;
 	new->xattri_da_args = args;
 
-	*attr = new;
-	return 0;
-}
-
-/* Sets an attribute for an inode as a deferred operation */
-static int
-xfs_attr_defer_add(
-	struct xfs_da_args	*args)
-{
-	struct xfs_attr_intent	*new;
-	int			error = 0;
-
-	error = xfs_attr_intent_init(args, XFS_ATTRI_OP_FLAGS_SET, &new);
-	if (error)
-		return error;
+	switch (op_flags) {
+	case XFS_ATTRI_OP_FLAGS_SET:
+		new->xattri_dela_state = xfs_attr_init_add_state(args);
+		break;
+	case XFS_ATTRI_OP_FLAGS_REPLACE:
+		new->xattri_dela_state = xfs_attr_init_replace_state(args);
+		break;
+	case XFS_ATTRI_OP_FLAGS_REMOVE:
+		new->xattri_dela_state = xfs_attr_init_remove_state(args);
+		break;
+	default:
+		ASSERT(0);
+	}
 
-	new->xattri_dela_state = xfs_attr_init_add_state(args);
 	xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
 	trace_xfs_attr_defer_add(new->xattri_dela_state, args->dp);
-
-	return 0;
-}
-
-/* Sets an attribute for an inode as a deferred operation */
-static int
-xfs_attr_defer_replace(
-	struct xfs_da_args	*args)
-{
-	struct xfs_attr_intent	*new;
-	int			error = 0;
-
-	error = xfs_attr_intent_init(args, XFS_ATTRI_OP_FLAGS_REPLACE, &new);
-	if (error)
-		return error;
-
-	new->xattri_dela_state = xfs_attr_init_replace_state(args);
-	xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
-	trace_xfs_attr_defer_replace(new->xattri_dela_state, args->dp);
-
-	return 0;
-}
-
-/* Removes an attribute for an inode as a deferred operation */
-static int
-xfs_attr_defer_remove(
-	struct xfs_da_args	*args)
-{
-
-	struct xfs_attr_intent	*new;
-	int			error;
-
-	error  = xfs_attr_intent_init(args, XFS_ATTRI_OP_FLAGS_REMOVE, &new);
-	if (error)
-		return error;
-
-	new->xattri_dela_state = xfs_attr_init_remove_state(args);
-	xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
-	trace_xfs_attr_defer_remove(new->xattri_dela_state, args->dp);
-
-	return 0;
 }
 
 /*
@@ -1038,16 +993,16 @@ xfs_attr_set(
 	error = xfs_attr_lookup(args);
 	switch (error) {
 	case -EEXIST:
-		/* if no value, we are performing a remove operation */
 		if (!args->value) {
-			error = xfs_attr_defer_remove(args);
+			/* if no value, we are performing a remove operation */
+			xfs_attr_defer_add(args, XFS_ATTRI_OP_FLAGS_REMOVE);
 			break;
 		}
+
 		/* Pure create fails if the attr already exists */
 		if (args->attr_flags & XATTR_CREATE)
 			goto out_trans_cancel;
-
-		error = xfs_attr_defer_replace(args);
+		xfs_attr_defer_add(args, XFS_ATTRI_OP_FLAGS_REPLACE);
 		break;
 	case -ENOATTR:
 		/* Can't remove what isn't there. */
@@ -1057,14 +1012,11 @@ xfs_attr_set(
 		/* Pure replace fails if no existing attr to replace. */
 		if (args->attr_flags & XATTR_REPLACE)
 			goto out_trans_cancel;
-
-		error = xfs_attr_defer_add(args);
+		xfs_attr_defer_add(args, XFS_ATTRI_OP_FLAGS_SET);
 		break;
 	default:
 		goto out_trans_cancel;
 	}
-	if (error)
-		goto out_trans_cancel;
 
 	/*
 	 * If this is a synchronous mount, make sure that the
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 514095b6ba2bdb..516529c151ae1c 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -4408,8 +4408,6 @@ DEFINE_DAS_STATE_EVENT(xfs_attr_remove_iter_return);
 DEFINE_DAS_STATE_EVENT(xfs_attr_rmtval_alloc);
 DEFINE_DAS_STATE_EVENT(xfs_attr_rmtval_remove_return);
 DEFINE_DAS_STATE_EVENT(xfs_attr_defer_add);
-DEFINE_DAS_STATE_EVENT(xfs_attr_defer_replace);
-DEFINE_DAS_STATE_EVENT(xfs_attr_defer_remove);
 
 
 TRACE_EVENT(xfs_force_shutdown,
-- 
2.39.2

Re: [PATCH 1/5] xfs: consolidate the xfs_attr_defer_* helpers

[Darrick J. Wong]

On Wed, Dec 13, 2023 at 10:06:29AM +0100, Christoph Hellwig wrote:
> Consolidate the xfs_attr_defer_* helpers into a single xfs_attr_defer_add
> one that picks the right dela_state based on the passed in operation.
> Also move to a single trace point as the actual operation is visible
> through the flags in the delta_state passed to the trace point.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Heh, I had been thinking about the same consolidation...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_attr.c | 90 ++++++++++------------------------------
>  fs/xfs/xfs_trace.h       |  2 -
>  2 files changed, 21 insertions(+), 71 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index e28d93d232de49..4fed0c87a968ab 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -880,11 +880,10 @@ xfs_attr_lookup(
>  	return error;
>  }
>  
> -static int
> -xfs_attr_intent_init(
> +static void
> +xfs_attr_defer_add(
>  	struct xfs_da_args	*args,
> -	unsigned int		op_flags,	/* op flag (set or remove) */
> -	struct xfs_attr_intent	**attr)		/* new xfs_attr_intent */
> +	unsigned int		op_flags)
>  {
>  
>  	struct xfs_attr_intent	*new;
> @@ -893,66 +892,22 @@ xfs_attr_intent_init(
>  	new->xattri_op_flags = op_flags;
>  	new->xattri_da_args = args;
>  
> -	*attr = new;
> -	return 0;
> -}
> -
> -/* Sets an attribute for an inode as a deferred operation */
> -static int
> -xfs_attr_defer_add(
> -	struct xfs_da_args	*args)
> -{
> -	struct xfs_attr_intent	*new;
> -	int			error = 0;
> -
> -	error = xfs_attr_intent_init(args, XFS_ATTRI_OP_FLAGS_SET, &new);
> -	if (error)
> -		return error;
> +	switch (op_flags) {
> +	case XFS_ATTRI_OP_FLAGS_SET:
> +		new->xattri_dela_state = xfs_attr_init_add_state(args);
> +		break;
> +	case XFS_ATTRI_OP_FLAGS_REPLACE:
> +		new->xattri_dela_state = xfs_attr_init_replace_state(args);
> +		break;
> +	case XFS_ATTRI_OP_FLAGS_REMOVE:
> +		new->xattri_dela_state = xfs_attr_init_remove_state(args);
> +		break;
> +	default:
> +		ASSERT(0);
> +	}
>  
> -	new->xattri_dela_state = xfs_attr_init_add_state(args);
>  	xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
>  	trace_xfs_attr_defer_add(new->xattri_dela_state, args->dp);
> -
> -	return 0;
> -}
> -
> -/* Sets an attribute for an inode as a deferred operation */
> -static int
> -xfs_attr_defer_replace(
> -	struct xfs_da_args	*args)
> -{
> -	struct xfs_attr_intent	*new;
> -	int			error = 0;
> -
> -	error = xfs_attr_intent_init(args, XFS_ATTRI_OP_FLAGS_REPLACE, &new);
> -	if (error)
> -		return error;
> -
> -	new->xattri_dela_state = xfs_attr_init_replace_state(args);
> -	xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
> -	trace_xfs_attr_defer_replace(new->xattri_dela_state, args->dp);
> -
> -	return 0;
> -}
> -
> -/* Removes an attribute for an inode as a deferred operation */
> -static int
> -xfs_attr_defer_remove(
> -	struct xfs_da_args	*args)
> -{
> -
> -	struct xfs_attr_intent	*new;
> -	int			error;
> -
> -	error  = xfs_attr_intent_init(args, XFS_ATTRI_OP_FLAGS_REMOVE, &new);
> -	if (error)
> -		return error;
> -
> -	new->xattri_dela_state = xfs_attr_init_remove_state(args);
> -	xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
> -	trace_xfs_attr_defer_remove(new->xattri_dela_state, args->dp);
> -
> -	return 0;
>  }
>  
>  /*
> @@ -1038,16 +993,16 @@ xfs_attr_set(
>  	error = xfs_attr_lookup(args);
>  	switch (error) {
>  	case -EEXIST:
> -		/* if no value, we are performing a remove operation */
>  		if (!args->value) {
> -			error = xfs_attr_defer_remove(args);
> +			/* if no value, we are performing a remove operation */
> +			xfs_attr_defer_add(args, XFS_ATTRI_OP_FLAGS_REMOVE);
>  			break;
>  		}
> +
>  		/* Pure create fails if the attr already exists */
>  		if (args->attr_flags & XATTR_CREATE)
>  			goto out_trans_cancel;
> -
> -		error = xfs_attr_defer_replace(args);
> +		xfs_attr_defer_add(args, XFS_ATTRI_OP_FLAGS_REPLACE);
>  		break;
>  	case -ENOATTR:
>  		/* Can't remove what isn't there. */
> @@ -1057,14 +1012,11 @@ xfs_attr_set(
>  		/* Pure replace fails if no existing attr to replace. */
>  		if (args->attr_flags & XATTR_REPLACE)
>  			goto out_trans_cancel;
> -
> -		error = xfs_attr_defer_add(args);
> +		xfs_attr_defer_add(args, XFS_ATTRI_OP_FLAGS_SET);
>  		break;
>  	default:
>  		goto out_trans_cancel;
>  	}
> -	if (error)
> -		goto out_trans_cancel;
>  
>  	/*
>  	 * If this is a synchronous mount, make sure that the
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 514095b6ba2bdb..516529c151ae1c 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -4408,8 +4408,6 @@ DEFINE_DAS_STATE_EVENT(xfs_attr_remove_iter_return);
>  DEFINE_DAS_STATE_EVENT(xfs_attr_rmtval_alloc);
>  DEFINE_DAS_STATE_EVENT(xfs_attr_rmtval_remove_return);
>  DEFINE_DAS_STATE_EVENT(xfs_attr_defer_add);
> -DEFINE_DAS_STATE_EVENT(xfs_attr_defer_replace);
> -DEFINE_DAS_STATE_EVENT(xfs_attr_defer_remove);
>  
>  
>  TRACE_EVENT(xfs_force_shutdown,
> -- 
> 2.39.2
> 
> 

[PATCH 2/5] xfs: move xfs_attr_defer_type up in xfs_attr_item.c

[Christoph Hellwig]

We'll reference it directly in xlog_recover_attri_commit_pass2, so move
it up a bit.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_attr_item.c | 66 +++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 39f2c5a46179f7..4e0eaa2640e0d2 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -654,6 +654,39 @@ xfs_attr_relog_intent(
 	return &new_attrip->attri_item;
 }
 
+/* Get an ATTRD so we can process all the attrs. */
+static struct xfs_log_item *
+xfs_attr_create_done(
+	struct xfs_trans		*tp,
+	struct xfs_log_item		*intent,
+	unsigned int			count)
+{
+	struct xfs_attri_log_item	*attrip;
+	struct xfs_attrd_log_item	*attrdp;
+
+	attrip = ATTRI_ITEM(intent);
+
+	attrdp = kmem_cache_zalloc(xfs_attrd_cache, GFP_NOFS | __GFP_NOFAIL);
+
+	xfs_log_item_init(tp->t_mountp, &attrdp->attrd_item, XFS_LI_ATTRD,
+			  &xfs_attrd_item_ops);
+	attrdp->attrd_attrip = attrip;
+	attrdp->attrd_format.alfd_alf_id = attrip->attri_format.alfi_id;
+
+	return &attrdp->attrd_item;
+}
+
+const struct xfs_defer_op_type xfs_attr_defer_type = {
+	.max_items	= 1,
+	.create_intent	= xfs_attr_create_intent,
+	.abort_intent	= xfs_attr_abort_intent,
+	.create_done	= xfs_attr_create_done,
+	.finish_item	= xfs_attr_finish_item,
+	.cancel_item	= xfs_attr_cancel_item,
+	.recover_work	= xfs_attr_recover_work,
+	.relog_intent	= xfs_attr_relog_intent,
+};
+
 STATIC int
 xlog_recover_attri_commit_pass2(
 	struct xlog                     *log,
@@ -730,39 +763,6 @@ xlog_recover_attri_commit_pass2(
 	return 0;
 }
 
-/* Get an ATTRD so we can process all the attrs. */
-static struct xfs_log_item *
-xfs_attr_create_done(
-	struct xfs_trans		*tp,
-	struct xfs_log_item		*intent,
-	unsigned int			count)
-{
-	struct xfs_attri_log_item	*attrip;
-	struct xfs_attrd_log_item	*attrdp;
-
-	attrip = ATTRI_ITEM(intent);
-
-	attrdp = kmem_cache_zalloc(xfs_attrd_cache, GFP_NOFS | __GFP_NOFAIL);
-
-	xfs_log_item_init(tp->t_mountp, &attrdp->attrd_item, XFS_LI_ATTRD,
-			  &xfs_attrd_item_ops);
-	attrdp->attrd_attrip = attrip;
-	attrdp->attrd_format.alfd_alf_id = attrip->attri_format.alfi_id;
-
-	return &attrdp->attrd_item;
-}
-
-const struct xfs_defer_op_type xfs_attr_defer_type = {
-	.max_items	= 1,
-	.create_intent	= xfs_attr_create_intent,
-	.abort_intent	= xfs_attr_abort_intent,
-	.create_done	= xfs_attr_create_done,
-	.finish_item	= xfs_attr_finish_item,
-	.cancel_item	= xfs_attr_cancel_item,
-	.recover_work	= xfs_attr_recover_work,
-	.relog_intent	= xfs_attr_relog_intent,
-};
-
 /*
  * This routine is called when an ATTRD format structure is found in a committed
  * transaction in the log. Its purpose is to cancel the corresponding ATTRI if
-- 
2.39.2

Re: [PATCH 2/5] xfs: move xfs_attr_defer_type up in xfs_attr_item.c

[Darrick J. Wong]

On Wed, Dec 13, 2023 at 10:06:30AM +0100, Christoph Hellwig wrote:
> We'll reference it directly in xlog_recover_attri_commit_pass2, so move
> it up a bit.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_attr_item.c | 66 +++++++++++++++++++++---------------------
>  1 file changed, 33 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 39f2c5a46179f7..4e0eaa2640e0d2 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -654,6 +654,39 @@ xfs_attr_relog_intent(
>  	return &new_attrip->attri_item;
>  }
>  
> +/* Get an ATTRD so we can process all the attrs. */
> +static struct xfs_log_item *
> +xfs_attr_create_done(
> +	struct xfs_trans		*tp,
> +	struct xfs_log_item		*intent,
> +	unsigned int			count)
> +{
> +	struct xfs_attri_log_item	*attrip;
> +	struct xfs_attrd_log_item	*attrdp;
> +
> +	attrip = ATTRI_ITEM(intent);
> +
> +	attrdp = kmem_cache_zalloc(xfs_attrd_cache, GFP_NOFS | __GFP_NOFAIL);
> +
> +	xfs_log_item_init(tp->t_mountp, &attrdp->attrd_item, XFS_LI_ATTRD,
> +			  &xfs_attrd_item_ops);
> +	attrdp->attrd_attrip = attrip;
> +	attrdp->attrd_format.alfd_alf_id = attrip->attri_format.alfi_id;
> +
> +	return &attrdp->attrd_item;
> +}
> +
> +const struct xfs_defer_op_type xfs_attr_defer_type = {
> +	.max_items	= 1,
> +	.create_intent	= xfs_attr_create_intent,
> +	.abort_intent	= xfs_attr_abort_intent,
> +	.create_done	= xfs_attr_create_done,
> +	.finish_item	= xfs_attr_finish_item,
> +	.cancel_item	= xfs_attr_cancel_item,
> +	.recover_work	= xfs_attr_recover_work,
> +	.relog_intent	= xfs_attr_relog_intent,
> +};
> +
>  STATIC int
>  xlog_recover_attri_commit_pass2(
>  	struct xlog                     *log,
> @@ -730,39 +763,6 @@ xlog_recover_attri_commit_pass2(
>  	return 0;
>  }
>  
> -/* Get an ATTRD so we can process all the attrs. */
> -static struct xfs_log_item *
> -xfs_attr_create_done(
> -	struct xfs_trans		*tp,
> -	struct xfs_log_item		*intent,
> -	unsigned int			count)
> -{
> -	struct xfs_attri_log_item	*attrip;
> -	struct xfs_attrd_log_item	*attrdp;
> -
> -	attrip = ATTRI_ITEM(intent);
> -
> -	attrdp = kmem_cache_zalloc(xfs_attrd_cache, GFP_NOFS | __GFP_NOFAIL);
> -
> -	xfs_log_item_init(tp->t_mountp, &attrdp->attrd_item, XFS_LI_ATTRD,
> -			  &xfs_attrd_item_ops);
> -	attrdp->attrd_attrip = attrip;
> -	attrdp->attrd_format.alfd_alf_id = attrip->attri_format.alfi_id;
> -
> -	return &attrdp->attrd_item;
> -}
> -
> -const struct xfs_defer_op_type xfs_attr_defer_type = {
> -	.max_items	= 1,
> -	.create_intent	= xfs_attr_create_intent,
> -	.abort_intent	= xfs_attr_abort_intent,
> -	.create_done	= xfs_attr_create_done,
> -	.finish_item	= xfs_attr_finish_item,
> -	.cancel_item	= xfs_attr_cancel_item,
> -	.recover_work	= xfs_attr_recover_work,
> -	.relog_intent	= xfs_attr_relog_intent,
> -};
> -
>  /*
>   * This routine is called when an ATTRD format structure is found in a committed
>   * transaction in the log. Its purpose is to cancel the corresponding ATTRI if
> -- 
> 2.39.2
> 
> 

[PATCH 3/5] xfs: store an ops pointer in struct xfs_defer_pending

[Christoph Hellwig]

The dfp_type field in struct xfs_defer_pending is only used to either
look up the operations associated with the pending word or in trace
points.  Replace it with a direct pointer to the operations vector,
and store a pretty name in the vector for tracing.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_defer.c  | 43 +++++++++++++++-----------------------
 fs/xfs/libxfs/xfs_defer.h  |  5 +++--
 fs/xfs/xfs_attr_item.c     |  1 +
 fs/xfs/xfs_bmap_item.c     |  1 +
 fs/xfs/xfs_extfree_item.c  |  2 ++
 fs/xfs/xfs_refcount_item.c |  1 +
 fs/xfs/xfs_rmap_item.c     |  1 +
 fs/xfs/xfs_trace.h         | 16 +++++++-------
 8 files changed, 34 insertions(+), 36 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index ecc2f7ec699169..e70881ae5cc597 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -251,7 +251,6 @@ xfs_defer_create_done(
 	struct xfs_trans		*tp,
 	struct xfs_defer_pending	*dfp)
 {
-	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
 	struct xfs_log_item		*lip;
 
 	/* If there is no log intent item, there can be no log done item. */
@@ -266,7 +265,7 @@ xfs_defer_create_done(
 	 * 2.) shuts down the filesystem
 	 */
 	tp->t_flags |= XFS_TRANS_DIRTY;
-	lip = ops->create_done(tp, dfp->dfp_intent, dfp->dfp_count);
+	lip = dfp->dfp_ops->create_done(tp, dfp->dfp_intent, dfp->dfp_count);
 	if (!lip)
 		return;
 
@@ -287,13 +286,13 @@ xfs_defer_create_intent(
 	struct xfs_defer_pending	*dfp,
 	bool				sort)
 {
-	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
 	struct xfs_log_item		*lip;
 
 	if (dfp->dfp_intent)
 		return 1;
 
-	lip = ops->create_intent(tp, &dfp->dfp_work, dfp->dfp_count, sort);
+	lip = dfp->dfp_ops->create_intent(tp, &dfp->dfp_work, dfp->dfp_count,
+			sort);
 	if (!lip)
 		return 0;
 	if (IS_ERR(lip))
@@ -338,12 +337,10 @@ xfs_defer_pending_abort(
 	struct xfs_mount		*mp,
 	struct xfs_defer_pending	*dfp)
 {
-	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
-
 	trace_xfs_defer_pending_abort(mp, dfp);
 
 	if (dfp->dfp_intent && !dfp->dfp_done) {
-		ops->abort_intent(dfp->dfp_intent);
+		dfp->dfp_ops->abort_intent(dfp->dfp_intent);
 		dfp->dfp_intent = NULL;
 	}
 }
@@ -353,7 +350,6 @@ xfs_defer_pending_cancel_work(
 	struct xfs_mount		*mp,
 	struct xfs_defer_pending	*dfp)
 {
-	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
 	struct list_head		*pwi;
 	struct list_head		*n;
 
@@ -364,7 +360,7 @@ xfs_defer_pending_cancel_work(
 		list_del(pwi);
 		dfp->dfp_count--;
 		trace_xfs_defer_cancel_item(mp, dfp, pwi);
-		ops->cancel_item(pwi);
+		dfp->dfp_ops->cancel_item(pwi);
 	}
 	ASSERT(dfp->dfp_count == 0);
 	kmem_cache_free(xfs_defer_pending_cache, dfp);
@@ -522,11 +518,10 @@ xfs_defer_relog_intent(
 	struct xfs_defer_pending	*dfp)
 {
 	struct xfs_log_item		*lip;
-	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
 
 	xfs_defer_create_done(tp, dfp);
 
-	lip = ops->relog_intent(tp, dfp->dfp_intent, dfp->dfp_done);
+	lip = dfp->dfp_ops->relog_intent(tp, dfp->dfp_intent, dfp->dfp_done);
 	if (lip) {
 		xfs_trans_add_item(tp, lip);
 		set_bit(XFS_LI_DIRTY, &lip->li_flags);
@@ -593,7 +588,7 @@ xfs_defer_finish_one(
 	struct xfs_trans		*tp,
 	struct xfs_defer_pending	*dfp)
 {
-	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
+	const struct xfs_defer_op_type	*ops = dfp->dfp_ops;
 	struct xfs_btree_cur		*state = NULL;
 	struct list_head		*li, *n;
 	int				error;
@@ -790,7 +785,6 @@ xfs_defer_cancel(
 static inline struct xfs_defer_pending *
 xfs_defer_find_last(
 	struct xfs_trans		*tp,
-	enum xfs_defer_ops_type		type,
 	const struct xfs_defer_op_type	*ops)
 {
 	struct xfs_defer_pending	*dfp = NULL;
@@ -803,7 +797,7 @@ xfs_defer_find_last(
 			dfp_list);
 
 	/* Wrong type? */
-	if (dfp->dfp_type != type)
+	if (dfp->dfp_ops != ops)
 		return NULL;
 	return dfp;
 }
@@ -836,13 +830,13 @@ xfs_defer_can_append(
 static inline struct xfs_defer_pending *
 xfs_defer_alloc(
 	struct xfs_trans		*tp,
-	enum xfs_defer_ops_type		type)
+	const struct xfs_defer_op_type	*ops)
 {
 	struct xfs_defer_pending	*dfp;
 
 	dfp = kmem_cache_zalloc(xfs_defer_pending_cache,
 			GFP_NOFS | __GFP_NOFAIL);
-	dfp->dfp_type = type;
+	dfp->dfp_ops = ops;
 	INIT_LIST_HEAD(&dfp->dfp_work);
 	list_add_tail(&dfp->dfp_list, &tp->t_dfops);
 
@@ -862,9 +856,9 @@ xfs_defer_add(
 	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
 	BUILD_BUG_ON(ARRAY_SIZE(defer_op_types) != XFS_DEFER_OPS_TYPE_MAX);
 
-	dfp = xfs_defer_find_last(tp, type, ops);
+	dfp = xfs_defer_find_last(tp, ops);
 	if (!dfp || !xfs_defer_can_append(dfp, ops))
-		dfp = xfs_defer_alloc(tp, type);
+		dfp = xfs_defer_alloc(tp, ops);
 
 	xfs_defer_add_item(dfp, li);
 	trace_xfs_defer_add_item(tp->t_mountp, dfp, li);
@@ -880,17 +874,15 @@ xfs_defer_add_barrier(
 	struct xfs_trans		*tp)
 {
 	struct xfs_defer_pending	*dfp;
-	const enum xfs_defer_ops_type	type = XFS_DEFER_OPS_TYPE_BARRIER;
-	const struct xfs_defer_op_type	*ops = defer_op_types[type];
 
 	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
 
 	/* If the last defer op added was a barrier, we're done. */
-	dfp = xfs_defer_find_last(tp, type, ops);
+	dfp = xfs_defer_find_last(tp, &xfs_barrier_defer_type);
 	if (dfp)
 		return;
 
-	xfs_defer_alloc(tp, type);
+	xfs_defer_alloc(tp, &xfs_barrier_defer_type);
 
 	trace_xfs_defer_add_item(tp->t_mountp, dfp, NULL);
 }
@@ -909,7 +901,7 @@ xfs_defer_start_recovery(
 
 	dfp = kmem_cache_zalloc(xfs_defer_pending_cache,
 			GFP_NOFS | __GFP_NOFAIL);
-	dfp->dfp_type = dfp_type;
+	dfp->dfp_ops = defer_op_types[dfp_type];
 	dfp->dfp_intent = lip;
 	INIT_LIST_HEAD(&dfp->dfp_work);
 	list_add_tail(&dfp->dfp_list, r_dfops);
@@ -935,13 +927,12 @@ xfs_defer_finish_recovery(
 	struct xfs_defer_pending	*dfp,
 	struct list_head		*capture_list)
 {
-	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
 	int				error;
 
-	error = ops->recover_work(dfp, capture_list);
+	error = dfp->dfp_ops->recover_work(dfp, capture_list);
 	if (error)
 		trace_xlog_intent_recovery_failed(mp, error,
-				ops->recover_work);
+				dfp->dfp_ops->recover_work);
 	return error;
 }
 
diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index 5b1990ef3e5df4..957a06278e880d 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -34,9 +34,9 @@ struct xfs_defer_pending {
 	struct list_head		dfp_work;	/* work items */
 	struct xfs_log_item		*dfp_intent;	/* log intent item */
 	struct xfs_log_item		*dfp_done;	/* log done item */
+	const struct xfs_defer_op_type	*dfp_ops;
 	unsigned int			dfp_count;	/* # extent items */
 	unsigned int			dfp_flags;
-	enum xfs_defer_ops_type		dfp_type;
 };
 
 /*
@@ -61,6 +61,8 @@ void xfs_defer_move(struct xfs_trans *dtp, struct xfs_trans *stp);
 
 /* Description of a deferred type. */
 struct xfs_defer_op_type {
+	const char		*name;
+	unsigned int		max_items;
 	struct xfs_log_item *(*create_intent)(struct xfs_trans *tp,
 			struct list_head *items, unsigned int count, bool sort);
 	void (*abort_intent)(struct xfs_log_item *intent);
@@ -76,7 +78,6 @@ struct xfs_defer_op_type {
 	struct xfs_log_item *(*relog_intent)(struct xfs_trans *tp,
 			struct xfs_log_item *intent,
 			struct xfs_log_item *done_item);
-	unsigned int		max_items;
 };
 
 extern const struct xfs_defer_op_type xfs_bmap_update_defer_type;
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 4e0eaa2640e0d2..beae2de824507b 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -677,6 +677,7 @@ xfs_attr_create_done(
 }
 
 const struct xfs_defer_op_type xfs_attr_defer_type = {
+	.name		= "attr",
 	.max_items	= 1,
 	.create_intent	= xfs_attr_create_intent,
 	.abort_intent	= xfs_attr_abort_intent,
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index bc48d733634a1f..f43abf0b648641 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -563,6 +563,7 @@ xfs_bmap_relog_intent(
 }
 
 const struct xfs_defer_op_type xfs_bmap_update_defer_type = {
+	.name		= "bmap",
 	.max_items	= XFS_BUI_MAX_FAST_EXTENTS,
 	.create_intent	= xfs_bmap_update_create_intent,
 	.abort_intent	= xfs_bmap_update_abort_intent,
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 3e3469504271eb..e67907a379c8e8 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -670,6 +670,7 @@ xfs_extent_free_relog_intent(
 }
 
 const struct xfs_defer_op_type xfs_extent_free_defer_type = {
+	.name		= "extent_free",
 	.max_items	= XFS_EFI_MAX_FAST_EXTENTS,
 	.create_intent	= xfs_extent_free_create_intent,
 	.abort_intent	= xfs_extent_free_abort_intent,
@@ -682,6 +683,7 @@ const struct xfs_defer_op_type xfs_extent_free_defer_type = {
 
 /* sub-type with special handling for AGFL deferred frees */
 const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
+	.name		= "agfl_free",
 	.max_items	= XFS_EFI_MAX_FAST_EXTENTS,
 	.create_intent	= xfs_extent_free_create_intent,
 	.abort_intent	= xfs_extent_free_abort_intent,
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 9974be81cb2bae..b08839550f34a3 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -523,6 +523,7 @@ xfs_refcount_relog_intent(
 }
 
 const struct xfs_defer_op_type xfs_refcount_update_defer_type = {
+	.name		= "refcount",
 	.max_items	= XFS_CUI_MAX_FAST_EXTENTS,
 	.create_intent	= xfs_refcount_update_create_intent,
 	.abort_intent	= xfs_refcount_update_abort_intent,
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 488c4a2a80a3bd..65b432eb5d025d 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -576,6 +576,7 @@ xfs_rmap_relog_intent(
 }
 
 const struct xfs_defer_op_type xfs_rmap_update_defer_type = {
+	.name		= "rmap",
 	.max_items	= XFS_RUI_MAX_FAST_EXTENTS,
 	.create_intent	= xfs_rmap_update_create_intent,
 	.abort_intent	= xfs_rmap_update_abort_intent,
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 516529c151ae1c..0efcdb79d10e51 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -2549,7 +2549,7 @@ DECLARE_EVENT_CLASS(xfs_defer_pending_class,
 	TP_ARGS(mp, dfp),
 	TP_STRUCT__entry(
 		__field(dev_t, dev)
-		__field(int, type)
+		__string(name, dfp->dfp_ops->name)
 		__field(void *, intent)
 		__field(unsigned int, flags)
 		__field(char, committed)
@@ -2557,15 +2557,15 @@ DECLARE_EVENT_CLASS(xfs_defer_pending_class,
 	),
 	TP_fast_assign(
 		__entry->dev = mp ? mp->m_super->s_dev : 0;
-		__entry->type = dfp->dfp_type;
+		__assign_str(name, dfp->dfp_ops->name);
 		__entry->intent = dfp->dfp_intent;
 		__entry->flags = dfp->dfp_flags;
 		__entry->committed = dfp->dfp_done != NULL;
 		__entry->nr = dfp->dfp_count;
 	),
-	TP_printk("dev %d:%d optype %d intent %p flags %s committed %d nr %d",
+	TP_printk("dev %d:%d optype %s intent %p flags %s committed %d nr %d",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
-		  __entry->type,
+		  __get_str(name),
 		  __entry->intent,
 		  __print_flags(__entry->flags, "|", XFS_DEFER_PENDING_STRINGS),
 		  __entry->committed,
@@ -2694,7 +2694,7 @@ DECLARE_EVENT_CLASS(xfs_defer_pending_item_class,
 	TP_ARGS(mp, dfp, item),
 	TP_STRUCT__entry(
 		__field(dev_t, dev)
-		__field(int, type)
+		__string(name, dfp->dfp_ops->name)
 		__field(void *, intent)
 		__field(void *, item)
 		__field(char, committed)
@@ -2703,16 +2703,16 @@ DECLARE_EVENT_CLASS(xfs_defer_pending_item_class,
 	),
 	TP_fast_assign(
 		__entry->dev = mp ? mp->m_super->s_dev : 0;
-		__entry->type = dfp->dfp_type;
+		__assign_str(name, dfp->dfp_ops->name);
 		__entry->intent = dfp->dfp_intent;
 		__entry->item = item;
 		__entry->committed = dfp->dfp_done != NULL;
 		__entry->flags = dfp->dfp_flags;
 		__entry->nr = dfp->dfp_count;
 	),
-	TP_printk("dev %d:%d optype %d intent %p item %p flags %s committed %d nr %d",
+	TP_printk("dev %d:%d optype %s intent %p item %p flags %s committed %d nr %d",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
-		  __entry->type,
+		  __get_str(name),
 		  __entry->intent,
 		  __entry->item,
 		  __print_flags(__entry->flags, "|", XFS_DEFER_PENDING_STRINGS),
-- 
2.39.2

Re: [PATCH 3/5] xfs: store an ops pointer in struct xfs_defer_pending

[Darrick J. Wong]

On Wed, Dec 13, 2023 at 10:06:31AM +0100, Christoph Hellwig wrote:
> The dfp_type field in struct xfs_defer_pending is only used to either
> look up the operations associated with the pending word or in trace
> points.  Replace it with a direct pointer to the operations vector,
> and store a pretty name in the vector for tracing.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Straightforward substitution, so
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_defer.c  | 43 +++++++++++++++-----------------------
>  fs/xfs/libxfs/xfs_defer.h  |  5 +++--
>  fs/xfs/xfs_attr_item.c     |  1 +
>  fs/xfs/xfs_bmap_item.c     |  1 +
>  fs/xfs/xfs_extfree_item.c  |  2 ++
>  fs/xfs/xfs_refcount_item.c |  1 +
>  fs/xfs/xfs_rmap_item.c     |  1 +
>  fs/xfs/xfs_trace.h         | 16 +++++++-------
>  8 files changed, 34 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index ecc2f7ec699169..e70881ae5cc597 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -251,7 +251,6 @@ xfs_defer_create_done(
>  	struct xfs_trans		*tp,
>  	struct xfs_defer_pending	*dfp)
>  {
> -	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
>  	struct xfs_log_item		*lip;
>  
>  	/* If there is no log intent item, there can be no log done item. */
> @@ -266,7 +265,7 @@ xfs_defer_create_done(
>  	 * 2.) shuts down the filesystem
>  	 */
>  	tp->t_flags |= XFS_TRANS_DIRTY;
> -	lip = ops->create_done(tp, dfp->dfp_intent, dfp->dfp_count);
> +	lip = dfp->dfp_ops->create_done(tp, dfp->dfp_intent, dfp->dfp_count);
>  	if (!lip)
>  		return;
>  
> @@ -287,13 +286,13 @@ xfs_defer_create_intent(
>  	struct xfs_defer_pending	*dfp,
>  	bool				sort)
>  {
> -	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
>  	struct xfs_log_item		*lip;
>  
>  	if (dfp->dfp_intent)
>  		return 1;
>  
> -	lip = ops->create_intent(tp, &dfp->dfp_work, dfp->dfp_count, sort);
> +	lip = dfp->dfp_ops->create_intent(tp, &dfp->dfp_work, dfp->dfp_count,
> +			sort);
>  	if (!lip)
>  		return 0;
>  	if (IS_ERR(lip))
> @@ -338,12 +337,10 @@ xfs_defer_pending_abort(
>  	struct xfs_mount		*mp,
>  	struct xfs_defer_pending	*dfp)
>  {
> -	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
> -
>  	trace_xfs_defer_pending_abort(mp, dfp);
>  
>  	if (dfp->dfp_intent && !dfp->dfp_done) {
> -		ops->abort_intent(dfp->dfp_intent);
> +		dfp->dfp_ops->abort_intent(dfp->dfp_intent);
>  		dfp->dfp_intent = NULL;
>  	}
>  }
> @@ -353,7 +350,6 @@ xfs_defer_pending_cancel_work(
>  	struct xfs_mount		*mp,
>  	struct xfs_defer_pending	*dfp)
>  {
> -	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
>  	struct list_head		*pwi;
>  	struct list_head		*n;
>  
> @@ -364,7 +360,7 @@ xfs_defer_pending_cancel_work(
>  		list_del(pwi);
>  		dfp->dfp_count--;
>  		trace_xfs_defer_cancel_item(mp, dfp, pwi);
> -		ops->cancel_item(pwi);
> +		dfp->dfp_ops->cancel_item(pwi);
>  	}
>  	ASSERT(dfp->dfp_count == 0);
>  	kmem_cache_free(xfs_defer_pending_cache, dfp);
> @@ -522,11 +518,10 @@ xfs_defer_relog_intent(
>  	struct xfs_defer_pending	*dfp)
>  {
>  	struct xfs_log_item		*lip;
> -	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
>  
>  	xfs_defer_create_done(tp, dfp);
>  
> -	lip = ops->relog_intent(tp, dfp->dfp_intent, dfp->dfp_done);
> +	lip = dfp->dfp_ops->relog_intent(tp, dfp->dfp_intent, dfp->dfp_done);
>  	if (lip) {
>  		xfs_trans_add_item(tp, lip);
>  		set_bit(XFS_LI_DIRTY, &lip->li_flags);
> @@ -593,7 +588,7 @@ xfs_defer_finish_one(
>  	struct xfs_trans		*tp,
>  	struct xfs_defer_pending	*dfp)
>  {
> -	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
> +	const struct xfs_defer_op_type	*ops = dfp->dfp_ops;
>  	struct xfs_btree_cur		*state = NULL;
>  	struct list_head		*li, *n;
>  	int				error;
> @@ -790,7 +785,6 @@ xfs_defer_cancel(
>  static inline struct xfs_defer_pending *
>  xfs_defer_find_last(
>  	struct xfs_trans		*tp,
> -	enum xfs_defer_ops_type		type,
>  	const struct xfs_defer_op_type	*ops)
>  {
>  	struct xfs_defer_pending	*dfp = NULL;
> @@ -803,7 +797,7 @@ xfs_defer_find_last(
>  			dfp_list);
>  
>  	/* Wrong type? */
> -	if (dfp->dfp_type != type)
> +	if (dfp->dfp_ops != ops)
>  		return NULL;
>  	return dfp;
>  }
> @@ -836,13 +830,13 @@ xfs_defer_can_append(
>  static inline struct xfs_defer_pending *
>  xfs_defer_alloc(
>  	struct xfs_trans		*tp,
> -	enum xfs_defer_ops_type		type)
> +	const struct xfs_defer_op_type	*ops)
>  {
>  	struct xfs_defer_pending	*dfp;
>  
>  	dfp = kmem_cache_zalloc(xfs_defer_pending_cache,
>  			GFP_NOFS | __GFP_NOFAIL);
> -	dfp->dfp_type = type;
> +	dfp->dfp_ops = ops;
>  	INIT_LIST_HEAD(&dfp->dfp_work);
>  	list_add_tail(&dfp->dfp_list, &tp->t_dfops);
>  
> @@ -862,9 +856,9 @@ xfs_defer_add(
>  	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
>  	BUILD_BUG_ON(ARRAY_SIZE(defer_op_types) != XFS_DEFER_OPS_TYPE_MAX);
>  
> -	dfp = xfs_defer_find_last(tp, type, ops);
> +	dfp = xfs_defer_find_last(tp, ops);
>  	if (!dfp || !xfs_defer_can_append(dfp, ops))
> -		dfp = xfs_defer_alloc(tp, type);
> +		dfp = xfs_defer_alloc(tp, ops);
>  
>  	xfs_defer_add_item(dfp, li);
>  	trace_xfs_defer_add_item(tp->t_mountp, dfp, li);
> @@ -880,17 +874,15 @@ xfs_defer_add_barrier(
>  	struct xfs_trans		*tp)
>  {
>  	struct xfs_defer_pending	*dfp;
> -	const enum xfs_defer_ops_type	type = XFS_DEFER_OPS_TYPE_BARRIER;
> -	const struct xfs_defer_op_type	*ops = defer_op_types[type];
>  
>  	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
>  
>  	/* If the last defer op added was a barrier, we're done. */
> -	dfp = xfs_defer_find_last(tp, type, ops);
> +	dfp = xfs_defer_find_last(tp, &xfs_barrier_defer_type);
>  	if (dfp)
>  		return;
>  
> -	xfs_defer_alloc(tp, type);
> +	xfs_defer_alloc(tp, &xfs_barrier_defer_type);
>  
>  	trace_xfs_defer_add_item(tp->t_mountp, dfp, NULL);
>  }
> @@ -909,7 +901,7 @@ xfs_defer_start_recovery(
>  
>  	dfp = kmem_cache_zalloc(xfs_defer_pending_cache,
>  			GFP_NOFS | __GFP_NOFAIL);
> -	dfp->dfp_type = dfp_type;
> +	dfp->dfp_ops = defer_op_types[dfp_type];
>  	dfp->dfp_intent = lip;
>  	INIT_LIST_HEAD(&dfp->dfp_work);
>  	list_add_tail(&dfp->dfp_list, r_dfops);
> @@ -935,13 +927,12 @@ xfs_defer_finish_recovery(
>  	struct xfs_defer_pending	*dfp,
>  	struct list_head		*capture_list)
>  {
> -	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
>  	int				error;
>  
> -	error = ops->recover_work(dfp, capture_list);
> +	error = dfp->dfp_ops->recover_work(dfp, capture_list);
>  	if (error)
>  		trace_xlog_intent_recovery_failed(mp, error,
> -				ops->recover_work);
> +				dfp->dfp_ops->recover_work);
>  	return error;
>  }
>  
> diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
> index 5b1990ef3e5df4..957a06278e880d 100644
> --- a/fs/xfs/libxfs/xfs_defer.h
> +++ b/fs/xfs/libxfs/xfs_defer.h
> @@ -34,9 +34,9 @@ struct xfs_defer_pending {
>  	struct list_head		dfp_work;	/* work items */
>  	struct xfs_log_item		*dfp_intent;	/* log intent item */
>  	struct xfs_log_item		*dfp_done;	/* log done item */
> +	const struct xfs_defer_op_type	*dfp_ops;
>  	unsigned int			dfp_count;	/* # extent items */
>  	unsigned int			dfp_flags;
> -	enum xfs_defer_ops_type		dfp_type;
>  };
>  
>  /*
> @@ -61,6 +61,8 @@ void xfs_defer_move(struct xfs_trans *dtp, struct xfs_trans *stp);
>  
>  /* Description of a deferred type. */
>  struct xfs_defer_op_type {
> +	const char		*name;
> +	unsigned int		max_items;
>  	struct xfs_log_item *(*create_intent)(struct xfs_trans *tp,
>  			struct list_head *items, unsigned int count, bool sort);
>  	void (*abort_intent)(struct xfs_log_item *intent);
> @@ -76,7 +78,6 @@ struct xfs_defer_op_type {
>  	struct xfs_log_item *(*relog_intent)(struct xfs_trans *tp,
>  			struct xfs_log_item *intent,
>  			struct xfs_log_item *done_item);
> -	unsigned int		max_items;
>  };
>  
>  extern const struct xfs_defer_op_type xfs_bmap_update_defer_type;
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 4e0eaa2640e0d2..beae2de824507b 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -677,6 +677,7 @@ xfs_attr_create_done(
>  }
>  
>  const struct xfs_defer_op_type xfs_attr_defer_type = {
> +	.name		= "attr",
>  	.max_items	= 1,
>  	.create_intent	= xfs_attr_create_intent,
>  	.abort_intent	= xfs_attr_abort_intent,
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index bc48d733634a1f..f43abf0b648641 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -563,6 +563,7 @@ xfs_bmap_relog_intent(
>  }
>  
>  const struct xfs_defer_op_type xfs_bmap_update_defer_type = {
> +	.name		= "bmap",
>  	.max_items	= XFS_BUI_MAX_FAST_EXTENTS,
>  	.create_intent	= xfs_bmap_update_create_intent,
>  	.abort_intent	= xfs_bmap_update_abort_intent,
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 3e3469504271eb..e67907a379c8e8 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -670,6 +670,7 @@ xfs_extent_free_relog_intent(
>  }
>  
>  const struct xfs_defer_op_type xfs_extent_free_defer_type = {
> +	.name		= "extent_free",
>  	.max_items	= XFS_EFI_MAX_FAST_EXTENTS,
>  	.create_intent	= xfs_extent_free_create_intent,
>  	.abort_intent	= xfs_extent_free_abort_intent,
> @@ -682,6 +683,7 @@ const struct xfs_defer_op_type xfs_extent_free_defer_type = {
>  
>  /* sub-type with special handling for AGFL deferred frees */
>  const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
> +	.name		= "agfl_free",
>  	.max_items	= XFS_EFI_MAX_FAST_EXTENTS,
>  	.create_intent	= xfs_extent_free_create_intent,
>  	.abort_intent	= xfs_extent_free_abort_intent,
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index 9974be81cb2bae..b08839550f34a3 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -523,6 +523,7 @@ xfs_refcount_relog_intent(
>  }
>  
>  const struct xfs_defer_op_type xfs_refcount_update_defer_type = {
> +	.name		= "refcount",
>  	.max_items	= XFS_CUI_MAX_FAST_EXTENTS,
>  	.create_intent	= xfs_refcount_update_create_intent,
>  	.abort_intent	= xfs_refcount_update_abort_intent,
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index 488c4a2a80a3bd..65b432eb5d025d 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -576,6 +576,7 @@ xfs_rmap_relog_intent(
>  }
>  
>  const struct xfs_defer_op_type xfs_rmap_update_defer_type = {
> +	.name		= "rmap",
>  	.max_items	= XFS_RUI_MAX_FAST_EXTENTS,
>  	.create_intent	= xfs_rmap_update_create_intent,
>  	.abort_intent	= xfs_rmap_update_abort_intent,
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 516529c151ae1c..0efcdb79d10e51 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -2549,7 +2549,7 @@ DECLARE_EVENT_CLASS(xfs_defer_pending_class,
>  	TP_ARGS(mp, dfp),
>  	TP_STRUCT__entry(
>  		__field(dev_t, dev)
> -		__field(int, type)
> +		__string(name, dfp->dfp_ops->name)
>  		__field(void *, intent)
>  		__field(unsigned int, flags)
>  		__field(char, committed)
> @@ -2557,15 +2557,15 @@ DECLARE_EVENT_CLASS(xfs_defer_pending_class,
>  	),
>  	TP_fast_assign(
>  		__entry->dev = mp ? mp->m_super->s_dev : 0;
> -		__entry->type = dfp->dfp_type;
> +		__assign_str(name, dfp->dfp_ops->name);
>  		__entry->intent = dfp->dfp_intent;
>  		__entry->flags = dfp->dfp_flags;
>  		__entry->committed = dfp->dfp_done != NULL;
>  		__entry->nr = dfp->dfp_count;
>  	),
> -	TP_printk("dev %d:%d optype %d intent %p flags %s committed %d nr %d",
> +	TP_printk("dev %d:%d optype %s intent %p flags %s committed %d nr %d",
>  		  MAJOR(__entry->dev), MINOR(__entry->dev),
> -		  __entry->type,
> +		  __get_str(name),
>  		  __entry->intent,
>  		  __print_flags(__entry->flags, "|", XFS_DEFER_PENDING_STRINGS),
>  		  __entry->committed,
> @@ -2694,7 +2694,7 @@ DECLARE_EVENT_CLASS(xfs_defer_pending_item_class,
>  	TP_ARGS(mp, dfp, item),
>  	TP_STRUCT__entry(
>  		__field(dev_t, dev)
> -		__field(int, type)
> +		__string(name, dfp->dfp_ops->name)
>  		__field(void *, intent)
>  		__field(void *, item)
>  		__field(char, committed)
> @@ -2703,16 +2703,16 @@ DECLARE_EVENT_CLASS(xfs_defer_pending_item_class,
>  	),
>  	TP_fast_assign(
>  		__entry->dev = mp ? mp->m_super->s_dev : 0;
> -		__entry->type = dfp->dfp_type;
> +		__assign_str(name, dfp->dfp_ops->name);
>  		__entry->intent = dfp->dfp_intent;
>  		__entry->item = item;
>  		__entry->committed = dfp->dfp_done != NULL;
>  		__entry->flags = dfp->dfp_flags;
>  		__entry->nr = dfp->dfp_count;
>  	),
> -	TP_printk("dev %d:%d optype %d intent %p item %p flags %s committed %d nr %d",
> +	TP_printk("dev %d:%d optype %s intent %p item %p flags %s committed %d nr %d",
>  		  MAJOR(__entry->dev), MINOR(__entry->dev),
> -		  __entry->type,
> +		  __get_str(name),
>  		  __entry->intent,
>  		  __entry->item,
>  		  __print_flags(__entry->flags, "|", XFS_DEFER_PENDING_STRINGS),
> -- 
> 2.39.2
> 
> 

[PATCH 4/5] xfs: pass the defer ops instead of type to xfs_defer_start_recovery

[Christoph Hellwig]

xfs_defer_start_recovery is only called from xlog_recover_intent_item,
and the callers of that all have the actual xfs_defer_ops_type operation
vector at hand.  Pass that directly instead of looking it up from the
defer_op_types table.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_defer.c       | 6 +++---
 fs/xfs/libxfs/xfs_defer.h       | 2 +-
 fs/xfs/libxfs/xfs_log_recover.h | 3 ++-
 fs/xfs/xfs_attr_item.c          | 2 +-
 fs/xfs/xfs_bmap_item.c          | 2 +-
 fs/xfs/xfs_extfree_item.c       | 2 +-
 fs/xfs/xfs_log_recover.c        | 4 ++--
 fs/xfs/xfs_refcount_item.c      | 2 +-
 fs/xfs/xfs_rmap_item.c          | 2 +-
 9 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index e70881ae5cc597..6a6444ffe5544b 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -894,14 +894,14 @@ xfs_defer_add_barrier(
 void
 xfs_defer_start_recovery(
 	struct xfs_log_item		*lip,
-	enum xfs_defer_ops_type		dfp_type,
-	struct list_head		*r_dfops)
+	struct list_head		*r_dfops,
+	const struct xfs_defer_op_type *ops)
 {
 	struct xfs_defer_pending	*dfp;
 
 	dfp = kmem_cache_zalloc(xfs_defer_pending_cache,
 			GFP_NOFS | __GFP_NOFAIL);
-	dfp->dfp_ops = defer_op_types[dfp_type];
+	dfp->dfp_ops = ops;
 	dfp->dfp_intent = lip;
 	INIT_LIST_HEAD(&dfp->dfp_work);
 	list_add_tail(&dfp->dfp_list, r_dfops);
diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index 957a06278e880d..60de91b6639225 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -147,7 +147,7 @@ void xfs_defer_ops_capture_abort(struct xfs_mount *mp,
 void xfs_defer_resources_rele(struct xfs_defer_resources *dres);
 
 void xfs_defer_start_recovery(struct xfs_log_item *lip,
-		enum xfs_defer_ops_type dfp_type, struct list_head *r_dfops);
+		struct list_head *r_dfops, const struct xfs_defer_op_type *ops);
 void xfs_defer_cancel_recovery(struct xfs_mount *mp,
 		struct xfs_defer_pending *dfp);
 int xfs_defer_finish_recovery(struct xfs_mount *mp,
diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h
index c8e5d912895bcd..9fe7a9564bca96 100644
--- a/fs/xfs/libxfs/xfs_log_recover.h
+++ b/fs/xfs/libxfs/xfs_log_recover.h
@@ -11,6 +11,7 @@
  * define how recovery should work for that type of log item.
  */
 struct xlog_recover_item;
+struct xfs_defer_op_type;
 
 /* Sorting hat for log items as they're read in. */
 enum xlog_recover_reorder {
@@ -156,7 +157,7 @@ xlog_recover_resv(const struct xfs_trans_res *r)
 struct xfs_defer_pending;
 
 void xlog_recover_intent_item(struct xlog *log, struct xfs_log_item *lip,
-		xfs_lsn_t lsn, unsigned int dfp_type);
+		xfs_lsn_t lsn, const struct xfs_defer_op_type *ops);
 int xlog_recover_finish_intent(struct xfs_trans *tp,
 		struct xfs_defer_pending *dfp);
 
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index beae2de824507b..9e02111bd89010 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -759,7 +759,7 @@ xlog_recover_attri_commit_pass2(
 	memcpy(&attrip->attri_format, attri_formatp, len);
 
 	xlog_recover_intent_item(log, &attrip->attri_item, lsn,
-			XFS_DEFER_OPS_TYPE_ATTR);
+			&xfs_attr_defer_type);
 	xfs_attri_log_nameval_put(nv);
 	return 0;
 }
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index f43abf0b648641..52fb8a148b7dcb 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -650,7 +650,7 @@ xlog_recover_bui_commit_pass2(
 	atomic_set(&buip->bui_next_extent, bui_formatp->bui_nextents);
 
 	xlog_recover_intent_item(log, &buip->bui_item, lsn,
-			XFS_DEFER_OPS_TYPE_BMAP);
+			&xfs_bmap_update_defer_type);
 	return 0;
 }
 
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index e67907a379c8e8..1d1185fca6a58e 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -747,7 +747,7 @@ xlog_recover_efi_commit_pass2(
 	atomic_set(&efip->efi_next_extent, efi_formatp->efi_nextents);
 
 	xlog_recover_intent_item(log, &efip->efi_item, lsn,
-			XFS_DEFER_OPS_TYPE_FREE);
+			&xfs_extent_free_defer_type);
 	return 0;
 }
 
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index c18692af2c651c..1251c81e55f982 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1942,11 +1942,11 @@ xlog_recover_intent_item(
 	struct xlog			*log,
 	struct xfs_log_item		*lip,
 	xfs_lsn_t			lsn,
-	unsigned int			dfp_type)
+	const struct xfs_defer_op_type	*ops)
 {
 	ASSERT(xlog_item_is_intent(lip));
 
-	xfs_defer_start_recovery(lip, dfp_type, &log->r_dfops);
+	xfs_defer_start_recovery(lip, &log->r_dfops, ops);
 
 	/*
 	 * Insert the intent into the AIL directly and drop one reference so
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index b08839550f34a3..20ad8086da60be 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -605,7 +605,7 @@ xlog_recover_cui_commit_pass2(
 	atomic_set(&cuip->cui_next_extent, cui_formatp->cui_nextents);
 
 	xlog_recover_intent_item(log, &cuip->cui_item, lsn,
-			XFS_DEFER_OPS_TYPE_REFCOUNT);
+			&xfs_refcount_update_defer_type);
 	return 0;
 }
 
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 65b432eb5d025d..79ad0087aecaf5 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -658,7 +658,7 @@ xlog_recover_rui_commit_pass2(
 	atomic_set(&ruip->rui_next_extent, rui_formatp->rui_nextents);
 
 	xlog_recover_intent_item(log, &ruip->rui_item, lsn,
-			XFS_DEFER_OPS_TYPE_RMAP);
+			&xfs_rmap_update_defer_type);
 	return 0;
 }
 
-- 
2.39.2

Re: [PATCH 4/5] xfs: pass the defer ops instead of type to xfs_defer_start_recovery

[Darrick J. Wong]

On Wed, Dec 13, 2023 at 10:06:32AM +0100, Christoph Hellwig wrote:
> xfs_defer_start_recovery is only called from xlog_recover_intent_item,
> and the callers of that all have the actual xfs_defer_ops_type operation
> vector at hand.  Pass that directly instead of looking it up from the
> defer_op_types table.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_defer.c       | 6 +++---
>  fs/xfs/libxfs/xfs_defer.h       | 2 +-
>  fs/xfs/libxfs/xfs_log_recover.h | 3 ++-
>  fs/xfs/xfs_attr_item.c          | 2 +-
>  fs/xfs/xfs_bmap_item.c          | 2 +-
>  fs/xfs/xfs_extfree_item.c       | 2 +-
>  fs/xfs/xfs_log_recover.c        | 4 ++--
>  fs/xfs/xfs_refcount_item.c      | 2 +-
>  fs/xfs/xfs_rmap_item.c          | 2 +-
>  9 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index e70881ae5cc597..6a6444ffe5544b 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -894,14 +894,14 @@ xfs_defer_add_barrier(
>  void
>  xfs_defer_start_recovery(
>  	struct xfs_log_item		*lip,
> -	enum xfs_defer_ops_type		dfp_type,
> -	struct list_head		*r_dfops)
> +	struct list_head		*r_dfops,
> +	const struct xfs_defer_op_type *ops)

Nit: tab before the parameter name    ^

With that fixed,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  {
>  	struct xfs_defer_pending	*dfp;
>  
>  	dfp = kmem_cache_zalloc(xfs_defer_pending_cache,
>  			GFP_NOFS | __GFP_NOFAIL);
> -	dfp->dfp_ops = defer_op_types[dfp_type];
> +	dfp->dfp_ops = ops;
>  	dfp->dfp_intent = lip;
>  	INIT_LIST_HEAD(&dfp->dfp_work);
>  	list_add_tail(&dfp->dfp_list, r_dfops);
> diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
> index 957a06278e880d..60de91b6639225 100644
> --- a/fs/xfs/libxfs/xfs_defer.h
> +++ b/fs/xfs/libxfs/xfs_defer.h
> @@ -147,7 +147,7 @@ void xfs_defer_ops_capture_abort(struct xfs_mount *mp,
>  void xfs_defer_resources_rele(struct xfs_defer_resources *dres);
>  
>  void xfs_defer_start_recovery(struct xfs_log_item *lip,
> -		enum xfs_defer_ops_type dfp_type, struct list_head *r_dfops);
> +		struct list_head *r_dfops, const struct xfs_defer_op_type *ops);
>  void xfs_defer_cancel_recovery(struct xfs_mount *mp,
>  		struct xfs_defer_pending *dfp);
>  int xfs_defer_finish_recovery(struct xfs_mount *mp,
> diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h
> index c8e5d912895bcd..9fe7a9564bca96 100644
> --- a/fs/xfs/libxfs/xfs_log_recover.h
> +++ b/fs/xfs/libxfs/xfs_log_recover.h
> @@ -11,6 +11,7 @@
>   * define how recovery should work for that type of log item.
>   */
>  struct xlog_recover_item;
> +struct xfs_defer_op_type;
>  
>  /* Sorting hat for log items as they're read in. */
>  enum xlog_recover_reorder {
> @@ -156,7 +157,7 @@ xlog_recover_resv(const struct xfs_trans_res *r)
>  struct xfs_defer_pending;
>  
>  void xlog_recover_intent_item(struct xlog *log, struct xfs_log_item *lip,
> -		xfs_lsn_t lsn, unsigned int dfp_type);
> +		xfs_lsn_t lsn, const struct xfs_defer_op_type *ops);
>  int xlog_recover_finish_intent(struct xfs_trans *tp,
>  		struct xfs_defer_pending *dfp);
>  
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index beae2de824507b..9e02111bd89010 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -759,7 +759,7 @@ xlog_recover_attri_commit_pass2(
>  	memcpy(&attrip->attri_format, attri_formatp, len);
>  
>  	xlog_recover_intent_item(log, &attrip->attri_item, lsn,
> -			XFS_DEFER_OPS_TYPE_ATTR);
> +			&xfs_attr_defer_type);
>  	xfs_attri_log_nameval_put(nv);
>  	return 0;
>  }
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index f43abf0b648641..52fb8a148b7dcb 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -650,7 +650,7 @@ xlog_recover_bui_commit_pass2(
>  	atomic_set(&buip->bui_next_extent, bui_formatp->bui_nextents);
>  
>  	xlog_recover_intent_item(log, &buip->bui_item, lsn,
> -			XFS_DEFER_OPS_TYPE_BMAP);
> +			&xfs_bmap_update_defer_type);
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index e67907a379c8e8..1d1185fca6a58e 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -747,7 +747,7 @@ xlog_recover_efi_commit_pass2(
>  	atomic_set(&efip->efi_next_extent, efi_formatp->efi_nextents);
>  
>  	xlog_recover_intent_item(log, &efip->efi_item, lsn,
> -			XFS_DEFER_OPS_TYPE_FREE);
> +			&xfs_extent_free_defer_type);
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index c18692af2c651c..1251c81e55f982 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1942,11 +1942,11 @@ xlog_recover_intent_item(
>  	struct xlog			*log,
>  	struct xfs_log_item		*lip,
>  	xfs_lsn_t			lsn,
> -	unsigned int			dfp_type)
> +	const struct xfs_defer_op_type	*ops)
>  {
>  	ASSERT(xlog_item_is_intent(lip));
>  
> -	xfs_defer_start_recovery(lip, dfp_type, &log->r_dfops);
> +	xfs_defer_start_recovery(lip, &log->r_dfops, ops);
>  
>  	/*
>  	 * Insert the intent into the AIL directly and drop one reference so
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index b08839550f34a3..20ad8086da60be 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -605,7 +605,7 @@ xlog_recover_cui_commit_pass2(
>  	atomic_set(&cuip->cui_next_extent, cui_formatp->cui_nextents);
>  
>  	xlog_recover_intent_item(log, &cuip->cui_item, lsn,
> -			XFS_DEFER_OPS_TYPE_REFCOUNT);
> +			&xfs_refcount_update_defer_type);
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index 65b432eb5d025d..79ad0087aecaf5 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -658,7 +658,7 @@ xlog_recover_rui_commit_pass2(
>  	atomic_set(&ruip->rui_next_extent, rui_formatp->rui_nextents);
>  
>  	xlog_recover_intent_item(log, &ruip->rui_item, lsn,
> -			XFS_DEFER_OPS_TYPE_RMAP);
> +			&xfs_rmap_update_defer_type);
>  	return 0;
>  }
>  
> -- 
> 2.39.2
> 
> 

[PATCH 4/5 v1.1] xfs: pass the defer ops instead of type to xfs_defer_start_recovery

[Christoph Hellwig]

xfs_defer_start_recovery is only called from xlog_recover_intent_item,
and the callers of that all have the actual xfs_defer_ops_type operation
vector at hand.  Pass that directly instead of looking it up from the
defer_op_types table.

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

- fix a whitespace issue

 fs/xfs/libxfs/xfs_defer.c       | 6 +++---
 fs/xfs/libxfs/xfs_defer.h       | 2 +-
 fs/xfs/libxfs/xfs_log_recover.h | 3 ++-
 fs/xfs/xfs_attr_item.c          | 2 +-
 fs/xfs/xfs_bmap_item.c          | 2 +-
 fs/xfs/xfs_extfree_item.c       | 2 +-
 fs/xfs/xfs_log_recover.c        | 4 ++--
 fs/xfs/xfs_refcount_item.c      | 2 +-
 fs/xfs/xfs_rmap_item.c          | 2 +-
 9 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index e70881ae5cc597..dd964bf825eb89 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -894,14 +894,14 @@ xfs_defer_add_barrier(
 void
 xfs_defer_start_recovery(
 	struct xfs_log_item		*lip,
-	enum xfs_defer_ops_type		dfp_type,
-	struct list_head		*r_dfops)
+	struct list_head		*r_dfops,
+	const struct xfs_defer_op_type	*ops)
 {
 	struct xfs_defer_pending	*dfp;
 
 	dfp = kmem_cache_zalloc(xfs_defer_pending_cache,
 			GFP_NOFS | __GFP_NOFAIL);
-	dfp->dfp_ops = defer_op_types[dfp_type];
+	dfp->dfp_ops = ops;
 	dfp->dfp_intent = lip;
 	INIT_LIST_HEAD(&dfp->dfp_work);
 	list_add_tail(&dfp->dfp_list, r_dfops);
diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index 957a06278e880d..60de91b6639225 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -147,7 +147,7 @@ void xfs_defer_ops_capture_abort(struct xfs_mount *mp,
 void xfs_defer_resources_rele(struct xfs_defer_resources *dres);
 
 void xfs_defer_start_recovery(struct xfs_log_item *lip,
-		enum xfs_defer_ops_type dfp_type, struct list_head *r_dfops);
+		struct list_head *r_dfops, const struct xfs_defer_op_type *ops);
 void xfs_defer_cancel_recovery(struct xfs_mount *mp,
 		struct xfs_defer_pending *dfp);
 int xfs_defer_finish_recovery(struct xfs_mount *mp,
diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h
index c8e5d912895bcd..9fe7a9564bca96 100644
--- a/fs/xfs/libxfs/xfs_log_recover.h
+++ b/fs/xfs/libxfs/xfs_log_recover.h
@@ -11,6 +11,7 @@
  * define how recovery should work for that type of log item.
  */
 struct xlog_recover_item;
+struct xfs_defer_op_type;
 
 /* Sorting hat for log items as they're read in. */
 enum xlog_recover_reorder {
@@ -156,7 +157,7 @@ xlog_recover_resv(const struct xfs_trans_res *r)
 struct xfs_defer_pending;
 
 void xlog_recover_intent_item(struct xlog *log, struct xfs_log_item *lip,
-		xfs_lsn_t lsn, unsigned int dfp_type);
+		xfs_lsn_t lsn, const struct xfs_defer_op_type *ops);
 int xlog_recover_finish_intent(struct xfs_trans *tp,
 		struct xfs_defer_pending *dfp);
 
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index beae2de824507b..9e02111bd89010 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -759,7 +759,7 @@ xlog_recover_attri_commit_pass2(
 	memcpy(&attrip->attri_format, attri_formatp, len);
 
 	xlog_recover_intent_item(log, &attrip->attri_item, lsn,
-			XFS_DEFER_OPS_TYPE_ATTR);
+			&xfs_attr_defer_type);
 	xfs_attri_log_nameval_put(nv);
 	return 0;
 }
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index f43abf0b648641..52fb8a148b7dcb 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -650,7 +650,7 @@ xlog_recover_bui_commit_pass2(
 	atomic_set(&buip->bui_next_extent, bui_formatp->bui_nextents);
 
 	xlog_recover_intent_item(log, &buip->bui_item, lsn,
-			XFS_DEFER_OPS_TYPE_BMAP);
+			&xfs_bmap_update_defer_type);
 	return 0;
 }
 
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index e67907a379c8e8..1d1185fca6a58e 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -747,7 +747,7 @@ xlog_recover_efi_commit_pass2(
 	atomic_set(&efip->efi_next_extent, efi_formatp->efi_nextents);
 
 	xlog_recover_intent_item(log, &efip->efi_item, lsn,
-			XFS_DEFER_OPS_TYPE_FREE);
+			&xfs_extent_free_defer_type);
 	return 0;
 }
 
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index c18692af2c651c..1251c81e55f982 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1942,11 +1942,11 @@ xlog_recover_intent_item(
 	struct xlog			*log,
 	struct xfs_log_item		*lip,
 	xfs_lsn_t			lsn,
-	unsigned int			dfp_type)
+	const struct xfs_defer_op_type	*ops)
 {
 	ASSERT(xlog_item_is_intent(lip));
 
-	xfs_defer_start_recovery(lip, dfp_type, &log->r_dfops);
+	xfs_defer_start_recovery(lip, &log->r_dfops, ops);
 
 	/*
 	 * Insert the intent into the AIL directly and drop one reference so
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index b08839550f34a3..20ad8086da60be 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -605,7 +605,7 @@ xlog_recover_cui_commit_pass2(
 	atomic_set(&cuip->cui_next_extent, cui_formatp->cui_nextents);
 
 	xlog_recover_intent_item(log, &cuip->cui_item, lsn,
-			XFS_DEFER_OPS_TYPE_REFCOUNT);
+			&xfs_refcount_update_defer_type);
 	return 0;
 }
 
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 65b432eb5d025d..79ad0087aecaf5 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -658,7 +658,7 @@ xlog_recover_rui_commit_pass2(
 	atomic_set(&ruip->rui_next_extent, rui_formatp->rui_nextents);
 
 	xlog_recover_intent_item(log, &ruip->rui_item, lsn,
-			XFS_DEFER_OPS_TYPE_RMAP);
+			&xfs_rmap_update_defer_type);
 	return 0;
 }
 
-- 
2.39.2

Re: [PATCH 4/5 v1.1] xfs: pass the defer ops instead of type to xfs_defer_start_recovery

[Darrick J. Wong]

On Thu, Dec 14, 2023 at 06:16:32AM +0100, Christoph Hellwig wrote:
> xfs_defer_start_recovery is only called from xlog_recover_intent_item,
> and the callers of that all have the actual xfs_defer_ops_type operation
> vector at hand.  Pass that directly instead of looking it up from the
> defer_op_types table.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
> 
> - fix a whitespace issue
> 
>  fs/xfs/libxfs/xfs_defer.c       | 6 +++---
>  fs/xfs/libxfs/xfs_defer.h       | 2 +-
>  fs/xfs/libxfs/xfs_log_recover.h | 3 ++-
>  fs/xfs/xfs_attr_item.c          | 2 +-
>  fs/xfs/xfs_bmap_item.c          | 2 +-
>  fs/xfs/xfs_extfree_item.c       | 2 +-
>  fs/xfs/xfs_log_recover.c        | 4 ++--
>  fs/xfs/xfs_refcount_item.c      | 2 +-
>  fs/xfs/xfs_rmap_item.c          | 2 +-
>  9 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index e70881ae5cc597..dd964bf825eb89 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -894,14 +894,14 @@ xfs_defer_add_barrier(
>  void
>  xfs_defer_start_recovery(
>  	struct xfs_log_item		*lip,
> -	enum xfs_defer_ops_type		dfp_type,
> -	struct list_head		*r_dfops)
> +	struct list_head		*r_dfops,
> +	const struct xfs_defer_op_type	*ops)
>  {
>  	struct xfs_defer_pending	*dfp;
>  
>  	dfp = kmem_cache_zalloc(xfs_defer_pending_cache,
>  			GFP_NOFS | __GFP_NOFAIL);
> -	dfp->dfp_ops = defer_op_types[dfp_type];
> +	dfp->dfp_ops = ops;
>  	dfp->dfp_intent = lip;
>  	INIT_LIST_HEAD(&dfp->dfp_work);
>  	list_add_tail(&dfp->dfp_list, r_dfops);
> diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
> index 957a06278e880d..60de91b6639225 100644
> --- a/fs/xfs/libxfs/xfs_defer.h
> +++ b/fs/xfs/libxfs/xfs_defer.h
> @@ -147,7 +147,7 @@ void xfs_defer_ops_capture_abort(struct xfs_mount *mp,
>  void xfs_defer_resources_rele(struct xfs_defer_resources *dres);
>  
>  void xfs_defer_start_recovery(struct xfs_log_item *lip,
> -		enum xfs_defer_ops_type dfp_type, struct list_head *r_dfops);
> +		struct list_head *r_dfops, const struct xfs_defer_op_type *ops);
>  void xfs_defer_cancel_recovery(struct xfs_mount *mp,
>  		struct xfs_defer_pending *dfp);
>  int xfs_defer_finish_recovery(struct xfs_mount *mp,
> diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h
> index c8e5d912895bcd..9fe7a9564bca96 100644
> --- a/fs/xfs/libxfs/xfs_log_recover.h
> +++ b/fs/xfs/libxfs/xfs_log_recover.h
> @@ -11,6 +11,7 @@
>   * define how recovery should work for that type of log item.
>   */
>  struct xlog_recover_item;
> +struct xfs_defer_op_type;
>  
>  /* Sorting hat for log items as they're read in. */
>  enum xlog_recover_reorder {
> @@ -156,7 +157,7 @@ xlog_recover_resv(const struct xfs_trans_res *r)
>  struct xfs_defer_pending;
>  
>  void xlog_recover_intent_item(struct xlog *log, struct xfs_log_item *lip,
> -		xfs_lsn_t lsn, unsigned int dfp_type);
> +		xfs_lsn_t lsn, const struct xfs_defer_op_type *ops);
>  int xlog_recover_finish_intent(struct xfs_trans *tp,
>  		struct xfs_defer_pending *dfp);
>  
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index beae2de824507b..9e02111bd89010 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -759,7 +759,7 @@ xlog_recover_attri_commit_pass2(
>  	memcpy(&attrip->attri_format, attri_formatp, len);
>  
>  	xlog_recover_intent_item(log, &attrip->attri_item, lsn,
> -			XFS_DEFER_OPS_TYPE_ATTR);
> +			&xfs_attr_defer_type);
>  	xfs_attri_log_nameval_put(nv);
>  	return 0;
>  }
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index f43abf0b648641..52fb8a148b7dcb 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -650,7 +650,7 @@ xlog_recover_bui_commit_pass2(
>  	atomic_set(&buip->bui_next_extent, bui_formatp->bui_nextents);
>  
>  	xlog_recover_intent_item(log, &buip->bui_item, lsn,
> -			XFS_DEFER_OPS_TYPE_BMAP);
> +			&xfs_bmap_update_defer_type);
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index e67907a379c8e8..1d1185fca6a58e 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -747,7 +747,7 @@ xlog_recover_efi_commit_pass2(
>  	atomic_set(&efip->efi_next_extent, efi_formatp->efi_nextents);
>  
>  	xlog_recover_intent_item(log, &efip->efi_item, lsn,
> -			XFS_DEFER_OPS_TYPE_FREE);
> +			&xfs_extent_free_defer_type);
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index c18692af2c651c..1251c81e55f982 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1942,11 +1942,11 @@ xlog_recover_intent_item(
>  	struct xlog			*log,
>  	struct xfs_log_item		*lip,
>  	xfs_lsn_t			lsn,
> -	unsigned int			dfp_type)
> +	const struct xfs_defer_op_type	*ops)
>  {
>  	ASSERT(xlog_item_is_intent(lip));
>  
> -	xfs_defer_start_recovery(lip, dfp_type, &log->r_dfops);
> +	xfs_defer_start_recovery(lip, &log->r_dfops, ops);
>  
>  	/*
>  	 * Insert the intent into the AIL directly and drop one reference so
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index b08839550f34a3..20ad8086da60be 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -605,7 +605,7 @@ xlog_recover_cui_commit_pass2(
>  	atomic_set(&cuip->cui_next_extent, cui_formatp->cui_nextents);
>  
>  	xlog_recover_intent_item(log, &cuip->cui_item, lsn,
> -			XFS_DEFER_OPS_TYPE_REFCOUNT);
> +			&xfs_refcount_update_defer_type);
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index 65b432eb5d025d..79ad0087aecaf5 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -658,7 +658,7 @@ xlog_recover_rui_commit_pass2(
>  	atomic_set(&ruip->rui_next_extent, rui_formatp->rui_nextents);
>  
>  	xlog_recover_intent_item(log, &ruip->rui_item, lsn,
> -			XFS_DEFER_OPS_TYPE_RMAP);
> +			&xfs_rmap_update_defer_type);
>  	return 0;
>  }
>  
> -- 
> 2.39.2
> 
> 

[PATCH 5/5] xfs: pass the defer ops directly to xfs_defer_add

[Christoph Hellwig]

Pass a pointer to the xfs_defer_op_type structure to xfs_defer_add and
remove the indirection through the xfs_defer_ops_type enum and a global
table of all possible operations.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_alloc.c    |  4 ++--
 fs/xfs/libxfs/xfs_attr.c     |  2 +-
 fs/xfs/libxfs/xfs_bmap.c     |  2 +-
 fs/xfs/libxfs/xfs_defer.c    | 16 ++--------------
 fs/xfs/libxfs/xfs_defer.h    | 18 ++----------------
 fs/xfs/libxfs/xfs_refcount.c |  2 +-
 fs/xfs/libxfs/xfs_rmap.c     |  2 +-
 7 files changed, 10 insertions(+), 36 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 4940f9377f21a1..60c2c18e8e54f9 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -2514,7 +2514,7 @@ xfs_defer_agfl_block(
 	trace_xfs_agfl_free_defer(mp, agno, 0, agbno, 1);
 
 	xfs_extent_free_get_group(mp, xefi);
-	xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_AGFL_FREE, &xefi->xefi_list);
+	xfs_defer_add(tp, &xefi->xefi_list, &xfs_agfl_free_defer_type);
 	return 0;
 }
 
@@ -2578,7 +2578,7 @@ xfs_defer_extent_free(
 			XFS_FSB_TO_AGBNO(tp->t_mountp, bno), len);
 
 	xfs_extent_free_get_group(mp, xefi);
-	*dfpp = xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_FREE, &xefi->xefi_list);
+	*dfpp = xfs_defer_add(tp, &xefi->xefi_list, &xfs_extent_free_defer_type);
 	return 0;
 }
 
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 4fed0c87a968ab..fa49c795f40745 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -906,7 +906,7 @@ xfs_attr_defer_add(
 		ASSERT(0);
 	}
 
-	xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
+	xfs_defer_add(args->trans, &new->xattri_list, &xfs_attr_defer_type);
 	trace_xfs_attr_defer_add(new->xattri_dela_state, args->dp);
 }
 
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index ca6614f4eac50a..e308d2f44a3c31 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -6091,7 +6091,7 @@ __xfs_bmap_add(
 	bi->bi_bmap = *bmap;
 
 	xfs_bmap_update_get_group(tp->t_mountp, bi);
-	xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_BMAP, &bi->bi_list);
+	xfs_defer_add(tp, &bi->bi_list, &xfs_bmap_update_defer_type);
 	return 0;
 }
 
diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 6a6444ffe5544b..10987877f19378 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -235,16 +235,6 @@ static const struct xfs_defer_op_type xfs_barrier_defer_type = {
 	.cancel_item	= xfs_defer_barrier_cancel_item,
 };
 
-static const struct xfs_defer_op_type *defer_op_types[] = {
-	[XFS_DEFER_OPS_TYPE_BMAP]	= &xfs_bmap_update_defer_type,
-	[XFS_DEFER_OPS_TYPE_REFCOUNT]	= &xfs_refcount_update_defer_type,
-	[XFS_DEFER_OPS_TYPE_RMAP]	= &xfs_rmap_update_defer_type,
-	[XFS_DEFER_OPS_TYPE_FREE]	= &xfs_extent_free_defer_type,
-	[XFS_DEFER_OPS_TYPE_AGFL_FREE]	= &xfs_agfl_free_defer_type,
-	[XFS_DEFER_OPS_TYPE_ATTR]	= &xfs_attr_defer_type,
-	[XFS_DEFER_OPS_TYPE_BARRIER]	= &xfs_barrier_defer_type,
-};
-
 /* Create a log intent done item for a log intent item. */
 static inline void
 xfs_defer_create_done(
@@ -847,14 +837,12 @@ xfs_defer_alloc(
 struct xfs_defer_pending *
 xfs_defer_add(
 	struct xfs_trans		*tp,
-	enum xfs_defer_ops_type		type,
-	struct list_head		*li)
+	struct list_head		*li,
+	const struct xfs_defer_op_type	*ops)
 {
 	struct xfs_defer_pending	*dfp = NULL;
-	const struct xfs_defer_op_type	*ops = defer_op_types[type];
 
 	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
-	BUILD_BUG_ON(ARRAY_SIZE(defer_op_types) != XFS_DEFER_OPS_TYPE_MAX);
 
 	dfp = xfs_defer_find_last(tp, ops);
 	if (!dfp || !xfs_defer_can_append(dfp, ops))
diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index 60de91b6639225..18a9fb92dde8e6 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -10,20 +10,6 @@ struct xfs_btree_cur;
 struct xfs_defer_op_type;
 struct xfs_defer_capture;
 
-/*
- * Header for deferred operation list.
- */
-enum xfs_defer_ops_type {
-	XFS_DEFER_OPS_TYPE_BMAP,
-	XFS_DEFER_OPS_TYPE_REFCOUNT,
-	XFS_DEFER_OPS_TYPE_RMAP,
-	XFS_DEFER_OPS_TYPE_FREE,
-	XFS_DEFER_OPS_TYPE_AGFL_FREE,
-	XFS_DEFER_OPS_TYPE_ATTR,
-	XFS_DEFER_OPS_TYPE_BARRIER,
-	XFS_DEFER_OPS_TYPE_MAX,
-};
-
 /*
  * Save a log intent item and a list of extents, so that we can replay
  * whatever action had to happen to the extent list and file the log done
@@ -51,8 +37,8 @@ struct xfs_defer_pending {
 void xfs_defer_item_pause(struct xfs_trans *tp, struct xfs_defer_pending *dfp);
 void xfs_defer_item_unpause(struct xfs_trans *tp, struct xfs_defer_pending *dfp);
 
-struct xfs_defer_pending *xfs_defer_add(struct xfs_trans *tp,
-		enum xfs_defer_ops_type type, struct list_head *h);
+struct xfs_defer_pending *xfs_defer_add(struct xfs_trans *tp, struct list_head *h,
+		const struct xfs_defer_op_type *ops);
 int xfs_defer_finish_noroll(struct xfs_trans **tp);
 int xfs_defer_finish(struct xfs_trans **tp);
 int xfs_defer_finish_one(struct xfs_trans *tp, struct xfs_defer_pending *dfp);
diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
index 3702b4a071100d..5b039cd022e073 100644
--- a/fs/xfs/libxfs/xfs_refcount.c
+++ b/fs/xfs/libxfs/xfs_refcount.c
@@ -1458,7 +1458,7 @@ __xfs_refcount_add(
 	ri->ri_blockcount = blockcount;
 
 	xfs_refcount_update_get_group(tp->t_mountp, ri);
-	xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_REFCOUNT, &ri->ri_list);
+	xfs_defer_add(tp, &ri->ri_list, &xfs_refcount_update_defer_type);
 }
 
 /*
diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
index fbb0b263746352..76bf7f48cb5acf 100644
--- a/fs/xfs/libxfs/xfs_rmap.c
+++ b/fs/xfs/libxfs/xfs_rmap.c
@@ -2567,7 +2567,7 @@ __xfs_rmap_add(
 	ri->ri_bmap = *bmap;
 
 	xfs_rmap_update_get_group(tp->t_mountp, ri);
-	xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_RMAP, &ri->ri_list);
+	xfs_defer_add(tp, &ri->ri_list, &xfs_rmap_update_defer_type);
 }
 
 /* Map an extent into a file. */
-- 
2.39.2

Re: [PATCH 5/5] xfs: pass the defer ops directly to xfs_defer_add

[Darrick J. Wong]

On Wed, Dec 13, 2023 at 10:06:33AM +0100, Christoph Hellwig wrote:
> Pass a pointer to the xfs_defer_op_type structure to xfs_defer_add and
> remove the indirection through the xfs_defer_ops_type enum and a global
> table of all possible operations.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

And now anyone can create their own deferred work loops without having
to register it in some dumb enum in xfs_defer.[ch].  Hooray!

Thanks for doing these cleanups,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_alloc.c    |  4 ++--
>  fs/xfs/libxfs/xfs_attr.c     |  2 +-
>  fs/xfs/libxfs/xfs_bmap.c     |  2 +-
>  fs/xfs/libxfs/xfs_defer.c    | 16 ++--------------
>  fs/xfs/libxfs/xfs_defer.h    | 18 ++----------------
>  fs/xfs/libxfs/xfs_refcount.c |  2 +-
>  fs/xfs/libxfs/xfs_rmap.c     |  2 +-
>  7 files changed, 10 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index 4940f9377f21a1..60c2c18e8e54f9 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -2514,7 +2514,7 @@ xfs_defer_agfl_block(
>  	trace_xfs_agfl_free_defer(mp, agno, 0, agbno, 1);
>  
>  	xfs_extent_free_get_group(mp, xefi);
> -	xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_AGFL_FREE, &xefi->xefi_list);
> +	xfs_defer_add(tp, &xefi->xefi_list, &xfs_agfl_free_defer_type);
>  	return 0;
>  }
>  
> @@ -2578,7 +2578,7 @@ xfs_defer_extent_free(
>  			XFS_FSB_TO_AGBNO(tp->t_mountp, bno), len);
>  
>  	xfs_extent_free_get_group(mp, xefi);
> -	*dfpp = xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_FREE, &xefi->xefi_list);
> +	*dfpp = xfs_defer_add(tp, &xefi->xefi_list, &xfs_extent_free_defer_type);
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 4fed0c87a968ab..fa49c795f40745 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -906,7 +906,7 @@ xfs_attr_defer_add(
>  		ASSERT(0);
>  	}
>  
> -	xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
> +	xfs_defer_add(args->trans, &new->xattri_list, &xfs_attr_defer_type);
>  	trace_xfs_attr_defer_add(new->xattri_dela_state, args->dp);
>  }
>  
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index ca6614f4eac50a..e308d2f44a3c31 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -6091,7 +6091,7 @@ __xfs_bmap_add(
>  	bi->bi_bmap = *bmap;
>  
>  	xfs_bmap_update_get_group(tp->t_mountp, bi);
> -	xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_BMAP, &bi->bi_list);
> +	xfs_defer_add(tp, &bi->bi_list, &xfs_bmap_update_defer_type);
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index 6a6444ffe5544b..10987877f19378 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -235,16 +235,6 @@ static const struct xfs_defer_op_type xfs_barrier_defer_type = {
>  	.cancel_item	= xfs_defer_barrier_cancel_item,
>  };
>  
> -static const struct xfs_defer_op_type *defer_op_types[] = {
> -	[XFS_DEFER_OPS_TYPE_BMAP]	= &xfs_bmap_update_defer_type,
> -	[XFS_DEFER_OPS_TYPE_REFCOUNT]	= &xfs_refcount_update_defer_type,
> -	[XFS_DEFER_OPS_TYPE_RMAP]	= &xfs_rmap_update_defer_type,
> -	[XFS_DEFER_OPS_TYPE_FREE]	= &xfs_extent_free_defer_type,
> -	[XFS_DEFER_OPS_TYPE_AGFL_FREE]	= &xfs_agfl_free_defer_type,
> -	[XFS_DEFER_OPS_TYPE_ATTR]	= &xfs_attr_defer_type,
> -	[XFS_DEFER_OPS_TYPE_BARRIER]	= &xfs_barrier_defer_type,
> -};
> -
>  /* Create a log intent done item for a log intent item. */
>  static inline void
>  xfs_defer_create_done(
> @@ -847,14 +837,12 @@ xfs_defer_alloc(
>  struct xfs_defer_pending *
>  xfs_defer_add(
>  	struct xfs_trans		*tp,
> -	enum xfs_defer_ops_type		type,
> -	struct list_head		*li)
> +	struct list_head		*li,
> +	const struct xfs_defer_op_type	*ops)
>  {
>  	struct xfs_defer_pending	*dfp = NULL;
> -	const struct xfs_defer_op_type	*ops = defer_op_types[type];
>  
>  	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
> -	BUILD_BUG_ON(ARRAY_SIZE(defer_op_types) != XFS_DEFER_OPS_TYPE_MAX);
>  
>  	dfp = xfs_defer_find_last(tp, ops);
>  	if (!dfp || !xfs_defer_can_append(dfp, ops))
> diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
> index 60de91b6639225..18a9fb92dde8e6 100644
> --- a/fs/xfs/libxfs/xfs_defer.h
> +++ b/fs/xfs/libxfs/xfs_defer.h
> @@ -10,20 +10,6 @@ struct xfs_btree_cur;
>  struct xfs_defer_op_type;
>  struct xfs_defer_capture;
>  
> -/*
> - * Header for deferred operation list.
> - */
> -enum xfs_defer_ops_type {
> -	XFS_DEFER_OPS_TYPE_BMAP,
> -	XFS_DEFER_OPS_TYPE_REFCOUNT,
> -	XFS_DEFER_OPS_TYPE_RMAP,
> -	XFS_DEFER_OPS_TYPE_FREE,
> -	XFS_DEFER_OPS_TYPE_AGFL_FREE,
> -	XFS_DEFER_OPS_TYPE_ATTR,
> -	XFS_DEFER_OPS_TYPE_BARRIER,
> -	XFS_DEFER_OPS_TYPE_MAX,
> -};
> -
>  /*
>   * Save a log intent item and a list of extents, so that we can replay
>   * whatever action had to happen to the extent list and file the log done
> @@ -51,8 +37,8 @@ struct xfs_defer_pending {
>  void xfs_defer_item_pause(struct xfs_trans *tp, struct xfs_defer_pending *dfp);
>  void xfs_defer_item_unpause(struct xfs_trans *tp, struct xfs_defer_pending *dfp);
>  
> -struct xfs_defer_pending *xfs_defer_add(struct xfs_trans *tp,
> -		enum xfs_defer_ops_type type, struct list_head *h);
> +struct xfs_defer_pending *xfs_defer_add(struct xfs_trans *tp, struct list_head *h,
> +		const struct xfs_defer_op_type *ops);
>  int xfs_defer_finish_noroll(struct xfs_trans **tp);
>  int xfs_defer_finish(struct xfs_trans **tp);
>  int xfs_defer_finish_one(struct xfs_trans *tp, struct xfs_defer_pending *dfp);
> diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> index 3702b4a071100d..5b039cd022e073 100644
> --- a/fs/xfs/libxfs/xfs_refcount.c
> +++ b/fs/xfs/libxfs/xfs_refcount.c
> @@ -1458,7 +1458,7 @@ __xfs_refcount_add(
>  	ri->ri_blockcount = blockcount;
>  
>  	xfs_refcount_update_get_group(tp->t_mountp, ri);
> -	xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_REFCOUNT, &ri->ri_list);
> +	xfs_defer_add(tp, &ri->ri_list, &xfs_refcount_update_defer_type);
>  }
>  
>  /*
> diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
> index fbb0b263746352..76bf7f48cb5acf 100644
> --- a/fs/xfs/libxfs/xfs_rmap.c
> +++ b/fs/xfs/libxfs/xfs_rmap.c
> @@ -2567,7 +2567,7 @@ __xfs_rmap_add(
>  	ri->ri_bmap = *bmap;
>  
>  	xfs_rmap_update_get_group(tp->t_mountp, ri);
> -	xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_RMAP, &ri->ri_list);
> +	xfs_defer_add(tp, &ri->ri_list, &xfs_rmap_update_defer_type);
>  }
>  
>  /* Map an extent into a file. */
> -- 
> 2.39.2
> 
>