On Mon, Jun 16, 2025 at 6:00 AM Christoph Hellwig <hch@xxxxxx> wrote:
>
> Replace ->map_blocks with a new ->writeback_range, which differs in the
> following ways:
>
> - it must also queue up the I/O for writeback, that is called into the
> slightly refactored and extended in scope iomap_add_to_ioend for
> each region
> - can handle only a part of the requested region, that is the retry
> loop for partial mappings moves to the caller
> - handles cleanup on failures as well, and thus also replaces the
> discard_folio method only implemented by XFS.
>
> This will allow to use the iomap writeback code also for file systems
> that are not block based like fuse.
>
> Co-developed-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> .../filesystems/iomap/operations.rst | 23 +--
> block/fops.c | 16 +-
> fs/gfs2/bmap.c | 14 +-
> fs/iomap/buffered-io.c | 93 +++++------
> fs/iomap/trace.h | 2 +-
> fs/xfs/xfs_aops.c | 154 ++++++++++--------
> fs/zonefs/file.c | 20 ++-
> include/linux/iomap.h | 20 +--
> 8 files changed, 170 insertions(+), 172 deletions(-)
> diff --git a/block/fops.c b/block/fops.c
> index 3394263d942b..2f41bd0950d0 100644
> --- a/block/fops.c
> +++ b/block/fops.c
> @@ -537,22 +537,26 @@ static void blkdev_readahead(struct readahead_control *rac)
> iomap_readahead(rac, &blkdev_iomap_ops);
> }
>
> -static int blkdev_map_blocks(struct iomap_writepage_ctx *wpc,
> - struct inode *inode, loff_t offset, unsigned int len)
> +static ssize_t blkdev_writeback_range(struct iomap_writepage_ctx *wpc,
> + struct folio *folio, u64 offset, unsigned int len, u64 end_pos)
> {
> - loff_t isize = i_size_read(inode);
> + loff_t isize = i_size_read(wpc->inode);
> + int error;
>
> if (WARN_ON_ONCE(offset >= isize))
> return -EIO;
> if (offset >= wpc->iomap.offset &&
> offset < wpc->iomap.offset + wpc->iomap.length)
> return 0;
I'm not acquainted with the block io / bio layer so please do ignore
this if my analysis here is wrong, but AFAICT we do still need to add
this range to the ioend in the case where the mapping is already
valid? Should this be "return iomap_add_to_ioend(wpc, folio, offset,
end_pos, len)" instead of return 0? It seems like otherwise too, with
the new logic in iomap_writeback_range() function that was added,
ret = wpc->ops->writeback_range(wpc, folio, pos, rlen, end_pos);
if (WARN_ON_ONCE(ret == 0))
return -EIO;
returning 0 here would always result in -EIO.
> - return blkdev_iomap_begin(inode, offset, isize - offset,
> - IOMAP_WRITE, &wpc->iomap, NULL);
> + error = blkdev_iomap_begin(wpc->inode, offset, isize - offset,
> + IOMAP_WRITE, &wpc->iomap, NULL);
> + if (error)
> + return error;
> + return iomap_add_to_ioend(wpc, folio, offset, end_pos, len);
> }
>
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 11a55da26a6f..5e832fa2a813 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
>
> -static int iomap_writepage_map_blocks(struct iomap_writepage_ctx *wpc,
> - struct folio *folio, u64 pos, u64 end_pos, unsigned dirty_len,
> +static int iomap_writeback_range(struct iomap_writepage_ctx *wpc,
> + struct folio *folio, u64 pos, u32 rlen, u64 end_pos,
> bool *wb_pending)
> {
> - int error;
> -
> do {
> - unsigned map_len;
> -
> - error = wpc->ops->map_blocks(wpc, wpc->inode, pos, dirty_len);
> - if (error)
> - break;
> - trace_iomap_writepage_map(wpc->inode, pos, dirty_len,
> - &wpc->iomap);
> -
> - map_len = min_t(u64, dirty_len,
> - wpc->iomap.offset + wpc->iomap.length - pos);
> - WARN_ON_ONCE(!folio->private && map_len < dirty_len);
> + ssize_t ret;
>
> - switch (wpc->iomap.type) {
> - case IOMAP_INLINE:
> - WARN_ON_ONCE(1);
> - error = -EIO;
> - break;
> - case IOMAP_HOLE:
> - break;
> - default:
> - error = iomap_add_to_ioend(wpc, folio, pos, end_pos,
> - map_len);
> - if (!error)
> - *wb_pending = true;
> - break;
> - }
> - dirty_len -= map_len;
> - pos += map_len;
> - } while (dirty_len && !error);
> + ret = wpc->ops->writeback_range(wpc, folio, pos, rlen, end_pos);
> + if (WARN_ON_ONCE(ret == 0))
> + return -EIO;
> + if (ret < 0)
> + return ret;
Should we also add a warn check here for if ret > rlen?
> + rlen -= ret;
> + pos += ret;
> + if (wpc->iomap.type != IOMAP_HOLE)
> + *wb_pending = true;
> + } while (rlen);
>
