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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx
