Re: [PATCH 3/5] xfs: add XFS_ITEM_UNSAFE for log item push return result

[Dave Chinner <david@fromorbit.com>]

On Fri, Aug 23, 2024 at 10:17:09AM -0700, Darrick J. Wong wrote:
> On Fri, Aug 23, 2024 at 07:04:37PM +0800, Long Li wrote:
> > After pushing log items, the log item may have been freed, making it
> > unsafe to access in tracepoints. This commit introduces XFS_ITEM_UNSAFE
> > to indicate when an item might be freed during the item push operation.
> > 
> > Signed-off-by: Long Li <leo.lilong@huawei.com>
> > ---
> >  fs/xfs/xfs_stats.h     | 1 +
> >  fs/xfs/xfs_trans.h     | 1 +
> >  fs/xfs/xfs_trans_ail.c | 7 +++++++
> >  3 files changed, 9 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_stats.h b/fs/xfs/xfs_stats.h
> > index a61fb56ed2e6..9a7a020587cf 100644
> > --- a/fs/xfs/xfs_stats.h
> > +++ b/fs/xfs/xfs_stats.h
> > @@ -86,6 +86,7 @@ struct __xfsstats {
> >  	uint32_t		xs_push_ail_pushbuf;
> >  	uint32_t		xs_push_ail_pinned;
> >  	uint32_t		xs_push_ail_locked;
> > +	uint32_t		xs_push_ail_unsafe;
> >  	uint32_t		xs_push_ail_flushing;
> >  	uint32_t		xs_push_ail_restarts;
> >  	uint32_t		xs_push_ail_flush;
> > diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> > index f06cc0f41665..fd4f04853fe2 100644
> > --- a/fs/xfs/xfs_trans.h
> > +++ b/fs/xfs/xfs_trans.h
> > @@ -117,6 +117,7 @@ void	xfs_log_item_init(struct xfs_mount *mp, struct xfs_log_item *item,
> >  #define XFS_ITEM_PINNED		1
> >  #define XFS_ITEM_LOCKED		2
> >  #define XFS_ITEM_FLUSHING	3
> > +#define XFS_ITEM_UNSAFE		4
> >  
> >  /*
> >   * This is the structure maintained for every active transaction.
> > diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> > index 8ede9d099d1f..a5ab1ffb8937 100644
> > --- a/fs/xfs/xfs_trans_ail.c
> > +++ b/fs/xfs/xfs_trans_ail.c
> > @@ -561,6 +561,13 @@ xfsaild_push(
> >  
> >  			stuck++;
> >  			break;
> > +		case XFS_ITEM_UNSAFE:
> > +			/*
> > +			 * The item may have been freed, so we can't access the
> > +			 * log item here.
> > +			 */
> > +			XFS_STATS_INC(mp, xs_push_ail_unsafe);
> 
> Aha, so this is in reaction to Dave's comment "So, yeah, these failure
> cases need to return something different to xfsaild_push() so it knows
> that it is unsafe to reference the log item, even for tracing purposes."

I wish I knew when I said that and what I said it about. There's no
pointer to previous versions of this fix in the series description -
it's not even marked as a "v2" patchset.

> What we're trying to communicate through xfsaild_push_item is that we've
> cycled the AIL lock and possibly done something (e.g. deleting the log
> item from the AIL) such that the lip reference might be stale.
> 
> Can we call this XFS_ITEM_STALEREF?  "Unsafe" is vague.

In this case, the returned value tells the aild what the result of
the push was. i.e. the return value is XFS_ITEM_FREED, telling the
aild that the push has freed the item (and so the aild should not
access it again).

-Dave.
-- 
Dave Chinner
david@fromorbit.com