diff mbox

[for-4.16,v4,2/4] block: use queue_lock when clearing QUEUE_FLAG_REGISTERED in blk_unregister_queue

Message ID 20180111201417.2042-3-snitzer@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mike Snitzer Jan. 11, 2018, 8:14 p.m. UTC
blk_unregister_queue() must protect against any modifications of
q->queue_flags (not just those performed in blk-sysfs.c).  Therefore
q->queue_lock needs to be used rather than q->sysfs_lock.

Fixes: e9a823fb34a8b ("block: fix warning when I/O elevator is changed as request_queue is being removed")
Cc: stable@vger.kernel.org # 4.14+
Reported-by: Bart Van Assche <Bart.VanAssche@wdc.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 block/blk-sysfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Bart Van Assche Jan. 12, 2018, 12:28 a.m. UTC | #1
On Thu, 2018-01-11 at 15:14 -0500, Mike Snitzer wrote:
> blk_unregister_queue() must protect against any modifications of

> q->queue_flags (not just those performed in blk-sysfs.c).  Therefore

> q->queue_lock needs to be used rather than q->sysfs_lock.

> 

> Fixes: e9a823fb34a8b ("block: fix warning when I/O elevator is changed as request_queue is being removed")

> Cc: stable@vger.kernel.org # 4.14+

> Reported-by: Bart Van Assche <Bart.VanAssche@wdc.com>

> Signed-off-by: Mike Snitzer <snitzer@redhat.com>

> ---

>  block/blk-sysfs.c | 4 ++--

>  1 file changed, 2 insertions(+), 2 deletions(-)

> 

> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c

> index 870484eaed1f..52f57539f1c7 100644

> --- a/block/blk-sysfs.c

> +++ b/block/blk-sysfs.c

> @@ -929,9 +929,9 @@ void blk_unregister_queue(struct gendisk *disk)

>  	if (WARN_ON(!q))

>  		return;

>  

> -	mutex_lock(&q->sysfs_lock);

> +	spin_lock_irq(q->queue_lock);

>  	queue_flag_clear_unlocked(QUEUE_FLAG_REGISTERED, q);

> -	mutex_unlock(&q->sysfs_lock);

> +	spin_unlock_irq(q->queue_lock);


Hello Mike,

The function name queue_flag_clear_unlocked() means "clear a queue flag
without holding the queue lock". So at least to me the above code is confusing.
Please consider to change queue_flag_clear_unlocked() into queue_flag_clear().

Thanks,

Bart.
Mike Snitzer Jan. 12, 2018, 2:53 a.m. UTC | #2
On Thu, Jan 11 2018 at  7:28pm -0500,
Bart Van Assche <Bart.VanAssche@wdc.com> wrote:

> On Thu, 2018-01-11 at 15:14 -0500, Mike Snitzer wrote:
> > blk_unregister_queue() must protect against any modifications of
> > q->queue_flags (not just those performed in blk-sysfs.c).  Therefore
> > q->queue_lock needs to be used rather than q->sysfs_lock.
> > 
> > Fixes: e9a823fb34a8b ("block: fix warning when I/O elevator is changed as request_queue is being removed")
> > Cc: stable@vger.kernel.org # 4.14+
> > Reported-by: Bart Van Assche <Bart.VanAssche@wdc.com>
> > Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> > ---
> >  block/blk-sysfs.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> > index 870484eaed1f..52f57539f1c7 100644
> > --- a/block/blk-sysfs.c
> > +++ b/block/blk-sysfs.c
> > @@ -929,9 +929,9 @@ void blk_unregister_queue(struct gendisk *disk)
> >  	if (WARN_ON(!q))
> >  		return;
> >  
> > -	mutex_lock(&q->sysfs_lock);
> > +	spin_lock_irq(q->queue_lock);
> >  	queue_flag_clear_unlocked(QUEUE_FLAG_REGISTERED, q);
> > -	mutex_unlock(&q->sysfs_lock);
> > +	spin_unlock_irq(q->queue_lock);
> 
> Hello Mike,
> 
> The function name queue_flag_clear_unlocked() means "clear a queue flag
> without holding the queue lock". So at least to me the above code is confusing.
> Please consider to change queue_flag_clear_unlocked() into queue_flag_clear().

If Jens would like to change it when applying the patch to his tree
that is fine by me.

But as you know, it doesn't matter:
queue_flag_clear() just has extra queue_lockdep_assert_held(q);

So I see no reason to respin this patch for this.  Especially when you
consider patch 3 replaces it with queue_flag_test_and_clear() -- and no
it isn't a problem for stable@ to carry on using
queue_flag_clear_unlocked

Mike
Ming Lei Jan. 12, 2018, 7:09 a.m. UTC | #3
On Thu, Jan 11, 2018 at 03:14:15PM -0500, Mike Snitzer wrote:
> blk_unregister_queue() must protect against any modifications of
> q->queue_flags (not just those performed in blk-sysfs.c).  Therefore
> q->queue_lock needs to be used rather than q->sysfs_lock.
> 
> Fixes: e9a823fb34a8b ("block: fix warning when I/O elevator is changed as request_queue is being removed")
> Cc: stable@vger.kernel.org # 4.14+
> Reported-by: Bart Van Assche <Bart.VanAssche@wdc.com>
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> ---
>  block/blk-sysfs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 870484eaed1f..52f57539f1c7 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -929,9 +929,9 @@ void blk_unregister_queue(struct gendisk *disk)
>  	if (WARN_ON(!q))
>  		return;
>  
> -	mutex_lock(&q->sysfs_lock);
> +	spin_lock_irq(q->queue_lock);
>  	queue_flag_clear_unlocked(QUEUE_FLAG_REGISTERED, q);
> -	mutex_unlock(&q->sysfs_lock);
> +	spin_unlock_irq(q->queue_lock);
>  
>  	wbt_exit(q);

Hi Mike,

This change may not be correct, since at least e9a823fb34a8b depends
on q->sysfs_lock to sync between testing the flag in __elevator_change()
and clearing it here.

Thanks,
Ming
Mike Snitzer Jan. 12, 2018, 12:53 p.m. UTC | #4
On Fri, Jan 12 2018 at  2:09am -0500,
Ming Lei <ming.lei@redhat.com> wrote:

> On Thu, Jan 11, 2018 at 03:14:15PM -0500, Mike Snitzer wrote:
> > blk_unregister_queue() must protect against any modifications of
> > q->queue_flags (not just those performed in blk-sysfs.c).  Therefore
> > q->queue_lock needs to be used rather than q->sysfs_lock.
> > 
> > Fixes: e9a823fb34a8b ("block: fix warning when I/O elevator is changed as request_queue is being removed")
> > Cc: stable@vger.kernel.org # 4.14+
> > Reported-by: Bart Van Assche <Bart.VanAssche@wdc.com>
> > Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> > ---
> >  block/blk-sysfs.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> > index 870484eaed1f..52f57539f1c7 100644
> > --- a/block/blk-sysfs.c
> > +++ b/block/blk-sysfs.c
> > @@ -929,9 +929,9 @@ void blk_unregister_queue(struct gendisk *disk)
> >  	if (WARN_ON(!q))
> >  		return;
> >  
> > -	mutex_lock(&q->sysfs_lock);
> > +	spin_lock_irq(q->queue_lock);
> >  	queue_flag_clear_unlocked(QUEUE_FLAG_REGISTERED, q);
> > -	mutex_unlock(&q->sysfs_lock);
> > +	spin_unlock_irq(q->queue_lock);
> >  
> >  	wbt_exit(q);
> 
> Hi Mike,
> 
> This change may not be correct, since at least e9a823fb34a8b depends
> on q->sysfs_lock to sync between testing the flag in __elevator_change()
> and clearing it here.

The header for commit e9a823fb34a8b says:
    To fix this warning, we can check the QUEUE_FLAG_REGISTERED flag when
    changing the elevator and use the request_queue's sysfs_lock to
    serialize between clearing the flag and the elevator testing the flag.

The reality is sysfs_lock isn't needed to serialize between
blk_unregister_queue() clearing the flag and __elevator_change() testing
the flag.

The original commit e9a823fb34a8b is pretty conflated.  "conflated" because
the resource being protected isn't the queue_flags (it is the 'queue'
kobj).

I'll respin v5 of this patchset to fix this up first, and then apply the
changes I _really_ need to land (DM queue initialization fix).

And then I'm going to slowly step away from block core and _not_ allow
myself to be tripped up, or baited, by historic block core issues for a
while... ;)

Thanks,
Mike
Ming Lei Jan. 12, 2018, 2:14 p.m. UTC | #5
On Fri, Jan 12, 2018 at 07:53:40AM -0500, Mike Snitzer wrote:
> On Fri, Jan 12 2018 at  2:09am -0500,
> Ming Lei <ming.lei@redhat.com> wrote:
> 
> > On Thu, Jan 11, 2018 at 03:14:15PM -0500, Mike Snitzer wrote:
> > > blk_unregister_queue() must protect against any modifications of
> > > q->queue_flags (not just those performed in blk-sysfs.c).  Therefore
> > > q->queue_lock needs to be used rather than q->sysfs_lock.
> > > 
> > > Fixes: e9a823fb34a8b ("block: fix warning when I/O elevator is changed as request_queue is being removed")
> > > Cc: stable@vger.kernel.org # 4.14+
> > > Reported-by: Bart Van Assche <Bart.VanAssche@wdc.com>
> > > Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> > > ---
> > >  block/blk-sysfs.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> > > index 870484eaed1f..52f57539f1c7 100644
> > > --- a/block/blk-sysfs.c
> > > +++ b/block/blk-sysfs.c
> > > @@ -929,9 +929,9 @@ void blk_unregister_queue(struct gendisk *disk)
> > >  	if (WARN_ON(!q))
> > >  		return;
> > >  
> > > -	mutex_lock(&q->sysfs_lock);
> > > +	spin_lock_irq(q->queue_lock);
> > >  	queue_flag_clear_unlocked(QUEUE_FLAG_REGISTERED, q);
> > > -	mutex_unlock(&q->sysfs_lock);
> > > +	spin_unlock_irq(q->queue_lock);
> > >  
> > >  	wbt_exit(q);
> > 
> > Hi Mike,
> > 
> > This change may not be correct, since at least e9a823fb34a8b depends
> > on q->sysfs_lock to sync between testing the flag in __elevator_change()
> > and clearing it here.
> 
> The header for commit e9a823fb34a8b says:
>     To fix this warning, we can check the QUEUE_FLAG_REGISTERED flag when
>     changing the elevator and use the request_queue's sysfs_lock to
>     serialize between clearing the flag and the elevator testing the flag.
> 
> The reality is sysfs_lock isn't needed to serialize between
> blk_unregister_queue() clearing the flag and __elevator_change() testing
> the flag.

Without holding sysfs_lock, __elevator_change() may just be called after
q->kobj is deleted from blk_unregister_queue(), then the warning of
'WARNING: CPU: 3 PID: 14075 at lib/kobject.c:244 kobject_add_internal+0x103/0x2d0'
can be triggered again.

BTW, __elevator_change() is always run with holding sysfs_lock.

Thanks,
Ming
Mike Snitzer Jan. 12, 2018, 3:05 p.m. UTC | #6
On Fri, Jan 12 2018 at  9:14am -0500,
Ming Lei <ming.lei@redhat.com> wrote:

> On Fri, Jan 12, 2018 at 07:53:40AM -0500, Mike Snitzer wrote:
> > On Fri, Jan 12 2018 at  2:09am -0500,
> > Ming Lei <ming.lei@redhat.com> wrote:
> > 
> > > On Thu, Jan 11, 2018 at 03:14:15PM -0500, Mike Snitzer wrote:
> > > > blk_unregister_queue() must protect against any modifications of
> > > > q->queue_flags (not just those performed in blk-sysfs.c).  Therefore
> > > > q->queue_lock needs to be used rather than q->sysfs_lock.
> > > > 
> > > > Fixes: e9a823fb34a8b ("block: fix warning when I/O elevator is changed as request_queue is being removed")
> > > > Cc: stable@vger.kernel.org # 4.14+
> > > > Reported-by: Bart Van Assche <Bart.VanAssche@wdc.com>
> > > > Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> > > > ---
> > > >  block/blk-sysfs.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> > > > index 870484eaed1f..52f57539f1c7 100644
> > > > --- a/block/blk-sysfs.c
> > > > +++ b/block/blk-sysfs.c
> > > > @@ -929,9 +929,9 @@ void blk_unregister_queue(struct gendisk *disk)
> > > >  	if (WARN_ON(!q))
> > > >  		return;
> > > >  
> > > > -	mutex_lock(&q->sysfs_lock);
> > > > +	spin_lock_irq(q->queue_lock);
> > > >  	queue_flag_clear_unlocked(QUEUE_FLAG_REGISTERED, q);
> > > > -	mutex_unlock(&q->sysfs_lock);
> > > > +	spin_unlock_irq(q->queue_lock);
> > > >  
> > > >  	wbt_exit(q);
> > > 
> > > Hi Mike,
> > > 
> > > This change may not be correct, since at least e9a823fb34a8b depends
> > > on q->sysfs_lock to sync between testing the flag in __elevator_change()
> > > and clearing it here.
> > 
> > The header for commit e9a823fb34a8b says:
> >     To fix this warning, we can check the QUEUE_FLAG_REGISTERED flag when
> >     changing the elevator and use the request_queue's sysfs_lock to
> >     serialize between clearing the flag and the elevator testing the flag.
> > 
> > The reality is sysfs_lock isn't needed to serialize between
> > blk_unregister_queue() clearing the flag and __elevator_change() testing
> > the flag.
> 
> Without holding sysfs_lock, __elevator_change() may just be called after
> q->kobj is deleted from blk_unregister_queue(), then the warning of
> 'WARNING: CPU: 3 PID: 14075 at lib/kobject.c:244 kobject_add_internal+0x103/0x2d0'
> can be triggered again.
> 
> BTW, __elevator_change() is always run with holding sysfs_lock.

Yes, I'm well aware.  Please see v5's PATCH 02/04 which is inbound now.

Thanks,
Mike
diff mbox

Patch

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 870484eaed1f..52f57539f1c7 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -929,9 +929,9 @@  void blk_unregister_queue(struct gendisk *disk)
 	if (WARN_ON(!q))
 		return;
 
-	mutex_lock(&q->sysfs_lock);
+	spin_lock_irq(q->queue_lock);
 	queue_flag_clear_unlocked(QUEUE_FLAG_REGISTERED, q);
-	mutex_unlock(&q->sysfs_lock);
+	spin_unlock_irq(q->queue_lock);
 
 	wbt_exit(q);