Re: [PATCH 1/5] xfs: always set xfs_healthmon::first_event when inserting at front of list

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Aug 25, 2026 at 08:40:17AM +0200, Christoph Hellwig wrote:
> On Mon, Aug 24, 2026 at 10:36:12PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > LOLLM complains that __xfs_healthmon_insert purports to insert a
> > xfs_healthmon_event event at the start of the event list, but neglects
> > to update first_event to point to the unmount event if there were
> > already events in the queue.  That results in list corruption, so let's
> > fix this problem.
> 
> I can't really follow this..

Let me try again:

"LOLLM complains that while __xfs_healthmon_insert is supposed to insert
an event at the head of the list, it doesn't do that correctly if the
list isn't empty.  In that case it *should* make our new event point to
the current head, and then make the head point to the new event, but
it doesn't actually update the head."

> > diff --git a/fs/xfs/xfs_healthmon.c b/fs/xfs/xfs_healthmon.c
> > index d8b95af33a3e9f..166ef0d5864486 100644
> > --- a/fs/xfs/xfs_healthmon.c
> > +++ b/fs/xfs/xfs_healthmon.c
> > @@ -276,8 +276,7 @@ __xfs_healthmon_insert(
> >  	event->time_ns = (now.tv_sec * NSEC_PER_SEC) + now.tv_nsec;
> >  
> >  	event->next = hm->first_event;
> > -	if (!hm->first_event)
> > -		hm->first_event = event;
> > +	hm->first_event = event;
> >  	if (!hm->last_event)
> >  		hm->last_event = event;
> 
> event is the newly inserted event.  We want to queue it at the
> head of the list (why, btw?).

The only event that gets inserted at the front is the unmount event.
This signals that the filesystem is being completely unmounted (not just
detached from a mount ns) and there's nothing further that xfs_healer
can do to fix the filesystem.  Therefore we put the unmount item first
in the list so that xfs_healer won't waste time trying to find the
mount to do repairs that it won't be able to make.

> The next point in event points to
> first_event (which can be NULL).  And first should always point
> to event, otherwise we potentially never queue anything up?  I.e.
> we never ever actually set first?  Not sure how that is related
> to umount.

xfs_healthmon_push will set first_event if the list is empty.

(for context, all other events are _pushed on to the end of the list)

> Maybe this should just use standard list_head-based lists even
> if they waste an extra pointer in the event structure?

Yes, that at least wouldn't increase the size of xfs_healthmon_event.

--D