* [PATCH 1/2] blk-mq: register cpuhp callback after hctx is added to xarray table
2024-12-06 8:21 [PATCH 0/2] blk-mq: fix lockdep warning between sysfs_lock and cpuhotplug lock Ming Lei
@ 2024-12-06 8:21 ` Ming Lei
2024-12-06 8:21 ` [PATCH V2] null_blk: cleanup null_init_tag_set Ming Lei
` (4 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2024-12-06 8:21 UTC (permalink / raw)
To: Jens Axboe, linux-block
Cc: Ming Lei, Reinette Chatre, Fenghua Yu, Peter Newman, Babu Moger,
Luck Tony
We need to retrieve 'hctx' from xarray table in the cpuhp callback, so the
callback should be registered after this 'hctx' is added to xarray table.
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Peter Newman <peternewman@google.com>
Cc: Babu Moger <babu.moger@amd.com>
Cc: Luck Tony <tony.luck@intel.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
block/blk-mq.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 424239c075e2..a404465036de 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -3824,16 +3824,11 @@ static int blk_mq_init_hctx(struct request_queue *q,
{
hctx->queue_num = hctx_idx;
- if (!(hctx->flags & BLK_MQ_F_STACKING))
- cpuhp_state_add_instance_nocalls(CPUHP_AP_BLK_MQ_ONLINE,
- &hctx->cpuhp_online);
- cpuhp_state_add_instance_nocalls(CPUHP_BLK_MQ_DEAD, &hctx->cpuhp_dead);
-
hctx->tags = set->tags[hctx_idx];
if (set->ops->init_hctx &&
set->ops->init_hctx(hctx, set->driver_data, hctx_idx))
- goto unregister_cpu_notifier;
+ goto fail;
if (blk_mq_init_request(set, hctx->fq->flush_rq, hctx_idx,
hctx->numa_node))
@@ -3842,6 +3837,11 @@ static int blk_mq_init_hctx(struct request_queue *q,
if (xa_insert(&q->hctx_table, hctx_idx, hctx, GFP_KERNEL))
goto exit_flush_rq;
+ if (!(hctx->flags & BLK_MQ_F_STACKING))
+ cpuhp_state_add_instance_nocalls(CPUHP_AP_BLK_MQ_ONLINE,
+ &hctx->cpuhp_online);
+ cpuhp_state_add_instance_nocalls(CPUHP_BLK_MQ_DEAD, &hctx->cpuhp_dead);
+
return 0;
exit_flush_rq:
@@ -3850,8 +3850,7 @@ static int blk_mq_init_hctx(struct request_queue *q,
exit_hctx:
if (set->ops->exit_hctx)
set->ops->exit_hctx(hctx, hctx_idx);
- unregister_cpu_notifier:
- blk_mq_remove_cpuhp(hctx);
+ fail:
return -1;
}
--
2.47.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH V2] null_blk: cleanup null_init_tag_set
2024-12-06 8:21 [PATCH 0/2] blk-mq: fix lockdep warning between sysfs_lock and cpuhotplug lock Ming Lei
2024-12-06 8:21 ` [PATCH 1/2] blk-mq: register cpuhp callback after hctx is added to xarray table Ming Lei
@ 2024-12-06 8:21 ` Ming Lei
2024-12-06 8:21 ` [PATCH 1/3] ubd_drv: fix return value for COMMIT* command Ming Lei
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2024-12-06 8:21 UTC (permalink / raw)
To: Jens Axboe, linux-block; +Cc: Ming Lei, Vincent Fu
The passed 'nullb' can be NULL, so cause null ptr reference.
Fix the issue, meantime cleanup null_init_tag_set for avoiding to add
similar issue in future.
Meantime set BLK_MQ_F_NO_SCHED if g_no_sched is true in case of NULL
device, same with BLK_MQ_F_TAG_HCTX_SHARED.
Cc: Vincent Fu <vincent.fu@samsung.com>
Fixes: 37ae152c7a0d ("null_blk: add configfs variables for 2 options")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
V2:
- set BLK_MQ_F_NO_SCHED & BLK_MQ_F_TAG_HCTX_SHARED correctly in case
of null device, as suggested by Vincent Fu
drivers/block/null_blk/main.c | 53 +++++++++++++++++++++++------------
1 file changed, 35 insertions(+), 18 deletions(-)
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index c955a07dba2d..1501c85fc9e4 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -1898,31 +1898,48 @@ static int null_gendisk_register(struct nullb *nullb)
static int null_init_tag_set(struct nullb *nullb, struct blk_mq_tag_set *set)
{
+ unsigned int flags = BLK_MQ_F_SHOULD_MERGE;
+ int hw_queues, numa_node;
+ unsigned int queue_depth;
int poll_queues;
+ if (nullb) {
+ hw_queues = nullb->dev->submit_queues;
+ poll_queues = nullb->dev->poll_queues;
+ queue_depth = nullb->dev->hw_queue_depth;
+ numa_node = nullb->dev->home_node;
+ if (nullb->dev->no_sched)
+ flags |= BLK_MQ_F_NO_SCHED;
+ if (nullb->dev->shared_tag_bitmap)
+ flags |= BLK_MQ_F_TAG_HCTX_SHARED;
+ if (nullb->dev->blocking)
+ flags |= BLK_MQ_F_BLOCKING;
+ } else {
+ hw_queues = g_submit_queues;
+ poll_queues = g_poll_queues;
+ queue_depth = g_hw_queue_depth;
+ numa_node = g_home_node;
+ if (g_no_sched)
+ flags |= BLK_MQ_F_NO_SCHED;
+ if (g_shared_tag_bitmap)
+ flags |= BLK_MQ_F_TAG_HCTX_SHARED;
+ if (g_blocking)
+ flags |= BLK_MQ_F_BLOCKING;
+ }
+
set->ops = &null_mq_ops;
- set->nr_hw_queues = nullb ? nullb->dev->submit_queues :
- g_submit_queues;
- poll_queues = nullb ? nullb->dev->poll_queues : g_poll_queues;
- if (poll_queues)
- set->nr_hw_queues += poll_queues;
- set->queue_depth = nullb ? nullb->dev->hw_queue_depth :
- g_hw_queue_depth;
- set->numa_node = nullb ? nullb->dev->home_node : g_home_node;
set->cmd_size = sizeof(struct nullb_cmd);
- set->flags = BLK_MQ_F_SHOULD_MERGE;
- if (nullb->dev->no_sched)
- set->flags |= BLK_MQ_F_NO_SCHED;
- if (nullb->dev->shared_tag_bitmap)
- set->flags |= BLK_MQ_F_TAG_HCTX_SHARED;
+ set->flags = flags;
set->driver_data = nullb;
- if (poll_queues)
+ set->nr_hw_queues = hw_queues;
+ set->queue_depth = queue_depth;
+ set->numa_node = numa_node;
+ if (poll_queues) {
+ set->nr_hw_queues += poll_queues;
set->nr_maps = 3;
- else
+ } else {
set->nr_maps = 1;
-
- if ((nullb && nullb->dev->blocking) || g_blocking)
- set->flags |= BLK_MQ_F_BLOCKING;
+ }
return blk_mq_alloc_tag_set(set);
}
--
2.31.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 1/3] ubd_drv: fix return value for COMMIT* command
2024-12-06 8:21 [PATCH 0/2] blk-mq: fix lockdep warning between sysfs_lock and cpuhotplug lock Ming Lei
2024-12-06 8:21 ` [PATCH 1/2] blk-mq: register cpuhp callback after hctx is added to xarray table Ming Lei
2024-12-06 8:21 ` [PATCH V2] null_blk: cleanup null_init_tag_set Ming Lei
@ 2024-12-06 8:21 ` Ming Lei
2024-12-06 8:21 ` [PATCH 2/2] blk-mq: move cpuhp callback registering out of q->sysfs_lock Ming Lei
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2024-12-06 8:21 UTC (permalink / raw)
To: Jens Axboe, linux-block; +Cc: Ming Lei
Fixes: 6261abf48a88 ("ubd_drv: abort io command if queue is aborted")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
drivers/block/ubd_drv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/block/ubd_drv.c b/drivers/block/ubd_drv.c
index b7a59dc2229d..3b3723d78084 100644
--- a/drivers/block/ubd_drv.c
+++ b/drivers/block/ubd_drv.c
@@ -769,7 +769,7 @@ static int ubd_ch_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags)
goto out;
}
if (cmd_op == UBD_IO_COMMIT_REQ) {
- ret = ubq->aborted;
+ ret = UBD_IO_RES_ABORT;
goto out;
}
break;
--
2.31.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/2] blk-mq: move cpuhp callback registering out of q->sysfs_lock
2024-12-06 8:21 [PATCH 0/2] blk-mq: fix lockdep warning between sysfs_lock and cpuhotplug lock Ming Lei
` (2 preceding siblings ...)
2024-12-06 8:21 ` [PATCH 1/3] ubd_drv: fix return value for COMMIT* command Ming Lei
@ 2024-12-06 8:21 ` Ming Lei
2024-12-06 8:21 ` [PATCH 2/3] ubd_drv: fix io command buffer mapping Ming Lei
2024-12-06 8:24 ` [PATCH 0/2] blk-mq: fix lockdep warning between sysfs_lock and cpuhotplug lock Ming Lei
5 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2024-12-06 8:21 UTC (permalink / raw)
To: Jens Axboe, linux-block
Cc: Ming Lei, Reinette Chatre, Fenghua Yu, Peter Newman, Babu Moger,
Luck Tony
Registering and unregistering cpuhp requires global cpu hotplug lock,
which is used everywhere. Meantime q->sysfs_lock is used in block layer
almost everywhere.
It is easy to trigger lockdep warning[1] by connecting the two locks.
Fix the warning by moving blk-mq's cpuhp callback registering out of
q->sysfs_lock. Add one dedicated global lock for covering registering &
unregistering hctx's cpuhp, and it is safe to do so because hctx is
guaranteed to be live if our request_queue is live.
[1] https://lore.kernel.org/lkml/Z04pz3AlvI4o0Mr8@agluck-desk3/
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Peter Newman <peternewman@google.com>
Cc: Babu Moger <babu.moger@amd.com>
Reported-by: Luck Tony <tony.luck@intel.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
block/blk-mq.c | 103 +++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 92 insertions(+), 11 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index a404465036de..aa340b097b6e 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -43,6 +43,7 @@
static DEFINE_PER_CPU(struct llist_head, blk_cpu_done);
static DEFINE_PER_CPU(call_single_data_t, blk_cpu_csd);
+static DEFINE_MUTEX(blk_mq_cpuhp_lock);
static void blk_mq_insert_request(struct request *rq, blk_insert_t flags);
static void blk_mq_request_bypass_insert(struct request *rq,
@@ -3739,13 +3740,91 @@ static int blk_mq_hctx_notify_dead(unsigned int cpu, struct hlist_node *node)
return 0;
}
-static void blk_mq_remove_cpuhp(struct blk_mq_hw_ctx *hctx)
+static void __blk_mq_remove_cpuhp(struct blk_mq_hw_ctx *hctx)
{
- if (!(hctx->flags & BLK_MQ_F_STACKING))
+ lockdep_assert_held(&blk_mq_cpuhp_lock);
+
+ if (!(hctx->flags & BLK_MQ_F_STACKING) &&
+ !hlist_unhashed(&hctx->cpuhp_online)) {
cpuhp_state_remove_instance_nocalls(CPUHP_AP_BLK_MQ_ONLINE,
&hctx->cpuhp_online);
- cpuhp_state_remove_instance_nocalls(CPUHP_BLK_MQ_DEAD,
- &hctx->cpuhp_dead);
+ INIT_HLIST_NODE(&hctx->cpuhp_online);
+ }
+
+ if (!hlist_unhashed(&hctx->cpuhp_dead)) {
+ cpuhp_state_remove_instance_nocalls(CPUHP_BLK_MQ_DEAD,
+ &hctx->cpuhp_dead);
+ INIT_HLIST_NODE(&hctx->cpuhp_dead);
+ }
+}
+
+static void blk_mq_remove_cpuhp(struct blk_mq_hw_ctx *hctx)
+{
+ mutex_lock(&blk_mq_cpuhp_lock);
+ __blk_mq_remove_cpuhp(hctx);
+ mutex_unlock(&blk_mq_cpuhp_lock);
+}
+
+static void __blk_mq_add_cpuhp(struct blk_mq_hw_ctx *hctx)
+{
+ lockdep_assert_held(&blk_mq_cpuhp_lock);
+
+ if (!(hctx->flags & BLK_MQ_F_STACKING) &&
+ hlist_unhashed(&hctx->cpuhp_online))
+ cpuhp_state_add_instance_nocalls(CPUHP_AP_BLK_MQ_ONLINE,
+ &hctx->cpuhp_online);
+
+ if (hlist_unhashed(&hctx->cpuhp_dead))
+ cpuhp_state_add_instance_nocalls(CPUHP_BLK_MQ_DEAD,
+ &hctx->cpuhp_dead);
+}
+
+static void __blk_mq_remove_cpuhp_list(struct list_head *head)
+{
+ struct blk_mq_hw_ctx *hctx;
+
+ lockdep_assert_held(&blk_mq_cpuhp_lock);
+
+ list_for_each_entry(hctx, head, hctx_list)
+ __blk_mq_remove_cpuhp(hctx);
+}
+
+/*
+ * Unregister cpuhp callbacks from exited hw queues
+ *
+ * Safe to call if this `request_queue` is live
+ */
+static void blk_mq_remove_hw_queues_cpuhp(struct request_queue *q)
+{
+ LIST_HEAD(hctx_list);
+
+ spin_lock(&q->unused_hctx_lock);
+ list_splice_init(&q->unused_hctx_list, &hctx_list);
+ spin_unlock(&q->unused_hctx_lock);
+
+ mutex_lock(&blk_mq_cpuhp_lock);
+ __blk_mq_remove_cpuhp_list(&hctx_list);
+ mutex_unlock(&blk_mq_cpuhp_lock);
+
+ spin_lock(&q->unused_hctx_lock);
+ list_splice(&hctx_list, &q->unused_hctx_list);
+ spin_unlock(&q->unused_hctx_lock);
+}
+
+/*
+ * Register cpuhp callbacks from all hw queues
+ *
+ * Safe to call if this `request_queue` is live
+ */
+static void blk_mq_add_hw_queues_cpuhp(struct request_queue *q)
+{
+ struct blk_mq_hw_ctx *hctx;
+ unsigned long i;
+
+ mutex_lock(&blk_mq_cpuhp_lock);
+ queue_for_each_hw_ctx(q, hctx, i)
+ __blk_mq_add_cpuhp(hctx);
+ mutex_unlock(&blk_mq_cpuhp_lock);
}
/*
@@ -3796,8 +3875,6 @@ static void blk_mq_exit_hctx(struct request_queue *q,
if (set->ops->exit_hctx)
set->ops->exit_hctx(hctx, hctx_idx);
- blk_mq_remove_cpuhp(hctx);
-
xa_erase(&q->hctx_table, hctx_idx);
spin_lock(&q->unused_hctx_lock);
@@ -3814,6 +3891,7 @@ static void blk_mq_exit_hw_queues(struct request_queue *q,
queue_for_each_hw_ctx(q, hctx, i) {
if (i == nr_queue)
break;
+ blk_mq_remove_cpuhp(hctx);
blk_mq_exit_hctx(q, set, hctx, i);
}
}
@@ -3837,11 +3915,6 @@ static int blk_mq_init_hctx(struct request_queue *q,
if (xa_insert(&q->hctx_table, hctx_idx, hctx, GFP_KERNEL))
goto exit_flush_rq;
- if (!(hctx->flags & BLK_MQ_F_STACKING))
- cpuhp_state_add_instance_nocalls(CPUHP_AP_BLK_MQ_ONLINE,
- &hctx->cpuhp_online);
- cpuhp_state_add_instance_nocalls(CPUHP_BLK_MQ_DEAD, &hctx->cpuhp_dead);
-
return 0;
exit_flush_rq:
@@ -3876,6 +3949,8 @@ blk_mq_alloc_hctx(struct request_queue *q, struct blk_mq_tag_set *set,
INIT_DELAYED_WORK(&hctx->run_work, blk_mq_run_work_fn);
spin_lock_init(&hctx->lock);
INIT_LIST_HEAD(&hctx->dispatch);
+ INIT_HLIST_NODE(&hctx->cpuhp_dead);
+ INIT_HLIST_NODE(&hctx->cpuhp_online);
hctx->queue = q;
hctx->flags = set->flags & ~BLK_MQ_F_TAG_QUEUE_SHARED;
@@ -4414,6 +4489,12 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
xa_for_each_start(&q->hctx_table, j, hctx, j)
blk_mq_exit_hctx(q, set, hctx, j);
mutex_unlock(&q->sysfs_lock);
+
+ /* unregister cpuhp callbacks for exited hctxs */
+ blk_mq_remove_hw_queues_cpuhp(q);
+
+ /* register cpuhp for new initialized hctxs */
+ blk_mq_add_hw_queues_cpuhp(q);
}
int blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
--
2.47.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/3] ubd_drv: fix io command buffer mapping
2024-12-06 8:21 [PATCH 0/2] blk-mq: fix lockdep warning between sysfs_lock and cpuhotplug lock Ming Lei
` (3 preceding siblings ...)
2024-12-06 8:21 ` [PATCH 2/2] blk-mq: move cpuhp callback registering out of q->sysfs_lock Ming Lei
@ 2024-12-06 8:21 ` Ming Lei
2024-12-06 8:24 ` [PATCH 0/2] blk-mq: fix lockdep warning between sysfs_lock and cpuhotplug lock Ming Lei
5 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2024-12-06 8:21 UTC (permalink / raw)
To: Jens Axboe, linux-block; +Cc: Ming Lei
Unit of vma->vm_pgoff is PAGE_SIZE, so have to compute physical offset
first.
Without this patch, io command buffer gets corrupted in case of MQ.
Fixes: 54f5156aa8d1 ("ubd_drv: prepare for supporting MQ")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
drivers/block/ubd_drv.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/block/ubd_drv.c b/drivers/block/ubd_drv.c
index 3b3723d78084..bbcbb2f05840 100644
--- a/drivers/block/ubd_drv.c
+++ b/drivers/block/ubd_drv.c
@@ -558,14 +558,17 @@ static int ubd_ch_mmap(struct file *filp, struct vm_area_struct *vma)
struct ubd_device *ub = filp->private_data;
size_t sz = vma->vm_end - vma->vm_start;
unsigned max_sz = UBD_MAX_QUEUE_DEPTH * sizeof(struct ubdsrv_io_desc);
- unsigned long pfn, end;
+ unsigned long pfn, end, phys_off = vma->vm_pgoff << PAGE_SHIFT;
int q_id;
end = UBDSRV_CMD_BUF_OFFSET + ub->dev_info.nr_hw_queues * max_sz;
- if (vma->vm_pgoff < UBDSRV_CMD_BUF_OFFSET || vma->vm_pgoff >= end)
+ if (phys_off < UBDSRV_CMD_BUF_OFFSET || phys_off >= end)
return -EINVAL;
- q_id = (vma->vm_pgoff - UBDSRV_CMD_BUF_OFFSET) / max_sz;
+ q_id = (phys_off - UBDSRV_CMD_BUF_OFFSET) / max_sz;
+ pr_devel("%s: qid %d, pid %d, addr %lx pg_off %lx sz %lu\n",
+ __func__, q_id, current->pid, vma->vm_start,
+ phys_off, sz);
if (sz != ubd_queue_cmd_buf_size(ub, q_id))
return -EINVAL;
--
2.31.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 0/2] blk-mq: fix lockdep warning between sysfs_lock and cpuhotplug lock
2024-12-06 8:21 [PATCH 0/2] blk-mq: fix lockdep warning between sysfs_lock and cpuhotplug lock Ming Lei
` (4 preceding siblings ...)
2024-12-06 8:21 ` [PATCH 2/3] ubd_drv: fix io command buffer mapping Ming Lei
@ 2024-12-06 8:24 ` Ming Lei
5 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2024-12-06 8:24 UTC (permalink / raw)
To: Jens Axboe, linux-block
On Fri, Dec 6, 2024 at 4:22 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> Hello,
>
> The 1st patch is one prep patch.
>
> The 2nd one fixes lockdep warning triggered by dependency between
> q->sysfs_lock and cpuhotplug_lock.
>
>
> Ming Lei (2):
> blk-mq: register cpuhp callback after hctx is added to xarray table
> blk-mq: move cpuhp callback registering out of q->sysfs_lock
>
> block/blk-mq.c | 108 ++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 94 insertions(+), 14 deletions(-)
Please ignore the 2nd round mess, sorry for the noise again.
^ permalink raw reply [flat|nested] 9+ messages in thread