Re: [PATCH] fs: process fput task_work with TWA_SIGNAL
On Fri, Jan 08, 2021 at 09:26:40AM -0700, Jens Axboe wrote: > >> Can you show the callers that DO NOT need it? > > > > OK, so here's my suggestion: > > > > 1) For 5.11, we just re-instate the task_work run in get_signal(). This > >will make TWA_RESUME have the exact same behavior as before. > > > > 2) For 5.12, I'll prepare a patch that collapses TWA_RESUME and TWA_SIGNAL, > >turning it into a bool again (notify or no notify). > > > > How does that sound? > > Attached the patches - #1 is proposed for 5.11 to fix the current issue, > and then 2-4 can get queued for 5.12 to totally remove the difference > between TWA_RESUME and TWA_SIGNAL. > > Totally untested, but pretty straight forward. Umm... I'm looking at the callers of get_signal() and I really wonder how your support for TIF_NOTIFY_SIGNAL interacts with saved sigmask handling by various do_signal() (calls of restore_saved_sigmask()). Could you give pointers to relevant discussion or a braindump on the same? I realize that it had been months ago, but... Do we even need restore_saved_sigmask_unless() now? Could set_user_sigmask() simply set TIF_NOTIFY_SIGNAL? Oleg, could you comment on that? Another fun question is how does that thing interact with single-stepping logics; it's been about 8 years since I looked into those horrors, but they used to be bloody awful... What I'm trying to figure out is how costly TIF_NOTIFY_SIGNAL is on the work execution side; task_work_add() side is cheap enough, it's delivery that is interesting.
Re: [PATCH] fs: process fput task_work with TWA_SIGNAL
On 1/8/21 9:10 AM, Jens Axboe wrote:
> On 1/8/21 8:58 AM, Al Viro wrote:
>> On Fri, Jan 08, 2021 at 08:13:25AM -0700, Jens Axboe wrote:
Anyway, bedtime for me; right now it looks like at least for task ==
current we always want TWA_SIGNAL. I'll look into that more tomorrow
when I get up, but so far it smells like switching everything to
TWA_SIGNAL would be the right thing to do, if not going back to bool
notify for task_work_add()...
>>>
>>> Before the change, the fact that we ran task_work off get_signal() and
>>> thus processed even non-notify work in that path was a bit of a mess,
>>> imho. If you have work that needs processing now, in the same manner as
>>> signals, then you really should be using TWA_SIGNAL. For this pipe case,
>>> and I'd need to setup and reproduce it again, the task must have a
>>> signal pending and that would have previously caused the task_work to
>>> run, and now it does not. TWA_RESUME technically didn't change its
>>> behavior, it's still the same notification type, we just don't run
>>> task_work unconditionally (regardless of notification type) from
>>> get_signal().
>>
>> It sure as hell did change behaviour. Think of the effect of getting
>> hit with SIGSTOP. That's what that "bit of a mess" had been about.
>> Work done now vs. possibly several days later when SIGCONT finally
>> gets sent.
>>
>>> I think the main question here is if we want to re-instate the behavior
>>> of running task_work off get_signal(). I'm leaning towards not doing
>>> that and ensuring that callers that DO need that are using TWA_SIGNAL.
>>
>> Can you show the callers that DO NOT need it?
>
> OK, so here's my suggestion:
>
> 1) For 5.11, we just re-instate the task_work run in get_signal(). This
>will make TWA_RESUME have the exact same behavior as before.
>
> 2) For 5.12, I'll prepare a patch that collapses TWA_RESUME and TWA_SIGNAL,
>turning it into a bool again (notify or no notify).
>
> How does that sound?
Attached the patches - #1 is proposed for 5.11 to fix the current issue,
and then 2-4 can get queued for 5.12 to totally remove the difference
between TWA_RESUME and TWA_SIGNAL.
Totally untested, but pretty straight forward.
--
Jens Axboe
>From 069af1e44d70b6d3dd746e41a5f9d65505fb5490 Mon Sep 17 00:00:00 2001
From: Jens Axboe
Date: Fri, 8 Jan 2021 09:22:04 -0700
Subject: [PATCH 3/4] task_work: use true/false for task_work_add notification
type
There's no difference between TWA_SIGNAL and TWA_RESUME anymore, change
all callers to simply specify whether they need notification or not.
Signed-off-by: Jens Axboe
---
arch/x86/kernel/cpu/mce/core.c | 2 +-
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 2 +-
drivers/acpi/apei/ghes.c | 2 +-
drivers/android/binder.c | 2 +-
fs/file_table.c| 2 +-
fs/io_uring.c | 19 +--
fs/namespace.c | 2 +-
kernel/events/uprobes.c| 2 +-
kernel/irq/manage.c| 2 +-
kernel/sched/fair.c| 2 +-
kernel/time/posix-cpu-timers.c | 2 +-
security/keys/keyctl.c | 2 +-
security/yama/yama_lsm.c | 2 +-
13 files changed, 21 insertions(+), 22 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 13d3f1cbda17..9f315b4c022d 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1278,7 +1278,7 @@ static void queue_task_work(struct mce *m, int kill_current_task)
else
current->mce_kill_me.func = kill_me_maybe;
- task_work_add(current, >mce_kill_me, TWA_RESUME);
+ task_work_add(current, >mce_kill_me, true);
}
/*
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 29ffb95b25ff..109dd4fe72da 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -579,7 +579,7 @@ static int __rdtgroup_move_task(struct task_struct *tsk,
* callback has been invoked.
*/
atomic_inc(>waitcount);
- ret = task_work_add(tsk, >work, TWA_RESUME);
+ ret = task_work_add(tsk, >work, true);
if (ret) {
/*
* Task is exiting. Drop the refcount and free the callback.
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index fce7ade2aba9..99df00f64306 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -942,7 +942,7 @@ static void ghes_proc_in_irq(struct irq_work *irq_work)
estatus_node->task_work.func = ghes_kick_task_work;
estatus_node->task_work_cpu = smp_processor_id();
ret = task_work_add(current, _node->task_work,
- TWA_RESUME);
+ true);
if (ret)
estatus_node->task_work.func = NULL;
}
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index c119736ca56a..5b1b2ed7c020 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1839,7 +1839,7 @@ static
Re: [PATCH] fs: process fput task_work with TWA_SIGNAL
On 1/8/21 8:58 AM, Al Viro wrote: > On Fri, Jan 08, 2021 at 08:13:25AM -0700, Jens Axboe wrote: >>> Anyway, bedtime for me; right now it looks like at least for task == >>> current we always want TWA_SIGNAL. I'll look into that more tomorrow >>> when I get up, but so far it smells like switching everything to >>> TWA_SIGNAL would be the right thing to do, if not going back to bool >>> notify for task_work_add()... >> >> Before the change, the fact that we ran task_work off get_signal() and >> thus processed even non-notify work in that path was a bit of a mess, >> imho. If you have work that needs processing now, in the same manner as >> signals, then you really should be using TWA_SIGNAL. For this pipe case, >> and I'd need to setup and reproduce it again, the task must have a >> signal pending and that would have previously caused the task_work to >> run, and now it does not. TWA_RESUME technically didn't change its >> behavior, it's still the same notification type, we just don't run >> task_work unconditionally (regardless of notification type) from >> get_signal(). > > It sure as hell did change behaviour. Think of the effect of getting > hit with SIGSTOP. That's what that "bit of a mess" had been about. > Work done now vs. possibly several days later when SIGCONT finally > gets sent. > >> I think the main question here is if we want to re-instate the behavior >> of running task_work off get_signal(). I'm leaning towards not doing >> that and ensuring that callers that DO need that are using TWA_SIGNAL. > > Can you show the callers that DO NOT need it? OK, so here's my suggestion: 1) For 5.11, we just re-instate the task_work run in get_signal(). This will make TWA_RESUME have the exact same behavior as before. 2) For 5.12, I'll prepare a patch that collapses TWA_RESUME and TWA_SIGNAL, turning it into a bool again (notify or no notify). How does that sound? -- Jens Axboe
Re: [PATCH] fs: process fput task_work with TWA_SIGNAL
On Fri, Jan 08, 2021 at 08:13:25AM -0700, Jens Axboe wrote: > > Anyway, bedtime for me; right now it looks like at least for task == > > current we always want TWA_SIGNAL. I'll look into that more tomorrow > > when I get up, but so far it smells like switching everything to > > TWA_SIGNAL would be the right thing to do, if not going back to bool > > notify for task_work_add()... > > Before the change, the fact that we ran task_work off get_signal() and > thus processed even non-notify work in that path was a bit of a mess, > imho. If you have work that needs processing now, in the same manner as > signals, then you really should be using TWA_SIGNAL. For this pipe case, > and I'd need to setup and reproduce it again, the task must have a > signal pending and that would have previously caused the task_work to > run, and now it does not. TWA_RESUME technically didn't change its > behavior, it's still the same notification type, we just don't run > task_work unconditionally (regardless of notification type) from > get_signal(). It sure as hell did change behaviour. Think of the effect of getting hit with SIGSTOP. That's what that "bit of a mess" had been about. Work done now vs. possibly several days later when SIGCONT finally gets sent. > I think the main question here is if we want to re-instate the behavior > of running task_work off get_signal(). I'm leaning towards not doing > that and ensuring that callers that DO need that are using TWA_SIGNAL. Can you show the callers that DO NOT need it?
Re: [PATCH] fs: process fput task_work with TWA_SIGNAL
On 1/7/21 11:46 PM, Al Viro wrote:
> On Fri, Jan 08, 2021 at 05:26:51AM +, Al Viro wrote:
>> On Tue, Jan 05, 2021 at 11:29:11AM -0700, Jens Axboe wrote:
>>> Song reported a boot regression in a kvm image with 5.11-rc, and bisected
>>> it down to the below patch. Debugging this issue, turns out that the boot
>>> stalled when a task is waiting on a pipe being released. As we no longer
>>> run task_work from get_signal() unless it's queued with TWA_SIGNAL, the
>>> task goes idle without running the task_work. This prevents ->release()
>>> from being called on the pipe, which another boot task is waiting on.
>>>
>>> Use TWA_SIGNAL for the file fput work to ensure it's run before the task
>>> goes idle.
>>>
>>> Fixes: 98b89b649fce ("signal: kill JOBCTL_TASK_WORK")
>>> Reported-by: Song Liu
>>> Signed-off-by: Jens Axboe
>>>
>>> ---
>>>
>>> The other alternative here is obviously to re-instate the:
>>>
>>> if (unlikely(current->task_works))
>>> task_work_run();
>>>
>>> in get_signal() that we had before this change. Might be safer in case
>>> there are other cases that need to ensure the work is run in a timely
>>> fashion, though I do think it's cleaner to long term to correctly mark
>>> task_work with the needed notification type. Comments welcome...
>>
>> Interesting... I think I've missed the discussion of that thing; could
>> you forward the relevant thread my way or give an archive link to it?
The initial report from Song was off list, and I just worked from that
to get to understanding the issue. Most of it is in the commit message,
but the debugging basically involved figuring out what the stuck task
was doing (it was in idle), and that it still had pending task_work. The
task_work was 5 entries of fput, with 4 being ext4 files, and 1
being a pipe. So that lead to the theory of the pipe not being released,
and hence why we were stuck.
> Actually, why do we need TWA_RESUME at all? OK, a while ago you've added
> a way for task_work_add() to do wake_up_signal(). Fine, so if the sucker
> had been asleep in get_signal(), it gets woken up and the work gets run
> fast. Irrelevant for those who did task_work_add() for themselves.
> With that commit, though, you've suddenly changed the default behaviour -
> now if you do that task_work_add() for current *and* get asleep in
> get_signal(), task_work_add() gets delayed - potentially for a very
> long time.
Right, this is why I brought up that we can re-instate the get_signal()
running task_work unconditionally as another way of fixing it, because
that change was inadvertently done as part of the commit that killed off
JOBCTL_TASK_WORK.
> Now the default (TWA_RESUME) has changed semantics; matter of fact,
> TWA_SIGNAL seems to be a lot closer than what we used to have. I'm
> too sleepy right now to check if there are valid usecases for your new
> TWA_RESUME behaviour, but I very much doubt that old callers (before
> the TWA_RESUME/TWA_SIGNAL split) want that.
>
> In particular, for mntput_no_expire() we definitely do *not* want
> that, same as with fput(). Same, AFAICS, for YAMA report_access().
> And for binder_deferred_fd_close(). And task_tick_numa() looks that
> way as well...
>
> Anyway, bedtime for me; right now it looks like at least for task ==
> current we always want TWA_SIGNAL. I'll look into that more tomorrow
> when I get up, but so far it smells like switching everything to
> TWA_SIGNAL would be the right thing to do, if not going back to bool
> notify for task_work_add()...
Before the change, the fact that we ran task_work off get_signal() and
thus processed even non-notify work in that path was a bit of a mess,
imho. If you have work that needs processing now, in the same manner as
signals, then you really should be using TWA_SIGNAL. For this pipe case,
and I'd need to setup and reproduce it again, the task must have a
signal pending and that would have previously caused the task_work to
run, and now it does not. TWA_RESUME technically didn't change its
behavior, it's still the same notification type, we just don't run
task_work unconditionally (regardless of notification type) from
get_signal().
I think the main question here is if we want to re-instate the behavior
of running task_work off get_signal(). I'm leaning towards not doing
that and ensuring that callers that DO need that are using TWA_SIGNAL.
--
Jens Axboe
Re: [PATCH] fs: process fput task_work with TWA_SIGNAL
On Fri, Jan 08, 2021 at 07:21:52AM +0100, Sedat Dilek wrote:
> On Fri, Jan 8, 2021 at 6:30 AM Al Viro wrote:
> >
> > On Tue, Jan 05, 2021 at 11:29:11AM -0700, Jens Axboe wrote:
> > > Song reported a boot regression in a kvm image with 5.11-rc, and bisected
> > > it down to the below patch. Debugging this issue, turns out that the boot
> > > stalled when a task is waiting on a pipe being released. As we no longer
> > > run task_work from get_signal() unless it's queued with TWA_SIGNAL, the
> > > task goes idle without running the task_work. This prevents ->release()
> > > from being called on the pipe, which another boot task is waiting on.
> > >
> > > Use TWA_SIGNAL for the file fput work to ensure it's run before the task
> > > goes idle.
> > >
> > > Fixes: 98b89b649fce ("signal: kill JOBCTL_TASK_WORK")
> > > Reported-by: Song Liu
> > > Signed-off-by: Jens Axboe
> > >
> > > ---
> > >
> > > The other alternative here is obviously to re-instate the:
> > >
> > > if (unlikely(current->task_works))
> > > task_work_run();
> > >
> > > in get_signal() that we had before this change. Might be safer in case
> > > there are other cases that need to ensure the work is run in a timely
> > > fashion, though I do think it's cleaner to long term to correctly mark
> > > task_work with the needed notification type. Comments welcome...
> >
> > Interesting... I think I've missed the discussion of that thing; could
> > you forward the relevant thread my way or give an archive link to it?
>
> See [1].
>
> - Sedat -
>
> [1] https://marc.info/?t=16098715661=1=2
Wait, that's this very thread, starting with the posting I'd been replying
to. Really confused now... Was that a private bug report and an equally
private discussion? That's what I wanted to see...
Anyway, I'm more than half-asleep right now; will get back to that in the
morning.
Re: [PATCH] fs: process fput task_work with TWA_SIGNAL
On Fri, Jan 08, 2021 at 07:21:52AM +0100, Sedat Dilek wrote:
> On Fri, Jan 8, 2021 at 6:30 AM Al Viro wrote:
> >
> > On Tue, Jan 05, 2021 at 11:29:11AM -0700, Jens Axboe wrote:
> > > Song reported a boot regression in a kvm image with 5.11-rc, and bisected
> > > it down to the below patch. Debugging this issue, turns out that the boot
> > > stalled when a task is waiting on a pipe being released. As we no longer
> > > run task_work from get_signal() unless it's queued with TWA_SIGNAL, the
> > > task goes idle without running the task_work. This prevents ->release()
> > > from being called on the pipe, which another boot task is waiting on.
> > >
> > > Use TWA_SIGNAL for the file fput work to ensure it's run before the task
> > > goes idle.
> > >
> > > Fixes: 98b89b649fce ("signal: kill JOBCTL_TASK_WORK")
> > > Reported-by: Song Liu
> > > Signed-off-by: Jens Axboe
> > >
> > > ---
> > >
> > > The other alternative here is obviously to re-instate the:
> > >
> > > if (unlikely(current->task_works))
> > > task_work_run();
> > >
> > > in get_signal() that we had before this change. Might be safer in case
> > > there are other cases that need to ensure the work is run in a timely
> > > fashion, though I do think it's cleaner to long term to correctly mark
> > > task_work with the needed notification type. Comments welcome...
> >
> > Interesting... I think I've missed the discussion of that thing; could
> > you forward the relevant thread my way or give an archive link to it?
>
> See [1].
>
> - Sedat -
>
> [1] https://marc.info/?t=16098715661=1=2
Thanks; will check tomorrow.
Re: [PATCH] fs: process fput task_work with TWA_SIGNAL
On Fri, Jan 08, 2021 at 05:26:51AM +, Al Viro wrote:
> On Tue, Jan 05, 2021 at 11:29:11AM -0700, Jens Axboe wrote:
> > Song reported a boot regression in a kvm image with 5.11-rc, and bisected
> > it down to the below patch. Debugging this issue, turns out that the boot
> > stalled when a task is waiting on a pipe being released. As we no longer
> > run task_work from get_signal() unless it's queued with TWA_SIGNAL, the
> > task goes idle without running the task_work. This prevents ->release()
> > from being called on the pipe, which another boot task is waiting on.
> >
> > Use TWA_SIGNAL for the file fput work to ensure it's run before the task
> > goes idle.
> >
> > Fixes: 98b89b649fce ("signal: kill JOBCTL_TASK_WORK")
> > Reported-by: Song Liu
> > Signed-off-by: Jens Axboe
> >
> > ---
> >
> > The other alternative here is obviously to re-instate the:
> >
> > if (unlikely(current->task_works))
> > task_work_run();
> >
> > in get_signal() that we had before this change. Might be safer in case
> > there are other cases that need to ensure the work is run in a timely
> > fashion, though I do think it's cleaner to long term to correctly mark
> > task_work with the needed notification type. Comments welcome...
>
> Interesting... I think I've missed the discussion of that thing; could
> you forward the relevant thread my way or give an archive link to it?
Actually, why do we need TWA_RESUME at all? OK, a while ago you've added
a way for task_work_add() to do wake_up_signal(). Fine, so if the sucker
had been asleep in get_signal(), it gets woken up and the work gets run
fast. Irrelevant for those who did task_work_add() for themselves.
With that commit, though, you've suddenly changed the default behaviour -
now if you do that task_work_add() for current *and* get asleep in
get_signal(), task_work_add() gets delayed - potentially for a very
long time.
Now the default (TWA_RESUME) has changed semantics; matter of fact,
TWA_SIGNAL seems to be a lot closer than what we used to have. I'm
too sleepy right now to check if there are valid usecases for your
new TWA_RESUME behaviour, but I very much doubt that old callers
(before the TWA_RESUME/TWA_SIGNAL split) want that.
In particular, for mntput_no_expire() we definitely do *not* want
that, same as with fput(). Same, AFAICS, for YAMA report_access().
And for binder_deferred_fd_close(). And task_tick_numa() looks
that way as well...
Anyway, bedtime for me; right now it looks like at least for task == current
we always want TWA_SIGNAL. I'll look into that more tomorrow when I get
up, but so far it smells like switching everything to TWA_SIGNAL would
be the right thing to do, if not going back to bool notify for
task_work_add()...
Re: [PATCH] fs: process fput task_work with TWA_SIGNAL
On Fri, Jan 8, 2021 at 6:30 AM Al Viro wrote:
>
> On Tue, Jan 05, 2021 at 11:29:11AM -0700, Jens Axboe wrote:
> > Song reported a boot regression in a kvm image with 5.11-rc, and bisected
> > it down to the below patch. Debugging this issue, turns out that the boot
> > stalled when a task is waiting on a pipe being released. As we no longer
> > run task_work from get_signal() unless it's queued with TWA_SIGNAL, the
> > task goes idle without running the task_work. This prevents ->release()
> > from being called on the pipe, which another boot task is waiting on.
> >
> > Use TWA_SIGNAL for the file fput work to ensure it's run before the task
> > goes idle.
> >
> > Fixes: 98b89b649fce ("signal: kill JOBCTL_TASK_WORK")
> > Reported-by: Song Liu
> > Signed-off-by: Jens Axboe
> >
> > ---
> >
> > The other alternative here is obviously to re-instate the:
> >
> > if (unlikely(current->task_works))
> > task_work_run();
> >
> > in get_signal() that we had before this change. Might be safer in case
> > there are other cases that need to ensure the work is run in a timely
> > fashion, though I do think it's cleaner to long term to correctly mark
> > task_work with the needed notification type. Comments welcome...
>
> Interesting... I think I've missed the discussion of that thing; could
> you forward the relevant thread my way or give an archive link to it?
See [1].
- Sedat -
[1] https://marc.info/?t=16098715661=1=2
Re: [PATCH] fs: process fput task_work with TWA_SIGNAL
On Fri, Jan 8, 2021 at 4:56 AM Jens Axboe wrote:
>
> On 1/7/21 3:17 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Jan 5, 2021 at 10:30 AM Jens Axboe wrote:
> >>
> >> Song reported a boot regression in a kvm image with 5.11-rc, and bisected
> >> it down to the below patch. Debugging this issue, turns out that the boot
> >> stalled when a task is waiting on a pipe being released. As we no longer
> >> run task_work from get_signal() unless it's queued with TWA_SIGNAL, the
> >> task goes idle without running the task_work. This prevents ->release()
> >> from being called on the pipe, which another boot task is waiting on.
> >>
> >> Use TWA_SIGNAL for the file fput work to ensure it's run before the task
> >> goes idle.
> >>
> >> Fixes: 98b89b649fce ("signal: kill JOBCTL_TASK_WORK")
> >> Reported-by: Song Liu
> >> Signed-off-by: Jens Axboe
> >
> > I just spend a bit of time bisecting and landed on commit 98b89b649fce
> > ("signal: kill JOBCTL_TASK_WORK") causing my failure to bootup
> > mainline. Your patch fixes my problem. I haven't done any analysis
> > of the code--just testing, thus:
> >
> > Tested-by: Douglas Anderson
>
> Thanks, adding your Tested-by.
>
I have this in my patch-series since it appeared in [1].
Feel free to add my:
Tested-by: Sedat Dilek # LLVM/Clang version 11.0.1
- Sedat -
[1] https://git.kernel.dk/cgit/linux-block/log/?h=task_work
- Sedat -
Re: [PATCH] fs: process fput task_work with TWA_SIGNAL
On Tue, Jan 05, 2021 at 11:29:11AM -0700, Jens Axboe wrote:
> Song reported a boot regression in a kvm image with 5.11-rc, and bisected
> it down to the below patch. Debugging this issue, turns out that the boot
> stalled when a task is waiting on a pipe being released. As we no longer
> run task_work from get_signal() unless it's queued with TWA_SIGNAL, the
> task goes idle without running the task_work. This prevents ->release()
> from being called on the pipe, which another boot task is waiting on.
>
> Use TWA_SIGNAL for the file fput work to ensure it's run before the task
> goes idle.
>
> Fixes: 98b89b649fce ("signal: kill JOBCTL_TASK_WORK")
> Reported-by: Song Liu
> Signed-off-by: Jens Axboe
>
> ---
>
> The other alternative here is obviously to re-instate the:
>
> if (unlikely(current->task_works))
> task_work_run();
>
> in get_signal() that we had before this change. Might be safer in case
> there are other cases that need to ensure the work is run in a timely
> fashion, though I do think it's cleaner to long term to correctly mark
> task_work with the needed notification type. Comments welcome...
Interesting... I think I've missed the discussion of that thing; could
you forward the relevant thread my way or give an archive link to it?
Re: [PATCH] fs: process fput task_work with TWA_SIGNAL
On 1/7/21 3:17 PM, Doug Anderson wrote:
> Hi,
>
> On Tue, Jan 5, 2021 at 10:30 AM Jens Axboe wrote:
>>
>> Song reported a boot regression in a kvm image with 5.11-rc, and bisected
>> it down to the below patch. Debugging this issue, turns out that the boot
>> stalled when a task is waiting on a pipe being released. As we no longer
>> run task_work from get_signal() unless it's queued with TWA_SIGNAL, the
>> task goes idle without running the task_work. This prevents ->release()
>> from being called on the pipe, which another boot task is waiting on.
>>
>> Use TWA_SIGNAL for the file fput work to ensure it's run before the task
>> goes idle.
>>
>> Fixes: 98b89b649fce ("signal: kill JOBCTL_TASK_WORK")
>> Reported-by: Song Liu
>> Signed-off-by: Jens Axboe
>
> I just spend a bit of time bisecting and landed on commit 98b89b649fce
> ("signal: kill JOBCTL_TASK_WORK") causing my failure to bootup
> mainline. Your patch fixes my problem. I haven't done any analysis
> of the code--just testing, thus:
>
> Tested-by: Douglas Anderson
Thanks, adding your Tested-by.
--
Jens Axboe
Re: [PATCH] fs: process fput task_work with TWA_SIGNAL
Hi,
On Tue, Jan 5, 2021 at 10:30 AM Jens Axboe wrote:
>
> Song reported a boot regression in a kvm image with 5.11-rc, and bisected
> it down to the below patch. Debugging this issue, turns out that the boot
> stalled when a task is waiting on a pipe being released. As we no longer
> run task_work from get_signal() unless it's queued with TWA_SIGNAL, the
> task goes idle without running the task_work. This prevents ->release()
> from being called on the pipe, which another boot task is waiting on.
>
> Use TWA_SIGNAL for the file fput work to ensure it's run before the task
> goes idle.
>
> Fixes: 98b89b649fce ("signal: kill JOBCTL_TASK_WORK")
> Reported-by: Song Liu
> Signed-off-by: Jens Axboe
I just spend a bit of time bisecting and landed on commit 98b89b649fce
("signal: kill JOBCTL_TASK_WORK") causing my failure to bootup
mainline. Your patch fixes my problem. I haven't done any analysis
of the code--just testing, thus:
Tested-by: Douglas Anderson
[PATCH] fs: process fput task_work with TWA_SIGNAL
Song reported a boot regression in a kvm image with 5.11-rc, and bisected
it down to the below patch. Debugging this issue, turns out that the boot
stalled when a task is waiting on a pipe being released. As we no longer
run task_work from get_signal() unless it's queued with TWA_SIGNAL, the
task goes idle without running the task_work. This prevents ->release()
from being called on the pipe, which another boot task is waiting on.
Use TWA_SIGNAL for the file fput work to ensure it's run before the task
goes idle.
Fixes: 98b89b649fce ("signal: kill JOBCTL_TASK_WORK")
Reported-by: Song Liu
Signed-off-by: Jens Axboe
---
The other alternative here is obviously to re-instate the:
if (unlikely(current->task_works))
task_work_run();
in get_signal() that we had before this change. Might be safer in case
there are other cases that need to ensure the work is run in a timely
fashion, though I do think it's cleaner to long term to correctly mark
task_work with the needed notification type. Comments welcome...
diff --git a/fs/file_table.c b/fs/file_table.c
index 45437f8e1003..7c76b611c95b 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -338,7 +338,13 @@ void fput_many(struct file *file, unsigned int refs)
if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) {
init_task_work(>f_u.fu_rcuhead, fput);
- if (!task_work_add(task, >f_u.fu_rcuhead,
TWA_RESUME))
+ /*
+* We could be dependent on the fput task_work running,
+* eg for pipes where someone is waiting on release
+* being called. Use TWA_SIGNAL to ensure it's run
+* before the task goes idle.
+*/
+ if (!task_work_add(task, >f_u.fu_rcuhead,
TWA_SIGNAL))
return;
/*
* After this task has run exit_task_work(),
--
Jens Axboe
