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.
Suggested-by: Christoph Hellwig <[email protected]>
Signed-off-by: Li Nan <[email protected]>
---
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
Hi,
?? 2024/05/29 4:31, [email protected] д??:
> 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.
>
> Suggested-by: Christoph Hellwig <[email protected]>
> Signed-off-by: Li Nan <[email protected]>
> ---
> 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);
>
>
在 2024/5/28 21:23, Christoph Hellwig 写道:
> Looks good:
>
> Reviewed-by: Christoph Hellwig <[email protected]>
>
> .
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
Looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
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 <[email protected]>
>>>
>>> .
>> 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
>
> .
>
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 <[email protected]>
> >
> > .
> 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.
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)
On Tue, May 28, 2024 at 5:39 AM <[email protected]> wrote:
>
> 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.
>
> Suggested-by: Christoph Hellwig <[email protected]>
> Signed-off-by: Li Nan <[email protected]>
Applied v2 to md-6.11. Thanks!
Song