| Message ID | 20231126124720.1249310-9-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/13] iomap: clear the per-folio dirty bits on all writeback failures | expand |
Christoph Hellwig <hch@lst.de> writes: > The calculation in iomap_sector is pretty trivial and most of the time > iomap_add_to_ioend only callers either iomap_can_add_to_ioend or > iomap_alloc_ioend from a single invocation. > > Calculate the sector in the two lower level functions and stop passing it > from iomap_add_to_ioend and update the iomap_alloc_ioend argument passing > order to match that of iomap_add_to_ioend. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/iomap/buffered-io.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) Straight forward change. Looks good to me, please feel free to add - (small nit below on naming style convention) Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index 7f86d2f90e3863..329e2c342f1c64 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -1666,9 +1666,8 @@ iomap_submit_ioend(struct iomap_writepage_ctx *wpc, struct iomap_ioend *ioend, > return 0; > } > > -static struct iomap_ioend * > -iomap_alloc_ioend(struct inode *inode, struct iomap_writepage_ctx *wpc, > - loff_t offset, sector_t sector, struct writeback_control *wbc) > +static struct iomap_ioend *iomap_alloc_ioend(struct iomap_writepage_ctx *wpc, > + struct writeback_control *wbc, struct inode *inode, loff_t pos) > { > struct iomap_ioend *ioend; > struct bio *bio; > @@ -1676,7 +1675,7 @@ iomap_alloc_ioend(struct inode *inode, struct iomap_writepage_ctx *wpc, > bio = bio_alloc_bioset(wpc->iomap.bdev, BIO_MAX_VECS, > REQ_OP_WRITE | wbc_to_write_flags(wbc), > GFP_NOFS, &iomap_ioend_bioset); > - bio->bi_iter.bi_sector = sector; > + bio->bi_iter.bi_sector = iomap_sector(&wpc->iomap, pos); > wbc_init_bio(wbc, bio); > > ioend = container_of(bio, struct iomap_ioend, io_inline_bio); > @@ -1685,9 +1684,9 @@ iomap_alloc_ioend(struct inode *inode, struct iomap_writepage_ctx *wpc, > ioend->io_flags = wpc->iomap.flags; > ioend->io_inode = inode; > ioend->io_size = 0; > - ioend->io_offset = offset; > + ioend->io_offset = pos; > ioend->io_bio = bio; > - ioend->io_sector = sector; > + ioend->io_sector = bio->bi_iter.bi_sector; > > wpc->nr_folios = 0; > return ioend; > @@ -1716,8 +1715,7 @@ iomap_chain_bio(struct bio *prev) > } > > static bool > -iomap_can_add_to_ioend(struct iomap_writepage_ctx *wpc, loff_t offset, > - sector_t sector) > +iomap_can_add_to_ioend(struct iomap_writepage_ctx *wpc, loff_t offset) Not sure which style you would like to keep in fs/iomap/. Should the function name be in the same line as "static bool" or in the next line? For previous function you made the function name definition in the same line. Or is the naming style irrelevant for fs/iomap/? -ritesh
On Mon, Nov 27, 2023 at 03:24:49PM +0530, Ritesh Harjani wrote: > > static bool > > -iomap_can_add_to_ioend(struct iomap_writepage_ctx *wpc, loff_t offset, > > - sector_t sector) > > +iomap_can_add_to_ioend(struct iomap_writepage_ctx *wpc, loff_t offset) > > Not sure which style you would like to keep in fs/iomap/. > Should the function name be in the same line as "static bool" or in the next line? > For previous function you made the function name definition in the same > line. Or is the naming style irrelevant for fs/iomap/? The XFS style that iomap start out with has the separate line, and I actually kinda like it. But I think willy convinced us a while ago to move the common line which is the normal kernel style, and most new code seems to use this. And yes, I should probably be consistent and I should change it here as well.
On Mon, Nov 27, 2023 at 02:54:02PM +0100, Christoph Hellwig wrote: > On Mon, Nov 27, 2023 at 03:24:49PM +0530, Ritesh Harjani wrote: > > > static bool > > > -iomap_can_add_to_ioend(struct iomap_writepage_ctx *wpc, loff_t offset, > > > - sector_t sector) > > > +iomap_can_add_to_ioend(struct iomap_writepage_ctx *wpc, loff_t offset) Can you change @offset to @pos while you're changing the function signature? > > Not sure which style you would like to keep in fs/iomap/. > > Should the function name be in the same line as "static bool" or in the next line? > > For previous function you made the function name definition in the same > > line. Or is the naming style irrelevant for fs/iomap/? > > The XFS style that iomap start out with has the separate line, and I > actually kinda like it. But I think willy convinced us a while ago to > move the common line which is the normal kernel style, and most new code > seems to use this. And yes, I should probably be consistent and I > should change it here as well. I prefer xfs style, but I've been told privately to knock it off outside xfs. So. Fugly kernel style with too much manual whitespace maintenance it is. :/ --D
On Tue, Nov 28, 2023 at 08:53:57PM -0800, Darrick J. Wong wrote: > Can you change @offset to @pos while you're changing the function > signature? Sure.
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 7f86d2f90e3863..329e2c342f1c64 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -1666,9 +1666,8 @@ iomap_submit_ioend(struct iomap_writepage_ctx *wpc, struct iomap_ioend *ioend, return 0; } -static struct iomap_ioend * -iomap_alloc_ioend(struct inode *inode, struct iomap_writepage_ctx *wpc, - loff_t offset, sector_t sector, struct writeback_control *wbc) +static struct iomap_ioend *iomap_alloc_ioend(struct iomap_writepage_ctx *wpc, + struct writeback_control *wbc, struct inode *inode, loff_t pos) { struct iomap_ioend *ioend; struct bio *bio; @@ -1676,7 +1675,7 @@ iomap_alloc_ioend(struct inode *inode, struct iomap_writepage_ctx *wpc, bio = bio_alloc_bioset(wpc->iomap.bdev, BIO_MAX_VECS, REQ_OP_WRITE | wbc_to_write_flags(wbc), GFP_NOFS, &iomap_ioend_bioset); - bio->bi_iter.bi_sector = sector; + bio->bi_iter.bi_sector = iomap_sector(&wpc->iomap, pos); wbc_init_bio(wbc, bio); ioend = container_of(bio, struct iomap_ioend, io_inline_bio); @@ -1685,9 +1684,9 @@ iomap_alloc_ioend(struct inode *inode, struct iomap_writepage_ctx *wpc, ioend->io_flags = wpc->iomap.flags; ioend->io_inode = inode; ioend->io_size = 0; - ioend->io_offset = offset; + ioend->io_offset = pos; ioend->io_bio = bio; - ioend->io_sector = sector; + ioend->io_sector = bio->bi_iter.bi_sector; wpc->nr_folios = 0; return ioend; @@ -1716,8 +1715,7 @@ iomap_chain_bio(struct bio *prev) } static bool -iomap_can_add_to_ioend(struct iomap_writepage_ctx *wpc, loff_t offset, - sector_t sector) +iomap_can_add_to_ioend(struct iomap_writepage_ctx *wpc, loff_t offset) { if ((wpc->iomap.flags & IOMAP_F_SHARED) != (wpc->ioend->io_flags & IOMAP_F_SHARED)) @@ -1726,7 +1724,8 @@ iomap_can_add_to_ioend(struct iomap_writepage_ctx *wpc, loff_t offset, return false; if (offset != wpc->ioend->io_offset + wpc->ioend->io_size) return false; - if (sector != bio_end_sector(wpc->ioend->io_bio)) + if (iomap_sector(&wpc->iomap, offset) != + bio_end_sector(wpc->ioend->io_bio)) return false; /* * Limit ioend bio chain lengths to minimise IO completion latency. This @@ -1747,14 +1746,13 @@ static void iomap_add_to_ioend(struct iomap_writepage_ctx *wpc, struct inode *inode, loff_t pos, struct list_head *iolist) { struct iomap_folio_state *ifs = folio->private; - sector_t sector = iomap_sector(&wpc->iomap, pos); unsigned len = i_blocksize(inode); size_t poff = offset_in_folio(folio, pos); - if (!wpc->ioend || !iomap_can_add_to_ioend(wpc, pos, sector)) { + if (!wpc->ioend || !iomap_can_add_to_ioend(wpc, pos)) { if (wpc->ioend) list_add(&wpc->ioend->io_list, iolist); - wpc->ioend = iomap_alloc_ioend(inode, wpc, pos, sector, wbc); + wpc->ioend = iomap_alloc_ioend(wpc, wbc, inode, pos); } if (!bio_add_folio(wpc->ioend->io_bio, folio, len, poff)) {
The calculation in iomap_sector is pretty trivial and most of the time iomap_add_to_ioend only callers either iomap_can_add_to_ioend or iomap_alloc_ioend from a single invocation. Calculate the sector in the two lower level functions and stop passing it from iomap_add_to_ioend and update the iomap_alloc_ioend argument passing order to match that of iomap_add_to_ioend. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/iomap/buffered-io.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)