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@xxxxxxxxx> 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@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxxx>
>>> Signed-off-by: Tao Ma <boyu.mt@xxxxxxxxxx>
>>> ---
>>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/