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

[John Garry <john.garry@huawei.com>]

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?

Thanks,
John