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

[Yu Kuai <yukuai1@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxx>

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.

Thanks,
Kuai

> Fixes: f1c006f1c685 ("blk-cgroup: synchronize pd_free_fn() from blkg_free_workfn() and blkcg_deactivate_policy()")
> Fixes: 1059699f87eb ("block: move blkcg initialization/destroy into disk allocation/release handler")
> Cc: Yu Kuai <yukuai3@xxxxxxxxxx>
> Cc: Tejun Heo <tj@xxxxxxxxxx>
> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> ---
> blktests:
> 	https://lore.kernel.org/linux-block/20240407125717.4052964-1-ming.lei@xxxxxxxxxx/
>
>   block/blk-cgroup.c | 9 ++++++---
>   block/blk-cgroup.h | 2 ++
>   block/blk-core.c   | 2 ++
>   3 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index bdbb557feb5a..059467086b13 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -1409,6 +1409,12 @@ static int blkcg_css_online(struct cgroup_subsys_state *css)
>   	return 0;
>   }
>   
> +void blkg_init_queue(struct request_queue *q)
> +{
> +	INIT_LIST_HEAD(&q->blkg_list);
> +	mutex_init(&q->blkcg_mutex);
> +}
> +
>   int blkcg_init_disk(struct gendisk *disk)
>   {
>   	struct request_queue *q = disk->queue;
> @@ -1416,9 +1422,6 @@ int blkcg_init_disk(struct gendisk *disk)
>   	bool preloaded;
>   	int ret;
>   
> -	INIT_LIST_HEAD(&q->blkg_list);
> -	mutex_init(&q->blkcg_mutex);
> -
>   	new_blkg = blkg_alloc(&blkcg_root, disk, GFP_KERNEL);
>   	if (!new_blkg)
>   		return -ENOMEM;
> diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
> index 78b74106bf10..90b3959d88cf 100644
> --- a/block/blk-cgroup.h
> +++ b/block/blk-cgroup.h
> @@ -189,6 +189,7 @@ struct blkcg_policy {
>   extern struct blkcg blkcg_root;
>   extern bool blkcg_debug_stats;
>   
> +void blkg_init_queue(struct request_queue *q);
>   int blkcg_init_disk(struct gendisk *disk);
>   void blkcg_exit_disk(struct gendisk *disk);
>   
> @@ -482,6 +483,7 @@ struct blkcg {
>   };
>   
>   static inline struct blkcg_gq *blkg_lookup(struct blkcg *blkcg, void *key) { return NULL; }
> +static inline void blkg_init_queue(struct request_queue *q) { }
>   static inline int blkcg_init_disk(struct gendisk *disk) { return 0; }
>   static inline void blkcg_exit_disk(struct gendisk *disk) { }
>   static inline int blkcg_policy_register(struct blkcg_policy *pol) { return 0; }
> diff --git a/block/blk-core.c b/block/blk-core.c
> index a16b5abdbbf5..3a6f5603fb44 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -442,6 +442,8 @@ struct request_queue *blk_alloc_queue(struct queue_limits *lim, int node_id)
>   	init_waitqueue_head(&q->mq_freeze_wq);
>   	mutex_init(&q->mq_freeze_lock);
>   
> +	blkg_init_queue(q);
> +
>   	/*
>   	 * Init percpu_ref in atomic mode so that it's faster to shutdown.
>   	 * See blk_register_queue() for details.