Re: [PATCH] block: fix q->blkg_list corruption during disk rebind

[Ming Lei <ming.lei@redhat.com>]

On Sun, Apr 07, 2024 at 09:40:52PM +0800, Yu Kuai wrote:
> Hi,
> 
> 在 2024/04/07 20:59, Ming Lei 写道:
> > Multiple gendisk instances can allocated/added for single request queue
> > in case of disk rebind. blkg may still stay in q->blkg_list when calling
> > blkcg_init_disk() for rebind, then q->blkg_list becomes corrupted.
> > 
> > Fix the list corruption issue by:
> > 
> > - add blkg_init_queue() to initialize q->blkg_list & q->blkcg_mutex only
> > - move calling blkg_init_queue() into blk_alloc_queue()
> > 
> > The list corruption should be started since commit f1c006f1c685 ("blk-cgroup:
> > synchronize pd_free_fn() from blkg_free_workfn() and blkcg_deactivate_policy()")
> > which delays removing blkg from q->blkg_list into blkg_free_workfn().
> 
> I'm not familiar with how bind/unbind works yet, however, the patch
> itself looks reasonable to me, the initialization of fields related to
> queue should not be delayed to disk allocation.
> 
> Reviewed-by: Yu Kuai <yukuai3@huawei.com>

Thanks!

> 
> BTW, it looks like the whole blkcg_init_disk() can go away:
>  - init of ioprio and blk-throttle can be delayed to the first user
> configuration;
>  - root_blkg allocation doesn't rely on disk at all;
> 
> Or is there any plan to move the blkcg related field or code path to
> gendisk instead of queue? I might missing some previous discussions.

Christoph worked towards this direction, but not succeed:

a06377c5d01e Revert "blk-cgroup: pin the gendisk in struct blkcg_gq"
9a9c261e6b55 Revert "blk-cgroup: pass a gendisk to blkg_lookup"
1231039db31c Revert "blk-cgroup: move the cgroup information to struct gendisk"
dcb522014351 Revert "blk-cgroup: simplify blkg freeing from initialization failure paths"



Thanks, 
Ming