Re: [PATCH v26 02/12] xfs: don't commit the first deferred transaction without intents

["Darrick J. Wong" <djwong@kernel.org>]

On Sun, Jan 23, 2022 at 10:26:58PM -0700, Allison Henderson wrote:
> If the first operation in a string of defer ops has no intents,
> then there is no reason to commit it before running the first call
> to xfs_defer_finish_one(). This allows the defer ops to be used
> effectively for non-intent based operations without requiring an
> unnecessary extra transaction commit when first called.
> 
> This fixes a regression in per-attribute modification transaction
> count when delayed attributes are not being used.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_defer.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index 6dac8d6b8c21..51574f0371b5 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -187,7 +187,7 @@ static const struct xfs_defer_op_type *defer_op_types[] = {
>  	[XFS_DEFER_OPS_TYPE_AGFL_FREE]	= &xfs_agfl_free_defer_type,
>  };
>  
> -static void
> +static bool
>  xfs_defer_create_intent(
>  	struct xfs_trans		*tp,
>  	struct xfs_defer_pending	*dfp,
> @@ -198,6 +198,7 @@ xfs_defer_create_intent(
>  	if (!dfp->dfp_intent)
>  		dfp->dfp_intent = ops->create_intent(tp, &dfp->dfp_work,
>  						     dfp->dfp_count, sort);
> +	return dfp->dfp_intent;

Hm.  My first reaction is that this still ought to be an explicit
boolean comparison...

>  }
>  
>  /*
> @@ -205,16 +206,18 @@ xfs_defer_create_intent(
>   * associated extents, then add the entire intake list to the end of
>   * the pending list.
>   */
> -STATIC void
> +STATIC bool
>  xfs_defer_create_intents(
>  	struct xfs_trans		*tp)
>  {
>  	struct xfs_defer_pending	*dfp;
> +	bool				ret = false;
>  
>  	list_for_each_entry(dfp, &tp->t_dfops, dfp_list) {
>  		trace_xfs_defer_create_intent(tp->t_mountp, dfp);
> -		xfs_defer_create_intent(tp, dfp, true);
> +		ret |= xfs_defer_create_intent(tp, dfp, true);
>  	}
> +	return ret;
>  }
>  
>  /* Abort all the intents that were committed. */
> @@ -488,7 +491,7 @@ int
>  xfs_defer_finish_noroll(
>  	struct xfs_trans		**tp)
>  {
> -	struct xfs_defer_pending	*dfp;
> +	struct xfs_defer_pending	*dfp = NULL;
>  	int				error = 0;
>  	LIST_HEAD(dop_pending);
>  
> @@ -507,17 +510,19 @@ xfs_defer_finish_noroll(
>  		 * of time that any one intent item can stick around in memory,
>  		 * pinning the log tail.
>  		 */
> -		xfs_defer_create_intents(*tp);
> +		bool has_intents = xfs_defer_create_intents(*tp);

...but now it occurs to me that I think we can test ((*tp)->t_flags &
XFS_TRANS_DIRTY) instead of setting up the explicit return type.

If the ->create_intent function actually logs an intent item to the
transaction, we need to commit that intent item (to persist it to disk)
before we start on the work that it represents.  If an intent item has
been added, the transaction will be dirty.

At this point in the loop, we're trying to set ourselves up to call
->finish_one.  The ->finish_one implementations expect a clean
transaction, which means that we /never/ want to get to...

>  		list_splice_init(&(*tp)->t_dfops, &dop_pending);
>  
> -		error = xfs_defer_trans_roll(tp);
> -		if (error)
> -			goto out_shutdown;
> +		if (has_intents || dfp) {
> +			error = xfs_defer_trans_roll(tp);
> +			if (error)
> +				goto out_shutdown;
>  
> -		/* Possibly relog intent items to keep the log moving. */
> -		error = xfs_defer_relog(tp, &dop_pending);
> -		if (error)
> -			goto out_shutdown;
> +			/* Possibly relog intent items to keep the log moving. */
> +			error = xfs_defer_relog(tp, &dop_pending);
> +			if (error)
> +				goto out_shutdown;
> +		}

...this point here with the transaction still dirty.  Therefore, I think
all this patch really needs to change is that first _trans_roll:

	xfs_defer_create_intents(*tp);
	list_splice_init(&(*tp)->t_dfops, &dop_pending);

	/*
	 * We must ensure the transaction is clean before we try to finish
	 * deferred work by committing logged intent items and anything
	 * else that dirtied the transaction.
	 */
	if ((*tpp)->t_flags & XFS_TRANS_DIRTY) {
		error = xfs_defer_trans_roll(tp);
		if (error)
			goto out_shutdown;
	}

	/* Possibly relog intent items to keep the log moving. */
	error = xfs_defer_relog(tp, &dop_pending);
	if (error)
		goto out_shutdown;

	dfp = list_first_entry(&dop_pending, struct xfs_defer_pending,
			       dfp_list);
	error = xfs_defer_finish_one(*tp, dfp);
	if (error && error != -EAGAIN)
		goto out_shutdown;

Thoughts?

--D

>  
>  		dfp = list_first_entry(&dop_pending, struct xfs_defer_pending,
>  				       dfp_list);
> -- 
> 2.25.1
>