2024-05-25 11:03:30

by Li Nan

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

From: Li Nan <[email protected]>

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

Signed-off-by: Li Nan <[email protected]>
---
drivers/md/md.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index aff9118ff697..509e5638cea1 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -654,15 +654,12 @@ 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. */
+ spin_unlock_irq(&mddev->lock);
if (bio->bi_iter.bi_size == 0)
/* an empty barrier - all done */
bio_endio(bio);
--
2.39.2

2024-05-26 14:01:45

by Christoph Hellwig

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

On Sun, May 26, 2024 at 02:56:22AM +0800, [email protected] wrote:
> - 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. */
> + spin_unlock_irq(&mddev->lock);
> if (bio->bi_iter.bi_size == 0)
> /* an empty barrier - all done */

This stil looks like a somwwhat odd flow Why not go all the way
and turn it into:


...
queue_work(md_wq, &mddev->flush_work);
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;
}

2024-05-28 02:09:22

by Li Nan

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

在 2024/5/26 16:54, Christoph Hellwig 写道:
> On Sun, May 26, 2024 at 02:56:22AM +0800, [email protected] wrote:
>> - 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. */
>> + spin_unlock_irq(&mddev->lock);
>> if (bio->bi_iter.bi_size == 0)
>> /* an empty barrier - all done */
>
> This stil looks like a somwwhat odd flow Why not go all the way
> and turn it into:
>
>
> ...
> queue_work(md_wq, &mddev->flush_work);
> 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;
> }

Yeah, it looks better. I will changed it in v2.

Thansk for your suggestion.

--
Thanks,
Nan