[PATCH] block: fix blk_queue_end_tag()

[PATCH] block: fix blk_queue_end_tag()

[Dan Williams]

Commit 5e081591 "block: warn if tag is greater than real_max_depth"
cleaned up blk_queue_end_tag() to warn when the tag is truly invalid
(greater than real_max_depth).  However, it changed behavior in the tag
< max_depth case to not end the request.  Leading to triggering of
BUG_ON(blk_queued_rq(rq)) in the request completion path:

  http://marc.info/?l=linux-kernel&m=132204370518629&w=2

In order to allow blk_queue_resize_tags() to shrink the tag space
blk_queue_end_tag() must always complete tags with a value less than
real_max_depth regardless of the current max_depth.

Cc: Tao Ma <boyu.mt@taobao.com>
Reported-by: Meelis Roos <mroos@ut.ee>
Reported-by: Ed Nadolski <edmund.nadolski@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Functionally the same as Tao Ma's fix [1], but just moves the warn into the
if() directly.

[1]: http://marc.info/?l=linux-scsi&m=132439698602441&w=2

--
Dan

 block/blk-tag.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/blk-tag.c b/block/blk-tag.c
index e74d6d1..6e61262 100644
--- a/block/blk-tag.c
+++ b/block/blk-tag.c
@@ -286,12 +286,13 @@ void blk_queue_end_tag(struct request_queue *q, struct request *rq)
 
 	BUG_ON(tag == -1);
 
-	if (unlikely(tag >= bqt->max_depth)) {
+	if (WARN_ONCE(tag >= bqt->real_max_depth,
+		      "%s: tag %d greater than tag map size: %d\n",
+		      __func__, tag, bqt->real_max_depth)) {
 		/*
 		 * This can happen after tag depth has been reduced.
 		 * But tag shouldn't be larger than real_max_depth.
 		 */
-		WARN_ON(tag >= bqt->real_max_depth);
 		return;
 	}
 

Re: [PATCH] block: fix blk_queue_end_tag()

[Tao Ma]

On 12/21/2011 07:33 AM, Dan Williams wrote:
> Commit 5e081591 "block: warn if tag is greater than real_max_depth"
> cleaned up blk_queue_end_tag() to warn when the tag is truly invalid
> (greater than real_max_depth).  However, it changed behavior in the tag
> < max_depth case to not end the request.  Leading to triggering of
> BUG_ON(blk_queued_rq(rq)) in the request completion path:
> 
>   http://marc.info/?l=linux-kernel&m=132204370518629&w=2
> 
> In order to allow blk_queue_resize_tags() to shrink the tag space
> blk_queue_end_tag() must always complete tags with a value less than
> real_max_depth regardless of the current max_depth.
> 
> Cc: Tao Ma <boyu.mt@taobao.com>
> Reported-by: Meelis Roos <mroos@ut.ee>
> Reported-by: Ed Nadolski <edmund.nadolski@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> Functionally the same as Tao Ma's fix [1], but just moves the warn into the
> if() directly.
> 
> [1]: http://marc.info/?l=linux-scsi&m=132439698602441&w=2
> 
> --
> Dan
> 
>  block/blk-tag.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-tag.c b/block/blk-tag.c
> index e74d6d1..6e61262 100644
> --- a/block/blk-tag.c
> +++ b/block/blk-tag.c
> @@ -286,12 +286,13 @@ void blk_queue_end_tag(struct request_queue *q, struct request *rq)
>  
>  	BUG_ON(tag == -1);
>  
> -	if (unlikely(tag >= bqt->max_depth)) {
> +	if (WARN_ONCE(tag >= bqt->real_max_depth,
> +		      "%s: tag %d greater than tag map size: %d\n",
> +		      __func__, tag, bqt->real_max_depth)) {
>  		/*
>  		 * This can happen after tag depth has been reduced.
Please also change the comments here since it should never happen in the
right workload.

Thanks
Tao
>  		 * But tag shouldn't be larger than real_max_depth.
>  		 */
> -		WARN_ON(tag >= bqt->real_max_depth);
>  		return;
>  	}
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] block: fix blk_queue_end_tag()

[Meelis Roos]

> > -	if (unlikely(tag >= bqt->max_depth)) {
> > +	if (WARN_ONCE(tag >= bqt->real_max_depth,
> > +		      "%s: tag %d greater than tag map size: %d\n",
> > +		      __func__, tag, bqt->real_max_depth)) {
> >  		/*
> >  		 * This can happen after tag depth has been reduced.
> Please also change the comments here since it should never happen in the
> right workload.

What do you mean by right workload? Normal workload?

-- 
Meelis Roos (mroos@linux.ee)

Re: [PATCH] block: fix blk_queue_end_tag()

[Tao Ma]

On 12/21/2011 02:36 PM, Meelis Roos wrote:
>>> -	if (unlikely(tag >= bqt->max_depth)) {
>>> +	if (WARN_ONCE(tag >= bqt->real_max_depth,
>>> +		      "%s: tag %d greater than tag map size: %d\n",
>>> +		      __func__, tag, bqt->real_max_depth)) {
>>>  		/*
>>>  		 * This can happen after tag depth has been reduced.
>> Please also change the comments here since it should never happen in the
>> right workload.
> 
> What do you mean by right workload? Normal workload?
yeah, so real_max_depth is the maximum depth we ever have. So in normal
case(shrinking queue depth is also a normal user case), we should never
arrive here. In another word, if tag >= real_max_depth, we should have a
bug in the kernel.

Thanks
Tao

Re: [PATCH] block: fix blk_queue_end_tag()

[Williams, Dan J]

On Tue, Dec 20, 2011 at 10:48 PM, Tao Ma <tm@tao.ma> wrote:
> On 12/21/2011 02:36 PM, Meelis Roos wrote:
>>>> -   if (unlikely(tag >= bqt->max_depth)) {
>>>> +   if (WARN_ONCE(tag >= bqt->real_max_depth,
>>>> +                 "%s: tag %d greater than tag map size: %d\n",
>>>> +                 __func__, tag, bqt->real_max_depth)) {
>>>>             /*
>>>>              * This can happen after tag depth has been reduced.
>>> Please also change the comments here since it should never happen in the
>>> right workload.
>>
>> What do you mean by right workload? Normal workload?
> yeah, so real_max_depth is the maximum depth we ever have. So in normal
> case(shrinking queue depth is also a normal user case), we should never
> arrive here. In another word, if tag >= real_max_depth, we should have a
> bug in the kernel.

So this is what Ed Nadolski suggested, just cut to the chase and do,
the following.  Seems like the comment is what got us into trouble in
the first place.

diff --git a/block/blk-tag.c b/block/blk-tag.c
index e74d6d1..e297d9d7 100644
--- a/block/blk-tag.c
+++ b/block/blk-tag.c
@@ -284,16 +284,7 @@ void blk_queue_end_tag(struct request_queue *q,
struct request *rq)
        struct blk_queue_tag *bqt = q->queue_tags;
        int tag = rq->tag;

-       BUG_ON(tag == -1);
-
-       if (unlikely(tag >= bqt->max_depth)) {
-               /*
-                * This can happen after tag depth has been reduced.
-                * But tag shouldn't be larger than real_max_depth.
-                */
-               WARN_ON(tag >= bqt->real_max_depth);
-               return;
-       }
+       BUG_ON(tag == -1 || tag > bqt->real_max_depth);

Re: [PATCH] block: fix blk_queue_end_tag()

[Tao Ma]

On 12/21/2011 03:30 PM, Williams, Dan J wrote:
> On Tue, Dec 20, 2011 at 10:48 PM, Tao Ma <tm@tao.ma> wrote:
>> On 12/21/2011 02:36 PM, Meelis Roos wrote:
>>>>> -   if (unlikely(tag >= bqt->max_depth)) {
>>>>> +   if (WARN_ONCE(tag >= bqt->real_max_depth,
>>>>> +                 "%s: tag %d greater than tag map size: %d\n",
>>>>> +                 __func__, tag, bqt->real_max_depth)) {
>>>>>             /*
>>>>>              * This can happen after tag depth has been reduced.
>>>> Please also change the comments here since it should never happen in the
>>>> right workload.
>>>
>>> What do you mean by right workload? Normal workload?
>> yeah, so real_max_depth is the maximum depth we ever have. So in normal
>> case(shrinking queue depth is also a normal user case), we should never
>> arrive here. In another word, if tag >= real_max_depth, we should have a
>> bug in the kernel.
> 
> So this is what Ed Nadolski suggested, just cut to the chase and do,
> the following.  Seems like the comment is what got us into trouble in
> the first place.
> 
> diff --git a/block/blk-tag.c b/block/blk-tag.c
> index e74d6d1..e297d9d7 100644
> --- a/block/blk-tag.c
> +++ b/block/blk-tag.c
> @@ -284,16 +284,7 @@ void blk_queue_end_tag(struct request_queue *q,
> struct request *rq)
>         struct blk_queue_tag *bqt = q->queue_tags;
>         int tag = rq->tag;
> 
> -       BUG_ON(tag == -1);
> -
> -       if (unlikely(tag >= bqt->max_depth)) {
> -               /*
> -                * This can happen after tag depth has been reduced.
> -                * But tag shouldn't be larger than real_max_depth.
> -                */
> -               WARN_ON(tag >= bqt->real_max_depth);
> -               return;
> -       }
> +       BUG_ON(tag == -1 || tag > bqt->real_max_depth);
I guess tag = bqt->real_max_depth should also be a problem.

Thanks
Tao

Re: [PATCH] block: fix blk_queue_end_tag()

[Williams, Dan J]

On Wed, Dec 21, 2011 at 12:16 AM, Tao Ma <tm@tao.ma> wrote:
> On 12/21/2011 03:30 PM, Williams, Dan J wrote:
>> On Tue, Dec 20, 2011 at 10:48 PM, Tao Ma <tm@tao.ma> wrote:
>>> On 12/21/2011 02:36 PM, Meelis Roos wrote:
>>>>>> -   if (unlikely(tag >= bqt->max_depth)) {
>>>>>> +   if (WARN_ONCE(tag >= bqt->real_max_depth,
>>>>>> +                 "%s: tag %d greater than tag map size: %d\n",
>>>>>> +                 __func__, tag, bqt->real_max_depth)) {
>>>>>>             /*
>>>>>>              * This can happen after tag depth has been reduced.
>>>>> Please also change the comments here since it should never happen in the
>>>>> right workload.
>>>>
>>>> What do you mean by right workload? Normal workload?
>>> yeah, so real_max_depth is the maximum depth we ever have. So in normal
>>> case(shrinking queue depth is also a normal user case), we should never
>>> arrive here. In another word, if tag >= real_max_depth, we should have a
>>> bug in the kernel.
>>
>> So this is what Ed Nadolski suggested, just cut to the chase and do,
>> the following.  Seems like the comment is what got us into trouble in
>> the first place.
>>
>> diff --git a/block/blk-tag.c b/block/blk-tag.c
>> index e74d6d1..e297d9d7 100644
>> --- a/block/blk-tag.c
>> +++ b/block/blk-tag.c
>> @@ -284,16 +284,7 @@ void blk_queue_end_tag(struct request_queue *q,
>> struct request *rq)
>>         struct blk_queue_tag *bqt = q->queue_tags;
>>         int tag = rq->tag;
>>
>> -       BUG_ON(tag == -1);
>> -
>> -       if (unlikely(tag >= bqt->max_depth)) {
>> -               /*
>> -                * This can happen after tag depth has been reduced.
>> -                * But tag shouldn't be larger than real_max_depth.
>> -                */
>> -               WARN_ON(tag >= bqt->real_max_depth);
>> -               return;
>> -       }
>> +       BUG_ON(tag == -1 || tag > bqt->real_max_depth);
> I guess tag = bqt->real_max_depth should also be a problem.

Yes, sorry, should have been >=

Re: [PATCH] block: fix blk_queue_end_tag()

[Jens Axboe]

On 2011-12-21 09:22, Williams, Dan J wrote:
> On Wed, Dec 21, 2011 at 12:16 AM, Tao Ma <tm@tao.ma> wrote:
>> On 12/21/2011 03:30 PM, Williams, Dan J wrote:
>>> On Tue, Dec 20, 2011 at 10:48 PM, Tao Ma <tm@tao.ma> wrote:
>>>> On 12/21/2011 02:36 PM, Meelis Roos wrote:
>>>>>>> -   if (unlikely(tag >= bqt->max_depth)) {
>>>>>>> +   if (WARN_ONCE(tag >= bqt->real_max_depth,
>>>>>>> +                 "%s: tag %d greater than tag map size: %d\n",
>>>>>>> +                 __func__, tag, bqt->real_max_depth)) {
>>>>>>>             /*
>>>>>>>              * This can happen after tag depth has been reduced.
>>>>>> Please also change the comments here since it should never happen in the
>>>>>> right workload.
>>>>>
>>>>> What do you mean by right workload? Normal workload?
>>>> yeah, so real_max_depth is the maximum depth we ever have. So in normal
>>>> case(shrinking queue depth is also a normal user case), we should never
>>>> arrive here. In another word, if tag >= real_max_depth, we should have a
>>>> bug in the kernel.
>>>
>>> So this is what Ed Nadolski suggested, just cut to the chase and do,
>>> the following.  Seems like the comment is what got us into trouble in
>>> the first place.
>>>
>>> diff --git a/block/blk-tag.c b/block/blk-tag.c
>>> index e74d6d1..e297d9d7 100644
>>> --- a/block/blk-tag.c
>>> +++ b/block/blk-tag.c
>>> @@ -284,16 +284,7 @@ void blk_queue_end_tag(struct request_queue *q,
>>> struct request *rq)
>>>         struct blk_queue_tag *bqt = q->queue_tags;
>>>         int tag = rq->tag;
>>>
>>> -       BUG_ON(tag == -1);
>>> -
>>> -       if (unlikely(tag >= bqt->max_depth)) {
>>> -               /*
>>> -                * This can happen after tag depth has been reduced.
>>> -                * But tag shouldn't be larger than real_max_depth.
>>> -                */
>>> -               WARN_ON(tag >= bqt->real_max_depth);
>>> -               return;
>>> -       }
>>> +       BUG_ON(tag == -1 || tag > bqt->real_max_depth);
>> I guess tag = bqt->real_max_depth should also be a problem.
> 
> Yes, sorry, should have been >=

Can you resend a v2 and I'll get it applied, thanks.

-- 
Jens Axboe

Re: [PATCH] block: fix blk_queue_end_tag()

[Matthew Wilcox]

On Tue, Dec 20, 2011 at 11:30:14PM -0800, Williams, Dan J wrote:
> @@ -284,16 +284,7 @@ void blk_queue_end_tag(struct request_queue *q,
> struct request *rq)
>         struct blk_queue_tag *bqt = q->queue_tags;
>         int tag = rq->tag;
> 
> -       BUG_ON(tag == -1);
> -
> -       if (unlikely(tag >= bqt->max_depth)) {
> -               /*
> -                * This can happen after tag depth has been reduced.
> -                * But tag shouldn't be larger than real_max_depth.
> -                */
> -               WARN_ON(tag >= bqt->real_max_depth);
> -               return;
> -       }
> +       BUG_ON(tag == -1 || tag > bqt->real_max_depth);

or ...

-	int tag = rq->tag;
+	unsigned tag = rq->tag;
+	BUG_ON(tag >= bqt->real_max_depth);

since tags in the range INT_MIN to -2 are also a bug, right?

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] block: fix blk_queue_end_tag()

[Williams, Dan J]

On Wed, Dec 21, 2011 at 5:20 AM, Matthew Wilcox <matthew@wil.cx> wrote:
> On Tue, Dec 20, 2011 at 11:30:14PM -0800, Williams, Dan J wrote:
>> @@ -284,16 +284,7 @@ void blk_queue_end_tag(struct request_queue *q,
>> struct request *rq)
>>         struct blk_queue_tag *bqt = q->queue_tags;
>>         int tag = rq->tag;
>>
>> -       BUG_ON(tag == -1);
>> -
>> -       if (unlikely(tag >= bqt->max_depth)) {
>> -               /*
>> -                * This can happen after tag depth has been reduced.
>> -                * But tag shouldn't be larger than real_max_depth.
>> -                */
>> -               WARN_ON(tag >= bqt->real_max_depth);
>> -               return;
>> -       }
>> +       BUG_ON(tag == -1 || tag > bqt->real_max_depth);
>
> or ...
>
> -       int tag = rq->tag;
> +       unsigned tag = rq->tag;
> +       BUG_ON(tag >= bqt->real_max_depth);
>
> since tags in the range INT_MIN to -2 are also a bug, right?

Yes, even better... patch coming up.