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

[Christoph Hellwig <hch@lst.de>]

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);
	}

> +	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?

> +		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.

> +/* 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?

> +	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?