Re: [RFC v5 PATCH 7/9] xfs: buffer relogging support prototype

From: Brian Foster <bfoster@redhat.com>
To: Allison Collins <allison.henderson@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [RFC v5 PATCH 7/9] xfs: buffer relogging support prototype
Date: Fri, 28 Feb 2020 09:04:41 -0500	[thread overview]
Message-ID: <20200228140441.GG2751@bfoster> (raw)
In-Reply-To: <12e69f70-13e6-830c-83ef-9ad5b222301e@oracle.com>

On Thu, Feb 27, 2020 at 04:33:26PM -0700, Allison Collins wrote:
> On 2/27/20 6:43 AM, Brian Foster wrote:
> > Add a quick and dirty implementation of buffer relogging support.
> > There is currently no use case for buffer relogging. This is for
> > experimental use only and serves as an example to demonstrate the
> > ability to relog arbitrary items in the future, if necessary.
> > 
> > Add a hook to enable relogging a buffer in a transaction, update the
> > buffer log item handlers to support relogged BLIs and update the
> > relog handler to join the relogged buffer to the relog transaction.
> > 
> Alrighty, thanks for the example!  It sounds like it's meant more to be a
> demo than to really be applied though?
> 

Yeah, I just wanted to include something that demonstrates how this can
be used for something other than intents, because that concern was
raised on previous versions...

Brian

> Allison
> 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >   fs/xfs/xfs_buf_item.c  |  5 +++++
> >   fs/xfs/xfs_trans.h     |  1 +
> >   fs/xfs/xfs_trans_ail.c | 19 ++++++++++++++++---
> >   fs/xfs/xfs_trans_buf.c | 22 ++++++++++++++++++++++
> >   4 files changed, 44 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> > index 663810e6cd59..4ef2725fa8ce 100644
> > --- a/fs/xfs/xfs_buf_item.c
> > +++ b/fs/xfs/xfs_buf_item.c
> > @@ -463,6 +463,7 @@ xfs_buf_item_unpin(
> >   			list_del_init(&bp->b_li_list);
> >   			bp->b_iodone = NULL;
> >   		} else {
> > +			xfs_trans_relog_item_cancel(lip, false);
> >   			spin_lock(&ailp->ail_lock);
> >   			xfs_trans_ail_delete(ailp, lip, SHUTDOWN_LOG_IO_ERROR);
> >   			xfs_buf_item_relse(bp);
> > @@ -528,6 +529,9 @@ xfs_buf_item_push(
> >   		return XFS_ITEM_LOCKED;
> >   	}
> > +	if (test_bit(XFS_LI_RELOG, &lip->li_flags))
> > +		return XFS_ITEM_RELOG;
> > +
> >   	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
> >   	trace_xfs_buf_item_push(bip);
> > @@ -956,6 +960,7 @@ STATIC void
> >   xfs_buf_item_free(
> >   	struct xfs_buf_log_item	*bip)
> >   {
> > +	ASSERT(!test_bit(XFS_LI_RELOG, &bip->bli_item.li_flags));
> >   	xfs_buf_item_free_format(bip);
> >   	kmem_free(bip->bli_item.li_lv_shadow);
> >   	kmem_cache_free(xfs_buf_item_zone, bip);
> > diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> > index 1637df32c64c..81cb42f552d9 100644
> > --- a/fs/xfs/xfs_trans.h
> > +++ b/fs/xfs/xfs_trans.h
> > @@ -226,6 +226,7 @@ void		xfs_trans_inode_buf(xfs_trans_t *, struct xfs_buf *);
> >   void		xfs_trans_stale_inode_buf(xfs_trans_t *, struct xfs_buf *);
> >   bool		xfs_trans_ordered_buf(xfs_trans_t *, struct xfs_buf *);
> >   void		xfs_trans_dquot_buf(xfs_trans_t *, struct xfs_buf *, uint);
> > +bool		xfs_trans_relog_buf(struct xfs_trans *, struct xfs_buf *);
> >   void		xfs_trans_inode_alloc_buf(xfs_trans_t *, struct xfs_buf *);
> >   void		xfs_trans_ichgtime(struct xfs_trans *, struct xfs_inode *, int);
> >   void		xfs_trans_ijoin(struct xfs_trans *, struct xfs_inode *, uint);
> > diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> > index 71a47faeaae8..103ab62e61be 100644
> > --- a/fs/xfs/xfs_trans_ail.c
> > +++ b/fs/xfs/xfs_trans_ail.c
> > @@ -18,6 +18,7 @@
> >   #include "xfs_error.h"
> >   #include "xfs_log.h"
> >   #include "xfs_log_priv.h"
> > +#include "xfs_buf_item.h"
> >   #ifdef DEBUG
> >   /*
> > @@ -187,9 +188,21 @@ xfs_ail_relog(
> >   			xfs_log_ticket_put(ailp->ail_relog_tic);
> >   		spin_unlock(&ailp->ail_lock);
> > -		xfs_trans_add_item(tp, lip);
> > -		set_bit(XFS_LI_DIRTY, &lip->li_flags);
> > -		tp->t_flags |= XFS_TRANS_DIRTY;
> > +		/*
> > +		 * TODO: Ideally, relog transaction management would be pushed
> > +		 * down into the ->iop_push() callbacks rather than playing
> > +		 * games with ->li_trans and looking at log item types here.
> > +		 */
> > +		if (lip->li_type == XFS_LI_BUF) {
> > +			struct xfs_buf_log_item	*bli = (struct xfs_buf_log_item *) lip;
> > +			xfs_buf_hold(bli->bli_buf);
> > +			xfs_trans_bjoin(tp, bli->bli_buf);
> > +			xfs_trans_dirty_buf(tp, bli->bli_buf);
> > +		} else {
> > +			xfs_trans_add_item(tp, lip);
> > +			set_bit(XFS_LI_DIRTY, &lip->li_flags);
> > +			tp->t_flags |= XFS_TRANS_DIRTY;
> > +		}
> >   		/* XXX: include ticket owner task fix */
> >   		error = xfs_trans_roll(&tp);
> >   		ASSERT(!error);
> > diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> > index 08174ffa2118..e17715ac23fc 100644
> > --- a/fs/xfs/xfs_trans_buf.c
> > +++ b/fs/xfs/xfs_trans_buf.c
> > @@ -787,3 +787,25 @@ xfs_trans_dquot_buf(
> >   	xfs_trans_buf_set_type(tp, bp, type);
> >   }
> > +
> > +/*
> > + * Enable automatic relogging on a buffer. This essentially pins a dirty buffer
> > + * in-core until relogging is disabled. Note that the buffer must not already be
> > + * queued for writeback.
> > + */
> > +bool
> > +xfs_trans_relog_buf(
> > +	struct xfs_trans	*tp,
> > +	struct xfs_buf		*bp)
> > +{
> > +	struct xfs_buf_log_item	*bip = bp->b_log_item;
> > +
> > +	ASSERT(tp->t_flags & XFS_TRANS_RELOG);
> > +	ASSERT(xfs_buf_islocked(bp));
> > +
> > +	if (bp->b_flags & _XBF_DELWRI_Q)
> > +		return false;
> > +
> > +	xfs_trans_relog_item(&bip->bli_item);
> > +	return true;
> > +}
> > 
> 