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

[PATCHSET v2 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...

v2: add rvbs, tweak commit messages, do more cleanup of healthmon lists

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: port healthmon event list to list_head
 * xfs: check healthmon outbuffer space correctly
 * xfs: merge healthmon insert/push helpers
 * 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/xfs_healthmon.h        |    5 +-
 fs/xfs/xfs_trace.h            |    4 +
 fs/xfs/scrub/dirtree.c        |   31 +++++++++-
 fs/xfs/scrub/dirtree_repair.c |    1 
 fs/xfs/xfs_healthmon.c        |  131 ++++++++++++++++++++++++-----------------
 6 files changed, 144 insertions(+), 61 deletions(-)

Unreviewed patches in this series:

[PATCHSET v2 2/2] xfs: LLM-inspired bug fixes, part 8
  [PATCH 1/7] xfs: always set xfs_healthmon::first_event when inserting
  [PATCH 2/7] xfs: port healthmon event list to list_head
  [PATCH 3/7] xfs: check healthmon outbuffer space correctly
  [PATCH 4/7] xfs: merge healthmon insert/push helpers

[PATCH 1/7] 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 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 so we never see the new event.

Fix this by always reassigning first_event.  A subsequent patch will
clean this up to use a standard list_head, but I felt it important to
call out the bug fix first.

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 3ae5f4496ad1aa..a4efc084a8fc0b 100644
--- a/fs/xfs/xfs_healthmon.c
+++ b/fs/xfs/xfs_healthmon.c
@@ -278,8 +278,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/7] xfs: port healthmon event list to list_head

[Darrick J. Wong]

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

Simplify the healthmon codebase by porting the single-link event list to
a standard list_head.

Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/xfs_healthmon.h |    5 +--
 fs/xfs/xfs_healthmon.c |   73 +++++++++++++++++++++++++++++-------------------
 2 files changed, 46 insertions(+), 32 deletions(-)


diff --git a/fs/xfs/xfs_healthmon.h b/fs/xfs/xfs_healthmon.h
index 0e936507037fda..ef90ab03c2b399 100644
--- a/fs/xfs/xfs_healthmon.h
+++ b/fs/xfs/xfs_healthmon.h
@@ -31,8 +31,7 @@ struct xfs_healthmon {
 	struct mutex			lock;
 
 	/* list of event objects */
-	struct xfs_healthmon_event	*first_event;
-	struct xfs_healthmon_event	*last_event;
+	struct list_head		event_list;
 
 	/* preallocated event for unmount */
 	struct xfs_healthmon_event	*unmount_event;
@@ -110,7 +109,7 @@ enum xfs_healthmon_domain {
 };
 
 struct xfs_healthmon_event {
-	struct xfs_healthmon_event	*next;
+	struct list_head		list;
 
 	enum xfs_healthmon_type		type;
 	enum xfs_healthmon_domain	domain;
diff --git a/fs/xfs/xfs_healthmon.c b/fs/xfs/xfs_healthmon.c
index a4efc084a8fc0b..ef2538a1ea6994 100644
--- a/fs/xfs/xfs_healthmon.c
+++ b/fs/xfs/xfs_healthmon.c
@@ -87,12 +87,10 @@ xfs_healthmon_put(
 	struct xfs_healthmon		*hm)
 {
 	if (refcount_dec_and_test(&hm->ref)) {
-		struct xfs_healthmon_event	*event;
-		struct xfs_healthmon_event	*next = hm->first_event;
+		struct xfs_healthmon_event	*event, *s;
 
-		while ((event = next) != NULL) {
+		list_for_each_entry_safe(event, s, &hm->event_list, list) {
 			trace_xfs_healthmon_drop(hm, event);
-			next = event->next;
 			kfree(event);
 		}
 
@@ -173,9 +171,13 @@ static inline void xfs_healthmon_bump_lost(struct xfs_healthmon *hm)
  */
 static bool
 xfs_healthmon_merge_events(
-	struct xfs_healthmon_event		*existing,
+	struct xfs_healthmon			*hm,
 	const struct xfs_healthmon_event	*new)
 {
+	struct xfs_healthmon_event		*existing =
+		list_last_entry_or_null(&hm->event_list, struct
+				xfs_healthmon_event, list);
+
 	if (!existing)
 		return false;
 
@@ -277,10 +279,7 @@ __xfs_healthmon_insert(
 	ktime_get_coarse_real_ts64(&now);
 	event->time_ns = (now.tv_sec * NSEC_PER_SEC) + now.tv_nsec;
 
-	event->next = hm->first_event;
-	hm->first_event = event;
-	if (!hm->last_event)
-		hm->last_event = event;
+	list_add(&event->list, &hm->event_list);
 	xfs_healthmon_bump_events(hm);
 	wake_up(&hm->wait);
 
@@ -300,18 +299,36 @@ __xfs_healthmon_push(
 	ktime_get_coarse_real_ts64(&now);
 	event->time_ns = (now.tv_sec * NSEC_PER_SEC) + now.tv_nsec;
 
-	if (!hm->first_event)
-		hm->first_event = event;
-	if (hm->last_event)
-		hm->last_event->next = event;
-	hm->last_event = event;
-	event->next = NULL;
+	list_add_tail(&event->list, &hm->event_list);
 	xfs_healthmon_bump_events(hm);
 	wake_up(&hm->wait);
 
 	trace_xfs_healthmon_push(hm, event);
 }
 
+static inline struct xfs_healthmon_event *xfs_healthmon_alloc_event(void)
+{
+	struct xfs_healthmon_event		*event =
+		kzalloc_obj(struct xfs_healthmon_event, GFP_NOFS);
+
+	if (event)
+		INIT_LIST_HEAD(&event->list);
+	return event;
+}
+
+static inline struct xfs_healthmon_event *
+xfs_healthmon_dup_event(
+	const struct xfs_healthmon_event	*template)
+{
+	struct xfs_healthmon_event		*event =
+		kmemdup(template, sizeof(struct xfs_healthmon_event),
+				GFP_NOFS);
+
+	if (event)
+		INIT_LIST_HEAD(&event->list);
+	return event;
+}
+
 /* Deal with any previously lost events */
 static int
 xfs_healthmon_clear_lost_prev(
@@ -324,15 +341,14 @@ xfs_healthmon_clear_lost_prev(
 	};
 	struct xfs_healthmon_event	*event = NULL;
 
-	if (xfs_healthmon_merge_events(hm->last_event, &lost_event)) {
-		trace_xfs_healthmon_merge(hm, hm->last_event);
+	if (xfs_healthmon_merge_events(hm, &lost_event)) {
+		trace_xfs_healthmon_merge(hm, &lost_event);
 		wake_up(&hm->wait);
 		goto cleared;
 	}
 
 	if (hm->events < XFS_HEALTHMON_MAX_EVENTS)
-		event = kmemdup(&lost_event, sizeof(struct xfs_healthmon_event),
-				GFP_NOFS);
+		event = xfs_healthmon_dup_event(&lost_event);
 	if (!event)
 		return -ENOMEM;
 
@@ -372,16 +388,15 @@ xfs_healthmon_push(
 	}
 
 	/* Try to merge with the newest event */
-	if (xfs_healthmon_merge_events(hm->last_event, template)) {
-		trace_xfs_healthmon_merge(hm, hm->last_event);
+	if (xfs_healthmon_merge_events(hm, template)) {
+		trace_xfs_healthmon_merge(hm, template);
 		wake_up(&hm->wait);
 		goto out_unlock;
 	}
 
 	/* Only create a heap event object if we're not already at capacity. */
 	if (hm->events < XFS_HEALTHMON_MAX_EVENTS)
-		event = kmemdup(template, sizeof(struct xfs_healthmon_event),
-				GFP_NOFS);
+		event = xfs_healthmon_dup_event(template);
 	if (!event) {
 		/* No memory means we lose the event */
 		trace_xfs_healthmon_lost_event(hm);
@@ -899,11 +914,10 @@ xfs_healthmon_format_pop(
 		return NULL;
 
 	mutex_lock(&hm->lock);
-	event = hm->first_event;
+	event = list_first_entry_or_null(&hm->event_list,
+			struct xfs_healthmon_event, list);
 	if (event) {
-		if (hm->last_event == event)
-			hm->last_event = NULL;
-		hm->first_event = event->next;
+		list_del_init(&event->list);
 		hm->events--;
 
 		trace_xfs_healthmon_pop(hm, event);
@@ -1203,6 +1217,7 @@ xfs_ioc_health_monitor(
 		return -ENOMEM;
 	hm->dev = mp->m_super->s_dev;
 	refcount_set(&hm->ref, 1);
+	INIT_LIST_HEAD(&hm->event_list);
 
 	mutex_init(&hm->lock);
 	init_waitqueue_head(&hm->wait);
@@ -1211,7 +1226,7 @@ xfs_ioc_health_monitor(
 		hm->verbose = true;
 
 	/* Queue up the first event that lets the client know we're running. */
-	running_event = kzalloc_obj(struct xfs_healthmon_event, GFP_NOFS);
+	running_event = xfs_healthmon_alloc_event();
 	if (!running_event) {
 		ret = -ENOMEM;
 		goto out_hm;
@@ -1225,7 +1240,7 @@ xfs_ioc_health_monitor(
 	 * filesystem later.  This is key for triggering fast exit of the
 	 * xfs_healer daemon.
 	 */
-	hm->unmount_event = kzalloc_obj(struct xfs_healthmon_event, GFP_NOFS);
+	hm->unmount_event = xfs_healthmon_alloc_event();
 	if (!hm->unmount_event) {
 		ret = -ENOMEM;
 		goto out_hm;

[PATCH 3/7] 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 |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)


diff --git a/fs/xfs/xfs_healthmon.c b/fs/xfs/xfs_healthmon.c
index ef2538a1ea6994..d0e0ecc6fc0c8f 100644
--- a/fs/xfs/xfs_healthmon.c
+++ b/fs/xfs/xfs_healthmon.c
@@ -758,6 +758,13 @@ 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(
@@ -824,10 +831,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);
 	}
@@ -910,7 +917,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 4/7] xfs: merge healthmon insert/push helpers

[Darrick J. Wong]

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

These functions are basically the same except for where in the queue the
new event is added.  Refactor them as a single function that takes an
action verb to tell us where; and rename the tracepoints to describe
directly what happens.

Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/xfs_trace.h     |    4 ++--
 fs/xfs/xfs_healthmon.c |   46 +++++++++++++++++++++-------------------------
 2 files changed, 23 insertions(+), 27 deletions(-)


diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index aeb89ac53bf190..02f54bc5aa91c6 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -6139,8 +6139,8 @@ DEFINE_EVENT(xfs_healthmon_event_class, name, \
 	TP_PROTO(const struct xfs_healthmon *hm, \
 		 const struct xfs_healthmon_event *event), \
 	TP_ARGS(hm, event))
-DEFINE_HEALTHMONEVENT_EVENT(xfs_healthmon_insert);
-DEFINE_HEALTHMONEVENT_EVENT(xfs_healthmon_push);
+DEFINE_HEALTHMONEVENT_EVENT(xfs_healthmon_insert_head);
+DEFINE_HEALTHMONEVENT_EVENT(xfs_healthmon_insert_tail);
 DEFINE_HEALTHMONEVENT_EVENT(xfs_healthmon_pop);
 DEFINE_HEALTHMONEVENT_EVENT(xfs_healthmon_format);
 DEFINE_HEALTHMONEVENT_EVENT(xfs_healthmon_format_overflow);
diff --git a/fs/xfs/xfs_healthmon.c b/fs/xfs/xfs_healthmon.c
index d0e0ecc6fc0c8f..190779f2e13655 100644
--- a/fs/xfs/xfs_healthmon.c
+++ b/fs/xfs/xfs_healthmon.c
@@ -266,10 +266,16 @@ xfs_healthmon_merge_events(
 	return false;
 }
 
-/* Insert an event onto the start of the queue. */
+enum insert_where {
+	INSERT_HEAD,
+	INSERT_TAIL,
+};
+
+/* Add an event onto the start or the end of the queue. */
 static inline void
 __xfs_healthmon_insert(
 	struct xfs_healthmon		*hm,
+	enum insert_where		where,
 	struct xfs_healthmon_event	*event)
 {
 	struct timespec64		now;
@@ -279,31 +285,21 @@ __xfs_healthmon_insert(
 	ktime_get_coarse_real_ts64(&now);
 	event->time_ns = (now.tv_sec * NSEC_PER_SEC) + now.tv_nsec;
 
-	list_add(&event->list, &hm->event_list);
-	xfs_healthmon_bump_events(hm);
-	wake_up(&hm->wait);
+	switch (where) {
+	case INSERT_HEAD:
+		trace_xfs_healthmon_insert_head(hm, event);
 
-	trace_xfs_healthmon_insert(hm, event);
-}
+		list_add(&event->list, &hm->event_list);
+		break;
+	case INSERT_TAIL:
+		trace_xfs_healthmon_insert_tail(hm, event);
 
-/* Push an event onto the end of the queue. */
-static inline void
-__xfs_healthmon_push(
-	struct xfs_healthmon		*hm,
-	struct xfs_healthmon_event	*event)
-{
-	struct timespec64		now;
+		list_add_tail(&event->list, &hm->event_list);
+		break;
+	}
 
-	lockdep_assert_held(&hm->lock);
-
-	ktime_get_coarse_real_ts64(&now);
-	event->time_ns = (now.tv_sec * NSEC_PER_SEC) + now.tv_nsec;
-
-	list_add_tail(&event->list, &hm->event_list);
 	xfs_healthmon_bump_events(hm);
 	wake_up(&hm->wait);
-
-	trace_xfs_healthmon_push(hm, event);
 }
 
 static inline struct xfs_healthmon_event *xfs_healthmon_alloc_event(void)
@@ -352,7 +348,7 @@ xfs_healthmon_clear_lost_prev(
 	if (!event)
 		return -ENOMEM;
 
-	__xfs_healthmon_push(hm, event);
+	__xfs_healthmon_insert(hm, INSERT_TAIL, event);
 cleared:
 	hm->lost_prev_event = 0;
 	return 0;
@@ -405,7 +401,7 @@ xfs_healthmon_push(
 		goto out_unlock;
 	}
 
-	__xfs_healthmon_push(hm, event);
+	__xfs_healthmon_insert(hm, INSERT_TAIL, event);
 
 out_unlock:
 	mutex_unlock(&hm->lock);
@@ -434,7 +430,7 @@ xfs_healthmon_unmount(
 	 * we've inserted the unmount event, hm no longer owns that event.
 	 */
 	mutex_lock(&hm->lock);
-	__xfs_healthmon_insert(hm, hm->unmount_event);
+	__xfs_healthmon_insert(hm, INSERT_HEAD, hm->unmount_event);
 	hm->unmount_event = NULL;
 	mutex_unlock(&hm->lock);
 
@@ -1244,7 +1240,7 @@ xfs_ioc_health_monitor(
 	}
 	running_event->type = XFS_HEALTHMON_RUNNING;
 	running_event->domain = XFS_HEALTHMON_MOUNT;
-	__xfs_healthmon_insert(hm, running_event);
+	__xfs_healthmon_insert(hm, INSERT_HEAD, running_event);
 
 	/*
 	 * Preallocate the unmount event so that we can't fail to notify the

[PATCH 5/7] 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
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 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 190779f2e13655..38b2ce1263b0cd 100644
--- a/fs/xfs/xfs_healthmon.c
+++ b/fs/xfs/xfs_healthmon.c
@@ -345,8 +345,10 @@ xfs_healthmon_clear_lost_prev(
 
 	if (hm->events < XFS_HEALTHMON_MAX_EVENTS)
 		event = xfs_healthmon_dup_event(&lost_event);
-	if (!event)
+	if (!event) {
+		xfs_healthmon_bump_lost(hm);
 		return -ENOMEM;
+	}
 
 	__xfs_healthmon_insert(hm, INSERT_TAIL, event);
 cleared:

[PATCH 6/7] 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
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 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 7/7] 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
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 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 2/7] xfs: port healthmon event list to list_head

[Anuj Gupta]

> -	if (xfs_healthmon_merge_events(hm->last_event, &lost_event)) {
> -		trace_xfs_healthmon_merge(hm, hm->last_event);
> +	if (xfs_healthmon_merge_events(hm, &lost_event)) {
> +		trace_xfs_healthmon_merge(hm, &lost_event);

This traces the lost_event now, but the merge updates existing tail
event. Is the change in behaviour intentional?

>  	/* Try to merge with the newest event */
> -	if (xfs_healthmon_merge_events(hm->last_event, template)) {
> -		trace_xfs_healthmon_merge(hm, hm->last_event);
> +	if (xfs_healthmon_merge_events(hm, template)) {
> +		trace_xfs_healthmon_merge(hm, template);

Same: the merge updates the existing tail event, but this traces the
incoming template/event. Before this patch, surviving post-merge tail
was traced.

Re: [PATCH 2/7] xfs: port healthmon event list to list_head

[Darrick J. Wong]

On Thu, Aug 27, 2026 at 03:03:46PM +0530, Anuj Gupta wrote:
> > -	if (xfs_healthmon_merge_events(hm->last_event, &lost_event)) {
> > -		trace_xfs_healthmon_merge(hm, hm->last_event);
> > +	if (xfs_healthmon_merge_events(hm, &lost_event)) {
> > +		trace_xfs_healthmon_merge(hm, &lost_event);
> 
> This traces the lost_event now, but the merge updates existing tail
> event. Is the change in behaviour intentional?
> 
> >  	/* Try to merge with the newest event */
> > -	if (xfs_healthmon_merge_events(hm->last_event, template)) {
> > -		trace_xfs_healthmon_merge(hm, hm->last_event);
> > +	if (xfs_healthmon_merge_events(hm, template)) {
> > +		trace_xfs_healthmon_merge(hm, template);
> 
> Same: the merge updates the existing tail event, but this traces the
> incoming template/event. Before this patch, surviving post-merge tail
> was traced.

Oops, that should become list_last_entry() for both.

--D

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

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 2/7] xfs: port healthmon event list to list_head

[Christoph Hellwig]

On Wed, Aug 26, 2026 at 10:32:59PM -0700, Darrick J. Wong wrote:
>  struct xfs_healthmon_event {
> -	struct xfs_healthmon_event	*next;
> +	struct list_head		list;

Call this entry to make clear it's a list entry and not the list?

> +static inline struct xfs_healthmon_event *xfs_healthmon_alloc_event(void)
> +{
> +	struct xfs_healthmon_event		*event =
> +		kzalloc_obj(struct xfs_healthmon_event, GFP_NOFS);
> +
> +	if (event)
> +		INIT_LIST_HEAD(&event->list);
> +	return event;

There is no need to initialize a list_head only used as an entry.

> +}
> +
> +static inline struct xfs_healthmon_event *
> +xfs_healthmon_dup_event(
> +	const struct xfs_healthmon_event	*template)
> +{
> +	struct xfs_healthmon_event		*event =
> +		kmemdup(template, sizeof(struct xfs_healthmon_event),
> +				GFP_NOFS);
> +
> +	if (event)
> +		INIT_LIST_HEAD(&event->list);
> +	return event;

Same here.

Re: [PATCH 2/7] xfs: port healthmon event list to list_head

[Christoph Hellwig]

On Thu, Aug 27, 2026 at 07:47:59AM -0700, Darrick J. Wong wrote:
> On Thu, Aug 27, 2026 at 03:03:46PM +0530, Anuj Gupta wrote:
> > > -	if (xfs_healthmon_merge_events(hm->last_event, &lost_event)) {
> > > -		trace_xfs_healthmon_merge(hm, hm->last_event);
> > > +	if (xfs_healthmon_merge_events(hm, &lost_event)) {
> > > +		trace_xfs_healthmon_merge(hm, &lost_event);
> > 
> > This traces the lost_event now, but the merge updates existing tail
> > event. Is the change in behaviour intentional?
> > 
> > >  	/* Try to merge with the newest event */
> > > -	if (xfs_healthmon_merge_events(hm->last_event, template)) {
> > > -		trace_xfs_healthmon_merge(hm, hm->last_event);
> > > +	if (xfs_healthmon_merge_events(hm, template)) {
> > > +		trace_xfs_healthmon_merge(hm, template);
> > 
> > Same: the merge updates the existing tail event, but this traces the
> > incoming template/event. Before this patch, surviving post-merge tail
> > was traced.
> 
> Oops, that should become list_last_entry() for both.

Maybe move the trace inside the helper to make it more
clear?

Re: [PATCH 3/7] xfs: check healthmon outbuffer space correctly

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 4/7] xfs: merge healthmon insert/push helpers

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 2/7] xfs: port healthmon event list to list_head

[Darrick J. Wong]

On Mon, Aug 31, 2026 at 08:58:41AM +0200, Christoph Hellwig wrote:
> On Thu, Aug 27, 2026 at 07:47:59AM -0700, Darrick J. Wong wrote:
> > On Thu, Aug 27, 2026 at 03:03:46PM +0530, Anuj Gupta wrote:
> > > > -	if (xfs_healthmon_merge_events(hm->last_event, &lost_event)) {
> > > > -		trace_xfs_healthmon_merge(hm, hm->last_event);
> > > > +	if (xfs_healthmon_merge_events(hm, &lost_event)) {
> > > > +		trace_xfs_healthmon_merge(hm, &lost_event);
> > > 
> > > This traces the lost_event now, but the merge updates existing tail
> > > event. Is the change in behaviour intentional?
> > > 
> > > >  	/* Try to merge with the newest event */
> > > > -	if (xfs_healthmon_merge_events(hm->last_event, template)) {
> > > > -		trace_xfs_healthmon_merge(hm, hm->last_event);
> > > > +	if (xfs_healthmon_merge_events(hm, template)) {
> > > > +		trace_xfs_healthmon_merge(hm, template);
> > > 
> > > Same: the merge updates the existing tail event, but this traces the
> > > incoming template/event. Before this patch, surviving post-merge tail
> > > was traced.
> > 
> > Oops, that should become list_last_entry() for both.
> 
> Maybe move the trace inside the helper to make it more
> clear?

Will do.

--D

Re: [PATCH 2/7] xfs: port healthmon event list to list_head

[Darrick J. Wong]

On Mon, Aug 31, 2026 at 08:58:26AM +0200, Christoph Hellwig wrote:
> On Wed, Aug 26, 2026 at 10:32:59PM -0700, Darrick J. Wong wrote:
> >  struct xfs_healthmon_event {
> > -	struct xfs_healthmon_event	*next;
> > +	struct list_head		list;
> 
> Call this entry to make clear it's a list entry and not the list?
> 
> > +static inline struct xfs_healthmon_event *xfs_healthmon_alloc_event(void)
> > +{
> > +	struct xfs_healthmon_event		*event =
> > +		kzalloc_obj(struct xfs_healthmon_event, GFP_NOFS);
> > +
> > +	if (event)
> > +		INIT_LIST_HEAD(&event->list);
> > +	return event;
> 
> There is no need to initialize a list_head only used as an entry.
> 
> > +}
> > +
> > +static inline struct xfs_healthmon_event *
> > +xfs_healthmon_dup_event(
> > +	const struct xfs_healthmon_event	*template)
> > +{
> > +	struct xfs_healthmon_event		*event =
> > +		kmemdup(template, sizeof(struct xfs_healthmon_event),
> > +				GFP_NOFS);
> > +
> > +	if (event)
> > +		INIT_LIST_HEAD(&event->list);
> > +	return event;
> 
> Same here.

Will address these, thanks. :)

--D