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

[Hannes Reinecke]

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@xxxxxxx                              +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)