Re: [PATCH v5 3/3] blk-mq: Fix a race between iterating over requests and freeing requests

[Bart Van Assche <bvanassche@acm.org>]

On 4/5/21 2:34 PM, Jens Axboe wrote:
> For something out of left field, we can check if the page that the rq
> belongs to is still part of the tag set. If it isn't, then don't
> deref it.
> 
> Totally untested.
> 
> diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
> index e5bfecf2940d..6209c465e884 100644
> --- a/block/blk-mq-tag.c
> +++ b/block/blk-mq-tag.c
> @@ -196,9 +196,35 @@ struct bt_iter_data {
>  	struct blk_mq_hw_ctx *hctx;
>  	busy_iter_fn *fn;
>  	void *data;
> +	struct page *last_lookup;
>  	bool reserved;
>  };
>  
> +static bool rq_from_queue(struct bt_iter_data *iter_data, struct request *rq)
> +{
> +	struct blk_mq_hw_ctx *hctx = iter_data->hctx;
> +	struct page *rq_page, *page;
> +
> +	/*
> +	 * We can hit rq == NULL here, because the tagging functions
> +	 * test and set the bit before assigning ->rqs[].
> +	 */
> +	if (!rq)
> +		return false;
> +	rq_page = virt_to_page(rq);
> +	if (rq_page == iter_data->last_lookup)
> +		goto check_queue;
> +	list_for_each_entry(page, &hctx->tags->page_list, lru) {
> +		if (page == rq_page) {
> +			iter_data->last_lookup = page;
> +			goto check_queue;
> +		}
> +	}
> +	return false;
> +check_queue:
> +	return rq->q == hctx->queue && rq->mq_hctx == hctx;
> +}
> +
>  static bool bt_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
>  {
>  	struct bt_iter_data *iter_data = data;
> @@ -211,11 +237,7 @@ static bool bt_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
>  		bitnr += tags->nr_reserved_tags;
>  	rq = tags->rqs[bitnr];
>  
> -	/*
> -	 * We can hit rq == NULL here, because the tagging functions
> -	 * test and set the bit before assigning ->rqs[].
> -	 */
> -	if (rq && rq->q == hctx->queue && rq->mq_hctx == hctx)
> +	if (rq_from_queue(iter_data, rq))
>  		return iter_data->fn(hctx, rq, iter_data->data, reserved);
>  	return true;
>  }

Hi Jens,

That's a very interesting suggestion. However, it seems to me that Khazhy's
suggestion will result in shorter and faster code?

Khazhy pointed out another race to me off-list, namely a race between updating
the number of hardware queues and iterating over the tags in a tag set. I'm
currently analyzing how to fix that race too.

Thanks,

Bart.