Re: [PATCH 04/11] xfs: convey filesystem unmount events to the health monitor

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

On Tue, Jan 13, 2026 at 05:11:05PM +0100, Christoph Hellwig wrote:
> > +	event = kzalloc(sizeof(struct xfs_healthmon_event), GFP_NOFS);
> > +	mutex_lock(&hm->lock);
> > +
> > +	trace_xfs_healthmon_report_unmount(hm);
> > +
> > +	if (event) {
> > +		/*
> > +		 * Insert the unmount notification at the start of the event
> > +		 * queue so that userspace knows the filesystem went away as
> > +		 * soon as possible.  There's nothing actionable for userspace
> > +		 * after an unmount.
> > +		 */
> > +		event->type = XFS_HEALTHMON_UNMOUNT;
> > +		event->domain = XFS_HEALTHMON_MOUNT;
> > +
> > +		__xfs_healthmon_insert(hm, event);
> 
> The allocation (and locking) could move into __xfs_healthmon_insert
> sharing this code with xfs_ioc_health_monitor().  That means the wake_up
> would not be covered by the lock, but I can't see how that would need
> it.
> 
> > +	} else {
> > +		/*
> > +		 * Wake up the reader directly in case we didn't have enough
> > +		 * memory to queue the unmount event.  The filesystem is about
> > +		 * to go away so we don't care about reporting previously lost
> > +		 * events.
> > +		 */
> > +		wake_up(&hm->wait);
> > +	}
> 
> And then even after reading this many times I'm still not understanding
> it.  Yes, this wakes up the reader direct, but why is waking it up
> without an even ok?

I think that was an artifact from an earlier stage in development where
it was necessary to wake up the reader to get it to drain the event
queue no matter what.  At this point I'd much rather preallocate the
unmount event because...

> And if it is ok, why do we even bother with the
> unmount even?  If OTOH the unmount event is actually important, should
> we preallocate the event?  (and render my above suggestion impractical).

...the unmount is important, because the userspace daemon uses that as
the signal to quit immediately.  If userspace misses the event then
it'll just keep running forever with a dead healthmon fd.

xfs_healer_start will restart the whole service if someone mounts
another xfs filesystem at the same mountpoint, but if nobody does that
then we'll be stuck with a defunct daemon forever.

Let's preallocate the unmount event, and then this whole thing becomes:

	/*
	 * Insert the unmount notification at the start of the event
	 * queue so that userspace knows the filesystem went away as
	 * soon as possible.  There's nothing actionable for userspace
	 * after an unmount.  Once we've inserted unmount_event, hm no
	 * longer owns that event.
	 */
	mutex_lock(&hm->lock);
	__xfs_healthmon_insert(hm, hm->unmount_event);
	hm->unmount_event = NULL;
	mutex_unlock(&hm->lock);

--D