[PATCH RESEND] block: warn if tag is greater than real_max_depth.

[PATCH RESEND] block: warn if tag is greater than real_max_depth.

[Tao Ma]

From: Tao Ma <boyu.mt@taobao.com>

In case tag depth is reduced, it is max_depth not real_max_depth.
So we should allow a request with tag >= max_depth, but for a
tag >= real_max_depth, there really should be some problem.

Cc: Jens Axboe <jaxboe@fusionio.com>
Signed-off-by: Tao Ma <boyu.mt@taobao.com>
---
 block/blk-tag.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/blk-tag.c b/block/blk-tag.c
index ece65fc..e74d6d1 100644
--- a/block/blk-tag.c
+++ b/block/blk-tag.c
@@ -286,12 +286,14 @@ void blk_queue_end_tag(struct request_queue *q, struct request *rq)
 
 	BUG_ON(tag == -1);
 
-	if (unlikely(tag >= bqt->real_max_depth))
+	if (unlikely(tag >= bqt->max_depth)) {
 		/*
 		 * This can happen after tag depth has been reduced.
-		 * FIXME: how about a warning or info message here?
+		 * But tag shouldn't be larger than real_max_depth.
 		 */
+		WARN_ON(tag >= bqt->real_max_depth);
 		return;
+	}
 
 	list_del_init(&rq->queuelist);
 	rq->cmd_flags &= ~REQ_QUEUED;
-- 
1.7.1

[PATCH RESEND] block: Don't check QUEUE_FLAG_SAME_COMP in __blk_complete_request.

[Tao Ma]

From: Tao Ma <boyu.mt@taobao.com>

In __blk_complete_request, we check both QUEUE_FLAG_SAME_COMP and req->cpu
to decide whether we should use req->cpu. Actually the user can also
select the complete cpu by either setting BIO_CPU_AFFINE or by calling
bio_set_completion_cpu. Current solution makes these 2 ways don't work
any more. So we'd better just check req->cpu.

Cc: Jens Axboe <jaxboe@fusionio.com>
Signed-off-by: Tao Ma <boyu.mt@taobao.com>
---
 block/blk-softirq.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/block/blk-softirq.c b/block/blk-softirq.c
index 58340d0..1366a89 100644
--- a/block/blk-softirq.c
+++ b/block/blk-softirq.c
@@ -115,7 +115,7 @@ void __blk_complete_request(struct request *req)
 	/*
 	 * Select completion CPU
 	 */
-	if (test_bit(QUEUE_FLAG_SAME_COMP, &q->queue_flags) && req->cpu != -1) {
+	if (req->cpu != -1) {
 		ccpu = req->cpu;
 		if (!test_bit(QUEUE_FLAG_SAME_FORCE, &q->queue_flags)) {
 			ccpu = blk_cpu_to_group(ccpu);
-- 
1.7.1

Re: [PATCH RESEND] block: Don't check QUEUE_FLAG_SAME_COMP in __blk_complete_request.

[Shaohua Li]

2011/9/14 Tao Ma <tm@tao.ma>:
> From: Tao Ma <boyu.mt@taobao.com>
>
> In __blk_complete_request, we check both QUEUE_FLAG_SAME_COMP and req->cpu
> to decide whether we should use req->cpu. Actually the user can also
> select the complete cpu by either setting BIO_CPU_AFFINE or by calling
> bio_set_completion_cpu. Current solution makes these 2 ways don't work
> any more. So we'd better just check req->cpu.
>
> Cc: Jens Axboe <jaxboe@fusionio.com>
> Signed-off-by: Tao Ma <boyu.mt@taobao.com>
> ---
>  block/blk-softirq.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/block/blk-softirq.c b/block/blk-softirq.c
> index 58340d0..1366a89 100644
> --- a/block/blk-softirq.c
> +++ b/block/blk-softirq.c
> @@ -115,7 +115,7 @@ void __blk_complete_request(struct request *req)
>        /*
>         * Select completion CPU
>         */
> -       if (test_bit(QUEUE_FLAG_SAME_COMP, &q->queue_flags) && req->cpu != -1) {
> +       if (req->cpu != -1) {
>                ccpu = req->cpu;
>                if (!test_bit(QUEUE_FLAG_SAME_FORCE, &q->queue_flags)) {
>                        ccpu = blk_cpu_to_group(ccpu);
why not delete bio_set_completion_cpu()?
1. nobody uses it in my search
2. it's misleading. even setting the completion cpu, the bio isn't
always to run finish in the cpu,
it's just run in the group of the cpu.

Thanks,
Shaohua

Re: [PATCH RESEND] block: Don't check QUEUE_FLAG_SAME_COMP in __blk_complete_request.

[Tao Ma]

On 09/15/2011 09:05 AM, Shaohua Li wrote:
> 2011/9/14 Tao Ma <tm@tao.ma>:
>> From: Tao Ma <boyu.mt@taobao.com>
>>
>> In __blk_complete_request, we check both QUEUE_FLAG_SAME_COMP and req->cpu
>> to decide whether we should use req->cpu. Actually the user can also
>> select the complete cpu by either setting BIO_CPU_AFFINE or by calling
>> bio_set_completion_cpu. Current solution makes these 2 ways don't work
>> any more. So we'd better just check req->cpu.
>>
>> Cc: Jens Axboe <jaxboe@fusionio.com>
>> Signed-off-by: Tao Ma <boyu.mt@taobao.com>
>> ---
>>  block/blk-softirq.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/block/blk-softirq.c b/block/blk-softirq.c
>> index 58340d0..1366a89 100644
>> --- a/block/blk-softirq.c
>> +++ b/block/blk-softirq.c
>> @@ -115,7 +115,7 @@ void __blk_complete_request(struct request *req)
>>        /*
>>         * Select completion CPU
>>         */
>> -       if (test_bit(QUEUE_FLAG_SAME_COMP, &q->queue_flags) && req->cpu != -1) {
>> +       if (req->cpu != -1) {
>>                ccpu = req->cpu;
>>                if (!test_bit(QUEUE_FLAG_SAME_FORCE, &q->queue_flags)) {
>>                        ccpu = blk_cpu_to_group(ccpu);
> why not delete bio_set_completion_cpu()?
Not sure. It is added in commit c7c22e4d from the very beginning, and in
the commit log Jens described it as:
A bio helper (bio_set_completion_cpu()) is also added, so that queuers
can ask for completion on that specific CPU. Maybe it is obsolete.
Anyway, I am fine to generate a patch to remove it if you prefer.
> 1. nobody uses it in my search
yes, but it may be designed for someone to use it. The same goes with
BIO_CPU_AFFINE.
> 2. it's misleading. even setting the completion cpu, the bio isn't
> always to run finish in the cpu,
> it's just run in the group of the cpu.
Without this patch, it works as you said.
But with this patch, it the user uses bio_set_completion_cpu and
QUEUE_FLAG_SAME_FORCE isn't set, the completion will happen in the
req->cpu the user specified.

Jens,
	Do you have any option for it? Whether we should preserve it and make
it work or just totally remove it?

Thanks
Tao

Re: [PATCH RESEND] block: Don't check QUEUE_FLAG_SAME_COMP in __blk_complete_request.

[Christoph Hellwig]

On Thu, Sep 15, 2011 at 10:16:02AM +0800, Tao Ma wrote:
> > why not delete bio_set_completion_cpu()?
> Not sure. It is added in commit c7c22e4d from the very beginning, and in
> the commit log Jens described it as:
> A bio helper (bio_set_completion_cpu()) is also added, so that queuers
> can ask for completion on that specific CPU. Maybe it is obsolete.
> Anyway, I am fine to generate a patch to remove it if you prefer.
> > 1. nobody uses it in my search
> yes, but it may be designed for someone to use it. The same goes with
> BIO_CPU_AFFINE.

This code is unused, and from the all the discussions lately pretty
obviously broken.  The only thing keeping it serves is creating more
confusion and possibly more bugs.

Re: [PATCH RESEND] block: Don't check QUEUE_FLAG_SAME_COMP in __blk_complete_request.

[Jens Axboe]

On 2011-09-15 13:17, Christoph Hellwig wrote:
> On Thu, Sep 15, 2011 at 10:16:02AM +0800, Tao Ma wrote:
>>> why not delete bio_set_completion_cpu()?
>> Not sure. It is added in commit c7c22e4d from the very beginning, and in
>> the commit log Jens described it as:
>> A bio helper (bio_set_completion_cpu()) is also added, so that queuers
>> can ask for completion on that specific CPU. Maybe it is obsolete.
>> Anyway, I am fine to generate a patch to remove it if you prefer.
>>> 1. nobody uses it in my search
>> yes, but it may be designed for someone to use it. The same goes with
>> BIO_CPU_AFFINE.
> 
> This code is unused, and from the all the discussions lately pretty
> obviously broken.  The only thing keeping it serves is creating more
> confusion and possibly more bugs.

We can kill bio_set_completion_cpu(). I'm fine with leaving cpu control
to the request based drivers, they are the only ones that can toggle the
setting anyway.

-- 
Jens Axboe

Re: [PATCH RESEND] block: Don't check QUEUE_FLAG_SAME_COMP in __blk_complete_request.

[Tao Ma]

On 09/15/2011 07:28 PM, Jens Axboe wrote:
> On 2011-09-15 13:17, Christoph Hellwig wrote:
>> On Thu, Sep 15, 2011 at 10:16:02AM +0800, Tao Ma wrote:
>>>> why not delete bio_set_completion_cpu()?
>>> Not sure. It is added in commit c7c22e4d from the very beginning, and in
>>> the commit log Jens described it as:
>>> A bio helper (bio_set_completion_cpu()) is also added, so that queuers
>>> can ask for completion on that specific CPU. Maybe it is obsolete.
>>> Anyway, I am fine to generate a patch to remove it if you prefer.
>>>> 1. nobody uses it in my search
>>> yes, but it may be designed for someone to use it. The same goes with
>>> BIO_CPU_AFFINE.
>>
>> This code is unused, and from the all the discussions lately pretty
>> obviously broken.  The only thing keeping it serves is creating more
>> confusion and possibly more bugs.
> 
> We can kill bio_set_completion_cpu(). I'm fine with leaving cpu control
> to the request based drivers, they are the only ones that can toggle the
> setting anyway.
So we should remove bio_comp_cpu and BIO_CPU_AFFINE also if we have
decided to only leave cpu control to the request based drivers, right?

I will generate a patch for it later.

Thanks
Tao

Re: [PATCH RESEND] block: warn if tag is greater than real_max_depth.

[Tao Ma]

Hi Jens,
	any option with this patch?

Thanks
Tao
On 09/14/2011 03:23 PM, Tao Ma wrote:
> From: Tao Ma <boyu.mt@taobao.com>
> 
> In case tag depth is reduced, it is max_depth not real_max_depth.
> So we should allow a request with tag >= max_depth, but for a
> tag >= real_max_depth, there really should be some problem.
> 
> Cc: Jens Axboe <jaxboe@fusionio.com>
> Signed-off-by: Tao Ma <boyu.mt@taobao.com>
> ---
>  block/blk-tag.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-tag.c b/block/blk-tag.c
> index ece65fc..e74d6d1 100644
> --- a/block/blk-tag.c
> +++ b/block/blk-tag.c
> @@ -286,12 +286,14 @@ void blk_queue_end_tag(struct request_queue *q, struct request *rq)
>  
>  	BUG_ON(tag == -1);
>  
> -	if (unlikely(tag >= bqt->real_max_depth))
> +	if (unlikely(tag >= bqt->max_depth)) {
>  		/*
>  		 * This can happen after tag depth has been reduced.
> -		 * FIXME: how about a warning or info message here?
> +		 * But tag shouldn't be larger than real_max_depth.
>  		 */
> +		WARN_ON(tag >= bqt->real_max_depth);
>  		return;
> +	}
>  
>  	list_del_init(&rq->queuelist);
>  	rq->cmd_flags &= ~REQ_QUEUED;

Re: [PATCH RESEND] block: warn if tag is greater than real_max_depth.

[Jens Axboe]

On 2011-10-24 17:03, Tao Ma wrote:
> Hi Jens,
> 	any option with this patch?
> 
> Thanks
> Tao
> On 09/14/2011 03:23 PM, Tao Ma wrote:
>> From: Tao Ma <boyu.mt@taobao.com>
>>
>> In case tag depth is reduced, it is max_depth not real_max_depth.
>> So we should allow a request with tag >= max_depth, but for a
>> tag >= real_max_depth, there really should be some problem.
>>
>> Cc: Jens Axboe <jaxboe@fusionio.com>
>> Signed-off-by: Tao Ma <boyu.mt@taobao.com>
>> ---
>>  block/blk-tag.c |    6 ++++--
>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/blk-tag.c b/block/blk-tag.c
>> index ece65fc..e74d6d1 100644
>> --- a/block/blk-tag.c
>> +++ b/block/blk-tag.c
>> @@ -286,12 +286,14 @@ void blk_queue_end_tag(struct request_queue *q, struct request *rq)
>>  
>>  	BUG_ON(tag == -1);
>>  
>> -	if (unlikely(tag >= bqt->real_max_depth))
>> +	if (unlikely(tag >= bqt->max_depth)) {
>>  		/*
>>  		 * This can happen after tag depth has been reduced.
>> -		 * FIXME: how about a warning or info message here?
>> +		 * But tag shouldn't be larger than real_max_depth.
>>  		 */
>> +		WARN_ON(tag >= bqt->real_max_depth);
>>  		return;
>> +	}
>>  
>>  	list_del_init(&rq->queuelist);
>>  	rq->cmd_flags &= ~REQ_QUEUED;

Looks good, better than what we had. Applied.

-- 
Jens Axboe

Re: [PATCH RESEND] block: warn if tag is greater than real_max_depth.

[Dan Williams]

On Tue, Oct 25, 2011 at 1:19 AM, Jens Axboe <axboe@kernel.dk> wrote:
> On 2011-10-24 17:03, Tao Ma wrote:
>> Hi Jens,
>>       any option with this patch?
>>
>> Thanks
>> Tao
>> On 09/14/2011 03:23 PM, Tao Ma wrote:
>>> From: Tao Ma <boyu.mt@taobao.com>
>>>
>>> In case tag depth is reduced, it is max_depth not real_max_depth.
>>> So we should allow a request with tag >= max_depth, but for a
>>> tag >= real_max_depth, there really should be some problem.
>>>
>>> Cc: Jens Axboe <jaxboe@fusionio.com>
>>> Signed-off-by: Tao Ma <boyu.mt@taobao.com>
>>> ---
>>>  block/blk-tag.c |    6 ++++--
>>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/blk-tag.c b/block/blk-tag.c
>>> index ece65fc..e74d6d1 100644
>>> --- a/block/blk-tag.c
>>> +++ b/block/blk-tag.c
>>> @@ -286,12 +286,14 @@ void blk_queue_end_tag(struct request_queue *q, struct request *rq)
>>>
>>>      BUG_ON(tag == -1);
>>>
>>> -    if (unlikely(tag >= bqt->real_max_depth))
>>> +    if (unlikely(tag >= bqt->max_depth)) {
>>>              /*
>>>               * This can happen after tag depth has been reduced.
>>> -             * FIXME: how about a warning or info message here?
>>> +             * But tag shouldn't be larger than real_max_depth.
>>>               */
>>> +            WARN_ON(tag >= bqt->real_max_depth);
>>>              return;
>>> +    }
>>>
>>>      list_del_init(&rq->queuelist);
>>>      rq->cmd_flags &= ~REQ_QUEUED;
>
> Looks good, better than what we had. Applied.

This appears to interact badly with scsi_adjust_queue_depth() when the
tag space shrinks.  I can reproduce a similar crash as reported in
"3.2-rc2+git: kernel BUG at block/blk-core.c:1000!
(__scsi_queue_insert)" [1].

I can hit "kernel BUG at block/blk-core.c:2268!" which is the same
BUG_ON(blk_queued_rq(rq)) check reliably with:
# for i in $(seq 0 10); do dd if=/dev/zero of=/dev/sdX & done
# echo 4 > /sys/class/block/sdX/device/queue_depth

The following fixes it for me, if this looks ok (versus reverting
commit 5e081591) I'll roll it into a formal patch with Ed and Meelis'
Reported-by.

diff --git a/block/blk-tag.c b/block/blk-tag.c
index e74d6d1..09cf867 100644
--- a/block/blk-tag.c
+++ b/block/blk-tag.c
@@ -230,9 +230,12 @@ int blk_queue_resize_tags(struct request_queue
*q, int new_depth)
        /*
         * if we already have large enough real_max_depth.  just
         * adjust max_depth.  *NOTE* as requests with tag value
-        * between new_depth and real_max_depth can be in-flight, tag
+        * between new_depth and max_depth can be in-flight, tag
         * map can not be shrunk blindly here.
         */
+       if (new_depth <= bqt->max_depth)
+               return 0;
+
        if (new_depth <= bqt->real_max_depth) {
                bqt->max_depth = new_depth;
                return 0;

--
Dan

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

Re: [PATCH RESEND] block: warn if tag is greater than real_max_depth.

[Tao Ma]

On 12/20/2011 08:07 AM, Dan Williams wrote:
> On Tue, Oct 25, 2011 at 1:19 AM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 2011-10-24 17:03, Tao Ma wrote:
>>> Hi Jens,
>>>       any option with this patch?
>>>
>>> Thanks
>>> Tao
>>> On 09/14/2011 03:23 PM, Tao Ma wrote:
>>>> From: Tao Ma <boyu.mt@taobao.com>
>>>>
>>>> In case tag depth is reduced, it is max_depth not real_max_depth.
>>>> So we should allow a request with tag >= max_depth, but for a
>>>> tag >= real_max_depth, there really should be some problem.
>>>>
>>>> Cc: Jens Axboe <jaxboe@fusionio.com>
>>>> Signed-off-by: Tao Ma <boyu.mt@taobao.com>
>>>> ---
>>>>  block/blk-tag.c |    6 ++++--
>>>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/block/blk-tag.c b/block/blk-tag.c
>>>> index ece65fc..e74d6d1 100644
>>>> --- a/block/blk-tag.c
>>>> +++ b/block/blk-tag.c
>>>> @@ -286,12 +286,14 @@ void blk_queue_end_tag(struct request_queue *q, struct request *rq)
>>>>
>>>>      BUG_ON(tag == -1);
>>>>
>>>> -    if (unlikely(tag >= bqt->real_max_depth))
>>>> +    if (unlikely(tag >= bqt->max_depth)) {
>>>>              /*
>>>>               * This can happen after tag depth has been reduced.
>>>> -             * FIXME: how about a warning or info message here?
>>>> +             * But tag shouldn't be larger than real_max_depth.
>>>>               */
>>>> +            WARN_ON(tag >= bqt->real_max_depth);
>>>>              return;
>>>> +    }
>>>>
>>>>      list_del_init(&rq->queuelist);
>>>>      rq->cmd_flags &= ~REQ_QUEUED;
>>
>> Looks good, better than what we had. Applied.
> 
> This appears to interact badly with scsi_adjust_queue_depth() when the
> tag space shrinks.  I can reproduce a similar crash as reported in
> "3.2-rc2+git: kernel BUG at block/blk-core.c:1000!
> (__scsi_queue_insert)" [1].
> 
> I can hit "kernel BUG at block/blk-core.c:2268!" which is the same
> BUG_ON(blk_queued_rq(rq)) check reliably with:
> # for i in $(seq 0 10); do dd if=/dev/zero of=/dev/sdX & done
> # echo 4 > /sys/class/block/sdX/device/queue_depth
> 
> The following fixes it for me, if this looks ok (versus reverting
> commit 5e081591) I'll roll it into a formal patch with Ed and Meelis'
> Reported-by.
Interesting. If I read the code correctly, real_max_depth is the maximum
queue depth we ever have and max_depth is the current depth.

In your fix, we never resize the tag size to be smaller than max_depth.
So I think this patch does expose some problem, but not lead to the BUG.
And in your new comment, you mentioned that "request between new_depth
and max_depth can be in-flight", but max_depth <= real_max_depth, so
what's wrong with the comment? Sorry, but am I missing something here?

Having said that, I have tried the test in my machine here, but have no
luck by now. My kernel version is 3.2.0-rc5+.

Thanks
Tao
> 
> diff --git a/block/blk-tag.c b/block/blk-tag.c
> index e74d6d1..09cf867 100644
> --- a/block/blk-tag.c
> +++ b/block/blk-tag.c
> @@ -230,9 +230,12 @@ int blk_queue_resize_tags(struct request_queue
> *q, int new_depth)
>         /*
>          * if we already have large enough real_max_depth.  just
>          * adjust max_depth.  *NOTE* as requests with tag value
> -        * between new_depth and real_max_depth can be in-flight, tag
> +        * between new_depth and max_depth can be in-flight, tag
>          * map can not be shrunk blindly here.
>          */
> +       if (new_depth <= bqt->max_depth)
> +               return 0;
> +
>         if (new_depth <= bqt->real_max_depth) {
>                 bqt->max_depth = new_depth;
>                 return 0;
> 
> --
> Dan
> 
> [1]: http://marc.info/?l=linux-kernel&m=132204370518629&w=2
> --
> 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 RESEND] block: warn if tag is greater than real_max_depth.

[Dan Williams]

On Mon, Dec 19, 2011 at 5:11 PM, Tao Ma <tm@tao.ma> wrote:
>>> Looks good, better than what we had. Applied.
>>
>> This appears to interact badly with scsi_adjust_queue_depth() when the
>> tag space shrinks.  I can reproduce a similar crash as reported in
>> "3.2-rc2+git: kernel BUG at block/blk-core.c:1000!
>> (__scsi_queue_insert)" [1].
>>
>> I can hit "kernel BUG at block/blk-core.c:2268!" which is the same
>> BUG_ON(blk_queued_rq(rq)) check reliably with:
>> # for i in $(seq 0 10); do dd if=/dev/zero of=/dev/sdX & done
>> # echo 4 > /sys/class/block/sdX/device/queue_depth
>>
>> The following fixes it for me, if this looks ok (versus reverting
>> commit 5e081591) I'll roll it into a formal patch with Ed and Meelis'
>> Reported-by.
> Interesting. If I read the code correctly, real_max_depth is the maximum
> queue depth we ever have and max_depth is the current depth.
>
> In your fix, we never resize the tag size to be smaller than max_depth.
> So I think this patch does expose some problem, but not lead to the BUG.

Yes, if we keep the "if (unlikely(tag >= bqt->max_depth))" check in
blk_queue_end_tag() then the side effect is that we can never shrink
the tag depth, which I don't think was intended.

> And in your new comment, you mentioned that "request between new_depth
> and max_depth can be in-flight", but max_depth <= real_max_depth, so
> what's wrong with the comment? Sorry, but am I missing something here?

Prior to the change blk_queue_end_tag() would continue to complete
requests with a tag > max_depth, now it silently drops them on the
floor leaving BUG_ON(blk_queued_rq(rq)) to trigger when we try to end
the request

> Having said that, I have tried the test in my machine here, but have no
> luck by now. My kernel version is 3.2.0-rc5+.

What controller are you using?  I am using the isci driver with a sas
disk.  Meelis saw it with aic7xxx on a parallel scsi disk.

--
Dan

Re: [PATCH RESEND] block: warn if tag is greater than real_max_depth.

[Jens Axboe]

On 2011-12-20 02:45, Dan Williams wrote:
> On Mon, Dec 19, 2011 at 5:11 PM, Tao Ma <tm@tao.ma> wrote:
>>>> Looks good, better than what we had. Applied.
>>>
>>> This appears to interact badly with scsi_adjust_queue_depth() when the
>>> tag space shrinks.  I can reproduce a similar crash as reported in
>>> "3.2-rc2+git: kernel BUG at block/blk-core.c:1000!
>>> (__scsi_queue_insert)" [1].
>>>
>>> I can hit "kernel BUG at block/blk-core.c:2268!" which is the same
>>> BUG_ON(blk_queued_rq(rq)) check reliably with:
>>> # for i in $(seq 0 10); do dd if=/dev/zero of=/dev/sdX & done
>>> # echo 4 > /sys/class/block/sdX/device/queue_depth
>>>
>>> The following fixes it for me, if this looks ok (versus reverting
>>> commit 5e081591) I'll roll it into a formal patch with Ed and Meelis'
>>> Reported-by.
>> Interesting. If I read the code correctly, real_max_depth is the maximum
>> queue depth we ever have and max_depth is the current depth.
>>
>> In your fix, we never resize the tag size to be smaller than max_depth.
>> So I think this patch does expose some problem, but not lead to the BUG.
> 
> Yes, if we keep the "if (unlikely(tag >= bqt->max_depth))" check in
> blk_queue_end_tag() then the side effect is that we can never shrink
> the tag depth, which I don't think was intended.
> 
>> And in your new comment, you mentioned that "request between new_depth
>> and max_depth can be in-flight", but max_depth <= real_max_depth, so
>> what's wrong with the comment? Sorry, but am I missing something here?
> 
> Prior to the change blk_queue_end_tag() would continue to complete
> requests with a tag > max_depth, now it silently drops them on the
> floor leaving BUG_ON(blk_queued_rq(rq)) to trigger when we try to end
> the request

Yeah, that's just wrong. Tao Ma, which bug was the original fix intended
to fix?

The reason we have these two ceilings is exactly for shrinking depth
situations. It's quite legal to have an inflight request with a tag
inbetween max_depth and real_max_depth.

-- 
Jens Axboe

Re: [PATCH RESEND] block: warn if tag is greater than real_max_depth.

[Tao Ma]

On 12/20/2011 09:56 PM, Jens Axboe wrote:
> On 2011-12-20 02:45, Dan Williams wrote:
>> On Mon, Dec 19, 2011 at 5:11 PM, Tao Ma <tm@tao.ma> wrote:
>>>>> Looks good, better than what we had. Applied.
>>>>
>>>> This appears to interact badly with scsi_adjust_queue_depth() when the
>>>> tag space shrinks.  I can reproduce a similar crash as reported in
>>>> "3.2-rc2+git: kernel BUG at block/blk-core.c:1000!
>>>> (__scsi_queue_insert)" [1].
>>>>
>>>> I can hit "kernel BUG at block/blk-core.c:2268!" which is the same
>>>> BUG_ON(blk_queued_rq(rq)) check reliably with:
>>>> # for i in $(seq 0 10); do dd if=/dev/zero of=/dev/sdX & done
>>>> # echo 4 > /sys/class/block/sdX/device/queue_depth
>>>>
>>>> The following fixes it for me, if this looks ok (versus reverting
>>>> commit 5e081591) I'll roll it into a formal patch with Ed and Meelis'
>>>> Reported-by.
>>> Interesting. If I read the code correctly, real_max_depth is the maximum
>>> queue depth we ever have and max_depth is the current depth.
>>>
>>> In your fix, we never resize the tag size to be smaller than max_depth.
>>> So I think this patch does expose some problem, but not lead to the BUG.
>>
>> Yes, if we keep the "if (unlikely(tag >= bqt->max_depth))" check in
>> blk_queue_end_tag() then the side effect is that we can never shrink
>> the tag depth, which I don't think was intended.
>>
>>> And in your new comment, you mentioned that "request between new_depth
>>> and max_depth can be in-flight", but max_depth <= real_max_depth, so
>>> what's wrong with the comment? Sorry, but am I missing something here?
>>
>> Prior to the change blk_queue_end_tag() would continue to complete
>> requests with a tag > max_depth, now it silently drops them on the
>> floor leaving BUG_ON(blk_queued_rq(rq)) to trigger when we try to end
>> the request
Oh, I see. So maybe we should modify blk_queue_end_tag instead of it?
> 
> Yeah, that' is just wrong. Tao Ma, which bug was the original fix intended
> to fix?
uh, actually there is no original bug for it.
> 
> The reason we have these two ceilings is exactly for shrinking depth
> situations. It's quite legal to have an inflight request with a tag
> inbetween max_depth and real_max_depth.
yeah, so I guess we should fix that in blk_queue_end_tag instead of
reverting this check.

Thanks
Tao

[PATCH] block: warn the wrong tag only if it is greater than real_max_depth.

[Tao Ma]

From: Tao Ma <boyu.mt@taobao.com>

In queue depth decreases, we could meet with a tag greater than max_depth,
but it should never be greater than real_max_depth. So only WARN if
it is greater than real_max_depth and let it complets if it is greater
than max_depth.

Reported-by: Dan Williams <dan.j.williams@intel.com>
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Tao Ma <boyu.mt@taobao.com>
---
 block/blk-tag.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

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

Re: [PATCH] block: warn the wrong tag only if it is greater than real_max_depth.

[Williams, Dan J]

[ adding original bug reporters ]

On Tue, Dec 20, 2011 at 7:58 AM, Tao Ma <tm@tao.ma> wrote:
> From: Tao Ma <boyu.mt@taobao.com>
>
> In queue depth decreases, we could meet with a tag greater than max_depth,
> but it should never be greater than real_max_depth. So only WARN if
> it is greater than real_max_depth and let it complets if it is greater
> than max_depth.
>
> Reported-by: Dan Williams <dan.j.williams@intel.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Tao Ma <boyu.mt@taobao.com>
> ---
>  block/blk-tag.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/block/blk-tag.c b/block/blk-tag.c
> index e74d6d1..1c50d34 100644
> --- a/block/blk-tag.c
> +++ b/block/blk-tag.c
> @@ -286,12 +286,12 @@ void blk_queue_end_tag(struct request_queue *q, struct request *rq)
>
>        BUG_ON(tag == -1);
>
> -       if (unlikely(tag >= bqt->max_depth)) {
> +       if (unlikely(tag >= bqt->real_max_depth)) {
>                /*
> -                * This can happen after tag depth has been reduced.
> -                * But tag shouldn't be larger than real_max_depth.
> +                * tag shouldn't be larger than real_max_depth.
>                 */
> -               WARN_ON(tag >= bqt->real_max_depth);
> +               WARN(1, "tag %d, bqt->real_max_depth %d\n",
> +                    tag, bqt->real_max_depth);
>                return;
>        }

You can use the WARN macro in the if() statement, and maybe it should
only trigger once??

diff --git a/block/blk-tag.c b/block/blk-tag.c
index 918d82c..19cdde4 100644
--- a/block/blk-tag.c
+++ b/block/blk-tag.c
@@ -291,12 +291,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;
        }