cleanups/fix for blk_mq_get_tag()
cover cleanups/fix for blk_mq_get_tag()
[1/3] fixes using tag_offset of a different HW queue. May only fire if it's allowed for queues to have different breserved_tags or a resize happened while it was waiting (can be?). Two others are simple cleanups catched the eye.
Pavel Begunkov (3):
blk-mq: use right tag offset after wait
blk-mq: deduplicate blk_mq_get_tag() bits
blk-mq: idiomatic use of WARN_ON_ONCE
block/blk-mq-tag.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
Signature · 1 line
2.24.0
PATCH 1/3 blk-mq: use right tag offset after wait
blk_mq_get_tag() selects tag_offset in the beginning and doesn't update it even though the tag set it may have changed hw queue during waiting.
--- block/blk-mq-tag.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
block/blk-mq-tag.c +5 -2
@@ -101,10 +101,8 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)return BLK_MQ_NO_TAG ; } bt = tags->breserved_tags; - tag_offset =0 ; }else { bt = tags->bitmap_tags; - tag_offset = tags->nr_reserved_tags; } tag =__blk_mq_get_tag (data, bt);
@@ -167,6 +165,11 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)sbitmap_finish_wait (bt, ws, &wait); found_tag: +if (data->flags &BLK_MQ_REQ_RESERVED ) + tag_offset =0 ; +else + tag_offset = tags->nr_reserved_tags; +/* * Give up this allocation if the hctx is inactive. The caller will * retry on an active hctx.
Signature · 1 line
2.24.0
Re: [PATCH 1/3] blk-mq: use right tag offset after wait
Quoted 24 lines · Pavel Begunkov
blk_mq_get_tag()selectstag_offsetin the beginning and doesn't update it even though the tag set it may have changed hw queue during waiting.Signed-off-by Pavel Begunkov <[email protected]>--- block/blk-mq-tag.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)block/blk-mq-tag.c +0
-2@@ -101,10 +101,8 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)return BLK_MQ_NO_TAG ; } bt = tags->breserved_tags; - tag_offset =0 ; }else { bt = tags->bitmap_tags; - tag_offset = tags->nr_reserved_tags; } tag =__blk_mq_get_tag (data, bt);
block/blk-mq-tag.c unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data) · line 172
Quoted 8 lines
block/blk-mq-tag.c +5
-0@@ -167,6 +165,11 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)sbitmap_finish_wait (bt, ws, &wait); found_tag: +if (data->flags &BLK_MQ_REQ_RESERVED ) + tag_offset =0 ; +else + tag_offset = tags->nr_reserved_tags; +
So far, the tag offset is tag-set wide, and 'tag_offset' is same for all tags, looks no need to add the extra check.
thanks,
Ming
Re: [PATCH 1/3] blk-mq: use right tag offset after wait
Quoted 27 lines · Ming Lei
Quoted 26 lines · Pavel Begunkov
blk_mq_get_tag()selectstag_offsetin the beginning and doesn't update it even though the tag set it may have changed hw queue during waiting.Signed-off-by Pavel Begunkov <[email protected]>--- block/blk-mq-tag.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)block/blk-mq-tag.c +0
-2@@ -101,10 +101,8 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)return BLK_MQ_NO_TAG ; } bt = tags->breserved_tags; - tag_offset =0 ; }else { bt = tags->bitmap_tags; - tag_offset = tags->nr_reserved_tags; } tag =__blk_mq_get_tag (data, bt);@@ -167,6 +165,11 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)sbitmap_finish_wait (bt, ws, &wait);
Quoted 9 lines
block/blk-mq-tag.c unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data) · line 172
Quoted 6 lines
block/blk-mq-tag.c +5
-0found_tag: +if (data->flags &BLK_MQ_REQ_RESERVED ) + tag_offset =0 ; +else + tag_offset = tags->nr_reserved_tags; +So far, the tag offset is tag-set wide, and '
tag_offset' is same for all tags, looks no need to add the extra check.
Thanks for clearing that. I still think that's a good thing to do as more apparent to me and shouldn't be slower -- it unconditionally stores offset on-stack, even for no-wait path (gcc 9.2). With it's optimised more like
if (!(data->flags & BLK_MQ_REQ_RESERVED))
tag += tags->nr_reserved_tags;
I like assembly a bit more, but not like that matters much.
Signature · 1 line
Pavel Begunkov
PATCH 2/3 blk-mq: deduplicate blk_mq_get_tag() bits
In reply to Pavel BegunkovDeduplicate bt_wait_ptr() call in blk_mq_get_tag(), condition-less while and absence of loop controls in-between former and current call location makes the change functionally identical.
--- block/blk-mq-tag.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
block/blk-mq-tag.c +2 -3
@@ -112,10 +112,11 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)if (data->flags &BLK_MQ_REQ_NOWAIT )return BLK_MQ_NO_TAG ; - ws =bt_wait_ptr (bt, data->hctx); do { struct sbitmap_queue *bt_prev; + ws = bt_wait_ptr(bt, data->hctx); + /* * We're out of tags on this hardware queue, kick any * pending IO submits before going to sleep waiting for
@@ -158,8 +159,6 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)*/ if (bt != bt_prev)sbitmap_queue_wake_up (bt_prev); - - ws =bt_wait_ptr (bt, data->hctx); }while (1 );sbitmap_finish_wait (bt, ws, &wait);
Signature · 1 line
2.24.0
PATCH 3/3 blk-mq: idiomatic use of WARN_ON_ONCE
In reply to Pavel BegunkovLet WARN_ON_ONCE() to be more clear by passing the warn condition directly, so it'll be printed.
--- block/blk-mq-tag.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
block/blk-mq-tag.c +1 -3
@@ -96,10 +96,8 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)int tag;if (data->flags &BLK_MQ_REQ_RESERVED ) { -if (unlikely (!tags->nr_reserved_tags)) { -WARN_ON_ONCE (1 ); +if (WARN_ON_ONCE (!tags->nr_reserved_tags))return BLK_MQ_NO_TAG ; - } bt = tags->breserved_tags; }else { bt = tags->bitmap_tags;
Signature · 1 line
2.24.0