linux-kernel - Re: [PATCH v2 08/11] blk-mq: Add blk_mq_ops.init_request_no

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]

Date:   Wed, 18 Aug 2021 09:46:30 +0100
From:   John Garry <john.garry@...wei.com>
To:     Ming Lei <ming.lei@...hat.com>
CC:     <axboe@...nel.dk>, <jejb@...ux.ibm.com>,
        <martin.petersen@...cle.com>, <linux-block@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-scsi@...r.kernel.org>,
        <kashyap.desai@...adcom.com>, <hare@...e.de>
Subject: Re: [PATCH v2 08/11] blk-mq: Add blk_mq_ops.init_request_no_hctx()

On 18/08/2021 08:38, Ming Lei wrote:
> On Mon, Aug 09, 2021 at 10:29:35PM +0800, John Garry wrote:
>> Add a variant of the init_request function which does not pass a hctx_idx
>> arg.
>>
>> This is important for shared sbitmap support, as it needs to be ensured for
>> introducing shared static rqs that the LLDD cannot think that requests
>> are associated with a specific HW queue.
>>
>> Signed-off-by: John Garry<john.garry@...wei.com>
>> ---
>>   block/blk-mq.c         | 15 ++++++++++-----
>>   include/linux/blk-mq.h |  7 +++++++
>>   2 files changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index f14cc2705f9b..4d6723cfa582 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -2427,13 +2427,15 @@ struct blk_mq_tags *blk_mq_alloc_rq_map(struct blk_mq_tag_set *set,
>>   static int blk_mq_init_request(struct blk_mq_tag_set *set, struct request *rq,
>>   			       unsigned int hctx_idx, int node)
>>   {
>> -	int ret;
>> +	int ret = 0;
>>   
>> -	if (set->ops->init_request) {
>> +	if (set->ops->init_request)
>>   		ret = set->ops->init_request(set, rq, hctx_idx, node);
>> -		if (ret)
>> -			return ret;
>> -	}
>> +	else if (set->ops->init_request_no_hctx)
>> +		ret = set->ops->init_request_no_hctx(set, rq, node);

Hi Ming,

> The only shared sbitmap user of SCSI does not use passed hctx_idx, not
> sure we need such new callback.

Sure, actually most versions of init_request callback don't use 
hctx_idx. Or numa_node arg.
> If you really want to do this, just wondering why not pass '-1' as
> hctx_idx in case of shared sbitmap?

Yeah, I did consider that. hctx_idx is an unsigned, and I generally 
don't like -1U - but that's no big deal. But I also didn't like how it 
relies on the driver init_request callback to check the value, which 
changes the semantics.

Obviously we don't add new versions of init_request for new block 
drivers which use shared sbitmap everyday, and any new ones would get it 
right.

I suppose I can go the way you suggest - I just thought that this method 
was neat as well.

Thanks,
John