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@xxxxxx>
> ---
> 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
>
