| Message ID | 20180111201417.2042-3-snitzer@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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
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
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
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 --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);
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(-)