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

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

On Wed, Feb 05, 2025 at 03:27:31PM -0500, Brian Foster wrote:
> 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.

Ah, ok. :)

--D

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