linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V3 0/2] block: fix race between adding wbt and normal IO
@ 2021-06-09  1:58 Ming Lei
  2021-06-09  1:58 ` [PATCH V3 1/2] block: fix race between adding/removing rq qos " Ming Lei
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Ming Lei @ 2021-06-09  1:58 UTC (permalink / raw)
  To: Jens Axboe, Christoph Hellwig
  Cc: linux-block, Ming Lei, Yi Zhang, Bart Van Assche

Hello,

Yi reported several kernel panics on:

[16687.001777] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000008
...
[16687.163549] pc : __rq_qos_track+0x38/0x60

or

[  997.690455] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000020
...
[  997.850347] pc : __rq_qos_done+0x2c/0x50

Turns out it is caused by race between adding wbt and normal IO.

Fix the issue by freezing request queue when adding/deleting rq qos.

V3:
	- use ->queue_lock for protecting concurrent adding/deleting rqos on
	  same queue

V2:
	- switch to the approach of freezing queue, which is more generic
	  than V1.



Ming Lei (2):
  block: fix race between adding/removing rq qos and normal IO
  block: mark queue init done at the end of blk_register_queue

 block/blk-rq-qos.h | 24 ++++++++++++++++++++++++
 block/blk-sysfs.c  | 29 +++++++++++++++--------------
 2 files changed, 39 insertions(+), 14 deletions(-)

Cc: Yi Zhang <yi.zhang@redhat.com>
Cc: Bart Van Assche <bvanassche@acm.org>
-- 
2.31.1


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

* [PATCH V3 1/2] block: fix race between adding/removing rq qos and normal IO
  2021-06-09  1:58 [PATCH V3 0/2] block: fix race between adding wbt and normal IO Ming Lei
@ 2021-06-09  1:58 ` Ming Lei
  2021-06-09 20:05   ` Bart Van Assche
  2021-06-09  1:58 ` [PATCH V3 2/2] block: mark queue init done at the end of blk_register_queue Ming Lei
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Ming Lei @ 2021-06-09  1:58 UTC (permalink / raw)
  To: Jens Axboe, Christoph Hellwig
  Cc: linux-block, Ming Lei, Yi Zhang, Bart Van Assche

Yi reported several kernel panics on:

[16687.001777] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000008
...
[16687.163549] pc : __rq_qos_track+0x38/0x60

or

[  997.690455] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000020
...
[  997.850347] pc : __rq_qos_done+0x2c/0x50

Turns out it is caused by race between adding rq qos(wbt) and normal IO
because rq_qos_add can be run when IO is being submitted, fix this issue
by freezing queue before adding/deleting rq qos to queue.

rq_qos_exit() needn't to freeze queue because it is called after queue
has been frozen.

iolatency calls rq_qos_add() during allocating queue, so freezing won't
add delay because queue usage refcount works at atomic mode at that
time.

iocost calls rq_qos_add() when writing cgroup attribute file, that is
fine to freeze queue at that time since we usually freeze queue when
storing to queue sysfs attribute, meantime iocost only exists on the
root cgroup.

wbt_init calls it in blk_register_queue() and queue sysfs attribute
store(queue_wb_lat_store() when write it 1st time in case of !BLK_WBT_MQ),
the following patch will speedup the queue freezing in wbt_init.

Reported-by: Yi Zhang <yi.zhang@redhat.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-rq-qos.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/block/blk-rq-qos.h b/block/blk-rq-qos.h
index 2bc43e94f4c4..2bcb3495e376 100644
--- a/block/blk-rq-qos.h
+++ b/block/blk-rq-qos.h
@@ -7,6 +7,7 @@
 #include <linux/blk_types.h>
 #include <linux/atomic.h>
 #include <linux/wait.h>
+#include <linux/blk-mq.h>
 
 #include "blk-mq-debugfs.h"
 
@@ -99,8 +100,21 @@ static inline void rq_wait_init(struct rq_wait *rq_wait)
 
 static inline void rq_qos_add(struct request_queue *q, struct rq_qos *rqos)
 {
+	/*
+	 * No IO can be in-flight when adding rqos, so freeze queue, which
+	 * is fine since we only support rq_qos for blk-mq queue.
+	 *
+	 * Reuse ->queue_lock for protecting against other concurrent
+	 * rq_qos adding/deleting
+	 */
+	blk_mq_freeze_queue(q);
+
+	spin_lock_irq(&q->queue_lock);
 	rqos->next = q->rq_qos;
 	q->rq_qos = rqos;
+	spin_unlock_irq(&q->queue_lock);
+
+	blk_mq_unfreeze_queue(q);
 
 	if (rqos->ops->debugfs_attrs)
 		blk_mq_debugfs_register_rqos(rqos);
@@ -110,12 +124,22 @@ static inline void rq_qos_del(struct request_queue *q, struct rq_qos *rqos)
 {
 	struct rq_qos **cur;
 
+	/*
+	 * See comment in rq_qos_add() about freezing queue & using
+	 * ->queue_lock.
+	 */
+	blk_mq_freeze_queue(q);
+
+	spin_lock_irq(&q->queue_lock);
 	for (cur = &q->rq_qos; *cur; cur = &(*cur)->next) {
 		if (*cur == rqos) {
 			*cur = rqos->next;
 			break;
 		}
 	}
+	spin_unlock_irq(&q->queue_lock);
+
+	blk_mq_unfreeze_queue(q);
 
 	blk_mq_debugfs_unregister_rqos(rqos);
 }
-- 
2.31.1


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

* [PATCH V3 2/2] block: mark queue init done at the end of blk_register_queue
  2021-06-09  1:58 [PATCH V3 0/2] block: fix race between adding wbt and normal IO Ming Lei
  2021-06-09  1:58 ` [PATCH V3 1/2] block: fix race between adding/removing rq qos " Ming Lei
@ 2021-06-09  1:58 ` Ming Lei
  2021-06-09 20:09   ` Bart Van Assche
  2021-06-10  0:45 ` [PATCH V3 0/2] block: fix race between adding wbt and normal IO Yi Zhang
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Ming Lei @ 2021-06-09  1:58 UTC (permalink / raw)
  To: Jens Axboe, Christoph Hellwig
  Cc: linux-block, Ming Lei, Yi Zhang, Bart Van Assche

Mark queue init done when everything is done well in blk_register_queue(),
so that wbt_enable_default() can be run quickly without any RCU period
involved since adding rq qos requires to freeze queue.

Also no any side effect by delaying to mark queue init done.

Reported-by: Yi Zhang <yi.zhang@redhat.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-sysfs.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index f89e2fc3963b..370d83c18057 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -866,20 +866,6 @@ int blk_register_queue(struct gendisk *disk)
 		  "%s is registering an already registered queue\n",
 		  kobject_name(&dev->kobj));
 
-	/*
-	 * SCSI probing may synchronously create and destroy a lot of
-	 * request_queues for non-existent devices.  Shutting down a fully
-	 * functional queue takes measureable wallclock time as RCU grace
-	 * periods are involved.  To avoid excessive latency in these
-	 * cases, a request_queue starts out in a degraded mode which is
-	 * faster to shut down and is made fully functional here as
-	 * request_queues for non-existent devices never get registered.
-	 */
-	if (!blk_queue_init_done(q)) {
-		blk_queue_flag_set(QUEUE_FLAG_INIT_DONE, q);
-		percpu_ref_switch_to_percpu(&q->q_usage_counter);
-	}
-
 	blk_queue_update_readahead(q);
 
 	ret = blk_trace_init_sysfs(dev);
@@ -938,6 +924,21 @@ int blk_register_queue(struct gendisk *disk)
 	ret = 0;
 unlock:
 	mutex_unlock(&q->sysfs_dir_lock);
+
+	/*
+	 * SCSI probing may synchronously create and destroy a lot of
+	 * request_queues for non-existent devices.  Shutting down a fully
+	 * functional queue takes measureable wallclock time as RCU grace
+	 * periods are involved.  To avoid excessive latency in these
+	 * cases, a request_queue starts out in a degraded mode which is
+	 * faster to shut down and is made fully functional here as
+	 * request_queues for non-existent devices never get registered.
+	 */
+	if (!blk_queue_init_done(q)) {
+		blk_queue_flag_set(QUEUE_FLAG_INIT_DONE, q);
+		percpu_ref_switch_to_percpu(&q->q_usage_counter);
+	}
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(blk_register_queue);
-- 
2.31.1


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

* Re: [PATCH V3 1/2] block: fix race between adding/removing rq qos and normal IO
  2021-06-09  1:58 ` [PATCH V3 1/2] block: fix race between adding/removing rq qos " Ming Lei
@ 2021-06-09 20:05   ` Bart Van Assche
  0 siblings, 0 replies; 9+ messages in thread
From: Bart Van Assche @ 2021-06-09 20:05 UTC (permalink / raw)
  To: Ming Lei, Jens Axboe, Christoph Hellwig; +Cc: linux-block, Yi Zhang

On 6/8/21 6:58 PM, Ming Lei wrote:
> Yi reported several kernel panics on [ ... ]

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

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

* Re: [PATCH V3 2/2] block: mark queue init done at the end of blk_register_queue
  2021-06-09  1:58 ` [PATCH V3 2/2] block: mark queue init done at the end of blk_register_queue Ming Lei
@ 2021-06-09 20:09   ` Bart Van Assche
  0 siblings, 0 replies; 9+ messages in thread
From: Bart Van Assche @ 2021-06-09 20:09 UTC (permalink / raw)
  To: Ming Lei, Jens Axboe, Christoph Hellwig; +Cc: linux-block, Yi Zhang

On 6/8/21 6:58 PM, Ming Lei wrote:
> Mark queue init done when everything is done well in blk_register_queue(),
> so that wbt_enable_default() can be run quickly without any RCU period
> involved since adding rq qos requires to freeze queue.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

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

* Re: [PATCH V3 0/2] block: fix race between adding wbt and normal IO
  2021-06-09  1:58 [PATCH V3 0/2] block: fix race between adding wbt and normal IO Ming Lei
  2021-06-09  1:58 ` [PATCH V3 1/2] block: fix race between adding/removing rq qos " Ming Lei
  2021-06-09  1:58 ` [PATCH V3 2/2] block: mark queue init done at the end of blk_register_queue Ming Lei
@ 2021-06-10  0:45 ` Yi Zhang
  2021-06-14 21:55 ` Ming Lei
  2021-06-16 14:42 ` Jens Axboe
  4 siblings, 0 replies; 9+ messages in thread
From: Yi Zhang @ 2021-06-10  0:45 UTC (permalink / raw)
  To: Ming Lei; +Cc: Jens Axboe, Christoph Hellwig, linux-block, Bart Van Assche

Thanks for the fix.

Tested-by: Yi Zhang <yi.zhang@redhat.com>

On Wed, Jun 9, 2021 at 9:58 AM Ming Lei <ming.lei@redhat.com> wrote:
>
> Hello,
>
> Yi reported several kernel panics on:
>
> [16687.001777] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000008
> ...
> [16687.163549] pc : __rq_qos_track+0x38/0x60
>
> or
>
> [  997.690455] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000020
> ...
> [  997.850347] pc : __rq_qos_done+0x2c/0x50
>
> Turns out it is caused by race between adding wbt and normal IO.
>
> Fix the issue by freezing request queue when adding/deleting rq qos.
>
> V3:
>         - use ->queue_lock for protecting concurrent adding/deleting rqos on
>           same queue
>
> V2:
>         - switch to the approach of freezing queue, which is more generic
>           than V1.
>
>
>
> Ming Lei (2):
>   block: fix race between adding/removing rq qos and normal IO
>   block: mark queue init done at the end of blk_register_queue
>
>  block/blk-rq-qos.h | 24 ++++++++++++++++++++++++
>  block/blk-sysfs.c  | 29 +++++++++++++++--------------
>  2 files changed, 39 insertions(+), 14 deletions(-)
>
> Cc: Yi Zhang <yi.zhang@redhat.com>
> Cc: Bart Van Assche <bvanassche@acm.org>
> --
> 2.31.1
>


-- 
Best Regards,
  Yi Zhang


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

* Re: [PATCH V3 0/2] block: fix race between adding wbt and normal IO
  2021-06-09  1:58 [PATCH V3 0/2] block: fix race between adding wbt and normal IO Ming Lei
                   ` (2 preceding siblings ...)
  2021-06-10  0:45 ` [PATCH V3 0/2] block: fix race between adding wbt and normal IO Yi Zhang
@ 2021-06-14 21:55 ` Ming Lei
  2021-06-16 14:11   ` Ming Lei
  2021-06-16 14:42 ` Jens Axboe
  4 siblings, 1 reply; 9+ messages in thread
From: Ming Lei @ 2021-06-14 21:55 UTC (permalink / raw)
  To: Jens Axboe, Christoph Hellwig; +Cc: linux-block, Yi Zhang, Bart Van Assche

On Wed, Jun 09, 2021 at 09:58:20AM +0800, Ming Lei wrote:
> Hello,
> 
> Yi reported several kernel panics on:
> 
> [16687.001777] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000008
> ...
> [16687.163549] pc : __rq_qos_track+0x38/0x60
> 
> or
> 
> [  997.690455] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000020
> ...
> [  997.850347] pc : __rq_qos_done+0x2c/0x50
> 
> Turns out it is caused by race between adding wbt and normal IO.
> 
> Fix the issue by freezing request queue when adding/deleting rq qos.
> 
> V3:
> 	- use ->queue_lock for protecting concurrent adding/deleting rqos on
> 	  same queue
> 
> V2:
> 	- switch to the approach of freezing queue, which is more generic
> 	  than V1.
> 
> 
> 
> Ming Lei (2):
>   block: fix race between adding/removing rq qos and normal IO
>   block: mark queue init done at the end of blk_register_queue
> 
>  block/blk-rq-qos.h | 24 ++++++++++++++++++++++++
>  block/blk-sysfs.c  | 29 +++++++++++++++--------------
>  2 files changed, 39 insertions(+), 14 deletions(-)
> 
> Cc: Yi Zhang <yi.zhang@redhat.com>
> Cc: Bart Van Assche <bvanassche@acm.org>

Hello Jens,

Any chance to merge the fixes if you are fine?

Thanks,
Ming


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

* Re: [PATCH V3 0/2] block: fix race between adding wbt and normal IO
  2021-06-14 21:55 ` Ming Lei
@ 2021-06-16 14:11   ` Ming Lei
  0 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2021-06-16 14:11 UTC (permalink / raw)
  To: Jens Axboe, Christoph Hellwig; +Cc: linux-block, Yi Zhang, Bart Van Assche

On Tue, Jun 15, 2021 at 05:55:20AM +0800, Ming Lei wrote:
> On Wed, Jun 09, 2021 at 09:58:20AM +0800, Ming Lei wrote:
> > Hello,
> > 
> > Yi reported several kernel panics on:
> > 
> > [16687.001777] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000008
> > ...
> > [16687.163549] pc : __rq_qos_track+0x38/0x60
> > 
> > or
> > 
> > [  997.690455] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000020
> > ...
> > [  997.850347] pc : __rq_qos_done+0x2c/0x50
> > 
> > Turns out it is caused by race between adding wbt and normal IO.
> > 
> > Fix the issue by freezing request queue when adding/deleting rq qos.
> > 
> > V3:
> > 	- use ->queue_lock for protecting concurrent adding/deleting rqos on
> > 	  same queue
> > 
> > V2:
> > 	- switch to the approach of freezing queue, which is more generic
> > 	  than V1.
> > 
> > 
> > 
> > Ming Lei (2):
> >   block: fix race between adding/removing rq qos and normal IO
> >   block: mark queue init done at the end of blk_register_queue
> > 
> >  block/blk-rq-qos.h | 24 ++++++++++++++++++++++++
> >  block/blk-sysfs.c  | 29 +++++++++++++++--------------
> >  2 files changed, 39 insertions(+), 14 deletions(-)
> > 
> > Cc: Yi Zhang <yi.zhang@redhat.com>
> > Cc: Bart Van Assche <bvanassche@acm.org>
> 
> Hello Jens,
> 
> Any chance to merge the fixes if you are fine?

Hi Jens,

Yi finds that the issue can be triggered in his more tests, and
has escalate it in RH BZ, can we fix the issue?


Thanks,
Ming


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

* Re: [PATCH V3 0/2] block: fix race between adding wbt and normal IO
  2021-06-09  1:58 [PATCH V3 0/2] block: fix race between adding wbt and normal IO Ming Lei
                   ` (3 preceding siblings ...)
  2021-06-14 21:55 ` Ming Lei
@ 2021-06-16 14:42 ` Jens Axboe
  4 siblings, 0 replies; 9+ messages in thread
From: Jens Axboe @ 2021-06-16 14:42 UTC (permalink / raw)
  To: Ming Lei, Christoph Hellwig; +Cc: linux-block, Yi Zhang, Bart Van Assche

On 6/8/21 7:58 PM, Ming Lei wrote:
> Hello,
> 
> Yi reported several kernel panics on:
> 
> [16687.001777] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000008
> ...
> [16687.163549] pc : __rq_qos_track+0x38/0x60
> 
> or
> 
> [  997.690455] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000020
> ...
> [  997.850347] pc : __rq_qos_done+0x2c/0x50
> 
> Turns out it is caused by race between adding wbt and normal IO.
> 
> Fix the issue by freezing request queue when adding/deleting rq qos.

Applied, thanks.

-- 
Jens Axboe


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

end of thread, other threads:[~2021-06-16 14:42 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-09  1:58 [PATCH V3 0/2] block: fix race between adding wbt and normal IO Ming Lei
2021-06-09  1:58 ` [PATCH V3 1/2] block: fix race between adding/removing rq qos " Ming Lei
2021-06-09 20:05   ` Bart Van Assche
2021-06-09  1:58 ` [PATCH V3 2/2] block: mark queue init done at the end of blk_register_queue Ming Lei
2021-06-09 20:09   ` Bart Van Assche
2021-06-10  0:45 ` [PATCH V3 0/2] block: fix race between adding wbt and normal IO Yi Zhang
2021-06-14 21:55 ` Ming Lei
2021-06-16 14:11   ` Ming Lei
2021-06-16 14:42 ` Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).