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@xxxxxxxxxxxxx
