On Tue, Jan 23, 2018 at 07:05:57AM -0500, Brian Foster wrote:

On Sat, Jan 20, 2018 at 09:34:10PM -0800, Darrick J. Wong wrote:

quoted

From: Darrick J. Wong <redacted>

Ensure that we've attached all the necessary dquots before performing
reflink operations so that quota accounting is accurate.

Signed-off-by: Darrick J. Wong <redacted>
---
 fs/xfs/xfs_reflink.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 5d1ff5a..947d0637 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c

@@ -54,6 +54,7 @@
 #include "xfs_rmap_btree.h"
 #include "xfs_sb.h"
 #include "xfs_ag_resv.h"
+#include "xfs_qm.h"
 
 /*
  * Copy on Write of Shared Blocks

@@ -282,6 +283,10 @@ xfs_reflink_reserve_cow(
 	 * tree.
 	 */
 
+	error = xfs_qm_dqattach_locked(ip, 0);
+	if (error)
+		return error;
+

The same call exists further down in the function. Was the intent to
move it? I suspect we don't need it twice, at least.

Drat, I don't know why either of these are there, I think I got paste-happy?

OH, yuck, this is the debug patch from an earlier revision.  The only
dqattach we actually need is...

quoted

 	if (!xfs_iext_lookup_extent(ip, ifp, imap->br_startoff, &icur, &got))
 		eof = true;
 	if (!eof && got.br_startoff <= imap->br_startoff) {

@@ -396,6 +401,10 @@ xfs_reflink_allocate_cow(
 	ASSERT(xfs_is_reflink_inode(ip));
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
 
+	error = xfs_qm_dqattach_locked(ip, 0);
+	if (error)
+		return error;
+

Similar pattern here, but for this one the assert above suggests we
could have the shared lock. xfs_qm_dqattach_locked() looks like it
expects the exclusive lock (and that's what it looks like the second
call deals with). Hm?

Brian

quoted

 	/*
 	 * Even if the extent is not shared we might have a preallocation for
 	 * it in the COW fork.  If so use it.

@@ -1356,6 +1365,14 @@ xfs_reflink_remap_range(
 	if (IS_DAX(inode_in) || IS_DAX(inode_out))
 		goto out_unlock;
 
+	/* Attach dquots to both inodes */
+	ret = xfs_qm_dqattach(src, 0);
+	if (ret)
+		goto out_unlock;
+	ret = xfs_qm_dqattach(dest, 0);

...this one, because we're not changing the src file's block allocations,
but we /are/ forgetting to ensure they're attached to dest.

Sorry for the noise, I'll send out the proper patch later today.

--D

quoted

+	if (ret)
+		goto out_unlock;
+
 	ret = vfs_clone_file_prep_inodes(inode_in, pos_in, inode_out, pos_out,
 			&len, is_dedupe);
 	if (ret <= 0)

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

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