* [PATCH 6.1 CANDIDATE 1/3] xfs: explicitly specify cpu when forcing inodegc delayed work to run immediately
2023-07-12 9:47 [PATCH 6.1 CANDIDATE 0/3] xfs inodegc fixes for 6.1.y (from v6.4) Amir Goldstein
@ 2023-07-12 9:47 ` Amir Goldstein
2023-07-12 9:47 ` [PATCH 6.1 CANDIDATE 2/3] xfs: check that per-cpu inodegc workers actually run on that cpu Amir Goldstein
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Amir Goldstein @ 2023-07-12 9:47 UTC (permalink / raw)
To: Darrick J . Wong
Cc: Chandan Babu R, Leah Rumancik, Dave Chinner, Chris Dunlop,
linux-xfs, Dave Chinner
From: "Darrick J. Wong" <djwong@kernel.org>
commit 03e0add80f4cf3f7393edb574eeb3a89a1db7758 upstream.
I've been noticing odd racing behavior in the inodegc code that could
only be explained by one cpu adding an inode to its inactivation llist
at the same time that another cpu is processing that cpu's llist.
Preemption is disabled between get/put_cpu_ptr, so the only explanation
is scheduler mayhem. I inserted the following debug code into
xfs_inodegc_worker (see the next patch):
ASSERT(gc->cpu == smp_processor_id());
This assertion tripped during overnight tests on the arm64 machines, but
curiously not on x86_64. I think we haven't observed any resource leaks
here because the lockfree list code can handle simultaneous llist_add
and llist_del_all functions operating on the same list. However, the
whole point of having percpu inodegc lists is to take advantage of warm
memory caches by inactivating inodes on the last processor to touch the
inode.
The incorrect scheduling seems to occur after an inodegc worker is
subjected to mod_delayed_work(). This wraps mod_delayed_work_on with
WORK_CPU_UNBOUND specified as the cpu number. Unbound allows for
scheduling on any cpu, not necessarily the same one that scheduled the
work.
Because preemption is disabled for as long as we have the gc pointer, I
think it's safe to use current_cpu() (aka smp_processor_id) to queue the
delayed work item on the correct cpu.
Fixes: 7cf2b0f9611b ("xfs: bound maximum wait time for inodegc work")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
fs/xfs/xfs_icache.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index eae7427062cf..536885f8b8a8 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -2052,7 +2052,8 @@ xfs_inodegc_queue(
queue_delay = 0;
trace_xfs_inodegc_queue(mp, __return_address);
- mod_delayed_work(mp->m_inodegc_wq, &gc->work, queue_delay);
+ mod_delayed_work_on(current_cpu(), mp->m_inodegc_wq, &gc->work,
+ queue_delay);
put_cpu_ptr(gc);
if (xfs_inodegc_want_flush_work(ip, items, shrinker_hits)) {
@@ -2096,7 +2097,8 @@ xfs_inodegc_cpu_dead(
if (xfs_is_inodegc_enabled(mp)) {
trace_xfs_inodegc_queue(mp, __return_address);
- mod_delayed_work(mp->m_inodegc_wq, &gc->work, 0);
+ mod_delayed_work_on(current_cpu(), mp->m_inodegc_wq, &gc->work,
+ 0);
}
put_cpu_ptr(gc);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 6.1 CANDIDATE 2/3] xfs: check that per-cpu inodegc workers actually run on that cpu
2023-07-12 9:47 [PATCH 6.1 CANDIDATE 0/3] xfs inodegc fixes for 6.1.y (from v6.4) Amir Goldstein
2023-07-12 9:47 ` [PATCH 6.1 CANDIDATE 1/3] xfs: explicitly specify cpu when forcing inodegc delayed work to run immediately Amir Goldstein
@ 2023-07-12 9:47 ` Amir Goldstein
2023-07-12 9:47 ` [PATCH 6.1 CANDIDATE 3/3] xfs: fix xfs_inodegc_stop racing with mod_delayed_work Amir Goldstein
2023-07-12 15:44 ` [PATCH 6.1 CANDIDATE 0/3] xfs inodegc fixes for 6.1.y (from v6.4) Darrick J. Wong
3 siblings, 0 replies; 6+ messages in thread
From: Amir Goldstein @ 2023-07-12 9:47 UTC (permalink / raw)
To: Darrick J . Wong
Cc: Chandan Babu R, Leah Rumancik, Dave Chinner, Chris Dunlop,
linux-xfs, Dave Chinner
From: "Darrick J. Wong" <djwong@kernel.org>
commit b37c4c8339cd394ea6b8b415026603320a185651 upstream.
Now that we've allegedly worked out the problem of the per-cpu inodegc
workers being scheduled on the wrong cpu, let's put in a debugging knob
to let us know if a worker ever gets mis-scheduled again.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
fs/xfs/xfs_icache.c | 2 ++
fs/xfs/xfs_mount.h | 3 +++
fs/xfs/xfs_super.c | 3 +++
3 files changed, 8 insertions(+)
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 536885f8b8a8..7ce262dcabca 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -1848,6 +1848,8 @@ xfs_inodegc_worker(
struct llist_node *node = llist_del_all(&gc->list);
struct xfs_inode *ip, *n;
+ ASSERT(gc->cpu == smp_processor_id());
+
WRITE_ONCE(gc->items, 0);
if (!node)
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 8aca2cc173ac..69ddd5319634 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -66,6 +66,9 @@ struct xfs_inodegc {
/* approximate count of inodes in the list */
unsigned int items;
unsigned int shrinker_hits;
+#if defined(DEBUG) || defined(XFS_WARN)
+ unsigned int cpu;
+#endif
};
/*
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index ee4b429a2f2c..4b179526913f 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1084,6 +1084,9 @@ xfs_inodegc_init_percpu(
for_each_possible_cpu(cpu) {
gc = per_cpu_ptr(mp->m_inodegc, cpu);
+#if defined(DEBUG) || defined(XFS_WARN)
+ gc->cpu = cpu;
+#endif
init_llist_head(&gc->list);
gc->items = 0;
INIT_DELAYED_WORK(&gc->work, xfs_inodegc_worker);
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 6.1 CANDIDATE 3/3] xfs: fix xfs_inodegc_stop racing with mod_delayed_work
2023-07-12 9:47 [PATCH 6.1 CANDIDATE 0/3] xfs inodegc fixes for 6.1.y (from v6.4) Amir Goldstein
2023-07-12 9:47 ` [PATCH 6.1 CANDIDATE 1/3] xfs: explicitly specify cpu when forcing inodegc delayed work to run immediately Amir Goldstein
2023-07-12 9:47 ` [PATCH 6.1 CANDIDATE 2/3] xfs: check that per-cpu inodegc workers actually run on that cpu Amir Goldstein
@ 2023-07-12 9:47 ` Amir Goldstein
2023-07-12 15:44 ` [PATCH 6.1 CANDIDATE 0/3] xfs inodegc fixes for 6.1.y (from v6.4) Darrick J. Wong
3 siblings, 0 replies; 6+ messages in thread
From: Amir Goldstein @ 2023-07-12 9:47 UTC (permalink / raw)
To: Darrick J . Wong
Cc: Chandan Babu R, Leah Rumancik, Dave Chinner, Chris Dunlop,
linux-xfs, Dave Chinner
From: "Darrick J. Wong" <djwong@kernel.org>
commit 2254a7396a0ca6309854948ee1c0a33fa4268cec upstream.
syzbot reported this warning from the faux inodegc shrinker that tries
to kick off inodegc work:
------------[ cut here ]------------
WARNING: CPU: 1 PID: 102 at kernel/workqueue.c:1445 __queue_work+0xd44/0x1120 kernel/workqueue.c:1444
RIP: 0010:__queue_work+0xd44/0x1120 kernel/workqueue.c:1444
Call Trace:
__queue_delayed_work+0x1c8/0x270 kernel/workqueue.c:1672
mod_delayed_work_on+0xe1/0x220 kernel/workqueue.c:1746
xfs_inodegc_shrinker_scan fs/xfs/xfs_icache.c:2212 [inline]
xfs_inodegc_shrinker_scan+0x250/0x4f0 fs/xfs/xfs_icache.c:2191
do_shrink_slab+0x428/0xaa0 mm/vmscan.c:853
shrink_slab+0x175/0x660 mm/vmscan.c:1013
shrink_one+0x502/0x810 mm/vmscan.c:5343
shrink_many mm/vmscan.c:5394 [inline]
lru_gen_shrink_node mm/vmscan.c:5511 [inline]
shrink_node+0x2064/0x35f0 mm/vmscan.c:6459
kswapd_shrink_node mm/vmscan.c:7262 [inline]
balance_pgdat+0xa02/0x1ac0 mm/vmscan.c:7452
kswapd+0x677/0xd60 mm/vmscan.c:7712
kthread+0x2e8/0x3a0 kernel/kthread.c:376
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308
This warning corresponds to this code in __queue_work:
/*
* For a draining wq, only works from the same workqueue are
* allowed. The __WQ_DESTROYING helps to spot the issue that
* queues a new work item to a wq after destroy_workqueue(wq).
*/
if (unlikely(wq->flags & (__WQ_DESTROYING | __WQ_DRAINING) &&
WARN_ON_ONCE(!is_chained_work(wq))))
return;
For this to trip, we must have a thread draining the inodedgc workqueue
and a second thread trying to queue inodegc work to that workqueue.
This can happen if freezing or a ro remount race with reclaim poking our
faux inodegc shrinker and another thread dropping an unlinked O_RDONLY
file:
Thread 0 Thread 1 Thread 2
xfs_inodegc_stop
xfs_inodegc_shrinker_scan
xfs_is_inodegc_enabled
<yes, will continue>
xfs_clear_inodegc_enabled
xfs_inodegc_queue_all
<list empty, do not queue inodegc worker>
xfs_inodegc_queue
<add to list>
xfs_is_inodegc_enabled
<no, returns>
drain_workqueue
<set WQ_DRAINING>
llist_empty
<no, will queue list>
mod_delayed_work_on(..., 0)
__queue_work
<sees WQ_DRAINING, kaboom>
In other words, everything between the access to inodegc_enabled state
and the decision to poke the inodegc workqueue requires some kind of
coordination to avoid the WQ_DRAINING state. We could perhaps introduce
a lock here, but we could also try to eliminate WQ_DRAINING from the
picture.
We could replace the drain_workqueue call with a loop that flushes the
workqueue and queues workers as long as there is at least one inode
present in the per-cpu inodegc llists. We've disabled inodegc at this
point, so we know that the number of queued inodes will eventually hit
zero as long as xfs_inodegc_start cannot reactivate the workers.
There are four callers of xfs_inodegc_start. Three of them come from the
VFS with s_umount held: filesystem thawing, failed filesystem freezing,
and the rw remount transition. The fourth caller is mounting rw (no
remount or freezing possible).
There are three callers ofs xfs_inodegc_stop. One is unmounting (no
remount or thaw possible). Two of them come from the VFS with s_umount
held: fs freezing and ro remount transition.
Hence, it is correct to replace the drain_workqueue call with a loop
that drains the inodegc llists.
Fixes: 6191cf3ad59f ("xfs: flush inodegc workqueue tasks before cancel")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
fs/xfs/xfs_icache.c | 32 +++++++++++++++++++++++++++-----
1 file changed, 27 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 7ce262dcabca..d884cba1d707 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -431,18 +431,23 @@ xfs_iget_check_free_state(
}
/* Make all pending inactivation work start immediately. */
-static void
+static bool
xfs_inodegc_queue_all(
struct xfs_mount *mp)
{
struct xfs_inodegc *gc;
int cpu;
+ bool ret = false;
for_each_online_cpu(cpu) {
gc = per_cpu_ptr(mp->m_inodegc, cpu);
- if (!llist_empty(&gc->list))
+ if (!llist_empty(&gc->list)) {
mod_delayed_work_on(cpu, mp->m_inodegc_wq, &gc->work, 0);
+ ret = true;
+ }
}
+
+ return ret;
}
/*
@@ -1894,24 +1899,41 @@ xfs_inodegc_flush(
/*
* Flush all the pending work and then disable the inode inactivation background
- * workers and wait for them to stop.
+ * workers and wait for them to stop. Caller must hold sb->s_umount to
+ * coordinate changes in the inodegc_enabled state.
*/
void
xfs_inodegc_stop(
struct xfs_mount *mp)
{
+ bool rerun;
+
if (!xfs_clear_inodegc_enabled(mp))
return;
+ /*
+ * Drain all pending inodegc work, including inodes that could be
+ * queued by racing xfs_inodegc_queue or xfs_inodegc_shrinker_scan
+ * threads that sample the inodegc state just prior to us clearing it.
+ * The inodegc flag state prevents new threads from queuing more
+ * inodes, so we queue pending work items and flush the workqueue until
+ * all inodegc lists are empty. IOWs, we cannot use drain_workqueue
+ * here because it does not allow other unserialized mechanisms to
+ * reschedule inodegc work while this draining is in progress.
+ */
xfs_inodegc_queue_all(mp);
- drain_workqueue(mp->m_inodegc_wq);
+ do {
+ flush_workqueue(mp->m_inodegc_wq);
+ rerun = xfs_inodegc_queue_all(mp);
+ } while (rerun);
trace_xfs_inodegc_stop(mp, __return_address);
}
/*
* Enable the inode inactivation background workers and schedule deferred inode
- * inactivation work if there is any.
+ * inactivation work if there is any. Caller must hold sb->s_umount to
+ * coordinate changes in the inodegc_enabled state.
*/
void
xfs_inodegc_start(
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 6.1 CANDIDATE 0/3] xfs inodegc fixes for 6.1.y (from v6.4)
2023-07-12 9:47 [PATCH 6.1 CANDIDATE 0/3] xfs inodegc fixes for 6.1.y (from v6.4) Amir Goldstein
` (2 preceding siblings ...)
2023-07-12 9:47 ` [PATCH 6.1 CANDIDATE 3/3] xfs: fix xfs_inodegc_stop racing with mod_delayed_work Amir Goldstein
@ 2023-07-12 15:44 ` Darrick J. Wong
2023-07-13 4:47 ` Amir Goldstein
3 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2023-07-12 15:44 UTC (permalink / raw)
To: Amir Goldstein
Cc: Chandan Babu R, Leah Rumancik, Dave Chinner, Chris Dunlop,
linux-xfs
On Wed, Jul 12, 2023 at 12:47:30PM +0300, Amir Goldstein wrote:
> Darrick,
>
> These are the patches we discussed that Leah requested for the 5.15.y
> backport of non-blocking inodegc pushes series [1].
>
> They may or may not help the 5.15.y -> 6.1.y regression that was
> reported by Chris [2].
>
> Note that I did not include:
> 2d5f38a31980 ("xfs: disable reaping in fscounters scrub")
> in this backport set, because I generally do not want to deal with
> backporting fixes for experimental features.
I don't agree with this decision because the comment for
xfs_inodegc_stop now says that callers must hold s_umount.
xchk_stop_reaping definitely does /not/ hold that lock, which means it's
now buggy. Someone downstream could be using scrub, even if it's still
experimental.
I've generally said not to bother with scrub fixes, but I don't think
it's correct to introduce a bug in an LTS kernel. Please backport
2d5f38a31980 since all it does is removes the offending call and turns
off code in fscounters.c.
--D
> This series has gone through the usual kdevops testing routine.
>
> Please ACK.
>
> Thanks,
> Amir.
>
> [1] https://www.spinics.net/lists/linux-xfs/msg61813.html
> [2] https://lore.kernel.org/all/ZK4E%2FgGuaBu+qvKL@dread.disaster.area/
>
> Darrick J. Wong (3):
> xfs: explicitly specify cpu when forcing inodegc delayed work to run
> immediately
> xfs: check that per-cpu inodegc workers actually run on that cpu
> xfs: fix xfs_inodegc_stop racing with mod_delayed_work
>
> fs/xfs/xfs_icache.c | 40 +++++++++++++++++++++++++++++++++-------
> fs/xfs/xfs_mount.h | 3 +++
> fs/xfs/xfs_super.c | 3 +++
> 3 files changed, 39 insertions(+), 7 deletions(-)
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 6.1 CANDIDATE 0/3] xfs inodegc fixes for 6.1.y (from v6.4)
2023-07-12 15:44 ` [PATCH 6.1 CANDIDATE 0/3] xfs inodegc fixes for 6.1.y (from v6.4) Darrick J. Wong
@ 2023-07-13 4:47 ` Amir Goldstein
0 siblings, 0 replies; 6+ messages in thread
From: Amir Goldstein @ 2023-07-13 4:47 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Chandan Babu R, Leah Rumancik, Dave Chinner, Chris Dunlop,
linux-xfs
On Wed, Jul 12, 2023 at 6:44 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Wed, Jul 12, 2023 at 12:47:30PM +0300, Amir Goldstein wrote:
> > Darrick,
> >
> > These are the patches we discussed that Leah requested for the 5.15.y
> > backport of non-blocking inodegc pushes series [1].
> >
> > They may or may not help the 5.15.y -> 6.1.y regression that was
> > reported by Chris [2].
> >
> > Note that I did not include:
> > 2d5f38a31980 ("xfs: disable reaping in fscounters scrub")
> > in this backport set, because I generally do not want to deal with
> > backporting fixes for experimental features.
>
> I don't agree with this decision because the comment for
> xfs_inodegc_stop now says that callers must hold s_umount.
> xchk_stop_reaping definitely does /not/ hold that lock, which means it's
> now buggy. Someone downstream could be using scrub, even if it's still
> experimental.
>
> I've generally said not to bother with scrub fixes, but I don't think
> it's correct to introduce a bug in an LTS kernel. Please backport
> 2d5f38a31980 since all it does is removes the offending call and turns
> off code in fscounters.c.
>
Makes sense.
Will do.
Thanks,
Amir.
^ permalink raw reply [flat|nested] 6+ messages in thread