Re: [PATCH v5 08/10] iomap: advance the iter directly on buffered writes

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Feb 05, 2025 at 08:58:19AM -0500, Brian Foster wrote:
> Modify the buffered write path to advance the iter directly. Replace
> the local pos and length calculations with direct advances and loop
> based on iter state instead.
> 
> Also remove the -EAGAIN return hack as it is no longer necessary now
> that separate return channels exist for processing progress and error
> returns. For example, the existing write handler must return either a
> count of bytes written or error if the write is interrupted, but
> presumably wants to return -EAGAIN directly in order to break the higher
> level iomap_iter() loop.
> 
> Since the current iteration may have made some progress, it unwinds the
> iter on the way out to return the error while ensuring that portion of
> the write can be retried. If -EAGAIN occurs at any point beyond the
> first iteration, iomap_file_buffered_write() will then observe progress
> based on iter->pos to return a short write.
> 
> With incremental advances on the iomap_iter, iomap_write_iter() can
> simply return the error. iomap_iter() completes whatever progress was
> made based on iomap_iter position and still breaks out of the iter loop
> based on the error code in iter.processed. The end result of the write
> is similar in terms of being a short write if progress was made or error
> return otherwise.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

LGTM
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/iomap/buffered-io.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index d303e6c8900c..678c189faa58 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -909,8 +909,6 @@ static bool iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len,
>  
>  static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
>  {
> -	loff_t length = iomap_length(iter);
> -	loff_t pos = iter->pos;
>  	ssize_t total_written = 0;
>  	long status = 0;
>  	struct address_space *mapping = iter->inode->i_mapping;
> @@ -923,7 +921,8 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
>  		size_t offset;		/* Offset into folio */
>  		size_t bytes;		/* Bytes to write to folio */
>  		size_t copied;		/* Bytes copied from user */
> -		size_t written;		/* Bytes have been written */
> +		u64 written;		/* Bytes have been written */
> +		loff_t pos = iter->pos;
>  
>  		bytes = iov_iter_count(i);
>  retry:
> @@ -934,8 +933,8 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
>  		if (unlikely(status))
>  			break;
>  
> -		if (bytes > length)
> -			bytes = length;
> +		if (bytes > iomap_length(iter))
> +			bytes = iomap_length(iter);
>  
>  		/*
>  		 * Bring in the user page that we'll copy from _first_.
> @@ -1006,17 +1005,12 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
>  				goto retry;
>  			}
>  		} else {
> -			pos += written;
>  			total_written += written;
> -			length -= written;
> +			iomap_iter_advance(iter, &written);
>  		}
> -	} while (iov_iter_count(i) && length);
> +	} while (iov_iter_count(i) && iomap_length(iter));
>  
> -	if (status == -EAGAIN) {
> -		iov_iter_revert(i, total_written);
> -		return -EAGAIN;
> -	}
> -	return total_written ? total_written : status;
> +	return total_written ? 0 : status;
>  }
>  
>  ssize_t
> -- 
> 2.48.1
> 
>