Re: [RFC PATCH v3 04/18] iomap: Add async buffered write support

[Christoph Hellwig <hch@infradead.org>]

On Wed, May 18, 2022 at 04:36:55PM -0700, Stefan Roesch wrote:
> This adds async buffered write support to iomap. The support is focused
> on the changes necessary to support XFS with iomap.
> 
> Support for other filesystems might require additional changes.

What would those other changes be?  Inline data support should not
matter here, so I guess it is buffer_head support?  Please spell out
the actual limitations instead of the use case.  Preferably including
asserts in the code to catch the case of a file system trying to use
the now supported cases.

> 
> Signed-off-by: Stefan Roesch <shr@fb.com>
> ---
>  fs/iomap/buffered-io.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 6b06fd358958..b029e2b10e07 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -580,12 +580,18 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos,
>  	size_t from = offset_in_folio(folio, pos), to = from + len;
>  	size_t poff, plen;
>  	gfp_t  gfp = GFP_NOFS | __GFP_NOFAIL;
> +	bool no_wait = (iter->flags & IOMAP_NOWAIT);
> +
> +	if (no_wait)

Does thi flag really buy us anything?  My preference woud be to see
the IOMAP_NOWAIT directy as that is easier for me to read than trying to
figure out what no_wait actually means.

> +		gfp = GFP_NOWAIT;
>  
>  	if (folio_test_uptodate(folio))
>  		return 0;
>  	folio_clear_error(folio);
>  
>  	iop = iomap_page_create_gfp(iter->inode, folio, nr_blocks, gfp);

And maybe the btter iomap_page_create inteface would be one that passes
the flags so that we can centralize the gfp_t selection.

> @@ -602,6 +608,8 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos,
>  			if (WARN_ON_ONCE(iter->flags & IOMAP_UNSHARE))
>  				return -EIO;
>  			folio_zero_segments(folio, poff, from, to, poff + plen);
> +		} else if (no_wait) {
> +			return -EAGAIN;
>  		} else {
>  			int status = iomap_read_folio_sync(block_start, folio,
>  					poff, plen, srcmap);

That's a somewhat unnatural code flow.  I'd much prefer:

		} else {
			int status;

			if (iter->flags & IOMAP_NOWAIT)
				return -EAGAIN;
			iomap_read_folio_sync(block_start, folio,
					poff, plen, srcmap);

Or maybe even pass the iter to iomap_read_folio_sync and just do the
IOMAP_NOWAIT check there.