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

Commit Message

Goldwyn Rodrigues Sept. 5, 2019, 3:06 p.m. UTC

From: Goldwyn Rodrigues <[e-mail]>

Set the IOMAP_F_COW flag and create the srcmap based on
current extents to read from.

Signed-off-by: Goldwyn Rodrigues <[e-mail]>
---
 fs/xfs/xfs_iomap.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Christoph Hellwig Sept. 6, 2019, 4:55 p.m. UTC | #1

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

Dave Chinner Sept. 6, 2019, 11:28 p.m. UTC | #2

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.

Patch

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;
 		end_fsb = imap.br_startoff + imap.br_blockcount;
 		length = XFS_FSB_TO_B(mp, end_fsb) - offset;
 	}