Re: [PATCH 15/15] xfs: Use the new iomap infrastructure for CoW

Dave Chinner
2019-09-06T23:28:18Z

On Fri, Sep 06, 2019 at 06:55:07PM +0200, Christoph Hellwig wrote:
> On Thu, Sep 05, 2019 at 10:06:50AM -0500, Goldwyn Rodrigues wrote:
> > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> > index 8321733c16c3..13495d8a1ee2 100644
> > --- a/fs/xfs/xfs_iomap.c
> > +++ b/fs/xfs/xfs_iomap.c
> > @@ -1006,7 +1006,10 @@ xfs_file_iomap_begin(
> > */
> > if (directio || imap.br_startblock == HOLESTARTBLOCK)
> > imap = cmap;
> > + else
> > + xfs_bmbt_to_iomap(ip, srcmap, &cmap, false);
> >
> > + iomap->flags |= IOMAP_F_COW;
>
> I don't think this is correct. We should only set IOMAP_F_COW
> when we actually fill out the srcmap. Which is a very good agument
> for Darrick's suggestion to check for a non-emptry srcmap.
>
> Also this is missing the actually interesting part in
> xfs_file_iomap_begin_delay.
>
> I ended up spending the better half of the day trying to implement
> that and did run into a few bugs in the core iomap changes, mostly
> due to a confusion which iomap to use. So I ended up reworking those
> a bit to:
>
> a) check srcmap->type to see if there is a valid srcmap
> b) set the srcmap pointer to iomap so that it doesn't need to
> be special cased all over
> c) fixed up a few more places to use the srcmap
>
> This now at least survives xfstests -g quick on a 4k xfs file system
> for. Here is my current tree:
>
> http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-cow-iomap

That looks somewhat reasonable. The XFS mapping function is turning
into spagetti and getting really hard to follow again, though.
Perhaps we should consider splitting the shared/COW path out of
it...

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com