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

[Brian Foster <bfoster@redhat.com>]

On Mon, Jul 30, 2018 at 01:20:37PM -0700, Darrick J. Wong wrote:
> 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_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
...
> > @@ -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.
> 

Yup, good point. I'll fix up the error checking in both of these
patches.

Brian

> --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
> --
> 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