diff mbox series

[1/3] blk-mq: use right tag offset after wait

Message ID beee275f48a98e42f073ee63221e9610fc9470b5.1606699505.git.asml.silence@gmail.com (mailing list archive)
State New, archived
Headers show
Series cleanups/fix for blk_mq_get_tag() | expand

Commit Message

Pavel Begunkov Nov. 30, 2020, 1:36 a.m. UTC
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.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 block/blk-mq-tag.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Ming Lei Dec. 3, 2020, 6:49 a.m. UTC | #1
On Mon, Nov 30, 2020 at 01:36:25AM +0000, Pavel Begunkov wrote:
> 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.
> 
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  block/blk-mq-tag.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
> index 9c92053e704d..dbbf11edf9a6 100644
> --- a/block/blk-mq-tag.c
> +++ b/block/blk-mq-tag.c
> @@ -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;
> +

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
Pavel Begunkov Dec. 3, 2020, 12:06 p.m. UTC | #2
On 03/12/2020 06:49, Ming Lei wrote:
> On Mon, Nov 30, 2020 at 01:36:25AM +0000, Pavel Begunkov wrote:
>> 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.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>  block/blk-mq-tag.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
>> index 9c92053e704d..dbbf11edf9a6 100644
>> --- a/block/blk-mq-tag.c
>> +++ b/block/blk-mq-tag.c
>> @@ -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;
>> +
> 
> 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.
diff mbox series

Patch

diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
index 9c92053e704d..dbbf11edf9a6 100644
--- a/block/blk-mq-tag.c
+++ b/block/blk-mq-tag.c
@@ -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.