[PATCH] blk-mq: streamline handling of q->mq_ops->queue_rq result

[PATCH] blk-mq: streamline handling of q->mq_ops->queue_rq result

[Ming Lei]

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

Re: [PATCH] blk-mq: streamline handling of q->mq_ops->queue_rq result

[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);
 

Re: [PATCH] blk-mq: streamline handling of q->mq_ops->queue_rq result

[Johannes Thumshirn]

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.

Re: [PATCH] blk-mq: streamline handling of q->mq_ops->queue_rq result

[Ming Lei]

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