Re: [PATCH] blk-mq: Put driver tag in blk_mq_dispatch_rq_list() when no budget

Re: [PATCH] blk-mq: Put driver tag in blk_mq_dispatch_rq_list() when no budget

[Doug Anderson]

Hi,

On Mon, Apr 20, 2020 at 1:23 AM John Garry <john.garry@huawei.com> wrote:
>
> On 18/04/2020 03:43, Bart Van Assche wrote:
> > On 2020-04-16 04:18, John Garry wrote:
> >> If in blk_mq_dispatch_rq_list() we find no budget, then we break of the
> >> dispatch loop, but the request may keep the driver tag, evaulated
> >> in 'nxt' in the previous loop iteration.
> >>
> >> Fix by putting the driver tag for that request.
> >>
> >> Signed-off-by: John Garry <john.garry@huawei.com>
> >>
> >> diff --git a/block/blk-mq.c b/block/blk-mq.c
> >> index 8e56884fd2e9..a7785df2c944 100644
> >> --- a/block/blk-mq.c
> >> +++ b/block/blk-mq.c
> >> @@ -1222,8 +1222,10 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
> >>              rq = list_first_entry(list, struct request, queuelist);
> >>
> >>              hctx = rq->mq_hctx;
> >> -            if (!got_budget && !blk_mq_get_dispatch_budget(hctx))
> >> +            if (!got_budget && !blk_mq_get_dispatch_budget(hctx)) {
> >> +                    blk_mq_put_driver_tag(rq);
> >>                      break;
> >> +            }
> >>
> >>              if (!blk_mq_get_driver_tag(rq)) {
> >>                      /*
> >
> > Is this something that can only happen if q->mq_ops->queue_rq(hctx, &bd)
> > returns another value than BLK_STS_OK, BLK_STS_RESOURCE and
> > BLK_STS_DEV_RESOURCE?
>
> Right, as that case is handled in blk_mq_handle_dev_resource()
>
> If so, please add a comment in the source code
> > that explains this.
>
> So important that we should now do this in an extra patch?
>
> >
> > Is this perhaps a bug fix for 0bca799b9280 ("blk-mq: order getting
> > budget and driver tag")? If so, please mention this and add Cc tags for
> > the people who were Cc-ed on that patch.
>
> So it looks like 0bca799b9280 had a flaw, but I am not sure if anything
> got broken there and worthy of stable backport.
>
> I found this issue while debugging Ming's blk-mq cpu hotplug patchset,
> which I feel is ready to merge.
>
> Having said that, this nasty issue did take > 1 day for me to debug...
> so let me know.

As per the above conversation, presumably this should go to stable
then for any kernel that has commit 0bca799b9280 ("blk-mq: order
getting budget and driver tag")?  For instance, I think 4.19 would be
affected?  When I picked it there I got a conflict due to not having
commit ea4f995ee8b8 ("blk-mq: cache request hardware queue mapping")
but I think it's just a context collision and easy to resolve.

I'm no expert in the block code, but I posted my backport to 4.19 at
<https://crrev.com/c/2163313>.  I'm happy to send an email as a patch
to the list too or double-check that someone else's conflict
resolution matches mine.

-Doug

Re: [PATCH] blk-mq: Put driver tag in blk_mq_dispatch_rq_list() when no budget

[Ming Lei]

On Thu, Apr 23, 2020 at 03:42:37PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Mon, Apr 20, 2020 at 1:23 AM John Garry <john.garry@huawei.com> wrote:
> >
> > On 18/04/2020 03:43, Bart Van Assche wrote:
> > > On 2020-04-16 04:18, John Garry wrote:
> > >> If in blk_mq_dispatch_rq_list() we find no budget, then we break of the
> > >> dispatch loop, but the request may keep the driver tag, evaulated
> > >> in 'nxt' in the previous loop iteration.
> > >>
> > >> Fix by putting the driver tag for that request.
> > >>
> > >> Signed-off-by: John Garry <john.garry@huawei.com>
> > >>
> > >> diff --git a/block/blk-mq.c b/block/blk-mq.c
> > >> index 8e56884fd2e9..a7785df2c944 100644
> > >> --- a/block/blk-mq.c
> > >> +++ b/block/blk-mq.c
> > >> @@ -1222,8 +1222,10 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
> > >>              rq = list_first_entry(list, struct request, queuelist);
> > >>
> > >>              hctx = rq->mq_hctx;
> > >> -            if (!got_budget && !blk_mq_get_dispatch_budget(hctx))
> > >> +            if (!got_budget && !blk_mq_get_dispatch_budget(hctx)) {
> > >> +                    blk_mq_put_driver_tag(rq);
> > >>                      break;
> > >> +            }
> > >>
> > >>              if (!blk_mq_get_driver_tag(rq)) {
> > >>                      /*
> > >
> > > Is this something that can only happen if q->mq_ops->queue_rq(hctx, &bd)
> > > returns another value than BLK_STS_OK, BLK_STS_RESOURCE and
> > > BLK_STS_DEV_RESOURCE?
> >
> > Right, as that case is handled in blk_mq_handle_dev_resource()
> >
> > If so, please add a comment in the source code
> > > that explains this.
> >
> > So important that we should now do this in an extra patch?
> >
> > >
> > > Is this perhaps a bug fix for 0bca799b9280 ("blk-mq: order getting
> > > budget and driver tag")? If so, please mention this and add Cc tags for
> > > the people who were Cc-ed on that patch.
> >
> > So it looks like 0bca799b9280 had a flaw, but I am not sure if anything
> > got broken there and worthy of stable backport.
> >
> > I found this issue while debugging Ming's blk-mq cpu hotplug patchset,
> > which I feel is ready to merge.
> >
> > Having said that, this nasty issue did take > 1 day for me to debug...
> > so let me know.
> 
> As per the above conversation, presumably this should go to stable
> then for any kernel that has commit 0bca799b9280 ("blk-mq: order
> getting budget and driver tag")?  For instance, I think 4.19 would be
> affected?  When I picked it there I got a conflict due to not having
> commit ea4f995ee8b8 ("blk-mq: cache request hardware queue mapping")
> but I think it's just a context collision and easy to resolve.
> 
> I'm no expert in the block code, but I posted my backport to 4.19 at
> <https://crrev.com/c/2163313>.  I'm happy to send an email as a patch
> to the list too or double-check that someone else's conflict
> resolution matches mine.

The thing is that there may not user visible effect by this issue,
when one tag isn't freed, this request will be re-dispatched soon.
That said it just makes the tag lifetime longer.

It could only be an issue in case of request dependency, meantime
the tag space is quite limited. However, not sure if there is such
case in reality.


Thanks,
Ming

Re: [PATCH] blk-mq: Put driver tag in blk_mq_dispatch_rq_list() when no budget

[John Garry]

On 24/04/2020 02:35, Ming Lei wrote:
> On Thu, Apr 23, 2020 at 03:42:37PM -0700, Doug Anderson wrote:
>> Hi,
>>
>> On Mon, Apr 20, 2020 at 1:23 AM John Garry <john.garry@huawei.com> wrote:
>>>
>>> On 18/04/2020 03:43, Bart Van Assche wrote:
>>>> On 2020-04-16 04:18, John Garry wrote:
>>>>> If in blk_mq_dispatch_rq_list() we find no budget, then we break of the
>>>>> dispatch loop, but the request may keep the driver tag, evaulated
>>>>> in 'nxt' in the previous loop iteration.
>>>>>
>>>>> Fix by putting the driver tag for that request.
>>>>>
>>>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>>>>
>>>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>>>> index 8e56884fd2e9..a7785df2c944 100644
>>>>> --- a/block/blk-mq.c
>>>>> +++ b/block/blk-mq.c
>>>>> @@ -1222,8 +1222,10 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
>>>>>               rq = list_first_entry(list, struct request, queuelist);
>>>>>
>>>>>               hctx = rq->mq_hctx;
>>>>> -            if (!got_budget && !blk_mq_get_dispatch_budget(hctx))
>>>>> +            if (!got_budget && !blk_mq_get_dispatch_budget(hctx)) {
>>>>> +                    blk_mq_put_driver_tag(rq);
>>>>>                       break;
>>>>> +            }
>>>>>
>>>>>               if (!blk_mq_get_driver_tag(rq)) {
>>>>>                       /*
>>>>
>>>> Is this something that can only happen if q->mq_ops->queue_rq(hctx, &bd)
>>>> returns another value than BLK_STS_OK, BLK_STS_RESOURCE and
>>>> BLK_STS_DEV_RESOURCE?
>>>
>>> Right, as that case is handled in blk_mq_handle_dev_resource()
>>>
>>> If so, please add a comment in the source code
>>>> that explains this.
>>>
>>> So important that we should now do this in an extra patch?
>>>
>>>>
>>>> Is this perhaps a bug fix for 0bca799b9280 ("blk-mq: order getting
>>>> budget and driver tag")? If so, please mention this and add Cc tags for
>>>> the people who were Cc-ed on that patch.
>>>
>>> So it looks like 0bca799b9280 had a flaw, but I am not sure if anything
>>> got broken there and worthy of stable backport.
>>>
>>> I found this issue while debugging Ming's blk-mq cpu hotplug patchset,
>>> which I feel is ready to merge.
>>>
>>> Having said that, this nasty issue did take > 1 day for me to debug...
>>> so let me know.
>>
>> As per the above conversation, presumably this should go to stable
>> then for any kernel that has commit 0bca799b9280 ("blk-mq: order
>> getting budget and driver tag")?  For instance, I think 4.19 would be
>> affected?  When I picked it there I got a conflict due to not having
>> commit ea4f995ee8b8 ("blk-mq: cache request hardware queue mapping")
>> but I think it's just a context collision and easy to resolve.
>>
>> I'm no expert in the block code, but I posted my backport to 4.19 at
>> <https://crrev.com/c/2163313>.  I'm happy to send an email as a patch
>> to the list too or double-check that someone else's conflict
>> resolution matches mine.
> 
> The thing is that there may not user visible effect by this issue,
> when one tag isn't freed, this request will be re-dispatched soon.
> That said it just makes the tag lifetime longer.
> 
> It could only be an issue in case of request dependency, meantime
> the tag space is quite limited. However, not sure if there is such
> case in reality.
> 

FWIW, this was pretty nasty to debug, and if it's not going to cause 
harm, then I'd be more inclined to add to stable. In addition, some 
distro may backport patches on a stable baseline where it is visible 
separately, and miss this one.

Thanks,
John