[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;

[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);
 }

[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)

[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;

[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))

[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 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>

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>

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>

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>

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>

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>

Re: [PATCH 0/6] xfs: reflink fixes

[Darrick J. Wong]

On Sat, Jan 20, 2018 at 09:33:49PM -0800, Darrick J. Wong wrote:
> Hi all,
> 
> Running generic/232 with quotas and reflink demonstrated that there was
> something wrong with the way we did quota accounting -- on an otherwise
> idle system, fs-wide du block count numbers didn't match the quota
> reports.  I started digging into why the quota accounting was wrong, and
> the following are the results of my bug hunt.

Well, I wasn't expecting 4.15 to be delayed again, but I guess it has.
To disambiguate: this series is intended for 4.16, even though I already
sent out the AGFL series tagged for 4.17.

--D

> The first patch teaches the reflink code to break layout leases before
> commencing the block remapping work.  This time we avoid the "looping
> trying to get a lock" that Christoph complained about, in favor of
> dropping both locks and retrying if we can't cleanly break the layouts
> without waiting.
> 
> The second patch changes the source file locking (if src != dest) during
> a reflink operation to take the shared locks when possible.  The only
> thing changing in the source file is the setting of the reflink iflag,
> for which we will still take ILOCK_EXCL.  The net result of this is
> less lock contention during fsstress and a 30% lower runtime, not that
> anyone cares about fsstress benchmarking. :)
> 
> Patch three ensure that we attach dquots to inodes before we start
> reflinking their blocks.  This could lead to quota undercharging; an
> fstest to check this will be sent separately.
> 
> Patch four reorganizes the copy on write quota updating code to reflect
> how the CoW fork works now.  In short, the CoW fork is entirely in
> memory, so we can only use the in-memory quota reservation counters for
> all CoW blocks; the accounting only becomes permanent if we remap an
> extent into the data fork.
> 
> Patch five creates a separate i_cow_blocks counter to track all the CoW
> blocks assigned to a file, which makes changing a file's uid/gid/prjid
> easier, makes reporting cow blocks via stat easy, and enables various
> cleanups.
> 
> Patch six fixes a serious potential corruption problem with the cow
> extent allocation -- when we allocate into the CoW fork with the cow
> extent size hint set, the allocator enlarges the allocation request to
> try to hit alignment goals.  However, if the allocated extent does not
> actually fulfill any of the requested range, we send a garbage
> zero-length extent back to the iomap code (which also doesn't notice),
> and the write lands at the startblock of the garbage extent.  The fix is
> to detect that we didn't fill the entire requested range and fix up the
> returned mapping so that we always fill the first block of the
> requested allocation.
> 
> --D
> --
> 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 0/6] xfs: reflink fixes

[Darrick J. Wong]

Hi all,

Running generic/232 with quotas and reflink demonstrated that there was
something wrong with the way we did quota accounting -- on an otherwise
idle system, fs-wide du block count numbers didn't match the quota
reports.  I started digging into why the quota accounting was wrong, and
the following are the results of my bug hunt.

The first patch teaches the reflink code to break layout leases before
commencing the block remapping work.  This time we avoid the "looping
trying to get a lock" that Christoph complained about, in favor of
dropping both locks and retrying if we can't cleanly break the layouts
without waiting.

The second patch changes the source file locking (if src != dest) during
a reflink operation to take the shared locks when possible.  The only
thing changing in the source file is the setting of the reflink iflag,
for which we will still take ILOCK_EXCL.  The net result of this is
less lock contention during fsstress and a 30% lower runtime, not that
anyone cares about fsstress benchmarking. :)

Patch three ensure that we attach dquots to inodes before we start
reflinking their blocks.  This could lead to quota undercharging; an
fstest to check this will be sent separately.

Patch four reorganizes the copy on write quota updating code to reflect
how the CoW fork works now.  In short, the CoW fork is entirely in
memory, so we can only use the in-memory quota reservation counters for
all CoW blocks; the accounting only becomes permanent if we remap an
extent into the data fork.

Patch five creates a separate i_cow_blocks counter to track all the CoW
blocks assigned to a file, which makes changing a file's uid/gid/prjid
easier, makes reporting cow blocks via stat easy, and enables various
cleanups.

Patch six fixes a serious potential corruption problem with the cow
extent allocation -- when we allocate into the CoW fork with the cow
extent size hint set, the allocator enlarges the allocation request to
try to hit alignment goals.  However, if the allocated extent does not
actually fulfill any of the requested range, we send a garbage
zero-length extent back to the iomap code (which also doesn't notice),
and the write lands at the startblock of the garbage extent.  The fix is
to detect that we didn't fill the entire requested range and fix up the
returned mapping so that we always fill the first block of the
requested allocation.

--D