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

[PATCH V2] 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>
---
V2:
	- change 'if else' to switch as suggested by Christoph

 block/blk-mq.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 65e0846fd065..cc85775fc372 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1407,30 +1407,28 @@ 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) {
-			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);
 
-- 
2.25.2

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

[Ming Lei]

On Wed, Jul 01, 2020 at 09:58:57PM +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.
> 
> 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>
> ---
> V2:
> 	- change 'if else' to switch as suggested by Christoph
> 
>  block/blk-mq.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 65e0846fd065..cc85775fc372 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1407,30 +1407,28 @@ 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) {
> -			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);
>  
> -- 
> 2.25.2
> 

Hello,

Ping...

-- 
Ming

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

[Johannes Thumshirn]

Looks good to me,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

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

[John Garry]

On 08/07/2020 13:27, Ming Lei wrote:
> k;
> -		} 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

question not on this patch specifically: is this supposed to be 
"driver", and not "drive"? "driver" is mentioned earlier in the function

>   			 * accept.
>   			 */
>   			blk_mq_handle_zone_resource(rq, &zone_list);
> -			if (list_empty(list))
> -				break;
> -			continue;
> -		}
> -

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

[Johannes Thumshirn]

On 08/07/2020 16:06, John Garry wrote:
> On 08/07/2020 13:27, Ming Lei wrote:
>> k;
>> -		} 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
> 
> question not on this patch specifically: is this supposed to be 
> "driver", and not "drive"? "driver" is mentioned earlier in the function

No it's drive, the sole purpose of BLK_STS_ZONE_RESOURCE is the apply some back 
pressure for IO submitters to a SMR disk and the zone-append emulation it uses.
See drivers/scsi/sd_zbc.c for details.

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

[Ming Lei]

On Wed, Jul 08, 2020 at 03:05:03PM +0100, John Garry wrote:
> On 08/07/2020 13:27, Ming Lei wrote:
> > k;
> > -		} 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
> 
> question not on this patch specifically: is this supposed to be "driver",
> and not "drive"? "driver" is mentioned earlier in the function

Hi John,

Please focus on change added by this patch instead of existed context code.

If you have question on existed code, start a new thread for the discussion.

Thanks,
Ming

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

[Jens Axboe]

On 7/1/20 7:58 AM, 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.

Applied, thanks.

-- 
Jens Axboe