Re: [PATCH v5 09/10] iomap: advance the iter directly on unshare range

[Brian Foster <bfoster@redhat.com>]

On Wed, Feb 05, 2025 at 11:16:10AM -0800, Darrick J. Wong wrote:
> On Wed, Feb 05, 2025 at 08:58:20AM -0500, Brian Foster wrote:
> > Modify unshare range to advance the iter directly. Replace the local
> > pos and length calculations with direct advances and loop based on
> > iter state instead.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  fs/iomap/buffered-io.c | 23 +++++++++++------------
> >  1 file changed, 11 insertions(+), 12 deletions(-)
> > 
> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > index 678c189faa58..f953bf66beb1 100644
> > --- a/fs/iomap/buffered-io.c
> > +++ b/fs/iomap/buffered-io.c
> > @@ -1267,20 +1267,19 @@ EXPORT_SYMBOL_GPL(iomap_write_delalloc_release);
> >  static loff_t iomap_unshare_iter(struct iomap_iter *iter)
> >  {
> >  	struct iomap *iomap = &iter->iomap;
> > -	loff_t pos = iter->pos;
> > -	loff_t length = iomap_length(iter);
> > -	loff_t written = 0;
> > +	u64 bytes = iomap_length(iter);
> > +	int status;
> >  
> >  	if (!iomap_want_unshare_iter(iter))
> > -		return length;
> > +		return iomap_iter_advance(iter, &bytes);
> >  
> >  	do {
> >  		struct folio *folio;
> > -		int status;
> >  		size_t offset;
> > -		size_t bytes = min_t(u64, SIZE_MAX, length);
> > +		loff_t pos = iter->pos;
> 
> Do we still need the local variable here?
> 

Technically no.. Christoph brought up something similar in earlier
versions re: the pos/len variables (here and in subsequent patches) but
I'm leaving it like this for now because the folio batch work (which is
the impetus for this series) further refactors and removes much of this.

For example, pos gets pushed down into the write begin path so it can
manage state between the next folio in a provided batch and the current
position of the iter itself. So this pos code goes away from
unshare_iter() completely and this patch is just moving things one step
in that direction.

> Otherwise looks right to me, so
> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
> 

Thanks.

Brian

> --D
> 
> >  		bool ret;
> >  
> > +		bytes = min_t(u64, SIZE_MAX, bytes);
> >  		status = iomap_write_begin(iter, pos, bytes, &folio);
> >  		if (unlikely(status))
> >  			return status;
> > @@ -1298,14 +1297,14 @@ static loff_t iomap_unshare_iter(struct iomap_iter *iter)
> >  
> >  		cond_resched();
> >  
> > -		pos += bytes;
> > -		written += bytes;
> > -		length -= bytes;
> > -
> >  		balance_dirty_pages_ratelimited(iter->inode->i_mapping);
> > -	} while (length > 0);
> >  
> > -	return written;
> > +		status = iomap_iter_advance(iter, &bytes);
> > +		if (status)
> > +			break;
> > +	} while (bytes > 0);
> > +
> > +	return status;
> >  }
> >  
> >  int
> > -- 
> > 2.48.1
> > 
> > 
>