| Message ID | 1587035931-125028-1-git-send-email-john.garry@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-mq: Put driver tag in blk_mq_dispatch_rq_list() when no budget | expand |
On Thu, Apr 16, 2020 at 07:18:51PM +0800, 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)) { > /* > -- > 2.16.4 > Reviewed-by: Ming Lei <ming.lei@redhat.com>
On 4/16/20 5:18 AM, 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. Applied, thanks.
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? If so, please add a comment in the source code that explains this. 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. Thanks, Bart.
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. Thanks, John
On 4/20/20 1:22 AM, John Garry 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? Probably not, unless someone else feels strongly about this. Thanks, Bart.
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
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
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
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)) { /*
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>