Re: [PATCH 2/5] bfq: protect q->blkg_list iteration in bfq_end_wr_async() with blkcg_mutex

[Nilay Shroff]

On 6/3/26 6:57 PM, Yu Kuai wrote:
> bfq_end_wr_async() iterates q->blkg_list while only holding bfqd->lock,
> but not blkcg_mutex. This can race with blkg_free_workfn() that removes
> blkgs from the list while holding blkcg_mutex.
>
> Add blkcg_mutex protection in bfq_end_wr() before taking bfqd->lock to
> ensure proper synchronization when iterating q->blkg_list.
>
> Signed-off-by: Yu Kuai <yukuai@xxxxxxx>
> ---
>   block/bfq-cgroup.c  | 3 ++-
>   block/bfq-iosched.c | 6 ++++++
>   2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
> index 37ab70930c8d..f765e767d36a 100644
> --- a/block/bfq-cgroup.c
> +++ b/block/bfq-cgroup.c
> @@ -939,11 +939,12 @@ void bfq_end_wr_async(struct bfq_data *bfqd)
>   	struct blkcg_gq *blkg;
>   
>   	list_for_each_entry(blkg, &bfqd->queue->blkg_list, q_node) {
>   		struct bfq_group *bfqg = blkg_to_bfqg(blkg);
>   
> -		bfq_end_wr_async_queues(bfqd, bfqg);
> +		if (bfqg)
> +			bfq_end_wr_async_queues(bfqd, bfqg);
>   	}
>   	bfq_end_wr_async_queues(bfqd, bfqd->root_group);
>   }
>   
>   static int bfq_io_show_weight_legacy(struct seq_file *sf, void *v)
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 141c602d5e85..42ccfd0c6140 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -2643,10 +2643,13 @@ void bfq_end_wr_async_queues(struct bfq_data *bfqd,
>   static void bfq_end_wr(struct bfq_data *bfqd)
>   {
>   	struct bfq_queue *bfqq;
>   	int i;
>   
> +#ifdef CONFIG_BFQ_GROUP_IOSCHED
> +	mutex_lock(&bfqd->queue->blkcg_mutex);
> +#endif
>   	spin_lock_irq(&bfqd->lock);
>   
>   	for (i = 0; i < bfqd->num_actuators; i++) {
>   		list_for_each_entry(bfqq, &bfqd->active_list[i], bfqq_list)
>   			bfq_bfqq_end_wr(bfqq);
> @@ -2654,10 +2657,13 @@ static void bfq_end_wr(struct bfq_data *bfqd)
>   	list_for_each_entry(bfqq, &bfqd->idle_list, bfqq_list)
>   		bfq_bfqq_end_wr(bfqq);
>   	bfq_end_wr_async(bfqd);
>   
>   	spin_unlock_irq(&bfqd->lock);
> +#ifdef CONFIG_BFQ_GROUP_IOSCHED
> +	mutex_unlock(&bfqd->queue->blkcg_mutex);
> +#endif
>   }

The above change protects the q->blkg_list iteration in bfq_end_wr_async()
against list removal in blkg_free_workfn(). However the blkg insertion in
blkg_create() still doesn't use q->blkcg_mutex and so list traversal in
bfq_end_wr_async() may still race with blkg_create().

So I think we may also need to protect blkg insert in blkg_create() using
q->blkcg_mutex.

Thanks,
--Nilay