* [PATCH] blk-mq: put the driver tag of nxt rq before first one is requeued
@ 2017-09-12 17:14 Jianchao Wang
2017-09-12 10:23 ` Ming Lei
0 siblings, 1 reply; 13+ messages in thread
From: Jianchao Wang @ 2017-09-12 17:14 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, linux-kernel, Jianchao Wang
When free the driver tag of the next rq with I/O scheduler
configured, it get the first entry of the list, however, at the
moment, the failed rq has been requeued at the head of the list.
The rq it gets is the failed rq not the next rq.
Free the driver tag of next rq before the failed one is requeued
in the failure branch of queue_rq callback and it is just needed
there.
Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com>
---
block/blk-mq.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 4603b11..19f848e 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -983,7 +983,7 @@ static bool blk_mq_dispatch_wait_add(struct blk_mq_hw_ctx *hctx)
bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list)
{
struct blk_mq_hw_ctx *hctx;
- struct request *rq;
+ struct request *rq, *nxt;
int errors, queued;
if (list_empty(list))
@@ -1029,14 +1029,20 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list)
if (list_empty(list))
bd.last = true;
else {
- struct request *nxt;
-
nxt = list_first_entry(list, struct request, queuelist);
bd.last = !blk_mq_get_driver_tag(nxt, NULL, false);
}
ret = q->mq_ops->queue_rq(hctx, &bd);
if (ret == BLK_STS_RESOURCE) {
+ /*
+ * If an I/O scheduler has been configured and we got a
+ * driver tag for the next request already, free it again.
+ */
+ if (!list_empty(list)) {
+ nxt = list_first_entry(list, struct request, queuelist);
+ blk_mq_put_driver_tag(nxt);
+ }
blk_mq_put_driver_tag_hctx(hctx, rq);
list_add(&rq->queuelist, list);
__blk_mq_requeue_request(rq);
@@ -1059,13 +1065,6 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list)
* that is where we will continue on next queue run.
*/
if (!list_empty(list)) {
- /*
- * If an I/O scheduler has been configured and we got a driver
- * tag for the next request already, free it again.
- */
- rq = list_first_entry(list, struct request, queuelist);
- blk_mq_put_driver_tag(rq);
-
spin_lock(&hctx->lock);
list_splice_init(list, &hctx->dispatch);
spin_unlock(&hctx->lock);
--
2.7.4
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] blk-mq: put the driver tag of nxt rq before first one is requeued
2017-09-12 17:14 [PATCH] blk-mq: put the driver tag of nxt rq before first one is requeued Jianchao Wang
@ 2017-09-12 10:23 ` Ming Lei
2017-09-13 1:01 ` jianchao.wang
0 siblings, 1 reply; 13+ messages in thread
From: Ming Lei @ 2017-09-12 10:23 UTC (permalink / raw)
To: Jianchao Wang; +Cc: Jens Axboe, linux-block, linux-kernel
On Wed, Sep 13, 2017 at 01:14:35AM +0800, Jianchao Wang wrote:
> When free the driver tag of the next rq with I/O scheduler
> configured, it get the first entry of the list, however, at the
> moment, the failed rq has been requeued at the head of the list.
> The rq it gets is the failed rq not the next rq.
> Free the driver tag of next rq before the failed one is requeued
> in the failure branch of queue_rq callback and it is just needed
> there.
Looks a good catch.
>
> Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com>
> ---
> block/blk-mq.c | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 4603b11..19f848e 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -983,7 +983,7 @@ static bool blk_mq_dispatch_wait_add(struct blk_mq_hw_ctx *hctx)
> bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list)
> {
> struct blk_mq_hw_ctx *hctx;
> - struct request *rq;
> + struct request *rq, *nxt;
> int errors, queued;
>
> if (list_empty(list))
> @@ -1029,14 +1029,20 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list)
> if (list_empty(list))
> bd.last = true;
> else {
> - struct request *nxt;
> -
> nxt = list_first_entry(list, struct request, queuelist);
> bd.last = !blk_mq_get_driver_tag(nxt, NULL, false);
> }
>
> ret = q->mq_ops->queue_rq(hctx, &bd);
> if (ret == BLK_STS_RESOURCE) {
> + /*
> + * If an I/O scheduler has been configured and we got a
> + * driver tag for the next request already, free it again.
> + */
> + if (!list_empty(list)) {
> + nxt = list_first_entry(list, struct request, queuelist);
> + blk_mq_put_driver_tag(nxt);
> + }
The following way might be more simple and clean:
if (nxt)
blk_mq_put_driver_tag(nxt);
meantime 'nxt' need to be cleared inside the 'if (list_empty(list))'
before .queue_rq().
--
Ming
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] blk-mq: put the driver tag of nxt rq before first one is requeued
2017-09-12 10:23 ` Ming Lei
@ 2017-09-13 1:01 ` jianchao.wang
2017-09-13 1:24 ` Ming Lei
0 siblings, 1 reply; 13+ messages in thread
From: jianchao.wang @ 2017-09-13 1:01 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, linux-block, linux-kernel
Hi ming
On 09/12/2017 06:23 PM, Ming Lei wrote:
>> @@ -1029,14 +1029,20 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list)
>> if (list_empty(list))
>> bd.last = true;
>> else {
>> - struct request *nxt;
>> -
>> nxt = list_first_entry(list, struct request, queuelist);
>> bd.last = !blk_mq_get_driver_tag(nxt, NULL, false);
>> }
>>
>> ret = q->mq_ops->queue_rq(hctx, &bd);
>> if (ret == BLK_STS_RESOURCE) {
>> + /*
>> + * If an I/O scheduler has been configured and we got a
>> + * driver tag for the next request already, free it again.
>> + */
>> + if (!list_empty(list)) {
>> + nxt = list_first_entry(list, struct request, queuelist);
>> + blk_mq_put_driver_tag(nxt);
>> + }
> The following way might be more simple and clean:
>
> if (nxt)
> blk_mq_put_driver_tag(nxt);
>
> meantime 'nxt' need to be cleared inside the 'if (list_empty(list))'
> before .queue_rq().
I had ever thought about that, but to avoid add extra command in the
fast path, I made the patch above.
Thanks
Jianchao
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] blk-mq: put the driver tag of nxt rq before first one is requeued
2017-09-13 1:01 ` jianchao.wang
@ 2017-09-13 1:24 ` Ming Lei
2017-09-13 1:39 ` jianchao.wang
0 siblings, 1 reply; 13+ messages in thread
From: Ming Lei @ 2017-09-13 1:24 UTC (permalink / raw)
To: jianchao.wang; +Cc: Jens Axboe, linux-block, linux-kernel
On Wed, Sep 13, 2017 at 09:01:25AM +0800, jianchao.wang wrote:
> Hi ming
>
> On 09/12/2017 06:23 PM, Ming Lei wrote:
> >> @@ -1029,14 +1029,20 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list)
> >> if (list_empty(list))
> >> bd.last = true;
> >> else {
> >> - struct request *nxt;
> >> -
> >> nxt = list_first_entry(list, struct request, queuelist);
> >> bd.last = !blk_mq_get_driver_tag(nxt, NULL, false);
> >> }
> >>
> >> ret = q->mq_ops->queue_rq(hctx, &bd);
> >> if (ret == BLK_STS_RESOURCE) {
> >> + /*
> >> + * If an I/O scheduler has been configured and we got a
> >> + * driver tag for the next request already, free it again.
> >> + */
> >> + if (!list_empty(list)) {
> >> + nxt = list_first_entry(list, struct request, queuelist);
> >> + blk_mq_put_driver_tag(nxt);
> >> + }
> > The following way might be more simple and clean:
> >
> > if (nxt)
> > blk_mq_put_driver_tag(nxt);
> >
> > meantime 'nxt' need to be cleared inside the 'if (list_empty(list))'
> > before .queue_rq().
>
> I had ever thought about that, but to avoid add extra command in the
> fast path, I made the patch above.
Got it, so how about changing to the following way simply:
if (nxt && !list_empty(list))
blk_mq_put_driver_tag(nxt);
--
Ming
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] blk-mq: put the driver tag of nxt rq before first one is requeued
2017-09-13 1:24 ` Ming Lei
@ 2017-09-13 1:39 ` jianchao.wang
2017-09-13 1:52 ` Ming Lei
2017-09-13 2:23 ` Jens Axboe
0 siblings, 2 replies; 13+ messages in thread
From: jianchao.wang @ 2017-09-13 1:39 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, linux-block, linux-kernel
On 09/13/2017 09:24 AM, Ming Lei wrote:
> On Wed, Sep 13, 2017 at 09:01:25AM +0800, jianchao.wang wrote:
>> Hi ming
>>
>> On 09/12/2017 06:23 PM, Ming Lei wrote:
>>>> @@ -1029,14 +1029,20 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list)
>>>> if (list_empty(list))
>>>> bd.last = true;
>>>> else {
>>>> - struct request *nxt;
>>>> -
>>>> nxt = list_first_entry(list, struct request, queuelist);
>>>> bd.last = !blk_mq_get_driver_tag(nxt, NULL, false);
>>>> }
>>>>
>>>> ret = q->mq_ops->queue_rq(hctx, &bd);
>>>> if (ret == BLK_STS_RESOURCE) {
>>>> + /*
>>>> + * If an I/O scheduler has been configured and we got a
>>>> + * driver tag for the next request already, free it again.
>>>> + */
>>>> + if (!list_empty(list)) {
>>>> + nxt = list_first_entry(list, struct request, queuelist);
>>>> + blk_mq_put_driver_tag(nxt);
>>>> + }
>>> The following way might be more simple and clean:
>>>
>>> if (nxt)
>>> blk_mq_put_driver_tag(nxt);
>>>
>>> meantime 'nxt' need to be cleared inside the 'if (list_empty(list))'
>>> before .queue_rq().
>>
>> I had ever thought about that, but to avoid add extra command in the
>> fast path, I made the patch above.
>
> Got it, so how about changing to the following way simply:
>
> if (nxt && !list_empty(list))
> blk_mq_put_driver_tag(nxt);
>
It seems that we even could change it as following:
if (!list_empty(list))
blk_mq_put_driver_tag(nxt);
thanks
jianchao
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] blk-mq: put the driver tag of nxt rq before first one is requeued
2017-09-13 1:39 ` jianchao.wang
@ 2017-09-13 1:52 ` Ming Lei
2017-09-13 2:23 ` Jens Axboe
1 sibling, 0 replies; 13+ messages in thread
From: Ming Lei @ 2017-09-13 1:52 UTC (permalink / raw)
To: jianchao.wang; +Cc: Jens Axboe, linux-block, linux-kernel
On Wed, Sep 13, 2017 at 09:39:44AM +0800, jianchao.wang wrote:
>
>
> On 09/13/2017 09:24 AM, Ming Lei wrote:
> > On Wed, Sep 13, 2017 at 09:01:25AM +0800, jianchao.wang wrote:
> >> Hi ming
> >>
> >> On 09/12/2017 06:23 PM, Ming Lei wrote:
> >>>> @@ -1029,14 +1029,20 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list)
> >>>> if (list_empty(list))
> >>>> bd.last = true;
> >>>> else {
> >>>> - struct request *nxt;
> >>>> -
> >>>> nxt = list_first_entry(list, struct request, queuelist);
> >>>> bd.last = !blk_mq_get_driver_tag(nxt, NULL, false);
> >>>> }
> >>>>
> >>>> ret = q->mq_ops->queue_rq(hctx, &bd);
> >>>> if (ret == BLK_STS_RESOURCE) {
> >>>> + /*
> >>>> + * If an I/O scheduler has been configured and we got a
> >>>> + * driver tag for the next request already, free it again.
> >>>> + */
> >>>> + if (!list_empty(list)) {
> >>>> + nxt = list_first_entry(list, struct request, queuelist);
> >>>> + blk_mq_put_driver_tag(nxt);
> >>>> + }
> >>> The following way might be more simple and clean:
> >>>
> >>> if (nxt)
> >>> blk_mq_put_driver_tag(nxt);
> >>>
> >>> meantime 'nxt' need to be cleared inside the 'if (list_empty(list))'
> >>> before .queue_rq().
> >>
> >> I had ever thought about that, but to avoid add extra command in the
> >> fast path, I made the patch above.
> >
> > Got it, so how about changing to the following way simply:
> >
> > if (nxt && !list_empty(list))
> > blk_mq_put_driver_tag(nxt);
> >
> It seems that we even could change it as following:
> if (!list_empty(list))
> blk_mq_put_driver_tag(nxt);
Yeah, that is definitely more clean, :-)
Feel free to add "Reviewed-by: Ming Lei <ming.lei@redhat.com>"
in your next post.
--
Ming
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] blk-mq: put the driver tag of nxt rq before first one is requeued
2017-09-13 1:39 ` jianchao.wang
2017-09-13 1:52 ` Ming Lei
@ 2017-09-13 2:23 ` Jens Axboe
2017-09-13 2:42 ` jianchao.wang
1 sibling, 1 reply; 13+ messages in thread
From: Jens Axboe @ 2017-09-13 2:23 UTC (permalink / raw)
To: jianchao.wang, Ming Lei; +Cc: linux-block, linux-kernel
On 09/12/2017 07:39 PM, jianchao.wang wrote:
>
>
> On 09/13/2017 09:24 AM, Ming Lei wrote:
>> On Wed, Sep 13, 2017 at 09:01:25AM +0800, jianchao.wang wrote:
>>> Hi ming
>>>
>>> On 09/12/2017 06:23 PM, Ming Lei wrote:
>>>>> @@ -1029,14 +1029,20 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list)
>>>>> if (list_empty(list))
>>>>> bd.last = true;
>>>>> else {
>>>>> - struct request *nxt;
>>>>> -
>>>>> nxt = list_first_entry(list, struct request, queuelist);
>>>>> bd.last = !blk_mq_get_driver_tag(nxt, NULL, false);
>>>>> }
>>>>>
>>>>> ret = q->mq_ops->queue_rq(hctx, &bd);
>>>>> if (ret == BLK_STS_RESOURCE) {
>>>>> + /*
>>>>> + * If an I/O scheduler has been configured and we got a
>>>>> + * driver tag for the next request already, free it again.
>>>>> + */
>>>>> + if (!list_empty(list)) {
>>>>> + nxt = list_first_entry(list, struct request, queuelist);
>>>>> + blk_mq_put_driver_tag(nxt);
>>>>> + }
>>>> The following way might be more simple and clean:
>>>>
>>>> if (nxt)
>>>> blk_mq_put_driver_tag(nxt);
>>>>
>>>> meantime 'nxt' need to be cleared inside the 'if (list_empty(list))'
>>>> before .queue_rq().
>>>
>>> I had ever thought about that, but to avoid add extra command in the
>>> fast path, I made the patch above.
>>
>> Got it, so how about changing to the following way simply:
>>
>> if (nxt && !list_empty(list))
>> blk_mq_put_driver_tag(nxt);
>>
> It seems that we even could change it as following:
> if (!list_empty(list))
> blk_mq_put_driver_tag(nxt);
This is starting to get too clever for its own good, I generally don't
like to sacrifice readability for performance. In reality, the compiler
probably figures it out anyway...
So either make it explicit, or add a nice comment as to why it is the
way that it is.
--
Jens Axboe
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] blk-mq: put the driver tag of nxt rq before first one is requeued
2017-09-13 2:23 ` Jens Axboe
@ 2017-09-13 2:42 ` jianchao.wang
2017-09-13 2:45 ` Jens Axboe
0 siblings, 1 reply; 13+ messages in thread
From: jianchao.wang @ 2017-09-13 2:42 UTC (permalink / raw)
To: Jens Axboe, Ming Lei; +Cc: linux-block, linux-kernel
On 09/13/2017 10:23 AM, Jens Axboe wrote:
> On 09/12/2017 07:39 PM, jianchao.wang wrote:
>>
>>
>> On 09/13/2017 09:24 AM, Ming Lei wrote:
>>> On Wed, Sep 13, 2017 at 09:01:25AM +0800, jianchao.wang wrote:
>>>> Hi ming
>>>>
>>>> On 09/12/2017 06:23 PM, Ming Lei wrote:
>>>>>> @@ -1029,14 +1029,20 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list)
>>>>>> if (list_empty(list))
>>>>>> bd.last = true;
>>>>>> else {
>>>>>> - struct request *nxt;
>>>>>> -
>>>>>> nxt = list_first_entry(list, struct request, queuelist);
>>>>>> bd.last = !blk_mq_get_driver_tag(nxt, NULL, false);
>>>>>> }
>>>>>>
>>>>>> ret = q->mq_ops->queue_rq(hctx, &bd);
>>>>>> if (ret == BLK_STS_RESOURCE) {
>>>>>> + /*
>>>>>> + * If an I/O scheduler has been configured and we got a
>>>>>> + * driver tag for the next request already, free it again.
>>>>>> + */
>>>>>> + if (!list_empty(list)) {
>>>>>> + nxt = list_first_entry(list, struct request, queuelist);
>>>>>> + blk_mq_put_driver_tag(nxt);
>>>>>> + }
>>>>> The following way might be more simple and clean:
>>>>>
>>>>> if (nxt)
>>>>> blk_mq_put_driver_tag(nxt);
>>>>>
>>>>> meantime 'nxt' need to be cleared inside the 'if (list_empty(list))'
>>>>> before .queue_rq().
>>>>
>>>> I had ever thought about that, but to avoid add extra command in the
>>>> fast path, I made the patch above.
>>>
>>> Got it, so how about changing to the following way simply:
>>>
>>> if (nxt && !list_empty(list))
>>> blk_mq_put_driver_tag(nxt);
>>>
>> It seems that we even could change it as following:
>> if (!list_empty(list))
>> blk_mq_put_driver_tag(nxt);
>
> This is starting to get too clever for its own good, I generally don't
> like to sacrifice readability for performance. In reality, the compiler
> probably figures it out anyway...
>
> So either make it explicit, or add a nice comment as to why it is the
> way that it is.
>
yes, it indeed leads to compiler warning of "may be used uninitialized"
maybe the original one could be taken back.
if (!list_empty(list)) {
nxt = list_first_entry(list, struct request, queuelist);
blk_mq_put_driver_tag(nxt);
}
It is more readable and could avoid the warning.
Thanks
jianchao
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] blk-mq: put the driver tag of nxt rq before first one is requeued
2017-09-13 2:42 ` jianchao.wang
@ 2017-09-13 2:45 ` Jens Axboe
2017-09-13 3:39 ` jianchao.wang
2017-09-13 3:50 ` Ming Lei
0 siblings, 2 replies; 13+ messages in thread
From: Jens Axboe @ 2017-09-13 2:45 UTC (permalink / raw)
To: jianchao.wang, Ming Lei; +Cc: linux-block, linux-kernel
On 09/12/2017 08:42 PM, jianchao.wang wrote:
>
>
> On 09/13/2017 10:23 AM, Jens Axboe wrote:
>> On 09/12/2017 07:39 PM, jianchao.wang wrote:
>>>
>>>
>>> On 09/13/2017 09:24 AM, Ming Lei wrote:
>>>> On Wed, Sep 13, 2017 at 09:01:25AM +0800, jianchao.wang wrote:
>>>>> Hi ming
>>>>>
>>>>> On 09/12/2017 06:23 PM, Ming Lei wrote:
>>>>>>> @@ -1029,14 +1029,20 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list)
>>>>>>> if (list_empty(list))
>>>>>>> bd.last = true;
>>>>>>> else {
>>>>>>> - struct request *nxt;
>>>>>>> -
>>>>>>> nxt = list_first_entry(list, struct request, queuelist);
>>>>>>> bd.last = !blk_mq_get_driver_tag(nxt, NULL, false);
>>>>>>> }
>>>>>>>
>>>>>>> ret = q->mq_ops->queue_rq(hctx, &bd);
>>>>>>> if (ret == BLK_STS_RESOURCE) {
>>>>>>> + /*
>>>>>>> + * If an I/O scheduler has been configured and we got a
>>>>>>> + * driver tag for the next request already, free it again.
>>>>>>> + */
>>>>>>> + if (!list_empty(list)) {
>>>>>>> + nxt = list_first_entry(list, struct request, queuelist);
>>>>>>> + blk_mq_put_driver_tag(nxt);
>>>>>>> + }
>>>>>> The following way might be more simple and clean:
>>>>>>
>>>>>> if (nxt)
>>>>>> blk_mq_put_driver_tag(nxt);
>>>>>>
>>>>>> meantime 'nxt' need to be cleared inside the 'if (list_empty(list))'
>>>>>> before .queue_rq().
>>>>>
>>>>> I had ever thought about that, but to avoid add extra command in the
>>>>> fast path, I made the patch above.
>>>>
>>>> Got it, so how about changing to the following way simply:
>>>>
>>>> if (nxt && !list_empty(list))
>>>> blk_mq_put_driver_tag(nxt);
>>>>
>>> It seems that we even could change it as following:
>>> if (!list_empty(list))
>>> blk_mq_put_driver_tag(nxt);
>>
>> This is starting to get too clever for its own good, I generally don't
>> like to sacrifice readability for performance. In reality, the compiler
>> probably figures it out anyway...
>>
>> So either make it explicit, or add a nice comment as to why it is the
>> way that it is.
>>
> yes, it indeed leads to compiler warning of "may be used uninitialized"
> maybe the original one could be taken back.
> if (!list_empty(list)) {
> nxt = list_first_entry(list, struct request, queuelist);
> blk_mq_put_driver_tag(nxt);
> }
> It is more readable and could avoid the warning.
Exactly, and especially the readability is the key element here. It's
just not worth it to try and be too clever, especially not for something
like this. When you read the above, you immediately know what the code
does without needing a comment. That's not true for the other construct.
You both have to read other parts of the function to figure out what it
does, AND read the entire function to ensure it always does the right
thing. Fragile.
--
Jens Axboe
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] blk-mq: put the driver tag of nxt rq before first one is requeued
2017-09-13 2:45 ` Jens Axboe
@ 2017-09-13 3:39 ` jianchao.wang
2017-09-13 3:54 ` Jens Axboe
2017-09-13 3:50 ` Ming Lei
1 sibling, 1 reply; 13+ messages in thread
From: jianchao.wang @ 2017-09-13 3:39 UTC (permalink / raw)
To: Jens Axboe, Ming Lei; +Cc: linux-block, linux-kernel
On 09/13/2017 10:45 AM, Jens Axboe wrote:
>>>>>>>> @@ -1029,14 +1029,20 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list)
>>>>>>>> if (list_empty(list))
>>>>>>>> bd.last = true;
>>>>>>>> else {
>>>>>>>> - struct request *nxt;
>>>>>>>> -
>>>>>>>> nxt = list_first_entry(list, struct request, queuelist);
>>>>>>>> bd.last = !blk_mq_get_driver_tag(nxt, NULL, false);
>>>>>>>> }
>>>>>>>>
>>>>>>>> ret = q->mq_ops->queue_rq(hctx, &bd);
>>>>>>>> if (ret == BLK_STS_RESOURCE) {
>>>>>>>> + /*
>>>>>>>> + * If an I/O scheduler has been configured and we got a
>>>>>>>> + * driver tag for the next request already, free it again.
>>>>>>>> + */
>>>>>>>> + if (!list_empty(list)) {
>>>>>>>> + nxt = list_first_entry(list, struct request, queuelist);
>>>>>>>> + blk_mq_put_driver_tag(nxt);
>>>>>>>> + }
>>>>>>> The following way might be more simple and clean:
>>>>>>>
>>>>>>> if (nxt)
>>>>>>> blk_mq_put_driver_tag(nxt);
>>>>>>>
>>>>>>> meantime 'nxt' need to be cleared inside the 'if (list_empty(list))'
>>>>>>> before .queue_rq().
>>>>>> I had ever thought about that, but to avoid add extra command in the
>>>>>> fast path, I made the patch above.
>>>>> Got it, so how about changing to the following way simply:
>>>>>
>>>>> if (nxt && !list_empty(list))
>>>>> blk_mq_put_driver_tag(nxt);
>>>>>
>>>> It seems that we even could change it as following:
>>>> if (!list_empty(list))
>>>> blk_mq_put_driver_tag(nxt);
>>> This is starting to get too clever for its own good, I generally don't
>>> like to sacrifice readability for performance. In reality, the compiler
>>> probably figures it out anyway...
>>>
>>> So either make it explicit, or add a nice comment as to why it is the
>>> way that it is.
>>>
>> yes, it indeed leads to compiler warning of "may be used uninitialized"
>> maybe the original one could be taken back.
>> if (!list_empty(list)) {
>> nxt = list_first_entry(list, struct request, queuelist);
>> blk_mq_put_driver_tag(nxt);
>> }
>> It is more readable and could avoid the warning.
> Exactly, and especially the readability is the key element here. It's
> just not worth it to try and be too clever, especially not for something
> like this. When you read the above, you immediately know what the code
> does without needing a comment. That's not true for the other construct.
> You both have to read other parts of the function to figure out what it
> does, AND read the entire function to ensure it always does the right
> thing. Fragile.
Thanks for your comments , jens and ming. I＇m really appreciative of that.
About the fragility, do you mean the possibility that may release the tag of the next rq
which has a driver tag itself (maybe a flush) ?
Thanks
jianchao
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] blk-mq: put the driver tag of nxt rq before first one is requeued
2017-09-13 3:39 ` jianchao.wang
@ 2017-09-13 3:54 ` Jens Axboe
2017-09-13 3:59 ` jianchao.wang
0 siblings, 1 reply; 13+ messages in thread
From: Jens Axboe @ 2017-09-13 3:54 UTC (permalink / raw)
To: jianchao.wang, Ming Lei; +Cc: linux-block, linux-kernel
On 09/12/2017 09:39 PM, jianchao.wang wrote:
>> Exactly, and especially the readability is the key element here. It's
>> just not worth it to try and be too clever, especially not for
>> something like this. When you read the above, you immediately know
>> what the code does without needing a comment. That's not true for the
>> other construct. You both have to read other parts of the function
>> to figure out what it does, AND read the entire function to ensure it
>> always does the right thing. Fragile.
>
> Thanks for your comments , jens and ming. I＇m really appreciative of
> that. About the fragility, do you mean the possibility that may
> release the tag of the next rq which has a driver tag itself (maybe a
> flush) ?
I mean that if you do:
if (!list_empty(list))
blk_mq_put_driver_tag(nxt);
it's fragile code in the sense that changes elsewhere in the function
are harder to validate and/or can impact the functionality of that
simple if and tag put.
The actual release must always be safe, of course.
--
Jens Axboe
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] blk-mq: put the driver tag of nxt rq before first one is requeued
2017-09-13 3:54 ` Jens Axboe
@ 2017-09-13 3:59 ` jianchao.wang
0 siblings, 0 replies; 13+ messages in thread
From: jianchao.wang @ 2017-09-13 3:59 UTC (permalink / raw)
To: Jens Axboe, Ming Lei; +Cc: linux-block, linux-kernel
On 09/13/2017 11:54 AM, Jens Axboe wrote:
> On 09/12/2017 09:39 PM, jianchao.wang wrote:
>>> Exactly, and especially the readability is the key element here. It's
>>> just not worth it to try and be too clever, especially not for
>>> something like this. When you read the above, you immediately know
>>> what the code does without needing a comment. That's not true for the
>>> other construct. You both have to read other parts of the function
>>> to figure out what it does, AND read the entire function to ensure it
>>> always does the right thing. Fragile.
>>
>> Thanks for your comments , jens and ming. I＇m really appreciative of
>> that. About the fragility, do you mean the possibility that may
>> release the tag of the next rq which has a driver tag itself (maybe a
>> flush) ?
>
> I mean that if you do:
>
> if (!list_empty(list))
> blk_mq_put_driver_tag(nxt);
>
> it's fragile code in the sense that changes elsewhere in the function
> are harder to validate and/or can impact the functionality of that
> simple if and tag put.
>
> The actual release must always be safe, of course.
>
Got it, thanks a lot.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] blk-mq: put the driver tag of nxt rq before first one is requeued
2017-09-13 2:45 ` Jens Axboe
2017-09-13 3:39 ` jianchao.wang
@ 2017-09-13 3:50 ` Ming Lei
1 sibling, 0 replies; 13+ messages in thread
From: Ming Lei @ 2017-09-13 3:50 UTC (permalink / raw)
To: Jens Axboe; +Cc: jianchao.wang, linux-block, linux-kernel
On Tue, Sep 12, 2017 at 08:45:19PM -0600, Jens Axboe wrote:
> On 09/12/2017 08:42 PM, jianchao.wang wrote:
> >
> >
> > On 09/13/2017 10:23 AM, Jens Axboe wrote:
> >> On 09/12/2017 07:39 PM, jianchao.wang wrote:
> >>>
> >>>
> >>> On 09/13/2017 09:24 AM, Ming Lei wrote:
> >>>> On Wed, Sep 13, 2017 at 09:01:25AM +0800, jianchao.wang wrote:
> >>>>> Hi ming
> >>>>>
> >>>>> On 09/12/2017 06:23 PM, Ming Lei wrote:
> >>>>>>> @@ -1029,14 +1029,20 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list)
> >>>>>>> if (list_empty(list))
> >>>>>>> bd.last = true;
> >>>>>>> else {
> >>>>>>> - struct request *nxt;
> >>>>>>> -
> >>>>>>> nxt = list_first_entry(list, struct request, queuelist);
> >>>>>>> bd.last = !blk_mq_get_driver_tag(nxt, NULL, false);
> >>>>>>> }
> >>>>>>>
> >>>>>>> ret = q->mq_ops->queue_rq(hctx, &bd);
> >>>>>>> if (ret == BLK_STS_RESOURCE) {
> >>>>>>> + /*
> >>>>>>> + * If an I/O scheduler has been configured and we got a
> >>>>>>> + * driver tag for the next request already, free it again.
> >>>>>>> + */
> >>>>>>> + if (!list_empty(list)) {
> >>>>>>> + nxt = list_first_entry(list, struct request, queuelist);
> >>>>>>> + blk_mq_put_driver_tag(nxt);
> >>>>>>> + }
> >>>>>> The following way might be more simple and clean:
> >>>>>>
> >>>>>> if (nxt)
> >>>>>> blk_mq_put_driver_tag(nxt);
> >>>>>>
> >>>>>> meantime 'nxt' need to be cleared inside the 'if (list_empty(list))'
> >>>>>> before .queue_rq().
> >>>>>
> >>>>> I had ever thought about that, but to avoid add extra command in the
> >>>>> fast path, I made the patch above.
> >>>>
> >>>> Got it, so how about changing to the following way simply:
> >>>>
> >>>> if (nxt && !list_empty(list))
> >>>> blk_mq_put_driver_tag(nxt);
> >>>>
> >>> It seems that we even could change it as following:
> >>> if (!list_empty(list))
> >>> blk_mq_put_driver_tag(nxt);
> >>
> >> This is starting to get too clever for its own good, I generally don't
> >> like to sacrifice readability for performance. In reality, the compiler
> >> probably figures it out anyway...
> >>
> >> So either make it explicit, or add a nice comment as to why it is the
> >> way that it is.
> >>
> > yes, it indeed leads to compiler warning of "may be used uninitialized"
> > maybe the original one could be taken back.
> > if (!list_empty(list)) {
> > nxt = list_first_entry(list, struct request, queuelist);
> > blk_mq_put_driver_tag(nxt);
> > }
> > It is more readable and could avoid the warning.
>
> Exactly, and especially the readability is the key element here. It's
> just not worth it to try and be too clever, especially not for something
> like this. When you read the above, you immediately know what the code
> does without needing a comment. That's not true for the other construct.
> You both have to read other parts of the function to figure out what it
> does, AND read the entire function to ensure it always does the right
> thing. Fragile.
OK, fair enough wrt. readability.
Reviewed-by: Ming Lei <ming.lei@redhat.com>
For the original post.
--
Ming
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-09-13 3:59 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-12 17:14 [PATCH] blk-mq: put the driver tag of nxt rq before first one is requeued Jianchao Wang
2017-09-12 10:23 ` Ming Lei
2017-09-13 1:01 ` jianchao.wang
2017-09-13 1:24 ` Ming Lei
2017-09-13 1:39 ` jianchao.wang
2017-09-13 1:52 ` Ming Lei
2017-09-13 2:23 ` Jens Axboe
2017-09-13 2:42 ` jianchao.wang
2017-09-13 2:45 ` Jens Axboe
2017-09-13 3:39 ` jianchao.wang
2017-09-13 3:54 ` Jens Axboe
2017-09-13 3:59 ` jianchao.wang
2017-09-13 3:50 ` Ming Lei
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.