optimize the COW I/O path

optimize the COW I/O path

[Christoph Hellwig]

This series contains a couple optimizations for the COW I/O path:

 - avoid any overhead for reads from COW files
 - streamline the COW handling in write_begin
 - reduce the number of extent lookups during COW I/O completion

It does not touch the direct I/O path (yet), I still have a few
unsolved problems there.

[PATCH 1/9] iomap: add IOMAP_REPORT

[Christoph Hellwig]

This allows the file system to tell a FIEMAP from a read operation, and thus
avoids the need to report flags that aren't actually used in the read path.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap.c            |  2 +-
 include/linux/iomap.h | 17 +++++++++++------
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/fs/iomap.c b/fs/iomap.c
index 013d1d3..a922040 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -561,7 +561,7 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
 	}
 
 	while (len > 0) {
-		ret = iomap_apply(inode, start, len, 0, ops, &ctx,
+		ret = iomap_apply(inode, start, len, IOMAP_REPORT, ops, &ctx,
 				iomap_fiemap_actor);
 		/* inode with no (attribute) mapping will give ENOENT */
 		if (ret == -ENOENT)
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index e63e288..7892f55 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -19,11 +19,15 @@ struct vm_fault;
 #define IOMAP_UNWRITTEN	0x04	/* blocks allocated @blkno in unwritten state */
 
 /*
- * Flags for iomap mappings:
+ * Flags for all iomap mappings:
  */
-#define IOMAP_F_MERGED	0x01	/* contains multiple blocks/extents */
-#define IOMAP_F_SHARED	0x02	/* block shared with another file */
-#define IOMAP_F_NEW	0x04	/* blocks have been newly allocated */
+#define IOMAP_F_NEW	0x01	/* blocks have been newly allocated */
+
+/*
+ * Flags that only need to be reported for IOMAP_REPORT requests:
+ */
+#define IOMAP_F_MERGED	0x10	/* contains multiple blocks/extents */
+#define IOMAP_F_SHARED	0x20	/* block shared with another file */
 
 /*
  * Magic value for blkno:
@@ -42,8 +46,9 @@ struct iomap {
 /*
  * Flags for iomap_begin / iomap_end.  No flag implies a read.
  */
-#define IOMAP_WRITE		(1 << 0)
-#define IOMAP_ZERO		(1 << 1)
+#define IOMAP_WRITE		(1 << 0) /* writing, must allocate blocks */
+#define IOMAP_ZERO		(1 << 1) /* zeroing operation, may skip holes */
+#define IOMAP_REPORT		(1 << 2) /* report extent status, e.g. FIEMAP */
 
 struct iomap_ops {
 	/*
-- 
2.10.1.382.ga23ca1b

Re: [PATCH 1/9] iomap: add IOMAP_REPORT

[Darrick J. Wong]

On Mon, Oct 10, 2016 at 03:37:57PM +0200, Christoph Hellwig wrote:
> This allows the file system to tell a FIEMAP from a read operation, and thus
> avoids the need to report flags that aren't actually used in the read path.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/iomap.c            |  2 +-
>  include/linux/iomap.h | 17 +++++++++++------
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/iomap.c b/fs/iomap.c
> index 013d1d3..a922040 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -561,7 +561,7 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
>  	}
>  
>  	while (len > 0) {
> -		ret = iomap_apply(inode, start, len, 0, ops, &ctx,
> +		ret = iomap_apply(inode, start, len, IOMAP_REPORT, ops, &ctx,

Hmm.  /me notices that generic/352, generic/353, and generic/372 fail after
this patchset is applied.  Will have a look at this tomorrow, unless this rings
a bell?

--D

>  				iomap_fiemap_actor);
>  		/* inode with no (attribute) mapping will give ENOENT */
>  		if (ret == -ENOENT)
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index e63e288..7892f55 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -19,11 +19,15 @@ struct vm_fault;
>  #define IOMAP_UNWRITTEN	0x04	/* blocks allocated @blkno in unwritten state */
>  
>  /*
> - * Flags for iomap mappings:
> + * Flags for all iomap mappings:
>   */
> -#define IOMAP_F_MERGED	0x01	/* contains multiple blocks/extents */
> -#define IOMAP_F_SHARED	0x02	/* block shared with another file */
> -#define IOMAP_F_NEW	0x04	/* blocks have been newly allocated */
> +#define IOMAP_F_NEW	0x01	/* blocks have been newly allocated */
> +
> +/*
> + * Flags that only need to be reported for IOMAP_REPORT requests:
> + */
> +#define IOMAP_F_MERGED	0x10	/* contains multiple blocks/extents */
> +#define IOMAP_F_SHARED	0x20	/* block shared with another file */
>  
>  /*
>   * Magic value for blkno:
> @@ -42,8 +46,9 @@ struct iomap {
>  /*
>   * Flags for iomap_begin / iomap_end.  No flag implies a read.
>   */
> -#define IOMAP_WRITE		(1 << 0)
> -#define IOMAP_ZERO		(1 << 1)
> +#define IOMAP_WRITE		(1 << 0) /* writing, must allocate blocks */
> +#define IOMAP_ZERO		(1 << 1) /* zeroing operation, may skip holes */
> +#define IOMAP_REPORT		(1 << 2) /* report extent status, e.g. FIEMAP */
>  
>  struct iomap_ops {
>  	/*
> -- 
> 2.10.1.382.ga23ca1b
> 

Re: [PATCH 1/9] iomap: add IOMAP_REPORT

[Christoph Hellwig]

On Mon, Oct 10, 2016 at 06:45:46PM -0700, Darrick J. Wong wrote:
> On Mon, Oct 10, 2016 at 03:37:57PM +0200, Christoph Hellwig wrote:
> > This allows the file system to tell a FIEMAP from a read operation, and thus
> > avoids the need to report flags that aren't actually used in the read path.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  fs/iomap.c            |  2 +-
> >  include/linux/iomap.h | 17 +++++++++++------
> >  2 files changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/iomap.c b/fs/iomap.c
> > index 013d1d3..a922040 100644
> > --- a/fs/iomap.c
> > +++ b/fs/iomap.c
> > @@ -561,7 +561,7 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
> >  	}
> >  
> >  	while (len > 0) {
> > -		ret = iomap_apply(inode, start, len, 0, ops, &ctx,
> > +		ret = iomap_apply(inode, start, len, IOMAP_REPORT, ops, &ctx,
> 
> Hmm.  /me notices that generic/352, generic/353, and generic/372 fail after
> this patchset is applied.  Will have a look at this tomorrow, unless this rings
> a bell?

I don't see any of those failures.

Re: [PATCH 1/9] iomap: add IOMAP_REPORT

[Darrick J. Wong]

On Tue, Oct 11, 2016 at 04:45:57PM +0200, Christoph Hellwig wrote:
> Just curious:  did you maybe manage to only rebuild the xfs module
> and not the core code and have a mismatch of the flags values?

D'oh!  Yes, I forgot to reboot the VMs to pick up the new vmlinux.  Everything
checks out now.

--D

[PATCH 2/9] xfs: add xfs_trim_extent

[Christoph Hellwig]

From: "Darrick J. Wong" <darrick.wong@oracle.com>

This helpers allows to trim an extent to a subset of it's original range
while making sure the block numbers in it remain valid,

In the future xfs_trim_extent and xfs_bmapi_trim_map should probably be
merged in some form.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
[hch: split from a previous patch from Darrick, moved around and added
 support for "raw" delayed extents"]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c | 33 +++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_bmap.h |  2 ++
 2 files changed, 35 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index c27344c..016dacc 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3999,6 +3999,39 @@ xfs_bmap_alloc(
 	return xfs_bmap_btalloc(ap);
 }
 
+/* Trim extent to fit a logical block range. */
+void
+xfs_trim_extent(
+	struct xfs_bmbt_irec	*irec,
+	xfs_fileoff_t		bno,
+	xfs_filblks_t		len)
+{
+	xfs_fileoff_t		distance;
+	xfs_fileoff_t		end = bno + len;
+
+	if (irec->br_startoff + irec->br_blockcount <= bno ||
+	    irec->br_startoff >= end) {
+		irec->br_blockcount = 0;
+		return;
+	}
+
+	if (irec->br_startoff < bno) {
+		distance = bno - irec->br_startoff;
+		if (isnullstartblock(irec->br_startblock))
+			irec->br_startblock = DELAYSTARTBLOCK;
+		if (irec->br_startblock != DELAYSTARTBLOCK &&
+		    irec->br_startblock != HOLESTARTBLOCK)
+			irec->br_startblock += distance;
+		irec->br_startoff += distance;
+		irec->br_blockcount -= distance;
+	}
+
+	if (end < irec->br_startoff + irec->br_blockcount) {
+		distance = irec->br_startoff + irec->br_blockcount - end;
+		irec->br_blockcount -= distance;
+	}
+}
+
 /*
  * Trim the returned map to the required bounds
  */
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index f97db71..eb86af0 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -190,6 +190,8 @@ void	xfs_bmap_trace_exlist(struct xfs_inode *ip, xfs_extnum_t cnt,
 #define	XFS_BMAP_TRACE_EXLIST(ip,c,w)
 #endif
 
+void	xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno,
+		xfs_filblks_t len);
 int	xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd);
 void	xfs_bmap_local_to_extents_empty(struct xfs_inode *ip, int whichfork);
 void	xfs_bmap_add_free(struct xfs_mount *mp, struct xfs_defer_ops *dfops,
-- 
2.10.1.382.ga23ca1b

[PATCH 3/9] xfs: handle "raw" delayed extents xfs_reflink_trim_around_shared

[Christoph Hellwig]

Delalloc extents in the extent list contain the number of reserved
indirect blocks in their startblock value and don't use the magic
DELAYSTARTBLOCK constant.  Ensure that xfs_reflink_trim_around_shared
handles them properly by checking for isnullstartblock().

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_reflink.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 5965e94..46c824c 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -182,7 +182,8 @@ xfs_reflink_trim_around_shared(
 	if (!xfs_is_reflink_inode(ip) ||
 	    ISUNWRITTEN(irec) ||
 	    irec->br_startblock == HOLESTARTBLOCK ||
-	    irec->br_startblock == DELAYSTARTBLOCK) {
+	    irec->br_startblock == DELAYSTARTBLOCK ||
+	    isnullstartblock(irec->br_startblock)) {
 		*shared = false;
 		return 0;
 	}
-- 
2.10.1.382.ga23ca1b

[PATCH 4/9] xfs: don't bother looking at the refcount tree for reads

[Christoph Hellwig]

There is no need to trim an extent into a shared or non-shared one, or
report any flags for plain old reads.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_iomap.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index d907eb9..1dabf2e 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -996,11 +996,14 @@ xfs_file_iomap_begin(
 		return error;
 	}
 
-	/* Trim the mapping to the nearest shared extent boundary. */
-	error = xfs_reflink_trim_around_shared(ip, &imap, &shared, &trimmed);
-	if (error) {
-		xfs_iunlock(ip, lockmode);
-		return error;
+	if (flags & (IOMAP_WRITE | IOMAP_ZERO | IOMAP_REPORT)) {
+		/* Trim the mapping to the nearest shared extent boundary. */
+		error = xfs_reflink_trim_around_shared(ip, &imap, &shared,
+				&trimmed);
+		if (error) {
+			xfs_iunlock(ip, lockmode);
+			return error;
+		}
 	}
 
 	if ((flags & IOMAP_WRITE) && imap_needs_alloc(inode, &imap, nimaps)) {
-- 
2.10.1.382.ga23ca1b

[PATCH 5/9] xfs: optimize writes to reflink files

[Christoph Hellwig]

Instead of reserving space as the first thing in write_begin move it past
reading the extent in the data fork.  That way we only have to read from
the data fork once and can reuse that information for trimming the extent
to the shared/unshared boundary.  Additionally this allows to easily
limit the actual write size to said boundary, and avoid a roundtrip on the
ilock.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_iomap.c   |  56 +++++++++++++-------
 fs/xfs/xfs_reflink.c | 141 +++++++++++++++++++++------------------------------
 fs/xfs/xfs_reflink.h |   4 +-
 fs/xfs/xfs_trace.h   |   3 +-
 4 files changed, 100 insertions(+), 104 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 1dabf2e..436e109 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -566,6 +566,17 @@ xfs_file_iomap_begin_delay(
 	xfs_bmap_search_extents(ip, offset_fsb, XFS_DATA_FORK, &eof, &idx,
 			&got, &prev);
 	if (!eof && got.br_startoff <= offset_fsb) {
+		if (xfs_is_reflink_inode(ip)) {
+			bool		shared;
+
+			end_fsb = min(XFS_B_TO_FSB(mp, offset + count),
+					maxbytes_fsb);
+			xfs_trim_extent(&got, offset_fsb, end_fsb - offset_fsb);
+			error = xfs_reflink_reserve_cow(ip, &got, &shared);
+			if (error)
+				goto out_unlock;
+		}
+
 		trace_xfs_iomap_found(ip, offset, count, 0, &got);
 		goto done;
 	}
@@ -961,19 +972,13 @@ xfs_file_iomap_begin(
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_bmbt_irec	imap;
 	xfs_fileoff_t		offset_fsb, end_fsb;
-	bool			shared, trimmed;
 	int			nimaps = 1, error = 0;
+	bool			shared = false, trimmed = false;
 	unsigned		lockmode;
 
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return -EIO;
 
-	if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && xfs_is_reflink_inode(ip)) {
-		error = xfs_reflink_reserve_cow_range(ip, offset, length);
-		if (error < 0)
-			return error;
-	}
-
 	if ((flags & IOMAP_WRITE) && !IS_DAX(inode) &&
 		   !xfs_get_extsz_hint(ip)) {
 		/* Reserve delalloc blocks for regular writeback. */
@@ -981,7 +986,16 @@ xfs_file_iomap_begin(
 				iomap);
 	}
 
-	lockmode = xfs_ilock_data_map_shared(ip);
+	/*
+	 * COW writes will allocate delalloc space, so we need to make sure
+	 * to take the lock exclusively here.
+	 */
+	if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && xfs_is_reflink_inode(ip)) {
+		lockmode = XFS_ILOCK_EXCL;
+		xfs_ilock(ip, XFS_ILOCK_EXCL);
+	} else {
+		lockmode = xfs_ilock_data_map_shared(ip);
+	}
 
 	ASSERT(offset <= mp->m_super->s_maxbytes);
 	if ((xfs_fsize_t)offset + length > mp->m_super->s_maxbytes)
@@ -991,19 +1005,24 @@ xfs_file_iomap_begin(
 
 	error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb, &imap,
 			       &nimaps, 0);
-	if (error) {
-		xfs_iunlock(ip, lockmode);
-		return error;
-	}
+	if (error)
+		goto out_unlock;
 
-	if (flags & (IOMAP_WRITE | IOMAP_ZERO | IOMAP_REPORT)) {
+	if (flags & IOMAP_REPORT) {
 		/* Trim the mapping to the nearest shared extent boundary. */
 		error = xfs_reflink_trim_around_shared(ip, &imap, &shared,
 				&trimmed);
-		if (error) {
-			xfs_iunlock(ip, lockmode);
-			return error;
-		}
+		if (error)
+			goto out_unlock;
+	}
+
+	if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && xfs_is_reflink_inode(ip)) {
+		error = xfs_reflink_reserve_cow(ip, &imap, &shared);
+		if (error)
+			goto out_unlock;
+
+		end_fsb = imap.br_startoff + imap.br_blockcount;
+		length = XFS_FSB_TO_B(mp, end_fsb) - offset;
 	}
 
 	if ((flags & IOMAP_WRITE) && imap_needs_alloc(inode, &imap, nimaps)) {
@@ -1042,6 +1061,9 @@ xfs_file_iomap_begin(
 	if (shared)
 		iomap->flags |= IOMAP_F_SHARED;
 	return 0;
+out_unlock:
+	xfs_iunlock(ip, lockmode);
+	return error;
 }
 
 static int
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 46c824c..5d230ea 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -228,50 +228,54 @@ xfs_reflink_trim_around_shared(
 	}
 }
 
-/* Create a CoW reservation for a range of blocks within a file. */
-static int
-__xfs_reflink_reserve_cow(
+/*
+ * Trim the passed in imap to the next shared/unshared extent boundary, and
+ * if imap->br_startoff points to a shared extent reserve space for it in the
+ * COW fork.  In this case *shared is set to true, else to false.
+ *
+ * Note that imap will always contain the block numbers for the existing blocks
+ * in the data fork, as the upper layers need them for read-modify-write
+ * operations.
+ */
+int
+xfs_reflink_reserve_cow(
 	struct xfs_inode	*ip,
-	xfs_fileoff_t		*offset_fsb,
-	xfs_fileoff_t		end_fsb,
-	bool			*skipped)
+	struct xfs_bmbt_irec	*imap,
+	bool			*shared)
 {
-	struct xfs_bmbt_irec	got, prev, imap;
-	xfs_fileoff_t		orig_end_fsb;
-	int			nimaps, eof = 0, error = 0;
-	bool			shared = false, trimmed = false;
+	struct xfs_bmbt_irec	got, prev;
+	xfs_fileoff_t		end_fsb, orig_end_fsb;
+	int			eof = 0, error = 0;
+	bool			trimmed;
 	xfs_extnum_t		idx;
 	xfs_extlen_t		align;
 
-	/* Already reserved?  Skip the refcount btree access. */
-	xfs_bmap_search_extents(ip, *offset_fsb, XFS_COW_FORK, &eof, &idx,
+	/*
+	 * Search the COW fork extent list first.  This serves two purposes:
+	 * first this implement the speculative preallocation using cowextisze,
+	 * so that we also unshared block adjacent to shared blocks instead
+	 * of just the shared blocks themselves.  Second the lookup in the
+	 * extent list is generally faster than going out to the shared extent
+	 * tree.
+	 */
+	xfs_bmap_search_extents(ip, imap->br_startoff, XFS_COW_FORK, &eof, &idx,
 			&got, &prev);
-	if (!eof && got.br_startoff <= *offset_fsb) {
-		end_fsb = orig_end_fsb = got.br_startoff + got.br_blockcount;
-		trace_xfs_reflink_cow_found(ip, &got);
-		goto done;
-	}
+	if (!eof && got.br_startoff <= imap->br_startoff) {
+		trace_xfs_reflink_cow_found(ip, imap);
+		xfs_trim_extent(imap, got.br_startoff, got.br_blockcount);
 
-	/* Read extent from the source file. */
-	nimaps = 1;
-	error = xfs_bmapi_read(ip, *offset_fsb, end_fsb - *offset_fsb,
-			&imap, &nimaps, 0);
-	if (error)
-		goto out_unlock;
-	ASSERT(nimaps == 1);
+		*shared = true;
+		return 0;
+	}
 
 	/* Trim the mapping to the nearest shared extent boundary. */
-	error = xfs_reflink_trim_around_shared(ip, &imap, &shared, &trimmed);
+	error = xfs_reflink_trim_around_shared(ip, imap, shared, &trimmed);
 	if (error)
-		goto out_unlock;
-
-	end_fsb = orig_end_fsb = imap.br_startoff + imap.br_blockcount;
+		return error;
 
 	/* Not shared?  Just report the (potentially capped) extent. */
-	if (!shared) {
-		*skipped = true;
-		goto done;
-	}
+	if (!*shared)
+		return 0;
 
 	/*
 	 * Fork all the shared blocks from our write offset until the end of
@@ -279,73 +283,40 @@ __xfs_reflink_reserve_cow(
 	 */
 	error = xfs_qm_dqattach_locked(ip, 0);
 	if (error)
-		goto out_unlock;
+		return error;
+
+	end_fsb = orig_end_fsb = imap->br_startoff + imap->br_blockcount;
 
 	align = xfs_eof_alignment(ip, xfs_get_cowextsz_hint(ip));
 	if (align)
 		end_fsb = roundup_64(end_fsb, align);
 
 retry:
-	error = xfs_bmapi_reserve_delalloc(ip, XFS_COW_FORK, *offset_fsb,
-			end_fsb - *offset_fsb, &got,
-			&prev, &idx, eof);
+	error = xfs_bmapi_reserve_delalloc(ip, XFS_COW_FORK, imap->br_startoff,
+			end_fsb - imap->br_startoff, &got, &prev, &idx, eof);
 	switch (error) {
 	case 0:
 		break;
 	case -ENOSPC:
 	case -EDQUOT:
 		/* retry without any preallocation */
-		trace_xfs_reflink_cow_enospc(ip, &imap);
+		trace_xfs_reflink_cow_enospc(ip, imap);
 		if (end_fsb != orig_end_fsb) {
 			end_fsb = orig_end_fsb;
 			goto retry;
 		}
 		/*FALLTHRU*/
 	default:
-		goto out_unlock;
+		return error;
 	}
 
 	if (end_fsb != orig_end_fsb)
 		xfs_inode_set_cowblocks_tag(ip);
 
 	trace_xfs_reflink_cow_alloc(ip, &got);
-done:
-	*offset_fsb = end_fsb;
-out_unlock:
-	return error;
+	return 0;
 }
 
-/* Create a CoW reservation for part of a file. */
-int
-xfs_reflink_reserve_cow_range(
-	struct xfs_inode	*ip,
-	xfs_off_t		offset,
-	xfs_off_t		count)
-{
-	struct xfs_mount	*mp = ip->i_mount;
-	xfs_fileoff_t		offset_fsb, end_fsb;
-	bool			skipped = false;
-	int			error;
-
-	trace_xfs_reflink_reserve_cow_range(ip, offset, count);
-
-	offset_fsb = XFS_B_TO_FSBT(mp, offset);
-	end_fsb = XFS_B_TO_FSB(mp, offset + count);
-
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	while (offset_fsb < end_fsb) {
-		error = __xfs_reflink_reserve_cow(ip, &offset_fsb, end_fsb,
-				&skipped);
-		if (error) {
-			trace_xfs_reflink_reserve_cow_range_error(ip, error,
-				_RET_IP_);
-			break;
-		}
-	}
-	xfs_iunlock(ip, XFS_ILOCK_EXCL);
-
-	return error;
-}
 
 /* Allocate all CoW reservations covering a range of blocks in a file. */
 static int
@@ -359,9 +330,8 @@ __xfs_reflink_allocate_cow(
 	struct xfs_defer_ops	dfops;
 	struct xfs_trans	*tp;
 	xfs_fsblock_t		first_block;
-	xfs_fileoff_t		next_fsb;
 	int			nimaps = 1, error;
-	bool			skipped = false;
+	bool			shared;
 
 	xfs_defer_init(&dfops, &first_block);
 
@@ -372,33 +342,38 @@ __xfs_reflink_allocate_cow(
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 
-	next_fsb = *offset_fsb;
-	error = __xfs_reflink_reserve_cow(ip, &next_fsb, end_fsb, &skipped);
+	/* Read extent from the source file. */
+	nimaps = 1;
+	error = xfs_bmapi_read(ip, *offset_fsb, end_fsb - *offset_fsb,
+			&imap, &nimaps, 0);
+	if (error)
+		goto out_unlock;
+	ASSERT(nimaps == 1);
+
+	error = xfs_reflink_reserve_cow(ip, &imap, &shared);
 	if (error)
 		goto out_trans_cancel;
 
-	if (skipped) {
-		*offset_fsb = next_fsb;
+	if (!shared) {
+		*offset_fsb = imap.br_startoff + imap.br_blockcount;
 		goto out_trans_cancel;
 	}
 
 	xfs_trans_ijoin(tp, ip, 0);
-	error = xfs_bmapi_write(tp, ip, *offset_fsb, next_fsb - *offset_fsb,
+	error = xfs_bmapi_write(tp, ip, imap.br_startoff, imap.br_blockcount,
 			XFS_BMAPI_COWFORK, &first_block,
 			XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK),
 			&imap, &nimaps, &dfops);
 	if (error)
 		goto out_trans_cancel;
 
-	/* We might not have been able to map the whole delalloc extent */
-	*offset_fsb = min(*offset_fsb + imap.br_blockcount, next_fsb);
-
 	error = xfs_defer_finish(&tp, &dfops, NULL);
 	if (error)
 		goto out_trans_cancel;
 
 	error = xfs_trans_commit(tp);
 
+	*offset_fsb = imap.br_startoff + imap.br_blockcount;
 out_unlock:
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	return error;
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index 5dc3c8a..9f76924 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -26,8 +26,8 @@ extern int xfs_reflink_find_shared(struct xfs_mount *mp, xfs_agnumber_t agno,
 extern int xfs_reflink_trim_around_shared(struct xfs_inode *ip,
 		struct xfs_bmbt_irec *irec, bool *shared, bool *trimmed);
 
-extern int xfs_reflink_reserve_cow_range(struct xfs_inode *ip,
-		xfs_off_t offset, xfs_off_t count);
+extern int xfs_reflink_reserve_cow(struct xfs_inode *ip,
+		struct xfs_bmbt_irec *imap, bool *shared);
 extern int xfs_reflink_allocate_cow_range(struct xfs_inode *ip,
 		xfs_off_t offset, xfs_off_t count);
 extern bool xfs_reflink_find_cow_mapping(struct xfs_inode *ip, xfs_off_t offset,
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 2586c9c..520660d 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -3347,7 +3347,7 @@ DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_alloc);
 DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_found);
 DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_enospc);
 
-DEFINE_RW_EVENT(xfs_reflink_reserve_cow_range);
+DEFINE_RW_EVENT(xfs_reflink_reserve_cow);
 DEFINE_RW_EVENT(xfs_reflink_allocate_cow_range);
 
 DEFINE_INODE_IREC_EVENT(xfs_reflink_bounce_dio_write);
@@ -3359,7 +3359,6 @@ DEFINE_SIMPLE_IO_EVENT(xfs_reflink_end_cow);
 DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_remap);
 DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_remap_piece);
 
-DEFINE_INODE_ERROR_EVENT(xfs_reflink_reserve_cow_range_error);
 DEFINE_INODE_ERROR_EVENT(xfs_reflink_allocate_cow_range_error);
 DEFINE_INODE_ERROR_EVENT(xfs_reflink_cancel_cow_range_error);
 DEFINE_INODE_ERROR_EVENT(xfs_reflink_end_cow_error);
-- 
2.10.1.382.ga23ca1b

Re: [PATCH 5/9] xfs: optimize writes to reflink files

[Brian Foster]

On Mon, Oct 10, 2016 at 03:38:01PM +0200, Christoph Hellwig wrote:
> Instead of reserving space as the first thing in write_begin move it past
> reading the extent in the data fork.  That way we only have to read from
> the data fork once and can reuse that information for trimming the extent
> to the shared/unshared boundary.  Additionally this allows to easily
> limit the actual write size to said boundary, and avoid a roundtrip on the
> ilock.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_iomap.c   |  56 +++++++++++++-------
>  fs/xfs/xfs_reflink.c | 141 +++++++++++++++++++++------------------------------
>  fs/xfs/xfs_reflink.h |   4 +-
>  fs/xfs/xfs_trace.h   |   3 +-
>  4 files changed, 100 insertions(+), 104 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 1dabf2e..436e109 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -566,6 +566,17 @@ xfs_file_iomap_begin_delay(
>  	xfs_bmap_search_extents(ip, offset_fsb, XFS_DATA_FORK, &eof, &idx,
>  			&got, &prev);
>  	if (!eof && got.br_startoff <= offset_fsb) {
> +		if (xfs_is_reflink_inode(ip)) {
> +			bool		shared;
> +
> +			end_fsb = min(XFS_B_TO_FSB(mp, offset + count),
> +					maxbytes_fsb);
> +			xfs_trim_extent(&got, offset_fsb, end_fsb - offset_fsb);
> +			error = xfs_reflink_reserve_cow(ip, &got, &shared);
> +			if (error)
> +				goto out_unlock;

All in all this seems fine, but I don't see why we need to get all the
way down through xfs_reflink_reserve_cow() ->
xfs_reflink_trim_around_shared() to handle the basic delalloc overwrite
case on a reflink inode. Could we enhance the is_reflink_inode() helper
or create a new one that can consider whether the data fork extent is a
hole or delalloc?

Otherwise looks Ok to me:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> +		}
> +
>  		trace_xfs_iomap_found(ip, offset, count, 0, &got);
>  		goto done;
>  	}
> @@ -961,19 +972,13 @@ xfs_file_iomap_begin(
>  	struct xfs_mount	*mp = ip->i_mount;
>  	struct xfs_bmbt_irec	imap;
>  	xfs_fileoff_t		offset_fsb, end_fsb;
> -	bool			shared, trimmed;
>  	int			nimaps = 1, error = 0;
> +	bool			shared = false, trimmed = false;
>  	unsigned		lockmode;
>  
>  	if (XFS_FORCED_SHUTDOWN(mp))
>  		return -EIO;
>  
> -	if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && xfs_is_reflink_inode(ip)) {
> -		error = xfs_reflink_reserve_cow_range(ip, offset, length);
> -		if (error < 0)
> -			return error;
> -	}
> -
>  	if ((flags & IOMAP_WRITE) && !IS_DAX(inode) &&
>  		   !xfs_get_extsz_hint(ip)) {
>  		/* Reserve delalloc blocks for regular writeback. */
> @@ -981,7 +986,16 @@ xfs_file_iomap_begin(
>  				iomap);
>  	}
>  
> -	lockmode = xfs_ilock_data_map_shared(ip);
> +	/*
> +	 * COW writes will allocate delalloc space, so we need to make sure
> +	 * to take the lock exclusively here.
> +	 */
> +	if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && xfs_is_reflink_inode(ip)) {
> +		lockmode = XFS_ILOCK_EXCL;
> +		xfs_ilock(ip, XFS_ILOCK_EXCL);
> +	} else {
> +		lockmode = xfs_ilock_data_map_shared(ip);
> +	}
>  
>  	ASSERT(offset <= mp->m_super->s_maxbytes);
>  	if ((xfs_fsize_t)offset + length > mp->m_super->s_maxbytes)
> @@ -991,19 +1005,24 @@ xfs_file_iomap_begin(
>  
>  	error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb, &imap,
>  			       &nimaps, 0);
> -	if (error) {
> -		xfs_iunlock(ip, lockmode);
> -		return error;
> -	}
> +	if (error)
> +		goto out_unlock;
>  
> -	if (flags & (IOMAP_WRITE | IOMAP_ZERO | IOMAP_REPORT)) {
> +	if (flags & IOMAP_REPORT) {
>  		/* Trim the mapping to the nearest shared extent boundary. */
>  		error = xfs_reflink_trim_around_shared(ip, &imap, &shared,
>  				&trimmed);
> -		if (error) {
> -			xfs_iunlock(ip, lockmode);
> -			return error;
> -		}
> +		if (error)
> +			goto out_unlock;
> +	}
> +
> +	if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && xfs_is_reflink_inode(ip)) {
> +		error = xfs_reflink_reserve_cow(ip, &imap, &shared);
> +		if (error)
> +			goto out_unlock;
> +
> +		end_fsb = imap.br_startoff + imap.br_blockcount;
> +		length = XFS_FSB_TO_B(mp, end_fsb) - offset;
>  	}
>  
>  	if ((flags & IOMAP_WRITE) && imap_needs_alloc(inode, &imap, nimaps)) {
> @@ -1042,6 +1061,9 @@ xfs_file_iomap_begin(
>  	if (shared)
>  		iomap->flags |= IOMAP_F_SHARED;
>  	return 0;
> +out_unlock:
> +	xfs_iunlock(ip, lockmode);
> +	return error;
>  }
>  
>  static int
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 46c824c..5d230ea 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -228,50 +228,54 @@ xfs_reflink_trim_around_shared(
>  	}
>  }
>  
> -/* Create a CoW reservation for a range of blocks within a file. */
> -static int
> -__xfs_reflink_reserve_cow(
> +/*
> + * Trim the passed in imap to the next shared/unshared extent boundary, and
> + * if imap->br_startoff points to a shared extent reserve space for it in the
> + * COW fork.  In this case *shared is set to true, else to false.
> + *
> + * Note that imap will always contain the block numbers for the existing blocks
> + * in the data fork, as the upper layers need them for read-modify-write
> + * operations.
> + */
> +int
> +xfs_reflink_reserve_cow(
>  	struct xfs_inode	*ip,
> -	xfs_fileoff_t		*offset_fsb,
> -	xfs_fileoff_t		end_fsb,
> -	bool			*skipped)
> +	struct xfs_bmbt_irec	*imap,
> +	bool			*shared)
>  {
> -	struct xfs_bmbt_irec	got, prev, imap;
> -	xfs_fileoff_t		orig_end_fsb;
> -	int			nimaps, eof = 0, error = 0;
> -	bool			shared = false, trimmed = false;
> +	struct xfs_bmbt_irec	got, prev;
> +	xfs_fileoff_t		end_fsb, orig_end_fsb;
> +	int			eof = 0, error = 0;
> +	bool			trimmed;
>  	xfs_extnum_t		idx;
>  	xfs_extlen_t		align;
>  
> -	/* Already reserved?  Skip the refcount btree access. */
> -	xfs_bmap_search_extents(ip, *offset_fsb, XFS_COW_FORK, &eof, &idx,
> +	/*
> +	 * Search the COW fork extent list first.  This serves two purposes:
> +	 * first this implement the speculative preallocation using cowextisze,
> +	 * so that we also unshared block adjacent to shared blocks instead
> +	 * of just the shared blocks themselves.  Second the lookup in the
> +	 * extent list is generally faster than going out to the shared extent
> +	 * tree.
> +	 */
> +	xfs_bmap_search_extents(ip, imap->br_startoff, XFS_COW_FORK, &eof, &idx,
>  			&got, &prev);
> -	if (!eof && got.br_startoff <= *offset_fsb) {
> -		end_fsb = orig_end_fsb = got.br_startoff + got.br_blockcount;
> -		trace_xfs_reflink_cow_found(ip, &got);
> -		goto done;
> -	}
> +	if (!eof && got.br_startoff <= imap->br_startoff) {
> +		trace_xfs_reflink_cow_found(ip, imap);
> +		xfs_trim_extent(imap, got.br_startoff, got.br_blockcount);
>  
> -	/* Read extent from the source file. */
> -	nimaps = 1;
> -	error = xfs_bmapi_read(ip, *offset_fsb, end_fsb - *offset_fsb,
> -			&imap, &nimaps, 0);
> -	if (error)
> -		goto out_unlock;
> -	ASSERT(nimaps == 1);
> +		*shared = true;
> +		return 0;
> +	}
>  
>  	/* Trim the mapping to the nearest shared extent boundary. */
> -	error = xfs_reflink_trim_around_shared(ip, &imap, &shared, &trimmed);
> +	error = xfs_reflink_trim_around_shared(ip, imap, shared, &trimmed);
>  	if (error)
> -		goto out_unlock;
> -
> -	end_fsb = orig_end_fsb = imap.br_startoff + imap.br_blockcount;
> +		return error;
>  
>  	/* Not shared?  Just report the (potentially capped) extent. */
> -	if (!shared) {
> -		*skipped = true;
> -		goto done;
> -	}
> +	if (!*shared)
> +		return 0;
>  
>  	/*
>  	 * Fork all the shared blocks from our write offset until the end of
> @@ -279,73 +283,40 @@ __xfs_reflink_reserve_cow(
>  	 */
>  	error = xfs_qm_dqattach_locked(ip, 0);
>  	if (error)
> -		goto out_unlock;
> +		return error;
> +
> +	end_fsb = orig_end_fsb = imap->br_startoff + imap->br_blockcount;
>  
>  	align = xfs_eof_alignment(ip, xfs_get_cowextsz_hint(ip));
>  	if (align)
>  		end_fsb = roundup_64(end_fsb, align);
>  
>  retry:
> -	error = xfs_bmapi_reserve_delalloc(ip, XFS_COW_FORK, *offset_fsb,
> -			end_fsb - *offset_fsb, &got,
> -			&prev, &idx, eof);
> +	error = xfs_bmapi_reserve_delalloc(ip, XFS_COW_FORK, imap->br_startoff,
> +			end_fsb - imap->br_startoff, &got, &prev, &idx, eof);
>  	switch (error) {
>  	case 0:
>  		break;
>  	case -ENOSPC:
>  	case -EDQUOT:
>  		/* retry without any preallocation */
> -		trace_xfs_reflink_cow_enospc(ip, &imap);
> +		trace_xfs_reflink_cow_enospc(ip, imap);
>  		if (end_fsb != orig_end_fsb) {
>  			end_fsb = orig_end_fsb;
>  			goto retry;
>  		}
>  		/*FALLTHRU*/
>  	default:
> -		goto out_unlock;
> +		return error;
>  	}
>  
>  	if (end_fsb != orig_end_fsb)
>  		xfs_inode_set_cowblocks_tag(ip);
>  
>  	trace_xfs_reflink_cow_alloc(ip, &got);
> -done:
> -	*offset_fsb = end_fsb;
> -out_unlock:
> -	return error;
> +	return 0;
>  }
>  
> -/* Create a CoW reservation for part of a file. */
> -int
> -xfs_reflink_reserve_cow_range(
> -	struct xfs_inode	*ip,
> -	xfs_off_t		offset,
> -	xfs_off_t		count)
> -{
> -	struct xfs_mount	*mp = ip->i_mount;
> -	xfs_fileoff_t		offset_fsb, end_fsb;
> -	bool			skipped = false;
> -	int			error;
> -
> -	trace_xfs_reflink_reserve_cow_range(ip, offset, count);
> -
> -	offset_fsb = XFS_B_TO_FSBT(mp, offset);
> -	end_fsb = XFS_B_TO_FSB(mp, offset + count);
> -
> -	xfs_ilock(ip, XFS_ILOCK_EXCL);
> -	while (offset_fsb < end_fsb) {
> -		error = __xfs_reflink_reserve_cow(ip, &offset_fsb, end_fsb,
> -				&skipped);
> -		if (error) {
> -			trace_xfs_reflink_reserve_cow_range_error(ip, error,
> -				_RET_IP_);
> -			break;
> -		}
> -	}
> -	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> -
> -	return error;
> -}
>  
>  /* Allocate all CoW reservations covering a range of blocks in a file. */
>  static int
> @@ -359,9 +330,8 @@ __xfs_reflink_allocate_cow(
>  	struct xfs_defer_ops	dfops;
>  	struct xfs_trans	*tp;
>  	xfs_fsblock_t		first_block;
> -	xfs_fileoff_t		next_fsb;
>  	int			nimaps = 1, error;
> -	bool			skipped = false;
> +	bool			shared;
>  
>  	xfs_defer_init(&dfops, &first_block);
>  
> @@ -372,33 +342,38 @@ __xfs_reflink_allocate_cow(
>  
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  
> -	next_fsb = *offset_fsb;
> -	error = __xfs_reflink_reserve_cow(ip, &next_fsb, end_fsb, &skipped);
> +	/* Read extent from the source file. */
> +	nimaps = 1;
> +	error = xfs_bmapi_read(ip, *offset_fsb, end_fsb - *offset_fsb,
> +			&imap, &nimaps, 0);
> +	if (error)
> +		goto out_unlock;
> +	ASSERT(nimaps == 1);
> +
> +	error = xfs_reflink_reserve_cow(ip, &imap, &shared);
>  	if (error)
>  		goto out_trans_cancel;
>  
> -	if (skipped) {
> -		*offset_fsb = next_fsb;
> +	if (!shared) {
> +		*offset_fsb = imap.br_startoff + imap.br_blockcount;
>  		goto out_trans_cancel;
>  	}
>  
>  	xfs_trans_ijoin(tp, ip, 0);
> -	error = xfs_bmapi_write(tp, ip, *offset_fsb, next_fsb - *offset_fsb,
> +	error = xfs_bmapi_write(tp, ip, imap.br_startoff, imap.br_blockcount,
>  			XFS_BMAPI_COWFORK, &first_block,
>  			XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK),
>  			&imap, &nimaps, &dfops);
>  	if (error)
>  		goto out_trans_cancel;
>  
> -	/* We might not have been able to map the whole delalloc extent */
> -	*offset_fsb = min(*offset_fsb + imap.br_blockcount, next_fsb);
> -
>  	error = xfs_defer_finish(&tp, &dfops, NULL);
>  	if (error)
>  		goto out_trans_cancel;
>  
>  	error = xfs_trans_commit(tp);
>  
> +	*offset_fsb = imap.br_startoff + imap.br_blockcount;
>  out_unlock:
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  	return error;
> diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
> index 5dc3c8a..9f76924 100644
> --- a/fs/xfs/xfs_reflink.h
> +++ b/fs/xfs/xfs_reflink.h
> @@ -26,8 +26,8 @@ extern int xfs_reflink_find_shared(struct xfs_mount *mp, xfs_agnumber_t agno,
>  extern int xfs_reflink_trim_around_shared(struct xfs_inode *ip,
>  		struct xfs_bmbt_irec *irec, bool *shared, bool *trimmed);
>  
> -extern int xfs_reflink_reserve_cow_range(struct xfs_inode *ip,
> -		xfs_off_t offset, xfs_off_t count);
> +extern int xfs_reflink_reserve_cow(struct xfs_inode *ip,
> +		struct xfs_bmbt_irec *imap, bool *shared);
>  extern int xfs_reflink_allocate_cow_range(struct xfs_inode *ip,
>  		xfs_off_t offset, xfs_off_t count);
>  extern bool xfs_reflink_find_cow_mapping(struct xfs_inode *ip, xfs_off_t offset,
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 2586c9c..520660d 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -3347,7 +3347,7 @@ DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_alloc);
>  DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_found);
>  DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_enospc);
>  
> -DEFINE_RW_EVENT(xfs_reflink_reserve_cow_range);
> +DEFINE_RW_EVENT(xfs_reflink_reserve_cow);
>  DEFINE_RW_EVENT(xfs_reflink_allocate_cow_range);
>  
>  DEFINE_INODE_IREC_EVENT(xfs_reflink_bounce_dio_write);
> @@ -3359,7 +3359,6 @@ DEFINE_SIMPLE_IO_EVENT(xfs_reflink_end_cow);
>  DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_remap);
>  DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_remap_piece);
>  
> -DEFINE_INODE_ERROR_EVENT(xfs_reflink_reserve_cow_range_error);
>  DEFINE_INODE_ERROR_EVENT(xfs_reflink_allocate_cow_range_error);
>  DEFINE_INODE_ERROR_EVENT(xfs_reflink_cancel_cow_range_error);
>  DEFINE_INODE_ERROR_EVENT(xfs_reflink_end_cow_error);
> -- 
> 2.10.1.382.ga23ca1b
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/9] xfs: optimize writes to reflink files

[Christoph Hellwig]

On Wed, Oct 12, 2016 at 10:12:34AM -0400, Brian Foster wrote:
> > +		if (xfs_is_reflink_inode(ip)) {
> > +			bool		shared;
> > +
> > +			end_fsb = min(XFS_B_TO_FSB(mp, offset + count),
> > +					maxbytes_fsb);
> > +			xfs_trim_extent(&got, offset_fsb, end_fsb - offset_fsb);
> > +			error = xfs_reflink_reserve_cow(ip, &got, &shared);
> > +			if (error)
> > +				goto out_unlock;
> 
> All in all this seems fine, but I don't see why we need to get all the
> way down through xfs_reflink_reserve_cow() ->
> xfs_reflink_trim_around_shared() to handle the basic delalloc overwrite
> case on a reflink inode. Could we enhance the is_reflink_inode() helper
> or create a new one that can consider whether the data fork extent is a
> hole or delalloc?

Do you mean delalloc non-overwrite?  We could skip non-overwrite extents
by factoring out a helper that checks for extent types that don't need to
be overwritten.  But this would defeat the COW fork speculative
preallocation logic, which causes additional COW operations even for
extents we would not nessecarily have to COW.  So we'll always have to
look at the COW fork first if we already have an allocation to implement
that scheme (and we should probably document it better).

xfs_reflink_trim_around_shared does a check for the non-COWable extent
types as the very first thing, so that's where we are done with the COW
overhead for a non-overwrite that doesn't have a speculative
preallocation in the COW fork.

Re: [PATCH 5/9] xfs: optimize writes to reflink files

[Brian Foster]

On Thu, Oct 13, 2016 at 08:49:25AM +0200, Christoph Hellwig wrote:
> On Wed, Oct 12, 2016 at 10:12:34AM -0400, Brian Foster wrote:
> > > +		if (xfs_is_reflink_inode(ip)) {
> > > +			bool		shared;
> > > +
> > > +			end_fsb = min(XFS_B_TO_FSB(mp, offset + count),
> > > +					maxbytes_fsb);
> > > +			xfs_trim_extent(&got, offset_fsb, end_fsb - offset_fsb);
> > > +			error = xfs_reflink_reserve_cow(ip, &got, &shared);
> > > +			if (error)
> > > +				goto out_unlock;
> > 
> > All in all this seems fine, but I don't see why we need to get all the
> > way down through xfs_reflink_reserve_cow() ->
> > xfs_reflink_trim_around_shared() to handle the basic delalloc overwrite
> > case on a reflink inode. Could we enhance the is_reflink_inode() helper
> > or create a new one that can consider whether the data fork extent is a
> > hole or delalloc?
> 
> Do you mean delalloc non-overwrite?  We could skip non-overwrite extents
> by factoring out a helper that checks for extent types that don't need to
> be overwritten.  But this would defeat the COW fork speculative
> preallocation logic, which causes additional COW operations even for
> extents we would not nessecarily have to COW.  So we'll always have to
> look at the COW fork first if we already have an allocation to implement
> that scheme (and we should probably document it better).
> 

Either way... delalloc into a hole or overwrite of an existing (data
fork) delalloc, will fall out of xfs_reflink_reserve_cow() so long as
nothing is in the cow fork, right?

But regardless, I see your point now. For whatever reason the comment
update in xfs_reflink_reserve_cow() went right over my head. IIUC, the
idea is that cow delalloc writes can include preallocation and thus have
delalloc for blocks that might not actually be shared in the data fork.
Therefore, we have to query the cow fork first and cannot reliably use
the data fork shared state to determine whether cow fork blocks actually
exist. A clarification of the comment is probably fine.. thanks for the
explanation.

Brian

> xfs_reflink_trim_around_shared does a check for the non-COWable extent
> types as the very first thing, so that's where we are done with the COW
> overhead for a non-overwrite that doesn't have a speculative
> preallocation in the COW fork.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/9] xfs: optimize writes to reflink files

[Darrick J. Wong]

On Thu, Oct 13, 2016 at 09:26:05AM -0400, Brian Foster wrote:
> On Thu, Oct 13, 2016 at 08:49:25AM +0200, Christoph Hellwig wrote:
> > On Wed, Oct 12, 2016 at 10:12:34AM -0400, Brian Foster wrote:
> > > > +		if (xfs_is_reflink_inode(ip)) {
> > > > +			bool		shared;
> > > > +
> > > > +			end_fsb = min(XFS_B_TO_FSB(mp, offset + count),
> > > > +					maxbytes_fsb);
> > > > +			xfs_trim_extent(&got, offset_fsb, end_fsb - offset_fsb);
> > > > +			error = xfs_reflink_reserve_cow(ip, &got, &shared);
> > > > +			if (error)
> > > > +				goto out_unlock;
> > > 
> > > All in all this seems fine, but I don't see why we need to get all the
> > > way down through xfs_reflink_reserve_cow() ->
> > > xfs_reflink_trim_around_shared() to handle the basic delalloc overwrite
> > > case on a reflink inode. Could we enhance the is_reflink_inode() helper
> > > or create a new one that can consider whether the data fork extent is a
> > > hole or delalloc?
> > 
> > Do you mean delalloc non-overwrite?  We could skip non-overwrite extents
> > by factoring out a helper that checks for extent types that don't need to
> > be overwritten.  But this would defeat the COW fork speculative
> > preallocation logic, which causes additional COW operations even for
> > extents we would not nessecarily have to COW.  So we'll always have to
> > look at the COW fork first if we already have an allocation to implement
> > that scheme (and we should probably document it better).
> > 
> 
> Either way... delalloc into a hole or overwrite of an existing (data
> fork) delalloc, will fall out of xfs_reflink_reserve_cow() so long as
> nothing is in the cow fork, right?
> 
> But regardless, I see your point now. For whatever reason the comment
> update in xfs_reflink_reserve_cow() went right over my head. IIUC, the
> idea is that cow delalloc writes can include preallocation and thus have
> delalloc for blocks that might not actually be shared in the data fork.
> Therefore, we have to query the cow fork first and cannot reliably use
> the data fork shared state to determine whether cow fork blocks actually
> exist. A clarification of the comment is probably fine.. thanks for the
> explanation.

Yep, that's correct.  We can promote non-CoW writes to CoW as a strategy
to try to reduce fragmentation.  I'll clarify that aspect in the docs.

--D

> 
> Brian
> 
> > xfs_reflink_trim_around_shared does a check for the non-COWable extent
> > types as the very first thing, so that's where we are done with the COW
> > overhead for a non-overwrite that doesn't have a speculative
> > preallocation in the COW fork.
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/9] xfs: refactor xfs_bunmapi_cow

[Christoph Hellwig]

Split out two helpers for deleting delayed or real extents from the COW fork.
This allows to call them directly from xfs_reflink_cow_end_io once that
function is refactored to iterate the extent tree.  It will also allow
to reuse the delalloc deletion from xfs_bunmapi in the future.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c | 374 ++++++++++++++++++++++++++++-------------------
 fs/xfs/libxfs/xfs_bmap.h |   5 +
 2 files changed, 225 insertions(+), 154 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 016dacc..814980d 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4862,6 +4862,219 @@ xfs_bmap_split_indlen(
 	return stolen;
 }
 
+int
+xfs_bmap_del_extent_delay(
+	struct xfs_inode	*ip,
+	int			whichfork,
+	xfs_extnum_t		*idx,
+	struct xfs_bmbt_irec	*got,
+	struct xfs_bmbt_irec	*del)
+{
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
+	struct xfs_bmbt_irec	new;
+	int64_t			da_old, da_new, da_diff = 0;
+	xfs_fileoff_t		del_endoff, got_endoff;
+	xfs_filblks_t		got_indlen, new_indlen, stolen;
+	int			error = 0, state = 0;
+	bool			isrt;
+
+	XFS_STATS_INC(mp, xs_del_exlist);
+
+	isrt = (whichfork == XFS_DATA_FORK) && XFS_IS_REALTIME_INODE(ip);
+	del_endoff = del->br_startoff + del->br_blockcount;
+	got_endoff = got->br_startoff + got->br_blockcount;
+	da_old = startblockval(got->br_startblock);
+	da_new = 0;
+
+	ASSERT(*idx >= 0);
+	ASSERT(*idx < ifp->if_bytes / sizeof(struct xfs_bmbt_rec));
+	ASSERT(del->br_blockcount > 0);
+	ASSERT(got->br_startoff <= del->br_startoff);
+	ASSERT(got_endoff >= del_endoff);
+
+	if (isrt) {
+		int64_t rtexts = XFS_FSB_TO_B(mp, del->br_blockcount);
+
+		do_div(rtexts, mp->m_sb.sb_rextsize);
+		xfs_mod_frextents(mp, rtexts);
+	}
+
+	/*
+	 * Update the inode delalloc counter now and wait to update the
+	 * sb counters as we might have to borrow some blocks for the
+	 * indirect block accounting.
+	 */
+	xfs_trans_reserve_quota_nblks(NULL, ip, -((long)del->br_blockcount), 0,
+			isrt ? XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS);
+	ip->i_delayed_blks -= del->br_blockcount;
+
+	if (whichfork == XFS_COW_FORK)
+		state |= BMAP_COWFORK;
+
+	if (got->br_startoff == del->br_startoff)
+		state |= BMAP_LEFT_CONTIG;
+	if (got_endoff == del_endoff)
+		state |= BMAP_RIGHT_CONTIG;
+
+	switch (state & (BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG)) {
+	case BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG:
+		/*
+		 * Matches the whole extent.  Delete the entry.
+		 */
+		xfs_iext_remove(ip, *idx, 1, state);
+		--*idx;
+		break;
+	case BMAP_LEFT_CONTIG:
+		/*
+		 * Deleting the first part of the extent.
+		 */
+		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
+		got->br_startoff = del_endoff;
+		got->br_blockcount -= del->br_blockcount;
+		da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip,
+				got->br_blockcount), da_old);
+		got->br_startblock = nullstartblock((int)da_new);
+		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
+		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
+		break;
+	case BMAP_RIGHT_CONTIG:
+		/*
+		 * Deleting the last part of the extent.
+		 */
+		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
+		got->br_blockcount = got->br_blockcount - del->br_blockcount;
+		da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip,
+				got->br_blockcount), da_old);
+		got->br_startblock = nullstartblock((int)da_new);
+		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
+		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
+		break;
+	case 0:
+		/*
+		 * Deleting the middle of the extent.
+		 *
+		 * Distribute the original indlen reservation across the two
+		 * new extents.  Steal blocks from the deleted extent if
+		 * necessary. Stealing blocks simply fudges the fdblocks
+		 * accounting in xfs_bunmapi().
+		 */
+		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
+		got->br_blockcount = del->br_startoff - got->br_startoff;
+
+		got_indlen = xfs_bmap_worst_indlen(ip, got->br_blockcount);
+		new_indlen = xfs_bmap_worst_indlen(ip, new.br_blockcount);
+		stolen = xfs_bmap_split_indlen(da_old, &got_indlen, &new_indlen,
+						       del->br_blockcount);
+		da_new = got_indlen + new_indlen - stolen;
+		del->br_blockcount -= stolen;
+
+		/*
+		 * Set the reservation for each extent. Warn if either
+		 * is zero as this can lead to delalloc problems.
+		 */
+		WARN_ON_ONCE(!got_indlen || !new_indlen);
+		got->br_startblock = nullstartblock((int)got_indlen);
+		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
+		trace_xfs_bmap_post_update(ip, *idx, 0, _THIS_IP_);
+
+		new.br_startoff = del_endoff;
+		new.br_blockcount = got_endoff - del_endoff;
+		new.br_state = got->br_state;
+		new.br_startblock = nullstartblock((int)new_indlen);
+
+		++*idx;
+		xfs_iext_insert(ip, *idx, 1, &new, state);
+		break;
+	}
+
+	ASSERT(da_old >= da_new);
+	da_diff = da_old - da_new;
+	if (!isrt)
+		da_diff += del->br_blockcount;
+	if (da_diff)
+		xfs_mod_fdblocks(mp, da_diff, false);
+	return error;
+}
+
+void
+xfs_bmap_del_extent_cow(
+	struct xfs_inode	*ip,
+	xfs_extnum_t		*idx,
+	struct xfs_bmbt_irec	*got,
+	struct xfs_bmbt_irec	*del)
+{
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
+	struct xfs_bmbt_irec	new;
+	xfs_fileoff_t		del_endoff, got_endoff;
+	int			state = BMAP_COWFORK;
+
+	XFS_STATS_INC(mp, xs_del_exlist);
+
+	del_endoff = del->br_startoff + del->br_blockcount;
+	got_endoff = got->br_startoff + got->br_blockcount;
+
+	ASSERT(*idx >= 0);
+	ASSERT(*idx < ifp->if_bytes / sizeof(struct xfs_bmbt_rec));
+	ASSERT(del->br_blockcount > 0);
+	ASSERT(got->br_startoff <= del->br_startoff);
+	ASSERT(got_endoff >= del_endoff);
+	ASSERT(!isnullstartblock(got->br_startblock));
+
+	if (got->br_startoff == del->br_startoff)
+		state |= BMAP_LEFT_CONTIG;
+	if (got_endoff == del_endoff)
+		state |= BMAP_RIGHT_CONTIG;
+
+	switch (state & (BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG)) {
+	case BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG:
+		/*
+		 * Matches the whole extent.  Delete the entry.
+		 */
+		xfs_iext_remove(ip, *idx, 1, state);
+		--*idx;
+		break;
+	case BMAP_LEFT_CONTIG:
+		/*
+		 * Deleting the first part of the extent.
+		 */
+		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
+		got->br_startoff = del_endoff;
+		got->br_blockcount -= del->br_blockcount;
+		got->br_startblock = del->br_startblock + del->br_blockcount;
+		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
+		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
+		break;
+	case BMAP_RIGHT_CONTIG:
+		/*
+		 * Deleting the last part of the extent.
+		 */
+		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
+		got->br_blockcount -= del->br_blockcount;
+		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
+		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
+		break;
+	case 0:
+		/*
+		 * Deleting the middle of the extent.
+		 */
+		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
+		got->br_blockcount = del->br_startoff - got->br_startoff;
+		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
+		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
+
+		new.br_startoff = del_endoff;
+		new.br_blockcount = got_endoff - del_endoff;
+		new.br_state = got->br_state;
+		new.br_startblock = del->br_startblock + del->br_blockcount;
+
+		++*idx;
+		xfs_iext_insert(ip, *idx, 1, &new, state);
+		break;
+	}
+}
+
 /*
  * Called by xfs_bmapi to update file extent records and the btree
  * after removing space (or undoing a delayed allocation).
@@ -5210,167 +5423,20 @@ xfs_bunmapi_cow(
 	struct xfs_inode		*ip,
 	struct xfs_bmbt_irec		*del)
 {
-	xfs_filblks_t			da_new;
-	xfs_filblks_t			da_old;
-	xfs_fsblock_t			del_endblock = 0;
-	xfs_fileoff_t			del_endoff;
-	int				delay;
 	struct xfs_bmbt_rec_host	*ep;
-	int				error;
 	struct xfs_bmbt_irec		got;
-	xfs_fileoff_t			got_endoff;
-	struct xfs_ifork		*ifp;
-	struct xfs_mount		*mp;
-	xfs_filblks_t			nblks;
 	struct xfs_bmbt_irec		new;
-	/* REFERENCED */
-	uint				qfield;
-	xfs_filblks_t			temp;
-	xfs_filblks_t			temp2;
-	int				state = BMAP_COWFORK;
 	int				eof;
 	xfs_extnum_t			eidx;
 
-	mp = ip->i_mount;
-	XFS_STATS_INC(mp, xs_del_exlist);
-
 	ep = xfs_bmap_search_extents(ip, del->br_startoff, XFS_COW_FORK, &eof,
-			&eidx, &got, &new);
-
-	ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK); ifp = ifp;
-	ASSERT((eidx >= 0) && (eidx < ifp->if_bytes /
-		(uint)sizeof(xfs_bmbt_rec_t)));
-	ASSERT(del->br_blockcount > 0);
-	ASSERT(got.br_startoff <= del->br_startoff);
-	del_endoff = del->br_startoff + del->br_blockcount;
-	got_endoff = got.br_startoff + got.br_blockcount;
-	ASSERT(got_endoff >= del_endoff);
-	delay = isnullstartblock(got.br_startblock);
-	ASSERT(isnullstartblock(del->br_startblock) == delay);
-	qfield = 0;
-	error = 0;
-	/*
-	 * If deleting a real allocation, must free up the disk space.
-	 */
-	if (!delay) {
-		nblks = del->br_blockcount;
-		qfield = XFS_TRANS_DQ_BCOUNT;
-		/*
-		 * Set up del_endblock and cur for later.
-		 */
-		del_endblock = del->br_startblock + del->br_blockcount;
-		da_old = da_new = 0;
-	} else {
-		da_old = startblockval(got.br_startblock);
-		da_new = 0;
-		nblks = 0;
-	}
-	qfield = qfield;
-	nblks = nblks;
-
-	/*
-	 * Set flag value to use in switch statement.
-	 * Left-contig is 2, right-contig is 1.
-	 */
-	switch (((got.br_startoff == del->br_startoff) << 1) |
-		(got_endoff == del_endoff)) {
-	case 3:
-		/*
-		 * Matches the whole extent.  Delete the entry.
-		 */
-		xfs_iext_remove(ip, eidx, 1, BMAP_COWFORK);
-		--eidx;
-		break;
-
-	case 2:
-		/*
-		 * Deleting the first part of the extent.
-		 */
-		trace_xfs_bmap_pre_update(ip, eidx, state, _THIS_IP_);
-		xfs_bmbt_set_startoff(ep, del_endoff);
-		temp = got.br_blockcount - del->br_blockcount;
-		xfs_bmbt_set_blockcount(ep, temp);
-		if (delay) {
-			temp = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip, temp),
-				da_old);
-			xfs_bmbt_set_startblock(ep, nullstartblock((int)temp));
-			trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
-			da_new = temp;
-			break;
-		}
-		xfs_bmbt_set_startblock(ep, del_endblock);
-		trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
-		break;
-
-	case 1:
-		/*
-		 * Deleting the last part of the extent.
-		 */
-		temp = got.br_blockcount - del->br_blockcount;
-		trace_xfs_bmap_pre_update(ip, eidx, state, _THIS_IP_);
-		xfs_bmbt_set_blockcount(ep, temp);
-		if (delay) {
-			temp = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip, temp),
-				da_old);
-			xfs_bmbt_set_startblock(ep, nullstartblock((int)temp));
-			trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
-			da_new = temp;
-			break;
-		}
-		trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
-		break;
-
-	case 0:
-		/*
-		 * Deleting the middle of the extent.
-		 */
-		temp = del->br_startoff - got.br_startoff;
-		trace_xfs_bmap_pre_update(ip, eidx, state, _THIS_IP_);
-		xfs_bmbt_set_blockcount(ep, temp);
-		new.br_startoff = del_endoff;
-		temp2 = got_endoff - del_endoff;
-		new.br_blockcount = temp2;
-		new.br_state = got.br_state;
-		if (!delay) {
-			new.br_startblock = del_endblock;
-		} else {
-			temp = xfs_bmap_worst_indlen(ip, temp);
-			xfs_bmbt_set_startblock(ep, nullstartblock((int)temp));
-			temp2 = xfs_bmap_worst_indlen(ip, temp2);
-			new.br_startblock = nullstartblock((int)temp2);
-			da_new = temp + temp2;
-			while (da_new > da_old) {
-				if (temp) {
-					temp--;
-					da_new--;
-					xfs_bmbt_set_startblock(ep,
-						nullstartblock((int)temp));
-				}
-				if (da_new == da_old)
-					break;
-				if (temp2) {
-					temp2--;
-					da_new--;
-					new.br_startblock =
-						nullstartblock((int)temp2);
-				}
-			}
-		}
-		trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
-		xfs_iext_insert(ip, eidx + 1, 1, &new, state);
-		++eidx;
-		break;
-	}
-
-	/*
-	 * Account for change in delayed indirect blocks.
-	 * Nothing to do for disk quota accounting here.
-	 */
-	ASSERT(da_old >= da_new);
-	if (da_old > da_new)
-		xfs_mod_fdblocks(mp, (int64_t)(da_old - da_new), false);
-
-	return error;
+				&eidx, &got, &new);
+	ASSERT(ep);
+	if (isnullstartblock(got.br_startblock))
+		xfs_bmap_del_extent_delay(ip, XFS_COW_FORK, &eidx, &got, del);
+	else
+		xfs_bmap_del_extent_cow(ip, &eidx, &got, del);
+	return 0;
 }
 
 /*
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index eb86af0..5f18248 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -224,6 +224,11 @@ int	xfs_bunmapi(struct xfs_trans *tp, struct xfs_inode *ip,
 		xfs_extnum_t nexts, xfs_fsblock_t *firstblock,
 		struct xfs_defer_ops *dfops, int *done);
 int	xfs_bunmapi_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *del);
+int	xfs_bmap_del_extent_delay(struct xfs_inode *ip, int whichfork,
+		xfs_extnum_t *idx, struct xfs_bmbt_irec *got,
+		struct xfs_bmbt_irec *del);
+void	xfs_bmap_del_extent_cow(struct xfs_inode *ip, xfs_extnum_t *idx,
+		struct xfs_bmbt_irec *got, struct xfs_bmbt_irec *del);
 int	xfs_check_nostate_extents(struct xfs_ifork *ifp, xfs_extnum_t idx,
 		xfs_extnum_t num);
 uint	xfs_default_attroffset(struct xfs_inode *ip);
-- 
2.10.1.382.ga23ca1b

Re: [PATCH 6/9] xfs: refactor xfs_bunmapi_cow

[Brian Foster]

On Mon, Oct 10, 2016 at 03:38:02PM +0200, Christoph Hellwig wrote:
> Split out two helpers for deleting delayed or real extents from the COW fork.
> This allows to call them directly from xfs_reflink_cow_end_io once that
> function is refactored to iterate the extent tree.  It will also allow
> to reuse the delalloc deletion from xfs_bunmapi in the future.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 374 ++++++++++++++++++++++++++++-------------------
>  fs/xfs/libxfs/xfs_bmap.h |   5 +
>  2 files changed, 225 insertions(+), 154 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 016dacc..814980d 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4862,6 +4862,219 @@ xfs_bmap_split_indlen(
>  	return stolen;
>  }
>  
> +int
> +xfs_bmap_del_extent_delay(
> +	struct xfs_inode	*ip,
> +	int			whichfork,
> +	xfs_extnum_t		*idx,
> +	struct xfs_bmbt_irec	*got,
> +	struct xfs_bmbt_irec	*del)
> +{
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
> +	struct xfs_bmbt_irec	new;
> +	int64_t			da_old, da_new, da_diff = 0;
> +	xfs_fileoff_t		del_endoff, got_endoff;
> +	xfs_filblks_t		got_indlen, new_indlen, stolen;
> +	int			error = 0, state = 0;
> +	bool			isrt;
> +
> +	XFS_STATS_INC(mp, xs_del_exlist);
> +
> +	isrt = (whichfork == XFS_DATA_FORK) && XFS_IS_REALTIME_INODE(ip);
> +	del_endoff = del->br_startoff + del->br_blockcount;
> +	got_endoff = got->br_startoff + got->br_blockcount;
> +	da_old = startblockval(got->br_startblock);
> +	da_new = 0;
> +
> +	ASSERT(*idx >= 0);
> +	ASSERT(*idx < ifp->if_bytes / sizeof(struct xfs_bmbt_rec));
> +	ASSERT(del->br_blockcount > 0);
> +	ASSERT(got->br_startoff <= del->br_startoff);
> +	ASSERT(got_endoff >= del_endoff);
> +
> +	if (isrt) {
> +		int64_t rtexts = XFS_FSB_TO_B(mp, del->br_blockcount);
> +
> +		do_div(rtexts, mp->m_sb.sb_rextsize);
> +		xfs_mod_frextents(mp, rtexts);
> +	}
> +
> +	/*
> +	 * Update the inode delalloc counter now and wait to update the
> +	 * sb counters as we might have to borrow some blocks for the
> +	 * indirect block accounting.
> +	 */
> +	xfs_trans_reserve_quota_nblks(NULL, ip, -((long)del->br_blockcount), 0,
> +			isrt ? XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS);
> +	ip->i_delayed_blks -= del->br_blockcount;
> +

This appears to be fixed up later, but i_delayed_blks is accounted twice
as of this patch. It would be nice if we could avoid known breakage,
even if transient.

> +	if (whichfork == XFS_COW_FORK)
> +		state |= BMAP_COWFORK;
> +
> +	if (got->br_startoff == del->br_startoff)
> +		state |= BMAP_LEFT_CONTIG;
> +	if (got_endoff == del_endoff)
> +		state |= BMAP_RIGHT_CONTIG;
> +
> +	switch (state & (BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG)) {
> +	case BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG:
> +		/*
> +		 * Matches the whole extent.  Delete the entry.
> +		 */
> +		xfs_iext_remove(ip, *idx, 1, state);
> +		--*idx;
> +		break;
> +	case BMAP_LEFT_CONTIG:
> +		/*
> +		 * Deleting the first part of the extent.
> +		 */
> +		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> +		got->br_startoff = del_endoff;
> +		got->br_blockcount -= del->br_blockcount;
> +		da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip,
> +				got->br_blockcount), da_old);
> +		got->br_startblock = nullstartblock((int)da_new);
> +		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> +		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
> +		break;
> +	case BMAP_RIGHT_CONTIG:
> +		/*
> +		 * Deleting the last part of the extent.
> +		 */
> +		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> +		got->br_blockcount = got->br_blockcount - del->br_blockcount;
> +		da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip,
> +				got->br_blockcount), da_old);
> +		got->br_startblock = nullstartblock((int)da_new);
> +		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> +		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
> +		break;
> +	case 0:
> +		/*
> +		 * Deleting the middle of the extent.
> +		 *
> +		 * Distribute the original indlen reservation across the two
> +		 * new extents.  Steal blocks from the deleted extent if
> +		 * necessary. Stealing blocks simply fudges the fdblocks
> +		 * accounting in xfs_bunmapi().
> +		 */
> +		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> +		got->br_blockcount = del->br_startoff - got->br_startoff;
> +
> +		got_indlen = xfs_bmap_worst_indlen(ip, got->br_blockcount);
> +		new_indlen = xfs_bmap_worst_indlen(ip, new.br_blockcount);

Doesn't look like new.br_blockcount is set until a few lines below.

Brian

> +		stolen = xfs_bmap_split_indlen(da_old, &got_indlen, &new_indlen,
> +						       del->br_blockcount);
> +		da_new = got_indlen + new_indlen - stolen;
> +		del->br_blockcount -= stolen;
> +
> +		/*
> +		 * Set the reservation for each extent. Warn if either
> +		 * is zero as this can lead to delalloc problems.
> +		 */
> +		WARN_ON_ONCE(!got_indlen || !new_indlen);
> +		got->br_startblock = nullstartblock((int)got_indlen);
> +		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> +		trace_xfs_bmap_post_update(ip, *idx, 0, _THIS_IP_);
> +
> +		new.br_startoff = del_endoff;
> +		new.br_blockcount = got_endoff - del_endoff;
> +		new.br_state = got->br_state;
> +		new.br_startblock = nullstartblock((int)new_indlen);
> +
> +		++*idx;
> +		xfs_iext_insert(ip, *idx, 1, &new, state);
> +		break;
> +	}
> +
> +	ASSERT(da_old >= da_new);
> +	da_diff = da_old - da_new;
> +	if (!isrt)
> +		da_diff += del->br_blockcount;
> +	if (da_diff)
> +		xfs_mod_fdblocks(mp, da_diff, false);
> +	return error;
> +}
> +
> +void
> +xfs_bmap_del_extent_cow(
> +	struct xfs_inode	*ip,
> +	xfs_extnum_t		*idx,
> +	struct xfs_bmbt_irec	*got,
> +	struct xfs_bmbt_irec	*del)
> +{
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
> +	struct xfs_bmbt_irec	new;
> +	xfs_fileoff_t		del_endoff, got_endoff;
> +	int			state = BMAP_COWFORK;
> +
> +	XFS_STATS_INC(mp, xs_del_exlist);
> +
> +	del_endoff = del->br_startoff + del->br_blockcount;
> +	got_endoff = got->br_startoff + got->br_blockcount;
> +
> +	ASSERT(*idx >= 0);
> +	ASSERT(*idx < ifp->if_bytes / sizeof(struct xfs_bmbt_rec));
> +	ASSERT(del->br_blockcount > 0);
> +	ASSERT(got->br_startoff <= del->br_startoff);
> +	ASSERT(got_endoff >= del_endoff);
> +	ASSERT(!isnullstartblock(got->br_startblock));
> +
> +	if (got->br_startoff == del->br_startoff)
> +		state |= BMAP_LEFT_CONTIG;
> +	if (got_endoff == del_endoff)
> +		state |= BMAP_RIGHT_CONTIG;
> +
> +	switch (state & (BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG)) {
> +	case BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG:
> +		/*
> +		 * Matches the whole extent.  Delete the entry.
> +		 */
> +		xfs_iext_remove(ip, *idx, 1, state);
> +		--*idx;
> +		break;
> +	case BMAP_LEFT_CONTIG:
> +		/*
> +		 * Deleting the first part of the extent.
> +		 */
> +		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> +		got->br_startoff = del_endoff;
> +		got->br_blockcount -= del->br_blockcount;
> +		got->br_startblock = del->br_startblock + del->br_blockcount;
> +		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> +		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
> +		break;
> +	case BMAP_RIGHT_CONTIG:
> +		/*
> +		 * Deleting the last part of the extent.
> +		 */
> +		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> +		got->br_blockcount -= del->br_blockcount;
> +		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> +		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
> +		break;
> +	case 0:
> +		/*
> +		 * Deleting the middle of the extent.
> +		 */
> +		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> +		got->br_blockcount = del->br_startoff - got->br_startoff;
> +		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> +		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
> +
> +		new.br_startoff = del_endoff;
> +		new.br_blockcount = got_endoff - del_endoff;
> +		new.br_state = got->br_state;
> +		new.br_startblock = del->br_startblock + del->br_blockcount;
> +
> +		++*idx;
> +		xfs_iext_insert(ip, *idx, 1, &new, state);
> +		break;
> +	}
> +}
> +
>  /*
>   * Called by xfs_bmapi to update file extent records and the btree
>   * after removing space (or undoing a delayed allocation).
> @@ -5210,167 +5423,20 @@ xfs_bunmapi_cow(
>  	struct xfs_inode		*ip,
>  	struct xfs_bmbt_irec		*del)
>  {
> -	xfs_filblks_t			da_new;
> -	xfs_filblks_t			da_old;
> -	xfs_fsblock_t			del_endblock = 0;
> -	xfs_fileoff_t			del_endoff;
> -	int				delay;
>  	struct xfs_bmbt_rec_host	*ep;
> -	int				error;
>  	struct xfs_bmbt_irec		got;
> -	xfs_fileoff_t			got_endoff;
> -	struct xfs_ifork		*ifp;
> -	struct xfs_mount		*mp;
> -	xfs_filblks_t			nblks;
>  	struct xfs_bmbt_irec		new;
> -	/* REFERENCED */
> -	uint				qfield;
> -	xfs_filblks_t			temp;
> -	xfs_filblks_t			temp2;
> -	int				state = BMAP_COWFORK;
>  	int				eof;
>  	xfs_extnum_t			eidx;
>  
> -	mp = ip->i_mount;
> -	XFS_STATS_INC(mp, xs_del_exlist);
> -
>  	ep = xfs_bmap_search_extents(ip, del->br_startoff, XFS_COW_FORK, &eof,
> -			&eidx, &got, &new);
> -
> -	ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK); ifp = ifp;
> -	ASSERT((eidx >= 0) && (eidx < ifp->if_bytes /
> -		(uint)sizeof(xfs_bmbt_rec_t)));
> -	ASSERT(del->br_blockcount > 0);
> -	ASSERT(got.br_startoff <= del->br_startoff);
> -	del_endoff = del->br_startoff + del->br_blockcount;
> -	got_endoff = got.br_startoff + got.br_blockcount;
> -	ASSERT(got_endoff >= del_endoff);
> -	delay = isnullstartblock(got.br_startblock);
> -	ASSERT(isnullstartblock(del->br_startblock) == delay);
> -	qfield = 0;
> -	error = 0;
> -	/*
> -	 * If deleting a real allocation, must free up the disk space.
> -	 */
> -	if (!delay) {
> -		nblks = del->br_blockcount;
> -		qfield = XFS_TRANS_DQ_BCOUNT;
> -		/*
> -		 * Set up del_endblock and cur for later.
> -		 */
> -		del_endblock = del->br_startblock + del->br_blockcount;
> -		da_old = da_new = 0;
> -	} else {
> -		da_old = startblockval(got.br_startblock);
> -		da_new = 0;
> -		nblks = 0;
> -	}
> -	qfield = qfield;
> -	nblks = nblks;
> -
> -	/*
> -	 * Set flag value to use in switch statement.
> -	 * Left-contig is 2, right-contig is 1.
> -	 */
> -	switch (((got.br_startoff == del->br_startoff) << 1) |
> -		(got_endoff == del_endoff)) {
> -	case 3:
> -		/*
> -		 * Matches the whole extent.  Delete the entry.
> -		 */
> -		xfs_iext_remove(ip, eidx, 1, BMAP_COWFORK);
> -		--eidx;
> -		break;
> -
> -	case 2:
> -		/*
> -		 * Deleting the first part of the extent.
> -		 */
> -		trace_xfs_bmap_pre_update(ip, eidx, state, _THIS_IP_);
> -		xfs_bmbt_set_startoff(ep, del_endoff);
> -		temp = got.br_blockcount - del->br_blockcount;
> -		xfs_bmbt_set_blockcount(ep, temp);
> -		if (delay) {
> -			temp = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip, temp),
> -				da_old);
> -			xfs_bmbt_set_startblock(ep, nullstartblock((int)temp));
> -			trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
> -			da_new = temp;
> -			break;
> -		}
> -		xfs_bmbt_set_startblock(ep, del_endblock);
> -		trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
> -		break;
> -
> -	case 1:
> -		/*
> -		 * Deleting the last part of the extent.
> -		 */
> -		temp = got.br_blockcount - del->br_blockcount;
> -		trace_xfs_bmap_pre_update(ip, eidx, state, _THIS_IP_);
> -		xfs_bmbt_set_blockcount(ep, temp);
> -		if (delay) {
> -			temp = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip, temp),
> -				da_old);
> -			xfs_bmbt_set_startblock(ep, nullstartblock((int)temp));
> -			trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
> -			da_new = temp;
> -			break;
> -		}
> -		trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
> -		break;
> -
> -	case 0:
> -		/*
> -		 * Deleting the middle of the extent.
> -		 */
> -		temp = del->br_startoff - got.br_startoff;
> -		trace_xfs_bmap_pre_update(ip, eidx, state, _THIS_IP_);
> -		xfs_bmbt_set_blockcount(ep, temp);
> -		new.br_startoff = del_endoff;
> -		temp2 = got_endoff - del_endoff;
> -		new.br_blockcount = temp2;
> -		new.br_state = got.br_state;
> -		if (!delay) {
> -			new.br_startblock = del_endblock;
> -		} else {
> -			temp = xfs_bmap_worst_indlen(ip, temp);
> -			xfs_bmbt_set_startblock(ep, nullstartblock((int)temp));
> -			temp2 = xfs_bmap_worst_indlen(ip, temp2);
> -			new.br_startblock = nullstartblock((int)temp2);
> -			da_new = temp + temp2;
> -			while (da_new > da_old) {
> -				if (temp) {
> -					temp--;
> -					da_new--;
> -					xfs_bmbt_set_startblock(ep,
> -						nullstartblock((int)temp));
> -				}
> -				if (da_new == da_old)
> -					break;
> -				if (temp2) {
> -					temp2--;
> -					da_new--;
> -					new.br_startblock =
> -						nullstartblock((int)temp2);
> -				}
> -			}
> -		}
> -		trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
> -		xfs_iext_insert(ip, eidx + 1, 1, &new, state);
> -		++eidx;
> -		break;
> -	}
> -
> -	/*
> -	 * Account for change in delayed indirect blocks.
> -	 * Nothing to do for disk quota accounting here.
> -	 */
> -	ASSERT(da_old >= da_new);
> -	if (da_old > da_new)
> -		xfs_mod_fdblocks(mp, (int64_t)(da_old - da_new), false);
> -
> -	return error;
> +				&eidx, &got, &new);
> +	ASSERT(ep);
> +	if (isnullstartblock(got.br_startblock))
> +		xfs_bmap_del_extent_delay(ip, XFS_COW_FORK, &eidx, &got, del);
> +	else
> +		xfs_bmap_del_extent_cow(ip, &eidx, &got, del);
> +	return 0;
>  }
>  
>  /*
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index eb86af0..5f18248 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -224,6 +224,11 @@ int	xfs_bunmapi(struct xfs_trans *tp, struct xfs_inode *ip,
>  		xfs_extnum_t nexts, xfs_fsblock_t *firstblock,
>  		struct xfs_defer_ops *dfops, int *done);
>  int	xfs_bunmapi_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *del);
> +int	xfs_bmap_del_extent_delay(struct xfs_inode *ip, int whichfork,
> +		xfs_extnum_t *idx, struct xfs_bmbt_irec *got,
> +		struct xfs_bmbt_irec *del);
> +void	xfs_bmap_del_extent_cow(struct xfs_inode *ip, xfs_extnum_t *idx,
> +		struct xfs_bmbt_irec *got, struct xfs_bmbt_irec *del);
>  int	xfs_check_nostate_extents(struct xfs_ifork *ifp, xfs_extnum_t idx,
>  		xfs_extnum_t num);
>  uint	xfs_default_attroffset(struct xfs_inode *ip);
> -- 
> 2.10.1.382.ga23ca1b
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/9] xfs: refactor xfs_bunmapi_cow

[Christoph Hellwig]

On Wed, Oct 12, 2016 at 10:13:08AM -0400, Brian Foster wrote:
> > +	/*
> > +	 * Update the inode delalloc counter now and wait to update the
> > +	 * sb counters as we might have to borrow some blocks for the
> > +	 * indirect block accounting.
> > +	 */
> > +	xfs_trans_reserve_quota_nblks(NULL, ip, -((long)del->br_blockcount), 0,
> > +			isrt ? XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS);
> > +	ip->i_delayed_blks -= del->br_blockcount;
> > +
> 
> This appears to be fixed up later, but i_delayed_blks is accounted twice
> as of this patch. It would be nice if we could avoid known breakage,
> even if transient.

Where do we account for it the second time?

> > +	case 0:
> > +		/*
> > +		 * Deleting the middle of the extent.
> > +		 *
> > +		 * Distribute the original indlen reservation across the two
> > +		 * new extents.  Steal blocks from the deleted extent if
> > +		 * necessary. Stealing blocks simply fudges the fdblocks
> > +		 * accounting in xfs_bunmapi().
> > +		 */
> > +		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> > +		got->br_blockcount = del->br_startoff - got->br_startoff;
> > +
> > +		got_indlen = xfs_bmap_worst_indlen(ip, got->br_blockcount);
> > +		new_indlen = xfs_bmap_worst_indlen(ip, new.br_blockcount);
> 
> Doesn't look like new.br_blockcount is set until a few lines below.

Indeed.  Fixed for the next resend.

Re: [PATCH 6/9] xfs: refactor xfs_bunmapi_cow

[Brian Foster]

On Thu, Oct 13, 2016 at 08:54:32AM +0200, Christoph Hellwig wrote:
> On Wed, Oct 12, 2016 at 10:13:08AM -0400, Brian Foster wrote:
> > > +	/*
> > > +	 * Update the inode delalloc counter now and wait to update the
> > > +	 * sb counters as we might have to borrow some blocks for the
> > > +	 * indirect block accounting.
> > > +	 */
> > > +	xfs_trans_reserve_quota_nblks(NULL, ip, -((long)del->br_blockcount), 0,
> > > +			isrt ? XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS);
> > > +	ip->i_delayed_blks -= del->br_blockcount;
> > > +
> > 
> > This appears to be fixed up later, but i_delayed_blks is accounted twice
> > as of this patch. It would be nice if we could avoid known breakage,
> > even if transient.
> 
> Where do we account for it the second time?
> 

Note again that this is transient and fixed up in a later patch (so you
won't see it looking at the code with the full series applied). But as
of this patch the following is possible:

  ...
  xfs_reflink_cancel_cow_blocks()
    i_delayed_blks -= blockcount
    xfs_bunmapi_cow()
      xfs_bmap_del_extent_delay()
        i_delayed_blks -= blockcount

Brian

> > > +	case 0:
> > > +		/*
> > > +		 * Deleting the middle of the extent.
> > > +		 *
> > > +		 * Distribute the original indlen reservation across the two
> > > +		 * new extents.  Steal blocks from the deleted extent if
> > > +		 * necessary. Stealing blocks simply fudges the fdblocks
> > > +		 * accounting in xfs_bunmapi().
> > > +		 */
> > > +		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> > > +		got->br_blockcount = del->br_startoff - got->br_startoff;
> > > +
> > > +		got_indlen = xfs_bmap_worst_indlen(ip, got->br_blockcount);
> > > +		new_indlen = xfs_bmap_worst_indlen(ip, new.br_blockcount);
> > 
> > Doesn't look like new.br_blockcount is set until a few lines below.
> 
> Indeed.  Fixed for the next resend.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 7/9] xfs: optimize xfs_reflink_cancel_cow_blocks

[Christoph Hellwig]

Rewrite xfs_reflink_cancel_cow_blocks so that we only do a search for
the first extent in the extent list and then iterate over the remaining
extents using the extent index, passing the extent we operate on
directly to xfs_bmap_del_extent_delay or xfs_bmap_del_extent_cow instead
of going through xfs_bunmapi and doing yet another extent list lookup.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_reflink.c | 59 +++++++++++++++++++++++-----------------------------
 1 file changed, 26 insertions(+), 33 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 5d230ea..08170bb 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -512,58 +512,52 @@ xfs_reflink_cancel_cow_blocks(
 	xfs_fileoff_t			offset_fsb,
 	xfs_fileoff_t			end_fsb)
 {
-	struct xfs_bmbt_irec		irec;
-	xfs_filblks_t			count_fsb;
+	struct xfs_ifork		*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
+	struct xfs_bmbt_irec		got, prev, del;
+	xfs_extnum_t			idx;
 	xfs_fsblock_t			firstfsb;
 	struct xfs_defer_ops		dfops;
-	int				error = 0;
-	int				nimaps;
+	int				error = 0, eof = 0;
 
 	if (!xfs_is_reflink_inode(ip))
 		return 0;
 
-	/* Go find the old extent in the CoW fork. */
-	while (offset_fsb < end_fsb) {
-		nimaps = 1;
-		count_fsb = (xfs_filblks_t)(end_fsb - offset_fsb);
-		error = xfs_bmapi_read(ip, offset_fsb, count_fsb, &irec,
-				&nimaps, XFS_BMAPI_COWFORK);
-		if (error)
-			break;
-		ASSERT(nimaps == 1);
+	xfs_bmap_search_extents(ip, offset_fsb, XFS_COW_FORK, &eof, &idx,
+			&got, &prev);
+	if (eof)
+		return 0;
 
-		trace_xfs_reflink_cancel_cow(ip, &irec);
+	do {
+		if (got.br_startoff >= end_fsb)
+			break;
 
-		if (irec.br_startblock == DELAYSTARTBLOCK) {
-			/* Free a delayed allocation. */
-			xfs_mod_fdblocks(ip->i_mount, irec.br_blockcount,
-					false);
-			ip->i_delayed_blks -= irec.br_blockcount;
+		del = got;
+		xfs_trim_extent(&del, offset_fsb, end_fsb - offset_fsb);
+		trace_xfs_reflink_cancel_cow(ip, &del);
 
-			/* Remove the mapping from the CoW fork. */
-			error = xfs_bunmapi_cow(ip, &irec);
+		if (isnullstartblock(del.br_startblock)) {
+			error = xfs_bmap_del_extent_delay(ip, XFS_COW_FORK,
+					&idx, &got, &del);
 			if (error)
 				break;
-		} else if (irec.br_startblock == HOLESTARTBLOCK) {
-			/* empty */
 		} else {
 			xfs_trans_ijoin(*tpp, ip, 0);
 			xfs_defer_init(&dfops, &firstfsb);
 
 			/* Free the CoW orphan record. */
 			error = xfs_refcount_free_cow_extent(ip->i_mount,
-					&dfops, irec.br_startblock,
-					irec.br_blockcount);
+					&dfops, del.br_startblock,
+					del.br_blockcount);
 			if (error)
 				break;
 
 			xfs_bmap_add_free(ip->i_mount, &dfops,
-					irec.br_startblock, irec.br_blockcount,
+					del.br_startblock, del.br_blockcount,
 					NULL);
 
 			/* Update quota accounting */
 			xfs_trans_mod_dquot_byino(*tpp, ip, XFS_TRANS_DQ_BCOUNT,
-					-(long)irec.br_blockcount);
+					-(long)del.br_blockcount);
 
 			/* Roll the transaction */
 			error = xfs_defer_finish(tpp, &dfops, ip);
@@ -573,14 +567,13 @@ xfs_reflink_cancel_cow_blocks(
 			}
 
 			/* Remove the mapping from the CoW fork. */
-			error = xfs_bunmapi_cow(ip, &irec);
-			if (error)
-				break;
+			xfs_bmap_del_extent_cow(ip, &idx, &got, &del);
 		}
 
-		/* Roll on... */
-		offset_fsb = irec.br_startoff + irec.br_blockcount;
-	}
+		if (++idx >= ifp->if_bytes / sizeof(struct xfs_bmbt_rec))
+			return 0;
+		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
+	} while (1);
 
 	return error;
 }
-- 
2.10.1.382.ga23ca1b

Re: [PATCH 7/9] xfs: optimize xfs_reflink_cancel_cow_blocks

[Brian Foster]

On Mon, Oct 10, 2016 at 03:38:03PM +0200, Christoph Hellwig wrote:
> Rewrite xfs_reflink_cancel_cow_blocks so that we only do a search for
> the first extent in the extent list and then iterate over the remaining
> extents using the extent index, passing the extent we operate on
> directly to xfs_bmap_del_extent_delay or xfs_bmap_del_extent_cow instead
> of going through xfs_bunmapi and doing yet another extent list lookup.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_reflink.c | 59 +++++++++++++++++++++++-----------------------------
>  1 file changed, 26 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 5d230ea..08170bb 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -512,58 +512,52 @@ xfs_reflink_cancel_cow_blocks(
>  	xfs_fileoff_t			offset_fsb,
>  	xfs_fileoff_t			end_fsb)
>  {
> -	struct xfs_bmbt_irec		irec;
> -	xfs_filblks_t			count_fsb;
> +	struct xfs_ifork		*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
> +	struct xfs_bmbt_irec		got, prev, del;
> +	xfs_extnum_t			idx;
>  	xfs_fsblock_t			firstfsb;
>  	struct xfs_defer_ops		dfops;
> -	int				error = 0;
> -	int				nimaps;
> +	int				error = 0, eof = 0;
>  
>  	if (!xfs_is_reflink_inode(ip))
>  		return 0;
>  
> -	/* Go find the old extent in the CoW fork. */
> -	while (offset_fsb < end_fsb) {
> -		nimaps = 1;
> -		count_fsb = (xfs_filblks_t)(end_fsb - offset_fsb);
> -		error = xfs_bmapi_read(ip, offset_fsb, count_fsb, &irec,
> -				&nimaps, XFS_BMAPI_COWFORK);
> -		if (error)
> -			break;
> -		ASSERT(nimaps == 1);
> +	xfs_bmap_search_extents(ip, offset_fsb, XFS_COW_FORK, &eof, &idx,
> +			&got, &prev);
> +	if (eof)
> +		return 0;
>  
> -		trace_xfs_reflink_cancel_cow(ip, &irec);
> +	do {
> +		if (got.br_startoff >= end_fsb)
> +			break;

How about using a while loop..?

	while (got.br_startoff < end_fsb) {
		...

Otherwise, looks fine:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  
> -		if (irec.br_startblock == DELAYSTARTBLOCK) {
> -			/* Free a delayed allocation. */
> -			xfs_mod_fdblocks(ip->i_mount, irec.br_blockcount,
> -					false);
> -			ip->i_delayed_blks -= irec.br_blockcount;
> +		del = got;
> +		xfs_trim_extent(&del, offset_fsb, end_fsb - offset_fsb);
> +		trace_xfs_reflink_cancel_cow(ip, &del);
>  
> -			/* Remove the mapping from the CoW fork. */
> -			error = xfs_bunmapi_cow(ip, &irec);
> +		if (isnullstartblock(del.br_startblock)) {
> +			error = xfs_bmap_del_extent_delay(ip, XFS_COW_FORK,
> +					&idx, &got, &del);
>  			if (error)
>  				break;
> -		} else if (irec.br_startblock == HOLESTARTBLOCK) {
> -			/* empty */
>  		} else {
>  			xfs_trans_ijoin(*tpp, ip, 0);
>  			xfs_defer_init(&dfops, &firstfsb);
>  
>  			/* Free the CoW orphan record. */
>  			error = xfs_refcount_free_cow_extent(ip->i_mount,
> -					&dfops, irec.br_startblock,
> -					irec.br_blockcount);
> +					&dfops, del.br_startblock,
> +					del.br_blockcount);
>  			if (error)
>  				break;
>  
>  			xfs_bmap_add_free(ip->i_mount, &dfops,
> -					irec.br_startblock, irec.br_blockcount,
> +					del.br_startblock, del.br_blockcount,
>  					NULL);
>  
>  			/* Update quota accounting */
>  			xfs_trans_mod_dquot_byino(*tpp, ip, XFS_TRANS_DQ_BCOUNT,
> -					-(long)irec.br_blockcount);
> +					-(long)del.br_blockcount);
>  
>  			/* Roll the transaction */
>  			error = xfs_defer_finish(tpp, &dfops, ip);
> @@ -573,14 +567,13 @@ xfs_reflink_cancel_cow_blocks(
>  			}
>  
>  			/* Remove the mapping from the CoW fork. */
> -			error = xfs_bunmapi_cow(ip, &irec);
> -			if (error)
> -				break;
> +			xfs_bmap_del_extent_cow(ip, &idx, &got, &del);
>  		}
>  
> -		/* Roll on... */
> -		offset_fsb = irec.br_startoff + irec.br_blockcount;
> -	}
> +		if (++idx >= ifp->if_bytes / sizeof(struct xfs_bmbt_rec))
> +			return 0;
> +		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
> +	} while (1);
>  
>  	return error;
>  }
> -- 
> 2.10.1.382.ga23ca1b
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/9] xfs: optimize xfs_reflink_cancel_cow_blocks

[Christoph Hellwig]

On Wed, Oct 12, 2016 at 10:13:25AM -0400, Brian Foster wrote:
> How about using a while loop..?
> 
> 	while (got.br_startoff < end_fsb) {
> 		...
> 
> Otherwise, looks fine:

Sure.  My initial version had a real do {} while loop, but the helper
to make that possible was dropped from this series and will be sent
separately.

[PATCH 8/9] xfs: optimize xfs_reflink_end_cow

[Christoph Hellwig]

Instead of doing a full extent list search for each extent that is to be
deleted using xfs_bmapi_read and then doing another one inside of
xfs_bunmapi_cow use the same scheme that xfs_bumapi uses:  look up the
last extent to be deleted and then use the extent index to walk downward
until we are outside the range to be deleted.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_reflink.c | 114 ++++++++++++++++++++++-----------------------------
 fs/xfs/xfs_trace.h   |   1 -
 2 files changed, 49 insertions(+), 66 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 08170bb..c4e7521 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -637,25 +637,22 @@ xfs_reflink_end_cow(
 	xfs_off_t			offset,
 	xfs_off_t			count)
 {
-	struct xfs_bmbt_irec		irec;
-	struct xfs_bmbt_irec		uirec;
+	struct xfs_ifork		*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
+	struct xfs_bmbt_irec		got, prev, del;
 	struct xfs_trans		*tp;
 	xfs_fileoff_t			offset_fsb;
 	xfs_fileoff_t			end_fsb;
-	xfs_filblks_t			count_fsb;
 	xfs_fsblock_t			firstfsb;
 	struct xfs_defer_ops		dfops;
-	int				error;
+	int				error, eof = 0;
 	unsigned int			resblks;
-	xfs_filblks_t			ilen;
 	xfs_filblks_t			rlen;
-	int				nimaps;
+	xfs_extnum_t			idx;
 
 	trace_xfs_reflink_end_cow(ip, offset, count);
 
 	offset_fsb = XFS_B_TO_FSBT(ip->i_mount, offset);
 	end_fsb = XFS_B_TO_FSB(ip->i_mount, offset + count);
-	count_fsb = (xfs_filblks_t)(end_fsb - offset_fsb);
 
 	/* Start a rolling transaction to switch the mappings */
 	resblks = XFS_EXTENTADD_SPACE_RES(ip->i_mount, XFS_DATA_FORK);
@@ -667,73 +664,61 @@ xfs_reflink_end_cow(
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, ip, 0);
 
-	/* Go find the old extent in the CoW fork. */
-	while (offset_fsb < end_fsb) {
-		/* Read extent from the source file */
-		nimaps = 1;
-		count_fsb = (xfs_filblks_t)(end_fsb - offset_fsb);
-		error = xfs_bmapi_read(ip, offset_fsb, count_fsb, &irec,
-				&nimaps, XFS_BMAPI_COWFORK);
-		if (error)
-			goto out_cancel;
-		ASSERT(nimaps == 1);
-
-		ASSERT(irec.br_startblock != DELAYSTARTBLOCK);
-		trace_xfs_reflink_cow_remap(ip, &irec);
+	xfs_bmap_search_extents(ip, end_fsb - 1, XFS_COW_FORK, &eof, &idx,
+			&got, &prev);
+	if (eof) {
+		ASSERT(idx > 0);
+		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, --idx), &got);
+	}
 
-		/*
-		 * We can have a hole in the CoW fork if part of a directio
-		 * write is CoW but part of it isn't.
-		 */
-		rlen = ilen = irec.br_blockcount;
-		if (irec.br_startblock == HOLESTARTBLOCK)
+	do {
+		ASSERT(!isnullstartblock(got.br_startblock));
+		del = got;
+		xfs_trim_extent(&del, offset_fsb, end_fsb - offset_fsb);
+		if (!del.br_blockcount)
 			goto next_extent;
 
 		/* Unmap the old blocks in the data fork. */
-		while (rlen) {
-			xfs_defer_init(&dfops, &firstfsb);
-			error = __xfs_bunmapi(tp, ip, irec.br_startoff,
-					&rlen, 0, 1, &firstfsb, &dfops);
-			if (error)
-				goto out_defer;
-
-			/*
-			 * Trim the extent to whatever got unmapped.
-			 * Remember, bunmapi works backwards.
-			 */
-			uirec.br_startblock = irec.br_startblock + rlen;
-			uirec.br_startoff = irec.br_startoff + rlen;
-			uirec.br_blockcount = irec.br_blockcount - rlen;
-			irec.br_blockcount = rlen;
-			trace_xfs_reflink_cow_remap_piece(ip, &uirec);
+		xfs_defer_init(&dfops, &firstfsb);
+		rlen = del.br_blockcount;
+		error = __xfs_bunmapi(tp, ip, del.br_startoff, &rlen, 0, 1,
+				&firstfsb, &dfops);
+		if (error)
+			goto out_defer;
 
-			/* Free the CoW orphan record. */
-			error = xfs_refcount_free_cow_extent(tp->t_mountp,
-					&dfops, uirec.br_startblock,
-					uirec.br_blockcount);
-			if (error)
-				goto out_defer;
+		/* Trim the extent to whatever got unmapped. */
+		if (rlen) {
+			xfs_trim_extent(&del, del.br_startoff + rlen,
+				del.br_blockcount - rlen);
+		}
+		trace_xfs_reflink_cow_remap(ip, &del);
 
-			/* Map the new blocks into the data fork. */
-			error = xfs_bmap_map_extent(tp->t_mountp, &dfops,
-					ip, &uirec);
-			if (error)
-				goto out_defer;
+		/* Free the CoW orphan record. */
+		error = xfs_refcount_free_cow_extent(tp->t_mountp, &dfops,
+				del.br_startblock, del.br_blockcount);
+		if (error)
+			goto out_defer;
 
-			/* Remove the mapping from the CoW fork. */
-			error = xfs_bunmapi_cow(ip, &uirec);
-			if (error)
-				goto out_defer;
+		/* Map the new blocks into the data fork. */
+		error = xfs_bmap_map_extent(tp->t_mountp, &dfops, ip, &del);
+		if (error)
+			goto out_defer;
 
-			error = xfs_defer_finish(&tp, &dfops, ip);
-			if (error)
-				goto out_defer;
-		}
+		/* Remove the mapping from the CoW fork. */
+		xfs_bmap_del_extent_cow(ip, &idx, &got, &del);
+
+		error = xfs_defer_finish(&tp, &dfops, ip);
+		if (error)
+			goto out_defer;
 
 next_extent:
-		/* Roll on... */
-		offset_fsb = irec.br_startoff + ilen;
-	}
+		if (del.br_startoff == offset_fsb || idx < 0)
+			break;
+
+		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
+		if (got.br_startoff >= del.br_startoff && --idx >= 0)
+			xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
+	} while (got.br_startoff < del.br_startoff);
 
 	error = xfs_trans_commit(tp);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
@@ -743,7 +728,6 @@ next_extent:
 
 out_defer:
 	xfs_defer_cancel(&dfops);
-out_cancel:
 	xfs_trans_cancel(tp);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 out:
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 520660d..f3b4418 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -3357,7 +3357,6 @@ DEFINE_INODE_IREC_EVENT(xfs_reflink_trim_irec);
 DEFINE_SIMPLE_IO_EVENT(xfs_reflink_cancel_cow_range);
 DEFINE_SIMPLE_IO_EVENT(xfs_reflink_end_cow);
 DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_remap);
-DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_remap_piece);
 
 DEFINE_INODE_ERROR_EVENT(xfs_reflink_allocate_cow_range_error);
 DEFINE_INODE_ERROR_EVENT(xfs_reflink_cancel_cow_range_error);
-- 
2.10.1.382.ga23ca1b

Re: [PATCH 8/9] xfs: optimize xfs_reflink_end_cow

[Brian Foster]

On Mon, Oct 10, 2016 at 03:38:04PM +0200, Christoph Hellwig wrote:
> Instead of doing a full extent list search for each extent that is to be
> deleted using xfs_bmapi_read and then doing another one inside of
> xfs_bunmapi_cow use the same scheme that xfs_bumapi uses:  look up the
> last extent to be deleted and then use the extent index to walk downward
> until we are outside the range to be deleted.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_reflink.c | 114 ++++++++++++++++++++++-----------------------------
>  fs/xfs/xfs_trace.h   |   1 -
>  2 files changed, 49 insertions(+), 66 deletions(-)
> 
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 08170bb..c4e7521 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -637,25 +637,22 @@ xfs_reflink_end_cow(
>  	xfs_off_t			offset,
>  	xfs_off_t			count)
>  {
> -	struct xfs_bmbt_irec		irec;
> -	struct xfs_bmbt_irec		uirec;
> +	struct xfs_ifork		*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
> +	struct xfs_bmbt_irec		got, prev, del;
>  	struct xfs_trans		*tp;
>  	xfs_fileoff_t			offset_fsb;
>  	xfs_fileoff_t			end_fsb;
> -	xfs_filblks_t			count_fsb;
>  	xfs_fsblock_t			firstfsb;
>  	struct xfs_defer_ops		dfops;
> -	int				error;
> +	int				error, eof = 0;
>  	unsigned int			resblks;
> -	xfs_filblks_t			ilen;
>  	xfs_filblks_t			rlen;
> -	int				nimaps;
> +	xfs_extnum_t			idx;
>  
>  	trace_xfs_reflink_end_cow(ip, offset, count);
>  
>  	offset_fsb = XFS_B_TO_FSBT(ip->i_mount, offset);
>  	end_fsb = XFS_B_TO_FSB(ip->i_mount, offset + count);
> -	count_fsb = (xfs_filblks_t)(end_fsb - offset_fsb);
>  
>  	/* Start a rolling transaction to switch the mappings */
>  	resblks = XFS_EXTENTADD_SPACE_RES(ip->i_mount, XFS_DATA_FORK);
> @@ -667,73 +664,61 @@ xfs_reflink_end_cow(
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  	xfs_trans_ijoin(tp, ip, 0);
>  
> -	/* Go find the old extent in the CoW fork. */
> -	while (offset_fsb < end_fsb) {
> -		/* Read extent from the source file */
> -		nimaps = 1;
> -		count_fsb = (xfs_filblks_t)(end_fsb - offset_fsb);
> -		error = xfs_bmapi_read(ip, offset_fsb, count_fsb, &irec,
> -				&nimaps, XFS_BMAPI_COWFORK);
> -		if (error)
> -			goto out_cancel;
> -		ASSERT(nimaps == 1);
> -
> -		ASSERT(irec.br_startblock != DELAYSTARTBLOCK);
> -		trace_xfs_reflink_cow_remap(ip, &irec);
> +	xfs_bmap_search_extents(ip, end_fsb - 1, XFS_COW_FORK, &eof, &idx,
> +			&got, &prev);
> +	if (eof) {
> +		ASSERT(idx > 0);
> +		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, --idx), &got);
> +	}

Hmm, should this happen? Do we really want to proceed with the previous
extent?

I suppose the extent trim below would catch if we've dropped outside the
range of the I/O, but note that if trim extent sets blockcount = 0, it
doesn't necessarily reset the start offset for the (del.br_startoff ==
offset_fsb) check at next_extent. Perhaps that should break once the del
end offset precedes offset_fsb..?

Either way, it would be nice to have a comment here if there's some
reason we shouldn't just bail out or flag a problem...

>  
> -		/*
> -		 * We can have a hole in the CoW fork if part of a directio
> -		 * write is CoW but part of it isn't.
> -		 */
> -		rlen = ilen = irec.br_blockcount;
> -		if (irec.br_startblock == HOLESTARTBLOCK)
> +	do {
> +		ASSERT(!isnullstartblock(got.br_startblock));
> +		del = got;
> +		xfs_trim_extent(&del, offset_fsb, end_fsb - offset_fsb);
> +		if (!del.br_blockcount)
>  			goto next_extent;
>  
>  		/* Unmap the old blocks in the data fork. */
> -		while (rlen) {
> -			xfs_defer_init(&dfops, &firstfsb);
> -			error = __xfs_bunmapi(tp, ip, irec.br_startoff,
> -					&rlen, 0, 1, &firstfsb, &dfops);
> -			if (error)
> -				goto out_defer;
> -
> -			/*
> -			 * Trim the extent to whatever got unmapped.
> -			 * Remember, bunmapi works backwards.
> -			 */
> -			uirec.br_startblock = irec.br_startblock + rlen;
> -			uirec.br_startoff = irec.br_startoff + rlen;
> -			uirec.br_blockcount = irec.br_blockcount - rlen;
> -			irec.br_blockcount = rlen;
> -			trace_xfs_reflink_cow_remap_piece(ip, &uirec);
> +		xfs_defer_init(&dfops, &firstfsb);
> +		rlen = del.br_blockcount;
> +		error = __xfs_bunmapi(tp, ip, del.br_startoff, &rlen, 0, 1,
> +				&firstfsb, &dfops);
> +		if (error)
> +			goto out_defer;
>  
> -			/* Free the CoW orphan record. */
> -			error = xfs_refcount_free_cow_extent(tp->t_mountp,
> -					&dfops, uirec.br_startblock,
> -					uirec.br_blockcount);
> -			if (error)
> -				goto out_defer;
> +		/* Trim the extent to whatever got unmapped. */
> +		if (rlen) {
> +			xfs_trim_extent(&del, del.br_startoff + rlen,
> +				del.br_blockcount - rlen);
> +		}
> +		trace_xfs_reflink_cow_remap(ip, &del);
>  
> -			/* Map the new blocks into the data fork. */
> -			error = xfs_bmap_map_extent(tp->t_mountp, &dfops,
> -					ip, &uirec);
> -			if (error)
> -				goto out_defer;
> +		/* Free the CoW orphan record. */
> +		error = xfs_refcount_free_cow_extent(tp->t_mountp, &dfops,
> +				del.br_startblock, del.br_blockcount);
> +		if (error)
> +			goto out_defer;
>  
> -			/* Remove the mapping from the CoW fork. */
> -			error = xfs_bunmapi_cow(ip, &uirec);
> -			if (error)
> -				goto out_defer;
> +		/* Map the new blocks into the data fork. */
> +		error = xfs_bmap_map_extent(tp->t_mountp, &dfops, ip, &del);
> +		if (error)
> +			goto out_defer;
>  
> -			error = xfs_defer_finish(&tp, &dfops, ip);
> -			if (error)
> -				goto out_defer;
> -		}
> +		/* Remove the mapping from the CoW fork. */
> +		xfs_bmap_del_extent_cow(ip, &idx, &got, &del);
> +
> +		error = xfs_defer_finish(&tp, &dfops, ip);
> +		if (error)
> +			goto out_defer;
>  
>  next_extent:
> -		/* Roll on... */
> -		offset_fsb = irec.br_startoff + ilen;
> -	}
> +		if (del.br_startoff == offset_fsb || idx < 0)
> +			break;
> +
> +		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
> +		if (got.br_startoff >= del.br_startoff && --idx >= 0)
> +			xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
> +	} while (got.br_startoff < del.br_startoff);

Maybe it's just me, but I find this whole loop kind of confusing. I
realize it is tricky because we have to handle several different cases
with regard to index (no more extents to the left, bmap_del_extent_cow()
pushing index forward or back).

In staring at it a bit, I'm wondering if something like the following
would work:

	/* walk backwards until we're out of the I/O range... */
	while (got.br_startoff + got.br_blockcount > offset_fsb) {
		del = got;
		xfs_trim_extent(&del, offset_fsb, end_fsb - offset_fsb);
		/* extent delete may have bumped idx forward */
		if (!del.br_blockcount) {
			idx--;
			goto next_extent;
		}

		...

next_extent:
		if (idx < 0)
			break;
		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
	}

Thoughts?

Brian

>  
>  	error = xfs_trans_commit(tp);
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> @@ -743,7 +728,6 @@ next_extent:
>  
>  out_defer:
>  	xfs_defer_cancel(&dfops);
> -out_cancel:
>  	xfs_trans_cancel(tp);
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  out:
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 520660d..f3b4418 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -3357,7 +3357,6 @@ DEFINE_INODE_IREC_EVENT(xfs_reflink_trim_irec);
>  DEFINE_SIMPLE_IO_EVENT(xfs_reflink_cancel_cow_range);
>  DEFINE_SIMPLE_IO_EVENT(xfs_reflink_end_cow);
>  DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_remap);
> -DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_remap_piece);
>  
>  DEFINE_INODE_ERROR_EVENT(xfs_reflink_allocate_cow_range_error);
>  DEFINE_INODE_ERROR_EVENT(xfs_reflink_cancel_cow_range_error);
> -- 
> 2.10.1.382.ga23ca1b
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9] xfs: optimize xfs_reflink_end_cow

[Christoph Hellwig]

On Wed, Oct 12, 2016 at 10:15:37AM -0400, Brian Foster wrote:
> > +	xfs_bmap_search_extents(ip, end_fsb - 1, XFS_COW_FORK, &eof, &idx,
> > +			&got, &prev);
> > +	if (eof) {
> > +		ASSERT(idx > 0);
> > +		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, --idx), &got);
> > +	}
> 
> Hmm, should this happen? Do we really want to proceed with the previous
> extent?

Yes.  This is the same pattern as xfs_bunmapi and it is needed.  For
an explanation we need to recall the semantics of xfs_bmap_search_extents,
which gives us the extent at bno IFF there is one, else if gives us the
extent beyond bno.

So we search for the last extent in the to be deleted range here,
but if there is no extent at the end of the range we'll get a return
outside the range, in which case we need to back one up (which
bunmapi does and we catch somewhat confusingly with the trim_extent
and goto next_extent, I should clean that up..), but as a special
case we can get the eof case where there is no extent beyong bno.

> I suppose the extent trim below would catch if we've dropped outside the
> range of the I/O, but note that if trim extent sets blockcount = 0, it
> doesn't necessarily reset the start offset for the (del.br_startoff ==
> offset_fsb) check at next_extent. Perhaps that should break once the del
> end offset precedes offset_fsb..?
> 
> Either way, it would be nice to have a comment here if there's some
> reason we shouldn't just bail out or flag a problem...

Yes, both this code and bumapi could use some cleanup..

> Maybe it's just me, but I find this whole loop kind of confusing. I
> realize it is tricky because we have to handle several different cases
> with regard to index (no more extents to the left, bmap_del_extent_cow()
> pushing index forward or back).

I found it rather confusing as well.
> 
> In staring at it a bit, I'm wondering if something like the following
> would work:
> 
> 	/* walk backwards until we're out of the I/O range... */
> 	while (got.br_startoff + got.br_blockcount > offset_fsb) {
> 		del = got;
> 		xfs_trim_extent(&del, offset_fsb, end_fsb - offset_fsb);
> 		/* extent delete may have bumped idx forward */
> 		if (!del.br_blockcount) {
> 			idx--;
> 			goto next_extent;
> 		}
> 
> 		...
> 
> next_extent:
> 		if (idx < 0)
> 			break;
> 		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
> 	}
> 
> Thoughts?

That looks reasonable, I'll see if it passes xfstests.. :)

Re: [PATCH 8/9] xfs: optimize xfs_reflink_end_cow

[Brian Foster]

On Thu, Oct 13, 2016 at 09:06:39AM +0200, Christoph Hellwig wrote:
> On Wed, Oct 12, 2016 at 10:15:37AM -0400, Brian Foster wrote:
> > > +	xfs_bmap_search_extents(ip, end_fsb - 1, XFS_COW_FORK, &eof, &idx,
> > > +			&got, &prev);
> > > +	if (eof) {
> > > +		ASSERT(idx > 0);
> > > +		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, --idx), &got);
> > > +	}
> > 
> > Hmm, should this happen? Do we really want to proceed with the previous
> > extent?
> 
> Yes.  This is the same pattern as xfs_bunmapi and it is needed.  For
> an explanation we need to recall the semantics of xfs_bmap_search_extents,
> which gives us the extent at bno IFF there is one, else if gives us the
> extent beyond bno.
> 

Ok, right..

> So we search for the last extent in the to be deleted range here,
> but if there is no extent at the end of the range we'll get a return
> outside the range, in which case we need to back one up (which
> bunmapi does and we catch somewhat confusingly with the trim_extent
> and goto next_extent, I should clean that up..), but as a special
> case we can get the eof case where there is no extent beyong bno.
> 

Got it, thanks. I think it was just the bmap_search_extents() semantics
that threw me off. For some reason I was thinking eof meant there was
nothing in the range, but that's not valid here because we are walking
backwards.

And the ASSERT(idx > 0) exists because if we're at eof, we must have at
least one preceding extent because we're in the end I/O handler, after
all. Makes sense.

> > I suppose the extent trim below would catch if we've dropped outside the
> > range of the I/O, but note that if trim extent sets blockcount = 0, it
> > doesn't necessarily reset the start offset for the (del.br_startoff ==
> > offset_fsb) check at next_extent. Perhaps that should break once the del
> > end offset precedes offset_fsb..?
> > 
> > Either way, it would be nice to have a comment here if there's some
> > reason we shouldn't just bail out or flag a problem...
> 
> Yes, both this code and bumapi could use some cleanup..
> 
> > Maybe it's just me, but I find this whole loop kind of confusing. I
> > realize it is tricky because we have to handle several different cases
> > with regard to index (no more extents to the left, bmap_del_extent_cow()
> > pushing index forward or back).
> 
> I found it rather confusing as well.
> > 
> > In staring at it a bit, I'm wondering if something like the following
> > would work:
> > 
> > 	/* walk backwards until we're out of the I/O range... */
> > 	while (got.br_startoff + got.br_blockcount > offset_fsb) {
> > 		del = got;
> > 		xfs_trim_extent(&del, offset_fsb, end_fsb - offset_fsb);
> > 		/* extent delete may have bumped idx forward */
> > 		if (!del.br_blockcount) {
> > 			idx--;
> > 			goto next_extent;
> > 		}
> > 
> > 		...
> > 
> > next_extent:
> > 		if (idx < 0)
> > 			break;
> > 		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
> > 	}
> > 
> > Thoughts?
> 
> That looks reasonable, I'll see if it passes xfstests.. :)

Great, thanks!

Brian

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 9/9] xfs: remove xfs_bunmapi_cow

[Christoph Hellwig]

Since no one uses it anymore.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c | 22 ----------------------
 fs/xfs/libxfs/xfs_bmap.h |  1 -
 2 files changed, 23 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 814980d..680fa9d 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5417,28 +5417,6 @@ done:
 	return error;
 }
 
-/* Remove an extent from the CoW fork.  Similar to xfs_bmap_del_extent. */
-int
-xfs_bunmapi_cow(
-	struct xfs_inode		*ip,
-	struct xfs_bmbt_irec		*del)
-{
-	struct xfs_bmbt_rec_host	*ep;
-	struct xfs_bmbt_irec		got;
-	struct xfs_bmbt_irec		new;
-	int				eof;
-	xfs_extnum_t			eidx;
-
-	ep = xfs_bmap_search_extents(ip, del->br_startoff, XFS_COW_FORK, &eof,
-				&eidx, &got, &new);
-	ASSERT(ep);
-	if (isnullstartblock(got.br_startblock))
-		xfs_bmap_del_extent_delay(ip, XFS_COW_FORK, &eidx, &got, del);
-	else
-		xfs_bmap_del_extent_cow(ip, &eidx, &got, del);
-	return 0;
-}
-
 /*
  * Unmap (remove) blocks from a file.
  * If nexts is nonzero then the number of extents to remove is limited to
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index 5f18248..7cae6ec 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -223,7 +223,6 @@ int	xfs_bunmapi(struct xfs_trans *tp, struct xfs_inode *ip,
 		xfs_fileoff_t bno, xfs_filblks_t len, int flags,
 		xfs_extnum_t nexts, xfs_fsblock_t *firstblock,
 		struct xfs_defer_ops *dfops, int *done);
-int	xfs_bunmapi_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *del);
 int	xfs_bmap_del_extent_delay(struct xfs_inode *ip, int whichfork,
 		xfs_extnum_t *idx, struct xfs_bmbt_irec *got,
 		struct xfs_bmbt_irec *del);
-- 
2.10.1.382.ga23ca1b

optimize the COW I/O path V2

[Christoph Hellwig]

This series contains a couple optimizations for the COW I/O path:

 - avoid any overhead for reads from COW files
 - streamline the COW handling in write_begin
 - reduce the number of extent lookups during COW I/O completion

It does not touch the direct I/O path (yet), I still have a few
unsolved problems there.

Changes since V1:
 - incorporated misc feedback from Brian

[PATCH 7/9] xfs: optimize xfs_reflink_cancel_cow_blocks

[Christoph Hellwig]

Rewrite xfs_reflink_cancel_cow_blocks so that we only do a search for
the first extent in the extent list and then iterate over the remaining
extents using the extent index, passing the extent we operate on
directly to xfs_bmap_del_extent_delay or xfs_bmap_del_extent_cow instead
of going through xfs_bunmapi and doing yet another extent list lookup.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_reflink.c | 51 +++++++++++++++++++++++----------------------------
 1 file changed, 23 insertions(+), 28 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index f33f737..7979d46 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -512,53 +512,49 @@ xfs_reflink_cancel_cow_blocks(
 	xfs_fileoff_t			offset_fsb,
 	xfs_fileoff_t			end_fsb)
 {
-	struct xfs_bmbt_irec		irec;
-	xfs_filblks_t			count_fsb;
+	struct xfs_ifork		*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
+	struct xfs_bmbt_irec		got, prev, del;
+	xfs_extnum_t			idx;
 	xfs_fsblock_t			firstfsb;
 	struct xfs_defer_ops		dfops;
-	int				error = 0;
-	int				nimaps;
+	int				error = 0, eof = 0;
 
 	if (!xfs_is_reflink_inode(ip))
 		return 0;
 
-	/* Go find the old extent in the CoW fork. */
-	while (offset_fsb < end_fsb) {
-		nimaps = 1;
-		count_fsb = (xfs_filblks_t)(end_fsb - offset_fsb);
-		error = xfs_bmapi_read(ip, offset_fsb, count_fsb, &irec,
-				&nimaps, XFS_BMAPI_COWFORK);
-		if (error)
-			break;
-		ASSERT(nimaps == 1);
+	xfs_bmap_search_extents(ip, offset_fsb, XFS_COW_FORK, &eof, &idx,
+			&got, &prev);
+	if (eof)
+		return 0;
 
-		trace_xfs_reflink_cancel_cow(ip, &irec);
+	while (got.br_startoff < end_fsb) {
+		del = got;
+		xfs_trim_extent(&del, offset_fsb, end_fsb - offset_fsb);
+		trace_xfs_reflink_cancel_cow(ip, &del);
 
-		if (irec.br_startblock == DELAYSTARTBLOCK) {
-			/* Remove the mapping from the CoW fork. */
-			error = xfs_bunmapi_cow(ip, &irec);
+		if (isnullstartblock(del.br_startblock)) {
+			error = xfs_bmap_del_extent_delay(ip, XFS_COW_FORK,
+					&idx, &got, &del);
 			if (error)
 				break;
-		} else if (irec.br_startblock == HOLESTARTBLOCK) {
-			/* empty */
 		} else {
 			xfs_trans_ijoin(*tpp, ip, 0);
 			xfs_defer_init(&dfops, &firstfsb);
 
 			/* Free the CoW orphan record. */
 			error = xfs_refcount_free_cow_extent(ip->i_mount,
-					&dfops, irec.br_startblock,
-					irec.br_blockcount);
+					&dfops, del.br_startblock,
+					del.br_blockcount);
 			if (error)
 				break;
 
 			xfs_bmap_add_free(ip->i_mount, &dfops,
-					irec.br_startblock, irec.br_blockcount,
+					del.br_startblock, del.br_blockcount,
 					NULL);
 
 			/* Update quota accounting */
 			xfs_trans_mod_dquot_byino(*tpp, ip, XFS_TRANS_DQ_BCOUNT,
-					-(long)irec.br_blockcount);
+					-(long)del.br_blockcount);
 
 			/* Roll the transaction */
 			error = xfs_defer_finish(tpp, &dfops, ip);
@@ -568,13 +564,12 @@ xfs_reflink_cancel_cow_blocks(
 			}
 
 			/* Remove the mapping from the CoW fork. */
-			error = xfs_bunmapi_cow(ip, &irec);
-			if (error)
-				break;
+			xfs_bmap_del_extent_cow(ip, &idx, &got, &del);
 		}
 
-		/* Roll on... */
-		offset_fsb = irec.br_startoff + irec.br_blockcount;
+		if (++idx >= ifp->if_bytes / sizeof(struct xfs_bmbt_rec))
+			return 0;
+		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
 	}
 
 	return error;
-- 
2.1.4

Re: [PATCH 7/9] xfs: optimize xfs_reflink_cancel_cow_blocks

[Brian Foster]

On Sat, Oct 15, 2016 at 10:52:32AM +0200, Christoph Hellwig wrote:
> Rewrite xfs_reflink_cancel_cow_blocks so that we only do a search for
> the first extent in the extent list and then iterate over the remaining
> extents using the extent index, passing the extent we operate on
> directly to xfs_bmap_del_extent_delay or xfs_bmap_del_extent_cow instead
> of going through xfs_bunmapi and doing yet another extent list lookup.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_reflink.c | 51 +++++++++++++++++++++++----------------------------
>  1 file changed, 23 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index f33f737..7979d46 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -512,53 +512,49 @@ xfs_reflink_cancel_cow_blocks(
>  	xfs_fileoff_t			offset_fsb,
>  	xfs_fileoff_t			end_fsb)
>  {
> -	struct xfs_bmbt_irec		irec;
> -	xfs_filblks_t			count_fsb;
> +	struct xfs_ifork		*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
> +	struct xfs_bmbt_irec		got, prev, del;
> +	xfs_extnum_t			idx;
>  	xfs_fsblock_t			firstfsb;
>  	struct xfs_defer_ops		dfops;
> -	int				error = 0;
> -	int				nimaps;
> +	int				error = 0, eof = 0;
>  
>  	if (!xfs_is_reflink_inode(ip))
>  		return 0;
>  
> -	/* Go find the old extent in the CoW fork. */
> -	while (offset_fsb < end_fsb) {
> -		nimaps = 1;
> -		count_fsb = (xfs_filblks_t)(end_fsb - offset_fsb);
> -		error = xfs_bmapi_read(ip, offset_fsb, count_fsb, &irec,
> -				&nimaps, XFS_BMAPI_COWFORK);
> -		if (error)
> -			break;
> -		ASSERT(nimaps == 1);
> +	xfs_bmap_search_extents(ip, offset_fsb, XFS_COW_FORK, &eof, &idx,
> +			&got, &prev);
> +	if (eof)
> +		return 0;
>  
> -		trace_xfs_reflink_cancel_cow(ip, &irec);
> +	while (got.br_startoff < end_fsb) {
> +		del = got;
> +		xfs_trim_extent(&del, offset_fsb, end_fsb - offset_fsb);
> +		trace_xfs_reflink_cancel_cow(ip, &del);
>  
> -		if (irec.br_startblock == DELAYSTARTBLOCK) {
> -			/* Remove the mapping from the CoW fork. */
> -			error = xfs_bunmapi_cow(ip, &irec);
> +		if (isnullstartblock(del.br_startblock)) {
> +			error = xfs_bmap_del_extent_delay(ip, XFS_COW_FORK,
> +					&idx, &got, &del);
>  			if (error)
>  				break;
> -		} else if (irec.br_startblock == HOLESTARTBLOCK) {
> -			/* empty */
>  		} else {
>  			xfs_trans_ijoin(*tpp, ip, 0);
>  			xfs_defer_init(&dfops, &firstfsb);
>  
>  			/* Free the CoW orphan record. */
>  			error = xfs_refcount_free_cow_extent(ip->i_mount,
> -					&dfops, irec.br_startblock,
> -					irec.br_blockcount);
> +					&dfops, del.br_startblock,
> +					del.br_blockcount);
>  			if (error)
>  				break;
>  
>  			xfs_bmap_add_free(ip->i_mount, &dfops,
> -					irec.br_startblock, irec.br_blockcount,
> +					del.br_startblock, del.br_blockcount,
>  					NULL);
>  
>  			/* Update quota accounting */
>  			xfs_trans_mod_dquot_byino(*tpp, ip, XFS_TRANS_DQ_BCOUNT,
> -					-(long)irec.br_blockcount);
> +					-(long)del.br_blockcount);
>  
>  			/* Roll the transaction */
>  			error = xfs_defer_finish(tpp, &dfops, ip);
> @@ -568,13 +564,12 @@ xfs_reflink_cancel_cow_blocks(
>  			}
>  
>  			/* Remove the mapping from the CoW fork. */
> -			error = xfs_bunmapi_cow(ip, &irec);
> -			if (error)
> -				break;
> +			xfs_bmap_del_extent_cow(ip, &idx, &got, &del);
>  		}
>  
> -		/* Roll on... */
> -		offset_fsb = irec.br_startoff + irec.br_blockcount;
> +		if (++idx >= ifp->if_bytes / sizeof(struct xfs_bmbt_rec))
> +			return 0;
> +		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
>  	}
>  
>  	return error;
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/9] xfs: optimize xfs_reflink_cancel_cow_blocks

[Darrick J. Wong]

On Sat, Oct 15, 2016 at 10:52:32AM +0200, Christoph Hellwig wrote:
> Rewrite xfs_reflink_cancel_cow_blocks so that we only do a search for
> the first extent in the extent list and then iterate over the remaining
> extents using the extent index, passing the extent we operate on
> directly to xfs_bmap_del_extent_delay or xfs_bmap_del_extent_cow instead
> of going through xfs_bunmapi and doing yet another extent list lookup.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> ---
>  fs/xfs/xfs_reflink.c | 51 +++++++++++++++++++++++----------------------------
>  1 file changed, 23 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index f33f737..7979d46 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -512,53 +512,49 @@ xfs_reflink_cancel_cow_blocks(
>  	xfs_fileoff_t			offset_fsb,
>  	xfs_fileoff_t			end_fsb)
>  {
> -	struct xfs_bmbt_irec		irec;
> -	xfs_filblks_t			count_fsb;
> +	struct xfs_ifork		*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
> +	struct xfs_bmbt_irec		got, prev, del;
> +	xfs_extnum_t			idx;
>  	xfs_fsblock_t			firstfsb;
>  	struct xfs_defer_ops		dfops;
> -	int				error = 0;
> -	int				nimaps;
> +	int				error = 0, eof = 0;
>  
>  	if (!xfs_is_reflink_inode(ip))
>  		return 0;
>  
> -	/* Go find the old extent in the CoW fork. */
> -	while (offset_fsb < end_fsb) {
> -		nimaps = 1;
> -		count_fsb = (xfs_filblks_t)(end_fsb - offset_fsb);
> -		error = xfs_bmapi_read(ip, offset_fsb, count_fsb, &irec,
> -				&nimaps, XFS_BMAPI_COWFORK);
> -		if (error)
> -			break;
> -		ASSERT(nimaps == 1);
> +	xfs_bmap_search_extents(ip, offset_fsb, XFS_COW_FORK, &eof, &idx,
> +			&got, &prev);
> +	if (eof)
> +		return 0;
>  
> -		trace_xfs_reflink_cancel_cow(ip, &irec);
> +	while (got.br_startoff < end_fsb) {
> +		del = got;
> +		xfs_trim_extent(&del, offset_fsb, end_fsb - offset_fsb);
> +		trace_xfs_reflink_cancel_cow(ip, &del);
>  
> -		if (irec.br_startblock == DELAYSTARTBLOCK) {
> -			/* Remove the mapping from the CoW fork. */
> -			error = xfs_bunmapi_cow(ip, &irec);
> +		if (isnullstartblock(del.br_startblock)) {
> +			error = xfs_bmap_del_extent_delay(ip, XFS_COW_FORK,
> +					&idx, &got, &del);
>  			if (error)
>  				break;
> -		} else if (irec.br_startblock == HOLESTARTBLOCK) {
> -			/* empty */
>  		} else {
>  			xfs_trans_ijoin(*tpp, ip, 0);
>  			xfs_defer_init(&dfops, &firstfsb);
>  
>  			/* Free the CoW orphan record. */
>  			error = xfs_refcount_free_cow_extent(ip->i_mount,
> -					&dfops, irec.br_startblock,
> -					irec.br_blockcount);
> +					&dfops, del.br_startblock,
> +					del.br_blockcount);
>  			if (error)
>  				break;
>  
>  			xfs_bmap_add_free(ip->i_mount, &dfops,
> -					irec.br_startblock, irec.br_blockcount,
> +					del.br_startblock, del.br_blockcount,
>  					NULL);
>  
>  			/* Update quota accounting */
>  			xfs_trans_mod_dquot_byino(*tpp, ip, XFS_TRANS_DQ_BCOUNT,
> -					-(long)irec.br_blockcount);
> +					-(long)del.br_blockcount);
>  
>  			/* Roll the transaction */
>  			error = xfs_defer_finish(tpp, &dfops, ip);
> @@ -568,13 +564,12 @@ xfs_reflink_cancel_cow_blocks(
>  			}
>  
>  			/* Remove the mapping from the CoW fork. */
> -			error = xfs_bunmapi_cow(ip, &irec);
> -			if (error)
> -				break;
> +			xfs_bmap_del_extent_cow(ip, &idx, &got, &del);
>  		}
>  
> -		/* Roll on... */
> -		offset_fsb = irec.br_startoff + irec.br_blockcount;
> +		if (++idx >= ifp->if_bytes / sizeof(struct xfs_bmbt_rec))
> +			return 0;
> +		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
>  	}
>  
>  	return error;
> -- 
> 2.1.4
>