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.
--D
> > I'm a little disappointed that we need two callout almost next to each
> > other, but given that we need to validate with the folio locked, and
> > gfs2 wants the callback with the folio unlocked I think we have to do
> > it that.
>
> <nod>
>
> --D
