Re: [PATCH] block: don't dereference request after flush insertion

[Jens Axboe <axboe@kernel.dk>]

On 10/17/21 8:02 PM, Ming Lei wrote:
> On Sun, Oct 17, 2021 at 07:50:24PM -0600, Jens Axboe wrote:
>> On 10/17/21 7:49 PM, Ming Lei wrote:
>>> On Sat, Oct 16, 2021 at 07:35:39PM -0600, Jens Axboe wrote:
>>>> We could have a race here, where the request gets freed before we call
>>>> into blk_mq_run_hw_queue(). If this happens, we cannot rely on the state
>>>> of the request.
>>>>
>>>> Grab the hardware context before inserting the flush.
>>>>
>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>>
>>>> ---
>>>>
>>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>>> index 2197cfbf081f..22b30a89bf3a 100644
>>>> --- a/block/blk-mq.c
>>>> +++ b/block/blk-mq.c
>>>> @@ -2468,9 +2468,10 @@ void blk_mq_submit_bio(struct bio *bio)
>>>>  	}
>>>>  
>>>>  	if (unlikely(is_flush_fua)) {
>>>> +		struct blk_mq_hw_ctx *hctx = rq->mq_hctx;
>>>>  		/* Bypass scheduler for flush requests */
>>>>  		blk_insert_flush(rq);
>>>> -		blk_mq_run_hw_queue(rq->mq_hctx, true);
>>>> +		blk_mq_run_hw_queue(hctx, true);
>>>
>>> If the request is freed before running queue, the request queue could
>>> be released and the hctx may be freed.
>>
>> No, we still hold a queue enter ref.
> 
> But that one is released when rq is freed since ac7c5675fa45 ("blk-mq: allow
> blk_mq_make_request to consume the q_usage_counter reference"), isn't
> it?

Yes I think you're right, we need to grab an extra ref in the flush case as
we're using it after it may potentially have completed. We could probably
make it smarter, but little point for just handling a flush.

commit ea0f672e7cc66e7ec12468ff907de6064656b6e7
Author: Jens Axboe <axboe@kernel.dk>
Date:   Sat Oct 16 07:34:49 2021 -0600

    block: grab extra reference for flush insertion
    
    We could have a race here, where the request gets freed before we call
    into blk_mq_run_hw_queue(). If this happens, we cannot rely on the state
    of the request, nor can we rely on the queue still being alive.
    
    Grab an extra queue reference before inserting the flush and then running
    the queue, to ensure that it is still valid.
    
    Signed-off-by: Jens Axboe <axboe@kernel.dk>

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 87dc2debedfb..d28423ccfe2b 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2284,9 +2284,18 @@ blk_qc_t blk_mq_submit_bio(struct bio *bio)
 	}
 
 	if (unlikely(is_flush_fua)) {
+		struct blk_mq_hw_ctx *hctx = rq->mq_hctx;
+
+		/*
+		 * Our queue ref may disappears as soon as the flush is
+		 * inserted, grab an extra one.
+		 */
+		percpu_ref_tryget_live(&q->q_usage_counter);
+
 		/* Bypass scheduler for flush requests */
 		blk_insert_flush(rq);
-		blk_mq_run_hw_queue(rq->mq_hctx, true);
+		blk_mq_run_hw_queue(hctx, true);
+		blk_queue_exit(q);
 	} else if (plug && (q->nr_hw_queues == 1 ||
 		   blk_mq_is_shared_tags(rq->mq_hctx->flags) ||
 		   q->mq_ops->commit_rqs || !blk_queue_nonrot(q))) {

-- 
Jens Axboe