Re: [PATCH 5/7] xfs: fill dirty folios on zero range of unwritten mappings

[Brian Foster <bfoster@redhat.com>]

On Mon, Jun 09, 2025 at 09:12:19AM -0700, Darrick J. Wong wrote:
> On Thu, Jun 05, 2025 at 01:33:55PM -0400, Brian Foster wrote:
> > Use the iomap folio batch mechanism to select folios to zero on zero
> > range of unwritten mappings. Trim the resulting mapping if the batch
> > is filled (unlikely for current use cases) to distinguish between a
> > range to skip and one that requires another iteration due to a full
> > batch.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/xfs_iomap.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> > index b5cf5bc6308d..63054f7ead0e 100644
> > --- a/fs/xfs/xfs_iomap.c
> > +++ b/fs/xfs/xfs_iomap.c
...
> > @@ -1769,6 +1772,26 @@ xfs_buffered_write_iomap_begin(
> >  		if (offset_fsb < eof_fsb && end_fsb > eof_fsb)
> >  			end_fsb = eof_fsb;
> >  
> > +		/*
> > +		 * Look up dirty folios for unwritten mappings within EOF.
> > +		 * Providing this bypasses the flush iomap uses to trigger
> > +		 * extent conversion when unwritten mappings have dirty
> > +		 * pagecache in need of zeroing.
> > +		 *
> > +		 * Trim the mapping to the end pos of the lookup, which in turn
> > +		 * was trimmed to the end of the batch if it became full before
> > +		 * the end of the mapping.
> > +		 */
> > +		if (imap.br_state == XFS_EXT_UNWRITTEN &&
> > +		    offset_fsb < eof_fsb) {
> > +			loff_t len = min(count,
> > +					 XFS_FSB_TO_B(mp, imap.br_blockcount));
> > +
> > +			end = iomap_fill_dirty_folios(iter, offset, len);
> 
> ...though I wonder, does this need to happen in
> xfs_buffered_write_iomap_begin?  Is it required to hold the ILOCK while
> we go look for folios in the mapping?  Or could this become a part of
> iomap_write_begin?
> 

Technically it does not need to be inside ->iomap_begin(). The "dirty
check" just needs to be before the fs drops its own locks associated
with the mapping lookup to maintain functional correctness, and that
includes doing it before the callout in the first place (i.e. this is
how the filemap_range_needs_writeback() logic works). I have various
older prototype versions of that work that tried to do things a bit more
generically in that way, but ultimately they seemed less elegant for the
purpose of zero range.

WRT zero range, the main reason this is in the callback is that it's
only required to search for dirty folios when the underlying mapping is
unwritten, and we don't know that until the filesystem provides the
mapping (and doing at after the fs drops locks is racy).

That said, if we eventually use this for something like buffered writes,
that is not so much of an issue and we probably want to instead
lookup/allocate/lock each successive folio up front. That could likely
occur at the iomap level (lock ordering issues and whatnot
notwithstanding).

The one caveat with zero range is that it's only really used for small
ranges in practice, so it may not really be that big of a deal if the
folio lookup occurred unconditionally. I think the justification for
that is tied to broader using of batching in iomap, however, so I don't
really want to force the issue unless it proves worthwhile. IOW what I'm
trying to say is that if we do end up with a few more ops using this
mechanism, it wouldn't surprise me if we just decided to deduplicate to
the lowest common denominator implementation at that point (and do the
lookups in iomap iter or something). We're just not there yet IMO.

Brian

> --D
> 
> > +			end_fsb = min_t(xfs_fileoff_t, end_fsb,
> > +					XFS_B_TO_FSB(mp, end));
> > +		}
> > +
> >  		xfs_trim_extent(&imap, offset_fsb, end_fsb - offset_fsb);
> >  	}
> >  
> > -- 
> > 2.49.0
> > 
> > 
>