xfs: embed dfops in the transaction

xfs_itruncate_extents[_flags]() uses a local dfops with a transaction provided by the caller. It uses hacky ->t_dfops replacement logic to avoid stomping over an already populated ->t_dfops.

The latter never occurs for current callers and the logic itself is not really appropriate. Clean this up by updating all callers to initialize a dfops and to use that down in xfs_itruncate_extents(). This more closely resembles the upcoming logic where dfops will be embedded within the transaction. We can also replace the xfs_defer_init() in the xfs_itruncate_extents_flags() loop with an assert. Both dfops and firstblock should be in a valid state after xfs_defer_finish() and the inode joined to the dfops is fixed throughout the loop.

Signed-off-by Brian Foster <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_attr_inactive.c | 3 +++
 fs/xfs/xfs_bmap_util.c     | 2 ++
 fs/xfs/xfs_inode.c         | 8 +++-----
 fs/xfs/xfs_iops.c          | 3 +++
 fs/xfs/xfs_qm_syscalls.c   | 3 +++
 5 files changed, 14 insertions(+), 5 deletions(-)

fs/xfs/xfs_attr_inactive.c +3 -0

index 7ce10055f275..d3055972d3a6 100644
@@ -26,6 +26,7 @@
 #include "xfs_quota.h"
 #include "xfs_trace.h"
 #include "xfs_dir2.h"
+#include "xfs_defer.h"
 
 /*
  * Look at all the extents for this logical region,

@@ -381,6 +382,7 @@ xfs_attr_inactive(
 {
 	struct xfs_trans	*trans;
 	struct xfs_mount	*mp;
+	struct xfs_defer_ops	dfops;
 	int			lock_mode = XFS_ILOCK_SHARED;
 	int			error = 0;
 

@@ -397,6 +399,7 @@ xfs_attr_inactive(
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_attrinval, 0, 0, 0, &trans);
 	if (error)
 		goto out_destroy_fork;
+	xfs_defer_init(trans, &dfops);
 
 	lock_mode = XFS_ILOCK_EXCL;
 	xfs_ilock(dp, lock_mode);

fs/xfs/xfs_bmap_util.c +2 -0

index d3a314fd721f..ee9481b142f0 100644
@@ -792,6 +792,7 @@ xfs_free_eofblocks(
 	int			nimaps;
 	struct xfs_bmbt_irec	imap;
 	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_defer_ops	dfops;
 
 	/*
 	 * Figure out if there are any blocks beyond the end

@@ -831,6 +832,7 @@ xfs_free_eofblocks(
 			ASSERT(XFS_FORCED_SHUTDOWN(mp));
 			return error;
 		}
+		xfs_defer_init(tp, &dfops);
 
 		xfs_ilock(ip, XFS_ILOCK_EXCL);
 		xfs_trans_ijoin(tp, ip, 0);

fs/xfs/xfs_inode.c +3 -5

index 7b2694d3901a..7d7d7e95fa17 100644
@@ -1541,8 +1541,6 @@ xfs_itruncate_extents_flags(
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*tp = *tpp;
-	struct xfs_defer_ops	*odfops = tp->t_dfops;
-	struct xfs_defer_ops	dfops;
 	xfs_fileoff_t		first_unmap_block;
 	xfs_fileoff_t		last_block;
 	xfs_filblks_t		unmap_len;

@@ -1579,7 +1577,7 @@ xfs_itruncate_extents_flags(
 	ASSERT(first_unmap_block < last_block);
 	unmap_len = last_block - first_unmap_block + 1;
 	while (!done) {
-		xfs_defer_init(tp, &dfops);
+		ASSERT(tp->t_firstblock == NULLFSBLOCK);
 		error = xfs_bunmapi(tp, ip, first_unmap_block, unmap_len, flags,
 				    XFS_ITRUNC_MAX_EXTENTS, &done);
 		if (error)

@@ -1618,8 +1616,6 @@ xfs_itruncate_extents_flags(
 	trace_xfs_itruncate_extents_end(ip, new_size);
 
 out:
-	/* ->t_dfops points to local stack, don't leak it! */
-	tp->t_dfops = odfops;
 	*tpp = tp;
 	return error;
 out_bmap_cancel:

@@ -1723,6 +1719,7 @@ xfs_inactive_truncate(
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*tp;
+	struct xfs_defer_ops	dfops;
 	int			error;
 
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);

@@ -1730,6 +1727,7 @@ xfs_inactive_truncate(
 		ASSERT(XFS_FORCED_SHUTDOWN(mp));
 		return error;
 	}
+	xfs_defer_init(tp, &dfops);
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, ip, 0);

fs/xfs/xfs_iops.c +3 -0

index 0fa29f39d658..704b57a8b99e 100644
@@ -26,6 +26,7 @@
 #include "xfs_dir2.h"
 #include "xfs_trans_space.h"
 #include "xfs_iomap.h"
+#include "xfs_defer.h"
 
 #include <linux/capability.h>
 #include <linux/xattr.h>

@@ -812,6 +813,7 @@ xfs_setattr_size(
 	struct inode		*inode = VFS_I(ip);
 	xfs_off_t		oldsize, newsize;
 	struct xfs_trans	*tp;
+	struct xfs_defer_ops	dfops;
 	int			error;
 	uint			lock_flags = 0;
 	bool			did_zeroing = false;

@@ -915,6 +917,7 @@ xfs_setattr_size(
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
 	if (error)
 		return error;
+	xfs_defer_init(tp, &dfops);
 
 	lock_flags |= XFS_ILOCK_EXCL;
 	xfs_ilock(ip, XFS_ILOCK_EXCL);

fs/xfs/xfs_qm_syscalls.c +3 -0

index abc8a21e3a82..df0783303887 100644
@@ -22,6 +22,7 @@
 #include "xfs_qm.h"
 #include "xfs_trace.h"
 #include "xfs_icache.h"
+#include "xfs_defer.h"
 
 STATIC int	xfs_qm_log_quotaoff(xfs_mount_t *, xfs_qoff_logitem_t **, uint);
 STATIC int	xfs_qm_log_quotaoff_end(xfs_mount_t *, xfs_qoff_logitem_t *,

@@ -213,6 +214,7 @@ xfs_qm_scall_trunc_qfile(
 {
 	struct xfs_inode	*ip;
 	struct xfs_trans	*tp;
+	struct xfs_defer_ops	dfops;
 	int			error;
 
 	if (ino == NULLFSINO)

@@ -229,6 +231,7 @@ xfs_qm_scall_trunc_qfile(
 		xfs_iunlock(ip, XFS_IOLOCK_EXCL);
 		goto out_put;
 	}
+	xfs_defer_init(tp, &dfops);
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, ip, 0);

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

xfs_itruncate_extents[_flags]() uses a local dfops with a transaction provided by the caller. It uses hacky ->t_dfops replacement logic to avoid stomping over an already populated ->t_dfops.

The latter never occurs for current callers and the logic itself is not really appropriate. Clean this up by updating all callers to initialize a dfops and to use that down in xfs_itruncate_extents(). This more closely resembles the upcoming logic where dfops will be embedded within the transaction. We can also replace the xfs_defer_init() in the xfs_itruncate_extents_flags() loop with an assert. Both dfops and firstblock should be in a valid state after xfs_defer_finish() and the inode joined to the dfops is fixed throughout the loop.

Signed-off-by Brian Foster <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_attr_inactive.c | 3 +++
 fs/xfs/xfs_bmap_util.c     | 2 ++
 fs/xfs/xfs_inode.c         | 8 +++-----
 fs/xfs/xfs_iops.c          | 3 +++
 fs/xfs/xfs_qm_syscalls.c   | 3 +++
 5 files changed, 14 insertions(+), 5 deletions(-)

fs/xfs/xfs_attr_inactive.c +3 -0

index 7ce10055f275..d3055972d3a6 100644
@@ -26,6 +26,7 @@
 #include "xfs_quota.h"
 #include "xfs_trace.h"
 #include "xfs_dir2.h"
+#include "xfs_defer.h"
 
 /*
  * Look at all the extents for this logical region,

@@ -381,6 +382,7 @@ xfs_attr_inactive(
 {
 	struct xfs_trans	*trans;
 	struct xfs_mount	*mp;
+	struct xfs_defer_ops	dfops;
 	int			lock_mode = XFS_ILOCK_SHARED;
 	int			error = 0;
 

@@ -397,6 +399,7 @@ xfs_attr_inactive(
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_attrinval, 0, 0, 0, &trans);
 	if (error)
 		goto out_destroy_fork;
+	xfs_defer_init(trans, &dfops);
 
 	lock_mode = XFS_ILOCK_EXCL;
 	xfs_ilock(dp, lock_mode);

fs/xfs/xfs_bmap_util.c +2 -0

index d3a314fd721f..ee9481b142f0 100644
@@ -792,6 +792,7 @@ xfs_free_eofblocks(
 	int			nimaps;
 	struct xfs_bmbt_irec	imap;
 	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_defer_ops	dfops;
 
 	/*
 	 * Figure out if there are any blocks beyond the end

@@ -831,6 +832,7 @@ xfs_free_eofblocks(
 			ASSERT(XFS_FORCED_SHUTDOWN(mp));
 			return error;
 		}
+		xfs_defer_init(tp, &dfops);
 
 		xfs_ilock(ip, XFS_ILOCK_EXCL);
 		xfs_trans_ijoin(tp, ip, 0);

fs/xfs/xfs_inode.c +3 -5

index 7b2694d3901a..7d7d7e95fa17 100644
@@ -1541,8 +1541,6 @@ xfs_itruncate_extents_flags(
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*tp = *tpp;
-	struct xfs_defer_ops	*odfops = tp->t_dfops;
-	struct xfs_defer_ops	dfops;
 	xfs_fileoff_t		first_unmap_block;
 	xfs_fileoff_t		last_block;
 	xfs_filblks_t		unmap_len;

@@ -1579,7 +1577,7 @@ xfs_itruncate_extents_flags(
 	ASSERT(first_unmap_block < last_block);
 	unmap_len = last_block - first_unmap_block + 1;
 	while (!done) {
-		xfs_defer_init(tp, &dfops);
+		ASSERT(tp->t_firstblock == NULLFSBLOCK);
 		error = xfs_bunmapi(tp, ip, first_unmap_block, unmap_len, flags,
 				    XFS_ITRUNC_MAX_EXTENTS, &done);
 		if (error)

@@ -1618,8 +1616,6 @@ xfs_itruncate_extents_flags(
 	trace_xfs_itruncate_extents_end(ip, new_size);
 
 out:
-	/* ->t_dfops points to local stack, don't leak it! */
-	tp->t_dfops = odfops;
 	*tpp = tp;
 	return error;
 out_bmap_cancel:

@@ -1723,6 +1719,7 @@ xfs_inactive_truncate(
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*tp;
+	struct xfs_defer_ops	dfops;
 	int			error;
 
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);

@@ -1730,6 +1727,7 @@ xfs_inactive_truncate(
 		ASSERT(XFS_FORCED_SHUTDOWN(mp));
 		return error;
 	}
+	xfs_defer_init(tp, &dfops);
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, ip, 0);

fs/xfs/xfs_iops.c +3 -0

index 0fa29f39d658..704b57a8b99e 100644
@@ -26,6 +26,7 @@
 #include "xfs_dir2.h"
 #include "xfs_trans_space.h"
 #include "xfs_iomap.h"
+#include "xfs_defer.h"
 
 #include <linux/capability.h>
 #include <linux/xattr.h>

@@ -812,6 +813,7 @@ xfs_setattr_size(
 	struct inode		*inode = VFS_I(ip);
 	xfs_off_t		oldsize, newsize;
 	struct xfs_trans	*tp;
+	struct xfs_defer_ops	dfops;
 	int			error;
 	uint			lock_flags = 0;
 	bool			did_zeroing = false;

@@ -915,6 +917,7 @@ xfs_setattr_size(
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
 	if (error)
 		return error;
+	xfs_defer_init(tp, &dfops);
 
 	lock_flags |= XFS_ILOCK_EXCL;
 	xfs_ilock(ip, XFS_ILOCK_EXCL);

fs/xfs/xfs_qm_syscalls.c +3 -0

index abc8a21e3a82..df0783303887 100644
@@ -22,6 +22,7 @@
 #include "xfs_qm.h"
 #include "xfs_trace.h"
 #include "xfs_icache.h"
+#include "xfs_defer.h"
 
 STATIC int	xfs_qm_log_quotaoff(xfs_mount_t *, xfs_qoff_logitem_t **, uint);
 STATIC int	xfs_qm_log_quotaoff_end(xfs_mount_t *, xfs_qoff_logitem_t *,

@@ -213,6 +214,7 @@ xfs_qm_scall_trunc_qfile(
 {
 	struct xfs_inode	*ip;
 	struct xfs_trans	*tp;
+	struct xfs_defer_ops	dfops;
 	int			error;
 
 	if (ino == NULLFSINO)

@@ -229,6 +231,7 @@ xfs_qm_scall_trunc_qfile(
 		xfs_iunlock(ip, XFS_IOLOCK_EXCL);
 		goto out_put;
 	}
+	xfs_defer_init(tp, &dfops);
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, ip, 0);

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

xlog_finish_defer_ops() processes the deferred operations collected over the entire intent recovery sequence. We can't xfs_defer_init() here because the dfops is already populated. Attach it manually and eliminate the last caller of xfs_defer_finish() that doesn't pass ->t_dfops.

Signed-off-by Brian Foster <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_log_recover.c | 2 ++
 1 file changed, 2 insertions(+)

fs/xfs/xfs_log_recover.c +2 -0

index cbac943896f4..3289811eb076 100644
@@ -4854,6 +4854,8 @@ xlog_finish_defer_ops(
 			0, XFS_TRANS_RESERVE, &tp);
 	if (error)
 		return error;
+	/* dfops is already populated so assign it manually */
+	tp->t_dfops = dfops;
 
 	error = xfs_defer_finish(&tp, dfops);
 	if (error)

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

xlog_finish_defer_ops() processes the deferred operations collected over the entire intent recovery sequence. We can't xfs_defer_init() here because the dfops is already populated. Attach it manually and eliminate the last caller of xfs_defer_finish() that doesn't pass ->t_dfops.

Signed-off-by Brian Foster <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_log_recover.c | 2 ++
 1 file changed, 2 insertions(+)

fs/xfs/xfs_log_recover.c +2 -0

index cbac943896f4..3289811eb076 100644
@@ -4854,6 +4854,8 @@ xlog_finish_defer_ops(
 			0, XFS_TRANS_RESERVE, &tp);
 	if (error)
 		return error;
+	/* dfops is already populated so assign it manually */
+	tp->t_dfops = dfops;
 
 	error = xfs_defer_finish(&tp, dfops);
 	if (error)

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

The xattr remote value set/remove handlers both clear args.trans in the error path without having cancelled the transaction. This leaks the transaction, causes warnings around returning to userspace with locks held and leads to system lockups or other general problems.

The higher level xfs_attr_[set|remove]() functions already detect and cancel args.trans when set in the error path. Drop the NULL assignments from the rmtval handlers and allow the callers to clean up the transaction correctly.

Signed-off-by Brian Foster <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/libxfs/xfs_attr_remote.c | 2 --
 1 file changed, 2 deletions(-)

fs/xfs/libxfs/xfs_attr_remote.c +0 -2

index 7841e6255129..829ab20f0cd7 100644
@@ -558,7 +558,6 @@ xfs_attr_rmtval_set(
 	return 0;
 out_defer_cancel:
 	xfs_defer_cancel(args->trans->t_dfops);
-	args->trans = NULL;
 	return error;
 }
 

@@ -646,6 +645,5 @@ xfs_attr_rmtval_remove(
 	return 0;
 out_defer_cancel:
 	xfs_defer_cancel(args->trans->t_dfops);
-	args->trans = NULL;
 	return error;
 }

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

The xattr remote value set/remove handlers both clear args.trans in the error path without having cancelled the transaction. This leaks the transaction, causes warnings around returning to userspace with locks held and leads to system lockups or other general problems.

The higher level xfs_attr_[set|remove]() functions already detect and cancel args.trans when set in the error path. Drop the NULL assignments from the rmtval handlers and allow the callers to clean up the transaction correctly.

Signed-off-by Brian Foster <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/libxfs/xfs_attr_remote.c | 2 --
 1 file changed, 2 deletions(-)

fs/xfs/libxfs/xfs_attr_remote.c +0 -2

index 7841e6255129..829ab20f0cd7 100644
@@ -558,7 +558,6 @@ xfs_attr_rmtval_set(
 	return 0;
 out_defer_cancel:
 	xfs_defer_cancel(args->trans->t_dfops);
-	args->trans = NULL;
 	return error;
 }
 

@@ -646,6 +645,5 @@ xfs_attr_rmtval_remove(
 	return 0;
 out_defer_cancel:
 	xfs_defer_cancel(args->trans->t_dfops);
-	args->trans = NULL;
 	return error;
 }

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

xfs_defer_finish() has a couple quirks that are not safe with respect to the upcoming internal dfops functionality. First, xfs_defer_finish() attaches the passed in dfops structure to ->t_dfops and caches and restores the original value. Second, it continues to use the initial dfops reference before and after the transaction roll.

These behaviors assume that dop is an independent memory allocation from the transaction itself, which may not always be true once transactions begin to use an embedded dfops structure. In the latter model, dfops processing creates a new xfs_defer_ops structure with each transaction and the associated state is migrated across to the new transaction.

Fix up xfs_defer_finish() to handle the possibility of the current dfops changing after a transaction roll. Since ->t_dfops is used unconditionally in this path, it is no longer necessary to attach/restore ->t_dfops and pass it explicitly down to xfs_defer_trans_roll(). Update dop in the latter function and the caller to ensure that it always refers to the current dfops structure.

Signed-off-by Brian Foster <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/libxfs/xfs_defer.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

fs/xfs/libxfs/xfs_defer.c +14 -18

index c4b0eaeb5190..ee734a8b3fa9 100644
@@ -225,9 +225,9 @@ xfs_defer_trans_abort(
 /* Roll a transaction so we can do some deferred op processing. */
 STATIC int
 xfs_defer_trans_roll(
-	struct xfs_trans		**tp,
-	struct xfs_defer_ops		*dop)
+	struct xfs_trans		**tp)
 {
+	struct xfs_defer_ops		*dop = (*tp)->t_dfops;
 	int				i;
 	int				error;
 

@@ -243,6 +243,7 @@ xfs_defer_trans_roll(
 
 	/* Roll the transaction. */
 	error = xfs_trans_roll(tp);
+	dop = (*tp)->t_dfops;
 	if (error) {
 		trace_xfs_defer_trans_roll_error((*tp)->t_mountp, dop, error);
 		xfs_defer_trans_abort(*tp, dop, error);

@@ -338,31 +339,25 @@ xfs_defer_finish(
 	void				*state;
 	int				error = 0;
 	void				(*cleanup_fn)(struct xfs_trans *, void *, int);
-	struct xfs_defer_ops		*orig_dop;
 
 	ASSERT((*tp)->t_flags & XFS_TRANS_PERM_LOG_RES);
+	ASSERT((*tp)->t_dfops == dop);
 
 	trace_xfs_defer_finish((*tp)->t_mountp, dop, _RET_IP_);
 
-	/*
-	 * Attach dfops to the transaction during deferred ops processing. This
-	 * explicitly causes calls into the allocator to defer AGFL block frees.
-	 * Note that this code can go away once all dfops users attach to the
-	 * associated tp.
-	 */
-	ASSERT(!(*tp)->t_dfops || ((*tp)->t_dfops == dop));
-	orig_dop = (*tp)->t_dfops;
-	(*tp)->t_dfops = dop;
-
 	/* Until we run out of pending work to finish... */
 	while (xfs_defer_has_unfinished_work(dop)) {
 		/* Log intents for work items sitting in the intake. */
 		xfs_defer_intake_work(*tp, dop);
 
-		/* Roll the transaction. */
-		error = xfs_defer_trans_roll(tp, dop);
+		/*
+		 * Roll the transaction and update dop in case dfops was
+		 * embedded in the transaction.
+		 */
+		error = xfs_defer_trans_roll(tp);
 		if (error)
 			goto out;
+		dop = (*tp)->t_dfops;
 
 		/* Log an intent-done item for the first pending item. */
 		dfp = list_first_entry(&dop->dop_pending,

@@ -428,10 +423,11 @@ xfs_defer_finish(
 	 * Roll the transaction once more to avoid returning to the caller
 	 * with a dirty transaction.
 	 */
-	if ((*tp)->t_flags & XFS_TRANS_DIRTY)
-		error = xfs_defer_trans_roll(tp, dop);
+	if ((*tp)->t_flags & XFS_TRANS_DIRTY) {
+		error = xfs_defer_trans_roll(tp);
+		dop = (*tp)->t_dfops;
+	}
 out:
-	(*tp)->t_dfops = orig_dop;
 	if (error)
 		trace_xfs_defer_finish_error((*tp)->t_mountp, dop, error);
 	else

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

xfs_defer_finish() has a couple quirks that are not safe with respect to the upcoming internal dfops functionality. First, xfs_defer_finish() attaches the passed in dfops structure to ->t_dfops and caches and restores the original value. Second, it continues to use the initial dfops reference before and after the transaction roll.

These behaviors assume that dop is an independent memory allocation from the transaction itself, which may not always be true once transactions begin to use an embedded dfops structure. In the latter model, dfops processing creates a new xfs_defer_ops structure with each transaction and the associated state is migrated across to the new transaction.

Fix up xfs_defer_finish() to handle the possibility of the current dfops changing after a transaction roll. Since ->t_dfops is used unconditionally in this path, it is no longer necessary to attach/restore ->t_dfops and pass it explicitly down to xfs_defer_trans_roll(). Update dop in the latter function and the caller to ensure that it always refers to the current dfops structure.

Signed-off-by Brian Foster <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/libxfs/xfs_defer.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

fs/xfs/libxfs/xfs_defer.c +14 -18

index c4b0eaeb5190..ee734a8b3fa9 100644
@@ -225,9 +225,9 @@ xfs_defer_trans_abort(
 /* Roll a transaction so we can do some deferred op processing. */
 STATIC int
 xfs_defer_trans_roll(
-	struct xfs_trans		**tp,
-	struct xfs_defer_ops		*dop)
+	struct xfs_trans		**tp)
 {
+	struct xfs_defer_ops		*dop = (*tp)->t_dfops;
 	int				i;
 	int				error;
 

@@ -243,6 +243,7 @@ xfs_defer_trans_roll(
 
 	/* Roll the transaction. */
 	error = xfs_trans_roll(tp);
+	dop = (*tp)->t_dfops;
 	if (error) {
 		trace_xfs_defer_trans_roll_error((*tp)->t_mountp, dop, error);
 		xfs_defer_trans_abort(*tp, dop, error);

@@ -338,31 +339,25 @@ xfs_defer_finish(
 	void				*state;
 	int				error = 0;
 	void				(*cleanup_fn)(struct xfs_trans *, void *, int);
-	struct xfs_defer_ops		*orig_dop;
 
 	ASSERT((*tp)->t_flags & XFS_TRANS_PERM_LOG_RES);
+	ASSERT((*tp)->t_dfops == dop);
 
 	trace_xfs_defer_finish((*tp)->t_mountp, dop, _RET_IP_);
 
-	/*
-	 * Attach dfops to the transaction during deferred ops processing. This
-	 * explicitly causes calls into the allocator to defer AGFL block frees.
-	 * Note that this code can go away once all dfops users attach to the
-	 * associated tp.
-	 */
-	ASSERT(!(*tp)->t_dfops || ((*tp)->t_dfops == dop));
-	orig_dop = (*tp)->t_dfops;
-	(*tp)->t_dfops = dop;
-
 	/* Until we run out of pending work to finish... */
 	while (xfs_defer_has_unfinished_work(dop)) {
 		/* Log intents for work items sitting in the intake. */
 		xfs_defer_intake_work(*tp, dop);
 
-		/* Roll the transaction. */
-		error = xfs_defer_trans_roll(tp, dop);
+		/*
+		 * Roll the transaction and update dop in case dfops was
+		 * embedded in the transaction.
+		 */
+		error = xfs_defer_trans_roll(tp);
 		if (error)
 			goto out;
+		dop = (*tp)->t_dfops;
 
 		/* Log an intent-done item for the first pending item. */
 		dfp = list_first_entry(&dop->dop_pending,

@@ -428,10 +423,11 @@ xfs_defer_finish(
 	 * Roll the transaction once more to avoid returning to the caller
 	 * with a dirty transaction.
 	 */
-	if ((*tp)->t_flags & XFS_TRANS_DIRTY)
-		error = xfs_defer_trans_roll(tp, dop);
+	if ((*tp)->t_flags & XFS_TRANS_DIRTY) {
+		error = xfs_defer_trans_roll(tp);
+		dop = (*tp)->t_dfops;
+	}
 out:
-	(*tp)->t_dfops = orig_dop;
 	if (error)
 		trace_xfs_defer_finish_error((*tp)->t_mountp, dop, error);
 	else

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

dop_committed is set when deferred item processing rolls the transaction at least once, but is only ever accessed in tracepoints. The transaction roll/commit events are already available via independent tracepoints, so remove the otherwise unused field.

Signed-off-by Brian Foster <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/libxfs/xfs_defer.c | 1 -
 fs/xfs/libxfs/xfs_defer.h | 1 -
 fs/xfs/xfs_trace.h        | 8 ++------
 3 files changed, 2 insertions(+), 8 deletions(-)

fs/xfs/libxfs/xfs_defer.c +0 -1

index ee734a8b3fa9..0df09c094e42 100644
@@ -249,7 +249,6 @@ xfs_defer_trans_roll(
 		xfs_defer_trans_abort(*tp, dop, error);
 		return error;
 	}
-	dop->dop_committed = true;
 
 	/* Rejoin the joined inodes. */
 	for (i = 0; i < XFS_DEFER_OPS_NR_INODES && dop->dop_inodes[i]; i++)

fs/xfs/libxfs/xfs_defer.h +0 -1

index c17c9deda995..58c979c9f3fa 100644
@@ -49,7 +49,6 @@ enum xfs_defer_ops_type {
 #define XFS_DEFER_OPS_NR_BUFS	2	/* join up to two buffers */
 
 struct xfs_defer_ops {
-	bool			dop_committed;	/* did any trans commit? */
 	bool			dop_low;	/* alloc in low mode */
 	struct list_head	dop_intake;	/* unlogged pending work */
 	struct list_head	dop_pending;	/* logged pending work */

fs/xfs/xfs_trace.h +2 -6

index b668fc127aa7..cc6995cfce66 100644
@@ -2229,14 +2229,12 @@ DECLARE_EVENT_CLASS(xfs_defer_class,
 	TP_fast_assign(
 		__entry->dev = mp ? mp->m_super->s_dev : 0;
 		__entry->dop = dop;
-		__entry->committed = dop->dop_committed;
 		__entry->low = dop->dop_low;
 		__entry->caller_ip = caller_ip;
 	),
-	TP_printk("dev %d:%d ops %p committed %d low %d, caller %pS",
+	TP_printk("dev %d:%d ops %p low %d, caller %pS",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->dop,
-		  __entry->committed,
 		  __entry->low,
 		  (char *)__entry->caller_ip)
 )

@@ -2259,14 +2257,12 @@ DECLARE_EVENT_CLASS(xfs_defer_error_class,
 	TP_fast_assign(
 		__entry->dev = mp ? mp->m_super->s_dev : 0;
 		__entry->dop = dop;
-		__entry->committed = dop->dop_committed;
 		__entry->low = dop->dop_low;
 		__entry->error = error;
 	),
-	TP_printk("dev %d:%d ops %p committed %d low %d err %d",
+	TP_printk("dev %d:%d ops %p low %d err %d",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->dop,
-		  __entry->committed,
 		  __entry->low,
 		  __entry->error)
 )

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

dop_committed is set when deferred item processing rolls the transaction at least once, but is only ever accessed in tracepoints. The transaction roll/commit events are already available via independent tracepoints, so remove the otherwise unused field.

Signed-off-by Brian Foster <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/libxfs/xfs_defer.c | 1 -
 fs/xfs/libxfs/xfs_defer.h | 1 -
 fs/xfs/xfs_trace.h        | 8 ++------
 3 files changed, 2 insertions(+), 8 deletions(-)

fs/xfs/libxfs/xfs_defer.c +0 -1

index ee734a8b3fa9..0df09c094e42 100644
@@ -249,7 +249,6 @@ xfs_defer_trans_roll(
 		xfs_defer_trans_abort(*tp, dop, error);
 		return error;
 	}
-	dop->dop_committed = true;
 
 	/* Rejoin the joined inodes. */
 	for (i = 0; i < XFS_DEFER_OPS_NR_INODES && dop->dop_inodes[i]; i++)

fs/xfs/libxfs/xfs_defer.h +0 -1

index c17c9deda995..58c979c9f3fa 100644
@@ -49,7 +49,6 @@ enum xfs_defer_ops_type {
 #define XFS_DEFER_OPS_NR_BUFS	2	/* join up to two buffers */
 
 struct xfs_defer_ops {
-	bool			dop_committed;	/* did any trans commit? */
 	bool			dop_low;	/* alloc in low mode */
 	struct list_head	dop_intake;	/* unlogged pending work */
 	struct list_head	dop_pending;	/* logged pending work */

fs/xfs/xfs_trace.h +2 -6

index b668fc127aa7..cc6995cfce66 100644
@@ -2229,14 +2229,12 @@ DECLARE_EVENT_CLASS(xfs_defer_class,
 	TP_fast_assign(
 		__entry->dev = mp ? mp->m_super->s_dev : 0;
 		__entry->dop = dop;
-		__entry->committed = dop->dop_committed;
 		__entry->low = dop->dop_low;
 		__entry->caller_ip = caller_ip;
 	),
-	TP_printk("dev %d:%d ops %p committed %d low %d, caller %pS",
+	TP_printk("dev %d:%d ops %p low %d, caller %pS",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->dop,
-		  __entry->committed,
 		  __entry->low,
 		  (char *)__entry->caller_ip)
 )

@@ -2259,14 +2257,12 @@ DECLARE_EVENT_CLASS(xfs_defer_error_class,
 	TP_fast_assign(
 		__entry->dev = mp ? mp->m_super->s_dev : 0;
 		__entry->dop = dop;
-		__entry->committed = dop->dop_committed;
 		__entry->low = dop->dop_low;
 		__entry->error = error;
 	),
-	TP_printk("dev %d:%d ops %p committed %d low %d err %d",
+	TP_printk("dev %d:%d ops %p low %d err %d",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->dop,
-		  __entry->committed,
 		  __entry->low,
 		  __entry->error)
 )

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

xfs_defer_init() is currently used in two particular situations. The first and most obvious case is raw initialization of an xfs_defer_ops struct. The other case is partial reinit of xfs_defer_ops on reuse due to iteration.

Most instances of the first case will be replaced by a single init of a dfops embedded in the transaction. Init calls are still technically required for the second case because the dfops may have low space mode enabled or have joined items that need to be reset before the dfops should be reused.

Since the current dfops usage expects either a final transaction commit after xfs_defer_finish() or xfs_defer_init() if dfops is to be reused, we can shift some of the init logic into xfs_defer_finish() such that the latter returns with a reinitialized dfops. This eliminates the second dependency noted above such that a dfops is immediately ready for reuse after an xfs_defer_finish() without the need to change any calling code.

Signed-off-by Brian Foster <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/libxfs/xfs_defer.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

fs/xfs/libxfs/xfs_defer.c +18 -2

index 0df09c094e42..23f2a52b088e 100644
@@ -319,6 +319,19 @@ xfs_defer_bjoin(
 	return -EFSCORRUPTED;
 }
 
+/*
+ * Reset an already used dfops after finish.
+ */
+static void
+xfs_defer_reset(
+	struct xfs_defer_ops	*dop)
+{
+	ASSERT(!xfs_defer_has_unfinished_work(dop));
+	dop->dop_low = false;
+	memset(dop->dop_inodes, 0, sizeof(dop->dop_inodes));
+	memset(dop->dop_bufs, 0, sizeof(dop->dop_bufs));
+}
+
 /*
  * Finish all the pending work.  This involves logging intent items for
  * any work items that wandered in since the last transaction roll (if

@@ -427,10 +440,13 @@ xfs_defer_finish(
 		dop = (*tp)->t_dfops;
 	}
 out:
-	if (error)
+	if (error) {
 		trace_xfs_defer_finish_error((*tp)->t_mountp, dop, error);
-	else
+	} else {
 		trace_xfs_defer_finish_done((*tp)->t_mountp, dop, _RET_IP_);
+		xfs_defer_reset(dop);
+	}
+
 	return error;
 }
 

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

xfs_defer_init() is currently used in two particular situations. The first and most obvious case is raw initialization of an xfs_defer_ops struct. The other case is partial reinit of xfs_defer_ops on reuse due to iteration.

Most instances of the first case will be replaced by a single init of a dfops embedded in the transaction. Init calls are still technically required for the second case because the dfops may have low space mode enabled or have joined items that need to be reset before the dfops should be reused.

Since the current dfops usage expects either a final transaction commit after xfs_defer_finish() or xfs_defer_init() if dfops is to be reused, we can shift some of the init logic into xfs_defer_finish() such that the latter returns with a reinitialized dfops. This eliminates the second dependency noted above such that a dfops is immediately ready for reuse after an xfs_defer_finish() without the need to change any calling code.

Signed-off-by Brian Foster <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/libxfs/xfs_defer.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

fs/xfs/libxfs/xfs_defer.c +18 -2

index 0df09c094e42..23f2a52b088e 100644
@@ -319,6 +319,19 @@ xfs_defer_bjoin(
 	return -EFSCORRUPTED;
 }
 
+/*
+ * Reset an already used dfops after finish.
+ */
+static void
+xfs_defer_reset(
+	struct xfs_defer_ops	*dop)
+{
+	ASSERT(!xfs_defer_has_unfinished_work(dop));
+	dop->dop_low = false;
+	memset(dop->dop_inodes, 0, sizeof(dop->dop_inodes));
+	memset(dop->dop_bufs, 0, sizeof(dop->dop_bufs));
+}
+
 /*
  * Finish all the pending work.  This involves logging intent items for
  * any work items that wandered in since the last transaction roll (if

@@ -427,10 +440,13 @@ xfs_defer_finish(
 		dop = (*tp)->t_dfops;
 	}
 out:
-	if (error)
+	if (error) {
 		trace_xfs_defer_finish_error((*tp)->t_mountp, dop, error);
-	else
+	} else {
 		trace_xfs_defer_finish_done((*tp)->t_mountp, dop, _RET_IP_);
+		xfs_defer_reset(dop);
+	}
+
 	return error;
 }
 

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Both structures have holes due to member alignment. Move dop_low to the end of xfs_defer ops to sanitize the cache line alignment and move t_flags to save 8 bytes in xfs_trans.

Signed-off-by Brian Foster <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/libxfs/xfs_defer.h | 3 ++-
 fs/xfs/xfs_trans.h        | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

fs/xfs/libxfs/xfs_defer.h +2 -1

index 58c979c9f3fa..8f58f217fdff 100644
@@ -49,13 +49,14 @@ enum xfs_defer_ops_type {
 #define XFS_DEFER_OPS_NR_BUFS	2	/* join up to two buffers */
 
 struct xfs_defer_ops {
-	bool			dop_low;	/* alloc in low mode */
 	struct list_head	dop_intake;	/* unlogged pending work */
 	struct list_head	dop_pending;	/* logged pending work */
 
 	/* relog these with each roll */
 	struct xfs_inode	*dop_inodes[XFS_DEFER_OPS_NR_INODES];
 	struct xfs_buf		*dop_bufs[XFS_DEFER_OPS_NR_BUFS];
+
+	bool			dop_low;	/* alloc in low mode */
 };
 
 void xfs_defer_add(struct xfs_defer_ops *dop, enum xfs_defer_ops_type type,

fs/xfs/xfs_trans.h +1 -1

index 37fdacc690c7..6f857af61455 100644
@@ -102,12 +102,12 @@ typedef struct xfs_trans {
 	unsigned int		t_blk_res_used;	/* # of resvd blocks used */
 	unsigned int		t_rtx_res;	/* # of rt extents resvd */
 	unsigned int		t_rtx_res_used;	/* # of resvd rt extents used */
+	unsigned int		t_flags;	/* misc flags */
 	xfs_fsblock_t		t_firstblock;	/* first block allocated */
 	struct xlog_ticket	*t_ticket;	/* log mgr ticket */
 	struct xfs_mount	*t_mountp;	/* ptr to fs mount struct */
 	struct xfs_dquot_acct   *t_dqinfo;	/* acctg info for dquots */
 	struct xfs_defer_ops	*t_dfops;	/* dfops reference */
-	unsigned int		t_flags;	/* misc flags */
 	int64_t			t_icount_delta;	/* superblock icount change */
 	int64_t			t_ifree_delta;	/* superblock ifree change */
 	int64_t			t_fdblocks_delta; /* superblock fdblocks chg */

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Both structures have holes due to member alignment. Move dop_low to the end of xfs_defer ops to sanitize the cache line alignment and move t_flags to save 8 bytes in xfs_trans.

Signed-off-by Brian Foster <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/libxfs/xfs_defer.h | 3 ++-
 fs/xfs/xfs_trans.h        | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

fs/xfs/libxfs/xfs_defer.h +2 -1

index 58c979c9f3fa..8f58f217fdff 100644
@@ -49,13 +49,14 @@ enum xfs_defer_ops_type {
 #define XFS_DEFER_OPS_NR_BUFS	2	/* join up to two buffers */
 
 struct xfs_defer_ops {
-	bool			dop_low;	/* alloc in low mode */
 	struct list_head	dop_intake;	/* unlogged pending work */
 	struct list_head	dop_pending;	/* logged pending work */
 
 	/* relog these with each roll */
 	struct xfs_inode	*dop_inodes[XFS_DEFER_OPS_NR_INODES];
 	struct xfs_buf		*dop_bufs[XFS_DEFER_OPS_NR_BUFS];
+
+	bool			dop_low;	/* alloc in low mode */
 };
 
 void xfs_defer_add(struct xfs_defer_ops *dop, enum xfs_defer_ops_type type,

fs/xfs/xfs_trans.h +1 -1

index 37fdacc690c7..6f857af61455 100644
@@ -102,12 +102,12 @@ typedef struct xfs_trans {
 	unsigned int		t_blk_res_used;	/* # of resvd blocks used */
 	unsigned int		t_rtx_res;	/* # of rt extents resvd */
 	unsigned int		t_rtx_res_used;	/* # of resvd rt extents used */
+	unsigned int		t_flags;	/* misc flags */
 	xfs_fsblock_t		t_firstblock;	/* first block allocated */
 	struct xlog_ticket	*t_ticket;	/* log mgr ticket */
 	struct xfs_mount	*t_mountp;	/* ptr to fs mount struct */
 	struct xfs_dquot_acct   *t_dqinfo;	/* acctg info for dquots */
 	struct xfs_defer_ops	*t_dfops;	/* dfops reference */
-	unsigned int		t_flags;	/* misc flags */
 	int64_t			t_icount_delta;	/* superblock icount change */
 	int64_t			t_ifree_delta;	/* superblock ifree change */
 	int64_t			t_fdblocks_delta; /* superblock fdblocks chg */

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

The dfops structure used by multi-transaction operations is typically stored on the stack and carried around by the associated transaction. The lifecycle of dfops does not quite match that of the transaction, but they are tightly related in that the former depends on the latter.

The relationship of these objects is tight enough that we can avoid the cumbersome boilerplate code required in most cases to manage them separately by just embedding an xfs_defer_ops in the transaction itself. This means that a transaction allocation returns with an initialized dfops, a transaction commit finishes pending deferred items before the tx commit, a transaction cancel cancels the dfops before the transaction and a transaction dup operation transfers the current dfops state to the new transaction.

The dup operation is slightly complicated by the fact that we can no longer just copy a dfops pointer from the old transaction to the new transaction. This is solved through a dfops move helper that transfers the pending items and other dfops state across the transactions. This also requires that transaction rolling code always refer to the transaction for the current dfops reference.

Finally, to facilitate incremental conversion to the internal dfops and continue to support the current external dfops mode of operation, create the new ->t_dfops_internal field with a layer of indirection. On allocation, ->t_dfops points to the internal dfops. This state is overridden by callers who re-init a local dfops on the transaction. Once ->t_dfops is overridden, the external dfops reference is maintained as the transaction rolls.

This patch adds the fundamental ability to support an internal dfops. All codepaths that perform deferred processing continue to override the internal dfops until they are converted over in subsequent patches.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/libxfs/xfs_defer.c | 22 ++++++++++++++++++++++
 fs/xfs/libxfs/xfs_defer.h | 16 ++--------------
 fs/xfs/xfs_trans.c        | 30 ++++++++++++++++++++++++++----
 fs/xfs/xfs_trans.h        | 17 ++++++++++++++++-
 4 files changed, 66 insertions(+), 19 deletions(-)

fs/xfs/libxfs/xfs_defer.c +22 -0

index 23f2a52b088e..b63cc9e730da 100644
@@ -555,3 +555,25 @@ xfs_defer_init(
 	}
 	trace_xfs_defer_init(mp, dop, _RET_IP_);
 }
+
+/*
+ * Move state from one xfs_defer_ops to another and reset the source to initial
+ * state. This is primarily used to carry state forward across transaction rolls
+ * with internal dfops.
+ */
+void
+xfs_defer_move(
+	struct xfs_defer_ops	*dst,
+	struct xfs_defer_ops	*src)
+{
+	ASSERT(dst != src);
+
+	list_splice_init(&src->dop_intake, &dst->dop_intake);
+	list_splice_init(&src->dop_pending, &dst->dop_pending);
+
+	memcpy(dst->dop_inodes, src->dop_inodes, sizeof(dst->dop_inodes));
+	memcpy(dst->dop_bufs, src->dop_bufs, sizeof(dst->dop_bufs));
+	dst->dop_low = src->dop_low;
+
+	xfs_defer_reset(src);
+}

fs/xfs/libxfs/xfs_defer.h +2 -14

index 8f58f217fdff..35507ca9a148 100644
@@ -7,6 +7,7 @@
 #define	__XFS_DEFER_H__
 
 struct xfs_defer_op_type;
+struct xfs_defer_ops;
 
 /*
  * Save a log intent item and a list of extents, so that we can replay

@@ -45,20 +46,6 @@ enum xfs_defer_ops_type {
 	XFS_DEFER_OPS_TYPE_MAX,
 };
 
-#define XFS_DEFER_OPS_NR_INODES	2	/* join up to two inodes */
-#define XFS_DEFER_OPS_NR_BUFS	2	/* join up to two buffers */
-
-struct xfs_defer_ops {
-	struct list_head	dop_intake;	/* unlogged pending work */
-	struct list_head	dop_pending;	/* logged pending work */
-
-	/* relog these with each roll */
-	struct xfs_inode	*dop_inodes[XFS_DEFER_OPS_NR_INODES];
-	struct xfs_buf		*dop_bufs[XFS_DEFER_OPS_NR_BUFS];
-
-	bool			dop_low;	/* alloc in low mode */
-};
-
 void xfs_defer_add(struct xfs_defer_ops *dop, enum xfs_defer_ops_type type,
 		struct list_head *h);
 int xfs_defer_finish(struct xfs_trans **tp, struct xfs_defer_ops *dop);

@@ -67,6 +54,7 @@ void xfs_defer_init(struct xfs_trans *tp, struct xfs_defer_ops *dop);
 bool xfs_defer_has_unfinished_work(struct xfs_defer_ops *dop);
 int xfs_defer_ijoin(struct xfs_defer_ops *dop, struct xfs_inode *ip);
 int xfs_defer_bjoin(struct xfs_defer_ops *dop, struct xfs_buf *bp);
+void xfs_defer_move(struct xfs_defer_ops *dst, struct xfs_defer_ops *src);
 
 /* Description of a deferred type. */
 struct xfs_defer_op_type {

fs/xfs/xfs_trans.c +26 -4

index de00f79ff698..412c8d236c71 100644
@@ -119,7 +119,13 @@ xfs_trans_dup(
 	ntp->t_rtx_res = tp->t_rtx_res - tp->t_rtx_res_used;
 	tp->t_rtx_res = tp->t_rtx_res_used;
 	ntp->t_pflags = tp->t_pflags;
-	ntp->t_dfops = tp->t_dfops;
+
+	/* copy the dfops pointer if it's external, otherwise move it */
+	xfs_defer_init(ntp, &ntp->t_dfops_internal);
+	if (tp->t_dfops != &tp->t_dfops_internal)
+		ntp->t_dfops = tp->t_dfops;
+	else
+		xfs_defer_move(ntp->t_dfops, tp->t_dfops);
 
 	xfs_trans_dup_dqinfo(tp, ntp);
 

@@ -275,6 +281,13 @@ xfs_trans_alloc(
 	INIT_LIST_HEAD(&tp->t_items);
 	INIT_LIST_HEAD(&tp->t_busy);
 	tp->t_firstblock = NULLFSBLOCK;
+	/*
+	 * We only roll transactions with permanent log reservation. Don't init
+	 * ->t_dfops to skip attempts to finish or cancel an empty dfops with a
+	 * non-permanent res.
+	 */
+	if (resp->tr_logflags & XFS_TRANS_PERM_LOG_RES)
+		xfs_defer_init(tp, &tp->t_dfops_internal);
 
 	error = xfs_trans_reserve(tp, resp, blocks, rtextents);
 	if (error) {

@@ -916,11 +929,17 @@ __xfs_trans_commit(
 	int			error = 0;
 	int			sync = tp->t_flags & XFS_TRANS_SYNC;
 
-	ASSERT(!tp->t_dfops ||
-	       !xfs_defer_has_unfinished_work(tp->t_dfops) || regrant);
-
 	trace_xfs_trans_commit(tp, _RET_IP_);
 
+	/* finish deferred items on final commit */
+	if (!regrant && tp->t_dfops) {
+		error = xfs_defer_finish(&tp, tp->t_dfops);
+		if (error) {
+			xfs_defer_cancel(tp->t_dfops);
+			goto out_unreserve;
+		}
+	}
+
 	/*
 	 * If there is nothing to be logged by the transaction,
 	 * then unlock all of the items associated with the

@@ -1010,6 +1029,9 @@ xfs_trans_cancel(
 
 	trace_xfs_trans_cancel(tp, _RET_IP_);
 
+	if (tp->t_dfops)
+		xfs_defer_cancel(tp->t_dfops);
+
 	/*
 	 * See if the caller is relying on us to shut down the
 	 * filesystem.  This happens in paths where we detect

fs/xfs/xfs_trans.h +16 -1

index 6f857af61455..dc79e3c1d3e8 100644
@@ -24,7 +24,6 @@ struct xfs_rui_log_item;
 struct xfs_btree_cur;
 struct xfs_cui_log_item;
 struct xfs_cud_log_item;
-struct xfs_defer_ops;
 struct xfs_bui_log_item;
 struct xfs_bud_log_item;
 

@@ -90,6 +89,21 @@ void	xfs_log_item_init(struct xfs_mount *mp, struct xfs_log_item *item,
 #define XFS_ITEM_LOCKED		2
 #define XFS_ITEM_FLUSHING	3
 
+/*
+ * Deferred operations tracking structure.
+ */
+#define XFS_DEFER_OPS_NR_INODES	2	/* join up to two inodes */
+#define XFS_DEFER_OPS_NR_BUFS	2	/* join up to two buffers */
+struct xfs_defer_ops {
+	struct list_head	dop_intake;	/* unlogged pending work */
+	struct list_head	dop_pending;	/* logged pending work */
+
+	/* relog these with each roll */
+	struct xfs_inode	*dop_inodes[XFS_DEFER_OPS_NR_INODES];
+	struct xfs_buf		*dop_bufs[XFS_DEFER_OPS_NR_BUFS];
+
+	bool			dop_low;	/* alloc in low mode */
+};
 
 /*
  * This is the structure maintained for every active transaction.

@@ -130,6 +144,7 @@ typedef struct xfs_trans {
 	struct list_head	t_items;	/* log item descriptors */
 	struct list_head	t_busy;		/* list of busy extents */
 	unsigned long		t_pflags;	/* saved process flags state */
+	struct xfs_defer_ops	t_dfops_internal;
 } xfs_trans_t;
 
 /*

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

The dfops structure used by multi-transaction operations is typically stored on the stack and carried around by the associated transaction. The lifecycle of dfops does not quite match that of the transaction, but they are tightly related in that the former depends on the latter.

The relationship of these objects is tight enough that we can avoid the cumbersome boilerplate code required in most cases to manage them separately by just embedding an xfs_defer_ops in the transaction itself. This means that a transaction allocation returns with an initialized dfops, a transaction commit finishes pending deferred items before the tx commit, a transaction cancel cancels the dfops before the transaction and a transaction dup operation transfers the current dfops state to the new transaction.

The dup operation is slightly complicated by the fact that we can no longer just copy a dfops pointer from the old transaction to the new transaction. This is solved through a dfops move helper that transfers the pending items and other dfops state across the transactions. This also requires that transaction rolling code always refer to the transaction for the current dfops reference.

Finally, to facilitate incremental conversion to the internal dfops and continue to support the current external dfops mode of operation, create the new ->t_dfops_internal field with a layer of indirection. On allocation, ->t_dfops points to the internal dfops. This state is overridden by callers who re-init a local dfops on the transaction. Once ->t_dfops is overridden, the external dfops reference is maintained as the transaction rolls.

This patch adds the fundamental ability to support an internal dfops. All codepaths that perform deferred processing continue to override the internal dfops until they are converted over in subsequent patches.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/libxfs/xfs_defer.c | 22 ++++++++++++++++++++++
 fs/xfs/libxfs/xfs_defer.h | 16 ++--------------
 fs/xfs/xfs_trans.c        | 30 ++++++++++++++++++++++++++----
 fs/xfs/xfs_trans.h        | 17 ++++++++++++++++-
 4 files changed, 66 insertions(+), 19 deletions(-)

fs/xfs/libxfs/xfs_defer.c +22 -0

index 23f2a52b088e..b63cc9e730da 100644
@@ -555,3 +555,25 @@ xfs_defer_init(
 	}
 	trace_xfs_defer_init(mp, dop, _RET_IP_);
 }
+
+/*
+ * Move state from one xfs_defer_ops to another and reset the source to initial
+ * state. This is primarily used to carry state forward across transaction rolls
+ * with internal dfops.
+ */
+void
+xfs_defer_move(
+	struct xfs_defer_ops	*dst,
+	struct xfs_defer_ops	*src)
+{
+	ASSERT(dst != src);
+
+	list_splice_init(&src->dop_intake, &dst->dop_intake);
+	list_splice_init(&src->dop_pending, &dst->dop_pending);
+
+	memcpy(dst->dop_inodes, src->dop_inodes, sizeof(dst->dop_inodes));
+	memcpy(dst->dop_bufs, src->dop_bufs, sizeof(dst->dop_bufs));
+	dst->dop_low = src->dop_low;
+
+	xfs_defer_reset(src);
+}

fs/xfs/libxfs/xfs_defer.h +2 -14

index 8f58f217fdff..35507ca9a148 100644
@@ -7,6 +7,7 @@
 #define	__XFS_DEFER_H__
 
 struct xfs_defer_op_type;
+struct xfs_defer_ops;
 
 /*
  * Save a log intent item and a list of extents, so that we can replay

@@ -45,20 +46,6 @@ enum xfs_defer_ops_type {
 	XFS_DEFER_OPS_TYPE_MAX,
 };
 
-#define XFS_DEFER_OPS_NR_INODES	2	/* join up to two inodes */
-#define XFS_DEFER_OPS_NR_BUFS	2	/* join up to two buffers */
-
-struct xfs_defer_ops {
-	struct list_head	dop_intake;	/* unlogged pending work */
-	struct list_head	dop_pending;	/* logged pending work */
-
-	/* relog these with each roll */
-	struct xfs_inode	*dop_inodes[XFS_DEFER_OPS_NR_INODES];
-	struct xfs_buf		*dop_bufs[XFS_DEFER_OPS_NR_BUFS];
-
-	bool			dop_low;	/* alloc in low mode */
-};
-
 void xfs_defer_add(struct xfs_defer_ops *dop, enum xfs_defer_ops_type type,
 		struct list_head *h);
 int xfs_defer_finish(struct xfs_trans **tp, struct xfs_defer_ops *dop);

@@ -67,6 +54,7 @@ void xfs_defer_init(struct xfs_trans *tp, struct xfs_defer_ops *dop);
 bool xfs_defer_has_unfinished_work(struct xfs_defer_ops *dop);
 int xfs_defer_ijoin(struct xfs_defer_ops *dop, struct xfs_inode *ip);
 int xfs_defer_bjoin(struct xfs_defer_ops *dop, struct xfs_buf *bp);
+void xfs_defer_move(struct xfs_defer_ops *dst, struct xfs_defer_ops *src);
 
 /* Description of a deferred type. */
 struct xfs_defer_op_type {

fs/xfs/xfs_trans.c +26 -4

index de00f79ff698..412c8d236c71 100644
@@ -119,7 +119,13 @@ xfs_trans_dup(
 	ntp->t_rtx_res = tp->t_rtx_res - tp->t_rtx_res_used;
 	tp->t_rtx_res = tp->t_rtx_res_used;
 	ntp->t_pflags = tp->t_pflags;
-	ntp->t_dfops = tp->t_dfops;
+
+	/* copy the dfops pointer if it's external, otherwise move it */
+	xfs_defer_init(ntp, &ntp->t_dfops_internal);
+	if (tp->t_dfops != &tp->t_dfops_internal)
+		ntp->t_dfops = tp->t_dfops;
+	else
+		xfs_defer_move(ntp->t_dfops, tp->t_dfops);
 
 	xfs_trans_dup_dqinfo(tp, ntp);
 

@@ -275,6 +281,13 @@ xfs_trans_alloc(
 	INIT_LIST_HEAD(&tp->t_items);
 	INIT_LIST_HEAD(&tp->t_busy);
 	tp->t_firstblock = NULLFSBLOCK;
+	/*
+	 * We only roll transactions with permanent log reservation. Don't init
+	 * ->t_dfops to skip attempts to finish or cancel an empty dfops with a
+	 * non-permanent res.
+	 */
+	if (resp->tr_logflags & XFS_TRANS_PERM_LOG_RES)
+		xfs_defer_init(tp, &tp->t_dfops_internal);
 
 	error = xfs_trans_reserve(tp, resp, blocks, rtextents);
 	if (error) {

@@ -916,11 +929,17 @@ __xfs_trans_commit(
 	int			error = 0;
 	int			sync = tp->t_flags & XFS_TRANS_SYNC;
 
-	ASSERT(!tp->t_dfops ||
-	       !xfs_defer_has_unfinished_work(tp->t_dfops) || regrant);
-
 	trace_xfs_trans_commit(tp, _RET_IP_);
 
+	/* finish deferred items on final commit */
+	if (!regrant && tp->t_dfops) {
+		error = xfs_defer_finish(&tp, tp->t_dfops);
+		if (error) {
+			xfs_defer_cancel(tp->t_dfops);
+			goto out_unreserve;
+		}
+	}
+
 	/*
 	 * If there is nothing to be logged by the transaction,
 	 * then unlock all of the items associated with the

@@ -1010,6 +1029,9 @@ xfs_trans_cancel(
 
 	trace_xfs_trans_cancel(tp, _RET_IP_);
 
+	if (tp->t_dfops)
+		xfs_defer_cancel(tp->t_dfops);
+
 	/*
 	 * See if the caller is relying on us to shut down the
 	 * filesystem.  This happens in paths where we detect

fs/xfs/xfs_trans.h +16 -1

index 6f857af61455..dc79e3c1d3e8 100644
@@ -24,7 +24,6 @@ struct xfs_rui_log_item;
 struct xfs_btree_cur;
 struct xfs_cui_log_item;
 struct xfs_cud_log_item;
-struct xfs_defer_ops;
 struct xfs_bui_log_item;
 struct xfs_bud_log_item;
 

@@ -90,6 +89,21 @@ void	xfs_log_item_init(struct xfs_mount *mp, struct xfs_log_item *item,
 #define XFS_ITEM_LOCKED		2
 #define XFS_ITEM_FLUSHING	3
 
+/*
+ * Deferred operations tracking structure.
+ */
+#define XFS_DEFER_OPS_NR_INODES	2	/* join up to two inodes */
+#define XFS_DEFER_OPS_NR_BUFS	2	/* join up to two buffers */
+struct xfs_defer_ops {
+	struct list_head	dop_intake;	/* unlogged pending work */
+	struct list_head	dop_pending;	/* logged pending work */
+
+	/* relog these with each roll */
+	struct xfs_inode	*dop_inodes[XFS_DEFER_OPS_NR_INODES];
+	struct xfs_buf		*dop_bufs[XFS_DEFER_OPS_NR_BUFS];
+
+	bool			dop_low;	/* alloc in low mode */
+};
 
 /*
  * This is the structure maintained for every active transaction.

@@ -130,6 +144,7 @@ typedef struct xfs_trans {
 	struct list_head	t_items;	/* log item descriptors */
 	struct list_head	t_busy;		/* list of busy extents */
 	unsigned long		t_pflags;	/* saved process flags state */
+	struct xfs_defer_ops	t_dfops_internal;
 } xfs_trans_t;
 
 /*

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

The dfops structure used by multi-transaction operations is typically stored on the stack and carried around by the associated transaction. The lifecycle of dfops does not quite match that of the transaction, but they are tightly related in that the former depends on the latter.

The relationship of these objects is tight enough that we can avoid the cumbersome boilerplate code required in most cases to manage them separately by just embedding an xfs_defer_ops in the transaction itself. This means that a transaction allocation returns with an initialized dfops, a transaction commit finishes pending deferred items before the tx commit, a transaction cancel cancels the dfops before the transaction and a transaction dup operation transfers the current dfops state to the new transaction.

The dup operation is slightly complicated by the fact that we can no longer just copy a dfops pointer from the old transaction to the new transaction. This is solved through a dfops move helper that transfers the pending items and other dfops state across the transactions. This also requires that transaction rolling code always refer to the transaction for the current dfops reference.

Finally, to facilitate incremental conversion to the internal dfops and continue to support the current external dfops mode of operation, create the new ->t_dfops_internal field with a layer of indirection. On allocation, ->t_dfops points to the internal dfops. This state is overridden by callers who re-init a local dfops on the transaction. Once ->t_dfops is overridden, the external dfops reference is maintained as the transaction rolls.

This patch adds the fundamental ability to support an internal dfops. All codepaths that perform deferred processing continue to override the internal dfops until they are converted over in subsequent patches.

Signed-off-by Brian Foster <[email protected]>

All callers either explicitly initialize a dfops or pass a transaction with an internal dfops. Drop the hacky old dfops replacement logic and use the one associated with the transaction.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/xfs_reflink.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

fs/xfs/xfs_reflink.c +2 -4

index 3143889097f1..c3f2a0d3e6f5 100644
@@ -483,8 +483,6 @@ xfs_reflink_cancel_cow_blocks(
 	struct xfs_ifork		*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
 	struct xfs_bmbt_irec		got, del;
 	struct xfs_iext_cursor		icur;
-	struct xfs_defer_ops		dfops;
-	struct xfs_defer_ops		*odfops = (*tpp)->t_dfops;
 	int				error = 0;
 
 	if (!xfs_is_reflink_inode(ip))

@@ -511,7 +509,8 @@ xfs_reflink_cancel_cow_blocks(
 			if (error)
 				break;
 		} else if (del.br_state == XFS_EXT_UNWRITTEN || cancel_real) {
-			xfs_defer_init(*tpp, &dfops);
+			ASSERT((*tpp)->t_dfops);
+			ASSERT((*tpp)->t_firstblock == NULLFSBLOCK);
 
 			/* Free the CoW orphan record. */
 			error = xfs_refcount_free_cow_extent(ip->i_mount,

@@ -553,7 +552,6 @@ xfs_reflink_cancel_cow_blocks(
 	/* clear tag if cow fork is emptied */
 	if (!ifp->if_bytes)
 		xfs_inode_clear_cowblocks_tag(ip);
-	(*tpp)->t_dfops = odfops;
 	return error;
 }
 

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

All callers either explicitly initialize a dfops or pass a transaction with an internal dfops. Drop the hacky old dfops replacement logic and use the one associated with the transaction.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/xfs_reflink.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

fs/xfs/xfs_reflink.c +2 -4

index 3143889097f1..c3f2a0d3e6f5 100644
@@ -483,8 +483,6 @@ xfs_reflink_cancel_cow_blocks(
 	struct xfs_ifork		*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
 	struct xfs_bmbt_irec		got, del;
 	struct xfs_iext_cursor		icur;
-	struct xfs_defer_ops		dfops;
-	struct xfs_defer_ops		*odfops = (*tpp)->t_dfops;
 	int				error = 0;
 
 	if (!xfs_is_reflink_inode(ip))

@@ -511,7 +509,8 @@ xfs_reflink_cancel_cow_blocks(
 			if (error)
 				break;
 		} else if (del.br_state == XFS_EXT_UNWRITTEN || cancel_real) {
-			xfs_defer_init(*tpp, &dfops);
+			ASSERT((*tpp)->t_dfops);
+			ASSERT((*tpp)->t_firstblock == NULLFSBLOCK);
 
 			/* Free the CoW orphan record. */
 			error = xfs_refcount_free_cow_extent(ip->i_mount,

@@ -553,7 +552,6 @@ xfs_reflink_cancel_cow_blocks(
 	/* clear tag if cow fork is emptied */
 	if (!ifp->if_bytes)
 		xfs_inode_clear_cowblocks_tag(ip);
-	(*tpp)->t_dfops = odfops;
 	return error;
 }
 

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

All callers either explicitly initialize a dfops or pass a transaction with an internal dfops. Drop the hacky old dfops replacement logic and use the one associated with the transaction.

Signed-off-by Brian Foster <[email protected]>

Remove the unnecessary on-stack dfops structure and use the internal transaction dfops instead. The lower level xattr code already appropriately accesses ->t_dfops throughout.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/libxfs/xfs_attr.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

fs/xfs/libxfs/xfs_attr.c +5 -11

index 927d4c968f9a..66a22c80a0db 100644
@@ -202,7 +202,6 @@ xfs_attr_set(
 	struct xfs_mount	*mp = dp->i_mount;
 	struct xfs_buf		*leaf_bp = NULL;
 	struct xfs_da_args	args;
-	struct xfs_defer_ops	dfops;
 	struct xfs_trans_res	tres;
 	int			rsvd = (flags & ATTR_ROOT) != 0;
 	int			error, err2, local;

@@ -251,7 +250,6 @@ xfs_attr_set(
 			rsvd ? XFS_TRANS_RESERVE : 0, &args.trans);
 	if (error)
 		return error;
-	xfs_defer_init(args.trans, &dfops);
 
 	xfs_ilock(dp, XFS_ILOCK_EXCL);
 	error = xfs_trans_reserve_quota_nblks(args.trans, dp, args.total, 0,

@@ -315,18 +313,18 @@ xfs_attr_set(
 		 */
 		error = xfs_attr_shortform_to_leaf(&args, &leaf_bp);
 		if (error)
-			goto out_defer_cancel;
+			goto out;
 		/*
 		 * Prevent the leaf buffer from being unlocked so that a
 		 * concurrent AIL push cannot grab the half-baked leaf
 		 * buffer and run into problems with the write verifier.
 		 */
 		xfs_trans_bhold(args.trans, leaf_bp);
-		xfs_defer_bjoin(&dfops, leaf_bp);
-		xfs_defer_ijoin(&dfops, dp);
-		error = xfs_defer_finish(&args.trans, &dfops);
+		xfs_defer_bjoin(args.trans->t_dfops, leaf_bp);
+		xfs_defer_ijoin(args.trans->t_dfops, dp);
+		error = xfs_defer_finish(&args.trans, args.trans->t_dfops);
 		if (error)
-			goto out_defer_cancel;
+			goto out;
 
 		/*
 		 * Commit the leaf transformation.  We'll need another (linked)

@@ -366,8 +364,6 @@ xfs_attr_set(
 
 	return error;
 
-out_defer_cancel:
-	xfs_defer_cancel(&dfops);
 out:
 	if (leaf_bp)
 		xfs_trans_brelse(args.trans, leaf_bp);

@@ -389,7 +385,6 @@ xfs_attr_remove(
 {
 	struct xfs_mount	*mp = dp->i_mount;
 	struct xfs_da_args	args;
-	struct xfs_defer_ops	dfops;
 	int			error;
 
 	XFS_STATS_INC(mp, xs_attr_remove);

@@ -422,7 +417,6 @@ xfs_attr_remove(
 			&args.trans);
 	if (error)
 		return error;
-	xfs_defer_init(args.trans, &dfops);
 
 	xfs_ilock(dp, XFS_ILOCK_EXCL);
 	/*

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Remove the unnecessary on-stack dfops structure and use the internal transaction dfops instead. The lower level xattr code already appropriately accesses ->t_dfops throughout.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/libxfs/xfs_attr.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

fs/xfs/libxfs/xfs_attr.c +5 -11

index 927d4c968f9a..66a22c80a0db 100644
@@ -202,7 +202,6 @@ xfs_attr_set(
 	struct xfs_mount	*mp = dp->i_mount;
 	struct xfs_buf		*leaf_bp = NULL;
 	struct xfs_da_args	args;
-	struct xfs_defer_ops	dfops;
 	struct xfs_trans_res	tres;
 	int			rsvd = (flags & ATTR_ROOT) != 0;
 	int			error, err2, local;

@@ -251,7 +250,6 @@ xfs_attr_set(
 			rsvd ? XFS_TRANS_RESERVE : 0, &args.trans);
 	if (error)
 		return error;
-	xfs_defer_init(args.trans, &dfops);
 
 	xfs_ilock(dp, XFS_ILOCK_EXCL);
 	error = xfs_trans_reserve_quota_nblks(args.trans, dp, args.total, 0,

@@ -315,18 +313,18 @@ xfs_attr_set(
 		 */
 		error = xfs_attr_shortform_to_leaf(&args, &leaf_bp);
 		if (error)
-			goto out_defer_cancel;
+			goto out;
 		/*
 		 * Prevent the leaf buffer from being unlocked so that a
 		 * concurrent AIL push cannot grab the half-baked leaf
 		 * buffer and run into problems with the write verifier.
 		 */
 		xfs_trans_bhold(args.trans, leaf_bp);
-		xfs_defer_bjoin(&dfops, leaf_bp);
-		xfs_defer_ijoin(&dfops, dp);
-		error = xfs_defer_finish(&args.trans, &dfops);
+		xfs_defer_bjoin(args.trans->t_dfops, leaf_bp);
+		xfs_defer_ijoin(args.trans->t_dfops, dp);
+		error = xfs_defer_finish(&args.trans, args.trans->t_dfops);
 		if (error)
-			goto out_defer_cancel;
+			goto out;
 
 		/*
 		 * Commit the leaf transformation.  We'll need another (linked)

@@ -366,8 +364,6 @@ xfs_attr_set(
 
 	return error;
 
-out_defer_cancel:
-	xfs_defer_cancel(&dfops);
 out:
 	if (leaf_bp)
 		xfs_trans_brelse(args.trans, leaf_bp);

@@ -389,7 +385,6 @@ xfs_attr_remove(
 {
 	struct xfs_mount	*mp = dp->i_mount;
 	struct xfs_da_args	args;
-	struct xfs_defer_ops	dfops;
 	int			error;
 
 	XFS_STATS_INC(mp, xs_attr_remove);

@@ -422,7 +417,6 @@ xfs_attr_remove(
 			&args.trans);
 	if (error)
 		return error;
-	xfs_defer_init(args.trans, &dfops);
 
 	xfs_ilock(dp, XFS_ILOCK_EXCL);
 	/*

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Remove the unnecessary on-stack dfops structure and use the internal transaction dfops instead. The lower level xattr code already appropriately accesses ->t_dfops throughout.

Signed-off-by Brian Foster <[email protected]>

bmap and refcount intent processing associates a dfops from the caller with a local transaction to collect all deferred items for post-processing. Use the internal dfops in both of these functions and move the deferred items to the parent dfops before the transaction commits.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/xfs_bmap_item.c     | 21 +++++++++++----------
 fs/xfs/xfs_log_recover.c   |  6 +++---
 fs/xfs/xfs_refcount_item.c | 30 ++++++++++++++++--------------
 3 files changed, 30 insertions(+), 27 deletions(-)

fs/xfs/xfs_bmap_item.c +11 -10

index 478bfc798861..bc5eb2e0ab0c 100644
@@ -441,7 +441,12 @@ xfs_bui_recover(
 			XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK), 0, 0, &tp);
 	if (error)
 		return error;
-	tp->t_dfops = dfops;
+	/*
+	 * Recovery stashes all deferred ops during intent processing and
+	 * finishes them on completion. Transfer current dfops state to this
+	 * transaction and transfer the result back before we return.
+	 */
+	xfs_defer_move(tp->t_dfops, dfops);
 	budp = xfs_trans_get_bud(tp, buip);
 
 	/* Grab the inode. */

@@ -470,7 +475,7 @@ xfs_bui_recover(
 	xfs_trans_ijoin(tp, ip, 0);
 
 	count = bmap->me_len;
-	error = xfs_trans_log_finish_bmap_update(tp, budp, dfops, type,
+	error = xfs_trans_log_finish_bmap_update(tp, budp, tp->t_dfops, type,
 			ip, whichfork, bmap->me_startoff,
 			bmap->me_startblock, &count, state);
 	if (error)

@@ -482,18 +487,14 @@ xfs_bui_recover(
 		irec.br_blockcount = count;
 		irec.br_startoff = bmap->me_startoff;
 		irec.br_state = state;
-		error = xfs_bmap_unmap_extent(tp->t_mountp, dfops, ip, &irec);
+		error = xfs_bmap_unmap_extent(tp->t_mountp, tp->t_dfops, ip,
+					      &irec);
 		if (error)
 			goto err_inode;
 	}
 
 	set_bit(XFS_BUI_RECOVERED, &buip->bui_flags);
-	/*
-	 * Recovery finishes all deferred ops once intent processing is
-	 * complete. Reset the trans reference because commit expects a finished
-	 * dfops or none at all.
-	 */
-	tp->t_dfops = NULL;
+	xfs_defer_move(dfops, tp->t_dfops);
 	error = xfs_trans_commit(tp);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	IRELE(ip);

@@ -501,7 +502,7 @@ xfs_bui_recover(
 	return error;
 
 err_inode:
-	tp->t_dfops = NULL;
+	xfs_defer_move(dfops, tp->t_dfops);
 	xfs_trans_cancel(tp);
 	if (ip) {
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);

fs/xfs/xfs_log_recover.c +3 -3

index 3289811eb076..958e9b96dc6a 100644
@@ -4854,10 +4854,10 @@ xlog_finish_defer_ops(
 			0, XFS_TRANS_RESERVE, &tp);
 	if (error)
 		return error;
-	/* dfops is already populated so assign it manually */
-	tp->t_dfops = dfops;
+	/* transfer all collected dfops to this transaction */
+	xfs_defer_move(tp->t_dfops, dfops);
 
-	error = xfs_defer_finish(&tp, dfops);
+	error = xfs_defer_finish(&tp, tp->t_dfops);
 	if (error)
 		goto out_cancel;
 

fs/xfs/xfs_refcount_item.c +16 -14

index 2064c689bc72..d3582a06626f 100644
@@ -452,7 +452,12 @@ xfs_cui_recover(
 			mp->m_refc_maxlevels * 2, 0, XFS_TRANS_RESERVE, &tp);
 	if (error)
 		return error;
-	tp->t_dfops = dfops;
+	/*
+	 * Recovery stashes all deferred ops during intent processing and
+	 * finishes them on completion. Transfer current dfops state to this
+	 * transaction and transfer the result back before we return.
+	 */
+	xfs_defer_move(tp->t_dfops, dfops);
 	cudp = xfs_trans_get_cud(tp, cuip);
 
 	for (i = 0; i < cuip->cui_format.cui_nextents; i++) {

@@ -474,8 +479,8 @@ xfs_cui_recover(
 			new_len = refc->pe_len;
 		} else
 			error = xfs_trans_log_finish_refcount_update(tp, cudp,
-				dfops, type, refc->pe_startblock, refc->pe_len,
-				&new_fsb, &new_len, &rcur);
+				tp->t_dfops, type, refc->pe_startblock,
+				refc->pe_len, &new_fsb, &new_len, &rcur);
 		if (error)
 			goto abort_error;
 

@@ -486,21 +491,23 @@ xfs_cui_recover(
 			switch (type) {
 			case XFS_REFCOUNT_INCREASE:
 				error = xfs_refcount_increase_extent(
-						tp->t_mountp, dfops, &irec);
+						tp->t_mountp, tp->t_dfops,
+						&irec);
 				break;
 			case XFS_REFCOUNT_DECREASE:
 				error = xfs_refcount_decrease_extent(
-						tp->t_mountp, dfops, &irec);
+						tp->t_mountp, tp->t_dfops,
+						&irec);
 				break;
 			case XFS_REFCOUNT_ALLOC_COW:
 				error = xfs_refcount_alloc_cow_extent(
-						tp->t_mountp, dfops,
+						tp->t_mountp, tp->t_dfops,
 						irec.br_startblock,
 						irec.br_blockcount);
 				break;
 			case XFS_REFCOUNT_FREE_COW:
 				error = xfs_refcount_free_cow_extent(
-						tp->t_mountp, dfops,
+						tp->t_mountp, tp->t_dfops,
 						irec.br_startblock,
 						irec.br_blockcount);
 				break;

@@ -515,18 +522,13 @@ xfs_cui_recover(
 
 	xfs_refcount_finish_one_cleanup(tp, rcur, error);
 	set_bit(XFS_CUI_RECOVERED, &cuip->cui_flags);
-	/*
-	 * Recovery finishes all deferred ops once intent processing is
-	 * complete. Reset the trans reference because commit expects a finished
-	 * dfops or none at all.
-	 */
-	tp->t_dfops = NULL;
+	xfs_defer_move(dfops, tp->t_dfops);
 	error = xfs_trans_commit(tp);
 	return error;
 
 abort_error:
 	xfs_refcount_finish_one_cleanup(tp, rcur, error);
-	tp->t_dfops = NULL;
+	xfs_defer_move(dfops, tp->t_dfops);
 	xfs_trans_cancel(tp);
 	return error;
 }

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

bmap and refcount intent processing associates a dfops from the caller with a local transaction to collect all deferred items for post-processing. Use the internal dfops in both of these functions and move the deferred items to the parent dfops before the transaction commits.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/xfs_bmap_item.c     | 21 +++++++++++----------
 fs/xfs/xfs_log_recover.c   |  6 +++---
 fs/xfs/xfs_refcount_item.c | 30 ++++++++++++++++--------------
 3 files changed, 30 insertions(+), 27 deletions(-)

fs/xfs/xfs_bmap_item.c +11 -10

index 478bfc798861..bc5eb2e0ab0c 100644
@@ -441,7 +441,12 @@ xfs_bui_recover(
 			XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK), 0, 0, &tp);
 	if (error)
 		return error;
-	tp->t_dfops = dfops;
+	/*
+	 * Recovery stashes all deferred ops during intent processing and
+	 * finishes them on completion. Transfer current dfops state to this
+	 * transaction and transfer the result back before we return.
+	 */
+	xfs_defer_move(tp->t_dfops, dfops);
 	budp = xfs_trans_get_bud(tp, buip);
 
 	/* Grab the inode. */

@@ -470,7 +475,7 @@ xfs_bui_recover(
 	xfs_trans_ijoin(tp, ip, 0);
 
 	count = bmap->me_len;
-	error = xfs_trans_log_finish_bmap_update(tp, budp, dfops, type,
+	error = xfs_trans_log_finish_bmap_update(tp, budp, tp->t_dfops, type,
 			ip, whichfork, bmap->me_startoff,
 			bmap->me_startblock, &count, state);
 	if (error)

@@ -482,18 +487,14 @@ xfs_bui_recover(
 		irec.br_blockcount = count;
 		irec.br_startoff = bmap->me_startoff;
 		irec.br_state = state;
-		error = xfs_bmap_unmap_extent(tp->t_mountp, dfops, ip, &irec);
+		error = xfs_bmap_unmap_extent(tp->t_mountp, tp->t_dfops, ip,
+					      &irec);
 		if (error)
 			goto err_inode;
 	}
 
 	set_bit(XFS_BUI_RECOVERED, &buip->bui_flags);
-	/*
-	 * Recovery finishes all deferred ops once intent processing is
-	 * complete. Reset the trans reference because commit expects a finished
-	 * dfops or none at all.
-	 */
-	tp->t_dfops = NULL;
+	xfs_defer_move(dfops, tp->t_dfops);
 	error = xfs_trans_commit(tp);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	IRELE(ip);

@@ -501,7 +502,7 @@ xfs_bui_recover(
 	return error;
 
 err_inode:
-	tp->t_dfops = NULL;
+	xfs_defer_move(dfops, tp->t_dfops);
 	xfs_trans_cancel(tp);
 	if (ip) {
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);

fs/xfs/xfs_log_recover.c +3 -3

index 3289811eb076..958e9b96dc6a 100644
@@ -4854,10 +4854,10 @@ xlog_finish_defer_ops(
 			0, XFS_TRANS_RESERVE, &tp);
 	if (error)
 		return error;
-	/* dfops is already populated so assign it manually */
-	tp->t_dfops = dfops;
+	/* transfer all collected dfops to this transaction */
+	xfs_defer_move(tp->t_dfops, dfops);
 
-	error = xfs_defer_finish(&tp, dfops);
+	error = xfs_defer_finish(&tp, tp->t_dfops);
 	if (error)
 		goto out_cancel;
 

fs/xfs/xfs_refcount_item.c +16 -14

index 2064c689bc72..d3582a06626f 100644
@@ -452,7 +452,12 @@ xfs_cui_recover(
 			mp->m_refc_maxlevels * 2, 0, XFS_TRANS_RESERVE, &tp);
 	if (error)
 		return error;
-	tp->t_dfops = dfops;
+	/*
+	 * Recovery stashes all deferred ops during intent processing and
+	 * finishes them on completion. Transfer current dfops state to this
+	 * transaction and transfer the result back before we return.
+	 */
+	xfs_defer_move(tp->t_dfops, dfops);
 	cudp = xfs_trans_get_cud(tp, cuip);
 
 	for (i = 0; i < cuip->cui_format.cui_nextents; i++) {

@@ -474,8 +479,8 @@ xfs_cui_recover(
 			new_len = refc->pe_len;
 		} else
 			error = xfs_trans_log_finish_refcount_update(tp, cudp,
-				dfops, type, refc->pe_startblock, refc->pe_len,
-				&new_fsb, &new_len, &rcur);
+				tp->t_dfops, type, refc->pe_startblock,
+				refc->pe_len, &new_fsb, &new_len, &rcur);
 		if (error)
 			goto abort_error;
 

@@ -486,21 +491,23 @@ xfs_cui_recover(
 			switch (type) {
 			case XFS_REFCOUNT_INCREASE:
 				error = xfs_refcount_increase_extent(
-						tp->t_mountp, dfops, &irec);
+						tp->t_mountp, tp->t_dfops,
+						&irec);
 				break;
 			case XFS_REFCOUNT_DECREASE:
 				error = xfs_refcount_decrease_extent(
-						tp->t_mountp, dfops, &irec);
+						tp->t_mountp, tp->t_dfops,
+						&irec);
 				break;
 			case XFS_REFCOUNT_ALLOC_COW:
 				error = xfs_refcount_alloc_cow_extent(
-						tp->t_mountp, dfops,
+						tp->t_mountp, tp->t_dfops,
 						irec.br_startblock,
 						irec.br_blockcount);
 				break;
 			case XFS_REFCOUNT_FREE_COW:
 				error = xfs_refcount_free_cow_extent(
-						tp->t_mountp, dfops,
+						tp->t_mountp, tp->t_dfops,
 						irec.br_startblock,
 						irec.br_blockcount);
 				break;

@@ -515,18 +522,13 @@ xfs_cui_recover(
 
 	xfs_refcount_finish_one_cleanup(tp, rcur, error);
 	set_bit(XFS_CUI_RECOVERED, &cuip->cui_flags);
-	/*
-	 * Recovery finishes all deferred ops once intent processing is
-	 * complete. Reset the trans reference because commit expects a finished
-	 * dfops or none at all.
-	 */
-	tp->t_dfops = NULL;
+	xfs_defer_move(dfops, tp->t_dfops);
 	error = xfs_trans_commit(tp);
 	return error;
 
 abort_error:
 	xfs_refcount_finish_one_cleanup(tp, rcur, error);
-	tp->t_dfops = NULL;
+	xfs_defer_move(dfops, tp->t_dfops);
 	xfs_trans_cancel(tp);
 	return error;
 }

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

bmap and refcount intent processing associates a dfops from the caller with a local transaction to collect all deferred items for post-processing. Use the internal dfops in both of these functions and move the deferred items to the parent dfops before the transaction commits.

Signed-off-by Brian Foster <[email protected]>