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)
