[PATCH 0/6] xfs: reflink fixes

[PATCH 0/6] xfs: reflink fixes

[Darrick J. Wong]

Hi all,

Here's a rollup of some reflink fixes that I've accumulated over the
past couple of weeks.  I added CLONERANGE support to fsstress and wrote
a fstest to run a long write-mostly fsstress, which very rapidly found
problems.

The first patch fixes a trivial null pointer problem; the second fixes
an ftrace stale data reporting problem; the third fixes a problem where
we ought to clear post-eof preallocations prior to allowing a reflink at
a higher offset than i_size; the fourth relaxes a locking assert
condition that is actually possible; the fifth fixes an infinite loop
when cancelling cow rservations; and the sixth one allows cow remap
operations to use the transaction block reserve to avoid enospc'ing
after a successful cow.

These patches apply directly to the end of 'xfs: logging fixes'.

--D

[PATCH 1/6] xfs: account for null transactions in bunmapi

[Darrick J. Wong]

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

In e1a4e37cc7b665 ("xfs: try to avoid blowing out the transaction
reservation when bunmaping a shared extent"), we try to constrain the
amount of real extents we unmap from the data fork in a given call so
that we don't blow out transaction reservations.

However, not all bunmapi operations require a transaction -- if we're
only removing a delalloc extent, no transaction is needed, so we have to
code against that.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_bmap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 1210f68..1bddbba 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5136,7 +5136,7 @@ __xfs_bunmapi(
 	 * blowing out the transaction with a mix of EFIs and reflink
 	 * adjustments.
 	 */
-	if (xfs_is_reflink_inode(ip) && whichfork == XFS_DATA_FORK)
+	if (tp && xfs_is_reflink_inode(ip) && whichfork == XFS_DATA_FORK)
 		max_len = min(len, xfs_refcount_max_unmap(tp->t_log_res));
 	else
 		max_len = len;

Re: [PATCH 1/6] xfs: account for null transactions in bunmapi

[Christoph Hellwig]

On Sun, Dec 10, 2017 at 06:16:20PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In e1a4e37cc7b665 ("xfs: try to avoid blowing out the transaction
> reservation when bunmaping a shared extent"), we try to constrain the
> amount of real extents we unmap from the data fork in a given call so
> that we don't blow out transaction reservations.
> 
> However, not all bunmapi operations require a transaction -- if we're
> only removing a delalloc extent, no transaction is needed, so we have to
> code against that.

Looks good for now.  And reminds me that I need to submit my patch
to get rid of that special case :)

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

[PATCH 2/6] xfs: move xfs_iext_insert tracepoint to report useful information

[Darrick J. Wong]

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

Move the tracepoint in xfs_iext_insert to after the point where we've
inserted the extent because otherwise we report stale extent data in
the ftrace output.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_iext_tree.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_iext_tree.c b/fs/xfs/libxfs/xfs_iext_tree.c
index 89bf16b..b0f3179 100644
--- a/fs/xfs/libxfs/xfs_iext_tree.c
+++ b/fs/xfs/libxfs/xfs_iext_tree.c
@@ -632,8 +632,6 @@ xfs_iext_insert(
 	struct xfs_iext_leaf	*new = NULL;
 	int			nr_entries, i;
 
-	trace_xfs_iext_insert(ip, cur, state, _RET_IP_);
-
 	if (ifp->if_height == 0)
 		xfs_iext_alloc_root(ifp, cur);
 	else if (ifp->if_height == 1)
@@ -661,6 +659,8 @@ xfs_iext_insert(
 	xfs_iext_set(cur_rec(cur), irec);
 	ifp->if_bytes += sizeof(struct xfs_iext_rec);
 
+	trace_xfs_iext_insert(ip, cur, state, _RET_IP_);
+
 	if (new)
 		xfs_iext_insert_node(ifp, xfs_iext_leaf_key(new, 0), new, 2);
 }

Re: [PATCH 2/6] xfs: move xfs_iext_insert tracepoint to report useful information

[Christoph Hellwig]

On Sun, Dec 10, 2017 at 06:16:26PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Move the tracepoint in xfs_iext_insert to after the point where we've
> inserted the extent because otherwise we report stale extent data in
> the ftrace output.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

[PATCH 3/6] xfs: remove dest file's post-eof preallocations before reflinking

[Darrick J. Wong]

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

If we try to reflink into a file with post-eof preallocations at an
offset well past the preallocations, we increase i_size as one would
expect.  However, those allocations do not have page cache backing them,
so they won't get cleaned out on their own.  This leads to asserts in
the collapse/insert range code and xfs_destroy_inode when they encounter
delalloc extents they weren't expecting to find.

Since there are plenty of other places where we dump those post-eof
blocks, do the same to the reflink destination file before we start
remapping extents.  This was found by adding clonerange support to
fsstress and running it in write-only mode.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_reflink.c |   11 +++++++++++
 1 file changed, 11 insertions(+)


diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index cf7c8f8..e13f5ad 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1291,6 +1291,17 @@ xfs_reflink_remap_range(
 
 	trace_xfs_reflink_remap_range(src, pos_in, len, dest, pos_out);
 
+	/*
+	 * Clear out post-eof preallocations because we don't have page cache
+	 * backing the delayed allocations and they'll never get freed on
+	 * their own.
+	 */
+	if (xfs_can_free_eofblocks(dest, true)) {
+		ret = xfs_free_eofblocks(dest);
+		if (ret)
+			goto out_unlock;
+	}
+
 	/* Set flags and remap blocks. */
 	ret = xfs_reflink_set_inode_flag(src, dest);
 	if (ret)

Re: [PATCH 3/6] xfs: remove dest file's post-eof preallocations before reflinking

[Christoph Hellwig]

Looks good,

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

[PATCH 4/6] xfs: relax is_reflink_inode assert in xfs_reflink_find_cow_mapping

[Darrick J. Wong]

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

We don't hold the ilock through the entire sequence of xfs_writepage_map
-> xfs_map_cow -> xfs_reflink_find_cow_mapping.  This means that we can
race with another thread that is trying to clear the inode reflink flag,
with the result that the flag is set for the xfs_map_cow check but
cleared before we get to the assert in find_cow_mapping.  When this
happens, we blow the assert even though everything is fine.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 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 e13f5ad..99c5852 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -490,8 +490,9 @@ xfs_reflink_find_cow_mapping(
 	struct xfs_iext_cursor		icur;
 
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
-	ASSERT(xfs_is_reflink_inode(ip));
 
+	if (!xfs_is_reflink_inode(ip))
+		return false;
 	offset_fsb = XFS_B_TO_FSBT(ip->i_mount, offset);
 	if (!xfs_iext_lookup_extent(ip, ifp, offset_fsb, &icur, &got))
 		return false;

Re: [PATCH 4/6] xfs: relax is_reflink_inode assert in xfs_reflink_find_cow_mapping

[Christoph Hellwig]

On Sun, Dec 10, 2017 at 06:16:41PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> We don't hold the ilock through the entire sequence of xfs_writepage_map
> -> xfs_map_cow -> xfs_reflink_find_cow_mapping.  This means that we can
> race with another thread that is trying to clear the inode reflink flag,
> with the result that the flag is set for the xfs_map_cow check but
> cleared before we get to the assert in find_cow_mapping.  When this
> happens, we blow the assert even though everything is fine.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

[PATCH 5/6] xfs: avoid infinite loop when cancelling CoW blocks after writeback failure

[Darrick J. Wong]

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

When we're cancelling a cow range, we don't always delete each extent
that we iterate, so we have to move icur backwards in the list to avoid
an infinite loop.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_reflink.c |    3 +++
 1 file changed, 3 insertions(+)


diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 99c5852..6931b0c 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -611,6 +611,9 @@ xfs_reflink_cancel_cow_blocks(
 
 			/* Remove the mapping from the CoW fork. */
 			xfs_bmap_del_extent_cow(ip, &icur, &got, &del);
+		} else {
+			/* Didn't do anything, push cursor back. */
+			xfs_iext_prev(ifp, &icur);
 		}
 next_extent:
 		if (!xfs_iext_get_extent(ifp, &icur, &got))

Re: [PATCH 5/6] xfs: avoid infinite loop when cancelling CoW blocks after writeback failure

[Christoph Hellwig]

On Sun, Dec 10, 2017 at 06:16:48PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When we're cancelling a cow range, we don't always delete each extent
> that we iterate, so we have to move icur backwards in the list to avoid
> an infinite loop.

Looks good,

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

[PATCH 6/6] xfs: allow CoW remap transactions to use reserve blocks

[Darrick J. Wong]

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

Since we as yet have no way of holding on to the indlen blocks that are
reserved as part of CoW fork delalloc reservations, let the CoW remap
transaction dip into the reserves so that we avoid failing writes.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_reflink.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 6931b0c..e49e6db 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -729,7 +729,7 @@ xfs_reflink_end_cow(
 			(unsigned int)(end_fsb - offset_fsb),
 			XFS_DATA_FORK);
 	error = xfs_trans_alloc(ip->i_mount, &M_RES(ip->i_mount)->tr_write,
-			resblks, 0, 0, &tp);
+			resblks, 0, XFS_TRANS_RESERVE, &tp);
 	if (error)
 		goto out;
 

Re: [PATCH 6/6] xfs: allow CoW remap transactions to use reserve blocks

[Christoph Hellwig]

On Sun, Dec 10, 2017 at 06:16:59PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Since we as yet have no way of holding on to the indlen blocks that are
> reserved as part of CoW fork delalloc reservations, let the CoW remap
> transaction dip into the reserves so that we avoid failing writes.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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