Re: [PATCH V2 2/3] blk-mq: complete request locally if the completion is from tagset iterator

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

On 4/27/21 12:06 AM, Ming Lei wrote:
> On Mon, Apr 26, 2021 at 07:30:51PM -0700, Bart Van Assche wrote:
>> On 4/26/21 6:45 PM, Ming Lei wrote:
>>> diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
>>> index 100fa44d52a6..773aea4db90c 100644
>>> --- a/block/blk-mq-tag.c
>>> +++ b/block/blk-mq-tag.c
>>> @@ -284,8 +284,11 @@ static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
>>>  	if ((iter_data->flags & BT_TAG_ITER_STARTED) &&
>>>  	    !blk_mq_request_started(rq))
>>>  		ret = true;
>>> -	else
>>> +	else {
>>> +		rq->rq_flags |= RQF_ITERATING;
>>>  		ret = iter_data->fn(rq, iter_data->data, reserved);
>>> +		rq->rq_flags &= ~RQF_ITERATING;
>>> +	}
>>>  	if (!iter_static_rqs)
>>>  		blk_mq_put_rq_ref(rq);
>>>  	return ret;
>>
>> All existing rq->rq_flags modifications are serialized. The above change
>> adds code that may change rq_flags concurrently with regular request
>> processing. I think that counts as a race condition.
> 
> Good catch, but we still can change .rq_flags via atomic op, such as:
> 
> 	do {
> 		old = rq->rq_flags;
> 		new = old | RQF_ITERATING;
> 	} while (cmpxchg(&rq->rq_flags, old, new) != old);

That's not sufficient because the above would not work correctly in
combination with statements like the following:

	rq->rq_flags &= ~RQF_MQ_INFLIGHT;
	req->rq_flags |= RQF_TIMED_OUT;

How about setting a flag in 'current', just like the memalloc_noio_*()
functions set or clear PF_MEMALLOC_NOIO to indicate whether or not
GFP_NOIO should be used? That should work fine in thread context and
also in interrupt context.

>> Additionally, the
>> RQF_ITERATING flag won't be set correctly in the (unlikely) case that
>> two concurrent bt_tags_iter() calls examine the same request at the same
>> time.
> 
> If the driver completes request from two concurrent bt_tags_iter(), there has
> been big trouble of double completion, so I'd rather not to consider this case.

bt_tags_iter() may be used for other purposes than completing requests.
Here is an example of a blk_mq_tagset_busy_iter() call (from debugfs)
that may run concurrently with other calls of that function:

static int hctx_busy_show(void *data, struct seq_file *m)
{
	struct blk_mq_hw_ctx *hctx = data;
	struct show_busy_params params = { .m = m, .hctx = hctx };

	blk_mq_tagset_busy_iter(hctx->queue->tag_set, hctx_show_busy_rq,
				&params);

	return 0;
}

Bart.