Re: [RFC v5 PATCH 9/9] xfs: relog random buffers based on errortag

[Brian Foster <bfoster@redhat.com>]

On Thu, Feb 27, 2020 at 04:48:59PM -0700, Allison Collins wrote:
> 
> 
> On 2/27/20 6:43 AM, Brian Foster wrote:
> > Since there is currently no specific use case for buffer relogging,
> > add some hacky and experimental code to relog random buffers when
> > the associated errortag is enabled. Update the relog reservation
> > calculation appropriately and use fixed termination logic to help
> > ensure that the relog queue doesn't grow indefinitely.
> > 
> > Note that this patch was useful in causing log reservation deadlocks
> > on an fsstress workload if the relog mechanism code is modified to
> > acquire its own log reservation rather than rely on the relog
> > pre-reservation mechanism. In other words, this helps prove that the
> > relog reservation management code effectively avoids log reservation
> > deadlocks.
> > 
> 
> Oh i see, so the last three are sort of an internal test case.  They look
> like they are good sand box tools for testing though.  I guess they dont
> really get RVBs since they dont apply?  Otherwise looks good for the purpose
> they are meant for.  :-)
> 

Right. I'm actually not opposed to polishing up the buffer relogging
code and packaging it with an fstest that invokes the errortag, but the
original intent is for these to stay RFC and drop off the series. I have
already cleaned up some of the code to use a new ->iop_relog() callback
and also have a patch to use buffer relogging as a solution for an
already fixed xattr buffer write verifier failure bug caused by
premature writeback. That use case is contrived (aside from being
already fixed), however, so by itself doesn't justify inclusion of the
buffer bits. I think it's more of a question of if anybody looks at this
patch and can think of any reasonable future use case ideas. If so, it
might be worth retaining and working out any kinks..

Brian

> Allison
> 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >   fs/xfs/libxfs/xfs_trans_resv.c |  8 +++++++-
> >   fs/xfs/xfs_trans.h             |  4 +++-
> >   fs/xfs/xfs_trans_ail.c         | 11 +++++++++++
> >   fs/xfs/xfs_trans_buf.c         | 13 +++++++++++++
> >   4 files changed, 34 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> > index f49b20c9ca33..59a328a0dec6 100644
> > --- a/fs/xfs/libxfs/xfs_trans_resv.c
> > +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> > @@ -840,7 +840,13 @@ STATIC uint
> >   xfs_calc_relog_reservation(
> >   	struct xfs_mount	*mp)
> >   {
> > -	return xfs_calc_qm_quotaoff_reservation(mp);
> > +	uint			res;
> > +
> > +	res = xfs_calc_qm_quotaoff_reservation(mp);
> > +#ifdef DEBUG
> > +	res = max(res, xfs_calc_buf_res(4, XFS_FSB_TO_B(mp, 1)));
> > +#endif
> > +	return res;
> >   }
> >   void
> > diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> > index 81cb42f552d9..1783441f6d03 100644
> > --- a/fs/xfs/xfs_trans.h
> > +++ b/fs/xfs/xfs_trans.h
> > @@ -61,6 +61,7 @@ struct xfs_log_item {
> >   #define	XFS_LI_DIRTY	3	/* log item dirty in transaction */
> >   #define	XFS_LI_RELOG	4	/* automatically relog item */
> >   #define	XFS_LI_RELOGGED	5	/* item relogged (not committed) */
> > +#define	XFS_LI_RELOG_RAND 6
> >   #define XFS_LI_FLAGS \
> >   	{ (1 << XFS_LI_IN_AIL),		"IN_AIL" }, \
> > @@ -68,7 +69,8 @@ struct xfs_log_item {
> >   	{ (1 << XFS_LI_FAILED),		"FAILED" }, \
> >   	{ (1 << XFS_LI_DIRTY),		"DIRTY" }, \
> >   	{ (1 << XFS_LI_RELOG),		"RELOG" }, \
> > -	{ (1 << XFS_LI_RELOGGED),	"RELOGGED" }
> > +	{ (1 << XFS_LI_RELOGGED),	"RELOGGED" }, \
> > +	{ (1 << XFS_LI_RELOG_RAND),	"RELOG_RAND" }
> >   struct xfs_item_ops {
> >   	unsigned flags;
> > diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> > index 103ab62e61be..9b1d7c8df6d8 100644
> > --- a/fs/xfs/xfs_trans_ail.c
> > +++ b/fs/xfs/xfs_trans_ail.c
> > @@ -188,6 +188,17 @@ xfs_ail_relog(
> >   			xfs_log_ticket_put(ailp->ail_relog_tic);
> >   		spin_unlock(&ailp->ail_lock);
> > +		/*
> > +		 * Terminate random/debug relogs at a fixed, aggressive rate to
> > +		 * avoid building up too much relog activity.
> > +		 */
> > +		if (test_bit(XFS_LI_RELOG_RAND, &lip->li_flags) &&
> > +		    ((prandom_u32() & 1) ||
> > +		     (mp->m_flags & XFS_MOUNT_UNMOUNTING))) {
> > +			clear_bit(XFS_LI_RELOG_RAND, &lip->li_flags);
> > +			xfs_trans_relog_item_cancel(lip, false);
> > +		}
> > +
> >   		/*
> >   		 * TODO: Ideally, relog transaction management would be pushed
> >   		 * down into the ->iop_push() callbacks rather than playing
> > diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> > index e17715ac23fc..de7b9a68fe38 100644
> > --- a/fs/xfs/xfs_trans_buf.c
> > +++ b/fs/xfs/xfs_trans_buf.c
> > @@ -14,6 +14,8 @@
> >   #include "xfs_buf_item.h"
> >   #include "xfs_trans_priv.h"
> >   #include "xfs_trace.h"
> > +#include "xfs_error.h"
> > +#include "xfs_errortag.h"
> >   /*
> >    * Check to see if a buffer matching the given parameters is already
> > @@ -527,6 +529,17 @@ xfs_trans_log_buf(
> >   	trace_xfs_trans_log_buf(bip);
> >   	xfs_buf_item_log(bip, first, last);
> > +
> > +	/*
> > +	 * Relog random buffers so long as the transaction is relog enabled and
> > +	 * the buffer wasn't already relogged explicitly.
> > +	 */
> > +	if (XFS_TEST_ERROR(false, tp->t_mountp, XFS_ERRTAG_RELOG) &&
> > +	    (tp->t_flags & XFS_TRANS_RELOG) &&
> > +	    !test_bit(XFS_LI_RELOG, &bip->bli_item.li_flags)) {
> > +		if (xfs_trans_relog_buf(tp, bp))
> > +			set_bit(XFS_LI_RELOG_RAND, &bip->bli_item.li_flags);
> > +	}
> >   }
> > 
>