Re: [PATCH 07/15] xfs: automatic dfops buffer relogging

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Mon, Jul 30, 2018 at 12:45:12PM -0400, Brian Foster wrote:
> Buffers that are held across deferred operations are explicitly
> joined to the dfops structure to ensure appropriate relogging.
> While buffers are currently joined explicitly, we can detect the
> conditions that require relogging at dfops finish time by inspecting
> the transaction item list for held buffers.
> 
> Replace the xfs_defer_bjoin() infrastructure with such detection and
> automatic relogging of held buffers. This eliminates the need for
> the per-dfops buffer list, replaced by an on-stack variant in
> xfs_defer_trans_roll().
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c  |  1 -
>  fs/xfs/libxfs/xfs_defer.c | 55 ++++++++++++++++-----------------------
>  fs/xfs/libxfs/xfs_defer.h |  1 -
>  fs/xfs/xfs_dquot.c        |  1 -
>  fs/xfs/xfs_trans.h        |  1 -
>  5 files changed, 23 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 3deb5cdadf08..227887bee00d 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -320,7 +320,6 @@ xfs_attr_set(
>  		 * buffer and run into problems with the write verifier.
>  		 */
>  		xfs_trans_bhold(args.trans, leaf_bp);
> -		xfs_defer_bjoin(args.trans->t_dfops, leaf_bp);
>  		xfs_defer_ijoin(args.trans->t_dfops, dp);
>  		error = xfs_defer_finish(&args.trans);
>  		if (error)
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index 0306187b5f56..d0ee55048a7a 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -14,6 +14,7 @@
>  #include "xfs_mount.h"
>  #include "xfs_defer.h"
>  #include "xfs_trans.h"
> +#include "xfs_buf_item.h"
>  #include "xfs_trace.h"
>  
>  /*
> @@ -228,6 +229,10 @@ xfs_defer_trans_roll(
>  	struct xfs_trans		**tp)
>  {
>  	struct xfs_defer_ops		*dop = (*tp)->t_dfops;
> +	struct xfs_buf_log_item		*bli;
> +	struct xfs_log_item		*lip;
> +	struct xfs_buf			*bplist[XFS_DEFER_OPS_NR_BUFS];
> +	int				bpcount = 0;
>  	int				i;
>  	int				error;
>  
> @@ -235,9 +240,21 @@ xfs_defer_trans_roll(
>  	for (i = 0; i < XFS_DEFER_OPS_NR_INODES && dop->dop_inodes[i]; i++)
>  		xfs_trans_log_inode(*tp, dop->dop_inodes[i], XFS_ILOG_CORE);
>  
> -	/* Hold the (previously bjoin'd) buffer locked across the roll. */
> -	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++)
> -		xfs_trans_dirty_buf(*tp, dop->dop_bufs[i]);
> +	list_for_each_entry(lip, &(*tp)->t_items, li_trans) {
> +		switch (lip->li_type) {
> +		case XFS_LI_BUF:
> +			bli = container_of(lip, struct xfs_buf_log_item,
> +					   bli_item);
> +			if (bli->bli_flags & XFS_BLI_HOLD) {
> +				ASSERT(bpcount < XFS_DEFER_OPS_NR_BUFS);

This is a bit of a behavior change -- previously if we oveflowed the
array we'd bail out with -EFSCORRUPTED rather than go corrupting
whatever came after dop_bufs.  Granted, we were pretty terrible about
checking the return values, but I think this ought to be:

if (bpcount >= XFS_DEFER_OPS_NR_BUFS) {
	ASSERT(0);
	return -EFSCORRUPTED;
}

rather than corrupting the on-stack array if we happen to bhold too many
buffers to the transaction.

--D

> +				xfs_trans_dirty_buf(*tp, bli->bli_buf);
> +				bplist[bpcount++] = bli->bli_buf;
> +			}
> +			break;
> +		default:
> +			break;
> +		}
> +	}
>  
>  	trace_xfs_defer_trans_roll((*tp)->t_mountp, dop, _RET_IP_);
>  
> @@ -255,9 +272,9 @@ xfs_defer_trans_roll(
>  		xfs_trans_ijoin(*tp, dop->dop_inodes[i], 0);
>  
>  	/* Rejoin the buffers and dirty them so the log moves forward. */
> -	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++) {
> -		xfs_trans_bjoin(*tp, dop->dop_bufs[i]);
> -		xfs_trans_bhold(*tp, dop->dop_bufs[i]);
> +	for (i = 0; i < bpcount; i++) {
> +		xfs_trans_bjoin(*tp, bplist[i]);
> +		xfs_trans_bhold(*tp, bplist[i]);
>  	}
>  
>  	return error;
> @@ -295,30 +312,6 @@ xfs_defer_ijoin(
>  	return -EFSCORRUPTED;
>  }
>  
> -/*
> - * Add this buffer to the deferred op.  Each joined buffer is relogged
> - * each time we roll the transaction.
> - */
> -int
> -xfs_defer_bjoin(
> -	struct xfs_defer_ops		*dop,
> -	struct xfs_buf			*bp)
> -{
> -	int				i;
> -
> -	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS; i++) {
> -		if (dop->dop_bufs[i] == bp)
> -			return 0;
> -		else if (dop->dop_bufs[i] == NULL) {
> -			dop->dop_bufs[i] = bp;
> -			return 0;
> -		}
> -	}
> -
> -	ASSERT(0);
> -	return -EFSCORRUPTED;
> -}
> -
>  /*
>   * Reset an already used dfops after finish.
>   */
> @@ -331,7 +324,6 @@ xfs_defer_reset(
>  	ASSERT(!xfs_defer_has_unfinished_work(dop));
>  
>  	memset(dop->dop_inodes, 0, sizeof(dop->dop_inodes));
> -	memset(dop->dop_bufs, 0, sizeof(dop->dop_bufs));
>  
>  	/*
>  	 * Low mode state transfers across transaction rolls to mirror dfops
> @@ -594,7 +586,6 @@ xfs_defer_move(
>  	list_splice_init(&src->dop_pending, &dst->dop_pending);
>  
>  	memcpy(dst->dop_inodes, src->dop_inodes, sizeof(dst->dop_inodes));
> -	memcpy(dst->dop_bufs, src->dop_bufs, sizeof(dst->dop_bufs));
>  
>  	/*
>  	 * Low free space mode was historically controlled by a dfops field.
> diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
> index 8908a2716774..4a8bb838adf2 100644
> --- a/fs/xfs/libxfs/xfs_defer.h
> +++ b/fs/xfs/libxfs/xfs_defer.h
> @@ -43,7 +43,6 @@ void xfs_defer_cancel(struct xfs_trans *);
>  void xfs_defer_init(struct xfs_trans *tp, struct xfs_defer_ops *dop);
>  bool xfs_defer_has_unfinished_work(struct xfs_defer_ops *dop);
>  int xfs_defer_ijoin(struct xfs_defer_ops *dop, struct xfs_inode *ip);
> -int xfs_defer_bjoin(struct xfs_defer_ops *dop, struct xfs_buf *bp);
>  void xfs_defer_move(struct xfs_trans *dtp, struct xfs_trans *stp);
>  
>  /* Description of a deferred type. */
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index da5c55cec966..e1196854dbcd 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -362,7 +362,6 @@ xfs_dquot_disk_alloc(
>  	 * manually or by committing the transaction.
>  	 */
>  	xfs_trans_bhold(tp, bp);
> -	error = xfs_defer_bjoin(tp->t_dfops, bp);
>  	if (error) {
>  		xfs_trans_bhold_release(tp, bp);
>  		xfs_trans_brelse(tp, bp);
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 7e493221160e..581456c79197 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -100,7 +100,6 @@ struct xfs_defer_ops {
>  
>  	/* relog these with each roll */
>  	struct xfs_inode	*dop_inodes[XFS_DEFER_OPS_NR_INODES];
> -	struct xfs_buf		*dop_bufs[XFS_DEFER_OPS_NR_BUFS];
>  };
>  
>  /*
> -- 
> 2.17.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html