On Tue, Jun 10, 2025 at 08:21:06AM -0400, Brian Foster wrote:
> On Mon, Jun 09, 2025 at 09:04:20AM -0700, Darrick J. Wong wrote:
> > On Thu, Jun 05, 2025 at 01:33:53PM -0400, Brian Foster wrote:
> > > The only way zero range can currently process unwritten mappings
> > > with dirty pagecache is to check whether the range is dirty before
> > > mapping lookup and then flush when at least one underlying mapping
> > > is unwritten. This ordering is required to prevent iomap lookup from
> > > racing with folio writeback and reclaim.
> > >
> > > Since zero range can skip ranges of unwritten mappings that are
> > > clean in cache, this operation can be improved by allowing the
> > > filesystem to provide a set of dirty folios that require zeroing. In
> > > turn, rather than flush or iterate file offsets, zero range can
> > > iterate on folios in the batch and advance over clean or uncached
> > > ranges in between.
> > >
> > > Add a folio_batch in struct iomap and provide a helper for fs' to
> > > populate the batch at lookup time. Update the folio lookup path to
> > > return the next folio in the batch, if provided, and advance the
> > > iter if the folio starts beyond the current offset.
> > >
> > > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> > > ---
> > > fs/iomap/buffered-io.c | 73 +++++++++++++++++++++++++++++++++++++++---
> > > fs/iomap/iter.c | 6 ++++
> > > include/linux/iomap.h | 4 +++
> > > 3 files changed, 78 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > > index 16499655e7b0..cf2f4f869920 100644
> > > --- a/fs/iomap/buffered-io.c
> > > +++ b/fs/iomap/buffered-io.c
> > > @@ -750,6 +750,16 @@ static struct folio *__iomap_get_folio(struct iomap_iter *iter, size_t len)
> > > if (!mapping_large_folio_support(iter->inode->i_mapping))
> > > len = min_t(size_t, len, PAGE_SIZE - offset_in_page(pos));
> > >
> > > + if (iter->fbatch) {
> > > + struct folio *folio = folio_batch_next(iter->fbatch);
> > > +
> > > + if (folio) {
> > > + folio_get(folio);
> > > + folio_lock(folio);
> >
> > Hrm. So each folio that is added to the batch isn't locked, nor does
> > the batch (or iomap) hold a refcount on the folio until we get here. Do
> > we have to re-check that folio->{mapping,index} match what iomap is
> > trying to process? Or can we assume that nobody has removed the folio
> > from the mapping?
> >
>
> The filemap helper grabs a reference to the folio but doesn't
> necessarily lock it. The ref is effectively transferred to the batch
> there and the _get() here creates the iomap reference (i.e. that is
> analogous to the traditional iomap get folio path). The batch is
> ultimately released via folio_batch_release() and the iomap refs dropped
> in the same way regardless of whether iomap grabbed it itself or was
> part of a patch.
Oh, ok, so that's really iomap getting its own ref on the folio to
remain independent of whatever the fbatch code does (or might some day
do).
> > I'm wondering because __filemap_get_folio/filemap_get_entry seem to do
> > all that for us. I think the folio_pos check below might cover some of
> > that revalidation?
> >
>
> I'm not totally sure the folio revalidation is necessarily required
> here.. If it is, I'd also need to think about whether it's ok to skip
> such folios or the approach here needs revisiting. I'll take a closer
> look and also try to document this better and get some feedback from
> people who know this code better in the next go around..
Hrmm. On closer examination, at least for xfs we've taken i_rwsem and
the invalidate_lock so I think it should be the case that you don't need
to revalidate. I think the same locks are held for iomap_unshare_range
(mentioned elsewhere in this thread) though it doesn't apply to regular
pagecache writes.
> > > + }
> > > + return folio;
> > > + }
> > > +
> > > if (folio_ops && folio_ops->get_folio)
> > > return folio_ops->get_folio(iter, pos, len);
> > > else
> ...
> > > @@ -819,6 +831,12 @@ static int iomap_write_begin(struct iomap_iter *iter, struct folio **foliop,
> > > if (IS_ERR(folio))
> > > return PTR_ERR(folio);
> > >
> > > + /* no folio means we're done with a batch */
> >
> > ...ran out of folios but *plen is nonzero, i.e. we still have range to
> > cover?
> >
>
> Yes I suppose that is implied by being in this path.. will fix.
>
> > > + if (!folio) {
> > > + WARN_ON_ONCE(!iter->fbatch);
> > > + return 0;
> > > + }
> > > +
> > > /*
> > > * Now we have a locked folio, before we do anything with it we need to
> > > * check that the iomap we have cached is not stale. The inode extent
> ...
> > > +
> > > int
> > > iomap_zero_range(struct inode *inode, loff_t pos, loff_t len, bool *did_zero,
> > > const struct iomap_ops *ops, void *private)
> ...
> > > @@ -1445,13 +1503,18 @@ iomap_zero_range(struct inode *inode, loff_t pos, loff_t len, bool *did_zero,
> > > * if dirty and the fs returns a mapping that might convert on
> > > * writeback.
> > > */
> > > - range_dirty = filemap_range_needs_writeback(inode->i_mapping,
> > > - iter.pos, iter.pos + iter.len - 1);
> > > + range_dirty = filemap_range_needs_writeback(mapping, iter.pos,
> > > + iter.pos + iter.len - 1);
> > > while ((ret = iomap_iter(&iter, ops)) > 0) {
> > > const struct iomap *srcmap = iomap_iter_srcmap(&iter);
> > >
> > > - if (srcmap->type == IOMAP_HOLE ||
> > > - srcmap->type == IOMAP_UNWRITTEN) {
> > > + if (WARN_ON_ONCE(iter.fbatch &&
> > > + srcmap->type != IOMAP_UNWRITTEN))
> >
> > I wonder, are you planning to expand the folio batching to other
> > buffered-io.c operations? Such that the iter.fbatch checks might some
> > day go away?
> >
>
> Yes.. but I'm not totally sure wrt impact on the fbatch checks quite
> yet. The next thing I wanted to look at is addressing the same unwritten
> mapping vs. dirty folios issue in the seek data/hole path. It's been a
> little while since I last investigated there (and that was also before
> the whole granular advance approach was devised), but IIRC it would look
> rather similar to what this is doing for zero range. That may or may
> not justify just making the batch required for both operations and
> potentially simplifying this logic further. I'll keep that in mind when
> I get to it..
>
> After that, I may play around with the buffered write path, but that is
> a larger change with slightly different scope and requirements..
<nod>
--D
> Brian
>
> > --D
> >
> > > + return -EIO;
> > > +
> > > + if (!iter.fbatch &&
> > > + (srcmap->type == IOMAP_HOLE ||
> > > + srcmap->type == IOMAP_UNWRITTEN)) {
> > > s64 status;
> > >
> > > if (range_dirty) {
> > > diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
> > > index 6ffc6a7b9ba5..89bd5951a6fd 100644
> > > --- a/fs/iomap/iter.c
> > > +++ b/fs/iomap/iter.c
> > > @@ -9,6 +9,12 @@
> > >
> > > static inline void iomap_iter_reset_iomap(struct iomap_iter *iter)
> > > {
> > > + if (iter->fbatch) {
> > > + folio_batch_release(iter->fbatch);
> > > + kfree(iter->fbatch);
> > > + iter->fbatch = NULL;
> > > + }
> > > +
> > > iter->status = 0;
> > > memset(&iter->iomap, 0, sizeof(iter->iomap));
> > > memset(&iter->srcmap, 0, sizeof(iter->srcmap));
> > > diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> > > index 522644d62f30..0b9b460b2873 100644
> > > --- a/include/linux/iomap.h
> > > +++ b/include/linux/iomap.h
> > > @@ -9,6 +9,7 @@
> > > #include <linux/types.h>
> > > #include <linux/mm_types.h>
> > > #include <linux/blkdev.h>
> > > +#include <linux/pagevec.h>
> > >
> > > struct address_space;
> > > struct fiemap_extent_info;
> > > @@ -239,6 +240,7 @@ struct iomap_iter {
> > > unsigned flags;
> > > struct iomap iomap;
> > > struct iomap srcmap;
> > > + struct folio_batch *fbatch;
> > > void *private;
> > > };
> > >
> > > @@ -345,6 +347,8 @@ void iomap_invalidate_folio(struct folio *folio, size_t offset, size_t len);
> > > bool iomap_dirty_folio(struct address_space *mapping, struct folio *folio);
> > > int iomap_file_unshare(struct inode *inode, loff_t pos, loff_t len,
> > > const struct iomap_ops *ops);
> > > +loff_t iomap_fill_dirty_folios(struct iomap_iter *iter, loff_t offset,
> > > + loff_t length);
> > > int iomap_zero_range(struct inode *inode, loff_t pos, loff_t len,
> > > bool *did_zero, const struct iomap_ops *ops, void *private);
> > > int iomap_truncate_page(struct inode *inode, loff_t pos, bool *did_zero,
> > > --
> > > 2.49.0
> > >
> > >
> >
>
>
