Re: [PATCH v1 2/8] iomap: add IOMAP_IN_MEM iomap type

[Joanne Koong <joannelkoong@gmail.com>]

On Wed, Jun 11, 2025 at 11:50 AM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Wed, Jun 11, 2025 at 11:33:40AM -0700, Joanne Koong wrote:
> > On Tue, Jun 10, 2025 at 11:00 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> > >
> > > On Tue, Jun 10, 2025 at 9:04 PM Christoph Hellwig <hch@infradead.org> wrote:
> > > >
> > > > On Tue, Jun 10, 2025 at 01:13:09PM -0700, Joanne Koong wrote:
> > > >
> > > > > For fuse at least, we definitely want granular reads, since reads may
> > > > > be extremely expensive (eg it may be a network fetch) and there's
> > > > > non-trivial mempcy overhead incurred with fuse needing to memcpy read
> > > > > buffer data from userspace back to the kernel.
> > > >
> > > > Ok, with that the plain ->read_folio variant is not going to fly.
> > > >
> > > > > > +               folio_lock(folio);
> > > > > > +               if (unlikely(folio->mapping != inode->i_mapping))
> > > > > > +                       return 1;
> > > > > > +               if (unlikely(!iomap_validate(iter)))
> > > > > > +                       return 1;
> > > > >
> > > > > Does this now basically mean that every caller that uses iomap for
> > > > > writes will have to implement ->iomap_valid and up the sequence
> > > > > counter anytime there's a write or truncate, in case the folio changes
> > > > > during the lock drop? Or were we already supposed to be doing this?
> > > >
> > > > Not any more than before.  It's is still option, but you still
> > > > very much want it to protect against races updating the mapping.
> > > >
> > > Okay thanks, I think I'll need to add this in for fuse then. I'll look
> > > at this some more
> >
> > I read some of the thread in [1] and I don't think fuse needs this
> > after all. The iomap mapping won't be changing state and concurrent
> > writes are already protected by the file lock (if we don't use the
> > plain ->read_folio variant).
> >
> > [1] https://lore.kernel.org/linux-xfs/20220817093627.GZ3600936@dread.disaster.area/
>
> <nod> If the mapping types don't change between read/write (which take
> i_rwsem in exclusive mode) and writeback (which doesn't take it at all)
> then I don't think there's a need to revalidate the mapping after
> grabbing a folio.  I think the other ways to avoid those races are (a)
> avoid unaligned zeroing if you can guarantee that the folios are always
> fully uptodate; and (b) don't do things that change the out-of-place
> write status of pagecache (e.g. reflink).
>
Awesome, thanks for verifying

I'll submit v2 rebased on top of the linux tree (if fuse is still
behind mainline) after Christoph sends out his patch

> --D