* [PATCH] blk-mq: streamline handling of q->mq_ops->queue_rq result
@ 2020-07-01 10:16 Ming Lei
2020-07-01 12:54 ` Christoph Hellwig
0 siblings, 1 reply; 4+ messages in thread
From: Ming Lei @ 2020-07-01 10:16 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Ming Lei, Christoph Hellwig
Current handling of q->mq_ops->queue_rq result is a bit ugly:
- two branches which needs to 'continue' have to check if the
dispatch local list is empty, otherwise one bad request may
be retrieved via 'rq = list_first_entry(list, struct request, queuelist);'
- the branch of 'if (unlikely(ret != BLK_STS_OK))' isn't easy
to follow, since it is actually one error branch.
Streamline this handling, so the code becomes more readable, meantime
potential kernel oops can be avoided in case that the last request in
local dispatch list is failed.
Fixes: fc17b6534eb8 ("blk-mq: switch ->queue_rq return value to blk_status_t")
Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
block/blk-mq.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 65e0846fd065..c3862144a2a7 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1407,7 +1407,10 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list,
if (nr_budgets)
nr_budgets--;
ret = q->mq_ops->queue_rq(hctx, &bd);
- if (ret == BLK_STS_RESOURCE || ret == BLK_STS_DEV_RESOURCE) {
+ if (likely(ret == BLK_STS_OK)) {
+ queued++;
+ } else if (ret == BLK_STS_RESOURCE ||
+ ret == BLK_STS_DEV_RESOURCE) {
blk_mq_handle_dev_resource(rq, list);
break;
} else if (ret == BLK_STS_ZONE_RESOURCE) {
@@ -1417,18 +1420,10 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list,
* accept.
*/
blk_mq_handle_zone_resource(rq, &zone_list);
- if (list_empty(list))
- break;
- continue;
- }
-
- if (unlikely(ret != BLK_STS_OK)) {
+ } else {
errors++;
blk_mq_end_request(rq, BLK_STS_IOERR);
- continue;
}
-
- queued++;
} while (!list_empty(list));
if (!list_empty(&zone_list))
--
2.25.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] blk-mq: streamline handling of q->mq_ops->queue_rq result
2020-07-01 10:16 [PATCH] blk-mq: streamline handling of q->mq_ops->queue_rq result Ming Lei
@ 2020-07-01 12:54 ` Christoph Hellwig
2020-07-01 12:58 ` Johannes Thumshirn
2020-07-01 13:51 ` Ming Lei
0 siblings, 2 replies; 4+ messages in thread
From: Christoph Hellwig @ 2020-07-01 12:54 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, linux-block, Christoph Hellwig
On Wed, Jul 01, 2020 at 06:16:17PM +0800, Ming Lei wrote:
> Current handling of q->mq_ops->queue_rq result is a bit ugly:
>
> - two branches which needs to 'continue' have to check if the
> dispatch local list is empty, otherwise one bad request may
> be retrieved via 'rq = list_first_entry(list, struct request, queuelist);'
>
> - the branch of 'if (unlikely(ret != BLK_STS_OK))' isn't easy
> to follow, since it is actually one error branch.
>
> Streamline this handling, so the code becomes more readable, meantime
> potential kernel oops can be avoided in case that the last request in
> local dispatch list is failed.
I don't really find that much easier to read. If we want to clean
this up for rea we should use a proper switch statement. Something like
this:
diff --git a/block/blk-mq.c b/block/blk-mq.c
index a9aa6d1e44cf32..f3721f274b800e 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1275,30 +1275,28 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
}
ret = q->mq_ops->queue_rq(hctx, &bd);
- if (ret == BLK_STS_RESOURCE || ret == BLK_STS_DEV_RESOURCE) {
- blk_mq_handle_dev_resource(rq, list);
+ switch (ret) {
+ case BLK_STS_OK:
+ queued++;
break;
- } else if (ret == BLK_STS_ZONE_RESOURCE) {
+ case BLK_STS_RESOURCE:
+ case BLK_STS_DEV_RESOURCE:
+ blk_mq_handle_dev_resource(rq, list);
+ goto out;
+ case BLK_STS_ZONE_RESOURCE:
/*
* Move the request to zone_list and keep going through
* the dispatch list to find more requests the drive can
* accept.
*/
blk_mq_handle_zone_resource(rq, &zone_list);
- if (list_empty(list))
- break;
- continue;
- }
-
- if (unlikely(ret != BLK_STS_OK)) {
+ break;
+ default:
errors++;
blk_mq_end_request(rq, BLK_STS_IOERR);
- continue;
}
-
- queued++;
} while (!list_empty(list));
-
+out:
if (!list_empty(&zone_list))
list_splice_tail_init(&zone_list, list);
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] blk-mq: streamline handling of q->mq_ops->queue_rq result
2020-07-01 12:54 ` Christoph Hellwig
@ 2020-07-01 12:58 ` Johannes Thumshirn
2020-07-01 13:51 ` Ming Lei
1 sibling, 0 replies; 4+ messages in thread
From: Johannes Thumshirn @ 2020-07-01 12:58 UTC (permalink / raw)
To: hch, Ming Lei; +Cc: Jens Axboe, linux-block
On 01/07/2020 14:54, Christoph Hellwig wrote:
> On Wed, Jul 01, 2020 at 06:16:17PM +0800, Ming Lei wrote:
>> Current handling of q->mq_ops->queue_rq result is a bit ugly:
>>
>> - two branches which needs to 'continue' have to check if the
>> dispatch local list is empty, otherwise one bad request may
>> be retrieved via 'rq = list_first_entry(list, struct request, queuelist);'
>>
>> - the branch of 'if (unlikely(ret != BLK_STS_OK))' isn't easy
>> to follow, since it is actually one error branch.
>>
>> Streamline this handling, so the code becomes more readable, meantime
>> potential kernel oops can be avoided in case that the last request in
>> local dispatch list is failed.
>
> I don't really find that much easier to read. If we want to clean
> this up for rea we should use a proper switch statement. Something like
> this:
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index a9aa6d1e44cf32..f3721f274b800e 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1275,30 +1275,28 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
> }
>
> ret = q->mq_ops->queue_rq(hctx, &bd);
> - if (ret == BLK_STS_RESOURCE || ret == BLK_STS_DEV_RESOURCE) {
> - blk_mq_handle_dev_resource(rq, list);
> + switch (ret) {
> + case BLK_STS_OK:
> + queued++;
> break;
> - } else if (ret == BLK_STS_ZONE_RESOURCE) {
> + case BLK_STS_RESOURCE:
> + case BLK_STS_DEV_RESOURCE:
> + blk_mq_handle_dev_resource(rq, list);
> + goto out;
> + case BLK_STS_ZONE_RESOURCE:
> /*
> * Move the request to zone_list and keep going through
> * the dispatch list to find more requests the drive can
> * accept.
> */
> blk_mq_handle_zone_resource(rq, &zone_list);
> - if (list_empty(list))
> - break;
> - continue;
> - }
> -
> - if (unlikely(ret != BLK_STS_OK)) {
> + break;
> + default:
> errors++;
> blk_mq_end_request(rq, BLK_STS_IOERR);
> - continue;
> }
> -
> - queued++;
> } while (!list_empty(list));
> -
> +out:
> if (!list_empty(&zone_list))
> list_splice_tail_init(&zone_list, list);
>
>
Agreed, also the presence of a switch (ret) makes it more than obvious that ret
has more than one case.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] blk-mq: streamline handling of q->mq_ops->queue_rq result
2020-07-01 12:54 ` Christoph Hellwig
2020-07-01 12:58 ` Johannes Thumshirn
@ 2020-07-01 13:51 ` Ming Lei
1 sibling, 0 replies; 4+ messages in thread
From: Ming Lei @ 2020-07-01 13:51 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Jens Axboe, linux-block
On Wed, Jul 01, 2020 at 01:54:09PM +0100, Christoph Hellwig wrote:
> On Wed, Jul 01, 2020 at 06:16:17PM +0800, Ming Lei wrote:
> > Current handling of q->mq_ops->queue_rq result is a bit ugly:
> >
> > - two branches which needs to 'continue' have to check if the
> > dispatch local list is empty, otherwise one bad request may
> > be retrieved via 'rq = list_first_entry(list, struct request, queuelist);'
> >
> > - the branch of 'if (unlikely(ret != BLK_STS_OK))' isn't easy
> > to follow, since it is actually one error branch.
> >
> > Streamline this handling, so the code becomes more readable, meantime
> > potential kernel oops can be avoided in case that the last request in
> > local dispatch list is failed.
>
> I don't really find that much easier to read. If we want to clean
> this up for rea we should use a proper switch statement. Something like
> this:
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index a9aa6d1e44cf32..f3721f274b800e 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1275,30 +1275,28 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
> }
>
> ret = q->mq_ops->queue_rq(hctx, &bd);
> - if (ret == BLK_STS_RESOURCE || ret == BLK_STS_DEV_RESOURCE) {
> - blk_mq_handle_dev_resource(rq, list);
> + switch (ret) {
> + case BLK_STS_OK:
> + queued++;
> break;
> - } else if (ret == BLK_STS_ZONE_RESOURCE) {
> + case BLK_STS_RESOURCE:
> + case BLK_STS_DEV_RESOURCE:
> + blk_mq_handle_dev_resource(rq, list);
> + goto out;
> + case BLK_STS_ZONE_RESOURCE:
> /*
> * Move the request to zone_list and keep going through
> * the dispatch list to find more requests the drive can
> * accept.
> */
> blk_mq_handle_zone_resource(rq, &zone_list);
> - if (list_empty(list))
> - break;
> - continue;
> - }
> -
> - if (unlikely(ret != BLK_STS_OK)) {
> + break;
> + default:
> errors++;
> blk_mq_end_request(rq, BLK_STS_IOERR);
> - continue;
> }
> -
> - queued++;
> } while (!list_empty(list));
> -
> +out:
> if (!list_empty(&zone_list))
> list_splice_tail_init(&zone_list, list);
I am fine to switch back to 'switch'. I doesn't take 'switch' because you
changed 'switch' to 'if else' before.
Thanks,
Ming
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-07-01 13:51 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-01 10:16 [PATCH] blk-mq: streamline handling of q->mq_ops->queue_rq result Ming Lei
2020-07-01 12:54 ` Christoph Hellwig
2020-07-01 12:58 ` Johannes Thumshirn
2020-07-01 13:51 ` 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.