linux-kernel - Re: [PATCH] blk-mq: Fix blk_mq_tagset_busy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <79266509-f327-9de3-d22e-0e9fe00387ee@huawei.com>
Date:   Wed, 13 Oct 2021 11:01:11 +0100
From:   John Garry <john.garry@...wei.com>
To:     Ming Lei <ming.lei@...hat.com>
CC:     <axboe@...nel.dk>, <linux-block@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <kashyap.desai@...adcom.com>,
        <hare@...e.de>
Subject: Re: [PATCH] blk-mq: Fix blk_mq_tagset_busy_iter() for shared tags

On 13/10/2021 10:22, Ming Lei wrote:
> On Wed, Oct 13, 2021 at 04:40:59PM +0800, John Garry wrote:
>> Since it is now possible for a tagset to share a single set of tags, the
>> iter function should not re-iter the tags for the count of #hw queues in
>> that case. Rather it should just iter once.
>>
>> Fixes: e0fdf846c7bb ("blk-mq: Use shared tags for shared sbitmap support")
>> Reported-by: Kashyap Desai<kashyap.desai@...adcom.com>
>> Signed-off-by: John Garry<john.garry@...wei.com>
>>
>> diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
>> index 72a2724a4eee..c943b6529619 100644
>> --- a/block/blk-mq-tag.c
>> +++ b/block/blk-mq-tag.c
>> @@ -378,9 +378,12 @@ void blk_mq_all_tag_iter(struct blk_mq_tags *tags, busy_tag_iter_fn *fn,
>>   void blk_mq_tagset_busy_iter(struct blk_mq_tag_set *tagset,
>>   		busy_tag_iter_fn *fn, void *priv)
>>   {
>> -	int i;
>> +	unsigned int flags = tagset->flags;
>> +	int i, nr_tags;
>> +
>> +	nr_tags = blk_mq_is_shared_tags(flags) ? 1 : tagset->nr_hw_queues;
>>   
>> -	for (i = 0; i < tagset->nr_hw_queues; i++) {
>> +	for (i = 0; i < nr_tags; i++) {
>>   		if (tagset->tags && tagset->tags[i])
>>   			__blk_mq_all_tag_iter(tagset->tags[i], fn, priv,
>>   					      BT_TAG_ITER_STARTED);
> blk_mq_queue_tag_busy_iter() needn't such change?

I didn't think so.

blk_mq_queue_tag_busy_iter() will indeed re-iter the tags per hctx. 
However in bt_iter(), we check rq->mq_hctx == hctx for calling the iter 
callback:

static bool bt_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
{
	...

	if (rq->q == hctx->queue && rq->mq_hctx == hctx)
		ret = iter_data->fn(hctx, rq, iter_data->data, reserved);

And this would only pass for the correct hctx which we're iter'ing for. 
Indeed, it would be nice not to iter excessive times, but I didn't see a 
straightforward way to change that.

There is also blk_mq_all_tag_iter():

void blk_mq_all_tag_iter(struct blk_mq_tags *tags, busy_tag_iter_fn *fn,
		void *priv)
{
	__blk_mq_all_tag_iter(tags, fn, priv, BT_TAG_ITER_STATIC_RQS);
}

But then the only user is blk_mq_hctx_has_requests():

static bool blk_mq_hctx_has_requests(struct blk_mq_hw_ctx *hctx)
{
	struct blk_mq_tags *tags = hctx->sched_tags ?
			hctx->sched_tags : hctx->tags;
	struct rq_iter_data data = {
		.hctx	= hctx,
	};

	blk_mq_all_tag_iter(tags, blk_mq_has_request, &data);
	return data.has_rq;
}

But, again like bt_iter(), blk_mq_has_request() will check the hctx matches:

static bool blk_mq_has_request(struct request *rq, void *data, bool 
reserved)
{
	struct rq_iter_data *iter_data = data;

	if (rq->mq_hctx != iter_data->hctx)
		return true;
	iter_data->has_rq = true;
	return false;
}

Thanks,
John