[PATCH 0/6] Some cleanups for blk-core.c and blk-flush.c

[PATCH 0/6] Some cleanups for blk-core.c and blk-flush.c

[Guoqing Jiang]

Hi,

This patchset updates code and comment in blk-core.c and blk-flush.c.
The first patch had been sent before, now update it with the Reviewed-by
tag from Bart.

Thanks,
Guoqing

Guoqing Jiang (6):
  block: fix comment for blk_cloned_rq_check_limits
  block: use bio_{wouldblock,io}_error in direct_make_request
  block: remove redundant setting of QUEUE_FLAG_DYING
  block: remove obsolete comments for _blk/blk_rq_prep_clone
  block: remove unneeded argument from blk_alloc_flush_queue
  block: cleanup comment for blk_flush_complete_seq

 block/blk-core.c  | 11 +++--------
 block/blk-flush.c |  7 ++-----
 block/blk-mq.c    |  3 +--
 block/blk.h       |  4 ++--
 4 files changed, 8 insertions(+), 17 deletions(-)

-- 
2.17.1

[PATCH 1/6] block: fix comment for blk_cloned_rq_check_limits

[Guoqing Jiang]

Since the later description mentioned "checked against the new queue
limits", so make the change to avoid confusion.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
---
 block/blk-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 089e890ab208..fd43266029be 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1203,7 +1203,7 @@ EXPORT_SYMBOL(submit_bio);
 
 /**
  * blk_cloned_rq_check_limits - Helper function to check a cloned request
- *                              for new the queue limits
+ *                              for the new queue limits
  * @q:  the queue
  * @rq: the request being checked
  *
-- 
2.17.1

[PATCH 2/6] block: use bio_{wouldblock,io}_error in direct_make_request

[Guoqing Jiang]

Use the two functions to simplify code.

Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
---
 block/blk-core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index fd43266029be..6d36c2ad40ba 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1121,10 +1121,9 @@ blk_qc_t direct_make_request(struct bio *bio)
 
 	if (unlikely(blk_queue_enter(q, nowait ? BLK_MQ_REQ_NOWAIT : 0))) {
 		if (nowait && !blk_queue_dying(q))
-			bio->bi_status = BLK_STS_AGAIN;
+			bio_wouldblock_error(bio);
 		else
-			bio->bi_status = BLK_STS_IOERR;
-		bio_endio(bio);
+			bio_io_error(bio);
 		return BLK_QC_T_NONE;
 	}
 
-- 
2.17.1

[PATCH 3/6] block: remove redundant setting of QUEUE_FLAG_DYING

[Guoqing Jiang]

Previously, blk_cleanup_queue has called blk_set_queue_dying to set the
flag, no need to do it again.

Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
---
 block/blk-core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 6d36c2ad40ba..883ffda216e4 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -346,7 +346,6 @@ void blk_cleanup_queue(struct request_queue *q)
 
 	blk_queue_flag_set(QUEUE_FLAG_NOMERGES, q);
 	blk_queue_flag_set(QUEUE_FLAG_NOXMERGES, q);
-	blk_queue_flag_set(QUEUE_FLAG_DYING, q);
 
 	/*
 	 * Drain all requests queued before DYING marking. Set DEAD flag to
-- 
2.17.1

[PATCH 4/6] block: remove obsolete comments for _blk/blk_rq_prep_clone

[Guoqing Jiang]

Both cmd and sense had been moved to scsi_request, so remove
the related comments to avoid confusion.

Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
---
 block/blk-core.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 883ffda216e4..9094fd7d1b01 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1583,7 +1583,6 @@ EXPORT_SYMBOL_GPL(blk_rq_unprep_clone);
 
 /*
  * Copy attributes of the original request to the clone request.
- * The actual data parts (e.g. ->cmd, ->sense) are not copied.
  */
 static void __blk_rq_prep_clone(struct request *dst, struct request *src)
 {
@@ -1610,8 +1609,6 @@ static void __blk_rq_prep_clone(struct request *dst, struct request *src)
  *
  * Description:
  *     Clones bios in @rq_src to @rq, and copies attributes of @rq_src to @rq.
- *     The actual data parts of @rq_src (e.g. ->cmd, ->sense)
- *     are not copied, and copying such parts is the caller's responsibility.
  *     Also, pages which the original bios are pointing to are not copied
  *     and the cloned bios just point same pages.
  *     So cloned bios must be completed before original bios, which means
-- 
2.17.1

[PATCH 5/6] block: remove unneeded argument from blk_alloc_flush_queue

[Guoqing Jiang]

Remove 'q' from arguments since it is not used anymore after
commit 7e992f847a08e ("block: remove non mq parts from the
flush code").

Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
---
 block/blk-flush.c | 4 ++--
 block/blk-mq.c    | 3 +--
 block/blk.h       | 4 ++--
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/block/blk-flush.c b/block/blk-flush.c
index 3f977c517960..963ae56d5aae 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -485,8 +485,8 @@ int blkdev_issue_flush(struct block_device *bdev, gfp_t gfp_mask,
 }
 EXPORT_SYMBOL(blkdev_issue_flush);
 
-struct blk_flush_queue *blk_alloc_flush_queue(struct request_queue *q,
-		int node, int cmd_size, gfp_t flags)
+struct blk_flush_queue *blk_alloc_flush_queue(int node, int cmd_size,
+					      gfp_t flags)
 {
 	struct blk_flush_queue *fq;
 	int rq_sz = sizeof(struct request);
diff --git a/block/blk-mq.c b/block/blk-mq.c
index a12b1763508d..9684ea9a9e1f 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2405,8 +2405,7 @@ blk_mq_alloc_hctx(struct request_queue *q, struct blk_mq_tag_set *set,
 	init_waitqueue_func_entry(&hctx->dispatch_wait, blk_mq_dispatch_wake);
 	INIT_LIST_HEAD(&hctx->dispatch_wait.entry);
 
-	hctx->fq = blk_alloc_flush_queue(q, hctx->numa_node, set->cmd_size,
-			gfp);
+	hctx->fq = blk_alloc_flush_queue(hctx->numa_node, set->cmd_size, gfp);
 	if (!hctx->fq)
 		goto free_bitmap;
 
diff --git a/block/blk.h b/block/blk.h
index 0b8884353f6b..670337b7cfa0 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -55,8 +55,8 @@ is_flush_rq(struct request *req, struct blk_mq_hw_ctx *hctx)
 	return hctx->fq->flush_rq == req;
 }
 
-struct blk_flush_queue *blk_alloc_flush_queue(struct request_queue *q,
-		int node, int cmd_size, gfp_t flags);
+struct blk_flush_queue *blk_alloc_flush_queue(int node, int cmd_size,
+					      gfp_t flags);
 void blk_free_flush_queue(struct blk_flush_queue *q);
 
 void blk_freeze_queue(struct request_queue *q);
-- 
2.17.1

[PATCH 6/6] block: cleanup comment for blk_flush_complete_seq

[Guoqing Jiang]

Remove the comment about return value, since it is not valid after
commit 404b8f5a03d84 ("block: cleanup kick/queued handling").

Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
---
 block/blk-flush.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/block/blk-flush.c b/block/blk-flush.c
index 963ae56d5aae..46fd6e83dd79 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -160,9 +160,6 @@ static void blk_account_io_flush(struct request *rq)
  *
  * CONTEXT:
  * spin_lock_irq(fq->mq_flush_lock)
- *
- * RETURNS:
- * %true if requests were added to the dispatch queue, %false otherwise.
  */
 static void blk_flush_complete_seq(struct request *rq,
 				   struct blk_flush_queue *fq,
-- 
2.17.1

Re: [PATCH 2/6] block: use bio_{wouldblock,io}_error in direct_make_request

[Nikolay Borisov]

On 28.02.20 г. 17:05 ч., Guoqing Jiang wrote:
> Use the two functions to simplify code.
> 
> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
> ---
>  block/blk-core.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index fd43266029be..6d36c2ad40ba 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1121,10 +1121,9 @@ blk_qc_t direct_make_request(struct bio *bio)
>  
>  	if (unlikely(blk_queue_enter(q, nowait ? BLK_MQ_REQ_NOWAIT : 0))) {
>  		if (nowait && !blk_queue_dying(q))
> -			bio->bi_status = BLK_STS_AGAIN;
> +			bio_wouldblock_error(bio);
>  		else
> -			bio->bi_status = BLK_STS_IOERR;
> -		bio_endio(bio);
> +			bio_io_error(bio);
>  		return BLK_QC_T_NONE;
>  	}
>  
> 

The code is functionally identical so:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Re: [PATCH 3/6] block: remove redundant setting of QUEUE_FLAG_DYING

[Nikolay Borisov]

On 28.02.20 г. 17:05 ч., Guoqing Jiang wrote:
> Previously, blk_cleanup_queue has called blk_set_queue_dying to set the
> flag, no need to do it again.
> 
> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
> ---
>  block/blk-core.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 6d36c2ad40ba..883ffda216e4 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -346,7 +346,6 @@ void blk_cleanup_queue(struct request_queue *q)
>  
>  	blk_queue_flag_set(QUEUE_FLAG_NOMERGES, q);
>  	blk_queue_flag_set(QUEUE_FLAG_NOXMERGES, q);
> -	blk_queue_flag_set(QUEUE_FLAG_DYING, q);
>  
>  	/*
>  	 * Drain all requests queued before DYING marking. Set DEAD flag to
> 


Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Re: [PATCH 5/6] block: remove unneeded argument from blk_alloc_flush_queue

[Nikolay Borisov]

On 28.02.20 г. 17:05 ч., Guoqing Jiang wrote:
> Remove 'q' from arguments since it is not used anymore after
> commit 7e992f847a08e ("block: remove non mq parts from the
> flush code").
> 
> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Re: [PATCH 6/6] block: cleanup comment for blk_flush_complete_seq

[Nikolay Borisov]

On 28.02.20 г. 17:05 ч., Guoqing Jiang wrote:
> Remove the comment about return value, since it is not valid after
> commit 404b8f5a03d84 ("block: cleanup kick/queued handling").
> 
> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>

Obviously correct, given the function is void.

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Re: [PATCH 2/6] block: use bio_{wouldblock,io}_error in direct_make_request

[Bart Van Assche]

On 2020-02-28 07:05, Guoqing Jiang wrote:
> Use the two functions to simplify code.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 3/6] block: remove redundant setting of QUEUE_FLAG_DYING

[Bart Van Assche]

On 2020-02-28 07:05, Guoqing Jiang wrote:
> Previously, blk_cleanup_queue has called blk_set_queue_dying to set the
> flag, no need to do it again.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 4/6] block: remove obsolete comments for _blk/blk_rq_prep_clone

[Bart Van Assche]

On 2020-02-28 07:05, Guoqing Jiang wrote:
> Both cmd and sense had been moved to scsi_request, so remove
> the related comments to avoid confusion.
> 
> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
> ---
>  block/blk-core.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 883ffda216e4..9094fd7d1b01 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1583,7 +1583,6 @@ EXPORT_SYMBOL_GPL(blk_rq_unprep_clone);
>  
>  /*
>   * Copy attributes of the original request to the clone request.
> - * The actual data parts (e.g. ->cmd, ->sense) are not copied.
>   */
>  static void __blk_rq_prep_clone(struct request *dst, struct request *src)
>  {

Although the removed comment is outdated, the new comment is not useful.

How about inlining __blk_rq_prep_clone() into its only caller and
removing the comment above that function entirely? It's not clear to me
why the code inside __blk_rq_prep_clone() ever was put into a separate
function.

>   *
>   * Description:
>   *     Clones bios in @rq_src to @rq, and copies attributes of @rq_src to @rq.
> - *     The actual data parts of @rq_src (e.g. ->cmd, ->sense)
> - *     are not copied, and copying such parts is the caller's responsibility.
>   *     Also, pages which the original bios are pointing to are not copied
>   *     and the cloned bios just point same pages.
>   *     So cloned bios must be completed before original bios, which means

Adding a comment that explains that some but not all struct request
members are copied and also why would be welcome.

Thanks,

Bart.

Re: [PATCH 5/6] block: remove unneeded argument from blk_alloc_flush_queue

[Bart Van Assche]

On 2020-02-28 07:05, Guoqing Jiang wrote:
> Remove 'q' from arguments since it is not used anymore after
> commit 7e992f847a08e ("block: remove non mq parts from the
> flush code").

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 6/6] block: cleanup comment for blk_flush_complete_seq

[Bart Van Assche]

On 2020-02-28 07:05, Guoqing Jiang wrote:
> Remove the comment about return value, since it is not valid after
> commit 404b8f5a03d84 ("block: cleanup kick/queued handling").

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 1/6] block: fix comment for blk_cloned_rq_check_limits

[Chaitanya Kulkarni]

Looks good to me.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
On 02/28/2020 07:06 AM, Guoqing Jiang wrote:
> Since the later description mentioned "checked against the new queue
> limits", so make the change to avoid confusion.
>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
> ---
>   block/blk-core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 089e890ab208..fd43266029be 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1203,7 +1203,7 @@ EXPORT_SYMBOL(submit_bio);
>
>   /**
>    * blk_cloned_rq_check_limits - Helper function to check a cloned request
> - *                              for new the queue limits
> + *                              for the new queue limits
>    * @q:  the queue
>    * @rq: the request being checked
>    *
>

Re: [PATCH 2/6] block: use bio_{wouldblock,io}_error in direct_make_request

[Chaitanya Kulkarni]

Neat, looks good to me.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
On 02/28/2020 07:06 AM, Guoqing Jiang wrote:
> Use the two functions to simplify code.
>
> Signed-off-by: Guoqing Jiang<guoqing.jiang@cloud.ionos.com>

Re: [PATCH 3/6] block: remove redundant setting of QUEUE_FLAG_DYING

[Chaitanya Kulkarni]

Looks good to me.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 02/28/2020 07:06 AM, Guoqing Jiang wrote:
> Previously, blk_cleanup_queue has called blk_set_queue_dying to set the
> flag, no need to do it again.
>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>

Re: [PATCH 5/6] block: remove unneeded argument from blk_alloc_flush_queue

[Chaitanya Kulkarni]

Looks good, except I did not count but please verify the
patch subject length, otherwise looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 02/28/2020 07:07 AM, Guoqing Jiang wrote:
> Remove 'q' from arguments since it is not used anymore after
> commit 7e992f847a08e ("block: remove non mq parts from the
> flush code").
>
> Signed-off-by: Guoqing Jiang<guoqing.jiang@cloud.ionos.com>

Re: [PATCH 6/6] block: cleanup comment for blk_flush_complete_seq

[Chaitanya Kulkarni]

Looks good to me.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 02/28/2020 07:06 AM, Guoqing Jiang wrote:
> Remove the comment about return value, since it is not valid after
> commit 404b8f5a03d84 ("block: cleanup kick/queued handling").
>
> Signed-off-by: Guoqing Jiang<guoqing.jiang@cloud.ionos.com>

Re: [PATCH 4/6] block: remove obsolete comments for _blk/blk_rq_prep_clone

[Guoqing Jiang]

Hi Bart,

Thanks for your review.

On 2/29/20 3:34 AM, Bart Van Assche wrote:
> On 2020-02-28 07:05, Guoqing Jiang wrote:
>> Both cmd and sense had been moved to scsi_request, so remove
>> the related comments to avoid confusion.
>>
>> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
>> ---
>>   block/blk-core.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 883ffda216e4..9094fd7d1b01 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -1583,7 +1583,6 @@ EXPORT_SYMBOL_GPL(blk_rq_unprep_clone);
>>   
>>   /*
>>    * Copy attributes of the original request to the clone request.
>> - * The actual data parts (e.g. ->cmd, ->sense) are not copied.
>>    */
>>   static void __blk_rq_prep_clone(struct request *dst, struct request *src)
>>   {
> 
> Although the removed comment is outdated, the new comment is not useful.
> 
> How about inlining __blk_rq_prep_clone() into its only caller and
> removing the comment above that function entirely?It's not clear to me
> why the code inside __blk_rq_prep_clone() ever was put into a separate
> function.

Not sure about it, the original code was introduced in commit b0fd271d5fba0
("block: add request clone interface (v2)"), maybe author preferred to use
a function to copy attributes from src request to dst request.

I will make the change based on your suggestion if no one objects it.

> 
>>    *
>>    * Description:
>>    *     Clones bios in @rq_src to @rq, and copies attributes of @rq_src to @rq.
>> - *     The actual data parts of @rq_src (e.g. ->cmd, ->sense)
>> - *     are not copied, and copying such parts is the caller's responsibility.
>>    *     Also, pages which the original bios are pointing to are not copied
>>    *     and the cloned bios just point same pages.
>>    *     So cloned bios must be completed before original bios, which means
> 
> Adding a comment that explains that some but not all struct request
> members are copied and also why would be welcome.

I think we need care about the actual data parts of request, I guess all the
actual data parts had been moved into scsi_request, but my understanding could
probably be wrong.

Thanks,
Guoqing

Re: [PATCH 5/6] block: remove unneeded argument from blk_alloc_flush_queue

[Guoqing Jiang]

On 3/1/20 12:19 AM, Chaitanya Kulkarni wrote:
> Looks good, except I did not count but please verify the
> patch subject length, otherwise looks good.
> 
> Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> 

Thanks for your review, I assume it is fine since checkpatch didn't complain.

Thanks,
Guoqing