All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] autofs4: Use wait_event_killable
@ 2018-03-19 19:16 Matthew Wilcox
  2018-03-19 19:25 ` David Rientjes
  2018-03-20  1:58 ` Ian Kent
  0 siblings, 2 replies; 6+ messages in thread
From: Matthew Wilcox @ 2018-03-19 19:16 UTC (permalink / raw)
  To: Ian Kent; +Cc: autofs, linux-fsdevel, linux-kernel, 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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] autofs4: Use wait_event_killable
  2018-03-19 19:16 [PATCH] autofs4: Use wait_event_killable Matthew Wilcox
@ 2018-03-19 19:25 ` David Rientjes
  2018-03-19 19:30   ` Matthew Wilcox
  2018-03-20  2:42   ` Ian Kent
  2018-03-20  1:58 ` Ian Kent
  1 sibling, 2 replies; 6+ messages in thread
From: David Rientjes @ 2018-03-19 19:25 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Ian Kent, autofs, linux-fsdevel, linux-kernel, Matthew Wilcox

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] autofs4: Use wait_event_killable
  2018-03-19 19:25 ` David Rientjes
@ 2018-03-19 19:30   ` Matthew Wilcox
  2018-03-20  2:42   ` Ian Kent
  1 sibling, 0 replies; 6+ messages in thread
From: Matthew Wilcox @ 2018-03-19 19:30 UTC (permalink / raw)
  To: David Rientjes
  Cc: Ian Kent, autofs, linux-fsdevel, linux-kernel, 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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] autofs4: Use wait_event_killable
  2018-03-19 19:16 [PATCH] autofs4: Use wait_event_killable Matthew Wilcox
  2018-03-19 19:25 ` David Rientjes
@ 2018-03-20  1:58 ` Ian Kent
  2018-03-20  3:12   ` Matthew Wilcox
  1 sibling, 1 reply; 6+ messages in thread
From: Ian Kent @ 2018-03-20  1:58 UTC (permalink / raw)
  To: Matthew Wilcox, Andrew Morton, viro; +Cc: autofs, linux-fsdevel, linux-kernel

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;
>  
>  	/*
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] autofs4: Use wait_event_killable
  2018-03-19 19:25 ` David Rientjes
  2018-03-19 19:30   ` Matthew Wilcox
@ 2018-03-20  2:42   ` Ian Kent
  1 sibling, 0 replies; 6+ messages in thread
From: Ian Kent @ 2018-03-20  2:42 UTC (permalink / raw)
  To: David Rientjes, Matthew Wilcox
  Cc: autofs, linux-fsdevel, linux-kernel, Matthew Wilcox

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] autofs4: Use wait_event_killable
  2018-03-20  1:58 ` Ian Kent
@ 2018-03-20  3:12   ` Matthew Wilcox
  0 siblings, 0 replies; 6+ messages in thread
From: Matthew Wilcox @ 2018-03-20  3:12 UTC (permalink / raw)
  To: Ian Kent; +Cc: Andrew Morton, viro, autofs, linux-fsdevel, linux-kernel

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-03-20  3:12 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-19 19:16 [PATCH] autofs4: Use wait_event_killable Matthew Wilcox
2018-03-19 19:25 ` David Rientjes
2018-03-19 19:30   ` Matthew Wilcox
2018-03-20  2:42   ` Ian Kent
2018-03-20  1:58 ` Ian Kent
2018-03-20  3:12   ` Matthew Wilcox

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.