* [PATCH REPOST REPOST v2] fscache: Use only one fscache_object_cong_wait.
@ 2021-12-23 16:35 Sebastian Andrzej Siewior
2021-12-23 17:17 ` David Howells
0 siblings, 1 reply; 5+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-12-23 16:35 UTC (permalink / raw)
To: linux-kernel, linux-cachefs, linux-fsdevel
Cc: David Howells, Tejun Heo, Gregor Beck, Thomas Gleixner,
Andrew Morton, Greg Kroah-Hartman, Sebastian Andrzej Siewior
In the commit mentioned below, fscache was converted from slow-work to
workqueue. slow_work_enqueue() and slow_work_sleep_till_thread_needed()
did not use a per-CPU workqueue. They choose from two global waitqueues
depending on the SLOW_WORK_VERY_SLOW bit which was not set so it always
one waitqueue.
I can't find out how it is ensured that a waiter on certain CPU is woken
up be the other side. My guess is that the timeout in schedule_timeout()
ensures that it does not wait forever (or a random wake up).
fscache_object_sleep_till_congested() must be invoked from preemptible
context in order for schedule() to work. In this case this_cpu_ptr()
should complain with CONFIG_DEBUG_PREEMPT enabled except the thread is
bound to one CPU.
wake_up() wakes only one waiter and I'm not sure if it is guaranteed
that only one waiter exists.
Replace the per-CPU waitqueue with one global waitqueue.
Fixes: 8b8edefa2fffb ("fscache: convert object to use workqueue instead of slow-work")
Reported-by: Gregor Beck <gregor.beck@gmail.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v2: https://lore.kernel.org/all/20211029083839.xwwt7jgzru3kcpii@linutronix.de/
Repost: https://lore.kernel.org/all/20211118165442.hekmz7xgisdzsyuh@linutronix.de/
Ping 1: https://lore.kernel.org/all/20211202205240.giqxuxqemlxxoobw@linutronix.de/
|I noticed that -next gained commit
| 608bfec640edb ("fscache: Remove the contents of the fscache driver, pending rewrite")
|
|which removes slow_work_sleep_till_thread_needed() and the per-CPU
|variable. Since it looks like a bug, what happens stable wise?
Ping 2: https://lore.kernel.org/all/YbdiYN+wU1RN9mWo@linutronix.de/
fs/fscache/internal.h | 1 -
fs/fscache/main.c | 6 ------
fs/fscache/object.c | 13 +++++--------
3 files changed, 5 insertions(+), 15 deletions(-)
diff --git a/fs/fscache/internal.h b/fs/fscache/internal.h
index c3e4804b8fcbf..9edb87e11680b 100644
--- a/fs/fscache/internal.h
+++ b/fs/fscache/internal.h
@@ -81,7 +81,6 @@ extern unsigned fscache_debug;
extern struct kobject *fscache_root;
extern struct workqueue_struct *fscache_object_wq;
extern struct workqueue_struct *fscache_op_wq;
-DECLARE_PER_CPU(wait_queue_head_t, fscache_object_cong_wait);
extern unsigned int fscache_hash(unsigned int salt, unsigned int *data, unsigned int n);
diff --git a/fs/fscache/main.c b/fs/fscache/main.c
index 4207f98e405fd..85f8cf3a323d5 100644
--- a/fs/fscache/main.c
+++ b/fs/fscache/main.c
@@ -41,8 +41,6 @@ struct kobject *fscache_root;
struct workqueue_struct *fscache_object_wq;
struct workqueue_struct *fscache_op_wq;
-DEFINE_PER_CPU(wait_queue_head_t, fscache_object_cong_wait);
-
/* these values serve as lower bounds, will be adjusted in fscache_init() */
static unsigned fscache_object_max_active = 4;
static unsigned fscache_op_max_active = 2;
@@ -138,7 +136,6 @@ unsigned int fscache_hash(unsigned int salt, unsigned int *data, unsigned int n)
static int __init fscache_init(void)
{
unsigned int nr_cpus = num_possible_cpus();
- unsigned int cpu;
int ret;
fscache_object_max_active =
@@ -161,9 +158,6 @@ static int __init fscache_init(void)
if (!fscache_op_wq)
goto error_op_wq;
- for_each_possible_cpu(cpu)
- init_waitqueue_head(&per_cpu(fscache_object_cong_wait, cpu));
-
ret = fscache_proc_init();
if (ret < 0)
goto error_proc;
diff --git a/fs/fscache/object.c b/fs/fscache/object.c
index 6a675652129b2..7a972d144b546 100644
--- a/fs/fscache/object.c
+++ b/fs/fscache/object.c
@@ -798,6 +798,8 @@ void fscache_object_destroy(struct fscache_object *object)
}
EXPORT_SYMBOL(fscache_object_destroy);
+static DECLARE_WAIT_QUEUE_HEAD(fscache_object_cong_wait);
+
/*
* enqueue an object for metadata-type processing
*/
@@ -806,16 +808,12 @@ void fscache_enqueue_object(struct fscache_object *object)
_enter("{OBJ%x}", object->debug_id);
if (fscache_get_object(object, fscache_obj_get_queue) >= 0) {
- wait_queue_head_t *cong_wq =
- &get_cpu_var(fscache_object_cong_wait);
if (queue_work(fscache_object_wq, &object->work)) {
if (fscache_object_congested())
- wake_up(cong_wq);
+ wake_up(&fscache_object_cong_wait);
} else
fscache_put_object(object, fscache_obj_put_queue);
-
- put_cpu_var(fscache_object_cong_wait);
}
}
@@ -833,16 +831,15 @@ void fscache_enqueue_object(struct fscache_object *object)
*/
bool fscache_object_sleep_till_congested(signed long *timeoutp)
{
- wait_queue_head_t *cong_wq = this_cpu_ptr(&fscache_object_cong_wait);
DEFINE_WAIT(wait);
if (fscache_object_congested())
return true;
- add_wait_queue_exclusive(cong_wq, &wait);
+ add_wait_queue_exclusive(&fscache_object_cong_wait, &wait);
if (!fscache_object_congested())
*timeoutp = schedule_timeout(*timeoutp);
- finish_wait(cong_wq, &wait);
+ finish_wait(&fscache_object_cong_wait, &wait);
return fscache_object_congested();
}
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH REPOST REPOST v2] fscache: Use only one fscache_object_cong_wait.
2021-12-23 16:35 [PATCH REPOST REPOST v2] fscache: Use only one fscache_object_cong_wait Sebastian Andrzej Siewior
@ 2021-12-23 17:17 ` David Howells
2021-12-23 18:15 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 5+ messages in thread
From: David Howells @ 2021-12-23 17:17 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: dhowells, linux-kernel, linux-cachefs, linux-fsdevel, Tejun Heo,
Gregor Beck, Thomas Gleixner, Andrew Morton, Greg Kroah-Hartman
Thanks, but this is gone in the upcoming fscache rewrite. I'm hoping that
will get in the next merge window.
David
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH REPOST REPOST v2] fscache: Use only one fscache_object_cong_wait.
2021-12-23 17:17 ` David Howells
@ 2021-12-23 18:15 ` Sebastian Andrzej Siewior
2021-12-27 0:20 ` Andrew Morton
0 siblings, 1 reply; 5+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-12-23 18:15 UTC (permalink / raw)
To: David Howells
Cc: linux-kernel, linux-cachefs, linux-fsdevel, Tejun Heo,
Gregor Beck, Thomas Gleixner, Andrew Morton, Greg Kroah-Hartman
On 2021-12-23 17:17:09 [+0000], David Howells wrote:
> Thanks, but this is gone in the upcoming fscache rewrite. I'm hoping that
> will get in the next merge window.
Yes, I noticed that. What about current tree, v5.16-rc6 and less?
Shouldn't this be addressed?
> David
Sebastian
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH REPOST REPOST v2] fscache: Use only one fscache_object_cong_wait.
2021-12-23 18:15 ` Sebastian Andrzej Siewior
@ 2021-12-27 0:20 ` Andrew Morton
2022-01-03 13:54 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2021-12-27 0:20 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: David Howells, linux-kernel, linux-cachefs, linux-fsdevel,
Tejun Heo, Gregor Beck, Thomas Gleixner, Greg Kroah-Hartman
On Thu, 23 Dec 2021 19:15:09 +0100 Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> On 2021-12-23 17:17:09 [+0000], David Howells wrote:
> > Thanks, but this is gone in the upcoming fscache rewrite. I'm hoping that
> > will get in the next merge window.
>
> Yes, I noticed that. What about current tree, v5.16-rc6 and less?
> Shouldn't this be addressed?
If the bug is serious enough to justify a -stable backport then yes, we
should merge a fix such as this ahead of the fscache rewrite, so we
have something suitable for backporting.
Is the bug serious enough?
Or is the bug in a not-yet-noticed state? In other words, is it
possible that four years from now, someone will hit this bug in a
5.15-based kernel and will then wish we'd backported a fix?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH REPOST REPOST v2] fscache: Use only one fscache_object_cong_wait.
2021-12-27 0:20 ` Andrew Morton
@ 2022-01-03 13:54 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 5+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-01-03 13:54 UTC (permalink / raw)
To: Andrew Morton
Cc: David Howells, linux-kernel, linux-cachefs, linux-fsdevel,
Tejun Heo, Gregor Beck, Thomas Gleixner, Greg Kroah-Hartman
On 2021-12-26 16:20:30 [-0800], Andrew Morton wrote:
> On Thu, 23 Dec 2021 19:15:09 +0100 Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
>
> > On 2021-12-23 17:17:09 [+0000], David Howells wrote:
> > > Thanks, but this is gone in the upcoming fscache rewrite. I'm hoping that
> > > will get in the next merge window.
> >
> > Yes, I noticed that. What about current tree, v5.16-rc6 and less?
> > Shouldn't this be addressed?
>
> If the bug is serious enough to justify a -stable backport then yes, we
> should merge a fix such as this ahead of the fscache rewrite, so we
> have something suitable for backporting.
>
> Is the bug serious enough?
>
> Or is the bug in a not-yet-noticed state? In other words, is it
> possible that four years from now, someone will hit this bug in a
> 5.15-based kernel and will then wish we'd backported a fix?
I can't answer how serious it is but:
- with CONFIG_DEBUG_PREEMPT enabled there has to be a visible backtrace
due this_cpu_ptr() usage.
- because of schedule_timeout(60 * HZ) there is no visible hang. It
should be either woken up properly (via the waitqueue) or after a
minute due to the timeout.
both things don't look good in general.
Sebastian
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-01-03 13:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-23 16:35 [PATCH REPOST REPOST v2] fscache: Use only one fscache_object_cong_wait Sebastian Andrzej Siewior
2021-12-23 17:17 ` David Howells
2021-12-23 18:15 ` Sebastian Andrzej Siewior
2021-12-27 0:20 ` Andrew Morton
2022-01-03 13:54 ` Sebastian Andrzej Siewior
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.