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 6/11/20 4:33 PM, John Garry wrote:
>>> +static int hctx_tags_shared_sbitmap_bitmap_show(void *data, struct 
>>> seq_file *m)
>>> +{
>>> +    struct blk_mq_hw_ctx *hctx = data;
>>> +    struct request_queue *q = hctx->queue;
>>> +    struct blk_mq_tag_set *set = q->tag_set;
>>> +    struct sbitmap shared_sb, *sb;
>>> +    int res;
>>> +
>>> +    if (!set)
>>> +        return 0;
>>> +
>>> +    /*
>>> +     * We could use the allocated sbitmap for that hctx here, but
>>> +     * that would mean that we would need to clean it prior to use.
>>> +     */
> 
> *
> 
>>> +    res = sbitmap_init_node(&shared_sb,
>>> +                set->__bitmap_tags.sb.depth,
>>> +                set->__bitmap_tags.sb.shift,
>>> +                GFP_KERNEL, NUMA_NO_NODE);
>>> +    if (res)
>>> +        return res;
>>> +    sb = &shared_sb;
>>> +
>>> +    res = mutex_lock_interruptible(&q->sysfs_lock);
>>> +    if (res)
>>> +        goto out;
>>> +    if (hctx->tags) {
>>> +        hctx_filter_sb(sb, hctx);
>>> +        sbitmap_bitmap_show(sb, m);
>>> +    }
>>> +
>>> +    mutex_unlock(&q->sysfs_lock);
>>> +
>>> +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?
> 
> 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.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                               +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer