| Message ID | 20240528203149.2383260-1-linan666@huaweicloud.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v2] md: make md_flush_request() more readable | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
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); > >
在 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?
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.
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 > > . >
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 <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
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);