[PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Ming Lei]

Hello,

dm-rq may be built on blk-mq device which marks BLK_MQ_F_BLOCKING, so
dm_mq_queue_rq() may become to sleep current context.

Fixes the issue by allowing dm-rq to set BLK_MQ_F_BLOCKING in case that
any underlying queue is marked as BLK_MQ_F_BLOCKING.

DM request queue is allocated before allocating tagset, this way is a
bit special, so we need to pre-allocate srcu payload, then use the queue
flag of QUEUE_FLAG_BLOCKING for locking dispatch.


Ming Lei (3):
  block: split having srcu from queue blocking
  block: add blk_alloc_disk_srcu
  dm: mark dm queue as blocking if any underlying is blocking

 block/blk-core.c       |  2 +-
 block/blk-mq.c         |  6 +++---
 block/blk-mq.h         |  2 +-
 block/blk-sysfs.c      |  2 +-
 block/genhd.c          |  5 +++--
 drivers/md/dm-rq.c     |  5 ++++-
 drivers/md/dm-rq.h     |  3 ++-
 drivers/md/dm-table.c  | 14 ++++++++++++++
 drivers/md/dm.c        |  5 +++--
 drivers/md/dm.h        |  1 +
 include/linux/blkdev.h |  5 +++--
 include/linux/genhd.h  | 12 ++++++++----
 12 files changed, 44 insertions(+), 18 deletions(-)

-- 
2.31.1

[dm-devel] [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Ming Lei]

Hello,

dm-rq may be built on blk-mq device which marks BLK_MQ_F_BLOCKING, so
dm_mq_queue_rq() may become to sleep current context.

Fixes the issue by allowing dm-rq to set BLK_MQ_F_BLOCKING in case that
any underlying queue is marked as BLK_MQ_F_BLOCKING.

DM request queue is allocated before allocating tagset, this way is a
bit special, so we need to pre-allocate srcu payload, then use the queue
flag of QUEUE_FLAG_BLOCKING for locking dispatch.


Ming Lei (3):
  block: split having srcu from queue blocking
  block: add blk_alloc_disk_srcu
  dm: mark dm queue as blocking if any underlying is blocking

 block/blk-core.c       |  2 +-
 block/blk-mq.c         |  6 +++---
 block/blk-mq.h         |  2 +-
 block/blk-sysfs.c      |  2 +-
 block/genhd.c          |  5 +++--
 drivers/md/dm-rq.c     |  5 ++++-
 drivers/md/dm-rq.h     |  3 ++-
 drivers/md/dm-table.c  | 14 ++++++++++++++
 drivers/md/dm.c        |  5 +++--
 drivers/md/dm.h        |  1 +
 include/linux/blkdev.h |  5 +++--
 include/linux/genhd.h  | 12 ++++++++----
 12 files changed, 44 insertions(+), 18 deletions(-)

-- 
2.31.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 1/3] block: split having srcu from queue blocking

[Ming Lei]

Now we reuse queue flag of QUEUE_FLAG_HAS_SRCU for both having srcu and
BLK_MQ_F_BLOCKING. Actually they are two things: one is that srcu is
allocated inside queue, another is that we need to handle blocking
->queue_rq. So far this way works as expected.

dm-rq needs to set BLK_MQ_F_BLOCKING if any underlying queue is
marked as BLK_MQ_F_BLOCKING. But dm queue is allocated before tagset
is allocated, one doable way is to always allocate SRCU for dm
queue, then set BLK_MQ_F_BLOCKING for the tagset if it is required,
meantime we can mark the request queue as supporting blocking
->queue_rq.

So add one new flag of QUEUE_FLAG_BLOCKING for supporting blocking
->queue_rq only, and use one private field to describe if request
queue has allocated srcu instance.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-core.c       | 2 +-
 block/blk-mq.c         | 6 +++---
 block/blk-mq.h         | 2 +-
 block/blk-sysfs.c      | 2 +-
 include/linux/blkdev.h | 5 +++--
 5 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 10619fd83c1b..7ba806a4e779 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -449,7 +449,7 @@ struct request_queue *blk_alloc_queue(int node_id, bool alloc_srcu)
 		return NULL;
 
 	if (alloc_srcu) {
-		blk_queue_flag_set(QUEUE_FLAG_HAS_SRCU, q);
+		q->has_srcu = true;
 		if (init_srcu_struct(q->srcu) != 0)
 			goto fail_q;
 	}
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 0d7c9d3e0329..1408a6b8ccdc 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -259,7 +259,7 @@ EXPORT_SYMBOL_GPL(blk_mq_quiesce_queue_nowait);
  */
 void blk_mq_wait_quiesce_done(struct request_queue *q)
 {
-	if (blk_queue_has_srcu(q))
+	if (blk_queue_blocking(q))
 		synchronize_srcu(q->srcu);
 	else
 		synchronize_rcu();
@@ -4024,8 +4024,8 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
 int blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
 		struct request_queue *q)
 {
-	WARN_ON_ONCE(blk_queue_has_srcu(q) !=
-			!!(set->flags & BLK_MQ_F_BLOCKING));
+	if (set->flags & BLK_MQ_F_BLOCKING)
+		blk_queue_flag_set(QUEUE_FLAG_BLOCKING, q);
 
 	/* mark the queue as mq asap */
 	q->mq_ops = set->ops;
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 948791ea2a3e..9601918e2034 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -377,7 +377,7 @@ static inline bool hctx_may_queue(struct blk_mq_hw_ctx *hctx,
 /* run the code block in @dispatch_ops with rcu/srcu read lock held */
 #define __blk_mq_run_dispatch_ops(q, check_sleep, dispatch_ops)	\
 do {								\
-	if (!blk_queue_has_srcu(q)) {				\
+	if (!blk_queue_blocking(q)) {				\
 		rcu_read_lock();				\
 		(dispatch_ops);					\
 		rcu_read_unlock();				\
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index e20eadfcf5c8..af89fabb58e3 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -736,7 +736,7 @@ static void blk_free_queue_rcu(struct rcu_head *rcu_head)
 	struct request_queue *q = container_of(rcu_head, struct request_queue,
 					       rcu_head);
 
-	kmem_cache_free(blk_get_queue_kmem_cache(blk_queue_has_srcu(q)), q);
+	kmem_cache_free(blk_get_queue_kmem_cache(q->has_srcu), q);
 }
 
 /* Unconfigure the I/O scheduler and dissociate from the cgroup controller. */
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index c80cfaefc0a8..d84abdb294c4 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -365,6 +365,7 @@ struct request_queue {
 #endif
 
 	bool			mq_sysfs_init_done;
+	bool			has_srcu;
 
 #define BLK_MAX_WRITE_HINTS	5
 	u64			write_hints[BLK_MAX_WRITE_HINTS];
@@ -385,7 +386,7 @@ struct request_queue {
 /* Keep blk_queue_flag_name[] in sync with the definitions below */
 #define QUEUE_FLAG_STOPPED	0	/* queue is stopped */
 #define QUEUE_FLAG_DYING	1	/* queue being torn down */
-#define QUEUE_FLAG_HAS_SRCU	2	/* SRCU is allocated */
+#define QUEUE_FLAG_BLOCKING	2	/* ->queue_rq may block */
 #define QUEUE_FLAG_NOMERGES     3	/* disable merge attempts */
 #define QUEUE_FLAG_SAME_COMP	4	/* complete on same CPU-group */
 #define QUEUE_FLAG_FAIL_IO	5	/* fake timeout */
@@ -423,7 +424,7 @@ bool blk_queue_flag_test_and_set(unsigned int flag, struct request_queue *q);
 
 #define blk_queue_stopped(q)	test_bit(QUEUE_FLAG_STOPPED, &(q)->queue_flags)
 #define blk_queue_dying(q)	test_bit(QUEUE_FLAG_DYING, &(q)->queue_flags)
-#define blk_queue_has_srcu(q)	test_bit(QUEUE_FLAG_HAS_SRCU, &(q)->queue_flags)
+#define blk_queue_blocking(q)	test_bit(QUEUE_FLAG_BLOCKING, &(q)->queue_flags)
 #define blk_queue_dead(q)	test_bit(QUEUE_FLAG_DEAD, &(q)->queue_flags)
 #define blk_queue_init_done(q)	test_bit(QUEUE_FLAG_INIT_DONE, &(q)->queue_flags)
 #define blk_queue_nomerges(q)	test_bit(QUEUE_FLAG_NOMERGES, &(q)->queue_flags)
-- 
2.31.1

[dm-devel] [PATCH 1/3] block: split having srcu from queue blocking

[Ming Lei]

Now we reuse queue flag of QUEUE_FLAG_HAS_SRCU for both having srcu and
BLK_MQ_F_BLOCKING. Actually they are two things: one is that srcu is
allocated inside queue, another is that we need to handle blocking
->queue_rq. So far this way works as expected.

dm-rq needs to set BLK_MQ_F_BLOCKING if any underlying queue is
marked as BLK_MQ_F_BLOCKING. But dm queue is allocated before tagset
is allocated, one doable way is to always allocate SRCU for dm
queue, then set BLK_MQ_F_BLOCKING for the tagset if it is required,
meantime we can mark the request queue as supporting blocking
->queue_rq.

So add one new flag of QUEUE_FLAG_BLOCKING for supporting blocking
->queue_rq only, and use one private field to describe if request
queue has allocated srcu instance.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-core.c       | 2 +-
 block/blk-mq.c         | 6 +++---
 block/blk-mq.h         | 2 +-
 block/blk-sysfs.c      | 2 +-
 include/linux/blkdev.h | 5 +++--
 5 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 10619fd83c1b..7ba806a4e779 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -449,7 +449,7 @@ struct request_queue *blk_alloc_queue(int node_id, bool alloc_srcu)
 		return NULL;
 
 	if (alloc_srcu) {
-		blk_queue_flag_set(QUEUE_FLAG_HAS_SRCU, q);
+		q->has_srcu = true;
 		if (init_srcu_struct(q->srcu) != 0)
 			goto fail_q;
 	}
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 0d7c9d3e0329..1408a6b8ccdc 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -259,7 +259,7 @@ EXPORT_SYMBOL_GPL(blk_mq_quiesce_queue_nowait);
  */
 void blk_mq_wait_quiesce_done(struct request_queue *q)
 {
-	if (blk_queue_has_srcu(q))
+	if (blk_queue_blocking(q))
 		synchronize_srcu(q->srcu);
 	else
 		synchronize_rcu();
@@ -4024,8 +4024,8 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
 int blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
 		struct request_queue *q)
 {
-	WARN_ON_ONCE(blk_queue_has_srcu(q) !=
-			!!(set->flags & BLK_MQ_F_BLOCKING));
+	if (set->flags & BLK_MQ_F_BLOCKING)
+		blk_queue_flag_set(QUEUE_FLAG_BLOCKING, q);
 
 	/* mark the queue as mq asap */
 	q->mq_ops = set->ops;
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 948791ea2a3e..9601918e2034 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -377,7 +377,7 @@ static inline bool hctx_may_queue(struct blk_mq_hw_ctx *hctx,
 /* run the code block in @dispatch_ops with rcu/srcu read lock held */
 #define __blk_mq_run_dispatch_ops(q, check_sleep, dispatch_ops)	\
 do {								\
-	if (!blk_queue_has_srcu(q)) {				\
+	if (!blk_queue_blocking(q)) {				\
 		rcu_read_lock();				\
 		(dispatch_ops);					\
 		rcu_read_unlock();				\
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index e20eadfcf5c8..af89fabb58e3 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -736,7 +736,7 @@ static void blk_free_queue_rcu(struct rcu_head *rcu_head)
 	struct request_queue *q = container_of(rcu_head, struct request_queue,
 					       rcu_head);
 
-	kmem_cache_free(blk_get_queue_kmem_cache(blk_queue_has_srcu(q)), q);
+	kmem_cache_free(blk_get_queue_kmem_cache(q->has_srcu), q);
 }
 
 /* Unconfigure the I/O scheduler and dissociate from the cgroup controller. */
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index c80cfaefc0a8..d84abdb294c4 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -365,6 +365,7 @@ struct request_queue {
 #endif
 
 	bool			mq_sysfs_init_done;
+	bool			has_srcu;
 
 #define BLK_MAX_WRITE_HINTS	5
 	u64			write_hints[BLK_MAX_WRITE_HINTS];
@@ -385,7 +386,7 @@ struct request_queue {
 /* Keep blk_queue_flag_name[] in sync with the definitions below */
 #define QUEUE_FLAG_STOPPED	0	/* queue is stopped */
 #define QUEUE_FLAG_DYING	1	/* queue being torn down */
-#define QUEUE_FLAG_HAS_SRCU	2	/* SRCU is allocated */
+#define QUEUE_FLAG_BLOCKING	2	/* ->queue_rq may block */
 #define QUEUE_FLAG_NOMERGES     3	/* disable merge attempts */
 #define QUEUE_FLAG_SAME_COMP	4	/* complete on same CPU-group */
 #define QUEUE_FLAG_FAIL_IO	5	/* fake timeout */
@@ -423,7 +424,7 @@ bool blk_queue_flag_test_and_set(unsigned int flag, struct request_queue *q);
 
 #define blk_queue_stopped(q)	test_bit(QUEUE_FLAG_STOPPED, &(q)->queue_flags)
 #define blk_queue_dying(q)	test_bit(QUEUE_FLAG_DYING, &(q)->queue_flags)
-#define blk_queue_has_srcu(q)	test_bit(QUEUE_FLAG_HAS_SRCU, &(q)->queue_flags)
+#define blk_queue_blocking(q)	test_bit(QUEUE_FLAG_BLOCKING, &(q)->queue_flags)
 #define blk_queue_dead(q)	test_bit(QUEUE_FLAG_DEAD, &(q)->queue_flags)
 #define blk_queue_init_done(q)	test_bit(QUEUE_FLAG_INIT_DONE, &(q)->queue_flags)
 #define blk_queue_nomerges(q)	test_bit(QUEUE_FLAG_NOMERGES, &(q)->queue_flags)
-- 
2.31.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 2/3] block: add blk_alloc_disk_srcu

[Ming Lei]

Add blk_alloc_disk_srcu() so that we can allocate srcu inside request queue
for supporting blocking ->queue_rq().

dm-rq needs this API.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/genhd.c         |  5 +++--
 include/linux/genhd.h | 12 ++++++++----
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 3c139a1b6f04..d21786fbb7bb 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1333,12 +1333,13 @@ struct gendisk *__alloc_disk_node(struct request_queue *q, int node_id,
 }
 EXPORT_SYMBOL(__alloc_disk_node);
 
-struct gendisk *__blk_alloc_disk(int node, struct lock_class_key *lkclass)
+struct gendisk *__blk_alloc_disk(int node, bool alloc_srcu,
+		struct lock_class_key *lkclass)
 {
 	struct request_queue *q;
 	struct gendisk *disk;
 
-	q = blk_alloc_queue(node, false);
+	q = blk_alloc_queue(node, alloc_srcu);
 	if (!q)
 		return NULL;
 
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 6906a45bc761..20259340b962 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -227,23 +227,27 @@ void blk_drop_partitions(struct gendisk *disk);
 struct gendisk *__alloc_disk_node(struct request_queue *q, int node_id,
 		struct lock_class_key *lkclass);
 extern void put_disk(struct gendisk *disk);
-struct gendisk *__blk_alloc_disk(int node, struct lock_class_key *lkclass);
+struct gendisk *__blk_alloc_disk(int node, bool alloc_srcu,
+		struct lock_class_key *lkclass);
 
 /**
- * blk_alloc_disk - allocate a gendisk structure
+ * __alloc_disk - allocate a gendisk structure
  * @node_id: numa node to allocate on
+ * @alloc_srcu: allocate srcu instance for supporting blocking ->queue_rq
  *
  * Allocate and pre-initialize a gendisk structure for use with BIO based
  * drivers.
  *
  * Context: can sleep
  */
-#define blk_alloc_disk(node_id)						\
+#define __alloc_disk(node_id, alloc_srcu)				\
 ({									\
 	static struct lock_class_key __key;				\
 									\
-	__blk_alloc_disk(node_id, &__key);				\
+	__blk_alloc_disk(node_id, alloc_srcu, &__key);			\
 })
+#define blk_alloc_disk(node_id) __alloc_disk(node_id, false)
+#define blk_alloc_disk_srcu(node_id) __alloc_disk(node_id, true)
 void blk_cleanup_disk(struct gendisk *disk);
 
 int __register_blkdev(unsigned int major, const char *name,
-- 
2.31.1

[dm-devel] [PATCH 2/3] block: add blk_alloc_disk_srcu

[Ming Lei]

Add blk_alloc_disk_srcu() so that we can allocate srcu inside request queue
for supporting blocking ->queue_rq().

dm-rq needs this API.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/genhd.c         |  5 +++--
 include/linux/genhd.h | 12 ++++++++----
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 3c139a1b6f04..d21786fbb7bb 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1333,12 +1333,13 @@ struct gendisk *__alloc_disk_node(struct request_queue *q, int node_id,
 }
 EXPORT_SYMBOL(__alloc_disk_node);
 
-struct gendisk *__blk_alloc_disk(int node, struct lock_class_key *lkclass)
+struct gendisk *__blk_alloc_disk(int node, bool alloc_srcu,
+		struct lock_class_key *lkclass)
 {
 	struct request_queue *q;
 	struct gendisk *disk;
 
-	q = blk_alloc_queue(node, false);
+	q = blk_alloc_queue(node, alloc_srcu);
 	if (!q)
 		return NULL;
 
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 6906a45bc761..20259340b962 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -227,23 +227,27 @@ void blk_drop_partitions(struct gendisk *disk);
 struct gendisk *__alloc_disk_node(struct request_queue *q, int node_id,
 		struct lock_class_key *lkclass);
 extern void put_disk(struct gendisk *disk);
-struct gendisk *__blk_alloc_disk(int node, struct lock_class_key *lkclass);
+struct gendisk *__blk_alloc_disk(int node, bool alloc_srcu,
+		struct lock_class_key *lkclass);
 
 /**
- * blk_alloc_disk - allocate a gendisk structure
+ * __alloc_disk - allocate a gendisk structure
  * @node_id: numa node to allocate on
+ * @alloc_srcu: allocate srcu instance for supporting blocking ->queue_rq
  *
  * Allocate and pre-initialize a gendisk structure for use with BIO based
  * drivers.
  *
  * Context: can sleep
  */
-#define blk_alloc_disk(node_id)						\
+#define __alloc_disk(node_id, alloc_srcu)				\
 ({									\
 	static struct lock_class_key __key;				\
 									\
-	__blk_alloc_disk(node_id, &__key);				\
+	__blk_alloc_disk(node_id, alloc_srcu, &__key);			\
 })
+#define blk_alloc_disk(node_id) __alloc_disk(node_id, false)
+#define blk_alloc_disk_srcu(node_id) __alloc_disk(node_id, true)
 void blk_cleanup_disk(struct gendisk *disk);
 
 int __register_blkdev(unsigned int major, const char *name,
-- 
2.31.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 3/3] dm: mark dm queue as blocking if any underlying is blocking

[Ming Lei]

dm request based driver doesn't set BLK_MQ_F_BLOCKING, so dm_queue_rq()
is supposed to not sleep.

However, blk_insert_cloned_request() is used by dm_queue_rq() for
queuing underlying request, but the underlying queue may be marked as
BLK_MQ_F_BLOCKING, so blk_insert_cloned_request() may become to block
current context, then rcu warning is triggered.

Fixes the issue by marking dm request based queue as BLK_MQ_F_BLOCKING
if any underlying queue is marked as BLK_MQ_F_BLOCKING, meantime we
need to allocate srcu beforehand.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/md/dm-rq.c    |  5 ++++-
 drivers/md/dm-rq.h    |  3 ++-
 drivers/md/dm-table.c | 14 ++++++++++++++
 drivers/md/dm.c       |  5 +++--
 drivers/md/dm.h       |  1 +
 5 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
index 579ab6183d4d..2297d37c62a9 100644
--- a/drivers/md/dm-rq.c
+++ b/drivers/md/dm-rq.c
@@ -535,7 +535,8 @@ static const struct blk_mq_ops dm_mq_ops = {
 	.init_request = dm_mq_init_request,
 };
 
-int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
+int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t,
+			     bool blocking)
 {
 	struct dm_target *immutable_tgt;
 	int err;
@@ -550,6 +551,8 @@ int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
 	md->tag_set->flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_STACKING;
 	md->tag_set->nr_hw_queues = dm_get_blk_mq_nr_hw_queues();
 	md->tag_set->driver_data = md;
+	if (blocking)
+		md->tag_set->flags |= BLK_MQ_F_BLOCKING;
 
 	md->tag_set->cmd_size = sizeof(struct dm_rq_target_io);
 	immutable_tgt = dm_table_get_immutable_target(t);
diff --git a/drivers/md/dm-rq.h b/drivers/md/dm-rq.h
index 1eea0da641db..5f3729f277d7 100644
--- a/drivers/md/dm-rq.h
+++ b/drivers/md/dm-rq.h
@@ -30,7 +30,8 @@ struct dm_rq_clone_bio_info {
 	struct bio clone;
 };
 
-int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t);
+int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t,
+			     bool blocking);
 void dm_mq_cleanup_mapped_device(struct mapped_device *md);
 
 void dm_start_queue(struct request_queue *q);
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index aa173f5bdc3d..e4bdd4f757a3 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1875,6 +1875,20 @@ static bool dm_table_supports_write_zeroes(struct dm_table *t)
 	return true;
 }
 
+/* If the device can block inside ->queue_rq */
+static int device_is_io_blocking(struct dm_target *ti, struct dm_dev *dev,
+			      sector_t start, sector_t len, void *data)
+{
+	struct request_queue *q = bdev_get_queue(dev->bdev);
+
+	return blk_queue_blocking(q);
+}
+
+bool dm_table_has_blocking_dev(struct dm_table *t)
+{
+	return dm_table_any_dev_attr(t, device_is_io_blocking, NULL);
+}
+
 static int device_not_nowait_capable(struct dm_target *ti, struct dm_dev *dev,
 				     sector_t start, sector_t len, void *data)
 {
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 280918cdcabd..2f72877752dd 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1761,7 +1761,7 @@ static struct mapped_device *alloc_dev(int minor)
 	 * established. If request-based table is loaded: blk-mq will
 	 * override accordingly.
 	 */
-	md->disk = blk_alloc_disk(md->numa_node_id);
+	md->disk = blk_alloc_disk_srcu(md->numa_node_id);
 	if (!md->disk)
 		goto bad;
 	md->queue = md->disk->queue;
@@ -2046,7 +2046,8 @@ int dm_setup_md_queue(struct mapped_device *md, struct dm_table *t)
 	switch (type) {
 	case DM_TYPE_REQUEST_BASED:
 		md->disk->fops = &dm_rq_blk_dops;
-		r = dm_mq_init_request_queue(md, t);
+		r = dm_mq_init_request_queue(md, t,
+				dm_table_has_blocking_dev(t));
 		if (r) {
 			DMERR("Cannot initialize queue for request-based dm mapped device");
 			return r;
diff --git a/drivers/md/dm.h b/drivers/md/dm.h
index 742d9c80efe1..f7f92b272cce 100644
--- a/drivers/md/dm.h
+++ b/drivers/md/dm.h
@@ -60,6 +60,7 @@ int dm_calculate_queue_limits(struct dm_table *table,
 			      struct queue_limits *limits);
 int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 			      struct queue_limits *limits);
+bool dm_table_has_blocking_dev(struct dm_table *t);
 struct list_head *dm_table_get_devices(struct dm_table *t);
 void dm_table_presuspend_targets(struct dm_table *t);
 void dm_table_presuspend_undo_targets(struct dm_table *t);
-- 
2.31.1

[dm-devel] [PATCH 3/3] dm: mark dm queue as blocking if any underlying is blocking

[Ming Lei]

dm request based driver doesn't set BLK_MQ_F_BLOCKING, so dm_queue_rq()
is supposed to not sleep.

However, blk_insert_cloned_request() is used by dm_queue_rq() for
queuing underlying request, but the underlying queue may be marked as
BLK_MQ_F_BLOCKING, so blk_insert_cloned_request() may become to block
current context, then rcu warning is triggered.

Fixes the issue by marking dm request based queue as BLK_MQ_F_BLOCKING
if any underlying queue is marked as BLK_MQ_F_BLOCKING, meantime we
need to allocate srcu beforehand.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/md/dm-rq.c    |  5 ++++-
 drivers/md/dm-rq.h    |  3 ++-
 drivers/md/dm-table.c | 14 ++++++++++++++
 drivers/md/dm.c       |  5 +++--
 drivers/md/dm.h       |  1 +
 5 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
index 579ab6183d4d..2297d37c62a9 100644
--- a/drivers/md/dm-rq.c
+++ b/drivers/md/dm-rq.c
@@ -535,7 +535,8 @@ static const struct blk_mq_ops dm_mq_ops = {
 	.init_request = dm_mq_init_request,
 };
 
-int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
+int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t,
+			     bool blocking)
 {
 	struct dm_target *immutable_tgt;
 	int err;
@@ -550,6 +551,8 @@ int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
 	md->tag_set->flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_STACKING;
 	md->tag_set->nr_hw_queues = dm_get_blk_mq_nr_hw_queues();
 	md->tag_set->driver_data = md;
+	if (blocking)
+		md->tag_set->flags |= BLK_MQ_F_BLOCKING;
 
 	md->tag_set->cmd_size = sizeof(struct dm_rq_target_io);
 	immutable_tgt = dm_table_get_immutable_target(t);
diff --git a/drivers/md/dm-rq.h b/drivers/md/dm-rq.h
index 1eea0da641db..5f3729f277d7 100644
--- a/drivers/md/dm-rq.h
+++ b/drivers/md/dm-rq.h
@@ -30,7 +30,8 @@ struct dm_rq_clone_bio_info {
 	struct bio clone;
 };
 
-int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t);
+int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t,
+			     bool blocking);
 void dm_mq_cleanup_mapped_device(struct mapped_device *md);
 
 void dm_start_queue(struct request_queue *q);
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index aa173f5bdc3d..e4bdd4f757a3 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1875,6 +1875,20 @@ static bool dm_table_supports_write_zeroes(struct dm_table *t)
 	return true;
 }
 
+/* If the device can block inside ->queue_rq */
+static int device_is_io_blocking(struct dm_target *ti, struct dm_dev *dev,
+			      sector_t start, sector_t len, void *data)
+{
+	struct request_queue *q = bdev_get_queue(dev->bdev);
+
+	return blk_queue_blocking(q);
+}
+
+bool dm_table_has_blocking_dev(struct dm_table *t)
+{
+	return dm_table_any_dev_attr(t, device_is_io_blocking, NULL);
+}
+
 static int device_not_nowait_capable(struct dm_target *ti, struct dm_dev *dev,
 				     sector_t start, sector_t len, void *data)
 {
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 280918cdcabd..2f72877752dd 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1761,7 +1761,7 @@ static struct mapped_device *alloc_dev(int minor)
 	 * established. If request-based table is loaded: blk-mq will
 	 * override accordingly.
 	 */
-	md->disk = blk_alloc_disk(md->numa_node_id);
+	md->disk = blk_alloc_disk_srcu(md->numa_node_id);
 	if (!md->disk)
 		goto bad;
 	md->queue = md->disk->queue;
@@ -2046,7 +2046,8 @@ int dm_setup_md_queue(struct mapped_device *md, struct dm_table *t)
 	switch (type) {
 	case DM_TYPE_REQUEST_BASED:
 		md->disk->fops = &dm_rq_blk_dops;
-		r = dm_mq_init_request_queue(md, t);
+		r = dm_mq_init_request_queue(md, t,
+				dm_table_has_blocking_dev(t));
 		if (r) {
 			DMERR("Cannot initialize queue for request-based dm mapped device");
 			return r;
diff --git a/drivers/md/dm.h b/drivers/md/dm.h
index 742d9c80efe1..f7f92b272cce 100644
--- a/drivers/md/dm.h
+++ b/drivers/md/dm.h
@@ -60,6 +60,7 @@ int dm_calculate_queue_limits(struct dm_table *table,
 			      struct queue_limits *limits);
 int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 			      struct queue_limits *limits);
+bool dm_table_has_blocking_dev(struct dm_table *t);
 struct list_head *dm_table_get_devices(struct dm_table *t);
 void dm_table_presuspend_targets(struct dm_table *t);
 void dm_table_presuspend_undo_targets(struct dm_table *t);
-- 
2.31.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Christoph Hellwig]

On Tue, Dec 21, 2021 at 10:14:56PM +0800, Ming Lei wrote:
> Hello,
> 
> dm-rq may be built on blk-mq device which marks BLK_MQ_F_BLOCKING, so
> dm_mq_queue_rq() may become to sleep current context.
> 
> Fixes the issue by allowing dm-rq to set BLK_MQ_F_BLOCKING in case that
> any underlying queue is marked as BLK_MQ_F_BLOCKING.
> 
> DM request queue is allocated before allocating tagset, this way is a
> bit special, so we need to pre-allocate srcu payload, then use the queue
> flag of QUEUE_FLAG_BLOCKING for locking dispatch.

What is the benefit over just forcing bio-based dm-mpath for these
devices?

Re: [dm-devel] [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Christoph Hellwig]

On Tue, Dec 21, 2021 at 10:14:56PM +0800, Ming Lei wrote:
> Hello,
> 
> dm-rq may be built on blk-mq device which marks BLK_MQ_F_BLOCKING, so
> dm_mq_queue_rq() may become to sleep current context.
> 
> Fixes the issue by allowing dm-rq to set BLK_MQ_F_BLOCKING in case that
> any underlying queue is marked as BLK_MQ_F_BLOCKING.
> 
> DM request queue is allocated before allocating tagset, this way is a
> bit special, so we need to pre-allocate srcu payload, then use the queue
> flag of QUEUE_FLAG_BLOCKING for locking dispatch.

What is the benefit over just forcing bio-based dm-mpath for these
devices?

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Ming Lei]

On Tue, Dec 21, 2021 at 08:21:39AM -0800, Christoph Hellwig wrote:
> On Tue, Dec 21, 2021 at 10:14:56PM +0800, Ming Lei wrote:
> > Hello,
> > 
> > dm-rq may be built on blk-mq device which marks BLK_MQ_F_BLOCKING, so
> > dm_mq_queue_rq() may become to sleep current context.
> > 
> > Fixes the issue by allowing dm-rq to set BLK_MQ_F_BLOCKING in case that
> > any underlying queue is marked as BLK_MQ_F_BLOCKING.
> > 
> > DM request queue is allocated before allocating tagset, this way is a
> > bit special, so we need to pre-allocate srcu payload, then use the queue
> > flag of QUEUE_FLAG_BLOCKING for locking dispatch.
> 
> What is the benefit over just forcing bio-based dm-mpath for these
> devices?

At least IO scheduler can't be used for bio based dm-mpath, also there should
be other drawbacks for bio based mpath and request mpath is often the default
option, maybe Mike has more input about bio vs request dm-mpath.



Thanks,
Ming

Re: [dm-devel] [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Ming Lei]

On Tue, Dec 21, 2021 at 08:21:39AM -0800, Christoph Hellwig wrote:
> On Tue, Dec 21, 2021 at 10:14:56PM +0800, Ming Lei wrote:
> > Hello,
> > 
> > dm-rq may be built on blk-mq device which marks BLK_MQ_F_BLOCKING, so
> > dm_mq_queue_rq() may become to sleep current context.
> > 
> > Fixes the issue by allowing dm-rq to set BLK_MQ_F_BLOCKING in case that
> > any underlying queue is marked as BLK_MQ_F_BLOCKING.
> > 
> > DM request queue is allocated before allocating tagset, this way is a
> > bit special, so we need to pre-allocate srcu payload, then use the queue
> > flag of QUEUE_FLAG_BLOCKING for locking dispatch.
> 
> What is the benefit over just forcing bio-based dm-mpath for these
> devices?

At least IO scheduler can't be used for bio based dm-mpath, also there should
be other drawbacks for bio based mpath and request mpath is often the default
option, maybe Mike has more input about bio vs request dm-mpath.



Thanks,
Ming

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Mike Snitzer]

On Wed, Dec 22 2021 at 11:16P -0500,
Ming Lei <ming.lei@redhat.com> wrote:

> On Tue, Dec 21, 2021 at 08:21:39AM -0800, Christoph Hellwig wrote:
> > On Tue, Dec 21, 2021 at 10:14:56PM +0800, Ming Lei wrote:
> > > Hello,
> > > 
> > > dm-rq may be built on blk-mq device which marks BLK_MQ_F_BLOCKING, so
> > > dm_mq_queue_rq() may become to sleep current context.
> > > 
> > > Fixes the issue by allowing dm-rq to set BLK_MQ_F_BLOCKING in case that
> > > any underlying queue is marked as BLK_MQ_F_BLOCKING.
> > > 
> > > DM request queue is allocated before allocating tagset, this way is a
> > > bit special, so we need to pre-allocate srcu payload, then use the queue
> > > flag of QUEUE_FLAG_BLOCKING for locking dispatch.
> > 
> > What is the benefit over just forcing bio-based dm-mpath for these
> > devices?
> 
> At least IO scheduler can't be used for bio based dm-mpath, also there should
> be other drawbacks for bio based mpath and request mpath is often the default
> option, maybe Mike has more input about bio vs request dm-mpath.

Yeah, people use request-based for IO scheduling and more capable path
selectors. Imposing bio-based would be a pretty jarring workaround for 
BLK_MQ_F_BLOCKING. request-based DM should properly support it.

I'm on vacation for the next week but will have a look at this
patchset once I'm back.

Thanks,
Mike

Re: [dm-devel] [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Mike Snitzer]

On Wed, Dec 22 2021 at 11:16P -0500,
Ming Lei <ming.lei@redhat.com> wrote:

> On Tue, Dec 21, 2021 at 08:21:39AM -0800, Christoph Hellwig wrote:
> > On Tue, Dec 21, 2021 at 10:14:56PM +0800, Ming Lei wrote:
> > > Hello,
> > > 
> > > dm-rq may be built on blk-mq device which marks BLK_MQ_F_BLOCKING, so
> > > dm_mq_queue_rq() may become to sleep current context.
> > > 
> > > Fixes the issue by allowing dm-rq to set BLK_MQ_F_BLOCKING in case that
> > > any underlying queue is marked as BLK_MQ_F_BLOCKING.
> > > 
> > > DM request queue is allocated before allocating tagset, this way is a
> > > bit special, so we need to pre-allocate srcu payload, then use the queue
> > > flag of QUEUE_FLAG_BLOCKING for locking dispatch.
> > 
> > What is the benefit over just forcing bio-based dm-mpath for these
> > devices?
> 
> At least IO scheduler can't be used for bio based dm-mpath, also there should
> be other drawbacks for bio based mpath and request mpath is often the default
> option, maybe Mike has more input about bio vs request dm-mpath.

Yeah, people use request-based for IO scheduling and more capable path
selectors. Imposing bio-based would be a pretty jarring workaround for 
BLK_MQ_F_BLOCKING. request-based DM should properly support it.

I'm on vacation for the next week but will have a look at this
patchset once I'm back.

Thanks,
Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 3/3] dm: mark dm queue as blocking if any underlying is blocking

[Mike Snitzer]

On Tue, Dec 21 2021 at  9:14P -0500,
Ming Lei <ming.lei@redhat.com> wrote:

> dm request based driver doesn't set BLK_MQ_F_BLOCKING, so dm_queue_rq()
> is supposed to not sleep.
> 
> However, blk_insert_cloned_request() is used by dm_queue_rq() for
> queuing underlying request, but the underlying queue may be marked as
> BLK_MQ_F_BLOCKING, so blk_insert_cloned_request() may become to block
> current context, then rcu warning is triggered.
> 
> Fixes the issue by marking dm request based queue as BLK_MQ_F_BLOCKING
> if any underlying queue is marked as BLK_MQ_F_BLOCKING, meantime we
> need to allocate srcu beforehand.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  drivers/md/dm-rq.c    |  5 ++++-
>  drivers/md/dm-rq.h    |  3 ++-
>  drivers/md/dm-table.c | 14 ++++++++++++++
>  drivers/md/dm.c       |  5 +++--
>  drivers/md/dm.h       |  1 +
>  5 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> index 579ab6183d4d..2297d37c62a9 100644
> --- a/drivers/md/dm-rq.c
> +++ b/drivers/md/dm-rq.c
> @@ -535,7 +535,8 @@ static const struct blk_mq_ops dm_mq_ops = {
>  	.init_request = dm_mq_init_request,
>  };
>  
> -int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
> +int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t,
> +			     bool blocking)
>  {
>  	struct dm_target *immutable_tgt;
>  	int err;
> @@ -550,6 +551,8 @@ int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
>  	md->tag_set->flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_STACKING;
>  	md->tag_set->nr_hw_queues = dm_get_blk_mq_nr_hw_queues();
>  	md->tag_set->driver_data = md;
> +	if (blocking)
> +		md->tag_set->flags |= BLK_MQ_F_BLOCKING;
>  
>  	md->tag_set->cmd_size = sizeof(struct dm_rq_target_io);
>  	immutable_tgt = dm_table_get_immutable_target(t);

As you can see, dm_table_get_immutable_target(t) is called here ^

Rather than pass 'blocking' in, please just call dm_table_has_blocking_dev(t);

But not a big deal, I can clean that up once this gets committed...

> diff --git a/drivers/md/dm-rq.h b/drivers/md/dm-rq.h
> index 1eea0da641db..5f3729f277d7 100644
> --- a/drivers/md/dm-rq.h
> +++ b/drivers/md/dm-rq.h
> @@ -30,7 +30,8 @@ struct dm_rq_clone_bio_info {
>  	struct bio clone;
>  };
>  
> -int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t);
> +int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t,
> +			     bool blocking);
>  void dm_mq_cleanup_mapped_device(struct mapped_device *md);
>  
>  void dm_start_queue(struct request_queue *q);
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index aa173f5bdc3d..e4bdd4f757a3 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -1875,6 +1875,20 @@ static bool dm_table_supports_write_zeroes(struct dm_table *t)
>  	return true;
>  }
>  
> +/* If the device can block inside ->queue_rq */
> +static int device_is_io_blocking(struct dm_target *ti, struct dm_dev *dev,
> +			      sector_t start, sector_t len, void *data)
> +{
> +	struct request_queue *q = bdev_get_queue(dev->bdev);
> +
> +	return blk_queue_blocking(q);
> +}
> +
> +bool dm_table_has_blocking_dev(struct dm_table *t)
> +{
> +	return dm_table_any_dev_attr(t, device_is_io_blocking, NULL);
> +}
> +
>  static int device_not_nowait_capable(struct dm_target *ti, struct dm_dev *dev,
>  				     sector_t start, sector_t len, void *data)
>  {
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 280918cdcabd..2f72877752dd 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1761,7 +1761,7 @@ static struct mapped_device *alloc_dev(int minor)
>  	 * established. If request-based table is loaded: blk-mq will
>  	 * override accordingly.
>  	 */
> -	md->disk = blk_alloc_disk(md->numa_node_id);
> +	md->disk = blk_alloc_disk_srcu(md->numa_node_id);
>  	if (!md->disk)
>  		goto bad;
>  	md->queue = md->disk->queue;
> @@ -2046,7 +2046,8 @@ int dm_setup_md_queue(struct mapped_device *md, struct dm_table *t)
>  	switch (type) {
>  	case DM_TYPE_REQUEST_BASED:
>  		md->disk->fops = &dm_rq_blk_dops;
> -		r = dm_mq_init_request_queue(md, t);
> +		r = dm_mq_init_request_queue(md, t,
> +				dm_table_has_blocking_dev(t));
>  		if (r) {
>  			DMERR("Cannot initialize queue for request-based dm mapped device");
>  			return r;
> diff --git a/drivers/md/dm.h b/drivers/md/dm.h
> index 742d9c80efe1..f7f92b272cce 100644
> --- a/drivers/md/dm.h
> +++ b/drivers/md/dm.h
> @@ -60,6 +60,7 @@ int dm_calculate_queue_limits(struct dm_table *table,
>  			      struct queue_limits *limits);
>  int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
>  			      struct queue_limits *limits);
> +bool dm_table_has_blocking_dev(struct dm_table *t);
>  struct list_head *dm_table_get_devices(struct dm_table *t);
>  void dm_table_presuspend_targets(struct dm_table *t);
>  void dm_table_presuspend_undo_targets(struct dm_table *t);
> -- 
> 2.31.1
> 

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

Late, given holidays we know why, but this patchset is needed for 5.17
(maybe with added: 'Fixes: 704b914f15fb7 "blk-mq: move srcu from
blk_mq_hw_ctx to request_queue"' to this 3rd patch?)

Jens, can you please pick this patchset up for 5.17?

Thanks,
Mike

Re: [dm-devel] [PATCH 3/3] dm: mark dm queue as blocking if any underlying is blocking

[Mike Snitzer]

On Tue, Dec 21 2021 at  9:14P -0500,
Ming Lei <ming.lei@redhat.com> wrote:

> dm request based driver doesn't set BLK_MQ_F_BLOCKING, so dm_queue_rq()
> is supposed to not sleep.
> 
> However, blk_insert_cloned_request() is used by dm_queue_rq() for
> queuing underlying request, but the underlying queue may be marked as
> BLK_MQ_F_BLOCKING, so blk_insert_cloned_request() may become to block
> current context, then rcu warning is triggered.
> 
> Fixes the issue by marking dm request based queue as BLK_MQ_F_BLOCKING
> if any underlying queue is marked as BLK_MQ_F_BLOCKING, meantime we
> need to allocate srcu beforehand.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  drivers/md/dm-rq.c    |  5 ++++-
>  drivers/md/dm-rq.h    |  3 ++-
>  drivers/md/dm-table.c | 14 ++++++++++++++
>  drivers/md/dm.c       |  5 +++--
>  drivers/md/dm.h       |  1 +
>  5 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> index 579ab6183d4d..2297d37c62a9 100644
> --- a/drivers/md/dm-rq.c
> +++ b/drivers/md/dm-rq.c
> @@ -535,7 +535,8 @@ static const struct blk_mq_ops dm_mq_ops = {
>  	.init_request = dm_mq_init_request,
>  };
>  
> -int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
> +int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t,
> +			     bool blocking)
>  {
>  	struct dm_target *immutable_tgt;
>  	int err;
> @@ -550,6 +551,8 @@ int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
>  	md->tag_set->flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_STACKING;
>  	md->tag_set->nr_hw_queues = dm_get_blk_mq_nr_hw_queues();
>  	md->tag_set->driver_data = md;
> +	if (blocking)
> +		md->tag_set->flags |= BLK_MQ_F_BLOCKING;
>  
>  	md->tag_set->cmd_size = sizeof(struct dm_rq_target_io);
>  	immutable_tgt = dm_table_get_immutable_target(t);

As you can see, dm_table_get_immutable_target(t) is called here ^

Rather than pass 'blocking' in, please just call dm_table_has_blocking_dev(t);

But not a big deal, I can clean that up once this gets committed...

> diff --git a/drivers/md/dm-rq.h b/drivers/md/dm-rq.h
> index 1eea0da641db..5f3729f277d7 100644
> --- a/drivers/md/dm-rq.h
> +++ b/drivers/md/dm-rq.h
> @@ -30,7 +30,8 @@ struct dm_rq_clone_bio_info {
>  	struct bio clone;
>  };
>  
> -int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t);
> +int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t,
> +			     bool blocking);
>  void dm_mq_cleanup_mapped_device(struct mapped_device *md);
>  
>  void dm_start_queue(struct request_queue *q);
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index aa173f5bdc3d..e4bdd4f757a3 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -1875,6 +1875,20 @@ static bool dm_table_supports_write_zeroes(struct dm_table *t)
>  	return true;
>  }
>  
> +/* If the device can block inside ->queue_rq */
> +static int device_is_io_blocking(struct dm_target *ti, struct dm_dev *dev,
> +			      sector_t start, sector_t len, void *data)
> +{
> +	struct request_queue *q = bdev_get_queue(dev->bdev);
> +
> +	return blk_queue_blocking(q);
> +}
> +
> +bool dm_table_has_blocking_dev(struct dm_table *t)
> +{
> +	return dm_table_any_dev_attr(t, device_is_io_blocking, NULL);
> +}
> +
>  static int device_not_nowait_capable(struct dm_target *ti, struct dm_dev *dev,
>  				     sector_t start, sector_t len, void *data)
>  {
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 280918cdcabd..2f72877752dd 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1761,7 +1761,7 @@ static struct mapped_device *alloc_dev(int minor)
>  	 * established. If request-based table is loaded: blk-mq will
>  	 * override accordingly.
>  	 */
> -	md->disk = blk_alloc_disk(md->numa_node_id);
> +	md->disk = blk_alloc_disk_srcu(md->numa_node_id);
>  	if (!md->disk)
>  		goto bad;
>  	md->queue = md->disk->queue;
> @@ -2046,7 +2046,8 @@ int dm_setup_md_queue(struct mapped_device *md, struct dm_table *t)
>  	switch (type) {
>  	case DM_TYPE_REQUEST_BASED:
>  		md->disk->fops = &dm_rq_blk_dops;
> -		r = dm_mq_init_request_queue(md, t);
> +		r = dm_mq_init_request_queue(md, t,
> +				dm_table_has_blocking_dev(t));
>  		if (r) {
>  			DMERR("Cannot initialize queue for request-based dm mapped device");
>  			return r;
> diff --git a/drivers/md/dm.h b/drivers/md/dm.h
> index 742d9c80efe1..f7f92b272cce 100644
> --- a/drivers/md/dm.h
> +++ b/drivers/md/dm.h
> @@ -60,6 +60,7 @@ int dm_calculate_queue_limits(struct dm_table *table,
>  			      struct queue_limits *limits);
>  int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
>  			      struct queue_limits *limits);
> +bool dm_table_has_blocking_dev(struct dm_table *t);
>  struct list_head *dm_table_get_devices(struct dm_table *t);
>  void dm_table_presuspend_targets(struct dm_table *t);
>  void dm_table_presuspend_undo_targets(struct dm_table *t);
> -- 
> 2.31.1
> 

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

Late, given holidays we know why, but this patchset is needed for 5.17
(maybe with added: 'Fixes: 704b914f15fb7 "blk-mq: move srcu from
blk_mq_hw_ctx to request_queue"' to this 3rd patch?)

Jens, can you please pick this patchset up for 5.17?

Thanks,
Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 3/3] dm: mark dm queue as blocking if any underlying is blocking

[Ming Lei]

On Thu, Jan 06, 2022 at 10:40:51AM -0500, Mike Snitzer wrote:
> On Tue, Dec 21 2021 at  9:14P -0500,
> Ming Lei <ming.lei@redhat.com> wrote:
> 
> > dm request based driver doesn't set BLK_MQ_F_BLOCKING, so dm_queue_rq()
> > is supposed to not sleep.
> > 
> > However, blk_insert_cloned_request() is used by dm_queue_rq() for
> > queuing underlying request, but the underlying queue may be marked as
> > BLK_MQ_F_BLOCKING, so blk_insert_cloned_request() may become to block
> > current context, then rcu warning is triggered.
> > 
> > Fixes the issue by marking dm request based queue as BLK_MQ_F_BLOCKING
> > if any underlying queue is marked as BLK_MQ_F_BLOCKING, meantime we
> > need to allocate srcu beforehand.
> > 
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  drivers/md/dm-rq.c    |  5 ++++-
> >  drivers/md/dm-rq.h    |  3 ++-
> >  drivers/md/dm-table.c | 14 ++++++++++++++
> >  drivers/md/dm.c       |  5 +++--
> >  drivers/md/dm.h       |  1 +
> >  5 files changed, 24 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> > index 579ab6183d4d..2297d37c62a9 100644
> > --- a/drivers/md/dm-rq.c
> > +++ b/drivers/md/dm-rq.c
> > @@ -535,7 +535,8 @@ static const struct blk_mq_ops dm_mq_ops = {
> >  	.init_request = dm_mq_init_request,
> >  };
> >  
> > -int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
> > +int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t,
> > +			     bool blocking)
> >  {
> >  	struct dm_target *immutable_tgt;
> >  	int err;
> > @@ -550,6 +551,8 @@ int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
> >  	md->tag_set->flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_STACKING;
> >  	md->tag_set->nr_hw_queues = dm_get_blk_mq_nr_hw_queues();
> >  	md->tag_set->driver_data = md;
> > +	if (blocking)
> > +		md->tag_set->flags |= BLK_MQ_F_BLOCKING;
> >  
> >  	md->tag_set->cmd_size = sizeof(struct dm_rq_target_io);
> >  	immutable_tgt = dm_table_get_immutable_target(t);
> 
> As you can see, dm_table_get_immutable_target(t) is called here ^
> 
> Rather than pass 'blocking' in, please just call dm_table_has_blocking_dev(t);
> 
> But not a big deal, I can clean that up once this gets committed...
> 
> > diff --git a/drivers/md/dm-rq.h b/drivers/md/dm-rq.h
> > index 1eea0da641db..5f3729f277d7 100644
> > --- a/drivers/md/dm-rq.h
> > +++ b/drivers/md/dm-rq.h
> > @@ -30,7 +30,8 @@ struct dm_rq_clone_bio_info {
> >  	struct bio clone;
> >  };
> >  
> > -int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t);
> > +int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t,
> > +			     bool blocking);
> >  void dm_mq_cleanup_mapped_device(struct mapped_device *md);
> >  
> >  void dm_start_queue(struct request_queue *q);
> > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > index aa173f5bdc3d..e4bdd4f757a3 100644
> > --- a/drivers/md/dm-table.c
> > +++ b/drivers/md/dm-table.c
> > @@ -1875,6 +1875,20 @@ static bool dm_table_supports_write_zeroes(struct dm_table *t)
> >  	return true;
> >  }
> >  
> > +/* If the device can block inside ->queue_rq */
> > +static int device_is_io_blocking(struct dm_target *ti, struct dm_dev *dev,
> > +			      sector_t start, sector_t len, void *data)
> > +{
> > +	struct request_queue *q = bdev_get_queue(dev->bdev);
> > +
> > +	return blk_queue_blocking(q);
> > +}
> > +
> > +bool dm_table_has_blocking_dev(struct dm_table *t)
> > +{
> > +	return dm_table_any_dev_attr(t, device_is_io_blocking, NULL);
> > +}
> > +
> >  static int device_not_nowait_capable(struct dm_target *ti, struct dm_dev *dev,
> >  				     sector_t start, sector_t len, void *data)
> >  {
> > diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> > index 280918cdcabd..2f72877752dd 100644
> > --- a/drivers/md/dm.c
> > +++ b/drivers/md/dm.c
> > @@ -1761,7 +1761,7 @@ static struct mapped_device *alloc_dev(int minor)
> >  	 * established. If request-based table is loaded: blk-mq will
> >  	 * override accordingly.
> >  	 */
> > -	md->disk = blk_alloc_disk(md->numa_node_id);
> > +	md->disk = blk_alloc_disk_srcu(md->numa_node_id);
> >  	if (!md->disk)
> >  		goto bad;
> >  	md->queue = md->disk->queue;
> > @@ -2046,7 +2046,8 @@ int dm_setup_md_queue(struct mapped_device *md, struct dm_table *t)
> >  	switch (type) {
> >  	case DM_TYPE_REQUEST_BASED:
> >  		md->disk->fops = &dm_rq_blk_dops;
> > -		r = dm_mq_init_request_queue(md, t);
> > +		r = dm_mq_init_request_queue(md, t,
> > +				dm_table_has_blocking_dev(t));
> >  		if (r) {
> >  			DMERR("Cannot initialize queue for request-based dm mapped device");
> >  			return r;
> > diff --git a/drivers/md/dm.h b/drivers/md/dm.h
> > index 742d9c80efe1..f7f92b272cce 100644
> > --- a/drivers/md/dm.h
> > +++ b/drivers/md/dm.h
> > @@ -60,6 +60,7 @@ int dm_calculate_queue_limits(struct dm_table *table,
> >  			      struct queue_limits *limits);
> >  int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
> >  			      struct queue_limits *limits);
> > +bool dm_table_has_blocking_dev(struct dm_table *t);
> >  struct list_head *dm_table_get_devices(struct dm_table *t);
> >  void dm_table_presuspend_targets(struct dm_table *t);
> >  void dm_table_presuspend_undo_targets(struct dm_table *t);
> > -- 
> > 2.31.1
> > 
> 
> Reviewed-by: Mike Snitzer <snitzer@redhat.com>

Thanks!

> 
> Late, given holidays we know why, but this patchset is needed for 5.17
> (maybe with added: 'Fixes: 704b914f15fb7 "blk-mq: move srcu from
> blk_mq_hw_ctx to request_queue"' to this 3rd patch?)

It is one long-term issue, not related with commit 704b914f15fb7. The
problem is that rcu read lock is held by blk-mq when running dm_queue_rq()
which calls underlying blocking queue's ->queue_rq() which may sleep
somewhere.


Thanks,
Ming

Re: [dm-devel] [PATCH 3/3] dm: mark dm queue as blocking if any underlying is blocking

[Ming Lei]

On Thu, Jan 06, 2022 at 10:40:51AM -0500, Mike Snitzer wrote:
> On Tue, Dec 21 2021 at  9:14P -0500,
> Ming Lei <ming.lei@redhat.com> wrote:
> 
> > dm request based driver doesn't set BLK_MQ_F_BLOCKING, so dm_queue_rq()
> > is supposed to not sleep.
> > 
> > However, blk_insert_cloned_request() is used by dm_queue_rq() for
> > queuing underlying request, but the underlying queue may be marked as
> > BLK_MQ_F_BLOCKING, so blk_insert_cloned_request() may become to block
> > current context, then rcu warning is triggered.
> > 
> > Fixes the issue by marking dm request based queue as BLK_MQ_F_BLOCKING
> > if any underlying queue is marked as BLK_MQ_F_BLOCKING, meantime we
> > need to allocate srcu beforehand.
> > 
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  drivers/md/dm-rq.c    |  5 ++++-
> >  drivers/md/dm-rq.h    |  3 ++-
> >  drivers/md/dm-table.c | 14 ++++++++++++++
> >  drivers/md/dm.c       |  5 +++--
> >  drivers/md/dm.h       |  1 +
> >  5 files changed, 24 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> > index 579ab6183d4d..2297d37c62a9 100644
> > --- a/drivers/md/dm-rq.c
> > +++ b/drivers/md/dm-rq.c
> > @@ -535,7 +535,8 @@ static const struct blk_mq_ops dm_mq_ops = {
> >  	.init_request = dm_mq_init_request,
> >  };
> >  
> > -int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
> > +int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t,
> > +			     bool blocking)
> >  {
> >  	struct dm_target *immutable_tgt;
> >  	int err;
> > @@ -550,6 +551,8 @@ int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
> >  	md->tag_set->flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_STACKING;
> >  	md->tag_set->nr_hw_queues = dm_get_blk_mq_nr_hw_queues();
> >  	md->tag_set->driver_data = md;
> > +	if (blocking)
> > +		md->tag_set->flags |= BLK_MQ_F_BLOCKING;
> >  
> >  	md->tag_set->cmd_size = sizeof(struct dm_rq_target_io);
> >  	immutable_tgt = dm_table_get_immutable_target(t);
> 
> As you can see, dm_table_get_immutable_target(t) is called here ^
> 
> Rather than pass 'blocking' in, please just call dm_table_has_blocking_dev(t);
> 
> But not a big deal, I can clean that up once this gets committed...
> 
> > diff --git a/drivers/md/dm-rq.h b/drivers/md/dm-rq.h
> > index 1eea0da641db..5f3729f277d7 100644
> > --- a/drivers/md/dm-rq.h
> > +++ b/drivers/md/dm-rq.h
> > @@ -30,7 +30,8 @@ struct dm_rq_clone_bio_info {
> >  	struct bio clone;
> >  };
> >  
> > -int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t);
> > +int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t,
> > +			     bool blocking);
> >  void dm_mq_cleanup_mapped_device(struct mapped_device *md);
> >  
> >  void dm_start_queue(struct request_queue *q);
> > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > index aa173f5bdc3d..e4bdd4f757a3 100644
> > --- a/drivers/md/dm-table.c
> > +++ b/drivers/md/dm-table.c
> > @@ -1875,6 +1875,20 @@ static bool dm_table_supports_write_zeroes(struct dm_table *t)
> >  	return true;
> >  }
> >  
> > +/* If the device can block inside ->queue_rq */
> > +static int device_is_io_blocking(struct dm_target *ti, struct dm_dev *dev,
> > +			      sector_t start, sector_t len, void *data)
> > +{
> > +	struct request_queue *q = bdev_get_queue(dev->bdev);
> > +
> > +	return blk_queue_blocking(q);
> > +}
> > +
> > +bool dm_table_has_blocking_dev(struct dm_table *t)
> > +{
> > +	return dm_table_any_dev_attr(t, device_is_io_blocking, NULL);
> > +}
> > +
> >  static int device_not_nowait_capable(struct dm_target *ti, struct dm_dev *dev,
> >  				     sector_t start, sector_t len, void *data)
> >  {
> > diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> > index 280918cdcabd..2f72877752dd 100644
> > --- a/drivers/md/dm.c
> > +++ b/drivers/md/dm.c
> > @@ -1761,7 +1761,7 @@ static struct mapped_device *alloc_dev(int minor)
> >  	 * established. If request-based table is loaded: blk-mq will
> >  	 * override accordingly.
> >  	 */
> > -	md->disk = blk_alloc_disk(md->numa_node_id);
> > +	md->disk = blk_alloc_disk_srcu(md->numa_node_id);
> >  	if (!md->disk)
> >  		goto bad;
> >  	md->queue = md->disk->queue;
> > @@ -2046,7 +2046,8 @@ int dm_setup_md_queue(struct mapped_device *md, struct dm_table *t)
> >  	switch (type) {
> >  	case DM_TYPE_REQUEST_BASED:
> >  		md->disk->fops = &dm_rq_blk_dops;
> > -		r = dm_mq_init_request_queue(md, t);
> > +		r = dm_mq_init_request_queue(md, t,
> > +				dm_table_has_blocking_dev(t));
> >  		if (r) {
> >  			DMERR("Cannot initialize queue for request-based dm mapped device");
> >  			return r;
> > diff --git a/drivers/md/dm.h b/drivers/md/dm.h
> > index 742d9c80efe1..f7f92b272cce 100644
> > --- a/drivers/md/dm.h
> > +++ b/drivers/md/dm.h
> > @@ -60,6 +60,7 @@ int dm_calculate_queue_limits(struct dm_table *table,
> >  			      struct queue_limits *limits);
> >  int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
> >  			      struct queue_limits *limits);
> > +bool dm_table_has_blocking_dev(struct dm_table *t);
> >  struct list_head *dm_table_get_devices(struct dm_table *t);
> >  void dm_table_presuspend_targets(struct dm_table *t);
> >  void dm_table_presuspend_undo_targets(struct dm_table *t);
> > -- 
> > 2.31.1
> > 
> 
> Reviewed-by: Mike Snitzer <snitzer@redhat.com>

Thanks!

> 
> Late, given holidays we know why, but this patchset is needed for 5.17
> (maybe with added: 'Fixes: 704b914f15fb7 "blk-mq: move srcu from
> blk_mq_hw_ctx to request_queue"' to this 3rd patch?)

It is one long-term issue, not related with commit 704b914f15fb7. The
problem is that rcu read lock is held by blk-mq when running dm_queue_rq()
which calls underlying blocking queue's ->queue_rq() which may sleep
somewhere.


Thanks,
Ming

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 3/3] dm: mark dm queue as blocking if any underlying is blocking

[Mike Snitzer]

On Thu, Jan 06 2022 at 10:51P -0500,
Ming Lei <ming.lei@redhat.com> wrote:

> On Thu, Jan 06, 2022 at 10:40:51AM -0500, Mike Snitzer wrote:
> > On Tue, Dec 21 2021 at  9:14P -0500,
> > Ming Lei <ming.lei@redhat.com> wrote:
> > 
> > > dm request based driver doesn't set BLK_MQ_F_BLOCKING, so dm_queue_rq()
> > > is supposed to not sleep.
> > > 
> > > However, blk_insert_cloned_request() is used by dm_queue_rq() for
> > > queuing underlying request, but the underlying queue may be marked as
> > > BLK_MQ_F_BLOCKING, so blk_insert_cloned_request() may become to block
> > > current context, then rcu warning is triggered.
> > > 
> > > Fixes the issue by marking dm request based queue as BLK_MQ_F_BLOCKING
> > > if any underlying queue is marked as BLK_MQ_F_BLOCKING, meantime we
> > > need to allocate srcu beforehand.
> > > 
> > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > ---
> > >  drivers/md/dm-rq.c    |  5 ++++-
> > >  drivers/md/dm-rq.h    |  3 ++-
> > >  drivers/md/dm-table.c | 14 ++++++++++++++
> > >  drivers/md/dm.c       |  5 +++--
> > >  drivers/md/dm.h       |  1 +
> > >  5 files changed, 24 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> > > index 579ab6183d4d..2297d37c62a9 100644
> > > --- a/drivers/md/dm-rq.c
> > > +++ b/drivers/md/dm-rq.c
> > > @@ -535,7 +535,8 @@ static const struct blk_mq_ops dm_mq_ops = {
> > >  	.init_request = dm_mq_init_request,
> > >  };
> > >  
> > > -int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
> > > +int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t,
> > > +			     bool blocking)
> > >  {
> > >  	struct dm_target *immutable_tgt;
> > >  	int err;
> > > @@ -550,6 +551,8 @@ int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
> > >  	md->tag_set->flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_STACKING;
> > >  	md->tag_set->nr_hw_queues = dm_get_blk_mq_nr_hw_queues();
> > >  	md->tag_set->driver_data = md;
> > > +	if (blocking)
> > > +		md->tag_set->flags |= BLK_MQ_F_BLOCKING;
> > >  
> > >  	md->tag_set->cmd_size = sizeof(struct dm_rq_target_io);
> > >  	immutable_tgt = dm_table_get_immutable_target(t);
> > 
> > As you can see, dm_table_get_immutable_target(t) is called here ^
> > 
> > Rather than pass 'blocking' in, please just call dm_table_has_blocking_dev(t);
> > 
> > But not a big deal, I can clean that up once this gets committed...
> > 
> > > diff --git a/drivers/md/dm-rq.h b/drivers/md/dm-rq.h
> > > index 1eea0da641db..5f3729f277d7 100644
> > > --- a/drivers/md/dm-rq.h
> > > +++ b/drivers/md/dm-rq.h
> > > @@ -30,7 +30,8 @@ struct dm_rq_clone_bio_info {
> > >  	struct bio clone;
> > >  };
> > >  
> > > -int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t);
> > > +int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t,
> > > +			     bool blocking);
> > >  void dm_mq_cleanup_mapped_device(struct mapped_device *md);
> > >  
> > >  void dm_start_queue(struct request_queue *q);
> > > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > > index aa173f5bdc3d..e4bdd4f757a3 100644
> > > --- a/drivers/md/dm-table.c
> > > +++ b/drivers/md/dm-table.c
> > > @@ -1875,6 +1875,20 @@ static bool dm_table_supports_write_zeroes(struct dm_table *t)
> > >  	return true;
> > >  }
> > >  
> > > +/* If the device can block inside ->queue_rq */
> > > +static int device_is_io_blocking(struct dm_target *ti, struct dm_dev *dev,
> > > +			      sector_t start, sector_t len, void *data)
> > > +{
> > > +	struct request_queue *q = bdev_get_queue(dev->bdev);
> > > +
> > > +	return blk_queue_blocking(q);
> > > +}
> > > +
> > > +bool dm_table_has_blocking_dev(struct dm_table *t)
> > > +{
> > > +	return dm_table_any_dev_attr(t, device_is_io_blocking, NULL);
> > > +}
> > > +
> > >  static int device_not_nowait_capable(struct dm_target *ti, struct dm_dev *dev,
> > >  				     sector_t start, sector_t len, void *data)
> > >  {
> > > diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> > > index 280918cdcabd..2f72877752dd 100644
> > > --- a/drivers/md/dm.c
> > > +++ b/drivers/md/dm.c
> > > @@ -1761,7 +1761,7 @@ static struct mapped_device *alloc_dev(int minor)
> > >  	 * established. If request-based table is loaded: blk-mq will
> > >  	 * override accordingly.
> > >  	 */
> > > -	md->disk = blk_alloc_disk(md->numa_node_id);
> > > +	md->disk = blk_alloc_disk_srcu(md->numa_node_id);
> > >  	if (!md->disk)
> > >  		goto bad;
> > >  	md->queue = md->disk->queue;
> > > @@ -2046,7 +2046,8 @@ int dm_setup_md_queue(struct mapped_device *md, struct dm_table *t)
> > >  	switch (type) {
> > >  	case DM_TYPE_REQUEST_BASED:
> > >  		md->disk->fops = &dm_rq_blk_dops;
> > > -		r = dm_mq_init_request_queue(md, t);
> > > +		r = dm_mq_init_request_queue(md, t,
> > > +				dm_table_has_blocking_dev(t));
> > >  		if (r) {
> > >  			DMERR("Cannot initialize queue for request-based dm mapped device");
> > >  			return r;
> > > diff --git a/drivers/md/dm.h b/drivers/md/dm.h
> > > index 742d9c80efe1..f7f92b272cce 100644
> > > --- a/drivers/md/dm.h
> > > +++ b/drivers/md/dm.h
> > > @@ -60,6 +60,7 @@ int dm_calculate_queue_limits(struct dm_table *table,
> > >  			      struct queue_limits *limits);
> > >  int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
> > >  			      struct queue_limits *limits);
> > > +bool dm_table_has_blocking_dev(struct dm_table *t);
> > >  struct list_head *dm_table_get_devices(struct dm_table *t);
> > >  void dm_table_presuspend_targets(struct dm_table *t);
> > >  void dm_table_presuspend_undo_targets(struct dm_table *t);
> > > -- 
> > > 2.31.1
> > > 
> > 
> > Reviewed-by: Mike Snitzer <snitzer@redhat.com>
> 
> Thanks!
> 
> > 
> > Late, given holidays we know why, but this patchset is needed for 5.17
> > (maybe with added: 'Fixes: 704b914f15fb7 "blk-mq: move srcu from
> > blk_mq_hw_ctx to request_queue"' to this 3rd patch?)
> 
> It is one long-term issue, not related with commit 704b914f15fb7. The
> problem is that rcu read lock is held by blk-mq when running dm_queue_rq()
> which calls underlying blocking queue's ->queue_rq() which may sleep
> somewhere.

OK, really don't _need_ a Fixes since it isn't getting marked for stable@ anyway.

Re: [dm-devel] [PATCH 3/3] dm: mark dm queue as blocking if any underlying is blocking

[Mike Snitzer]

On Thu, Jan 06 2022 at 10:51P -0500,
Ming Lei <ming.lei@redhat.com> wrote:

> On Thu, Jan 06, 2022 at 10:40:51AM -0500, Mike Snitzer wrote:
> > On Tue, Dec 21 2021 at  9:14P -0500,
> > Ming Lei <ming.lei@redhat.com> wrote:
> > 
> > > dm request based driver doesn't set BLK_MQ_F_BLOCKING, so dm_queue_rq()
> > > is supposed to not sleep.
> > > 
> > > However, blk_insert_cloned_request() is used by dm_queue_rq() for
> > > queuing underlying request, but the underlying queue may be marked as
> > > BLK_MQ_F_BLOCKING, so blk_insert_cloned_request() may become to block
> > > current context, then rcu warning is triggered.
> > > 
> > > Fixes the issue by marking dm request based queue as BLK_MQ_F_BLOCKING
> > > if any underlying queue is marked as BLK_MQ_F_BLOCKING, meantime we
> > > need to allocate srcu beforehand.
> > > 
> > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > ---
> > >  drivers/md/dm-rq.c    |  5 ++++-
> > >  drivers/md/dm-rq.h    |  3 ++-
> > >  drivers/md/dm-table.c | 14 ++++++++++++++
> > >  drivers/md/dm.c       |  5 +++--
> > >  drivers/md/dm.h       |  1 +
> > >  5 files changed, 24 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> > > index 579ab6183d4d..2297d37c62a9 100644
> > > --- a/drivers/md/dm-rq.c
> > > +++ b/drivers/md/dm-rq.c
> > > @@ -535,7 +535,8 @@ static const struct blk_mq_ops dm_mq_ops = {
> > >  	.init_request = dm_mq_init_request,
> > >  };
> > >  
> > > -int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
> > > +int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t,
> > > +			     bool blocking)
> > >  {
> > >  	struct dm_target *immutable_tgt;
> > >  	int err;
> > > @@ -550,6 +551,8 @@ int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
> > >  	md->tag_set->flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_STACKING;
> > >  	md->tag_set->nr_hw_queues = dm_get_blk_mq_nr_hw_queues();
> > >  	md->tag_set->driver_data = md;
> > > +	if (blocking)
> > > +		md->tag_set->flags |= BLK_MQ_F_BLOCKING;
> > >  
> > >  	md->tag_set->cmd_size = sizeof(struct dm_rq_target_io);
> > >  	immutable_tgt = dm_table_get_immutable_target(t);
> > 
> > As you can see, dm_table_get_immutable_target(t) is called here ^
> > 
> > Rather than pass 'blocking' in, please just call dm_table_has_blocking_dev(t);
> > 
> > But not a big deal, I can clean that up once this gets committed...
> > 
> > > diff --git a/drivers/md/dm-rq.h b/drivers/md/dm-rq.h
> > > index 1eea0da641db..5f3729f277d7 100644
> > > --- a/drivers/md/dm-rq.h
> > > +++ b/drivers/md/dm-rq.h
> > > @@ -30,7 +30,8 @@ struct dm_rq_clone_bio_info {
> > >  	struct bio clone;
> > >  };
> > >  
> > > -int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t);
> > > +int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t,
> > > +			     bool blocking);
> > >  void dm_mq_cleanup_mapped_device(struct mapped_device *md);
> > >  
> > >  void dm_start_queue(struct request_queue *q);
> > > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > > index aa173f5bdc3d..e4bdd4f757a3 100644
> > > --- a/drivers/md/dm-table.c
> > > +++ b/drivers/md/dm-table.c
> > > @@ -1875,6 +1875,20 @@ static bool dm_table_supports_write_zeroes(struct dm_table *t)
> > >  	return true;
> > >  }
> > >  
> > > +/* If the device can block inside ->queue_rq */
> > > +static int device_is_io_blocking(struct dm_target *ti, struct dm_dev *dev,
> > > +			      sector_t start, sector_t len, void *data)
> > > +{
> > > +	struct request_queue *q = bdev_get_queue(dev->bdev);
> > > +
> > > +	return blk_queue_blocking(q);
> > > +}
> > > +
> > > +bool dm_table_has_blocking_dev(struct dm_table *t)
> > > +{
> > > +	return dm_table_any_dev_attr(t, device_is_io_blocking, NULL);
> > > +}
> > > +
> > >  static int device_not_nowait_capable(struct dm_target *ti, struct dm_dev *dev,
> > >  				     sector_t start, sector_t len, void *data)
> > >  {
> > > diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> > > index 280918cdcabd..2f72877752dd 100644
> > > --- a/drivers/md/dm.c
> > > +++ b/drivers/md/dm.c
> > > @@ -1761,7 +1761,7 @@ static struct mapped_device *alloc_dev(int minor)
> > >  	 * established. If request-based table is loaded: blk-mq will
> > >  	 * override accordingly.
> > >  	 */
> > > -	md->disk = blk_alloc_disk(md->numa_node_id);
> > > +	md->disk = blk_alloc_disk_srcu(md->numa_node_id);
> > >  	if (!md->disk)
> > >  		goto bad;
> > >  	md->queue = md->disk->queue;
> > > @@ -2046,7 +2046,8 @@ int dm_setup_md_queue(struct mapped_device *md, struct dm_table *t)
> > >  	switch (type) {
> > >  	case DM_TYPE_REQUEST_BASED:
> > >  		md->disk->fops = &dm_rq_blk_dops;
> > > -		r = dm_mq_init_request_queue(md, t);
> > > +		r = dm_mq_init_request_queue(md, t,
> > > +				dm_table_has_blocking_dev(t));
> > >  		if (r) {
> > >  			DMERR("Cannot initialize queue for request-based dm mapped device");
> > >  			return r;
> > > diff --git a/drivers/md/dm.h b/drivers/md/dm.h
> > > index 742d9c80efe1..f7f92b272cce 100644
> > > --- a/drivers/md/dm.h
> > > +++ b/drivers/md/dm.h
> > > @@ -60,6 +60,7 @@ int dm_calculate_queue_limits(struct dm_table *table,
> > >  			      struct queue_limits *limits);
> > >  int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
> > >  			      struct queue_limits *limits);
> > > +bool dm_table_has_blocking_dev(struct dm_table *t);
> > >  struct list_head *dm_table_get_devices(struct dm_table *t);
> > >  void dm_table_presuspend_targets(struct dm_table *t);
> > >  void dm_table_presuspend_undo_targets(struct dm_table *t);
> > > -- 
> > > 2.31.1
> > > 
> > 
> > Reviewed-by: Mike Snitzer <snitzer@redhat.com>
> 
> Thanks!
> 
> > 
> > Late, given holidays we know why, but this patchset is needed for 5.17
> > (maybe with added: 'Fixes: 704b914f15fb7 "blk-mq: move srcu from
> > blk_mq_hw_ctx to request_queue"' to this 3rd patch?)
> 
> It is one long-term issue, not related with commit 704b914f15fb7. The
> problem is that rcu read lock is held by blk-mq when running dm_queue_rq()
> which calls underlying blocking queue's ->queue_rq() which may sleep
> somewhere.

OK, really don't _need_ a Fixes since it isn't getting marked for stable@ anyway.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Mike Snitzer]

On Tue, Dec 28 2021 at  4:30P -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Wed, Dec 22 2021 at 11:16P -0500,
> Ming Lei <ming.lei@redhat.com> wrote:
> 
> > On Tue, Dec 21, 2021 at 08:21:39AM -0800, Christoph Hellwig wrote:
> > > On Tue, Dec 21, 2021 at 10:14:56PM +0800, Ming Lei wrote:
> > > > Hello,
> > > > 
> > > > dm-rq may be built on blk-mq device which marks BLK_MQ_F_BLOCKING, so
> > > > dm_mq_queue_rq() may become to sleep current context.
> > > > 
> > > > Fixes the issue by allowing dm-rq to set BLK_MQ_F_BLOCKING in case that
> > > > any underlying queue is marked as BLK_MQ_F_BLOCKING.
> > > > 
> > > > DM request queue is allocated before allocating tagset, this way is a
> > > > bit special, so we need to pre-allocate srcu payload, then use the queue
> > > > flag of QUEUE_FLAG_BLOCKING for locking dispatch.
> > > 
> > > What is the benefit over just forcing bio-based dm-mpath for these
> > > devices?
> > 
> > At least IO scheduler can't be used for bio based dm-mpath, also there should
> > be other drawbacks for bio based mpath and request mpath is often the default
> > option, maybe Mike has more input about bio vs request dm-mpath.
> 
> Yeah, people use request-based for IO scheduling and more capable path
> selectors. Imposing bio-based would be a pretty jarring workaround for 
> BLK_MQ_F_BLOCKING. request-based DM should properly support it.
> 
> I'm on vacation for the next week but will have a look at this
> patchset once I'm back.

I replied last week and hoped Jens would pick this patchset up after
my Reviewed-by of patch 3/3.

Christoph wasn't back though, so best to kick this thread again.

Thoughts on this patchset?

Thanks,
Mike

Re: [dm-devel] [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Mike Snitzer]

On Tue, Dec 28 2021 at  4:30P -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Wed, Dec 22 2021 at 11:16P -0500,
> Ming Lei <ming.lei@redhat.com> wrote:
> 
> > On Tue, Dec 21, 2021 at 08:21:39AM -0800, Christoph Hellwig wrote:
> > > On Tue, Dec 21, 2021 at 10:14:56PM +0800, Ming Lei wrote:
> > > > Hello,
> > > > 
> > > > dm-rq may be built on blk-mq device which marks BLK_MQ_F_BLOCKING, so
> > > > dm_mq_queue_rq() may become to sleep current context.
> > > > 
> > > > Fixes the issue by allowing dm-rq to set BLK_MQ_F_BLOCKING in case that
> > > > any underlying queue is marked as BLK_MQ_F_BLOCKING.
> > > > 
> > > > DM request queue is allocated before allocating tagset, this way is a
> > > > bit special, so we need to pre-allocate srcu payload, then use the queue
> > > > flag of QUEUE_FLAG_BLOCKING for locking dispatch.
> > > 
> > > What is the benefit over just forcing bio-based dm-mpath for these
> > > devices?
> > 
> > At least IO scheduler can't be used for bio based dm-mpath, also there should
> > be other drawbacks for bio based mpath and request mpath is often the default
> > option, maybe Mike has more input about bio vs request dm-mpath.
> 
> Yeah, people use request-based for IO scheduling and more capable path
> selectors. Imposing bio-based would be a pretty jarring workaround for 
> BLK_MQ_F_BLOCKING. request-based DM should properly support it.
> 
> I'm on vacation for the next week but will have a look at this
> patchset once I'm back.

I replied last week and hoped Jens would pick this patchset up after
my Reviewed-by of patch 3/3.

Christoph wasn't back though, so best to kick this thread again.

Thoughts on this patchset?

Thanks,
Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Christoph Hellwig]

On Tue, Dec 28, 2021 at 04:30:08PM -0500, Mike Snitzer wrote:
> Yeah, people use request-based for IO scheduling and more capable path
> selectors. Imposing bio-based would be a pretty jarring workaround for 
> BLK_MQ_F_BLOCKING. request-based DM should properly support it.

Given that nvme-tcp is the only blocking driver that has multipath
driver that driver explicitly does not intend to support dm-multipath
I'm absolutely against adding block layer cruft for this particular
use case.

SCSI even has this:

	        /*
		 * SCSI never enables blk-mq's BLK_MQ_F_BLOCKING flag so
		 * calling synchronize_rcu() once is enough.
		 */
		WARN_ON_ONCE(shost->tag_set.flags & BLK_MQ_F_BLOCKING);

Re: [dm-devel] [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Christoph Hellwig]

On Tue, Dec 28, 2021 at 04:30:08PM -0500, Mike Snitzer wrote:
> Yeah, people use request-based for IO scheduling and more capable path
> selectors. Imposing bio-based would be a pretty jarring workaround for 
> BLK_MQ_F_BLOCKING. request-based DM should properly support it.

Given that nvme-tcp is the only blocking driver that has multipath
driver that driver explicitly does not intend to support dm-multipath
I'm absolutely against adding block layer cruft for this particular
use case.

SCSI even has this:

	        /*
		 * SCSI never enables blk-mq's BLK_MQ_F_BLOCKING flag so
		 * calling synchronize_rcu() once is enough.
		 */
		WARN_ON_ONCE(shost->tag_set.flags & BLK_MQ_F_BLOCKING);

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Mike Snitzer]

On Tue, Jan 11 2022 at  3:34P -0500,
Christoph Hellwig <hch@infradead.org> wrote:

> On Tue, Dec 28, 2021 at 04:30:08PM -0500, Mike Snitzer wrote:
> > Yeah, people use request-based for IO scheduling and more capable path
> > selectors. Imposing bio-based would be a pretty jarring workaround for 
> > BLK_MQ_F_BLOCKING. request-based DM should properly support it.
> 
> Given that nvme-tcp is the only blocking driver that has multipath
> driver that driver explicitly does not intend to support dm-multipath
> I'm absolutely against adding block layer cruft for this particular
> use case.

this diffstat amounts to what you call "cruft":

 block/blk-core.c       |  2 +-
 block/blk-mq.c         |  6 +++---
 block/blk-mq.h         |  2 +-
 block/blk-sysfs.c      |  2 +-
 block/genhd.c          |  5 +++--
 drivers/md/dm-rq.c     |  5 ++++-
 drivers/md/dm-rq.h     |  3 ++-
 drivers/md/dm-table.c  | 14 ++++++++++++++
 drivers/md/dm.c        |  5 +++--
 drivers/md/dm.h        |  1 +
 include/linux/blkdev.h |  5 +++--
 include/linux/genhd.h  | 12 ++++++++----
 12 files changed, 44 insertions(+), 18 deletions(-)

> SCSI even has this:
> 
> 	        /*
> 		 * SCSI never enables blk-mq's BLK_MQ_F_BLOCKING flag so
> 		 * calling synchronize_rcu() once is enough.
> 		 */
> 		WARN_ON_ONCE(shost->tag_set.flags & BLK_MQ_F_BLOCKING);
> 

Round and round we go.. Pretty tired of this.

You are perfectly fine with incrementally compromising request-based
DM's ability to evolve as block core does.

Seriously, this patchset shouldn't warrant bickering:
https://patchwork.kernel.org/project/dm-devel/list/?series=598823

Jens, this incremental weakening of what it is that DM is allowed to
do is not something I can continue to work with (nor should Ming's or
others' contributions be rejected for such reasons).

This tribal war needs to stop.

Mike

Re: [dm-devel] [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Mike Snitzer]

On Tue, Jan 11 2022 at  3:34P -0500,
Christoph Hellwig <hch@infradead.org> wrote:

> On Tue, Dec 28, 2021 at 04:30:08PM -0500, Mike Snitzer wrote:
> > Yeah, people use request-based for IO scheduling and more capable path
> > selectors. Imposing bio-based would be a pretty jarring workaround for 
> > BLK_MQ_F_BLOCKING. request-based DM should properly support it.
> 
> Given that nvme-tcp is the only blocking driver that has multipath
> driver that driver explicitly does not intend to support dm-multipath
> I'm absolutely against adding block layer cruft for this particular
> use case.

this diffstat amounts to what you call "cruft":

 block/blk-core.c       |  2 +-
 block/blk-mq.c         |  6 +++---
 block/blk-mq.h         |  2 +-
 block/blk-sysfs.c      |  2 +-
 block/genhd.c          |  5 +++--
 drivers/md/dm-rq.c     |  5 ++++-
 drivers/md/dm-rq.h     |  3 ++-
 drivers/md/dm-table.c  | 14 ++++++++++++++
 drivers/md/dm.c        |  5 +++--
 drivers/md/dm.h        |  1 +
 include/linux/blkdev.h |  5 +++--
 include/linux/genhd.h  | 12 ++++++++----
 12 files changed, 44 insertions(+), 18 deletions(-)

> SCSI even has this:
> 
> 	        /*
> 		 * SCSI never enables blk-mq's BLK_MQ_F_BLOCKING flag so
> 		 * calling synchronize_rcu() once is enough.
> 		 */
> 		WARN_ON_ONCE(shost->tag_set.flags & BLK_MQ_F_BLOCKING);
> 

Round and round we go.. Pretty tired of this.

You are perfectly fine with incrementally compromising request-based
DM's ability to evolve as block core does.

Seriously, this patchset shouldn't warrant bickering:
https://patchwork.kernel.org/project/dm-devel/list/?series=598823

Jens, this incremental weakening of what it is that DM is allowed to
do is not something I can continue to work with (nor should Ming's or
others' contributions be rejected for such reasons).

This tribal war needs to stop.

Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 1/3] block: split having srcu from queue blocking

[Jeff Moyer]

Ming Lei <ming.lei@redhat.com> writes:

> Now we reuse queue flag of QUEUE_FLAG_HAS_SRCU for both having srcu and
> BLK_MQ_F_BLOCKING. Actually they are two things: one is that srcu is
> allocated inside queue, another is that we need to handle blocking
> ->queue_rq. So far this way works as expected.
>
> dm-rq needs to set BLK_MQ_F_BLOCKING if any underlying queue is
> marked as BLK_MQ_F_BLOCKING. But dm queue is allocated before tagset
> is allocated, one doable way is to always allocate SRCU for dm
> queue, then set BLK_MQ_F_BLOCKING for the tagset if it is required,
> meantime we can mark the request queue as supporting blocking
> ->queue_rq.
>
> So add one new flag of QUEUE_FLAG_BLOCKING for supporting blocking
> ->queue_rq only, and use one private field to describe if request
> queue has allocated srcu instance.

OK, so you switched to has_srcu because it's an internaly only detail,
that makes sense.  I think testing for blocking makes more sense than
testing for the existence of srcu, so this actually makes the code a bit
more readable in my opinion.

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-core.c       | 2 +-
>  block/blk-mq.c         | 6 +++---
>  block/blk-mq.h         | 2 +-
>  block/blk-sysfs.c      | 2 +-
>  include/linux/blkdev.h | 5 +++--
>  5 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 10619fd83c1b..7ba806a4e779 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -449,7 +449,7 @@ struct request_queue *blk_alloc_queue(int node_id, bool alloc_srcu)
>  		return NULL;
>  
>  	if (alloc_srcu) {
> -		blk_queue_flag_set(QUEUE_FLAG_HAS_SRCU, q);
> +		q->has_srcu = true;
>  		if (init_srcu_struct(q->srcu) != 0)
>  			goto fail_q;
>  	}
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 0d7c9d3e0329..1408a6b8ccdc 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -259,7 +259,7 @@ EXPORT_SYMBOL_GPL(blk_mq_quiesce_queue_nowait);
>   */
>  void blk_mq_wait_quiesce_done(struct request_queue *q)
>  {
> -	if (blk_queue_has_srcu(q))
> +	if (blk_queue_blocking(q))
>  		synchronize_srcu(q->srcu);
>  	else
>  		synchronize_rcu();
> @@ -4024,8 +4024,8 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
>  int blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
>  		struct request_queue *q)
>  {
> -	WARN_ON_ONCE(blk_queue_has_srcu(q) !=
> -			!!(set->flags & BLK_MQ_F_BLOCKING));
> +	if (set->flags & BLK_MQ_F_BLOCKING)
> +		blk_queue_flag_set(QUEUE_FLAG_BLOCKING, q);
>  
>  	/* mark the queue as mq asap */
>  	q->mq_ops = set->ops;
> diff --git a/block/blk-mq.h b/block/blk-mq.h
> index 948791ea2a3e..9601918e2034 100644
> --- a/block/blk-mq.h
> +++ b/block/blk-mq.h
> @@ -377,7 +377,7 @@ static inline bool hctx_may_queue(struct blk_mq_hw_ctx *hctx,
>  /* run the code block in @dispatch_ops with rcu/srcu read lock held */
>  #define __blk_mq_run_dispatch_ops(q, check_sleep, dispatch_ops)	\
>  do {								\
> -	if (!blk_queue_has_srcu(q)) {				\
> +	if (!blk_queue_blocking(q)) {				\
>  		rcu_read_lock();				\
>  		(dispatch_ops);					\
>  		rcu_read_unlock();				\
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index e20eadfcf5c8..af89fabb58e3 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -736,7 +736,7 @@ static void blk_free_queue_rcu(struct rcu_head *rcu_head)
>  	struct request_queue *q = container_of(rcu_head, struct request_queue,
>  					       rcu_head);
>  
> -	kmem_cache_free(blk_get_queue_kmem_cache(blk_queue_has_srcu(q)), q);
> +	kmem_cache_free(blk_get_queue_kmem_cache(q->has_srcu), q);
>  }
>  
>  /* Unconfigure the I/O scheduler and dissociate from the cgroup controller. */
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index c80cfaefc0a8..d84abdb294c4 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -365,6 +365,7 @@ struct request_queue {
>  #endif
>  
>  	bool			mq_sysfs_init_done;
> +	bool			has_srcu;
>  
>  #define BLK_MAX_WRITE_HINTS	5
>  	u64			write_hints[BLK_MAX_WRITE_HINTS];
> @@ -385,7 +386,7 @@ struct request_queue {
>  /* Keep blk_queue_flag_name[] in sync with the definitions below */
>  #define QUEUE_FLAG_STOPPED	0	/* queue is stopped */
>  #define QUEUE_FLAG_DYING	1	/* queue being torn down */
> -#define QUEUE_FLAG_HAS_SRCU	2	/* SRCU is allocated */
> +#define QUEUE_FLAG_BLOCKING	2	/* ->queue_rq may block */
>  #define QUEUE_FLAG_NOMERGES     3	/* disable merge attempts */
>  #define QUEUE_FLAG_SAME_COMP	4	/* complete on same CPU-group */
>  #define QUEUE_FLAG_FAIL_IO	5	/* fake timeout */
> @@ -423,7 +424,7 @@ bool blk_queue_flag_test_and_set(unsigned int flag, struct request_queue *q);
>  
>  #define blk_queue_stopped(q)	test_bit(QUEUE_FLAG_STOPPED, &(q)->queue_flags)
>  #define blk_queue_dying(q)	test_bit(QUEUE_FLAG_DYING, &(q)->queue_flags)
> -#define blk_queue_has_srcu(q)	test_bit(QUEUE_FLAG_HAS_SRCU, &(q)->queue_flags)
> +#define blk_queue_blocking(q)	test_bit(QUEUE_FLAG_BLOCKING, &(q)->queue_flags)
>  #define blk_queue_dead(q)	test_bit(QUEUE_FLAG_DEAD, &(q)->queue_flags)
>  #define blk_queue_init_done(q)	test_bit(QUEUE_FLAG_INIT_DONE, &(q)->queue_flags)
>  #define blk_queue_nomerges(q)	test_bit(QUEUE_FLAG_NOMERGES, &(q)->queue_flags)

Re: [dm-devel] [PATCH 1/3] block: split having srcu from queue blocking

[Jeff Moyer]

Ming Lei <ming.lei@redhat.com> writes:

> Now we reuse queue flag of QUEUE_FLAG_HAS_SRCU for both having srcu and
> BLK_MQ_F_BLOCKING. Actually they are two things: one is that srcu is
> allocated inside queue, another is that we need to handle blocking
> ->queue_rq. So far this way works as expected.
>
> dm-rq needs to set BLK_MQ_F_BLOCKING if any underlying queue is
> marked as BLK_MQ_F_BLOCKING. But dm queue is allocated before tagset
> is allocated, one doable way is to always allocate SRCU for dm
> queue, then set BLK_MQ_F_BLOCKING for the tagset if it is required,
> meantime we can mark the request queue as supporting blocking
> ->queue_rq.
>
> So add one new flag of QUEUE_FLAG_BLOCKING for supporting blocking
> ->queue_rq only, and use one private field to describe if request
> queue has allocated srcu instance.

OK, so you switched to has_srcu because it's an internaly only detail,
that makes sense.  I think testing for blocking makes more sense than
testing for the existence of srcu, so this actually makes the code a bit
more readable in my opinion.

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-core.c       | 2 +-
>  block/blk-mq.c         | 6 +++---
>  block/blk-mq.h         | 2 +-
>  block/blk-sysfs.c      | 2 +-
>  include/linux/blkdev.h | 5 +++--
>  5 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 10619fd83c1b..7ba806a4e779 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -449,7 +449,7 @@ struct request_queue *blk_alloc_queue(int node_id, bool alloc_srcu)
>  		return NULL;
>  
>  	if (alloc_srcu) {
> -		blk_queue_flag_set(QUEUE_FLAG_HAS_SRCU, q);
> +		q->has_srcu = true;
>  		if (init_srcu_struct(q->srcu) != 0)
>  			goto fail_q;
>  	}
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 0d7c9d3e0329..1408a6b8ccdc 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -259,7 +259,7 @@ EXPORT_SYMBOL_GPL(blk_mq_quiesce_queue_nowait);
>   */
>  void blk_mq_wait_quiesce_done(struct request_queue *q)
>  {
> -	if (blk_queue_has_srcu(q))
> +	if (blk_queue_blocking(q))
>  		synchronize_srcu(q->srcu);
>  	else
>  		synchronize_rcu();
> @@ -4024,8 +4024,8 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
>  int blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
>  		struct request_queue *q)
>  {
> -	WARN_ON_ONCE(blk_queue_has_srcu(q) !=
> -			!!(set->flags & BLK_MQ_F_BLOCKING));
> +	if (set->flags & BLK_MQ_F_BLOCKING)
> +		blk_queue_flag_set(QUEUE_FLAG_BLOCKING, q);
>  
>  	/* mark the queue as mq asap */
>  	q->mq_ops = set->ops;
> diff --git a/block/blk-mq.h b/block/blk-mq.h
> index 948791ea2a3e..9601918e2034 100644
> --- a/block/blk-mq.h
> +++ b/block/blk-mq.h
> @@ -377,7 +377,7 @@ static inline bool hctx_may_queue(struct blk_mq_hw_ctx *hctx,
>  /* run the code block in @dispatch_ops with rcu/srcu read lock held */
>  #define __blk_mq_run_dispatch_ops(q, check_sleep, dispatch_ops)	\
>  do {								\
> -	if (!blk_queue_has_srcu(q)) {				\
> +	if (!blk_queue_blocking(q)) {				\
>  		rcu_read_lock();				\
>  		(dispatch_ops);					\
>  		rcu_read_unlock();				\
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index e20eadfcf5c8..af89fabb58e3 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -736,7 +736,7 @@ static void blk_free_queue_rcu(struct rcu_head *rcu_head)
>  	struct request_queue *q = container_of(rcu_head, struct request_queue,
>  					       rcu_head);
>  
> -	kmem_cache_free(blk_get_queue_kmem_cache(blk_queue_has_srcu(q)), q);
> +	kmem_cache_free(blk_get_queue_kmem_cache(q->has_srcu), q);
>  }
>  
>  /* Unconfigure the I/O scheduler and dissociate from the cgroup controller. */
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index c80cfaefc0a8..d84abdb294c4 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -365,6 +365,7 @@ struct request_queue {
>  #endif
>  
>  	bool			mq_sysfs_init_done;
> +	bool			has_srcu;
>  
>  #define BLK_MAX_WRITE_HINTS	5
>  	u64			write_hints[BLK_MAX_WRITE_HINTS];
> @@ -385,7 +386,7 @@ struct request_queue {
>  /* Keep blk_queue_flag_name[] in sync with the definitions below */
>  #define QUEUE_FLAG_STOPPED	0	/* queue is stopped */
>  #define QUEUE_FLAG_DYING	1	/* queue being torn down */
> -#define QUEUE_FLAG_HAS_SRCU	2	/* SRCU is allocated */
> +#define QUEUE_FLAG_BLOCKING	2	/* ->queue_rq may block */
>  #define QUEUE_FLAG_NOMERGES     3	/* disable merge attempts */
>  #define QUEUE_FLAG_SAME_COMP	4	/* complete on same CPU-group */
>  #define QUEUE_FLAG_FAIL_IO	5	/* fake timeout */
> @@ -423,7 +424,7 @@ bool blk_queue_flag_test_and_set(unsigned int flag, struct request_queue *q);
>  
>  #define blk_queue_stopped(q)	test_bit(QUEUE_FLAG_STOPPED, &(q)->queue_flags)
>  #define blk_queue_dying(q)	test_bit(QUEUE_FLAG_DYING, &(q)->queue_flags)
> -#define blk_queue_has_srcu(q)	test_bit(QUEUE_FLAG_HAS_SRCU, &(q)->queue_flags)
> +#define blk_queue_blocking(q)	test_bit(QUEUE_FLAG_BLOCKING, &(q)->queue_flags)
>  #define blk_queue_dead(q)	test_bit(QUEUE_FLAG_DEAD, &(q)->queue_flags)
>  #define blk_queue_init_done(q)	test_bit(QUEUE_FLAG_INIT_DONE, &(q)->queue_flags)
>  #define blk_queue_nomerges(q)	test_bit(QUEUE_FLAG_NOMERGES, &(q)->queue_flags)

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 2/3] block: add blk_alloc_disk_srcu

[Jeff Moyer]

Ming Lei <ming.lei@redhat.com> writes:

> Add blk_alloc_disk_srcu() so that we can allocate srcu inside request queue
> for supporting blocking ->queue_rq().
>
> dm-rq needs this API.
>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

> ---
>  block/genhd.c         |  5 +++--
>  include/linux/genhd.h | 12 ++++++++----
>  2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/block/genhd.c b/block/genhd.c
> index 3c139a1b6f04..d21786fbb7bb 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -1333,12 +1333,13 @@ struct gendisk *__alloc_disk_node(struct request_queue *q, int node_id,
>  }
>  EXPORT_SYMBOL(__alloc_disk_node);
>  
> -struct gendisk *__blk_alloc_disk(int node, struct lock_class_key *lkclass)
> +struct gendisk *__blk_alloc_disk(int node, bool alloc_srcu,
> +		struct lock_class_key *lkclass)
>  {
>  	struct request_queue *q;
>  	struct gendisk *disk;
>  
> -	q = blk_alloc_queue(node, false);
> +	q = blk_alloc_queue(node, alloc_srcu);
>  	if (!q)
>  		return NULL;
>  
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index 6906a45bc761..20259340b962 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -227,23 +227,27 @@ void blk_drop_partitions(struct gendisk *disk);
>  struct gendisk *__alloc_disk_node(struct request_queue *q, int node_id,
>  		struct lock_class_key *lkclass);
>  extern void put_disk(struct gendisk *disk);
> -struct gendisk *__blk_alloc_disk(int node, struct lock_class_key *lkclass);
> +struct gendisk *__blk_alloc_disk(int node, bool alloc_srcu,
> +		struct lock_class_key *lkclass);
>  
>  /**
> - * blk_alloc_disk - allocate a gendisk structure
> + * __alloc_disk - allocate a gendisk structure
>   * @node_id: numa node to allocate on
> + * @alloc_srcu: allocate srcu instance for supporting blocking ->queue_rq
>   *
>   * Allocate and pre-initialize a gendisk structure for use with BIO based
>   * drivers.
>   *
>   * Context: can sleep
>   */
> -#define blk_alloc_disk(node_id)						\
> +#define __alloc_disk(node_id, alloc_srcu)				\
>  ({									\
>  	static struct lock_class_key __key;				\
>  									\
> -	__blk_alloc_disk(node_id, &__key);				\
> +	__blk_alloc_disk(node_id, alloc_srcu, &__key);			\
>  })
> +#define blk_alloc_disk(node_id) __alloc_disk(node_id, false)
> +#define blk_alloc_disk_srcu(node_id) __alloc_disk(node_id, true)
>  void blk_cleanup_disk(struct gendisk *disk);
>  
>  int __register_blkdev(unsigned int major, const char *name,

Re: [dm-devel] [PATCH 2/3] block: add blk_alloc_disk_srcu

[Jeff Moyer]

Ming Lei <ming.lei@redhat.com> writes:

> Add blk_alloc_disk_srcu() so that we can allocate srcu inside request queue
> for supporting blocking ->queue_rq().
>
> dm-rq needs this API.
>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

> ---
>  block/genhd.c         |  5 +++--
>  include/linux/genhd.h | 12 ++++++++----
>  2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/block/genhd.c b/block/genhd.c
> index 3c139a1b6f04..d21786fbb7bb 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -1333,12 +1333,13 @@ struct gendisk *__alloc_disk_node(struct request_queue *q, int node_id,
>  }
>  EXPORT_SYMBOL(__alloc_disk_node);
>  
> -struct gendisk *__blk_alloc_disk(int node, struct lock_class_key *lkclass)
> +struct gendisk *__blk_alloc_disk(int node, bool alloc_srcu,
> +		struct lock_class_key *lkclass)
>  {
>  	struct request_queue *q;
>  	struct gendisk *disk;
>  
> -	q = blk_alloc_queue(node, false);
> +	q = blk_alloc_queue(node, alloc_srcu);
>  	if (!q)
>  		return NULL;
>  
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index 6906a45bc761..20259340b962 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -227,23 +227,27 @@ void blk_drop_partitions(struct gendisk *disk);
>  struct gendisk *__alloc_disk_node(struct request_queue *q, int node_id,
>  		struct lock_class_key *lkclass);
>  extern void put_disk(struct gendisk *disk);
> -struct gendisk *__blk_alloc_disk(int node, struct lock_class_key *lkclass);
> +struct gendisk *__blk_alloc_disk(int node, bool alloc_srcu,
> +		struct lock_class_key *lkclass);
>  
>  /**
> - * blk_alloc_disk - allocate a gendisk structure
> + * __alloc_disk - allocate a gendisk structure
>   * @node_id: numa node to allocate on
> + * @alloc_srcu: allocate srcu instance for supporting blocking ->queue_rq
>   *
>   * Allocate and pre-initialize a gendisk structure for use with BIO based
>   * drivers.
>   *
>   * Context: can sleep
>   */
> -#define blk_alloc_disk(node_id)						\
> +#define __alloc_disk(node_id, alloc_srcu)				\
>  ({									\
>  	static struct lock_class_key __key;				\
>  									\
> -	__blk_alloc_disk(node_id, &__key);				\
> +	__blk_alloc_disk(node_id, alloc_srcu, &__key);			\
>  })
> +#define blk_alloc_disk(node_id) __alloc_disk(node_id, false)
> +#define blk_alloc_disk_srcu(node_id) __alloc_disk(node_id, true)
>  void blk_cleanup_disk(struct gendisk *disk);
>  
>  int __register_blkdev(unsigned int major, const char *name,

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 3/3] dm: mark dm queue as blocking if any underlying is blocking

[Jeff Moyer]

Ming Lei <ming.lei@redhat.com> writes:

> dm request based driver doesn't set BLK_MQ_F_BLOCKING, so dm_queue_rq()
> is supposed to not sleep.
>
> However, blk_insert_cloned_request() is used by dm_queue_rq() for
> queuing underlying request, but the underlying queue may be marked as
> BLK_MQ_F_BLOCKING, so blk_insert_cloned_request() may become to block
> current context, then rcu warning is triggered.
>
> Fixes the issue by marking dm request based queue as BLK_MQ_F_BLOCKING
> if any underlying queue is marked as BLK_MQ_F_BLOCKING, meantime we
> need to allocate srcu beforehand.
>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Acked-by: Jeff Moyer <jmoyer@redhat.com>

> ---
>  drivers/md/dm-rq.c    |  5 ++++-
>  drivers/md/dm-rq.h    |  3 ++-
>  drivers/md/dm-table.c | 14 ++++++++++++++
>  drivers/md/dm.c       |  5 +++--
>  drivers/md/dm.h       |  1 +
>  5 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> index 579ab6183d4d..2297d37c62a9 100644
> --- a/drivers/md/dm-rq.c
> +++ b/drivers/md/dm-rq.c
> @@ -535,7 +535,8 @@ static const struct blk_mq_ops dm_mq_ops = {
>  	.init_request = dm_mq_init_request,
>  };
>  
> -int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
> +int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t,
> +			     bool blocking)
>  {
>  	struct dm_target *immutable_tgt;
>  	int err;
> @@ -550,6 +551,8 @@ int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
>  	md->tag_set->flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_STACKING;
>  	md->tag_set->nr_hw_queues = dm_get_blk_mq_nr_hw_queues();
>  	md->tag_set->driver_data = md;
> +	if (blocking)
> +		md->tag_set->flags |= BLK_MQ_F_BLOCKING;
>  
>  	md->tag_set->cmd_size = sizeof(struct dm_rq_target_io);
>  	immutable_tgt = dm_table_get_immutable_target(t);
> diff --git a/drivers/md/dm-rq.h b/drivers/md/dm-rq.h
> index 1eea0da641db..5f3729f277d7 100644
> --- a/drivers/md/dm-rq.h
> +++ b/drivers/md/dm-rq.h
> @@ -30,7 +30,8 @@ struct dm_rq_clone_bio_info {
>  	struct bio clone;
>  };
>  
> -int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t);
> +int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t,
> +			     bool blocking);
>  void dm_mq_cleanup_mapped_device(struct mapped_device *md);
>  
>  void dm_start_queue(struct request_queue *q);
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index aa173f5bdc3d..e4bdd4f757a3 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -1875,6 +1875,20 @@ static bool dm_table_supports_write_zeroes(struct dm_table *t)
>  	return true;
>  }
>  
> +/* If the device can block inside ->queue_rq */
> +static int device_is_io_blocking(struct dm_target *ti, struct dm_dev *dev,
> +			      sector_t start, sector_t len, void *data)
> +{
> +	struct request_queue *q = bdev_get_queue(dev->bdev);
> +
> +	return blk_queue_blocking(q);
> +}
> +
> +bool dm_table_has_blocking_dev(struct dm_table *t)
> +{
> +	return dm_table_any_dev_attr(t, device_is_io_blocking, NULL);
> +}
> +
>  static int device_not_nowait_capable(struct dm_target *ti, struct dm_dev *dev,
>  				     sector_t start, sector_t len, void *data)
>  {
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 280918cdcabd..2f72877752dd 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1761,7 +1761,7 @@ static struct mapped_device *alloc_dev(int minor)
>  	 * established. If request-based table is loaded: blk-mq will
>  	 * override accordingly.
>  	 */
> -	md->disk = blk_alloc_disk(md->numa_node_id);
> +	md->disk = blk_alloc_disk_srcu(md->numa_node_id);
>  	if (!md->disk)
>  		goto bad;
>  	md->queue = md->disk->queue;
> @@ -2046,7 +2046,8 @@ int dm_setup_md_queue(struct mapped_device *md, struct dm_table *t)
>  	switch (type) {
>  	case DM_TYPE_REQUEST_BASED:
>  		md->disk->fops = &dm_rq_blk_dops;
> -		r = dm_mq_init_request_queue(md, t);
> +		r = dm_mq_init_request_queue(md, t,
> +				dm_table_has_blocking_dev(t));
>  		if (r) {
>  			DMERR("Cannot initialize queue for request-based dm mapped device");
>  			return r;
> diff --git a/drivers/md/dm.h b/drivers/md/dm.h
> index 742d9c80efe1..f7f92b272cce 100644
> --- a/drivers/md/dm.h
> +++ b/drivers/md/dm.h
> @@ -60,6 +60,7 @@ int dm_calculate_queue_limits(struct dm_table *table,
>  			      struct queue_limits *limits);
>  int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
>  			      struct queue_limits *limits);
> +bool dm_table_has_blocking_dev(struct dm_table *t);
>  struct list_head *dm_table_get_devices(struct dm_table *t);
>  void dm_table_presuspend_targets(struct dm_table *t);
>  void dm_table_presuspend_undo_targets(struct dm_table *t);

Re: [dm-devel] [PATCH 3/3] dm: mark dm queue as blocking if any underlying is blocking

[Jeff Moyer]

Ming Lei <ming.lei@redhat.com> writes:

> dm request based driver doesn't set BLK_MQ_F_BLOCKING, so dm_queue_rq()
> is supposed to not sleep.
>
> However, blk_insert_cloned_request() is used by dm_queue_rq() for
> queuing underlying request, but the underlying queue may be marked as
> BLK_MQ_F_BLOCKING, so blk_insert_cloned_request() may become to block
> current context, then rcu warning is triggered.
>
> Fixes the issue by marking dm request based queue as BLK_MQ_F_BLOCKING
> if any underlying queue is marked as BLK_MQ_F_BLOCKING, meantime we
> need to allocate srcu beforehand.
>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Acked-by: Jeff Moyer <jmoyer@redhat.com>

> ---
>  drivers/md/dm-rq.c    |  5 ++++-
>  drivers/md/dm-rq.h    |  3 ++-
>  drivers/md/dm-table.c | 14 ++++++++++++++
>  drivers/md/dm.c       |  5 +++--
>  drivers/md/dm.h       |  1 +
>  5 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> index 579ab6183d4d..2297d37c62a9 100644
> --- a/drivers/md/dm-rq.c
> +++ b/drivers/md/dm-rq.c
> @@ -535,7 +535,8 @@ static const struct blk_mq_ops dm_mq_ops = {
>  	.init_request = dm_mq_init_request,
>  };
>  
> -int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
> +int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t,
> +			     bool blocking)
>  {
>  	struct dm_target *immutable_tgt;
>  	int err;
> @@ -550,6 +551,8 @@ int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
>  	md->tag_set->flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_STACKING;
>  	md->tag_set->nr_hw_queues = dm_get_blk_mq_nr_hw_queues();
>  	md->tag_set->driver_data = md;
> +	if (blocking)
> +		md->tag_set->flags |= BLK_MQ_F_BLOCKING;
>  
>  	md->tag_set->cmd_size = sizeof(struct dm_rq_target_io);
>  	immutable_tgt = dm_table_get_immutable_target(t);
> diff --git a/drivers/md/dm-rq.h b/drivers/md/dm-rq.h
> index 1eea0da641db..5f3729f277d7 100644
> --- a/drivers/md/dm-rq.h
> +++ b/drivers/md/dm-rq.h
> @@ -30,7 +30,8 @@ struct dm_rq_clone_bio_info {
>  	struct bio clone;
>  };
>  
> -int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t);
> +int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t,
> +			     bool blocking);
>  void dm_mq_cleanup_mapped_device(struct mapped_device *md);
>  
>  void dm_start_queue(struct request_queue *q);
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index aa173f5bdc3d..e4bdd4f757a3 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -1875,6 +1875,20 @@ static bool dm_table_supports_write_zeroes(struct dm_table *t)
>  	return true;
>  }
>  
> +/* If the device can block inside ->queue_rq */
> +static int device_is_io_blocking(struct dm_target *ti, struct dm_dev *dev,
> +			      sector_t start, sector_t len, void *data)
> +{
> +	struct request_queue *q = bdev_get_queue(dev->bdev);
> +
> +	return blk_queue_blocking(q);
> +}
> +
> +bool dm_table_has_blocking_dev(struct dm_table *t)
> +{
> +	return dm_table_any_dev_attr(t, device_is_io_blocking, NULL);
> +}
> +
>  static int device_not_nowait_capable(struct dm_target *ti, struct dm_dev *dev,
>  				     sector_t start, sector_t len, void *data)
>  {
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 280918cdcabd..2f72877752dd 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1761,7 +1761,7 @@ static struct mapped_device *alloc_dev(int minor)
>  	 * established. If request-based table is loaded: blk-mq will
>  	 * override accordingly.
>  	 */
> -	md->disk = blk_alloc_disk(md->numa_node_id);
> +	md->disk = blk_alloc_disk_srcu(md->numa_node_id);
>  	if (!md->disk)
>  		goto bad;
>  	md->queue = md->disk->queue;
> @@ -2046,7 +2046,8 @@ int dm_setup_md_queue(struct mapped_device *md, struct dm_table *t)
>  	switch (type) {
>  	case DM_TYPE_REQUEST_BASED:
>  		md->disk->fops = &dm_rq_blk_dops;
> -		r = dm_mq_init_request_queue(md, t);
> +		r = dm_mq_init_request_queue(md, t,
> +				dm_table_has_blocking_dev(t));
>  		if (r) {
>  			DMERR("Cannot initialize queue for request-based dm mapped device");
>  			return r;
> diff --git a/drivers/md/dm.h b/drivers/md/dm.h
> index 742d9c80efe1..f7f92b272cce 100644
> --- a/drivers/md/dm.h
> +++ b/drivers/md/dm.h
> @@ -60,6 +60,7 @@ int dm_calculate_queue_limits(struct dm_table *table,
>  			      struct queue_limits *limits);
>  int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
>  			      struct queue_limits *limits);
> +bool dm_table_has_blocking_dev(struct dm_table *t);
>  struct list_head *dm_table_get_devices(struct dm_table *t);
>  void dm_table_presuspend_targets(struct dm_table *t);
>  void dm_table_presuspend_undo_targets(struct dm_table *t);

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Jeff Moyer]

Christoph Hellwig <hch@infradead.org> writes:

> On Tue, Dec 28, 2021 at 04:30:08PM -0500, Mike Snitzer wrote:
>> Yeah, people use request-based for IO scheduling and more capable path
>> selectors. Imposing bio-based would be a pretty jarring workaround for 
>> BLK_MQ_F_BLOCKING. request-based DM should properly support it.
>
> Given that nvme-tcp is the only blocking driver that has multipath
> driver that driver explicitly does not intend to support dm-multipath
> I'm absolutely against adding block layer cruft for this particular
> use case.

Maybe I have bad taste, but the patches didn't look like cruft to me.
:)

I'm not sure why we'd prevent users from using dm-mpath on nvmeof.  I
think there's agreement that the nvme native multipath implementation is
the preferred way (that's the default in rhel9, even), but I don't think
that's a reason to nack this patch set.

Or have I missed your point entirely?

Thanks!
Jeff

Re: [dm-devel] [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Jeff Moyer]

Christoph Hellwig <hch@infradead.org> writes:

> On Tue, Dec 28, 2021 at 04:30:08PM -0500, Mike Snitzer wrote:
>> Yeah, people use request-based for IO scheduling and more capable path
>> selectors. Imposing bio-based would be a pretty jarring workaround for 
>> BLK_MQ_F_BLOCKING. request-based DM should properly support it.
>
> Given that nvme-tcp is the only blocking driver that has multipath
> driver that driver explicitly does not intend to support dm-multipath
> I'm absolutely against adding block layer cruft for this particular
> use case.

Maybe I have bad taste, but the patches didn't look like cruft to me.
:)

I'm not sure why we'd prevent users from using dm-mpath on nvmeof.  I
think there's agreement that the nvme native multipath implementation is
the preferred way (that's the default in rhel9, even), but I don't think
that's a reason to nack this patch set.

Or have I missed your point entirely?

Thanks!
Jeff

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Christoph Hellwig]

On Tue, Jan 11, 2022 at 11:15:09AM -0500, Mike Snitzer wrote:
> Round and round we go.. Pretty tired of this.

Same here.

> You are perfectly fine with incrementally compromising request-based
> DM's ability to evolve as block core does.

I would not word it that way, but I think we mean the same thing.  Yes,
I do not want to add more hooks for a complete oddball that has no
actual use case.  dm-rq does a good enough job for SCSI and has all
the infrastucture for it.  We should not more cruft to exteded it to
use cases for which there is no consesus and which are much better
covered by alredy existing code in the kernel.

Re: [dm-devel] [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Christoph Hellwig]

On Tue, Jan 11, 2022 at 11:15:09AM -0500, Mike Snitzer wrote:
> Round and round we go.. Pretty tired of this.

Same here.

> You are perfectly fine with incrementally compromising request-based
> DM's ability to evolve as block core does.

I would not word it that way, but I think we mean the same thing.  Yes,
I do not want to add more hooks for a complete oddball that has no
actual use case.  dm-rq does a good enough job for SCSI and has all
the infrastucture for it.  We should not more cruft to exteded it to
use cases for which there is no consesus and which are much better
covered by alredy existing code in the kernel.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Christoph Hellwig]

On Tue, Jan 11, 2022 at 01:23:53PM -0500, Jeff Moyer wrote:
> Maybe I have bad taste, but the patches didn't look like cruft to me.
> :)

They do to me.  The extend the corner case of request on request
stacking that already is a bit of mess even more by adding yet another
special case in the block layer.

> 
> I'm not sure why we'd prevent users from using dm-mpath on nvmeof.  I
> think there's agreement that the nvme native multipath implementation is
> the preferred way (that's the default in rhel9, even), but I don't think
> that's a reason to nack this patch set.
> 
> Or have I missed your point entirely?

No you have not missed the point.  nvme-multipath exists longer than
the nvme-tcp driver and is the only supported one for it upstream for
a good reason.  If RedHat wants to do their own weirdo setups they can
patch their kernels, but please leave the upstrem code alone.

Re: [dm-devel] [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Christoph Hellwig]

On Tue, Jan 11, 2022 at 01:23:53PM -0500, Jeff Moyer wrote:
> Maybe I have bad taste, but the patches didn't look like cruft to me.
> :)

They do to me.  The extend the corner case of request on request
stacking that already is a bit of mess even more by adding yet another
special case in the block layer.

> 
> I'm not sure why we'd prevent users from using dm-mpath on nvmeof.  I
> think there's agreement that the nvme native multipath implementation is
> the preferred way (that's the default in rhel9, even), but I don't think
> that's a reason to nack this patch set.
> 
> Or have I missed your point entirely?

No you have not missed the point.  nvme-multipath exists longer than
the nvme-tcp driver and is the only supported one for it upstream for
a good reason.  If RedHat wants to do their own weirdo setups they can
patch their kernels, but please leave the upstrem code alone.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Mike Snitzer]

On Mon, Jan 17 2022 at  3:10P -0500,
Christoph Hellwig <hch@infradead.org> wrote:

> On Tue, Jan 11, 2022 at 01:23:53PM -0500, Jeff Moyer wrote:
> > Maybe I have bad taste, but the patches didn't look like cruft to me.
> > :)
> 
> They do to me.  The extend the corner case of request on request
> stacking that already is a bit of mess even more by adding yet another
> special case in the block layer.

Ming's first patch:
https://patchwork.kernel.org/project/dm-devel/patch/20211221141459.1368176-2-ming.lei@redhat.com/
is pure cleanup for the mess that went in this merge cycle.

All that dm-rq context aside, Ming's 1st patch is the correct way to
clean up the block core flags/state (internal vs external, etc).

But the 2nd paragraph of that first patch's header should be moved to
Ming's 2nd patch because it explains why DM needs the new
blk_alloc_disk_srcu() interface, e.g.:
"But dm queue is allocated before tagset is allocated"
(so it confirms it best to explain that in 2nd patch).

But really even Ming's 2nd patch:
https://patchwork.kernel.org/project/dm-devel/patch/20211221141459.1368176-3-ming.lei@redhat.com/
should _not_ need to be debated like this.

Fact is alloc_disk() has always mirrored blk_alloc_queue()'s args.  So
Ming's 2nd patch should be done anyway to expose meaningful control
over request_queue allocation.  If anything, the 2nd patch should just
add the 'alloc_srcu' arg to blk_alloc_disk() and change all but NVMe
callers to pass false.

Put another way: when the 'node_id' arg was added to blk_alloc_queue()
a new blk_alloc_disk_numa_node() wasn't added (despite most block
drivers still only using NUMA_NO_NODE).  This new 'alloc_srcu' flag is
seen to be more niche, but it really should be no different on an
interface symmetry and design level.

> > I'm not sure why we'd prevent users from using dm-mpath on nvmeof.  I
> > think there's agreement that the nvme native multipath implementation is
> > the preferred way (that's the default in rhel9, even), but I don't think
> > that's a reason to nack this patch set.
> > 
> > Or have I missed your point entirely?
> 
> No you have not missed the point.  nvme-multipath exists longer than
> the nvme-tcp driver and is the only supported one for it upstream for
> a good reason.  If RedHat wants to do their own weirdo setups they can
> patch their kernels, but please leave the upstrem code alone.

Patch 3 can be left out if you'd like to force your world view on
everyone... you've already "won", _pretty please_ stop being so
punitive by blocking reasonable change.  We really can get along if
we're all willing to be intellectually honest.

To restate: Ming's patches 1 and 2 really are not "cruft".  They
expose control over request_queue allocation that should be accessible
by both blk_alloc_queue() and blk_alloc_disk().

Mike

Re: [dm-devel] [PATCH 0/3] blk-mq/dm-rq: support BLK_MQ_F_BLOCKING for dm-rq

[Mike Snitzer]

On Mon, Jan 17 2022 at  3:10P -0500,
Christoph Hellwig <hch@infradead.org> wrote:

> On Tue, Jan 11, 2022 at 01:23:53PM -0500, Jeff Moyer wrote:
> > Maybe I have bad taste, but the patches didn't look like cruft to me.
> > :)
> 
> They do to me.  The extend the corner case of request on request
> stacking that already is a bit of mess even more by adding yet another
> special case in the block layer.

Ming's first patch:
https://patchwork.kernel.org/project/dm-devel/patch/20211221141459.1368176-2-ming.lei@redhat.com/
is pure cleanup for the mess that went in this merge cycle.

All that dm-rq context aside, Ming's 1st patch is the correct way to
clean up the block core flags/state (internal vs external, etc).

But the 2nd paragraph of that first patch's header should be moved to
Ming's 2nd patch because it explains why DM needs the new
blk_alloc_disk_srcu() interface, e.g.:
"But dm queue is allocated before tagset is allocated"
(so it confirms it best to explain that in 2nd patch).

But really even Ming's 2nd patch:
https://patchwork.kernel.org/project/dm-devel/patch/20211221141459.1368176-3-ming.lei@redhat.com/
should _not_ need to be debated like this.

Fact is alloc_disk() has always mirrored blk_alloc_queue()'s args.  So
Ming's 2nd patch should be done anyway to expose meaningful control
over request_queue allocation.  If anything, the 2nd patch should just
add the 'alloc_srcu' arg to blk_alloc_disk() and change all but NVMe
callers to pass false.

Put another way: when the 'node_id' arg was added to blk_alloc_queue()
a new blk_alloc_disk_numa_node() wasn't added (despite most block
drivers still only using NUMA_NO_NODE).  This new 'alloc_srcu' flag is
seen to be more niche, but it really should be no different on an
interface symmetry and design level.

> > I'm not sure why we'd prevent users from using dm-mpath on nvmeof.  I
> > think there's agreement that the nvme native multipath implementation is
> > the preferred way (that's the default in rhel9, even), but I don't think
> > that's a reason to nack this patch set.
> > 
> > Or have I missed your point entirely?
> 
> No you have not missed the point.  nvme-multipath exists longer than
> the nvme-tcp driver and is the only supported one for it upstream for
> a good reason.  If RedHat wants to do their own weirdo setups they can
> patch their kernels, but please leave the upstrem code alone.

Patch 3 can be left out if you'd like to force your world view on
everyone... you've already "won", _pretty please_ stop being so
punitive by blocking reasonable change.  We really can get along if
we're all willing to be intellectually honest.

To restate: Ming's patches 1 and 2 really are not "cruft".  They
expose control over request_queue allocation that should be accessible
by both blk_alloc_queue() and blk_alloc_disk().

Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel