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

[PATCH v2] 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.

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

v2:
- Drop the xfs_reflink_has_real_cow_blocks() helper entirely.
v1: http://www.spinics.net/lists/linux-xfs/msg01732.html

 fs/xfs/xfs_icache.c  |  7 ++++++-
 fs/xfs/xfs_reflink.c | 34 ----------------------------------
 fs/xfs/xfs_reflink.h |  2 --
 3 files changed, 6 insertions(+), 37 deletions(-)

diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index f295049..1b4861f 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -1580,10 +1580,15 @@ xfs_inode_free_cowblocks(
 	struct xfs_eofblocks *eofb = args;
 	bool need_iolock = true;
 	int match;
+	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
 
 	ASSERT(!eofb || (eofb && eofb->eof_scan_owner != 0));
 
-	if (!xfs_reflink_has_real_cow_blocks(ip)) {
+	/*
+	 * Just clear the tag if we have an empty cow fork or none at all. It's
+	 * possible the inode was fully unshared since it was originally tagged.
+	 */
+	if (!xfs_is_reflink_inode(ip) || !ifp->if_bytes) {
 		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..c069048 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1697,37 +1697,3 @@ xfs_reflink_unshare(
 	trace_xfs_reflink_unshare_error(ip, error, _RET_IP_);
 	return error;
 }
-
-/*
- * Does this inode have any real CoW reservations?
- */
-bool
-xfs_reflink_has_real_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);
-	}
-
-	return false;
-}
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index fad1160..97ea9b4 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -50,6 +50,4 @@ 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);
-
 #endif /* __XFS_REFLINK_H */
-- 
2.7.4

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

[Darrick J. Wong]

On Mon, Oct 24, 2016 at 10:16:14AM -0400, Brian Foster wrote:
> 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.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks reasonable and passes the clone tests, so:
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> v2:
> - Drop the xfs_reflink_has_real_cow_blocks() helper entirely.
> v1: http://www.spinics.net/lists/linux-xfs/msg01732.html
> 
>  fs/xfs/xfs_icache.c  |  7 ++++++-
>  fs/xfs/xfs_reflink.c | 34 ----------------------------------
>  fs/xfs/xfs_reflink.h |  2 --
>  3 files changed, 6 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index f295049..1b4861f 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -1580,10 +1580,15 @@ xfs_inode_free_cowblocks(
>  	struct xfs_eofblocks *eofb = args;
>  	bool need_iolock = true;
>  	int match;
> +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
>  
>  	ASSERT(!eofb || (eofb && eofb->eof_scan_owner != 0));
>  
> -	if (!xfs_reflink_has_real_cow_blocks(ip)) {
> +	/*
> +	 * Just clear the tag if we have an empty cow fork or none at all. It's
> +	 * possible the inode was fully unshared since it was originally tagged.
> +	 */
> +	if (!xfs_is_reflink_inode(ip) || !ifp->if_bytes) {
>  		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..c069048 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -1697,37 +1697,3 @@ xfs_reflink_unshare(
>  	trace_xfs_reflink_unshare_error(ip, error, _RET_IP_);
>  	return error;
>  }
> -
> -/*
> - * Does this inode have any real CoW reservations?
> - */
> -bool
> -xfs_reflink_has_real_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);
> -	}
> -
> -	return false;
> -}
> diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
> index fad1160..97ea9b4 100644
> --- a/fs/xfs/xfs_reflink.h
> +++ b/fs/xfs/xfs_reflink.h
> @@ -50,6 +50,4 @@ 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);
> -
>  #endif /* __XFS_REFLINK_H */
> -- 
> 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