On 12/3/19 3:54 PM, John Garry wrote: >> @@ -483,8 +483,8 @@ static int hctx_tags_bitmap_show(void *data, >> struct seq_file *m) >> res = mutex_lock_interruptible(&q->sysfs_lock); >> if (res) >> goto out; >> - if (hctx->tags) >> - sbitmap_bitmap_show(&hctx->tags->bitmap_tags.sb, m); >> + if (hctx->tags) /* We should just iterate the relevant bits for >> this hctx FIXME */ > > Bart's solution to this problem seemed ok, if he doesn't mind us > borrowing his idea: > > https://lore.kernel.org/linux-block/5183ab13-0c81-95f0-95ba-40318569c6c6@xxxxxxxxxx/T/#m24394fe70b1ea79a154dfd9620f5e553c3e7e7da > > > See hctx_tags_bitmap_show(). > > It might be also reasonable to put that in another follow on patch, as > there would be no enablers of the "shared" bitmap until later patches. > Yeah, that was my plan, too. But then I'd rather wait for feedback on the general approach here; no point is wasting perfectly good bits if no-one's wanting them ... [ .. ] >> @@ -121,10 +121,10 @@ unsigned int blk_mq_get_tag(struct >> blk_mq_alloc_data *data) >> WARN_ON_ONCE(1); >> return BLK_MQ_TAG_FAIL; >> } >> - bt = &tags->breserved_tags; >> + bt = tags->breserved_tags; > > We could put all of this in an earlier patch (as you had in v4, modulo > dynamic memory part), which would be easier to review and get accepted. > Yeah, but I felt it a bit odd, just having pointers to an existing structure element. But yes, will be doing for the next round. >> tag_offset = 0; >> } else { >> - bt = &tags->bitmap_tags; >> + bt = tags->bitmap_tags; >> tag_offset = tags->nr_reserved_tags; >> } > > > [...] > >> if (!set) >> @@ -3160,6 +3179,7 @@ int blk_mq_update_nr_requests(struct >> request_queue *q, unsigned int nr) >> ret = blk_mq_tag_update_depth(hctx, &hctx->tags, nr, >> false); >> } else { >> + sched_tags = true; >> ret = blk_mq_tag_update_depth(hctx, &hctx->sched_tags, >> nr, true); >> } >> @@ -3169,8 +3189,43 @@ int blk_mq_update_nr_requests(struct >> request_queue *q, unsigned int nr) >> q->elevator->type->ops.depth_updated(hctx); >> } >> - if (!ret) >> + /* >> + * if ret is 0, all queues should have been updated to the same >> depth >> + * if not, then maybe some have been updated - yuk, need to >> handle this for shared sbitmap... >> + * if some are updated, we should probably roll back the change >> altogether. FIXME >> + */ > > If you don't have a shared sched bitmap - which I didn't think we needed > - then all we need is a simple sbitmap_queue_resize(&tagset->__bitmap_tags) > > Otherwise it's horrible to resize shared sched bitmaps... > Resizing shared sched bitmaps is done in patch 6/11. General idea is to move the scheduler bitmap into the request queue (well, actually the elevator), as this gives us a per-request_queue bitmap. Which is actually what we want here, as the scheduler will need to look at all requests, hence needing to access to the same bitmap. And it also gives us an easy way of resizing the sched tag bitmap, as then we can resize the bitmap on a per-queue basis, and leave the underlying tagset bitmap untouched. [ .. ] >> diff --git a/block/blk-mq.h b/block/blk-mq.h >> index 78d38b5f2793..4c1ea206d3f4 100644 >> --- a/block/blk-mq.h >> +++ b/block/blk-mq.h >> @@ -166,6 +166,11 @@ struct blk_mq_alloc_data { >> struct blk_mq_hw_ctx *hctx; >> }; >> +static inline bool blk_mq_is_sbitmap_shared(struct blk_mq_tag_set >> *tag_set) >> +{ >> + return !!(tag_set->flags & BLK_MQ_F_TAG_HCTX_SHARED); > > Bart already gave some comments on this > Ah. Missed that one. Will be including in the next round. Thanks for the review! Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking hare@xxxxxxx +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
