Re: [PATCH 2/2] block: only return started requests from blk_mq_tag_to_rq()

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

On 2020-06-19 07:09, Hannes Reinecke wrote:
> On 6/19/20 4:01 PM, Hannes Reinecke wrote:
>> blk_mq_tag_to_rq() is used from within the driver to map a tag
>> to a request. As such it should only return requests which are
>> already started (ie passed to the driver); otherwise the driver
>> might trip over requests which it has never seen and random
>> crashes will occur.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   block/blk-mq.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 4f57d27bfa73..f02d18113f9e 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -815,9 +815,13 @@ EXPORT_SYMBOL(blk_mq_delay_kick_requeue_list);
>>     struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags,
>> unsigned int tag)
>>   {
>> +    struct request *rq;
>> +
>>       if (tag < tags->nr_tags) {
>>           prefetch(tags->rqs[tag]);
>> -        return tags->rqs[tag];
>> +        rq = tags->rqs[tag];
>> +        if (blk_mq_request_started(rq))
>> +            return rq;
>>       }
>>         return NULL;
>>
> This becomes particularly obnoxious for SCSI drivers using
> scsi_host_find_tag() for cleaning up stale commands (ie drivers like
> qla4xxx, fnic, and snic).
> All other drivers use it from the completion routine, so one can expect
> a valid (and started) tag here. So for those it shouldn't matter.
> 
> But still, if there are objections I could look at fixing it within the
> SCSI stack; although that would most likely mean I'll have to implement
> the above patch as an additional function.

Hi Hannes,

Will the above patch make the fast path of every block driver slightly
slower?

Shouldn't SCSI drivers (and other block drivers) use
blk_mq_tagset_busy_iter() to clean up stale commands instead of
iterating over all tags and calling blk_mq_tag_to_rq() directly?

Thanks,

Bart.