[PATCHSET 2/2] xfs: LLM-inspired bug fixes, part 8

[PATCHSET 2/2] xfs: LLM-inspired bug fixes, part 8

[Darrick J. Wong]

Hi all,

Here's an eighth batch of xfs fixes resulting from a LLaMma.  Mwa mwa
mwa...

If you're going to start using this code, I strongly recommend pulling
from my git trees, which are linked below.

With a bit of luck, this should all go splendidly.
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=llm-fixes-8
---
Commits in this patchset:
 * xfs: always set xfs_healthmon::first_event when inserting at front of list
 * xfs: check healthmon outbuffer space correctly
 * xfs: bump lost_prev_errors if we lose even the healthmon lost event
 * xfs: report nonexistent parents as a filesystem corruption
 * xfs: destroy seen inode bitmap when we fail to add a dirpath
---
 fs/xfs/scrub/trace.h          |   33 +++++++++++++++++++++++++++++++++
 fs/xfs/scrub/dirtree.c        |   31 +++++++++++++++++++++++++++++--
 fs/xfs/scrub/dirtree_repair.c |    1 +
 fs/xfs/xfs_healthmon.c        |   23 +++++++++++++++++------
 4 files changed, 80 insertions(+), 8 deletions(-)

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

[Darrick J. Wong]

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.

Cc: <stable@vger.kernel.org> # v7.0
Fixes: b3a289a2a9397b ("xfs: create event queuing, formatting, and discovery infrastructure")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/xfs_healthmon.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)


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;
 	xfs_healthmon_bump_events(hm);

[PATCH 2/5] xfs: check healthmon outbuffer space correctly

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

LOLLM notices that the outbuf space check in xfs_healthmon_format_pop
isn't quite correct -- it checks that there's enough space to write a
xfs_healthmon_event object, but the outbuffer is supposed to contain
xfs_health_monitor_event objects.  Fix this by adding a helper, and
refactoring all three outbuf size checks to use it.

Cc: <stable@vger.kernel.org> # v7.0
Fixes: b3a289a2a9397b ("xfs: create event queuing, formatting, and discovery infrastructure")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/xfs_healthmon.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)


diff --git a/fs/xfs/xfs_healthmon.c b/fs/xfs/xfs_healthmon.c
index 166ef0d5864486..a2ae15a262a510 100644
--- a/fs/xfs/xfs_healthmon.c
+++ b/fs/xfs/xfs_healthmon.c
@@ -739,6 +739,12 @@ static const unsigned int type_map[] = {
 	[XFS_HEALTHMON_DATALOST]	= XFS_HEALTH_MONITOR_TYPE_DATALOST,
 };
 
+static inline bool
+xfs_healthmon_check_outbuffer_space(const struct xfs_healthmon *hm)
+{
+	return hm->bufhead + sizeof(struct xfs_health_monitor_event) <= hm->bufsize;
+}
+
 /* Render event as a V0 structure */
 STATIC int
 xfs_healthmon_format_v0(
@@ -805,10 +811,10 @@ xfs_healthmon_format_v0(
 		break;
 	}
 
-	ASSERT(hm->bufhead + sizeof(hme) <= hm->bufsize);
+	ASSERT(xfs_healthmon_check_outbuffer_space(hm));
 
 	/* copy formatted object to the outbuf */
-	if (hm->bufhead + sizeof(hme) <= hm->bufsize) {
+	if (xfs_healthmon_check_outbuffer_space(hm)) {
 		memcpy(hm->buffer + hm->bufhead, &hme, sizeof(hme));
 		hm->bufhead += sizeof(hme);
 	}
@@ -891,7 +897,11 @@ xfs_healthmon_format_pop(
 {
 	struct xfs_healthmon_event *event;
 
-	if (hm->bufhead + sizeof(*event) > hm->bufsize)
+	/*
+	 * Don't bother if there's not enough space to format even one event in
+	 * the outbuffer.
+	 */
+	if (!xfs_healthmon_check_outbuffer_space(hm))
 		return NULL;
 
 	mutex_lock(&hm->lock);

[PATCH 3/5] xfs: bump lost_prev_errors if we lose even the healthmon lost event

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

LOLLM observes that we don't bump xfs_healthmon::lost_prev_event even if
we can't allocate or queue a LOST event, which means that events can
disappear silently when things are going very wrong.  Bump the counter
to avoid this problem.

Cc: <stable@vger.kernel.org> # v7.0
Fixes: b3a289a2a9397b ("xfs: create event queuing, formatting, and discovery infrastructure")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/xfs_healthmon.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_healthmon.c b/fs/xfs/xfs_healthmon.c
index a2ae15a262a510..234ec20e1faed5 100644
--- a/fs/xfs/xfs_healthmon.c
+++ b/fs/xfs/xfs_healthmon.c
@@ -329,8 +329,10 @@ xfs_healthmon_clear_lost_prev(
 	if (hm->events < XFS_HEALTHMON_MAX_EVENTS)
 		event = kmemdup(&lost_event, sizeof(struct xfs_healthmon_event),
 				GFP_NOFS);
-	if (!event)
+	if (!event) {
+		xfs_healthmon_bump_lost(hm);
 		return -ENOMEM;
+	}
 
 	__xfs_healthmon_push(hm, event);
 cleared:

[PATCH 4/5] xfs: report nonexistent parents as a filesystem corruption

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

LOLLM noticed that when the directory tree scrubber tries to walk up a
parent pointer but the parent inumber doesn't point to an allocated
inode, we allow the EINVAL/ENOENT error code to bubble up to userspace.
That's not right, we should be reporting that as a cross-referencing
error so that someone runs the parent pointer checker.

Also add a termination check to xchk_dirpath_step_up because it's a loop
body function.

Cc: <stable@vger.kernel.org> # v6.10
Fixes: 928b721a11789a ("xfs: teach online scrub to find directory tree structure problems")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/trace.h   |   33 +++++++++++++++++++++++++++++++++
 fs/xfs/scrub/dirtree.c |   30 ++++++++++++++++++++++++++++--
 2 files changed, 61 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/scrub/trace.h b/fs/xfs/scrub/trace.h
index 362c6d39e9f580..0f5adc293962fa 100644
--- a/fs/xfs/scrub/trace.h
+++ b/fs/xfs/scrub/trace.h
@@ -1706,6 +1706,39 @@ DEFINE_EVENT(xchk_dirtree_class, name, \
 DEFINE_XCHK_DIRTREE_EVENT(xchk_dirtree_create_path);
 DEFINE_XCHK_DIRTREE_EVENT(xchk_dirpath_walk_upwards);
 
+TRACE_EVENT(xchk_dirpath_badino,
+	TP_PROTO(struct xfs_scrub *sc, unsigned int path_nr,
+		unsigned int step_nr, const struct xfs_name *name,
+		const struct xfs_parent_rec *pptr),
+	TP_ARGS(sc, path_nr, step_nr, name, pptr),
+	TP_STRUCT__entry(
+		__field(dev_t, dev)
+		__field(unsigned int, path_nr)
+		__field(unsigned int, step_nr)
+		__field(xfs_ino_t, parent_ino)
+		__field(unsigned int, parent_gen)
+		__field(unsigned int, namelen)
+		__dynamic_array(char, name, name->len)
+	),
+	TP_fast_assign(
+		__entry->dev = sc->mp->m_super->s_dev;
+		__entry->path_nr = path_nr;
+		__entry->step_nr = step_nr;
+		__entry->parent_ino = be64_to_cpu(pptr->p_ino);
+		__entry->parent_gen = be32_to_cpu(pptr->p_gen);
+		__entry->namelen = name->len;
+		memcpy(__get_str(name), name->name, name->len);
+	),
+	TP_printk("dev %d:%d path %u step %u parent_ino 0x%llx parent_gen 0x%x name '%.*s'",
+		  MAJOR(__entry->dev), MINOR(__entry->dev),
+		  __entry->path_nr,
+		  __entry->step_nr,
+		  __entry->parent_ino,
+		  __entry->parent_gen,
+		  __entry->namelen,
+		  __get_str(name))
+);
+
 DECLARE_EVENT_CLASS(xchk_dirpath_class,
 	TP_PROTO(struct xfs_scrub *sc, struct xfs_inode *ip,
 		 unsigned int path_nr, unsigned int step_nr,
diff --git a/fs/xfs/scrub/dirtree.c b/fs/xfs/scrub/dirtree.c
index b2cf6e5439d915..717cbac2956238 100644
--- a/fs/xfs/scrub/dirtree.c
+++ b/fs/xfs/scrub/dirtree.c
@@ -368,12 +368,38 @@ xchk_dirpath_step_up(
 	struct xfs_inode	*dp;
 	xfs_ino_t		parent_ino = be64_to_cpu(dl->pptr_rec.p_ino);
 	unsigned int		lock_mode;
-	int			error;
+	int			error = 0;
+
+	if (xchk_should_terminate(sc, &error))
+		return error;
 
 	/* Grab and lock the parent directory. */
 	error = xchk_iget(sc, parent_ino, &dp);
-	if (error)
+	switch (error) {
+	case -EINVAL:
+	case -ENOENT:
+		mutex_lock(&dl->lock);
+
+		if (dl->stale) {
+			/* live update detected a change in this path */
+			error = -ESTALE;
+		} else {
+			/* inode doesn't exist, path invalid */
+			error = -EFSCORRUPTED;
+
+			trace_xchk_dirpath_badino(dl->sc, path->path_nr,
+					path->nr_steps, &dl->xname,
+					&dl->pptr_rec);
+		}
+
+		mutex_unlock(&dl->lock);
 		return error;
+	case 0:
+		/* keep going */
+		break;
+	default:
+		return error;
+	}
 
 	lock_mode = xfs_ilock_attr_map_shared(dp);
 	mutex_lock(&dl->lock);

[PATCH 5/5] xfs: destroy seen inode bitmap when we fail to add a dirpath

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

LOLLM observes a memory leak in xchk_dirtree_create_path if we create
the directory path object but appending the name to the path fails.
When this happens, we don't tear down the (empty) seen inode bitmap.
This is a pretty trivial error, but let's not leave logic bombs.

Do the same for a similar bug in xrep_dirtree_create_adoption_path.

Cc: <stable@vger.kernel.org> # v6.10
Fixes: 928b721a11789a ("xfs: teach online scrub to find directory tree structure problems")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/dirtree.c        |    1 +
 fs/xfs/scrub/dirtree_repair.c |    1 +
 2 files changed, 2 insertions(+)


diff --git a/fs/xfs/scrub/dirtree.c b/fs/xfs/scrub/dirtree.c
index 717cbac2956238..9b0ab23166124e 100644
--- a/fs/xfs/scrub/dirtree.c
+++ b/fs/xfs/scrub/dirtree.c
@@ -259,6 +259,7 @@ xchk_dirtree_create_path(
 	dl->nr_paths++;
 	return 0;
 out_path:
+	xino_bitmap_destroy(&path->seen_inodes);
 	kfree(path);
 	return error;
 }
diff --git a/fs/xfs/scrub/dirtree_repair.c b/fs/xfs/scrub/dirtree_repair.c
index bbf6acf6fd400c..8acd55b8c7696d 100644
--- a/fs/xfs/scrub/dirtree_repair.c
+++ b/fs/xfs/scrub/dirtree_repair.c
@@ -618,6 +618,7 @@ xrep_dirtree_create_adoption_path(
 	return 0;
 
 out_path:
+	xino_bitmap_destroy(&path->seen_inodes);
 	kfree(path);
 	return error;
 }

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

[Christoph Hellwig]

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

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

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

Re: [PATCH 2/5] xfs: check healthmon outbuffer space correctly

[Christoph Hellwig]

On Mon, Aug 24, 2026 at 10:36:28PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> LOLLM notices that the outbuf space check in xfs_healthmon_format_pop
> isn't quite correct -- it checks that there's enough space to write a
> xfs_healthmon_event object, but the outbuffer is supposed to contain
> xfs_health_monitor_event objects.  Fix this by adding a helper, and
> refactoring all three outbuf size checks to use it.
> 
> Cc: <stable@vger.kernel.org> # v7.0
> Fixes: b3a289a2a9397b ("xfs: create event queuing, formatting, and discovery infrastructure")
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> Assisted-by: LOLLM # finding obvious bugs
> ---
>  fs/xfs/xfs_healthmon.c |   16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_healthmon.c b/fs/xfs/xfs_healthmon.c
> index 166ef0d5864486..a2ae15a262a510 100644
> --- a/fs/xfs/xfs_healthmon.c
> +++ b/fs/xfs/xfs_healthmon.c
> @@ -739,6 +739,12 @@ static const unsigned int type_map[] = {
>  	[XFS_HEALTHMON_DATALOST]	= XFS_HEALTH_MONITOR_TYPE_DATALOST,
>  };
>  
> +static inline bool
> +xfs_healthmon_check_outbuffer_space(const struct xfs_healthmon *hm)
> +{
> +	return hm->bufhead + sizeof(struct xfs_health_monitor_event) <= hm->bufsize;

Overly long line.

>  		memcpy(hm->buffer + hm->bufhead, &hme, sizeof(hme));
>  		hm->bufhead += sizeof(hme);
>  	}
> @@ -891,7 +897,11 @@ xfs_healthmon_format_pop(
>  {
>  	struct xfs_healthmon_event *event;
>  
> -	if (hm->bufhead + sizeof(*event) > hm->bufsize)
> +	/*
> +	 * Don't bother if there's not enough space to format even one event in
> +	 * the outbuffer.
> +	 */
> +	if (!xfs_healthmon_check_outbuffer_space(hm))
>  		return NULL;
>  
>  	mutex_lock(&hm->lock);

This is a bit annoying as we now require the type name instead of an
object, and the v0 implies there could be other formats.  But I guess
for now it is fine..

Re: [PATCH 3/5] xfs: bump lost_prev_errors if we lose even the healthmon lost event

[Christoph Hellwig]

On Mon, Aug 24, 2026 at 10:36:43PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> LOLLM observes that we don't bump xfs_healthmon::lost_prev_event even if
> we can't allocate or queue a LOST event, which means that events can
> disappear silently when things are going very wrong.  Bump the counter
> to avoid this problem.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 4/5] xfs: report nonexistent parents as a filesystem corruption

[Christoph Hellwig]

On Mon, Aug 24, 2026 at 10:36:59PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> LOLLM noticed that when the directory tree scrubber tries to walk up a
> parent pointer but the parent inumber doesn't point to an allocated
> inode, we allow the EINVAL/ENOENT error code to bubble up to userspace.
> That's not right, we should be reporting that as a cross-referencing
> error so that someone runs the parent pointer checker.

Test case?

Either way, the fix does looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 5/5] xfs: destroy seen inode bitmap when we fail to add a dirpath

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Darrick J. Wong]

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

Re: [PATCH 2/5] xfs: check healthmon outbuffer space correctly

[Darrick J. Wong]

On Tue, Aug 25, 2026 at 08:44:51AM +0200, Christoph Hellwig wrote:
> On Mon, Aug 24, 2026 at 10:36:28PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > LOLLM notices that the outbuf space check in xfs_healthmon_format_pop
> > isn't quite correct -- it checks that there's enough space to write a
> > xfs_healthmon_event object, but the outbuffer is supposed to contain
> > xfs_health_monitor_event objects.  Fix this by adding a helper, and
> > refactoring all three outbuf size checks to use it.
> > 
> > Cc: <stable@vger.kernel.org> # v7.0
> > Fixes: b3a289a2a9397b ("xfs: create event queuing, formatting, and discovery infrastructure")
> > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> > Assisted-by: LOLLM # finding obvious bugs
> > ---
> >  fs/xfs/xfs_healthmon.c |   16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_healthmon.c b/fs/xfs/xfs_healthmon.c
> > index 166ef0d5864486..a2ae15a262a510 100644
> > --- a/fs/xfs/xfs_healthmon.c
> > +++ b/fs/xfs/xfs_healthmon.c
> > @@ -739,6 +739,12 @@ static const unsigned int type_map[] = {
> >  	[XFS_HEALTHMON_DATALOST]	= XFS_HEALTH_MONITOR_TYPE_DATALOST,
> >  };
> >  
> > +static inline bool
> > +xfs_healthmon_check_outbuffer_space(const struct xfs_healthmon *hm)
> > +{
> > +	return hm->bufhead + sizeof(struct xfs_health_monitor_event) <= hm->bufsize;
> 
> Overly long line.

Will fix.

> >  		memcpy(hm->buffer + hm->bufhead, &hme, sizeof(hme));
> >  		hm->bufhead += sizeof(hme);
> >  	}
> > @@ -891,7 +897,11 @@ xfs_healthmon_format_pop(
> >  {
> >  	struct xfs_healthmon_event *event;
> >  
> > -	if (hm->bufhead + sizeof(*event) > hm->bufsize)
> > +	/*
> > +	 * Don't bother if there's not enough space to format even one event in
> > +	 * the outbuffer.
> > +	 */
> > +	if (!xfs_healthmon_check_outbuffer_space(hm))
> >  		return NULL;
> >  
> >  	mutex_lock(&hm->lock);
> 
> This is a bit annoying as we now require the type name instead of an
> object, and the v0 implies there could be other formats.  But I guess
> for now it is fine..

<nod> v0 is the only format, eventually that helper would end up looking
like this:

static inline bool
xfs_healthmon_check_outbuffer_space(const struct xfs_healthmon *hm)
{
	size_t	event_sz = 0;

	switch (hm->format) {
	case XFS_HEALTHMON_V0:
		event_sz = sizeof(struct xfs_health_monitor_event);
		break;
	case XFS_HEALTHMON_V1:
		event_sz = sizeof(struct xfs_health_monitor_event_v1);
		break;
	default:
		ASSERT(0);
		break;
	}

	return hm->bufhead + event_sz <= hm->bufsize;
}

--D

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

[Christoph Hellwig]

On Tue, Aug 25, 2026 at 11:33:08AM -0700, Darrick J. Wong wrote:
> 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."

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

Ok, me forgetting about two different ways to insert events was the
reason I had a really hard time with the commit log.

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

I think we should do this.  And merge __xfs_healthmon_push and
__xfs_healthmon_insert that has a at_head argument to switch between
list_add vs list_add_tail.  Thart would make this code much easier
to understand and maintain.

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

[Darrick J. Wong]

On Wed, Aug 26, 2026 at 06:48:20AM +0200, Christoph Hellwig wrote:
> On Tue, Aug 25, 2026 at 11:33:08AM -0700, Darrick J. Wong wrote:
> > 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."
> 
> > > 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.
> 
> Ok, me forgetting about two different ways to insert events was the
> reason I had a really hard time with the commit log.

<nod>

> > 
> > > 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.
> 
> I think we should do this.  And merge __xfs_healthmon_push and
> __xfs_healthmon_insert that has a at_head argument to switch between
> list_add vs list_add_tail.  Thart would make this code much easier
> to understand and maintain.

Ok.  Do you want me to mash the list conversion into this patch?  Or
just do the bare fix here and the list_head stuff in the next patch?

--D

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

[Christoph Hellwig]

On Tue, Aug 25, 2026 at 09:54:32PM -0700, Darrick J. Wong wrote:
> > I think we should do this.  And merge __xfs_healthmon_push and
> > __xfs_healthmon_insert that has a at_head argument to switch between
> > list_add vs list_add_tail.  Thart would make this code much easier
> > to understand and maintain.
> 
> Ok.  Do you want me to mash the list conversion into this patch?  Or
> just do the bare fix here and the list_head stuff in the next patch?

Either way is fine.  Personally I'd just fix it by using the library
routines and probably split out the cleanup (merging the helpers),
but if you prefer fixing the code before removing it I won't object.