diff mbox series

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

Message ID 20220518233709.1937634-5-shr@fb.com (mailing list archive)
State New
Headers show
Series io-uring/xfs: support async buffered writes | expand

Commit Message

Stefan Roesch May 18, 2022, 11:36 p.m. UTC
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.

Signed-off-by: Stefan Roesch <shr@fb.com>
---
 fs/iomap/buffered-io.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Christoph Hellwig May 19, 2022, 8:25 a.m. UTC | #1
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.
Stefan Roesch May 20, 2022, 6:29 p.m. UTC | #2
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.
diff mbox series

Patch

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)
+		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);
+	if (no_wait && !iop)
+		return -EAGAIN;
 
 	do {
 		iomap_adjust_read_range(iter->inode, folio, &block_start,
@@ -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);
@@ -632,6 +640,9 @@  static int iomap_write_begin(const struct iomap_iter *iter, loff_t pos,
 	unsigned fgp = FGP_LOCK | FGP_WRITE | FGP_CREAT | FGP_STABLE | FGP_NOFS;
 	int status = 0;
 
+	if (iter->flags & IOMAP_NOWAIT)
+		fgp |= FGP_NOWAIT;
+
 	BUG_ON(pos + len > iter->iomap.offset + iter->iomap.length);
 	if (srcmap != &iter->iomap)
 		BUG_ON(pos + len > srcmap->offset + srcmap->length);
@@ -789,6 +800,10 @@  static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 		 * Otherwise there's a nasty deadlock on copying from the
 		 * same page as we're writing to, without it being marked
 		 * up-to-date.
+		 *
+		 * For async buffered writes the assumption is that the user
+		 * page has already been faulted in. This can be optimized by
+		 * faulting the user page in the prepare phase of io-uring.
 		 */
 		if (unlikely(fault_in_iov_iter_readable(i, bytes) == bytes)) {
 			status = -EFAULT;
@@ -844,6 +859,9 @@  iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *i,
 	};
 	int ret;
 
+	if (iocb->ki_flags & IOCB_NOWAIT)
+		iter.flags |= IOMAP_NOWAIT;
+
 	while ((ret = iomap_iter(&iter, ops)) > 0)
 		iter.processed = iomap_write_iter(&iter, i);
 	if (iter.pos == iocb->ki_pos)