[PATCH] autofs4: Use wait_event_killable

[PATCH] autofs4: Use wait_event_killable

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

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 <mawilcox@microsoft.com>
---
 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(&current->sighand->siglock, irqflags);
-		oldset = current->blocked;
-		shutdown_sigs_mask = SHUTDOWN_SIGS & ~oldset.sig[0];
-		siginitsetinv(&current->blocked, shutdown_sigs_mask);
-		recalc_sigpending();
-		spin_unlock_irqrestore(&current->sighand->siglock, irqflags);
-
-		wait_event_interruptible(wq->queue, wq->name.name == NULL);
-
-		spin_lock_irqsave(&current->sighand->siglock, irqflags);
-		current->blocked = oldset;
-		recalc_sigpending();
-		spin_unlock_irqrestore(&current->sighand->siglock, irqflags);
-	} else {
-		pr_debug("skipped sleeping\n");
-	}
-
+	wait_event_killable(wq->queue, wq->name.name == NULL);
 	status = wq->status;
 
 	/*
-- 
2.16.2

Re: [PATCH] autofs4: Use wait_event_killable

[David Rientjes]

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(&current->sighand->siglock, irqflags);
> -		oldset = current->blocked;
> -		shutdown_sigs_mask = SHUTDOWN_SIGS & ~oldset.sig[0];
> -		siginitsetinv(&current->blocked, shutdown_sigs_mask);
> -		recalc_sigpending();
> -		spin_unlock_irqrestore(&current->sighand->siglock, irqflags);
> -
> -		wait_event_interruptible(wq->queue, wq->name.name == NULL);
> -
> -		spin_lock_irqsave(&current->sighand->siglock, irqflags);
> -		current->blocked = oldset;
> -		recalc_sigpending();
> -		spin_unlock_irqrestore(&current->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.

Re: [PATCH] autofs4: Use wait_event_killable

[Matthew Wilcox]

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

[Ian Kent]

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(&current->sighand->siglock, irqflags);
>> -		oldset = current->blocked;
>> -		shutdown_sigs_mask = SHUTDOWN_SIGS & ~oldset.sig[0];
>> -		siginitsetinv(&current->blocked, shutdown_sigs_mask);
>> -		recalc_sigpending();
>> -		spin_unlock_irqrestore(&current->sighand->siglock, irqflags);
>> -
>> -		wait_event_interruptible(wq->queue, wq->name.name == NULL);
>> -
>> -		spin_lock_irqsave(&current->sighand->siglock, irqflags);
>> -		current->blocked = oldset;
>> -		recalc_sigpending();
>> -		spin_unlock_irqrestore(&current->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

[Ian Kent]

On 20/03/18 03:16, Matthew Wilcox wrote:
> From: Matthew Wilcox <mawilcox@microsoft.com>

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 <mawilcox@microsoft.com>
Signed-off-by: Ian Kent <raven@themaw.net>
> ---
>  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(&current->sighand->siglock, irqflags);
> -		oldset = current->blocked;
> -		shutdown_sigs_mask = SHUTDOWN_SIGS & ~oldset.sig[0];
> -		siginitsetinv(&current->blocked, shutdown_sigs_mask);
> -		recalc_sigpending();
> -		spin_unlock_irqrestore(&current->sighand->siglock, irqflags);
> -
> -		wait_event_interruptible(wq->queue, wq->name.name == NULL);
> -
> -		spin_lock_irqsave(&current->sighand->siglock, irqflags);
> -		current->blocked = oldset;
> -		recalc_sigpending();
> -		spin_unlock_irqrestore(&current->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

[Matthew Wilcox]

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 <mawilcox@microsoft.com>
> > 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-willy@infradead.org

> > Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> Signed-off-by: Ian Kent <raven@themaw.net>


> > +	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.