Re: [PATCH] autofs4: Use wait_event_killable
On Tue, Mar 20, 2018 at 09:58:59AM +0800, Ian Kent wrote: > On 20/03/18 03:16, Matthew Wilcox wrote: > > From: Matthew Wilcox > > This playing with signals to allow only fatal signals appears to predate > > the introduction of wait_event_killable(), and I'm fairly sure that > > wait_event_killable is what was meant to happen here. > > Predates is an understatement, this is really, really old code. > Do I need to forward this to Al or Andrew? Looks like Andrew usually picks these up directly. Here's the line he'll want: Link: http://lkml.kernel.org/r/20180319191609.23880-1-wi...@infradead.org > > Signed-off-by: Matthew Wilcox > Signed-off-by: Ian Kent > > + wait_event_killable(wq->queue, wq->name.name == NULL); > > The wait event code looks like this will wake up on most any unmasked signal. > But my assumption is that TASK_KILLABLE tasks are only forwarded specific > signals ... > > Is that right or am I missing something? The signal code is gnarly. As far as I can decipher it, a fatal signal is always turned into SIGKILL (in complete_signal()), and the task is woken. For a task sleeping in TASK_KILLABLE, signal_wake_up() passes TASK_WAKEKILL to signal_wake_up_state() if the signal is SIGKILL. TASK_KILLABLE sets (TASK_WAKEKILL | TASK_UNINTERRUPTIBLE) so it will be woken in order to die. If the signal being sent isn't sig_fatal(), then we don't wake the task. The signal will still be in the pending set, so it can notice when exiting to userspace, but it won't be woken.
Re: [PATCH] autofs4: Use wait_event_killable
On 20/03/18 03:25, David Rientjes wrote:
> On Mon, 19 Mar 2018, Matthew Wilcox wrote:
>
>> diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
>> index a0c57c37fa21..c160e9b3aa0f 100644
>> --- a/fs/autofs4/waitq.c
>> +++ b/fs/autofs4/waitq.c
>> @@ -19,9 +19,6 @@
>> */
>> static autofs_wqt_t autofs4_next_wait_queue = 1;
>>
>> -/* These are the signals we allow interrupting a pending mount */
>> -#define SHUTDOWN_SIGS (sigmask(SIGKILL) | sigmask(SIGINT) |
>> sigmask(SIGQUIT))
>> -
>> void autofs4_catatonic_mode(struct autofs_sb_info *sbi)
>> {
>> struct autofs_wait_queue *wq, *nwq;
>> @@ -486,29 +483,7 @@ int autofs4_wait(struct autofs_sb_info *sbi,
>> * wq->name.name is NULL iff the lock is already released
>> * or the mount has been made catatonic.
>> */
>> -if (wq->name.name) {
>> -/* Block all but "shutdown" signals while waiting */
>> -unsigned long shutdown_sigs_mask;
>> -unsigned long irqflags;
>> -sigset_t oldset;
>> -
>> -spin_lock_irqsave(¤t->sighand->siglock, irqflags);
>> -oldset = current->blocked;
>> -shutdown_sigs_mask = SHUTDOWN_SIGS & ~oldset.sig[0];
>> -siginitsetinv(¤t->blocked, shutdown_sigs_mask);
>> -recalc_sigpending();
>> -spin_unlock_irqrestore(¤t->sighand->siglock, irqflags);
>> -
>> -wait_event_interruptible(wq->queue, wq->name.name == NULL);
>> -
>> -spin_lock_irqsave(¤t->sighand->siglock, irqflags);
>> -current->blocked = oldset;
>> -recalc_sigpending();
>> -spin_unlock_irqrestore(¤t->sighand->siglock, irqflags);
>> -} else {
>> -pr_debug("skipped sleeping\n");
>> -}
>> -
>> +wait_event_killable(wq->queue, wq->name.name == NULL);
>> status = wq->status;
>>
>> /*
>
> I understand converting the wait_event_interruptible() to
> wait_event_killable(), but why was the above wait_event_interruptible()
> only called when wq->name.name != NULL?
The code pre-dates my involvement in autofs too.
I always thought it was because wq->name.name can become NULL before
the wait is reached. Such as if the user space daemon manages to invoke
autofs4_wait_release() before the wait call.
And if the autofs mount is made catatonic before the wait is reached
wq->name.name will be set to NULL and wake up called for each waiter
so that doesn't seem to require the if conditional either.
Both of these cases to fit with what Matthew has already said and I
can't think of any others.
There may have been other reasons at some point, a lot has changed
over (a long) time.
Ian
Re: [PATCH] autofs4: Use wait_event_killable
On 20/03/18 03:16, Matthew Wilcox wrote:
> From: Matthew Wilcox
Hi Matthew,
>
> This playing with signals to allow only fatal signals appears to predate
> the introduction of wait_event_killable(), and I'm fairly sure that
> wait_event_killable is what was meant to happen here.
Predates is an understatement, this is really, really old code.
Do I need to forward this to Al or Andrew?
>
> Signed-off-by: Matthew Wilcox
Signed-off-by: Ian Kent
> ---
> fs/autofs4/waitq.c | 27 +--
> 1 file changed, 1 insertion(+), 26 deletions(-)
>
> diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> index a0c57c37fa21..c160e9b3aa0f 100644
> --- a/fs/autofs4/waitq.c
> +++ b/fs/autofs4/waitq.c
> @@ -19,9 +19,6 @@
> */
> static autofs_wqt_t autofs4_next_wait_queue = 1;
>
> -/* These are the signals we allow interrupting a pending mount */
> -#define SHUTDOWN_SIGS(sigmask(SIGKILL) | sigmask(SIGINT) |
> sigmask(SIGQUIT))
> -
> void autofs4_catatonic_mode(struct autofs_sb_info *sbi)
> {
> struct autofs_wait_queue *wq, *nwq;
> @@ -486,29 +483,7 @@ int autofs4_wait(struct autofs_sb_info *sbi,
>* wq->name.name is NULL iff the lock is already released
>* or the mount has been made catatonic.
>*/
> - if (wq->name.name) {
> - /* Block all but "shutdown" signals while waiting */
> - unsigned long shutdown_sigs_mask;
> - unsigned long irqflags;
> - sigset_t oldset;
> -
> - spin_lock_irqsave(¤t->sighand->siglock, irqflags);
> - oldset = current->blocked;
> - shutdown_sigs_mask = SHUTDOWN_SIGS & ~oldset.sig[0];
> - siginitsetinv(¤t->blocked, shutdown_sigs_mask);
> - recalc_sigpending();
> - spin_unlock_irqrestore(¤t->sighand->siglock, irqflags);
> -
> - wait_event_interruptible(wq->queue, wq->name.name == NULL);
> -
> - spin_lock_irqsave(¤t->sighand->siglock, irqflags);
> - current->blocked = oldset;
> - recalc_sigpending();
> - spin_unlock_irqrestore(¤t->sighand->siglock, irqflags);
> - } else {
> - pr_debug("skipped sleeping\n");
> - }
> -
> + wait_event_killable(wq->queue, wq->name.name == NULL);
The wait event code looks like this will wake up on most any unmasked signal.
But my assumption is that TASK_KILLABLE tasks are only forwarded specific
signals ...
Is that right or am I missing something?
> status = wq->status;
>
> /*
>
Re: [PATCH] autofs4: Use wait_event_killable
On Mon, Mar 19, 2018 at 12:25:58PM -0700, David Rientjes wrote:
> On Mon, 19 Mar 2018, Matthew Wilcox wrote:
> > * wq->name.name is NULL iff the lock is already released
> > * or the mount has been made catatonic.
> > */
> > - if (wq->name.name) {
> > - /* Block all but "shutdown" signals while waiting */
> > - unsigned long shutdown_sigs_mask;
> > - unsigned long irqflags;
> > - sigset_t oldset;
> > -
[...]
> > -
> > - wait_event_interruptible(wq->queue, wq->name.name == NULL);
[...]
> > - } else {
> > - pr_debug("skipped sleeping\n");
> > - }
> > -
> > + wait_event_killable(wq->queue, wq->name.name == NULL);
>
> I understand converting the wait_event_interruptible() to
> wait_event_killable(), but why was the above wait_event_interruptible()
> only called when wq->name.name != NULL?
My guess is that it was to avoid the overhead of diddling the signal set
when wq->name.name was already NULL. I don't really kow though, it
predates git history and I'm too lazy to go and poke through the historical
repos to see if that reason was captured by BitKeeper.
> wait_event_{killable,interruptible}() will return without sleeping when
> wq->name.name == NULL, so I suppose it has something to do with the
> comment above it.
Re: [PATCH] autofs4: Use wait_event_killable
On Mon, 19 Mar 2018, Matthew Wilcox wrote:
> diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> index a0c57c37fa21..c160e9b3aa0f 100644
> --- a/fs/autofs4/waitq.c
> +++ b/fs/autofs4/waitq.c
> @@ -19,9 +19,6 @@
> */
> static autofs_wqt_t autofs4_next_wait_queue = 1;
>
> -/* These are the signals we allow interrupting a pending mount */
> -#define SHUTDOWN_SIGS(sigmask(SIGKILL) | sigmask(SIGINT) |
> sigmask(SIGQUIT))
> -
> void autofs4_catatonic_mode(struct autofs_sb_info *sbi)
> {
> struct autofs_wait_queue *wq, *nwq;
> @@ -486,29 +483,7 @@ int autofs4_wait(struct autofs_sb_info *sbi,
>* wq->name.name is NULL iff the lock is already released
>* or the mount has been made catatonic.
>*/
> - if (wq->name.name) {
> - /* Block all but "shutdown" signals while waiting */
> - unsigned long shutdown_sigs_mask;
> - unsigned long irqflags;
> - sigset_t oldset;
> -
> - spin_lock_irqsave(¤t->sighand->siglock, irqflags);
> - oldset = current->blocked;
> - shutdown_sigs_mask = SHUTDOWN_SIGS & ~oldset.sig[0];
> - siginitsetinv(¤t->blocked, shutdown_sigs_mask);
> - recalc_sigpending();
> - spin_unlock_irqrestore(¤t->sighand->siglock, irqflags);
> -
> - wait_event_interruptible(wq->queue, wq->name.name == NULL);
> -
> - spin_lock_irqsave(¤t->sighand->siglock, irqflags);
> - current->blocked = oldset;
> - recalc_sigpending();
> - spin_unlock_irqrestore(¤t->sighand->siglock, irqflags);
> - } else {
> - pr_debug("skipped sleeping\n");
> - }
> -
> + wait_event_killable(wq->queue, wq->name.name == NULL);
> status = wq->status;
>
> /*
I understand converting the wait_event_interruptible() to
wait_event_killable(), but why was the above wait_event_interruptible()
only called when wq->name.name != NULL?
wait_event_{killable,interruptible}() will return without sleeping when
wq->name.name == NULL, so I suppose it has something to do with the
comment above it.
[PATCH] autofs4: Use wait_event_killable
From: Matthew Wilcox
This playing with signals to allow only fatal signals appears to predate
the introduction of wait_event_killable(), and I'm fairly sure that
wait_event_killable is what was meant to happen here.
Signed-off-by: Matthew Wilcox
---
fs/autofs4/waitq.c | 27 +--
1 file changed, 1 insertion(+), 26 deletions(-)
diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index a0c57c37fa21..c160e9b3aa0f 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -19,9 +19,6 @@
*/
static autofs_wqt_t autofs4_next_wait_queue = 1;
-/* These are the signals we allow interrupting a pending mount */
-#define SHUTDOWN_SIGS (sigmask(SIGKILL) | sigmask(SIGINT) | sigmask(SIGQUIT))
-
void autofs4_catatonic_mode(struct autofs_sb_info *sbi)
{
struct autofs_wait_queue *wq, *nwq;
@@ -486,29 +483,7 @@ int autofs4_wait(struct autofs_sb_info *sbi,
* wq->name.name is NULL iff the lock is already released
* or the mount has been made catatonic.
*/
- if (wq->name.name) {
- /* Block all but "shutdown" signals while waiting */
- unsigned long shutdown_sigs_mask;
- unsigned long irqflags;
- sigset_t oldset;
-
- spin_lock_irqsave(¤t->sighand->siglock, irqflags);
- oldset = current->blocked;
- shutdown_sigs_mask = SHUTDOWN_SIGS & ~oldset.sig[0];
- siginitsetinv(¤t->blocked, shutdown_sigs_mask);
- recalc_sigpending();
- spin_unlock_irqrestore(¤t->sighand->siglock, irqflags);
-
- wait_event_interruptible(wq->queue, wq->name.name == NULL);
-
- spin_lock_irqsave(¤t->sighand->siglock, irqflags);
- current->blocked = oldset;
- recalc_sigpending();
- spin_unlock_irqrestore(¤t->sighand->siglock, irqflags);
- } else {
- pr_debug("skipped sleeping\n");
- }
-
+ wait_event_killable(wq->queue, wq->name.name == NULL);
status = wq->status;
/*
--
2.16.2
