Re: [PATCH RFC v7 07/12] blk-mq: Add support in hctx_tags_bitmap_show() for a shared sbitmap

[Hannes Reinecke <hare@suse.de>]

On 7/13/20 11:41 AM, John Garry wrote:
> On 12/06/2020 07:06, Hannes Reinecke wrote:
>>>>>
>>>>> +out:
>>>>> +    sbitmap_free(&shared_sb);
>>>>> +    return res;
>>>>> +}
>>>>> +
>>>>>   static int hctx_tags_bitmap_show(void *data, struct seq_file *m)
>>>>>   {
>>>>>       struct blk_mq_hw_ctx *hctx = data;
>>>>> @@ -823,6 +884,7 @@ static const struct blk_mq_debugfs_attr
>>>>> blk_mq_debugfs_hctx_shared_sbitmap_attrs
>>>>>       {"busy", 0400, hctx_busy_show},
>>>>>       {"ctx_map", 0400, hctx_ctx_map_show},
>>>>>       {"tags", 0400, hctx_tags_show},
>>>>> +    {"tags_bitmap", 0400, hctx_tags_shared_sbitmap_bitmap_show},
>>>>>       {"sched_tags", 0400, hctx_sched_tags_show},
>>>>>       {"sched_tags_bitmap", 0400, hctx_sched_tags_bitmap_show},
>>>>>       {"io_poll", 0600, hctx_io_poll_show, hctx_io_poll_write},
>>>>>
>>>> Ah, right. Here it is.
>>>>
>>>> But I don't get it; why do we have to allocate a temporary bitmap
>>>> and can't walk the existing shared sbitmap?
>>>
> 
> Just coming back to this now...
> 
>>> For the bitmap dump - sbitmap_bitmap_show() - it is passed a struct
>>> sbitmap *. So we have to filter into a temp per-hctx struct sbitmap.
>>> We could change sbitmap_bitmap_show() to accept a filter iterator -
>>> which I think you're getting at - but I am not sure it's worth the
>>> change. Or else use the allocated sbitmap for the hctx, as above*,
>>> but I may be remove that (see 4/12 response).
>>>
>> Yes, I do think I would prefer updating sbitmap_bitmap_show() to
>> accept a filter. Especially as Ming Lei has now updated the tag
>> iterators to accept a filter, too, so it should be an easy change.
> 
> Can you please explain how you would use an iterator here?
> 
> In fact, I am half thinking of dropping this patch entirely.
> 
> So I feel that current behavior is a little strange, as some may expect
> /sys/kernel/debug/block/sdX/hctxY/tags_bitmap would only show tags for
> hctxY for sdX, while it is for hctxY for all queues. Same for
> /sys/kernel/debug/block/sdX/hctxY/tags
> 
> And then, for what we have in this patch:
> 
> static void hctx_filter_sb(struct sbitmap *sb, struct blk_mq_hw_ctx *hctx)
> {
> struct hctx_sb_data hctx_sb_data = { .sb = sb, .hctx = hctx };
> 
> blk_mq_queue_tag_busy_iter(hctx->queue, hctx_filter_fn, &hctx_sb_data);
> }
> 
> This will give tags only for this queue. So not the same. So I feel it's
> better to change current behavior (so consistent) or change neither. And
> changing current behavior would also mean we need to change what we show
> in /sys/kernel/debug/block/sdX/hctxY/tags, and that looks troublesome also.
> 
> Opinion?
> 
The whole notion of having sysfs presenting tags per hctx doesn't really
apply anymore when running with shared tags.

We could be sticking with the per-hctx attribute, but then busy tags
won't be displayed correctly as tags might be busy, but not on this hctx.
The alternative idea of projecting everything over to hctx0 (or just
duplicating the output from hctx0) would be technically correct, but
would be missing the per-hctx information.

Ideally we would have some sort of tri-state information here: busy,
busy on other hctx, not busy.
Then the per-hctx attribute would start making sense again.

Otherwise I would just leave it for now.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer