[PATCH 0/3] xfs: a few reflink cowblocks fixes

[PATCH 0/3] xfs: a few reflink cowblocks fixes

[Brian Foster]

This is just a few reflink fixes associated with cowblocks tagging that
I came across when playing around with and/or reviewing some code. The
issue associated with patch 3 is easily reproducible with something like
the following:

	echo 30 > /proc/sys/fs/xfs/speculative_cow_prealloc_lifetime

	xfs_io -fc "cowextsize 64k" $src
	xfs_io -c "pwrite 0 128k" $src
	cp --reflink $src $tgt

	xfs_io -c "pwrite 64k 32k" $tgt
	xfs_io -c "pwrite 96k 32k" $tgt
	echo sleep
	sleep 25
	echo delalloc
	xfs_io -c "pwrite 0 4k" $tgt

... which basically just sets the cowblocks tag, uses up all of the
resulting cow blocks, waits until just before the cowblocks scanner is
going to run and creates a delalloc reservation in the cow fork. The cow
fork fiemap thingy I sent the other day shows that the resulting cow
fork blocks remain indefinitely.

Thoughts, reviews, flames appreciated.

Brian

Brian Foster (3):
  xfs: fix up inode cowblocks tracking tracepoints
  xfs: clear cowblocks tag when cow fork is emptied
  xfs: don't skip cow forks w/ delalloc blocks in cowblocks scan

 fs/xfs/xfs_icache.c  | 10 +++++-----
 fs/xfs/xfs_reflink.c | 29 +++++++++--------------------
 fs/xfs/xfs_reflink.h |  2 +-
 3 files changed, 15 insertions(+), 26 deletions(-)

-- 
2.7.4

[PATCH 1/3] xfs: fix up inode cowblocks tracking tracepoints

[Brian Foster]

These calls are still using the eofblocks tracepoints. The cowblocks
equivalents are already defined, we just aren't actually calling them.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_icache.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 14796b7..f295049 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -1656,9 +1656,9 @@ void
 xfs_inode_set_cowblocks_tag(
 	xfs_inode_t	*ip)
 {
-	trace_xfs_inode_set_eofblocks_tag(ip);
+	trace_xfs_inode_set_cowblocks_tag(ip);
 	return __xfs_inode_set_eofblocks_tag(ip, xfs_queue_cowblocks,
-			trace_xfs_perag_set_eofblocks,
+			trace_xfs_perag_set_cowblocks,
 			XFS_ICI_COWBLOCKS_TAG);
 }
 
@@ -1666,7 +1666,7 @@ void
 xfs_inode_clear_cowblocks_tag(
 	xfs_inode_t	*ip)
 {
-	trace_xfs_inode_clear_eofblocks_tag(ip);
+	trace_xfs_inode_clear_cowblocks_tag(ip);
 	return __xfs_inode_clear_eofblocks_tag(ip,
-			trace_xfs_perag_clear_eofblocks, XFS_ICI_COWBLOCKS_TAG);
+			trace_xfs_perag_clear_cowblocks, XFS_ICI_COWBLOCKS_TAG);
 }
-- 
2.7.4

Re: [PATCH 1/3] xfs: fix up inode cowblocks tracking tracepoints

[Darrick J. Wong]

On Fri, Oct 21, 2016 at 01:22:11PM -0400, Brian Foster wrote:
> These calls are still using the eofblocks tracepoints. The cowblocks
> equivalents are already defined, we just aren't actually calling them.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Good catch!

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

> ---
>  fs/xfs/xfs_icache.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 14796b7..f295049 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -1656,9 +1656,9 @@ void
>  xfs_inode_set_cowblocks_tag(
>  	xfs_inode_t	*ip)
>  {
> -	trace_xfs_inode_set_eofblocks_tag(ip);
> +	trace_xfs_inode_set_cowblocks_tag(ip);
>  	return __xfs_inode_set_eofblocks_tag(ip, xfs_queue_cowblocks,
> -			trace_xfs_perag_set_eofblocks,
> +			trace_xfs_perag_set_cowblocks,
>  			XFS_ICI_COWBLOCKS_TAG);
>  }
>  
> @@ -1666,7 +1666,7 @@ void
>  xfs_inode_clear_cowblocks_tag(
>  	xfs_inode_t	*ip)
>  {
> -	trace_xfs_inode_clear_eofblocks_tag(ip);
> +	trace_xfs_inode_clear_cowblocks_tag(ip);
>  	return __xfs_inode_clear_eofblocks_tag(ip,
> -			trace_xfs_perag_clear_eofblocks, XFS_ICI_COWBLOCKS_TAG);
> +			trace_xfs_perag_clear_cowblocks, XFS_ICI_COWBLOCKS_TAG);
>  }
> -- 
> 2.7.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 1/3] xfs: fix up inode cowblocks tracking tracepoints

[Christoph Hellwig]

On Fri, Oct 21, 2016 at 01:22:11PM -0400, Brian Foster wrote:
> These calls are still using the eofblocks tracepoints. The cowblocks
> equivalents are already defined, we just aren't actually calling them.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/3] xfs: clear cowblocks tag when cow fork is emptied

[Brian Foster]

The background cowblocks scan job takes care of scanning for inodes with
potentially lingering blocks in the cow fork and clearing them out. If
the background scanner reclaims the cow fork blocks, however, it doesn't
immediately clear the cowblocks tag from the inode. Instead, the inode
remains tagged until the background scanner comes around again,
discovers the inode cow fork has no blocks, clears the tag and fires the
trace_xfs_inode_free_cowblocks_invalid() tracepoint to indicate that the
inode may have been incorrectly tagged.

This is not a major functional problem as the tag is ultimately cleared.
Nonetheless, clear the tag when an inode cow fork is explicitly emptied
to avoid the extra round trip through the background scanner and
spurious "invalid" tracepoint.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_reflink.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index cd308f1..a279b4e 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -567,10 +567,14 @@ xfs_reflink_cancel_cow_blocks(
 		}
 
 		if (++idx >= ifp->if_bytes / sizeof(struct xfs_bmbt_rec))
-			return 0;
+			break;
 		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
 	}
 
+	/* clear tag if cow fork is emptied */
+	if (!ifp->if_bytes)
+		xfs_inode_clear_cowblocks_tag(ip);
+
 	return error;
 }
 
-- 
2.7.4

Re: [PATCH 2/3] xfs: clear cowblocks tag when cow fork is emptied

[Darrick J. Wong]

On Fri, Oct 21, 2016 at 01:22:12PM -0400, Brian Foster wrote:
> The background cowblocks scan job takes care of scanning for inodes with
> potentially lingering blocks in the cow fork and clearing them out. If
> the background scanner reclaims the cow fork blocks, however, it doesn't
> immediately clear the cowblocks tag from the inode. Instead, the inode
> remains tagged until the background scanner comes around again,
> discovers the inode cow fork has no blocks, clears the tag and fires the
> trace_xfs_inode_free_cowblocks_invalid() tracepoint to indicate that the
> inode may have been incorrectly tagged.
> 
> This is not a major functional problem as the tag is ultimately cleared.
> Nonetheless, clear the tag when an inode cow fork is explicitly emptied
> to avoid the extra round trip through the background scanner and
> spurious "invalid" tracepoint.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks ok to me,

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

(Will give this series a more thorough run through the spin cycle.)

> ---
>  fs/xfs/xfs_reflink.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index cd308f1..a279b4e 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -567,10 +567,14 @@ xfs_reflink_cancel_cow_blocks(
>  		}
>  
>  		if (++idx >= ifp->if_bytes / sizeof(struct xfs_bmbt_rec))
> -			return 0;
> +			break;
>  		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
>  	}
>  
> +	/* clear tag if cow fork is emptied */
> +	if (!ifp->if_bytes)
> +		xfs_inode_clear_cowblocks_tag(ip);
> +
>  	return error;
>  }
>  
> -- 
> 2.7.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 2/3] xfs: clear cowblocks tag when cow fork is emptied

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 3/3] xfs: don't skip cow forks w/ delalloc blocks in cowblocks scan

[Brian Foster]

The cowblocks background scanner currently clears the cowblocks tag for
inodes without any real allocations in the cow fork. This excludes
inodes with only delalloc blocks in the cow fork. While we might never
expect to clear delalloc blocks from the cow fork in the background
scanner, it is not necessarily correct to clear the cowblocks tag from
such inodes.

For example, if the background scanner happens to process an inode
between a buffered write and writeback, the scanner catches the inode in
a state after delalloc blocks have been allocated to the cow fork but
before the delalloc blocks have been converted to real blocks by
writeback. The background scanner then incorrectly clears the cowblocks
tag, even if part of the aforementioned delalloc reservation will not be
remapped to the data fork (i.e., extra blocks due to the cowextsize
hint). This means that any such additional blocks in the cow fork might
never be reclaimed by the background scanner and could persist until the
inode itself is reclaimed.

To address this problem, only skip and clear inodes without any cow fork
allocations whatsoever from the background scanner. While we generally
do not want to cancel delalloc reservations from the background scanner,
the pagecache dirty check following the cowblocks check should prevent
that situation. If we do end up with delalloc cow fork blocks without a
dirty address space mapping, this is probably an indication that
something has gone wrong and the blocks should be reclaimed, as they may
never be converted to a real allocation.

XXX: There are probably multiple ways to deal with this. Another option
could be to tweak the has_cow_blocks() helper to separately return
whether a cow fork has delalloc blocks and skip over the inode without
actually clearing the tag. Thoughts?

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_icache.c  |  2 +-
 fs/xfs/xfs_reflink.c | 23 ++++-------------------
 fs/xfs/xfs_reflink.h |  2 +-
 3 files changed, 6 insertions(+), 21 deletions(-)

diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index f295049..1191064 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -1583,7 +1583,7 @@ xfs_inode_free_cowblocks(
 
 	ASSERT(!eofb || (eofb && eofb->eof_scan_owner != 0));
 
-	if (!xfs_reflink_has_real_cow_blocks(ip)) {
+	if (!xfs_reflink_has_cow_blocks(ip)) {
 		trace_xfs_inode_free_cowblocks_invalid(ip);
 		xfs_inode_clear_cowblocks_tag(ip);
 		return 0;
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index a279b4e..6057c37 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1699,35 +1699,20 @@ xfs_reflink_unshare(
 }
 
 /*
- * Does this inode have any real CoW reservations?
+ * Does this inode have any CoW reservations?
  */
 bool
-xfs_reflink_has_real_cow_blocks(
+xfs_reflink_has_cow_blocks(
 	struct xfs_inode		*ip)
 {
-	struct xfs_bmbt_irec		irec;
 	struct xfs_ifork		*ifp;
-	struct xfs_bmbt_rec_host	*gotp;
-	xfs_extnum_t			idx;
 
 	if (!xfs_is_reflink_inode(ip))
 		return false;
 
-	/* Go find the old extent in the CoW fork. */
 	ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
-	gotp = xfs_iext_bno_to_ext(ifp, 0, &idx);
-	while (gotp) {
-		xfs_bmbt_get_all(gotp, &irec);
-
-		if (!isnullstartblock(irec.br_startblock))
-			return true;
-
-		/* Roll on... */
-		idx++;
-		if (idx >= ifp->if_bytes / sizeof(xfs_bmbt_rec_t))
-			break;
-		gotp = xfs_iext_get_ext(ifp, idx);
-	}
+	if (ifp->if_bytes)
+		return true;
 
 	return false;
 }
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index fad1160..cdd3b1a 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -50,6 +50,6 @@ extern int xfs_reflink_clear_inode_flag(struct xfs_inode *ip,
 extern int xfs_reflink_unshare(struct xfs_inode *ip, xfs_off_t offset,
 		xfs_off_t len);
 
-extern bool xfs_reflink_has_real_cow_blocks(struct xfs_inode *ip);
+extern bool xfs_reflink_has_cow_blocks(struct xfs_inode *ip);
 
 #endif /* __XFS_REFLINK_H */
-- 
2.7.4

Re: [PATCH 3/3] xfs: don't skip cow forks w/ delalloc blocks in cowblocks scan

[Christoph Hellwig]

IFF we want to kill the delalloc blocks as well I'd suggest to kill
the helper and open code the two checks.