Re: [PATCH 6/7] xfs: use iomap_valid method to detect stale cached iomaps

Darrick J. Wong
2022-11-08T00:01:45Z

On Thu, Nov 03, 2022 at 09:36:05AM +1100, Dave Chinner wrote:
> On Wed, Nov 02, 2022 at 10:19:33AM -0700, Darrick J. Wong wrote:
> > On Tue, Nov 01, 2022 at 11:34:11AM +1100, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > >
> > > Now that iomap supports a mechanism to validate cached iomaps for
> > > buffered write operations, hook it up to the XFS buffered write ops
> > > so that we can avoid data corruptions that result from stale cached
> > > iomaps. See:
> > >
> > > https://lore.kernel.org/linux-xfs/20220817093627.GZ3600936@dread.disaster.area/
> > >
> > > or the ->iomap_valid() introduction commit for exact details of the
> > > corruption vector.
> > >
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > ---
> > > fs/xfs/libxfs/xfs_bmap.c | 4 +--
> > > fs/xfs/xfs_aops.c | 2 +-
> > > fs/xfs/xfs_iomap.c | 69 ++++++++++++++++++++++++++++++----------
> > > fs/xfs/xfs_iomap.h | 4 +--
> > > fs/xfs/xfs_pnfs.c | 5 +--
> > > 5 files changed, 61 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > > index 49d0d4ea63fc..db225130618c 100644
> > > --- a/fs/xfs/libxfs/xfs_bmap.c
> > > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > > @@ -4551,8 +4551,8 @@ xfs_bmapi_convert_delalloc(
> > > * the extent. Just return the real extent at this offset.
> > > */
> > > if (!isnullstartblock(bma.got.br_startblock)) {
> > > - xfs_bmbt_to_iomap(ip, iomap, &bma.got, 0, flags);
> > > *seq = READ_ONCE(ifp->if_seq);
> > > + xfs_bmbt_to_iomap(ip, iomap, &bma.got, 0, flags, *seq);
> > > goto out_trans_cancel;
> > > }
> > >
> > > @@ -4599,8 +4599,8 @@ xfs_bmapi_convert_delalloc(
> > > XFS_STATS_INC(mp, xs_xstrat_quick);
> > >
> > > ASSERT(!isnullstartblock(bma.got.br_startblock));
> > > - xfs_bmbt_to_iomap(ip, iomap, &bma.got, 0, flags);
> > > *seq = READ_ONCE(ifp->if_seq);
> > > + xfs_bmbt_to_iomap(ip, iomap, &bma.got, 0, flags, *seq);
> > >
> > > if (whichfork == XFS_COW_FORK)
> >
> > Hmm. @ifp here could be the cow fork, which means *seq will be from the
> > cow fork. This I think is going to cause problems in
> > xfs_buffered_write_iomap_valid...
>
> We can't get here from the buffered write path. This can only be
> reached by the writeback path via:
>
> iomap_do_writepage
> iomap_writepage_map
> xfs_map_blocks
> xfs_convert_blocks
> xfs_bmapi_convert_delalloc
>
> Hence the sequence numbers stored in iomap->private here are
> completely ignored by the writeback code. They still get stored
> in the struct xfs_writepage_ctx and checked by xfs_imap_valid(), so
> the changes here were really just making sure all the
> xfs_bmbt_to_iomap() callers stored the sequence number consistently.
>
> > > @@ -839,24 +848,25 @@ xfs_direct_write_iomap_begin(
> > > xfs_iunlock(ip, lockmode);
> > >
> > > error = xfs_iomap_write_direct(ip, offset_fsb, end_fsb - offset_fsb,
> > > - flags, &imap);
> > > + flags, &imap, &seq);
> > > if (error)
> > > return error;
> > >
> > > trace_xfs_iomap_alloc(ip, offset, length, XFS_DATA_FORK, &imap);
> > > return xfs_bmbt_to_iomap(ip, iomap, &imap, flags,
> > > - iomap_flags | IOMAP_F_NEW);
> > > + iomap_flags | IOMAP_F_NEW, seq);
> > >
> > > out_found_cow:
> > > + seq = READ_ONCE(ip->i_df.if_seq);
> > > xfs_iunlock(ip, lockmode);
> > > length = XFS_FSB_TO_B(mp, cmap.br_startoff + cmap.br_blockcount);
> > > trace_xfs_iomap_found(ip, offset, length - offset, XFS_COW_FORK, &cmap);
> > > if (imap.br_startblock != HOLESTARTBLOCK) {
> > > - error = xfs_bmbt_to_iomap(ip, srcmap, &imap, flags, 0);
> > > + error = xfs_bmbt_to_iomap(ip, srcmap, &imap, flags, 0, seq);
> > > if (error)
> > > return error;
> > > }
> > > - return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, IOMAP_F_SHARED);
> > > + return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, IOMAP_F_SHARED, seq);
> >
> > Here we've found a mapping from the COW fork and we're encoding it into
> > the struct iomap. Why is the sequence number from the *data* fork and
> > not the COW fork?
>
> Because my brain isn't big enough to hold all this code straight in
> my head. I found it all too easy to get confused by what iomap is
> passed where and how to tell them apart and I could not tell if
> there was an easy way to tell if the iomap passed to
> xfs_iomap_valid() needed to check against the data or cow fork.
>
> Hence I set it up such that the xfs_iomap_valid() callback only
> checks the data fork sequence so the only correct thing to set in
> the iomap is the data fork sequence and moved on to the next part of
> the problem...
>
> But, again, this information probably should be encoded into the
> sequence cookie we store. Which begs the question - if we are doing
> a COW operation, we have to check that neither the data fork (the
> srcmap) nor the COW fork (the iter->iomap) have changed, right? This
> is what the writeback code does (i.e. checks both data and COW fork
> sequence numbers), so perhaps that's also what we should be doing
> here?
>
> i.e. either the cookie for COW operations needs to contain both
> data+cow sequence numbers, and both need to match to proceed, or we
> have to validate both the srcmap and the iter->iomap with separate
> callouts once the folio is locked.
>
> Thoughts?

As we sorta discussed last week on IRC, I guess you could start by
encoding both values (as needed) in something fugly like this in struct
iomap:

union {
void *private;
u64 cookie;
};

But the longer term solution is (I suspect) to make a new struct that
XFS can envelop:

struct iomap_pagecache_ctx {
struct iomap_iter iter; /* do not touch */
struct iomap_page_ops *ops; /* callers can attach here */
};

struct xfs_pagecache_ctx {
struct iomap_pagecache_ctx xpctx;
u32 data_seq;
u32 cow_seq;
};

and pass that into the iomap functions:

struct xfs_pagecache_ctx ctx = { };

ret = iomap_file_buffered_write(iocb, from, &ctx,
&xfs_buffered_write_iomap_ops);

so that xfs_iomap_revalidate can do the usual struct unwrapping:

struct xfs_pagecache_ctx *xpctx;

xpctx = container_of(ipctx, struct xfs_pagecache_ctx, ipctx);
if (xpctx->data_seq != ip->i_df.df_seq)
return NOPE;

But that's a pretty aggressive refactoring, not suitable for a bugfix,
do it afterwards while you're waiting for the rest of us to check over
part 1, etc.

--D

> > > @@ -1328,9 +1342,26 @@ xfs_buffered_write_iomap_end(
> > > return 0;
> > > }
> > >
> > > +/*
> > > + * Check that the iomap passed to us is still valid for the given offset and
> > > + * length.
> > > + */
> > > +static bool
> > > +xfs_buffered_write_iomap_valid(
> > > + struct inode *inode,
> > > + const struct iomap *iomap)
> > > +{
> > > + int seq = *((int *)&iomap->private);
> > > +
> > > + if (seq != READ_ONCE(XFS_I(inode)->i_df.if_seq))
> >
> > Why is it ok to sample the data fork's sequence number even if the
> > mapping came from the COW fork? That doesn't make sense to me,
> > conceptually. I definitely think it's buggy that the revalidation
> > might not sample from the same sequence counter as the original
> > measurement.
>
> Yup, see above.
>
> > > const struct iomap_ops xfs_read_iomap_ops = {
> > > @@ -1438,7 +1471,7 @@ xfs_seek_iomap_begin(
> > > end_fsb = min(end_fsb, data_fsb);
> > > xfs_trim_extent(&cmap, offset_fsb, end_fsb);
> > > error = xfs_bmbt_to_iomap(ip, iomap, &cmap, flags,
> > > - IOMAP_F_SHARED);
> > > + IOMAP_F_SHARED, READ_ONCE(ip->i_cowfp->if_seq));
> >
> > Here we explicitly sample from the cow fork but the comparison is done
> > against the data fork. That /probably/ doesn't matter for reads since
> > you're not proposing that we revalidate on those. Should we?
>
> As I said elsewhere, I haven't even looked at the read path. The
> iomap code is now complex enough that I have trouble following it
> and keeping all the corner cases in my head. This is the down side
> of having people smarter than you write the code you need to debug
> when shit inevitably goes wrong.
>
> > What
> > happens if userspace hands us a large read() from an unwritten extent
> > and the same dirty/writeback/reclaim sequence happens to the last folio
> > in that read() range before read_iter actually gets there?
>
> No idea - shit may well go wrong there in the same way, but I've had
> my hands full just fixing the write path. I'm *really* tired of
> fighting with the iomap code right now...
>
> > > @@ -189,7 +190,7 @@ xfs_fs_map_blocks(
> > > xfs_iunlock(ip, lock_flags);
> > >
> > > error = xfs_iomap_write_direct(ip, offset_fsb,
> > > - end_fsb - offset_fsb, 0, &imap);
> > > + end_fsb - offset_fsb, 0, &imap, &seq);
> >
> > I got a compiler warning about seq not being set in the case where we
> > don't call xfs_iomap_write_direct.
>
> Yup, gcc 11.2 doesn't warn about it. Now to find out why my build
> system is using gcc 11.2 when I upgraded it months ago to use
> gcc-12 because of exactly these sorts of issues.... :(

--D

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