Re: [PATCH 13/16] iomap: make buffered writes work with RWF_UNCACHED

Brian Foster
2024-11-12T16:36:19Z

On Mon, Nov 11, 2024 at 04:37:40PM -0700, Jens Axboe wrote:
> Add iomap buffered write support for RWF_UNCACHED. If RWF_UNCACHED is
> set for a write, mark the folios being written with drop_writeback. Then

s/drop_writeback/uncached/ ?

BTW, this might be getting into wonky "don't care that much" territory,
but something else to be aware of is that certain writes can potentially
change pagecache state as a side effect outside of the actual buffered
write itself.

For example, xfs calls iomap_zero_range() on write extension (i.e. pos >
isize), which uses buffered writes and thus could populate a pagecache
folio without setting it uncached, even if done on behalf of an uncached
write.

I've only made a first pass and could be missing some details, but IIUC
I _think_ this means something like writing out a stream of small,
sparse and file extending uncached writes could actually end up behaving
more like sync I/O. Again, not saying that's something we really care
about, just raising it in case it's worth considering or documenting..

Brian

> writeback completion will drop the pages. The write_iter handler simply
> kicks off writeback for the pages, and writeback completion will take
> care of the rest.
>
> This still needs the user of the iomap buffered write helpers to call
> iocb_uncached_write() upon successful issue of the writes.
>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
> fs/iomap/buffered-io.c | 15 +++++++++++++--
> include/linux/iomap.h | 4 +++-
> 2 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index ef0b68bccbb6..2f2a5db04a68 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -603,6 +603,8 @@ struct folio *iomap_get_folio(struct iomap_iter *iter, loff_t pos, size_t len)
>
> if (iter->flags & IOMAP_NOWAIT)
> fgp |= FGP_NOWAIT;
> + if (iter->flags & IOMAP_UNCACHED)
> + fgp |= FGP_UNCACHED;
> fgp |= fgf_set_order(len);
>
> return __filemap_get_folio(iter->inode->i_mapping, pos >> PAGE_SHIFT,
> @@ -1023,8 +1025,9 @@ ssize_t
> iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *i,
> const struct iomap_ops *ops, void *private)
> {
> + struct address_space *mapping = iocb->ki_filp->f_mapping;
> struct iomap_iter iter = {
> - .inode = iocb->ki_filp->f_mapping->host,
> + .inode = mapping->host,
> .pos = iocb->ki_pos,
> .len = iov_iter_count(i),
> .flags = IOMAP_WRITE,
> @@ -1034,9 +1037,14 @@ iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *i,
>
> if (iocb->ki_flags & IOCB_NOWAIT)
> iter.flags |= IOMAP_NOWAIT;
> + if (iocb->ki_flags & IOCB_UNCACHED)
> + iter.flags |= IOMAP_UNCACHED;
>
> - while ((ret = iomap_iter(&iter, ops)) > 0)
> + while ((ret = iomap_iter(&iter, ops)) > 0) {
> + if (iocb->ki_flags & IOCB_UNCACHED)
> + iter.iomap.flags |= IOMAP_F_UNCACHED;
> iter.processed = iomap_write_iter(&iter, i);
> + }
>
> if (unlikely(iter.pos == iocb->ki_pos))
> return ret;
> @@ -1770,6 +1778,9 @@ static int iomap_add_to_ioend(struct iomap_writepage_ctx *wpc,
> size_t poff = offset_in_folio(folio, pos);
> int error;
>
> + if (folio_test_uncached(folio))
> + wpc->iomap.flags |= IOMAP_F_UNCACHED;
> +
> if (!wpc->ioend || !iomap_can_add_to_ioend(wpc, pos)) {
> new_ioend:
> error = iomap_submit_ioend(wpc, 0);
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index f61407e3b121..2efc72df19a2 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -64,6 +64,7 @@ struct vm_fault;
> #define IOMAP_F_BUFFER_HEAD 0
> #endif /* CONFIG_BUFFER_HEAD */
> #define IOMAP_F_XATTR (1U << 5)
> +#define IOMAP_F_UNCACHED (1U << 6)
>
> /*
> * Flags set by the core iomap code during operations:
> @@ -173,8 +174,9 @@ struct iomap_folio_ops {
> #define IOMAP_NOWAIT (1 << 5) /* do not block */
> #define IOMAP_OVERWRITE_ONLY (1 << 6) /* only pure overwrites allowed */
> #define IOMAP_UNSHARE (1 << 7) /* unshare_file_range */
> +#define IOMAP_UNCACHED (1 << 8) /* uncached IO */
> #ifdef CONFIG_FS_DAX
> -#define IOMAP_DAX (1 << 8) /* DAX mapping */
> +#define IOMAP_DAX (1 << 9) /* DAX mapping */
> #else
> #define IOMAP_DAX 0
> #endif /* CONFIG_FS_DAX */
> --
> 2.45.2
>
>