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

[Stefan Roesch <shr@fb.com>]

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

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.