Re: [PATCH 5/7] iomap: write iomap validity checks

[Dave Chinner <david@fromorbit.com>]

On Wed, Nov 02, 2022 at 09:58:34AM -0700, Darrick J. Wong wrote:
> On Wed, Nov 02, 2022 at 09:43:53AM -0700, Darrick J. Wong wrote:
> > On Wed, Nov 02, 2022 at 01:36:41AM -0700, Christoph Hellwig wrote:
> > > On Tue, Nov 01, 2022 at 11:34:10AM +1100, Dave Chinner wrote:
> > > > +	/*
> > > > +	 * 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
> > > > +	 * mapping can change due to concurrent IO in flight (e.g.
> > > > +	 * IOMAP_UNWRITTEN state can change and memory reclaim could have
> > > > +	 * reclaimed a previously partially written page at this index after IO
> > > > +	 * completion before this write reaches this file offset) and hence we
> > > > +	 * could do the wrong thing here (zero a page range incorrectly or fail
> > > > +	 * to zero) and corrupt data.
> > > > +	 */
> > > > +	if (ops->iomap_valid) {
> > > > +		bool iomap_valid = ops->iomap_valid(iter->inode, &iter->iomap);
> > > > +
> > > > +		if (!iomap_valid) {
> > > > +			iter->iomap.flags |= IOMAP_F_STALE;
> > > > +			status = 0;
> > > > +			goto out_unlock;
> > > > +		}
> > > > +	}
> > > 
> > > So the design so far has been that everything that applies at a page (or
> > > now folio) level goes into iomap_page_ops, not iomap_ops which is just
> > > the generic iteration, and I think we should probably do it that way.
> > 
> > I disagree here -- IMHO the sequence number is an attribute of the
> > iomapping, not the folio.
> 
> OFC now that I've reread iomap.h I realize that iomap_page_ops are
> passed back via struct iomap, so I withdraw this comment.

My first thought was to make this a page op, but I ended up deciding
against that because it isn't operating on the folio at all.
Perhaps I misunderstood what "page_ops" was actually intended for,
because it seems that the existing hooks are to allow the filesystem
to wrap per-folio operations with an external context, not to
perform iomap-specific per-folio operations.

I guess if I read "pageops" as "operations to perform on each folio
in an operation", then validating the iomap is not stale once the
folio is locked could be considered a page op. I think we could
probably make that work for writeback, too, because we have the
folio locked when we call ->map_blocks....

-Dave.
-- 
Dave Chinner
david@fromorbit.com