[PATCH V2 0/3] blk-mq/nvme-loop: use nvme-loop's lock class for addressing lockdep false positive warning

[PATCH V2 0/3] blk-mq/nvme-loop: use nvme-loop's lock class for addressing lockdep false positive warning

[Ming Lei]

Hi,

Qian reported there is hang during booting when shared host tagset is
introduced on megaraid sas. Sumit reported the whole SCSI probe takes
about ~45min in his test.

Turns out it is caused by nr_hw_queues increased, especially commit
b3c6a5997541("block: Fix a lockdep complaint triggered by request queue flushing")
adds synchronize_rcu() for each hctx's release handler.

Address the original lockdep false positive warning by simpler way, then
long scsi probe can be avoided with lockdep enabled.

V2:
	- add reviewed-by
	- adjust commit log of patch 3

Ming Lei (3):
  blk-mq: add new API of blk_mq_hctx_set_fq_lock_class
  nvme-loop: use blk_mq_hctx_set_fq_lock_class to set loop's lock class
  Revert "block: Fix a lockdep complaint triggered by request queue
    flushing"

 block/blk-flush.c          | 30 +++++++++++++++++++++++++-----
 block/blk.h                |  1 -
 drivers/nvme/target/loop.c | 10 ++++++++++
 include/linux/blk-mq.h     |  3 +++
 4 files changed, 38 insertions(+), 6 deletions(-)

Cc: Christoph Hellwig <hch@lst.de>
Cc: Qian Cai <cai@redhat.com>
Cc: Sumit Saxena <sumit.saxena@broadcom.com>
Cc: John Garry <john.garry@huawei.com>
Cc: Kashyap Desai <kashyap.desai@broadcom.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Hannes Reinecke <hare@suse.de>
-- 
2.28.0

[PATCH V2 1/3] blk-mq: add new API of blk_mq_hctx_set_fq_lock_class

[Ming Lei]

flush_end_io() may be called recursively from some driver, such as
nvme-loop, so lockdep may complain 'possible recursive locking'.
Commit b3c6a5997541("block: Fix a lockdep complaint triggered by
request queue flushing") tried to address this issue by assigning
dynamically allocated per-flush-queue lock class. This solution
adds synchronize_rcu() for each hctx's release handler, and causes
horrible SCSI MQ probe delay(more than half an hour on megaraid sas).

Add new API of blk_mq_hctx_set_fq_lock_class() for these drivers, so
we just need to use driver specific lock class for avoiding the
lockdep warning of 'possible recursive locking'.

Tested-by: Kashyap Desai <kashyap.desai@broadcom.com>
Reported-by: Qian Cai <cai@redhat.com>
Cc: Sumit Saxena <sumit.saxena@broadcom.com>
Cc: John Garry <john.garry@huawei.com>
Cc: Kashyap Desai <kashyap.desai@broadcom.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-flush.c      | 25 +++++++++++++++++++++++++
 include/linux/blk-mq.h |  3 +++
 2 files changed, 28 insertions(+)

diff --git a/block/blk-flush.c b/block/blk-flush.c
index 9507dcdd5881..bf51588762d8 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -490,3 +490,28 @@ void blk_free_flush_queue(struct blk_flush_queue *fq)
 	kfree(fq->flush_rq);
 	kfree(fq);
 }
+
+/*
+ * Allow driver to set its own lock class to fq->mq_flush_lock for
+ * avoiding lockdep complaint.
+ *
+ * flush_end_io() may be called recursively from some driver, such as
+ * nvme-loop, so lockdep may complain 'possible recursive locking' because
+ * all 'struct blk_flush_queue' instance share same mq_flush_lock lock class
+ * key. We need to assign different lock class for these driver's
+ * fq->mq_flush_lock for avoiding the lockdep warning.
+ *
+ * Use dynamically allocated lock class key for each 'blk_flush_queue'
+ * instance is over-kill, and more worse it introduces horrible boot delay
+ * issue because synchronize_rcu() is implied in lockdep_unregister_key which
+ * is called for each hctx release. SCSI probing may synchronously create and
+ * destroy lots of MQ request_queues for non-existent devices, and some robot
+ * test kernel always enable lockdep option. It is observed that more than half
+ * an hour is taken during SCSI MQ probe with per-fq lock class.
+ */
+void blk_mq_hctx_set_fq_lock_class(struct blk_mq_hw_ctx *hctx,
+		struct lock_class_key *key)
+{
+	lockdep_set_class(&hctx->fq->mq_flush_lock, key);
+}
+EXPORT_SYMBOL_GPL(blk_mq_hctx_set_fq_lock_class);
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 794b2a33a2c3..5f639240760e 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -5,6 +5,7 @@
 #include <linux/blkdev.h>
 #include <linux/sbitmap.h>
 #include <linux/srcu.h>
+#include <linux/lockdep.h>
 
 struct blk_mq_tags;
 struct blk_flush_queue;
@@ -594,5 +595,7 @@ static inline void blk_mq_cleanup_rq(struct request *rq)
 }
 
 blk_qc_t blk_mq_submit_bio(struct bio *bio);
+void blk_mq_hctx_set_fq_lock_class(struct blk_mq_hw_ctx *hctx,
+		struct lock_class_key *key);
 
 #endif
-- 
2.28.0

[PATCH V2 2/3] nvme-loop: use blk_mq_hctx_set_fq_lock_class to set loop's lock class

[Ming Lei]

Set nvme-loop's lock class via blk_mq_hctx_set_fq_lock_class for avoiding
lockdep possible recursive locking, then we can remove the dynamically
allocated lock class for each flush queue, finally we can avoid horrible
SCSI probe delay.

This way may not address situation in which one nvme-loop is backed on
another nvme-loop. However, in reality, people seldom uses this way
for test. Even though someone played in this way, it is just one
recursive locking false positive, no real deadlock issue.

Tested-by: Kashyap Desai <kashyap.desai@broadcom.com>
Reported-by: Qian Cai <cai@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Cc: Sumit Saxena <sumit.saxena@broadcom.com>
Cc: John Garry <john.garry@huawei.com>
Cc: Kashyap Desai <kashyap.desai@broadcom.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/nvme/target/loop.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index f6d81239be21..07806016c09d 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -211,6 +211,8 @@ static int nvme_loop_init_request(struct blk_mq_tag_set *set,
 			(set == &ctrl->tag_set) ? hctx_idx + 1 : 0);
 }
 
+static struct lock_class_key loop_hctx_fq_lock_key;
+
 static int nvme_loop_init_hctx(struct blk_mq_hw_ctx *hctx, void *data,
 		unsigned int hctx_idx)
 {
@@ -219,6 +221,14 @@ static int nvme_loop_init_hctx(struct blk_mq_hw_ctx *hctx, void *data,
 
 	BUG_ON(hctx_idx >= ctrl->ctrl.queue_count);
 
+	/*
+	 * flush_end_io() can be called recursively for us, so use our own
+	 * lock class key for avoiding lockdep possible recursive locking,
+	 * then we can remove the dynamically allocated lock class for each
+	 * flush queue, that way may cause horrible boot delay.
+	 */
+	blk_mq_hctx_set_fq_lock_class(hctx, &loop_hctx_fq_lock_key);
+
 	hctx->driver_data = queue;
 	return 0;
 }
-- 
2.28.0

[PATCH V2 3/3] Revert "block: Fix a lockdep complaint triggered by request queue flushing"

[Ming Lei]

This reverts commit b3c6a59975415bde29cfd76ff1ab008edbf614a9.

Now we can avoid nvme-loop lockdep warning of 'lockdep possible recursive locking'
by nvme-loop's lock class, no need to apply dynamically allocated lock class key,
so revert commit b3c6a5997541("block: Fix a lockdep complaint triggered by request
queue flushing").

This way fixes horrible SCSI probe delay issue on megaraid_sas, and it is reported
the whole probe may take more than half an hour.

Tested-by: Kashyap Desai <kashyap.desai@broadcom.com>
Reported-by: Qian Cai <cai@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Cc: Sumit Saxena <sumit.saxena@broadcom.com>
Cc: John Garry <john.garry@huawei.com>
Cc: Kashyap Desai <kashyap.desai@broadcom.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-flush.c | 5 -----
 block/blk.h       | 1 -
 2 files changed, 6 deletions(-)

diff --git a/block/blk-flush.c b/block/blk-flush.c
index bf51588762d8..996d5d03dade 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -69,7 +69,6 @@
 #include <linux/blkdev.h>
 #include <linux/gfp.h>
 #include <linux/blk-mq.h>
-#include <linux/lockdep.h>
 
 #include "blk.h"
 #include "blk-mq.h"
@@ -469,9 +468,6 @@ struct blk_flush_queue *blk_alloc_flush_queue(int node, int cmd_size,
 	INIT_LIST_HEAD(&fq->flush_queue[1]);
 	INIT_LIST_HEAD(&fq->flush_data_in_flight);
 
-	lockdep_register_key(&fq->key);
-	lockdep_set_class(&fq->mq_flush_lock, &fq->key);
-
 	return fq;
 
  fail_rq:
@@ -486,7 +482,6 @@ void blk_free_flush_queue(struct blk_flush_queue *fq)
 	if (!fq)
 		return;
 
-	lockdep_unregister_key(&fq->key);
 	kfree(fq->flush_rq);
 	kfree(fq);
 }
diff --git a/block/blk.h b/block/blk.h
index 98f0b1ae2641..d23d018fd2cd 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -25,7 +25,6 @@ struct blk_flush_queue {
 	struct list_head	flush_data_in_flight;
 	struct request		*flush_rq;
 
-	struct lock_class_key	key;
 	spinlock_t		mq_flush_lock;
 };
 
-- 
2.28.0

Re: [PATCH V2 1/3] blk-mq: add new API of blk_mq_hctx_set_fq_lock_class

[Hannes Reinecke]

On 12/3/20 2:26 AM, Ming Lei wrote:
> flush_end_io() may be called recursively from some driver, such as
> nvme-loop, so lockdep may complain 'possible recursive locking'.
> Commit b3c6a5997541("block: Fix a lockdep complaint triggered by
> request queue flushing") tried to address this issue by assigning
> dynamically allocated per-flush-queue lock class. This solution
> adds synchronize_rcu() for each hctx's release handler, and causes
> horrible SCSI MQ probe delay(more than half an hour on megaraid sas).
> 
> Add new API of blk_mq_hctx_set_fq_lock_class() for these drivers, so
> we just need to use driver specific lock class for avoiding the
> lockdep warning of 'possible recursive locking'.
> 
> Tested-by: Kashyap Desai <kashyap.desai@broadcom.com>
> Reported-by: Qian Cai <cai@redhat.com>
> Cc: Sumit Saxena <sumit.saxena@broadcom.com>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Kashyap Desai <kashyap.desai@broadcom.com>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/blk-flush.c      | 25 +++++++++++++++++++++++++
>   include/linux/blk-mq.h |  3 +++
>   2 files changed, 28 insertions(+)
> 
> diff --git a/block/blk-flush.c b/block/blk-flush.c
> index 9507dcdd5881..bf51588762d8 100644
> --- a/block/blk-flush.c
> +++ b/block/blk-flush.c
> @@ -490,3 +490,28 @@ void blk_free_flush_queue(struct blk_flush_queue *fq)
>   	kfree(fq->flush_rq);
>   	kfree(fq);
>   }
> +
> +/*
> + * Allow driver to set its own lock class to fq->mq_flush_lock for
> + * avoiding lockdep complaint.
> + *
> + * flush_end_io() may be called recursively from some driver, such as
> + * nvme-loop, so lockdep may complain 'possible recursive locking' because
> + * all 'struct blk_flush_queue' instance share same mq_flush_lock lock class
> + * key. We need to assign different lock class for these driver's
> + * fq->mq_flush_lock for avoiding the lockdep warning.
> + *
> + * Use dynamically allocated lock class key for each 'blk_flush_queue'
> + * instance is over-kill, and more worse it introduces horrible boot delay
> + * issue because synchronize_rcu() is implied in lockdep_unregister_key which
> + * is called for each hctx release. SCSI probing may synchronously create and
> + * destroy lots of MQ request_queues for non-existent devices, and some robot
> + * test kernel always enable lockdep option. It is observed that more than half
> + * an hour is taken during SCSI MQ probe with per-fq lock class.
> + */
> +void blk_mq_hctx_set_fq_lock_class(struct blk_mq_hw_ctx *hctx,
> +		struct lock_class_key *key)
> +{
> +	lockdep_set_class(&hctx->fq->mq_flush_lock, key);
> +}
> +EXPORT_SYMBOL_GPL(blk_mq_hctx_set_fq_lock_class);
> diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
> index 794b2a33a2c3..5f639240760e 100644
> --- a/include/linux/blk-mq.h
> +++ b/include/linux/blk-mq.h
> @@ -5,6 +5,7 @@
>   #include <linux/blkdev.h>
>   #include <linux/sbitmap.h>
>   #include <linux/srcu.h>
> +#include <linux/lockdep.h>
>   
>   struct blk_mq_tags;
>   struct blk_flush_queue;
> @@ -594,5 +595,7 @@ static inline void blk_mq_cleanup_rq(struct request *rq)
>   }
>   
>   blk_qc_t blk_mq_submit_bio(struct bio *bio);
> +void blk_mq_hctx_set_fq_lock_class(struct blk_mq_hw_ctx *hctx,
> +		struct lock_class_key *key);
>   
>   #endif
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH V2 2/3] nvme-loop: use blk_mq_hctx_set_fq_lock_class to set loop's lock class

[Hannes Reinecke]

On 12/3/20 2:26 AM, Ming Lei wrote:
> Set nvme-loop's lock class via blk_mq_hctx_set_fq_lock_class for avoiding
> lockdep possible recursive locking, then we can remove the dynamically
> allocated lock class for each flush queue, finally we can avoid horrible
> SCSI probe delay.
> 
> This way may not address situation in which one nvme-loop is backed on
> another nvme-loop. However, in reality, people seldom uses this way
> for test. Even though someone played in this way, it is just one
> recursive locking false positive, no real deadlock issue.
> 
> Tested-by: Kashyap Desai <kashyap.desai@broadcom.com>
> Reported-by: Qian Cai <cai@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Cc: Sumit Saxena <sumit.saxena@broadcom.com>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Kashyap Desai <kashyap.desai@broadcom.com>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   drivers/nvme/target/loop.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
> index f6d81239be21..07806016c09d 100644
> --- a/drivers/nvme/target/loop.c
> +++ b/drivers/nvme/target/loop.c
> @@ -211,6 +211,8 @@ static int nvme_loop_init_request(struct blk_mq_tag_set *set,
>   			(set == &ctrl->tag_set) ? hctx_idx + 1 : 0);
>   }
>   
> +static struct lock_class_key loop_hctx_fq_lock_key;
> +
>   static int nvme_loop_init_hctx(struct blk_mq_hw_ctx *hctx, void *data,
>   		unsigned int hctx_idx)
>   {
> @@ -219,6 +221,14 @@ static int nvme_loop_init_hctx(struct blk_mq_hw_ctx *hctx, void *data,
>   
>   	BUG_ON(hctx_idx >= ctrl->ctrl.queue_count);
>   
> +	/*
> +	 * flush_end_io() can be called recursively for us, so use our own
> +	 * lock class key for avoiding lockdep possible recursive locking,
> +	 * then we can remove the dynamically allocated lock class for each
> +	 * flush queue, that way may cause horrible boot delay.
> +	 */
> +	blk_mq_hctx_set_fq_lock_class(hctx, &loop_hctx_fq_lock_key);
> +
>   	hctx->driver_data = queue;
>   	return 0;
>   }
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH V2 3/3] Revert "block: Fix a lockdep complaint triggered by request queue flushing"

[Hannes Reinecke]

On 12/3/20 2:26 AM, Ming Lei wrote:
> This reverts commit b3c6a59975415bde29cfd76ff1ab008edbf614a9.
> 
> Now we can avoid nvme-loop lockdep warning of 'lockdep possible recursive locking'
> by nvme-loop's lock class, no need to apply dynamically allocated lock class key,
> so revert commit b3c6a5997541("block: Fix a lockdep complaint triggered by request
> queue flushing").
> 
> This way fixes horrible SCSI probe delay issue on megaraid_sas, and it is reported
> the whole probe may take more than half an hour.
> 
> Tested-by: Kashyap Desai <kashyap.desai@broadcom.com>
> Reported-by: Qian Cai <cai@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Cc: Sumit Saxena <sumit.saxena@broadcom.com>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Kashyap Desai <kashyap.desai@broadcom.com>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/blk-flush.c | 5 -----
>   block/blk.h       | 1 -
>   2 files changed, 6 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH V2 0/3] blk-mq/nvme-loop: use nvme-loop's lock class for addressing lockdep false positive warning

[Ming Lei]

On Thu, Dec 03, 2020 at 09:26:35AM +0800, Ming Lei wrote:
> Hi,
> 
> Qian reported there is hang during booting when shared host tagset is
> introduced on megaraid sas. Sumit reported the whole SCSI probe takes
> about ~45min in his test.
> 
> Turns out it is caused by nr_hw_queues increased, especially commit
> b3c6a5997541("block: Fix a lockdep complaint triggered by request queue flushing")
> adds synchronize_rcu() for each hctx's release handler.
> 
> Address the original lockdep false positive warning by simpler way, then
> long scsi probe can be avoided with lockdep enabled.
> 
> V2:
> 	- add reviewed-by
> 	- adjust commit log of patch 3
> 
> Ming Lei (3):
>   blk-mq: add new API of blk_mq_hctx_set_fq_lock_class
>   nvme-loop: use blk_mq_hctx_set_fq_lock_class to set loop's lock class
>   Revert "block: Fix a lockdep complaint triggered by request queue
>     flushing"
> 
>  block/blk-flush.c          | 30 +++++++++++++++++++++++++-----
>  block/blk.h                |  1 -
>  drivers/nvme/target/loop.c | 10 ++++++++++
>  include/linux/blk-mq.h     |  3 +++
>  4 files changed, 38 insertions(+), 6 deletions(-)
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Qian Cai <cai@redhat.com>
> Cc: Sumit Saxena <sumit.saxena@broadcom.com>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Kashyap Desai <kashyap.desai@broadcom.com>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Hannes Reinecke <hare@suse.de>
> -- 
> 2.28.0
> 

Hello Jens,

Ping...

-- 
Ming

Re: [PATCH V2 0/3] blk-mq/nvme-loop: use nvme-loop's lock class for addressing lockdep false positive warning

[Jens Axboe]

On 12/2/20 6:26 PM, Ming Lei wrote:
> Hi,
> 
> Qian reported there is hang during booting when shared host tagset is
> introduced on megaraid sas. Sumit reported the whole SCSI probe takes
> about ~45min in his test.
> 
> Turns out it is caused by nr_hw_queues increased, especially commit
> b3c6a5997541("block: Fix a lockdep complaint triggered by request queue flushing")
> adds synchronize_rcu() for each hctx's release handler.
> 
> Address the original lockdep false positive warning by simpler way, then
> long scsi probe can be avoided with lockdep enabled.

Applied, thanks.

-- 
Jens Axboe