Re: [PATCH RFC 3/5] blk-mq: Facilitate a shared tags per tagset

[Hannes Reinecke <hare@suse.de>]

On 11/13/19 5:21 PM, John Garry wrote:
> On 13/11/2019 15:38, Hannes Reinecke wrote:
>>>>> -        if (clear_ctx_on_error)
>>>>> -            data->ctx = NULL;
>>>>> -        blk_queue_exit(q);
>>>>> -        return NULL;
>>>>> +    if (data->hctx->shared_tags) {
>>>>> +        shared_tag = blk_mq_get_shared_tag(data);
>>>>> +        if (shared_tag == BLK_MQ_TAG_FAIL)
>>>>> +            goto err_shared_tag;
>>>>>        }
>>>>>    -    rq = blk_mq_rq_ctx_init(data, tag, data->cmd_flags,
>>>>> alloc_time_ns);
>>>>> +    tag = blk_mq_get_tag(data);
>>>>> +    if (tag == BLK_MQ_TAG_FAIL)
>>>>> +        goto err_tag;
>>>>> +
>>>>> +    rq = blk_mq_rq_ctx_init(data, tag, shared_tag, data->cmd_flags,
>>>>> alloc_time_ns);
>>>>>        if (!op_is_flush(data->cmd_flags)) {
>>>>>            rq->elv.icq = NULL;
>>>>>            if (e && e->type->ops.prepare_request) {
>>> Hi Hannes,
>>>
>>>> Why do you need to keep a parallel tag accounting between 'normal' and
>>>> 'shared' tags here?
>>>> Isn't is sufficient to get a shared tag only, and us that in lieo of 
>>>> the
>>>> 'real' one?
>>> In theory, yes. Just the 'shared' tag should be adequate.
>>>
>>> A problem I see with this approach is that we lose the identity of which
>>> tags are allocated for each hctx. As an example for this, consider
>>> blk_mq_queue_tag_busy_iter(), which iterates the bits for each hctx.
>>> Now, if you're just using shared tags only, that wouldn't work.
>>>
>>> Consider blk_mq_can_queue() as another example - this tells us if a hctx
>>> has any bits unset, while with only using shared tags it would tell if
>>> any bits unset over all queues, and this change in semantics could break
>>> things. At a glance, function __blk_mq_tag_idle() looks problematic 
>>> also.
>>>
>>> And this is where it becomes messy to implement.
>>>
> 
> Hi Hannes,
> 
>> Oh, my. Indeed, that's correct.
> 
> The tags could be kept in sync like this:
> 
> shared_tag = blk_mq_get_tag(shared_tagset);
> if (shared_tag != -1)
>      sbitmap_set(hctx->tags, shared_tag);
> 
> But that's obviously not ideal.
> 
Actually, I _do_ prefer keeping both in sync.
We might want to check if the 'normal' tag is set (typically it would 
not, but then, who knows ...)
The beauty here is that both 'shared' and 'normal' tag are in sync, so 
if a driver would be wanting to use the tag as index into a command 
array it can do so without any surprises.

Why do you think it's not ideal?

>>
>> But then we don't really care _which_ shared tag is assigned; so
>> wouldn't be we better off by just having an atomic counter here?
>> Cache locality will be blown anyway ...
> The atomic counter would solve the issuing more than Scsi_host.can_queue 
> to the LLDD, but we still need a unique tag, which is what the shared 
> tag is.
> 
Yeah, true. Daft idea :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)