On Sun, Jun 11, 2023 at 08:14:25PM -0700, Linus Torvalds wrote: > On Sun, Jun 11, 2023 at 7:22 PM Dave Chinner <david@xxxxxxxxxxxxx> wrote: > > > > I guess the regression fix needs a regression fix.... > > Yup. > > From the description of the problem, it sounds like this happens on > real hardware, no vhost anywhere? > > Or maybe Darrick (who doesn't see the issue) is running on raw > hardware, and you and Zorro are running in a virtual environment? Ahah, it turns out that liburing-dev isn't installed on the test fleet, so fstests didn't get built with io_uring support. That probably explains why I don't see any of these hangs. Oh. I can't *install* the debian liburing-dev package because it has a versioned dependency on linux-libc-dev >= 5.1, which isn't compatible with me having a linux-libc-dev-djwong package that contains the uapi headers for the latest upstream kernel and Replaces: linux-libc-dev. So either I have to create a dummy linux-libc-dev with adequate version number that pulls in my own libc header package, or rename that package. <sigh> It's going to take me a while to research how best to split this stupid knot. --D > It sounds like zap_other_threads() and coredump_task_exit() do not > agree about the core_state->nr_threads counting, which is part of what > changed there. > > [ Goes off to look ] > > Hmm. Both seem to be using the same test for > > (t->flags & (PF_IO_WORKER | PF_USER_WORKER)) != PF_USER_WORKER > > which I don't love - I don't think io_uring threads should participate > in core dumping either, so I think the test could just be > > (t->flags & PF_IO_WORKER) > > but that shouldn't be the issue here. > > But according to > > https://lore.kernel.org/all/20230611124836.whfktwaumnefm5z5@zlang-mailbox/ > > it's clearly hanging in wait_for_completion_state() in > coredump_wait(), so it really looks like some confusion about that > core_waiters (aka core_state->nr_threads) count. > > Oh. Humm. Mike changed that initial rough patch of mine, and I had > moved the "if you don't participate in c ore dumps" test up also past > the "do_coredump()" logic. > > And I think it's horribly *wrong* for a thread that doesn't get > counted for core-dumping to go into do_coredump(), because then it > will set the "core_state" to possibly be the core-state of the vhost > thread that isn't even counted. > > So *maybe* this attached patch might fix it? I haven't thought very > deeply about this, but vhost workers most definitely shouldn't call > do_coredump(), since they are then not counted. > > (And again, I think we should just check that PF_IO_WORKER bit, not > use this more complex test, but that's a separate and bigger change). > > Linus > kernel/signal.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/kernel/signal.c b/kernel/signal.c > index 2547fa73bde5..a1e11ee8537c 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -2847,6 +2847,10 @@ bool get_signal(struct ksignal *ksig) > */ > current->flags |= PF_SIGNALED; > > + /* vhost workers don't participate in core dups */ > + if ((current->flags & (PF_IO_WORKER | PF_USER_WORKER)) != PF_USER_WORKER) > + goto out; > + > if (sig_kernel_coredump(signr)) { > if (print_fatal_signals) > print_fatal_signal(ksig->info.si_signo);
