Re: [PATCH] blk-cgroup: fix circular locking dependency in blkg_conf_prep

Hi,

在 2026/8/12 4:48, Sudarsan Mahendran 写道:
> In blkg_conf_prep, we may do IO when allocating while
> holding blkcg_mutex, however this presents a deadlock
> scenario since we may grab sysfs_lock while IO is
> suspended in elevator_exit, and an existing chain
> exists from sysfs_mutex -> blkcg_mutex.
>
> We can fix this by performing only GFP_NOIO allocations within
> blkg_conf_prep when blkcg_mutex is held.
> This patch fixes our reproducer.
>
> Full lockdep splat:
>
> [ 3795.003422] ======================================================
> [ 3795.009595] WARNING: possible circular locking dependency detected
> [ 3795.015773] 6.18.20-dbg-DEV #1 Tainted: G                 N
> [ 3795.021432] ------------------------------------------------------
> [ 3795.027604] kpython3/435194 is trying to acquire lock:
> [ 3795.032744] ffffffffa36959b8 (fs_reclaim){+.+.}-{0:0}, at: kmem_cache_alloc_noprof+0x49/0x5c0
> [ 3795.041260]
> [ 3795.041260] but task is already holding lock:
> [ 3795.047086] ffff98c8a06477b0 (&q->blkcg_mutex){+.+.}-{4:4}, at: blkg_conf_prep+0x71/0x5e0
> [ 3795.055257]
> [ 3795.055257] which lock already depends on the new lock.
> [ 3795.055257]
> [ 3795.063422]
> [ 3795.063422] the existing dependency chain (in reverse order) is:
> [ 3795.070893]
> [ 3795.070893] -> #4 (&q->blkcg_mutex){+.+.}-{4:4}:
> [ 3795.076986]        __mutex_lock+0x92/0xb70
> [ 3795.081085]        blkcg_deactivate_policy+0x7f/0x230
> [ 3795.086137]        bfq_exit_queue+0xe1/0x110
> [ 3795.090401]        blk_mq_exit_sched+0xd1/0x170
> [ 3795.094935]        elevator_exit+0x50/0x70
> (...)
> [ 3795.135019]
> [ 3795.135019] -> #3 (&eq->sysfs_lock){+.+.}-{4:4}:
> [ 3795.141110]        __mutex_lock+0x92/0xb70
> [ 3795.145211]        elevator_exit+0x45/0x70
> [ 3795.149308]        elevator_switch+0x98/0x290
> [ 3795.153660]        elevator_change+0x109/0x190
> [ 3795.158106]        elv_iosched_store+0x198/0x240
> (...)
> [ 3795.185276]
> [ 3795.185276] -> #2 (&q->elevator_lock){+.+.}-{4:4}:
> [ 3795.191541]        __mutex_lock+0x92/0xb70
> [ 3795.195642]        elevator_change+0xb6/0x190
> [ 3795.200001]        elv_iosched_store+0x198/0x240
> (...)
> [ 3795.227179]
> [ 3795.227179] -> #1 (&q->q_usage_counter(io)#19){++++}-{0:0}:
> [ 3795.234224]        blk_alloc_queue+0x33e/0x370
> (...)
> [ 3795.266621]
> [ 3795.266621] -> #0 (fs_reclaim){+.+.}-{0:0}:
> [ 3795.272280]        __lock_acquire+0x157c/0x2980
> [ 3795.276813]        lock_acquire+0xd3/0x2a0
> [ 3795.280904]        fs_reclaim_acquire+0x55/0xc0
> [ 3795.285435]        kmem_cache_alloc_noprof+0x49/0x5c0
> [ 3795.290481]        __radix_tree_preload+0xef/0x250
> [ 3795.295273]        blkg_conf_prep+0x2aa/0x5e0
> [ 3795.299633]        tg_set_conf+0xa5/0x1a0
> (...)
> [ 3795.330808]
> [ 3795.330808] other info that might help us debug this:
> [ 3795.330808]
> [ 3795.338806] Chain exists of:
> [ 3795.338806]   fs_reclaim --> &eq->sysfs_lock --> &q->blkcg_mutex
> [ 3795.338806]
> [ 3795.349146]  Possible unsafe locking scenario:
> [ 3795.349146]
> [ 3795.355057]        CPU0                    CPU1
> [ 3795.359590]        ----                    ----
> [ 3795.364121]   lock(&q->blkcg_mutex);
> [ 3795.367691]                                lock(&eq->sysfs_lock);
> [ 3795.373776]                                lock(&q->blkcg_mutex);
> [ 3795.379862]   lock(fs_reclaim);
> [ 3795.383006]
> [ 3795.383006]  *** DEADLOCK ***
>
> Reviewed-by: Khazhy Kumykov <[email protected]>
> Signed-off-by: Sudarsan Mahendran <[email protected]>
> ---
>   block/blk-cgroup.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index d9676126c5b5..b981a80bbd94 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -867,7 +867,6 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
>   			parent = blkcg_parent(parent);
>   		}
>   
> -		/* Drop locks to do new blkg allocation with GFP_KERNEL. */
>   		spin_unlock_irq(&q->queue_lock);
>   
>   		new_blkg = blkg_alloc(pos, disk, GFP_NOIO);
> @@ -876,7 +875,7 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
>   			goto fail_exit;
>   		}
>   
> -		if (radix_tree_preload(GFP_KERNEL)) {
> +		if (radix_tree_preload(GFP_NOIO)) {

Thanks for the patch, however, there is a pending patchset that this radix_tree_preload will be removed,
I think which is a better solution:

[RFC PATCH v2 2/4] blk-cgroup: use a request_queue rhashtable for blkg 
lookup - Yu Kuai <https://lore.kernel.org/all/[email protected]/>

>   			blkg_free(new_blkg);
>   			ret = -ENOMEM;
>   			goto fail_exit;

-- 
Thanks,
Kuai