On 5/19/22 1:25 AM, Christoph Hellwig wrote:
> 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.
>
I was only trying to make the point that I haven't enabled this on other
filesystems than XFS. Removing the statement as it causes confusion.
>>
>> Signed-off-by: Stefan Roesch <shr@xxxxxx>
>> ---
>> 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.
>
Removed the no_wait variable and instead used the flag check directly in the code.
>> + 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:
>
I made the below change.
> } 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.
