| Message ID | 20210809061244.1196573-20-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/30] iomap: fix a trivial comment typo in trace.h | expand |
On Mon, Aug 09, 2021 at 08:12:33AM +0200, Christoph Hellwig wrote: > Rewrite the ->bmap implementation based on iomap_iter. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/iomap/fiemap.c | 31 +++++++++++++------------------ > 1 file changed, 13 insertions(+), 18 deletions(-) > > diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c > index acad09a8c188df..60daadba16c149 100644 > --- a/fs/iomap/fiemap.c > +++ b/fs/iomap/fiemap.c > @@ -92,35 +92,30 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi, > } > EXPORT_SYMBOL_GPL(iomap_fiemap); > > -static loff_t > -iomap_bmap_actor(struct inode *inode, loff_t pos, loff_t length, > - void *data, struct iomap *iomap, struct iomap *srcmap) > -{ > - sector_t *bno = data, addr; > - > - if (iomap->type == IOMAP_MAPPED) { > - addr = (pos - iomap->offset + iomap->addr) >> inode->i_blkbits; > - *bno = addr; > - } > - return 0; > -} > - > /* legacy ->bmap interface. 0 is the error return (!) */ > sector_t > iomap_bmap(struct address_space *mapping, sector_t bno, > const struct iomap_ops *ops) > { > - struct inode *inode = mapping->host; > - loff_t pos = bno << inode->i_blkbits; > - unsigned blocksize = i_blocksize(inode); > + struct iomap_iter iter = { > + .inode = mapping->host, > + .pos = (loff_t)bno << mapping->host->i_blkbits, > + .len = i_blocksize(mapping->host), > + .flags = IOMAP_REPORT, > + }; > int ret; > > if (filemap_write_and_wait(mapping)) > return 0; > > bno = 0; > - ret = iomap_apply(inode, pos, blocksize, 0, ops, &bno, > - iomap_bmap_actor); > + while ((ret = iomap_iter(&iter, ops)) > 0) { > + if (iter.iomap.type != IOMAP_MAPPED) > + continue; I still feel uncomfortable about this use of "continue" here, because it really means "call iomap_iter again to clean up and exit even though we know it won't even look for more iomaps to iterate". To me that feels subtly broken (I usually associate 'continue' with 'go run the loop body again'), and even though bmap has been a quirky hot mess for 45 years, we don't need to make it even moreso. Can't this at least be rephrased as: const uint bno_shift = (mapping->host->i_blkbits - SECTOR_SHIFT); while ((ret = iomap_iter(&iter, ops)) > 0) { if (iter.iomap.type == IOMAP_MAPPED) bno = iomap_sector(iomap, iter.pos) << bno_shift; /* leave iter.processed unset to stop iteration */ } to make the loop exit more explicit? --D > + bno = (iter.pos - iter.iomap.offset + iter.iomap.addr) >> > + mapping->host->i_blkbits; > + } > + > if (ret) > return 0; > return bno; > -- > 2.30.2 >
On Mon, Aug 09, 2021 at 11:39:51PM -0700, Darrick J. Wong wrote: > Can't this at least be rephrased as: > > const uint bno_shift = (mapping->host->i_blkbits - SECTOR_SHIFT); > > while ((ret = iomap_iter(&iter, ops)) > 0) { > if (iter.iomap.type == IOMAP_MAPPED) > bno = iomap_sector(iomap, iter.pos) << bno_shift; > /* leave iter.processed unset to stop iteration */ > } > > to make the loop exit more explicit? Sure.
diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c index acad09a8c188df..60daadba16c149 100644 --- a/fs/iomap/fiemap.c +++ b/fs/iomap/fiemap.c @@ -92,35 +92,30 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi, } EXPORT_SYMBOL_GPL(iomap_fiemap); -static loff_t -iomap_bmap_actor(struct inode *inode, loff_t pos, loff_t length, - void *data, struct iomap *iomap, struct iomap *srcmap) -{ - sector_t *bno = data, addr; - - if (iomap->type == IOMAP_MAPPED) { - addr = (pos - iomap->offset + iomap->addr) >> inode->i_blkbits; - *bno = addr; - } - return 0; -} - /* legacy ->bmap interface. 0 is the error return (!) */ sector_t iomap_bmap(struct address_space *mapping, sector_t bno, const struct iomap_ops *ops) { - struct inode *inode = mapping->host; - loff_t pos = bno << inode->i_blkbits; - unsigned blocksize = i_blocksize(inode); + struct iomap_iter iter = { + .inode = mapping->host, + .pos = (loff_t)bno << mapping->host->i_blkbits, + .len = i_blocksize(mapping->host), + .flags = IOMAP_REPORT, + }; int ret; if (filemap_write_and_wait(mapping)) return 0; bno = 0; - ret = iomap_apply(inode, pos, blocksize, 0, ops, &bno, - iomap_bmap_actor); + while ((ret = iomap_iter(&iter, ops)) > 0) { + if (iter.iomap.type != IOMAP_MAPPED) + continue; + bno = (iter.pos - iter.iomap.offset + iter.iomap.addr) >> + mapping->host->i_blkbits; + } + if (ret) return 0; return bno;
Rewrite the ->bmap implementation based on iomap_iter. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/iomap/fiemap.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-)