Re: [PATCH v2] md: make md_flush_request() more readable

Re: [PATCH v2] md: make md_flush_request() more readable

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2] md: make md_flush_request() more readable

[Yu Kuai]

Hi,

在 2024/05/29 4:31, linan666@huaweicloud.com 写道:
> From: Li Nan <linan122@huawei.com>
> 
> Setting bio to NULL and checking 'if(!bio)' is redundant and looks strange,
> just consolidate them into one condition. There are no functional changes.
> 
> Suggested-by: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Li Nan <linan122@huawei.com>
> ---
> v2: Rewrite the code according to Christoph's suggestion.
> 
>   drivers/md/md.c | 28 +++++++++++++---------------
>   1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index aff9118ff697..9598b4898ea9 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -654,24 +654,22 @@ bool md_flush_request(struct mddev *mddev, struct bio *bio)
>   		WARN_ON(percpu_ref_is_zero(&mddev->active_io));
>   		percpu_ref_get(&mddev->active_io);
>   		mddev->flush_bio = bio;
> -		bio = NULL;
> -	}
> -	spin_unlock_irq(&mddev->lock);
> -
> -	if (!bio) {
> +		spin_unlock_irq(&mddev->lock);
>   		INIT_WORK(&mddev->flush_work, submit_flushes);
>   		queue_work(md_wq, &mddev->flush_work);
> -	} else {
> -		/* flush was performed for some other bio while we waited. */
> -		if (bio->bi_iter.bi_size == 0)
> -			/* an empty barrier - all done */
> -			bio_endio(bio);
> -		else {
> -			bio->bi_opf &= ~REQ_PREFLUSH;
> -			return false;
> -		}
> +		return true;
>   	}
> -	return true;
> +
> +	/* flush was performed for some other bio while we waited. */
> +	spin_unlock_irq(&mddev->lock);
> +	if (bio->bi_iter.bi_size == 0) {

It's better to use bio_sectors() here.

Thanks,
Kuai

> +		/* pure flush without data - all done */
> +		bio_endio(bio);
> +		return true;
> +	}
> +
> +	bio->bi_opf &= ~REQ_PREFLUSH;
> +	return false;
>   }
>   EXPORT_SYMBOL(md_flush_request);
>   
> 

Re: [PATCH v2] md: make md_flush_request() more readable

[Li Nan]

在 2024/5/28 21:23, Christoph Hellwig 写道:
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> .
As suggested by Kuai, I will use bio_sectors instead of bi_size in v3.

-       if (bio->bi_iter.bi_size == 0) {
+       if (!bio_sectors(bio)) {

There is no any other changes, could I add your review by in v3?

-- 
Thanks,
Nan

[PATCH v2] md: make md_flush_request() more readable

[linan666]

From: Li Nan <linan122@huawei.com>

Setting bio to NULL and checking 'if(!bio)' is redundant and looks strange,
just consolidate them into one condition. There are no functional changes.

Suggested-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Li Nan <linan122@huawei.com>
---
v2: Rewrite the code according to Christoph's suggestion.

 drivers/md/md.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index aff9118ff697..9598b4898ea9 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -654,24 +654,22 @@ bool md_flush_request(struct mddev *mddev, struct bio *bio)
 		WARN_ON(percpu_ref_is_zero(&mddev->active_io));
 		percpu_ref_get(&mddev->active_io);
 		mddev->flush_bio = bio;
-		bio = NULL;
-	}
-	spin_unlock_irq(&mddev->lock);
-
-	if (!bio) {
+		spin_unlock_irq(&mddev->lock);
 		INIT_WORK(&mddev->flush_work, submit_flushes);
 		queue_work(md_wq, &mddev->flush_work);
-	} else {
-		/* flush was performed for some other bio while we waited. */
-		if (bio->bi_iter.bi_size == 0)
-			/* an empty barrier - all done */
-			bio_endio(bio);
-		else {
-			bio->bi_opf &= ~REQ_PREFLUSH;
-			return false;
-		}
+		return true;
 	}
-	return true;
+
+	/* flush was performed for some other bio while we waited. */
+	spin_unlock_irq(&mddev->lock);
+	if (bio->bi_iter.bi_size == 0) {
+		/* pure flush without data - all done */
+		bio_endio(bio);
+		return true;
+	}
+
+	bio->bi_opf &= ~REQ_PREFLUSH;
+	return false;
 }
 EXPORT_SYMBOL(md_flush_request);
 
-- 
2.39.2

Re: [PATCH v2] md: make md_flush_request() more readable

[Christoph Hellwig]

On Tue, May 28, 2024 at 09:49:44PM +0800, Li Nan wrote:
> 
> 
> 在 2024/5/28 21:23, Christoph Hellwig 写道:
> > Looks good:
> > 
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > 
> > .
> As suggested by Kuai, I will use bio_sectors instead of bi_size in v3.
> 
> -       if (bio->bi_iter.bi_size == 0) {
> +       if (!bio_sectors(bio)) {

That looks weird.   bio_sectors literally just shifts
bio->bi_iter.bi_size to be in units of sectors, which doesn't
matter for comparing with 0.

Re: [PATCH v2] md: make md_flush_request() more readable

[Yu Kuai]

Hi,

在 2024/05/29 13:44, Christoph Hellwig 写道:
> On Tue, May 28, 2024 at 09:49:44PM +0800, Li Nan wrote:
>>
>>
>> 在 2024/5/28 21:23, Christoph Hellwig 写道:
>>> Looks good:
>>>
>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>>
>>> .
>> As suggested by Kuai, I will use bio_sectors instead of bi_size in v3.
>>
>> -       if (bio->bi_iter.bi_size == 0) {
>> +       if (!bio_sectors(bio)) {
> 
> That looks weird.   bio_sectors literally just shifts
> bio->bi_iter.bi_size to be in units of sectors, which doesn't
> matter for comparing with 0.

The block layer use the same code several times to check if flush bio
contain data, for example:

submit_bio_noacct
  if (op_is_flush(bio->bi_opf))
   if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags))
    if (!bio_sectors(bio))
     bio_endio(bio);

Or will the bi_size to be less than one sector?

Thanks,
Kuai
> 
> .
> 

Re: [PATCH v2] md: make md_flush_request() more readable

[Christoph Hellwig]

On Wed, May 29, 2024 at 02:08:20PM +0800, Yu Kuai wrote:
> 
> submit_bio_noacct
>  if (op_is_flush(bio->bi_opf))
>   if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags))
>    if (!bio_sectors(bio))
>     bio_endio(bio);
> 
> Or will the bi_size to be less than one sector?

bi_size is always aligned to the sector size except for passthrough
command.  So the two versions are 100% equivalent.  bio_sectors just
does a useless shift (which the compiler hopefully optimizes away)

Re: [PATCH v2] md: make md_flush_request() more readable

[Song Liu]

On Tue, May 28, 2024 at 5:39 AM <linan666@huaweicloud.com> wrote:
>
> From: Li Nan <linan122@huawei.com>
>
> Setting bio to NULL and checking 'if(!bio)' is redundant and looks strange,
> just consolidate them into one condition. There are no functional changes.
>
> Suggested-by: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Li Nan <linan122@huawei.com>

Applied v2 to md-6.11. Thanks!

Song