Re: [PATCH 03/11] xfs: create event queuing, formatting, and discovery infrastructure

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

On Wed, Jan 07, 2026 at 10:32:45AM +0100, Christoph Hellwig wrote:
> On Mon, Jan 05, 2026 at 11:11:24PM -0800, Darrick J. Wong wrote:
> > diff --git a/fs/xfs/xfs_healthmon.c b/fs/xfs/xfs_healthmon.c
> > index 3fdac72b478f3f..799e0687ae3263 100644
> > --- a/fs/xfs/xfs_healthmon.c
> > +++ b/fs/xfs/xfs_healthmon.c
> > @@ -45,6 +45,13 @@
> >  /* sign of a detached health monitor */
> >  #define DETACHED_MOUNT_COOKIE		((uintptr_t)0)
> >  
> > +/* Constrain the number of event objects that can build up in memory. */
> > +#define XFS_HEALTHMON_MAX_EVENTS \
> > +		(SZ_32K / sizeof(struct xfs_healthmon_event))
> 
> The double tab indent here looks a bit weird.
> 
> > +/* Free all events */
> > +STATIC void
> > +xfs_healthmon_free_events(
> > +	struct xfs_healthmon		*hm)
> > +{
> > +	struct xfs_healthmon_event	*event, *next;
> > +
> > +	event = hm->first_event;
> > +	while (event != NULL) {
> > +		trace_xfs_healthmon_drop(hm, event);
> > +		next = event->next;
> > +		kfree(event);
> > +		event = next;
> > +	}
> 
> This could be simplified a bit to:
> 
> 	struct xfs_healthmon_event	*event = hm->first_event;
> 
> 	while (event) {
> 		struct xfs_healthmon_event	*next = event->next;
> 
> 		trace_xfs_healthmon_drop(hm, event);
> 		kfree(event);
> 		event = next;
> 	}
> 
> or alternatively:
> 	
> 	struct xfs_healthmon_event	*event, *next = hm->first_event;
> 
> 	while ((event = next) != NULL) {
> 		trace_xfs_healthmon_drop(hm, event);
> 		next = event->next;
> 		kfree(event);
> 	}

Changed.

> > +	hm->first_event = hm->last_event = NULL;
> 
> Always personal preference, but I always hate decoding double assignments
> like this vs the more verbose:
> 
> 	hm->first_event = NULL;
> 	hm->last_event = NULL;
> 
> that beeing said, do we even need to zero these given that hm gets freed
> right after?

Nope.

> > +		return false;
> > +
> > +	switch (existing->type) {
> > +	case XFS_HEALTHMON_RUNNING:
> > +		/* should only ever be one of these events anyway */
> > +		return false;
> > +
> > +	case XFS_HEALTHMON_LOST:
> > +		existing->lostcount += new->lostcount;
> > +		return true;
> > +	}
> > +
> > +	return false;
> 
> I think the XFS_HEALTHMON_RUNNING check here is redundant, so you could
> just special case XFS_HEALTHMON_LOST with an if instead of the switch
> statement.

The switch statement fills out as we add more event types, and with the
way it's written now, gcc will complain if someone enlarges the enum
without adding a switch case here.  So I'd prefer to keep this the way
it is now.

> > +/* Make a stack event dynamic so we can put it on the list. */
> > +static inline struct xfs_healthmon_event *
> > +xfs_healthmon_event_dup(
> > +	const struct xfs_healthmon_event	*event)
> > +{
> > +	return kmemdup(event, sizeof(struct xfs_healthmon_event), GFP_NOFS);
> > +}
> 
> The callers of this and and xfs_healthmon_merge_events seem to share
> the same logic.  Maybe add a helper for the two calls and the
> XFS_HEALTHMON_MAX_EVENTS check, and fold at least xfs_healthmon_event_dup
> (and maybe xfs_healthmon_merge_events if it works out) into that?

_event_dup can be folded into its callers.

I'm less sure about _merge_events -- it would be pretty easy to
opencode its logic in _clear_lost_prev:

	if (hm->last_event &&
	    hm->last_event->type == XFS_HEALTHMON_LOST &&
	    hm->last_event->domain == XFS_HEALTHMON_MOUNT) {
		hm->last_event->lostcount += hm->lost_prev_event;
		trace_xfs_healthmon_merge(hm, hm->last_event);
		wake_up(&hm->wait);
		goto cleared;
	}

But the downside is that either we leave the dead XFS_HEALTHMON_LOST
case in the switch statement in _merge_events to avoid giving up the
compiler checking, or we add a default: case which then means that
authors of future extensions can miss things without noticing.

I think I'll remove _event_dup but leave the merge function.

> > +	if (file->f_flags & O_NONBLOCK) {
> > +		if (!xfs_healthmon_has_eventdata(hm))
> > +			return -EAGAIN;
> > +	} else {
> > +		ret = wait_event_interruptible(hm->wait,
> > +				xfs_healthmon_has_eventdata(hm));
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	inode_lock(inode);
> 
> should this be a trylock + -EAGAIN for O_NONBLOCK?

Oops yes.

--D