[PATCH 00/14] xfs: embed dfops in the transaction

[PATCH 00/14] xfs: embed dfops in the transaction

[Brian Foster]

Hi all,

Here's the last dfops refactoring series. The objective is to embed
dfops in the transaction. In doing so, the boilerplate xfs_trans_alloc()
-> xfs_defer_init() -> xfs_defer_finish() -> xfs_trans_commit() pattern
is reduced to a transaction allocation and commit.

Patches 1-7 are various preparatory, cleanup and/or bug fix patches.
Patch 8 adds an internal dfops field to xfs_trans with some supporting
mechanism bits. Patches 9-11 convert the various deferred op using paths
over to use the internal transaction dfops instead of the on-stack
variant. Patches 12-13 remove a bunch of the boilerplate dfops code that
is made superfluous by the previous changes. Finally, patch 14 drops the
no longer necessary dfops param to xfs_defer_finish().

I think most of this is relatively straightforward and survives my
testing so far. FWIW, I think there are still a couple more related
cleanups that could be made on top of this series. For one, I need to
optimize on top of Darrick's recently posted xfs_defer_finish() patch to
return a clean transaction. Other things to consider might be to do away
with support for external dfops and the ->t_dfops pointer indirection,
or perhaps even consider going the other direction: allocate dfops from
a separate zone to save some memory on non-permanent transactions (note
that 16 of 28 transactions use a permanent log res. last I looked, so it
may not be worth it atm).

I know Christoph also had thoughts around condensing some of the items
joined to the dfops to those with the transaction. I have yet to think
about that one, but I do have an RFC quality patch laying around that
replaces the ->dop_low flag with a transaction flag (->t_flags),
eliminating the need for that extra byte in xfs_defer_ops. The one quirk
associated with that is the question of whether we want to preserve the
behavior where low mode remains active across the series of transactions
associated with the traditional (on-stack) dfops or is reset on
transaction roll (a la firstblock). I'll post that RFC separately for a
more proper discussion..

Brian

Brian Foster (14):
  xfs: pull up dfops from xfs_itruncate_extents()
  xfs: use ->t_dfops in log recovery intent processing
  xfs: fix transaction leak on remote attr set/remove failure
  xfs: make deferred processing safe for embedded dfops
  xfs: remove unused deferred ops committed field
  xfs: reset dfops to initial state after finish
  xfs: pack holes in xfs_defer_ops and xfs_trans
  xfs: support embedded dfops in transaction
  xfs: use internal dfops in cow blocks cancel
  xfs: use internal dfops in attr code
  xfs: use internal dfops during [b|c]ui recovery
  xfs: remove all boilerplate defer init/finish code
  xfs: remove unnecessary dfops init calls in xattr code
  xfs: drop unnecessary xfs_defer_finish() dfops parameter

 fs/xfs/libxfs/xfs_alloc_btree.c    |  1 +
 fs/xfs/libxfs/xfs_attr.c           | 49 ++++++---------
 fs/xfs/libxfs/xfs_attr_leaf.c      |  1 +
 fs/xfs/libxfs/xfs_attr_remote.c    | 13 ++--
 fs/xfs/libxfs/xfs_bmap.c           | 16 +----
 fs/xfs/libxfs/xfs_da_btree.c       |  1 +
 fs/xfs/libxfs/xfs_defer.c          | 95 +++++++++++++++++++++++-------
 fs/xfs/libxfs/xfs_defer.h          |  9 +--
 fs/xfs/libxfs/xfs_dir2_block.c     |  1 +
 fs/xfs/libxfs/xfs_dir2_data.c      |  1 +
 fs/xfs/libxfs/xfs_dir2_leaf.c      |  1 +
 fs/xfs/libxfs/xfs_dir2_node.c      |  1 +
 fs/xfs/libxfs/xfs_dir2_sf.c        |  1 +
 fs/xfs/libxfs/xfs_dquot_buf.c      |  1 +
 fs/xfs/libxfs/xfs_ialloc_btree.c   |  1 +
 fs/xfs/libxfs/xfs_inode_fork.c     |  1 +
 fs/xfs/libxfs/xfs_refcount.c       | 10 +---
 fs/xfs/libxfs/xfs_refcount_btree.c |  1 +
 fs/xfs/libxfs/xfs_symlink_remote.c |  1 +
 fs/xfs/libxfs/xfs_trans_resv.c     |  1 +
 fs/xfs/xfs_aops.c                  |  1 +
 fs/xfs/xfs_attr_inactive.c         |  1 +
 fs/xfs/xfs_attr_list.c             |  1 +
 fs/xfs/xfs_bmap_item.c             | 21 +++----
 fs/xfs/xfs_bmap_util.c             | 45 +++-----------
 fs/xfs/xfs_buf_item.c              |  1 +
 fs/xfs/xfs_dir2_readdir.c          |  1 +
 fs/xfs/xfs_dquot.c                 |  8 +--
 fs/xfs/xfs_dquot_item.c            |  1 +
 fs/xfs/xfs_export.c                |  1 +
 fs/xfs/xfs_extent_busy.c           |  1 +
 fs/xfs/xfs_extfree_item.c          |  1 +
 fs/xfs/xfs_file.c                  |  1 +
 fs/xfs/xfs_icache.c                |  1 +
 fs/xfs/xfs_icreate_item.c          |  1 +
 fs/xfs/xfs_inode.c                 | 87 +++++----------------------
 fs/xfs/xfs_inode_item.c            |  1 +
 fs/xfs/xfs_ioctl.c                 |  1 +
 fs/xfs/xfs_iomap.c                 | 26 +-------
 fs/xfs/xfs_iops.c                  |  1 +
 fs/xfs/xfs_log.c                   |  1 +
 fs/xfs/xfs_log_cil.c               |  1 +
 fs/xfs/xfs_log_recover.c           | 12 +---
 fs/xfs/xfs_pnfs.c                  |  1 +
 fs/xfs/xfs_qm.c                    |  1 +
 fs/xfs/xfs_qm_bhv.c                |  1 +
 fs/xfs/xfs_qm_syscalls.c           |  1 +
 fs/xfs/xfs_quotaops.c              |  1 +
 fs/xfs/xfs_refcount_item.c         | 30 +++++-----
 fs/xfs/xfs_reflink.c               | 51 ++++++----------
 fs/xfs/xfs_rtalloc.c               |  9 +--
 fs/xfs/xfs_super.c                 |  1 +
 fs/xfs/xfs_symlink.c               | 38 +++---------
 fs/xfs/xfs_trace.h                 |  8 +--
 fs/xfs/xfs_trans.c                 | 32 ++++++++--
 fs/xfs/xfs_trans.h                 | 12 +++-
 fs/xfs/xfs_trans_ail.c             |  1 +
 fs/xfs/xfs_trans_buf.c             |  1 +
 fs/xfs/xfs_trans_dquot.c           |  1 +
 fs/xfs/xfs_trans_inode.c           |  1 +
 60 files changed, 266 insertions(+), 346 deletions(-)

-- 
2.17.1

[PATCH 01/14] xfs: pull up dfops from xfs_itruncate_extents()

[Brian Foster]

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 <bfoster@redhat.com>
---
 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(-)

diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
index 7ce10055f275..d3055972d3a6 100644
--- a/fs/xfs/xfs_attr_inactive.c
+++ b/fs/xfs/xfs_attr_inactive.c
@@ -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);
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index d3a314fd721f..ee9481b142f0 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -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);
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 7b2694d3901a..7d7d7e95fa17 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -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);
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 0fa29f39d658..704b57a8b99e 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -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);
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index abc8a21e3a82..df0783303887 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -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);
-- 
2.17.1

[PATCH 02/14] xfs: use ->t_dfops in log recovery intent processing

[Brian Foster]

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 <bfoster@redhat.com>
---
 fs/xfs/xfs_log_recover.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index cbac943896f4..3289811eb076 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -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)
-- 
2.17.1

[PATCH 03/14] xfs: fix transaction leak on remote attr set/remove failure

[Brian Foster]

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 <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_attr_remote.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index 7841e6255129..829ab20f0cd7 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -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;
 }
-- 
2.17.1

[PATCH 04/14] xfs: make deferred processing safe for embedded dfops

[Brian Foster]

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 <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_defer.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 2713e2d808a7..ef1535ceb5e7 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -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,
@@ -425,7 +420,6 @@ xfs_defer_finish(
 	}
 
 out:
-	(*tp)->t_dfops = orig_dop;
 	if (error)
 		trace_xfs_defer_finish_error((*tp)->t_mountp, dop, error);
 	else
-- 
2.17.1

[PATCH 05/14] xfs: remove unused deferred ops committed field

[Brian Foster]

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 <bfoster@redhat.com>
---
 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(-)

diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index ef1535ceb5e7..d3466087db16 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -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++)
diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index c17c9deda995..58c979c9f3fa 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -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 */
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index b668fc127aa7..cc6995cfce66 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -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)
 )
-- 
2.17.1

[PATCH 06/14] xfs: reset dfops to initial state after finish

[Brian Foster]

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 <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_defer.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index d3466087db16..e6baa27a690b 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -319,6 +319,29 @@ xfs_defer_bjoin(
 	return -EFSCORRUPTED;
 }
 
+/*
+ * Reset an already used dfops after finish.
+ */
+static void
+xfs_defer_reset(
+	struct xfs_defer_ops	*dop)
+{
+	int 			i;
+
+	ASSERT(!xfs_defer_has_unfinished_work(dop));
+	dop->dop_low = false;
+	for (i = 0; i < XFS_DEFER_OPS_NR_INODES; i++) {
+		if (!dop->dop_inodes[i])
+			break;
+		dop->dop_inodes[i] = NULL;
+	}
+	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS; i++) {
+		if (!dop->dop_bufs[i])
+			break;
+		dop->dop_bufs[i] = NULL;
+	}
+}
+
 /*
  * Finish all the pending work.  This involves logging intent items for
  * any work items that wandered in since the last transaction roll (if
@@ -419,10 +442,13 @@ xfs_defer_finish(
 	}
 
 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;
 }
 
-- 
2.17.1

[PATCH 07/14] xfs: pack holes in xfs_defer_ops and xfs_trans

[Brian Foster]

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 <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_defer.h | 3 ++-
 fs/xfs/xfs_trans.h        | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index 58c979c9f3fa..8f58f217fdff 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -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,
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 37fdacc690c7..6f857af61455 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -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 */
-- 
2.17.1

[PATCH 08/14] xfs: support embedded dfops in transaction

[Brian Foster]

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 <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_alloc_btree.c    |  1 +
 fs/xfs/libxfs/xfs_attr_leaf.c      |  1 +
 fs/xfs/libxfs/xfs_da_btree.c       |  1 +
 fs/xfs/libxfs/xfs_defer.c          | 33 ++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_defer.h          |  1 +
 fs/xfs/libxfs/xfs_dir2_block.c     |  1 +
 fs/xfs/libxfs/xfs_dir2_data.c      |  1 +
 fs/xfs/libxfs/xfs_dir2_leaf.c      |  1 +
 fs/xfs/libxfs/xfs_dir2_node.c      |  1 +
 fs/xfs/libxfs/xfs_dir2_sf.c        |  1 +
 fs/xfs/libxfs/xfs_dquot_buf.c      |  1 +
 fs/xfs/libxfs/xfs_ialloc_btree.c   |  1 +
 fs/xfs/libxfs/xfs_inode_fork.c     |  1 +
 fs/xfs/libxfs/xfs_refcount_btree.c |  1 +
 fs/xfs/libxfs/xfs_symlink_remote.c |  1 +
 fs/xfs/libxfs/xfs_trans_resv.c     |  1 +
 fs/xfs/xfs_aops.c                  |  1 +
 fs/xfs/xfs_attr_inactive.c         |  2 +-
 fs/xfs/xfs_attr_list.c             |  1 +
 fs/xfs/xfs_buf_item.c              |  1 +
 fs/xfs/xfs_dir2_readdir.c          |  1 +
 fs/xfs/xfs_dquot_item.c            |  1 +
 fs/xfs/xfs_export.c                |  1 +
 fs/xfs/xfs_extent_busy.c           |  1 +
 fs/xfs/xfs_extfree_item.c          |  1 +
 fs/xfs/xfs_file.c                  |  1 +
 fs/xfs/xfs_icache.c                |  1 +
 fs/xfs/xfs_icreate_item.c          |  1 +
 fs/xfs/xfs_inode_item.c            |  1 +
 fs/xfs/xfs_ioctl.c                 |  1 +
 fs/xfs/xfs_iops.c                  |  2 +-
 fs/xfs/xfs_log.c                   |  1 +
 fs/xfs/xfs_log_cil.c               |  1 +
 fs/xfs/xfs_pnfs.c                  |  1 +
 fs/xfs/xfs_qm.c                    |  1 +
 fs/xfs/xfs_qm_bhv.c                |  1 +
 fs/xfs/xfs_qm_syscalls.c           |  2 +-
 fs/xfs/xfs_quotaops.c              |  1 +
 fs/xfs/xfs_super.c                 |  1 +
 fs/xfs/xfs_trans.c                 | 32 ++++++++++++++++++++++++-----
 fs/xfs/xfs_trans.h                 |  2 +-
 fs/xfs/xfs_trans_ail.c             |  1 +
 fs/xfs/xfs_trans_buf.c             |  1 +
 fs/xfs/xfs_trans_dquot.c           |  1 +
 fs/xfs/xfs_trans_inode.c           |  1 +
 45 files changed, 103 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
index 4e59cc8a2802..a2ecb6e94cfe 100644
--- a/fs/xfs/libxfs/xfs_alloc_btree.c
+++ b/fs/xfs/libxfs/xfs_alloc_btree.c
@@ -18,6 +18,7 @@
 #include "xfs_error.h"
 #include "xfs_trace.h"
 #include "xfs_cksum.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 
 
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 251304f3bc5d..56156c985e4d 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -16,6 +16,7 @@
 #include "xfs_da_format.h"
 #include "xfs_da_btree.h"
 #include "xfs_inode.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_inode_item.h"
 #include "xfs_bmap_btree.h"
diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index 9efbd2038ffb..b5c8dbf5c037 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -17,6 +17,7 @@
 #include "xfs_dir2.h"
 #include "xfs_dir2_priv.h"
 #include "xfs_inode.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_inode_item.h"
 #include "xfs_alloc.h"
diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index e6baa27a690b..415fcf720cc9 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -557,3 +557,36 @@ 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)
+{
+	int			i;
+
+	ASSERT(dst != src);
+
+	list_splice_init(&src->dop_intake, &dst->dop_intake);
+	list_splice_init(&src->dop_pending, &dst->dop_pending);
+
+	for (i = 0; i < XFS_DEFER_OPS_NR_INODES; i++) {
+		if (!src->dop_inodes[i])
+			break;
+		dst->dop_inodes[i] = src->dop_inodes[i];
+	}
+	for (i = 0; i< XFS_DEFER_OPS_NR_BUFS; i++) {
+		if (!src->dop_bufs[i])
+			break;
+		dst->dop_bufs[i] = src->dop_bufs[i];
+	}
+
+	dst->dop_low = src->dop_low;
+
+	xfs_defer_reset(src);
+}
diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index 8f58f217fdff..349b4d906fb2 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -67,6 +67,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 {
diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
index 30ed5919da72..76b45a309a23 100644
--- a/fs/xfs/libxfs/xfs_dir2_block.c
+++ b/fs/xfs/libxfs/xfs_dir2_block.c
@@ -13,6 +13,7 @@
 #include "xfs_da_format.h"
 #include "xfs_da_btree.h"
 #include "xfs_inode.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_inode_item.h"
 #include "xfs_bmap.h"
diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
index 01162c62ec8f..d4e2318cf235 100644
--- a/fs/xfs/libxfs/xfs_dir2_data.c
+++ b/fs/xfs/libxfs/xfs_dir2_data.c
@@ -16,6 +16,7 @@
 #include "xfs_dir2.h"
 #include "xfs_dir2_priv.h"
 #include "xfs_error.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_buf_item.h"
 #include "xfs_cksum.h"
diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
index 1728a3e6f5cf..c514f116812e 100644
--- a/fs/xfs/libxfs/xfs_dir2_leaf.c
+++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
@@ -18,6 +18,7 @@
 #include "xfs_dir2_priv.h"
 #include "xfs_error.h"
 #include "xfs_trace.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_buf_item.h"
 #include "xfs_cksum.h"
diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
index 2daf874969ab..1a904d2d130e 100644
--- a/fs/xfs/libxfs/xfs_dir2_node.c
+++ b/fs/xfs/libxfs/xfs_dir2_node.c
@@ -18,6 +18,7 @@
 #include "xfs_dir2_priv.h"
 #include "xfs_error.h"
 #include "xfs_trace.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_buf_item.h"
 #include "xfs_cksum.h"
diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index 585dfdb7b6b6..26ae3e0a257f 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -12,6 +12,7 @@
 #include "xfs_da_format.h"
 #include "xfs_da_btree.h"
 #include "xfs_inode.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_inode_item.h"
 #include "xfs_error.h"
diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
index d293f371dd54..1ddd5860fbf8 100644
--- a/fs/xfs/libxfs/xfs_dquot_buf.c
+++ b/fs/xfs/libxfs/xfs_dquot_buf.c
@@ -13,6 +13,7 @@
 #include "xfs_mount.h"
 #include "xfs_inode.h"
 #include "xfs_quota.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_qm.h"
 #include "xfs_error.h"
diff --git a/fs/xfs/libxfs/xfs_ialloc_btree.c b/fs/xfs/libxfs/xfs_ialloc_btree.c
index a5237afec5ab..e91919764cad 100644
--- a/fs/xfs/libxfs/xfs_ialloc_btree.c
+++ b/fs/xfs/libxfs/xfs_ialloc_btree.c
@@ -19,6 +19,7 @@
 #include "xfs_error.h"
 #include "xfs_trace.h"
 #include "xfs_cksum.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_rmap.h"
 
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 183ec0cb8921..4c1f4a2af811 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -12,6 +12,7 @@
 #include "xfs_trans_resv.h"
 #include "xfs_mount.h"
 #include "xfs_inode.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_inode_item.h"
 #include "xfs_btree.h"
diff --git a/fs/xfs/libxfs/xfs_refcount_btree.c b/fs/xfs/libxfs/xfs_refcount_btree.c
index 26d2300ed865..3e3e7509a518 100644
--- a/fs/xfs/libxfs/xfs_refcount_btree.c
+++ b/fs/xfs/libxfs/xfs_refcount_btree.c
@@ -18,6 +18,7 @@
 #include "xfs_error.h"
 #include "xfs_trace.h"
 #include "xfs_cksum.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_bit.h"
 #include "xfs_rmap.h"
diff --git a/fs/xfs/libxfs/xfs_symlink_remote.c b/fs/xfs/libxfs/xfs_symlink_remote.c
index 95374ab2dee7..f231dd836cd5 100644
--- a/fs/xfs/libxfs/xfs_symlink_remote.c
+++ b/fs/xfs/libxfs/xfs_symlink_remote.c
@@ -17,6 +17,7 @@
 #include "xfs_trace.h"
 #include "xfs_symlink.h"
 #include "xfs_cksum.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_buf_item.h"
 #include "xfs_log.h"
diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index f99a7aefe418..db356294707b 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -17,6 +17,7 @@
 #include "xfs_bmap_btree.h"
 #include "xfs_ialloc.h"
 #include "xfs_quota.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_qm.h"
 #include "xfs_trans_space.h"
diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index f4d3252236c1..9c121168dd9b 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -11,6 +11,7 @@
 #include "xfs_trans_resv.h"
 #include "xfs_mount.h"
 #include "xfs_inode.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_inode_item.h"
 #include "xfs_alloc.h"
diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
index d3055972d3a6..cb0ad9b6678e 100644
--- a/fs/xfs/xfs_attr_inactive.c
+++ b/fs/xfs/xfs_attr_inactive.c
@@ -17,6 +17,7 @@
 #include "xfs_inode.h"
 #include "xfs_alloc.h"
 #include "xfs_attr_remote.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_inode_item.h"
 #include "xfs_bmap.h"
@@ -26,7 +27,6 @@
 #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,
diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
index f9ca80154c9c..8c6acc994419 100644
--- a/fs/xfs/xfs_attr_list.c
+++ b/fs/xfs/xfs_attr_list.c
@@ -14,6 +14,7 @@
 #include "xfs_da_format.h"
 #include "xfs_da_btree.h"
 #include "xfs_inode.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_inode_item.h"
 #include "xfs_bmap.h"
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 1c9d1398980b..335d05b5d1e7 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -11,6 +11,7 @@
 #include "xfs_bit.h"
 #include "xfs_sb.h"
 #include "xfs_mount.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_buf_item.h"
 #include "xfs_trans_priv.h"
diff --git a/fs/xfs/xfs_dir2_readdir.c b/fs/xfs/xfs_dir2_readdir.c
index 5142e64e2345..a4b38c94effe 100644
--- a/fs/xfs/xfs_dir2_readdir.c
+++ b/fs/xfs/xfs_dir2_readdir.c
@@ -19,6 +19,7 @@
 #include "xfs_error.h"
 #include "xfs_trace.h"
 #include "xfs_bmap.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 
 /*
diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
index 7dedd17c4813..8fb101ad8b91 100644
--- a/fs/xfs/xfs_dquot_item.c
+++ b/fs/xfs/xfs_dquot_item.c
@@ -12,6 +12,7 @@
 #include "xfs_inode.h"
 #include "xfs_quota.h"
 #include "xfs_error.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_buf_item.h"
 #include "xfs_trans_priv.h"
diff --git a/fs/xfs/xfs_export.c b/fs/xfs/xfs_export.c
index 3cf4682e2510..ef5b7959b18b 100644
--- a/fs/xfs/xfs_export.c
+++ b/fs/xfs/xfs_export.c
@@ -13,6 +13,7 @@
 #include "xfs_dir2.h"
 #include "xfs_export.h"
 #include "xfs_inode.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_inode_item.h"
 #include "xfs_trace.h"
diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
index 0ed68379e551..3558a832f483 100644
--- a/fs/xfs/xfs_extent_busy.c
+++ b/fs/xfs/xfs_extent_busy.c
@@ -16,6 +16,7 @@
 #include "xfs_alloc.h"
 #include "xfs_extent_busy.h"
 #include "xfs_trace.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_log.h"
 
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index d9da66c718bb..55cc4f4fae36 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -10,6 +10,7 @@
 #include "xfs_trans_resv.h"
 #include "xfs_bit.h"
 #include "xfs_mount.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_trans_priv.h"
 #include "xfs_buf_item.h"
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 6b31f41eafa2..8e06b19ce8c0 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -13,6 +13,7 @@
 #include "xfs_da_format.h"
 #include "xfs_da_btree.h"
 #include "xfs_inode.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_inode_item.h"
 #include "xfs_bmap.h"
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 47f417d20a30..873113913d4a 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -12,6 +12,7 @@
 #include "xfs_mount.h"
 #include "xfs_inode.h"
 #include "xfs_error.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_trans_priv.h"
 #include "xfs_inode_item.h"
diff --git a/fs/xfs/xfs_icreate_item.c b/fs/xfs/xfs_icreate_item.c
index 8381d34cb102..04f86ede18c7 100644
--- a/fs/xfs/xfs_icreate_item.c
+++ b/fs/xfs/xfs_icreate_item.c
@@ -11,6 +11,7 @@
 #include "xfs_trans_resv.h"
 #include "xfs_bit.h"
 #include "xfs_mount.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_trans_priv.h"
 #include "xfs_error.h"
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 2389c34c172d..423c2183a383 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -10,6 +10,7 @@
 #include "xfs_trans_resv.h"
 #include "xfs_mount.h"
 #include "xfs_inode.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_inode_item.h"
 #include "xfs_error.h"
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 0ef5ece5634c..d816eaec685d 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -26,6 +26,7 @@
 #include "xfs_trace.h"
 #include "xfs_icache.h"
 #include "xfs_symlink.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_acl.h"
 #include "xfs_btree.h"
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 704b57a8b99e..32bbd5001b9f 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -18,6 +18,7 @@
 #include "xfs_quota.h"
 #include "xfs_error.h"
 #include "xfs_attr.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_trace.h"
 #include "xfs_icache.h"
@@ -26,7 +27,6 @@
 #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>
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 5e56f3b93d4b..b6a89e4ff847 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -12,6 +12,7 @@
 #include "xfs_mount.h"
 #include "xfs_errortag.h"
 #include "xfs_error.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_trans_priv.h"
 #include "xfs_log.h"
diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index d3884e08b43c..92050c99c430 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -14,6 +14,7 @@
 #include "xfs_alloc.h"
 #include "xfs_extent_busy.h"
 #include "xfs_discard.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_trans_priv.h"
 #include "xfs_log.h"
diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
index f44c3599527d..f3fe6ed2da96 100644
--- a/fs/xfs/xfs_pnfs.c
+++ b/fs/xfs/xfs_pnfs.c
@@ -10,6 +10,7 @@
 #include "xfs_sb.h"
 #include "xfs_mount.h"
 #include "xfs_inode.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_log.h"
 #include "xfs_bmap.h"
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 9ceb85cce33a..166ca6054179 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -20,6 +20,7 @@
 #include "xfs_bmap.h"
 #include "xfs_bmap_btree.h"
 #include "xfs_bmap_util.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_trans_space.h"
 #include "xfs_qm.h"
diff --git a/fs/xfs/xfs_qm_bhv.c b/fs/xfs/xfs_qm_bhv.c
index 73a1d77ec187..1ebcb9e9418f 100644
--- a/fs/xfs/xfs_qm_bhv.c
+++ b/fs/xfs/xfs_qm_bhv.c
@@ -12,6 +12,7 @@
 #include "xfs_mount.h"
 #include "xfs_inode.h"
 #include "xfs_error.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_qm.h"
 
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index df0783303887..94ca3bff8d07 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -16,13 +16,13 @@
 #include "xfs_sb.h"
 #include "xfs_mount.h"
 #include "xfs_inode.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_error.h"
 #include "xfs_quota.h"
 #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 *,
diff --git a/fs/xfs/xfs_quotaops.c b/fs/xfs/xfs_quotaops.c
index 205fbb2a77e4..a698b964d13a 100644
--- a/fs/xfs/xfs_quotaops.c
+++ b/fs/xfs/xfs_quotaops.c
@@ -10,6 +10,7 @@
 #include "xfs_mount.h"
 #include "xfs_inode.h"
 #include "xfs_quota.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_trace.h"
 #include "xfs_icache.h"
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index f9f8dc490d3d..25a6d227d1fe 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -18,6 +18,7 @@
 #include "xfs_alloc.h"
 #include "xfs_error.h"
 #include "xfs_fsops.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_buf_item.h"
 #include "xfs_log.h"
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index de00f79ff698..142410632a36 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -14,12 +14,12 @@
 #include "xfs_inode.h"
 #include "xfs_extent_busy.h"
 #include "xfs_quota.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_trans_priv.h"
 #include "xfs_log.h"
 #include "xfs_trace.h"
 #include "xfs_error.h"
-#include "xfs_defer.h"
 
 kmem_zone_t	*xfs_trans_zone;
 
@@ -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
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 6f857af61455..bab857c36d3a 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -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;
 
@@ -130,6 +129,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;
 
 /*
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 55326f971cb3..91aa25a753fc 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -10,6 +10,7 @@
 #include "xfs_log_format.h"
 #include "xfs_trans_resv.h"
 #include "xfs_mount.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_trans_priv.h"
 #include "xfs_trace.h"
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index 15919f67a88f..165044d7b6c3 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -11,6 +11,7 @@
 #include "xfs_trans_resv.h"
 #include "xfs_mount.h"
 #include "xfs_inode.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_buf_item.h"
 #include "xfs_trans_priv.h"
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index c23257a26c2b..d737a5c00148 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -12,6 +12,7 @@
 #include "xfs_mount.h"
 #include "xfs_inode.h"
 #include "xfs_error.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_trans_priv.h"
 #include "xfs_quota.h"
diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
index 542927321a61..7848911f5e98 100644
--- a/fs/xfs/xfs_trans_inode.c
+++ b/fs/xfs/xfs_trans_inode.c
@@ -11,6 +11,7 @@
 #include "xfs_trans_resv.h"
 #include "xfs_mount.h"
 #include "xfs_inode.h"
+#include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_trans_priv.h"
 #include "xfs_inode_item.h"
-- 
2.17.1

[PATCH 09/14] xfs: use internal dfops in cow blocks cancel

[Brian Foster]

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 <bfoster@redhat.com>
---
 fs/xfs/xfs_reflink.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 3143889097f1..276deb8ca531 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -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 &&
+			       (*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;
 }
 
-- 
2.17.1

[PATCH 10/14] xfs: use internal dfops in attr code

[Brian Foster]

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 <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_attr.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 927d4c968f9a..66a22c80a0db 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -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);
 	/*
-- 
2.17.1

[PATCH 11/14] xfs: use internal dfops during [b|c]ui recovery

[Brian Foster]

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 <bfoster@redhat.com>
---
 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(-)

diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 478bfc798861..bc5eb2e0ab0c 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -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);
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 3289811eb076..958e9b96dc6a 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -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;
 
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 2064c689bc72..d3582a06626f 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -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;
 }
-- 
2.17.1

[PATCH 12/14] xfs: remove all boilerplate defer init/finish code

[Brian Foster]

At this point, the transaction subsystem completely manages deferred
items internally such that the common and boilerplate
xfs_trans_alloc() -> xfs_defer_init() -> xfs_defer_finish() ->
xfs_trans_commit() sequence can be replaced with a simple
transaction allocation and commit.

Remove all such boilerplate deferred ops code. In doing so, we
change each case over to use the dfops in the transaction and
specifically eliminate:

- The on-stack dfops and associated xfs_defer_init() call, as the
  internal dfops is initialized on transaction allocation.
- xfs_bmap_finish() calls that precede a final xfs_trans_commit() of
  a transaction.
- xfs_defer_cancel() calls in error handlers that precede a
  transaction cancel.

The only deferred ops calls that remain are those that are
non-deterministic with respect to the final commit of the associated
transaction or are open-coded due to special handling.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_bmap.c     | 16 +-------
 fs/xfs/libxfs/xfs_refcount.c | 10 +----
 fs/xfs/xfs_attr_inactive.c   |  2 -
 fs/xfs/xfs_bmap_util.c       | 43 +++-----------------
 fs/xfs/xfs_dquot.c           |  4 --
 fs/xfs/xfs_inode.c           | 79 ++++++------------------------------
 fs/xfs/xfs_iomap.c           | 26 +-----------
 fs/xfs/xfs_iops.c            |  2 -
 fs/xfs/xfs_log_recover.c     |  8 ----
 fs/xfs/xfs_qm_syscalls.c     |  2 -
 fs/xfs/xfs_reflink.c         | 37 ++++++-----------
 fs/xfs/xfs_rtalloc.c         |  9 +---
 fs/xfs/xfs_symlink.c         | 38 ++++-------------
 13 files changed, 44 insertions(+), 232 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 7b93b1e16ad9..60138514ea86 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1019,7 +1019,6 @@ xfs_bmap_add_attrfork(
 	int			size,		/* space new attribute needs */
 	int			rsvd)		/* xact may use reserved blks */
 {
-	struct xfs_defer_ops	dfops;		/* freed extent records */
 	xfs_mount_t		*mp;		/* mount structure */
 	xfs_trans_t		*tp;		/* transaction pointer */
 	int			blks;		/* space reservation */
@@ -1038,7 +1037,6 @@ xfs_bmap_add_attrfork(
 			rsvd ? XFS_TRANS_RESERVE : 0, &tp);
 	if (error)
 		return error;
-	xfs_defer_init(tp, &dfops);
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	error = xfs_trans_reserve_quota_nblks(tp, ip, blks, 0, rsvd ?
@@ -1103,7 +1101,7 @@ xfs_bmap_add_attrfork(
 	if (logflags)
 		xfs_trans_log_inode(tp, ip, logflags);
 	if (error)
-		goto bmap_cancel;
+		goto trans_cancel;
 	if (!xfs_sb_version_hasattr(&mp->m_sb) ||
 	   (!xfs_sb_version_hasattr2(&mp->m_sb) && version == 2)) {
 		bool log_sb = false;
@@ -1122,15 +1120,10 @@ xfs_bmap_add_attrfork(
 			xfs_log_sb(tp);
 	}
 
-	error = xfs_defer_finish(&tp, &dfops);
-	if (error)
-		goto bmap_cancel;
 	error = xfs_trans_commit(tp);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	return error;
 
-bmap_cancel:
-	xfs_defer_cancel(&dfops);
 trans_cancel:
 	xfs_trans_cancel(tp);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
@@ -5961,14 +5954,12 @@ xfs_bmap_split_extent(
 {
 	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_write,
 			XFS_DIOSTRAT_SPACE_RES(mp, 0), 0, 0, &tp);
 	if (error)
 		return error;
-	xfs_defer_init(tp, &dfops);
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
@@ -5977,14 +5968,9 @@ xfs_bmap_split_extent(
 	if (error)
 		goto out;
 
-	error = xfs_defer_finish(&tp, &dfops);
-	if (error)
-		goto out;
-
 	return xfs_trans_commit(tp);
 
 out:
-	xfs_defer_cancel(&dfops);
 	xfs_trans_cancel(tp);
 	return error;
 }
diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
index 2ecfb0518580..85e2d7c56c14 100644
--- a/fs/xfs/libxfs/xfs_refcount.c
+++ b/fs/xfs/libxfs/xfs_refcount.c
@@ -1635,7 +1635,6 @@ xfs_refcount_recover_cow_leftovers(
 	struct list_head		debris;
 	union xfs_btree_irec		low;
 	union xfs_btree_irec		high;
-	struct xfs_defer_ops		dfops;
 	xfs_fsblock_t			fsb;
 	xfs_agblock_t			agbno;
 	int				error;
@@ -1691,22 +1690,17 @@ xfs_refcount_recover_cow_leftovers(
 		trace_xfs_refcount_recover_extent(mp, agno, &rr->rr_rrec);
 
 		/* Free the orphan record */
-		xfs_defer_init(tp, &dfops);
 		agbno = rr->rr_rrec.rc_startblock - XFS_REFC_COW_START;
 		fsb = XFS_AGB_TO_FSB(mp, agno, agbno);
 		error = xfs_refcount_free_cow_extent(mp, tp->t_dfops, fsb,
 				rr->rr_rrec.rc_blockcount);
 		if (error)
-			goto out_defer;
+			goto out_trans;
 
 		/* Free the block. */
 		xfs_bmap_add_free(mp, tp->t_dfops, fsb,
 				rr->rr_rrec.rc_blockcount, NULL);
 
-		error = xfs_defer_finish(&tp, tp->t_dfops);
-		if (error)
-			goto out_defer;
-
 		error = xfs_trans_commit(tp);
 		if (error)
 			goto out_free;
@@ -1716,8 +1710,6 @@ xfs_refcount_recover_cow_leftovers(
 	}
 
 	return error;
-out_defer:
-	xfs_defer_cancel(tp->t_dfops);
 out_trans:
 	xfs_trans_cancel(tp);
 out_free:
diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
index cb0ad9b6678e..f755ceb9bc65 100644
--- a/fs/xfs/xfs_attr_inactive.c
+++ b/fs/xfs/xfs_attr_inactive.c
@@ -382,7 +382,6 @@ 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;
 
@@ -399,7 +398,6 @@ 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);
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index ee9481b142f0..6b0f31402a81 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -792,7 +792,6 @@ 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
@@ -832,7 +831,6 @@ 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);
@@ -880,7 +878,6 @@ xfs_alloc_file_space(
 	int			rt;
 	xfs_trans_t		*tp;
 	xfs_bmbt_irec_t		imaps[1], *imapp;
-	struct xfs_defer_ops	dfops;
 	uint			qblocks, resblks, resrtextents;
 	int			error;
 
@@ -973,7 +970,6 @@ xfs_alloc_file_space(
 
 		xfs_trans_ijoin(tp, ip, 0);
 
-		xfs_defer_init(tp, &dfops);
 		error = xfs_bmapi_write(tp, ip, startoffset_fsb,
 					allocatesize_fsb, alloc_type, resblks,
 					imapp, &nimaps);
@@ -983,10 +979,6 @@ xfs_alloc_file_space(
 		/*
 		 * Complete the transaction
 		 */
-		error = xfs_defer_finish(&tp, tp->t_dfops);
-		if (error)
-			goto error0;
-
 		error = xfs_trans_commit(tp);
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);
 		if (error)
@@ -1005,8 +997,7 @@ xfs_alloc_file_space(
 
 	return error;
 
-error0:	/* Cancel bmap, unlock inode, unreserve quota blocks, cancel trans */
-	xfs_defer_cancel(&dfops);
+error0:	/* unlock inode, unreserve quota blocks, cancel trans */
 	xfs_trans_unreserve_quota_nblks(tp, ip, (long)qblocks, 0, quota_flag);
 
 error1:	/* Just cancel transaction */
@@ -1024,7 +1015,6 @@ xfs_unmap_extent(
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*tp;
-	struct xfs_defer_ops	dfops;
 	uint			resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
 	int			error;
 
@@ -1042,23 +1032,17 @@ xfs_unmap_extent(
 
 	xfs_trans_ijoin(tp, ip, 0);
 
-	xfs_defer_init(tp, &dfops);
 	error = xfs_bunmapi(tp, ip, startoffset_fsb, len_fsb, 0, 2, done);
 	if (error)
-		goto out_bmap_cancel;
+		goto out_trans_cancel;
 
 	xfs_defer_ijoin(tp->t_dfops, ip);
-	error = xfs_defer_finish(&tp, tp->t_dfops);
-	if (error)
-		goto out_bmap_cancel;
 
 	error = xfs_trans_commit(tp);
 out_unlock:
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	return error;
 
-out_bmap_cancel:
-	xfs_defer_cancel(tp->t_dfops);
 out_trans_cancel:
 	xfs_trans_cancel(tp);
 	goto out_unlock;
@@ -1310,7 +1294,6 @@ xfs_collapse_file_space(
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*tp;
 	int			error;
-	struct xfs_defer_ops	dfops;
 	xfs_fileoff_t		next_fsb = XFS_B_TO_FSB(mp, offset + len);
 	xfs_fileoff_t		shift_fsb = XFS_B_TO_FSB(mp, len);
 	uint			resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
@@ -1343,22 +1326,16 @@ xfs_collapse_file_space(
 			goto out_trans_cancel;
 		xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
 
-		xfs_defer_init(tp, &dfops);
 		error = xfs_bmap_collapse_extents(tp, ip, &next_fsb, shift_fsb,
 				&done);
 		if (error)
-			goto out_bmap_cancel;
+			goto out_trans_cancel;
 
-		error = xfs_defer_finish(&tp, tp->t_dfops);
-		if (error)
-			goto out_bmap_cancel;
 		error = xfs_trans_commit(tp);
 	}
 
 	return error;
 
-out_bmap_cancel:
-	xfs_defer_cancel(tp->t_dfops);
 out_trans_cancel:
 	xfs_trans_cancel(tp);
 	return error;
@@ -1385,7 +1362,6 @@ xfs_insert_file_space(
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*tp;
 	int			error;
-	struct xfs_defer_ops	dfops;
 	xfs_fileoff_t		stop_fsb = XFS_B_TO_FSB(mp, offset);
 	xfs_fileoff_t		next_fsb = NULLFSBLOCK;
 	xfs_fileoff_t		shift_fsb = XFS_B_TO_FSB(mp, len);
@@ -1421,22 +1397,17 @@ xfs_insert_file_space(
 
 		xfs_ilock(ip, XFS_ILOCK_EXCL);
 		xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
-		xfs_defer_init(tp, &dfops);
 		error = xfs_bmap_insert_extents(tp, ip, &next_fsb, shift_fsb,
 				&done, stop_fsb);
 		if (error)
-			goto out_bmap_cancel;
+			goto out_trans_cancel;
 
-		error = xfs_defer_finish(&tp, tp->t_dfops);
-		if (error)
-			goto out_bmap_cancel;
 		error = xfs_trans_commit(tp);
 	}
 
 	return error;
 
-out_bmap_cancel:
-	xfs_defer_cancel(tp->t_dfops);
+out_trans_cancel:
 	xfs_trans_cancel(tp);
 	return error;
 }
@@ -1607,7 +1578,7 @@ xfs_swap_extent_rmap(
 
 		/* Unmap the old blocks in the source file. */
 		while (tirec.br_blockcount) {
-			xfs_defer_init(tp, tp->t_dfops);
+			ASSERT(tp->t_firstblock == NULLFSBLOCK);
 			trace_xfs_swap_extent_rmap_remap_piece(tip, &tirec);
 
 			/* Read extent from the source file */
@@ -1841,7 +1812,6 @@ xfs_swap_extents(
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*tp;
-	struct xfs_defer_ops	dfops;
 	struct xfs_bstat	*sbp = &sxp->sx_stat;
 	int			src_log_flags, target_log_flags;
 	int			error = 0;
@@ -1911,7 +1881,6 @@ xfs_swap_extents(
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, 0, &tp);
 	if (error)
 		goto out_unlock;
-	xfs_defer_init(tp, &dfops);
 
 	/*
 	 * Lock and join the inodes to the tansaction so that transaction commit
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index c53de34c9ae5..a57d5e8c3118 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -295,8 +295,6 @@ xfs_dquot_disk_alloc(
 
 	trace_xfs_dqalloc(dqp);
 
-	xfs_defer_init(tp, tp->t_dfops);
-
 	xfs_ilock(quotip, XFS_ILOCK_EXCL);
 	if (!xfs_this_quota_on(dqp->q_mount, dqp->dq_flags)) {
 		/*
@@ -538,7 +536,6 @@ xfs_qm_dqread_alloc(
 	struct xfs_buf		**bpp)
 {
 	struct xfs_trans	*tp;
-	struct xfs_defer_ops	dfops;
 	struct xfs_buf		*bp;
 	int			error;
 
@@ -546,7 +543,6 @@ xfs_qm_dqread_alloc(
 			XFS_QM_DQALLOC_SPACE_RES(mp), 0, 0, &tp);
 	if (error)
 		goto err;
-	xfs_defer_init(tp, &dfops);
 
 	error = xfs_dquot_disk_alloc(&tp, dqp, &bp);
 	if (error)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 7d7d7e95fa17..c47183a2f167 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1142,7 +1142,6 @@ xfs_create(
 	struct xfs_inode	*ip = NULL;
 	struct xfs_trans	*tp = NULL;
 	int			error;
-	struct xfs_defer_ops	dfops;
 	bool                    unlock_dp_on_error = false;
 	prid_t			prid;
 	struct xfs_dquot	*udqp = NULL;
@@ -1194,8 +1193,6 @@ xfs_create(
 	xfs_ilock(dp, XFS_ILOCK_EXCL | XFS_ILOCK_PARENT);
 	unlock_dp_on_error = true;
 
-	xfs_defer_init(tp, &dfops);
-
 	/*
 	 * Reserve disk quota and the inode.
 	 */
@@ -1236,11 +1233,11 @@ xfs_create(
 	if (is_dir) {
 		error = xfs_dir_init(tp, ip, dp);
 		if (error)
-			goto out_bmap_cancel;
+			goto out_trans_cancel;
 
 		error = xfs_bumplink(tp, dp);
 		if (error)
-			goto out_bmap_cancel;
+			goto out_trans_cancel;
 	}
 
 	/*
@@ -1258,10 +1255,6 @@ xfs_create(
 	 */
 	xfs_qm_vop_create_dqattach(tp, ip, udqp, gdqp, pdqp);
 
-	error = xfs_defer_finish(&tp, &dfops);
-	if (error)
-		goto out_bmap_cancel;
-
 	error = xfs_trans_commit(tp);
 	if (error)
 		goto out_release_inode;
@@ -1273,8 +1266,6 @@ xfs_create(
 	*ipp = ip;
 	return 0;
 
- out_bmap_cancel:
-	xfs_defer_cancel(&dfops);
  out_trans_cancel:
 	xfs_trans_cancel(tp);
  out_release_inode:
@@ -1399,7 +1390,6 @@ xfs_link(
 	xfs_mount_t		*mp = tdp->i_mount;
 	xfs_trans_t		*tp;
 	int			error;
-	struct xfs_defer_ops	dfops;
 	int			resblks;
 
 	trace_xfs_link(tdp, target_name);
@@ -1448,8 +1438,6 @@ xfs_link(
 			goto error_return;
 	}
 
-	xfs_defer_init(tp, &dfops);
-
 	/*
 	 * Handle initial link state of O_TMPFILE inode
 	 */
@@ -1478,12 +1466,6 @@ xfs_link(
 	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
 		xfs_trans_set_sync(tp);
 
-	error = xfs_defer_finish(&tp, &dfops);
-	if (error) {
-		xfs_defer_cancel(&dfops);
-		goto error_return;
-	}
-
 	return xfs_trans_commit(tp);
 
  error_return:
@@ -1719,7 +1701,6 @@ 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);
@@ -1727,8 +1708,6 @@ 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);
 
@@ -1769,7 +1748,6 @@ STATIC int
 xfs_inactive_ifree(
 	struct xfs_inode *ip)
 {
-	struct xfs_defer_ops	dfops;
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*tp;
 	int			error;
@@ -1806,7 +1784,6 @@ xfs_inactive_ifree(
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, ip, 0);
 
-	xfs_defer_init(tp, &dfops);
 	error = xfs_ifree(tp, ip);
 	if (error) {
 		/*
@@ -1833,12 +1810,6 @@ xfs_inactive_ifree(
 	 * Just ignore errors at this point.  There is nothing we can do except
 	 * to try to keep going. Make sure it's not a silent error.
 	 */
-	error = xfs_defer_finish(&tp, &dfops);
-	if (error) {
-		xfs_notice(mp, "%s: xfs_defer_finish returned error %d",
-			__func__, error);
-		xfs_defer_cancel(&dfops);
-	}
 	error = xfs_trans_commit(tp);
 	if (error)
 		xfs_notice(mp, "%s: xfs_trans_commit returned error %d",
@@ -2569,7 +2540,6 @@ xfs_remove(
 	xfs_trans_t             *tp = NULL;
 	int			is_dir = S_ISDIR(VFS_I(ip)->i_mode);
 	int                     error = 0;
-	struct xfs_defer_ops	dfops;
 	uint			resblks;
 
 	trace_xfs_remove(dp, name);
@@ -2649,11 +2619,10 @@ xfs_remove(
 	if (error)
 		goto out_trans_cancel;
 
-	xfs_defer_init(tp, &dfops);
 	error = xfs_dir_removename(tp, dp, name, ip->i_ino, resblks);
 	if (error) {
 		ASSERT(error != -ENOENT);
-		goto out_bmap_cancel;
+		goto out_trans_cancel;
 	}
 
 	/*
@@ -2664,10 +2633,6 @@ xfs_remove(
 	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
 		xfs_trans_set_sync(tp);
 
-	error = xfs_defer_finish(&tp, &dfops);
-	if (error)
-		goto out_bmap_cancel;
-
 	error = xfs_trans_commit(tp);
 	if (error)
 		goto std_return;
@@ -2677,8 +2642,6 @@ xfs_remove(
 
 	return 0;
 
- out_bmap_cancel:
-	xfs_defer_cancel(&dfops);
  out_trans_cancel:
 	xfs_trans_cancel(tp);
  std_return:
@@ -2740,9 +2703,6 @@ static int
 xfs_finish_rename(
 	struct xfs_trans	*tp)
 {
-	struct xfs_defer_ops	*dfops = tp->t_dfops;
-	int			error;
-
 	/*
 	 * If this is a synchronous mount, make sure that the rename transaction
 	 * goes to disk before returning to the user.
@@ -2750,13 +2710,6 @@ xfs_finish_rename(
 	if (tp->t_mountp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
 		xfs_trans_set_sync(tp);
 
-	error = xfs_defer_finish(&tp, dfops);
-	if (error) {
-		xfs_defer_cancel(dfops);
-		xfs_trans_cancel(tp);
-		return error;
-	}
-
 	return xfs_trans_commit(tp);
 }
 
@@ -2869,7 +2822,6 @@ xfs_cross_rename(
 	return xfs_finish_rename(tp);
 
 out_trans_abort:
-	xfs_defer_cancel(tp->t_dfops);
 	xfs_trans_cancel(tp);
 	return error;
 }
@@ -2924,7 +2876,6 @@ xfs_rename(
 {
 	struct xfs_mount	*mp = src_dp->i_mount;
 	struct xfs_trans	*tp;
-	struct xfs_defer_ops	dfops;
 	struct xfs_inode	*wip = NULL;		/* whiteout inode */
 	struct xfs_inode	*inodes[__XFS_SORT_INODES];
 	int			num_inodes = __XFS_SORT_INODES;
@@ -3006,8 +2957,6 @@ xfs_rename(
 		goto out_trans_cancel;
 	}
 
-	xfs_defer_init(tp, &dfops);
-
 	/* RENAME_EXCHANGE is unique from here on. */
 	if (flags & RENAME_EXCHANGE)
 		return xfs_cross_rename(tp, src_dp, src_name, src_ip,
@@ -3035,7 +2984,7 @@ xfs_rename(
 		error = xfs_dir_createname(tp, target_dp, target_name,
 					   src_ip->i_ino, spaceres);
 		if (error)
-			goto out_bmap_cancel;
+			goto out_trans_cancel;
 
 		xfs_trans_ichgtime(tp, target_dp,
 					XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
@@ -3043,7 +2992,7 @@ xfs_rename(
 		if (new_parent && src_is_directory) {
 			error = xfs_bumplink(tp, target_dp);
 			if (error)
-				goto out_bmap_cancel;
+				goto out_trans_cancel;
 		}
 	} else { /* target_ip != NULL */
 		/*
@@ -3074,7 +3023,7 @@ xfs_rename(
 		error = xfs_dir_replace(tp, target_dp, target_name,
 					src_ip->i_ino, spaceres);
 		if (error)
-			goto out_bmap_cancel;
+			goto out_trans_cancel;
 
 		xfs_trans_ichgtime(tp, target_dp,
 					XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
@@ -3085,7 +3034,7 @@ xfs_rename(
 		 */
 		error = xfs_droplink(tp, target_ip);
 		if (error)
-			goto out_bmap_cancel;
+			goto out_trans_cancel;
 
 		if (src_is_directory) {
 			/*
@@ -3093,7 +3042,7 @@ xfs_rename(
 			 */
 			error = xfs_droplink(tp, target_ip);
 			if (error)
-				goto out_bmap_cancel;
+				goto out_trans_cancel;
 		}
 	} /* target_ip != NULL */
 
@@ -3109,7 +3058,7 @@ xfs_rename(
 					target_dp->i_ino, spaceres);
 		ASSERT(error != -EEXIST);
 		if (error)
-			goto out_bmap_cancel;
+			goto out_trans_cancel;
 	}
 
 	/*
@@ -3135,7 +3084,7 @@ xfs_rename(
 		 */
 		error = xfs_droplink(tp, src_dp);
 		if (error)
-			goto out_bmap_cancel;
+			goto out_trans_cancel;
 	}
 
 	/*
@@ -3150,7 +3099,7 @@ xfs_rename(
 		error = xfs_dir_removename(tp, src_dp, src_name, src_ip->i_ino,
 					   spaceres);
 	if (error)
-		goto out_bmap_cancel;
+		goto out_trans_cancel;
 
 	/*
 	 * For whiteouts, we need to bump the link count on the whiteout inode.
@@ -3164,10 +3113,10 @@ xfs_rename(
 		ASSERT(VFS_I(wip)->i_nlink == 0);
 		error = xfs_bumplink(tp, wip);
 		if (error)
-			goto out_bmap_cancel;
+			goto out_trans_cancel;
 		error = xfs_iunlink_remove(tp, wip);
 		if (error)
-			goto out_bmap_cancel;
+			goto out_trans_cancel;
 		xfs_trans_log_inode(tp, wip, XFS_ILOG_CORE);
 
 		/*
@@ -3188,8 +3137,6 @@ xfs_rename(
 		IRELE(wip);
 	return error;
 
-out_bmap_cancel:
-	xfs_defer_cancel(&dfops);
 out_trans_cancel:
 	xfs_trans_cancel(tp);
 out_release_wip:
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 756694219f77..8e8ca9f03f0e 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -157,7 +157,6 @@ xfs_iomap_write_direct(
 	int		quota_flag;
 	int		rt;
 	xfs_trans_t	*tp;
-	struct xfs_defer_ops dfops;
 	uint		qblocks, resblks, resrtextents;
 	int		error;
 	int		lockmode;
@@ -253,20 +252,15 @@ xfs_iomap_write_direct(
 	 * From this point onwards we overwrite the imap pointer that the
 	 * caller gave to us.
 	 */
-	xfs_defer_init(tp, &dfops);
 	nimaps = 1;
 	error = xfs_bmapi_write(tp, ip, offset_fsb, count_fsb,
 				bmapi_flags, resblks, imap, &nimaps);
 	if (error)
-		goto out_bmap_cancel;
+		goto out_res_cancel;
 
 	/*
 	 * Complete the transaction
 	 */
-	error = xfs_defer_finish(&tp, tp->t_dfops);
-	if (error)
-		goto out_bmap_cancel;
-
 	error = xfs_trans_commit(tp);
 	if (error)
 		goto out_unlock;
@@ -286,8 +280,7 @@ xfs_iomap_write_direct(
 	xfs_iunlock(ip, lockmode);
 	return error;
 
-out_bmap_cancel:
-	xfs_defer_cancel(tp->t_dfops);
+out_res_cancel:
 	xfs_trans_unreserve_quota_nblks(tp, ip, (long)qblocks, 0, quota_flag);
 out_trans_cancel:
 	xfs_trans_cancel(tp);
@@ -663,7 +656,6 @@ xfs_iomap_write_allocate(
 	xfs_mount_t	*mp = ip->i_mount;
 	xfs_fileoff_t	offset_fsb, last_block;
 	xfs_fileoff_t	end_fsb, map_start_fsb;
-	struct xfs_defer_ops	dfops;
 	xfs_filblks_t	count_fsb;
 	xfs_trans_t	*tp;
 	int		nimaps;
@@ -713,8 +705,6 @@ xfs_iomap_write_allocate(
 			xfs_ilock(ip, XFS_ILOCK_EXCL);
 			xfs_trans_ijoin(tp, ip, 0);
 
-			xfs_defer_init(tp, &dfops);
-
 			/*
 			 * it is possible that the extents have changed since
 			 * we did the read call as we dropped the ilock for a
@@ -772,10 +762,6 @@ xfs_iomap_write_allocate(
 			if (error)
 				goto trans_cancel;
 
-			error = xfs_defer_finish(&tp, tp->t_dfops);
-			if (error)
-				goto trans_cancel;
-
 			error = xfs_trans_commit(tp);
 			if (error)
 				goto error0;
@@ -806,7 +792,6 @@ xfs_iomap_write_allocate(
 	}
 
 trans_cancel:
-	xfs_defer_cancel(tp->t_dfops);
 	xfs_trans_cancel(tp);
 error0:
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
@@ -827,7 +812,6 @@ xfs_iomap_write_unwritten(
 	int		nimaps;
 	xfs_trans_t	*tp;
 	xfs_bmbt_irec_t imap;
-	struct xfs_defer_ops dfops;
 	struct inode	*inode = VFS_I(ip);
 	xfs_fsize_t	i_size;
 	uint		resblks;
@@ -872,7 +856,6 @@ xfs_iomap_write_unwritten(
 		/*
 		 * Modify the unwritten extent state of the buffer.
 		 */
-		xfs_defer_init(tp, &dfops);
 		nimaps = 1;
 		error = xfs_bmapi_write(tp, ip, offset_fsb, count_fsb,
 					XFS_BMAPI_CONVERT, resblks, &imap,
@@ -896,10 +879,6 @@ xfs_iomap_write_unwritten(
 			xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 		}
 
-		error = xfs_defer_finish(&tp, tp->t_dfops);
-		if (error)
-			goto error_on_bmapi_transaction;
-
 		error = xfs_trans_commit(tp);
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);
 		if (error)
@@ -923,7 +902,6 @@ xfs_iomap_write_unwritten(
 	return 0;
 
 error_on_bmapi_transaction:
-	xfs_defer_cancel(tp->t_dfops);
 	xfs_trans_cancel(tp);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	return error;
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 32bbd5001b9f..a6bda1afbbed 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -813,7 +813,6 @@ 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;
@@ -917,7 +916,6 @@ 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);
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 958e9b96dc6a..265e1f561157 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -4857,15 +4857,7 @@ xlog_finish_defer_ops(
 	/* transfer all collected dfops to this transaction */
 	xfs_defer_move(tp->t_dfops, dfops);
 
-	error = xfs_defer_finish(&tp, tp->t_dfops);
-	if (error)
-		goto out_cancel;
-
 	return xfs_trans_commit(tp);
-
-out_cancel:
-	xfs_trans_cancel(tp);
-	return error;
 }
 
 /*
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 94ca3bff8d07..1604fd7c307e 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -214,7 +214,6 @@ xfs_qm_scall_trunc_qfile(
 {
 	struct xfs_inode	*ip;
 	struct xfs_trans	*tp;
-	struct xfs_defer_ops	dfops;
 	int			error;
 
 	if (ino == NULLFSINO)
@@ -231,7 +230,6 @@ 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);
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 276deb8ca531..f46983b1b302 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -364,7 +364,6 @@ xfs_reflink_allocate_cow(
 	xfs_fileoff_t		offset_fsb = imap->br_startoff;
 	xfs_filblks_t		count_fsb = imap->br_blockcount;
 	struct xfs_bmbt_irec	got;
-	struct xfs_defer_ops	dfops;
 	struct xfs_trans	*tp = NULL;
 	int			nimaps, error = 0;
 	bool			trimmed;
@@ -424,7 +423,6 @@ xfs_reflink_allocate_cow(
 
 	xfs_trans_ijoin(tp, ip, 0);
 
-	xfs_defer_init(tp, &dfops);
 	nimaps = 1;
 
 	/* Allocate the entire reservation as unwritten blocks. */
@@ -432,15 +430,11 @@ xfs_reflink_allocate_cow(
 			XFS_BMAPI_COWFORK | XFS_BMAPI_PREALLOC,
 			resblks, imap, &nimaps);
 	if (error)
-		goto out_bmap_cancel;
+		goto out_trans_cancel;
 
 	xfs_inode_set_cowblocks_tag(ip);
 
 	/* Finish up. */
-	error = xfs_defer_finish(&tp, tp->t_dfops);
-	if (error)
-		goto out_bmap_cancel;
-
 	error = xfs_trans_commit(tp);
 	if (error)
 		return error;
@@ -453,8 +447,7 @@ xfs_reflink_allocate_cow(
 		return -ENOSPC;
 convert:
 	return xfs_reflink_convert_cow_extent(ip, imap, offset_fsb, count_fsb);
-out_bmap_cancel:
-	xfs_defer_cancel(tp->t_dfops);
+out_trans_cancel:
 	xfs_trans_unreserve_quota_nblks(tp, ip, (long)resblks, 0,
 			XFS_QMOPT_RES_REGBLKS);
 out:
@@ -624,7 +617,6 @@ xfs_reflink_end_cow(
 	struct xfs_trans		*tp;
 	xfs_fileoff_t			offset_fsb;
 	xfs_fileoff_t			end_fsb;
-	struct xfs_defer_ops		dfops;
 	int				error;
 	unsigned int			resblks;
 	xfs_filblks_t			rlen;
@@ -691,11 +683,11 @@ xfs_reflink_end_cow(
 			goto prev_extent;
 
 		/* Unmap the old blocks in the data fork. */
-		xfs_defer_init(tp, &dfops);
+		ASSERT(tp->t_dfops && tp->t_firstblock == NULLFSBLOCK);
 		rlen = del.br_blockcount;
 		error = __xfs_bunmapi(tp, ip, del.br_startoff, &rlen, 0, 1);
 		if (error)
-			goto out_defer;
+			goto out_cancel;
 
 		/* Trim the extent to whatever got unmapped. */
 		if (rlen) {
@@ -708,13 +700,13 @@ xfs_reflink_end_cow(
 		error = xfs_refcount_free_cow_extent(tp->t_mountp, tp->t_dfops,
 				del.br_startblock, del.br_blockcount);
 		if (error)
-			goto out_defer;
+			goto out_cancel;
 
 		/* Map the new blocks into the data fork. */
 		error = xfs_bmap_map_extent(tp->t_mountp, tp->t_dfops, ip,
 					    &del);
 		if (error)
-			goto out_defer;
+			goto out_cancel;
 
 		/* Charge this new data fork mapping to the on-disk quota. */
 		xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_DELBCOUNT,
@@ -726,7 +718,7 @@ xfs_reflink_end_cow(
 		xfs_defer_ijoin(tp->t_dfops, ip);
 		error = xfs_defer_finish(&tp, tp->t_dfops);
 		if (error)
-			goto out_defer;
+			goto out_cancel;
 		if (!xfs_iext_get_extent(ifp, &icur, &got))
 			break;
 		continue;
@@ -741,8 +733,6 @@ xfs_reflink_end_cow(
 		goto out;
 	return 0;
 
-out_defer:
-	xfs_defer_cancel(tp->t_dfops);
 out_cancel:
 	xfs_trans_cancel(tp);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
@@ -998,7 +988,6 @@ xfs_reflink_remap_extent(
 	bool			real_extent = xfs_bmap_is_real_extent(irec);
 	struct xfs_trans	*tp;
 	unsigned int		resblks;
-	struct xfs_defer_ops	dfops;
 	struct xfs_bmbt_irec	uirec;
 	xfs_filblks_t		rlen;
 	xfs_filblks_t		unmap_len;
@@ -1039,10 +1028,10 @@ xfs_reflink_remap_extent(
 	/* Unmap the old blocks in the data fork. */
 	rlen = unmap_len;
 	while (rlen) {
-		xfs_defer_init(tp, &dfops);
+		ASSERT(tp->t_dfops && tp->t_firstblock == NULLFSBLOCK);
 		error = __xfs_bunmapi(tp, ip, destoff, &rlen, 0, 1);
 		if (error)
-			goto out_defer;
+			goto out_cancel;
 
 		/*
 		 * Trim the extent to whatever got unmapped.
@@ -1063,12 +1052,12 @@ xfs_reflink_remap_extent(
 		/* Update the refcount tree */
 		error = xfs_refcount_increase_extent(mp, tp->t_dfops, &uirec);
 		if (error)
-			goto out_defer;
+			goto out_cancel;
 
 		/* Map the new blocks into the data fork. */
 		error = xfs_bmap_map_extent(mp, tp->t_dfops, ip, &uirec);
 		if (error)
-			goto out_defer;
+			goto out_cancel;
 
 		/* Update quota accounting. */
 		xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT,
@@ -1090,7 +1079,7 @@ xfs_reflink_remap_extent(
 		xfs_defer_ijoin(tp->t_dfops, ip);
 		error = xfs_defer_finish(&tp, tp->t_dfops);
 		if (error)
-			goto out_defer;
+			goto out_cancel;
 	}
 
 	error = xfs_trans_commit(tp);
@@ -1099,8 +1088,6 @@ xfs_reflink_remap_extent(
 		goto out;
 	return 0;
 
-out_defer:
-	xfs_defer_cancel(tp->t_dfops);
 out_cancel:
 	xfs_trans_cancel(tp);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index bc471d42a968..86d7d2f76226 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -761,7 +761,6 @@ xfs_growfs_rt_alloc(
 	struct xfs_buf		*bp;	/* temporary buffer for zeroing */
 	xfs_daddr_t		d;		/* disk block address */
 	int			error;		/* error return value */
-	struct xfs_defer_ops	dfops;		/* list of freed blocks */
 	xfs_fsblock_t		fsbno;		/* filesystem block for bno */
 	struct xfs_bmbt_irec	map;		/* block map output */
 	int			nmap;		/* number of block maps */
@@ -786,7 +785,6 @@ xfs_growfs_rt_alloc(
 		xfs_ilock(ip, XFS_ILOCK_EXCL);
 		xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
 
-		xfs_defer_init(tp, &dfops);
 		/*
 		 * Allocate blocks to the bitmap file.
 		 */
@@ -797,13 +795,10 @@ xfs_growfs_rt_alloc(
 		if (!error && nmap < 1)
 			error = -ENOSPC;
 		if (error)
-			goto out_bmap_cancel;
+			goto out_trans_cancel;
 		/*
 		 * Free any blocks freed up in the transaction, then commit.
 		 */
-		error = xfs_defer_finish(&tp, tp->t_dfops);
-		if (error)
-			goto out_bmap_cancel;
 		error = xfs_trans_commit(tp);
 		if (error)
 			return error;
@@ -853,8 +848,6 @@ xfs_growfs_rt_alloc(
 
 	return 0;
 
-out_bmap_cancel:
-	xfs_defer_cancel(tp->t_dfops);
 out_trans_cancel:
 	xfs_trans_cancel(tp);
 	return error;
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index d1ab0afa2723..ce801aedbcdc 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -163,7 +163,6 @@ xfs_symlink(
 	struct xfs_inode	*ip = NULL;
 	int			error = 0;
 	int			pathlen;
-	struct xfs_defer_ops	dfops;
 	bool                    unlock_dp_on_error = false;
 	xfs_fileoff_t		first_fsb;
 	xfs_filblks_t		fs_blocks;
@@ -241,12 +240,6 @@ xfs_symlink(
 	if (error)
 		goto out_trans_cancel;
 
-	/*
-	 * Initialize the bmap freelist prior to calling either
-	 * bmapi or the directory create code.
-	 */
-	xfs_defer_init(tp, &dfops);
-
 	/*
 	 * Allocate an inode for the symlink.
 	 */
@@ -290,7 +283,7 @@ xfs_symlink(
 		error = xfs_bmapi_write(tp, ip, first_fsb, fs_blocks,
 				  XFS_BMAPI_METADATA, resblks, mval, &nmaps);
 		if (error)
-			goto out_bmap_cancel;
+			goto out_trans_cancel;
 
 		if (resblks)
 			resblks -= fs_blocks;
@@ -308,7 +301,7 @@ xfs_symlink(
 					       BTOBB(byte_cnt), 0);
 			if (!bp) {
 				error = -ENOMEM;
-				goto out_bmap_cancel;
+				goto out_trans_cancel;
 			}
 			bp->b_ops = &xfs_symlink_buf_ops;
 
@@ -337,7 +330,7 @@ xfs_symlink(
 	 */
 	error = xfs_dir_createname(tp, dp, link_name, ip->i_ino, resblks);
 	if (error)
-		goto out_bmap_cancel;
+		goto out_trans_cancel;
 	xfs_trans_ichgtime(tp, dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
 	xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
 
@@ -350,10 +343,6 @@ xfs_symlink(
 		xfs_trans_set_sync(tp);
 	}
 
-	error = xfs_defer_finish(&tp, tp->t_dfops);
-	if (error)
-		goto out_bmap_cancel;
-
 	error = xfs_trans_commit(tp);
 	if (error)
 		goto out_release_inode;
@@ -365,8 +354,6 @@ xfs_symlink(
 	*ipp = ip;
 	return 0;
 
-out_bmap_cancel:
-	xfs_defer_cancel(tp->t_dfops);
 out_trans_cancel:
 	xfs_trans_cancel(tp);
 out_release_inode:
@@ -399,7 +386,6 @@ xfs_inactive_symlink_rmt(
 	xfs_buf_t	*bp;
 	int		done;
 	int		error;
-	struct xfs_defer_ops	dfops;
 	int		i;
 	xfs_mount_t	*mp;
 	xfs_bmbt_irec_t	mval[XFS_SYMLINK_MAPS];
@@ -438,7 +424,6 @@ xfs_inactive_symlink_rmt(
 	 * Find the block(s) so we can inval and unmap them.
 	 */
 	done = 0;
-	xfs_defer_init(tp, &dfops);
 	nmaps = ARRAY_SIZE(mval);
 	error = xfs_bmapi_read(ip, 0, xfs_symlink_blocks(mp, size),
 				mval, &nmaps, 0);
@@ -453,7 +438,7 @@ xfs_inactive_symlink_rmt(
 			XFS_FSB_TO_BB(mp, mval[i].br_blockcount), 0);
 		if (!bp) {
 			error = -ENOMEM;
-			goto error_bmap_cancel;
+			goto error_trans_cancel;
 		}
 		xfs_trans_binval(tp, bp);
 	}
@@ -462,19 +447,14 @@ xfs_inactive_symlink_rmt(
 	 */
 	error = xfs_bunmapi(tp, ip, 0, size, 0, nmaps, &done);
 	if (error)
-		goto error_bmap_cancel;
+		goto error_trans_cancel;
 	ASSERT(done);
-	/*
-	 * Commit the first transaction.  This logs the EFI and the inode.
-	 */
-	xfs_defer_ijoin(tp->t_dfops, ip);
-	error = xfs_defer_finish(&tp, tp->t_dfops);
-	if (error)
-		goto error_bmap_cancel;
 
 	/*
-	 * Commit the transaction containing extent freeing and EFDs.
+	 * Commit the transaction. This first logs the EFI and the inode, then
+	 * rolls and commits the transaction that frees the extents.
 	 */
+	xfs_defer_ijoin(tp->t_dfops, ip);
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 	error = xfs_trans_commit(tp);
 	if (error) {
@@ -492,8 +472,6 @@ xfs_inactive_symlink_rmt(
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	return 0;
 
-error_bmap_cancel:
-	xfs_defer_cancel(tp->t_dfops);
 error_trans_cancel:
 	xfs_trans_cancel(tp);
 error_unlock:
-- 
2.17.1

[PATCH 13/14] xfs: remove unnecessary dfops init calls in xattr code

[Brian Foster]

Each xfs_defer_init() call in the xattr code uses the internal dfops
reference. In addition, a successful xfs_defer_finish() always
returns with a reset xfs_defer_ops structure.

Given that along with the fact that every xfs_defer_init() call in
the xattr code is followed up by an xfs_defer_finish(), the former
calls are no longer necessary and can be removed.

Note that the xfs_defer_init() call in the remote value copy loop of
xfs_attr_rmtval_set() is not followed by a finish, but the dfops is
unused in this instance.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_attr.c        | 8 --------
 fs/xfs/libxfs/xfs_attr_remote.c | 3 ---
 2 files changed, 11 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 66a22c80a0db..3e98f0af389c 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -587,7 +587,6 @@ xfs_attr_leaf_addname(
 		 * Commit that transaction so that the node_addname() call
 		 * can manage its own transactions.
 		 */
-		xfs_defer_init(args->trans, args->trans->t_dfops);
 		error = xfs_attr3_leaf_to_node(args);
 		if (error)
 			goto out_defer_cancel;
@@ -676,7 +675,6 @@ xfs_attr_leaf_addname(
 		 * If the result is small enough, shrink it all into the inode.
 		 */
 		if ((forkoff = xfs_attr_shortform_allfit(bp, dp))) {
-			xfs_defer_init(args->trans, args->trans->t_dfops);
 			error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
 			/* bp is gone due to xfs_da_shrink_inode */
 			if (error)
@@ -741,7 +739,6 @@ xfs_attr_leaf_removename(
 	 * If the result is small enough, shrink it all into the inode.
 	 */
 	if ((forkoff = xfs_attr_shortform_allfit(bp, dp))) {
-		xfs_defer_init(args->trans, args->trans->t_dfops);
 		error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
 		/* bp is gone due to xfs_da_shrink_inode */
 		if (error)
@@ -870,7 +867,6 @@ xfs_attr_node_addname(
 			 */
 			xfs_da_state_free(state);
 			state = NULL;
-			xfs_defer_init(args->trans, args->trans->t_dfops);
 			error = xfs_attr3_leaf_to_node(args);
 			if (error)
 				goto out_defer_cancel;
@@ -897,7 +893,6 @@ xfs_attr_node_addname(
 		 * in the index/blkno/rmtblkno/rmtblkcnt fields and
 		 * in the index2/blkno2/rmtblkno2/rmtblkcnt2 fields.
 		 */
-		xfs_defer_init(args->trans, args->trans->t_dfops);
 		error = xfs_da3_split(state);
 		if (error)
 			goto out_defer_cancel;
@@ -995,7 +990,6 @@ xfs_attr_node_addname(
 		 * Check to see if the tree needs to be collapsed.
 		 */
 		if (retval && (state->path.active > 1)) {
-			xfs_defer_init(args->trans, args->trans->t_dfops);
 			error = xfs_da3_join(state);
 			if (error)
 				goto out_defer_cancel;
@@ -1120,7 +1114,6 @@ xfs_attr_node_removename(
 	 * Check to see if the tree needs to be collapsed.
 	 */
 	if (retval && (state->path.active > 1)) {
-		xfs_defer_init(args->trans, args->trans->t_dfops);
 		error = xfs_da3_join(state);
 		if (error)
 			goto out_defer_cancel;
@@ -1152,7 +1145,6 @@ xfs_attr_node_removename(
 			goto out;
 
 		if ((forkoff = xfs_attr_shortform_allfit(bp, dp))) {
-			xfs_defer_init(args->trans, args->trans->t_dfops);
 			error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
 			/* bp is gone due to xfs_da_shrink_inode */
 			if (error)
diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index 829ab20f0cd7..0fbfb740949e 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -480,7 +480,6 @@ xfs_attr_rmtval_set(
 		 * extent and then crash then the block may not contain the
 		 * correct metadata after log recovery occurs.
 		 */
-		xfs_defer_init(args->trans, args->trans->t_dfops);
 		nmap = 1;
 		error = xfs_bmapi_write(args->trans, dp, (xfs_fileoff_t)lblkno,
 				  blkcnt, XFS_BMAPI_ATTRFORK, args->total, &map,
@@ -522,7 +521,6 @@ xfs_attr_rmtval_set(
 
 		ASSERT(blkcnt > 0);
 
-		xfs_defer_init(args->trans, args->trans->t_dfops);
 		nmap = 1;
 		error = xfs_bmapi_read(dp, (xfs_fileoff_t)lblkno,
 				       blkcnt, &map, &nmap,
@@ -625,7 +623,6 @@ xfs_attr_rmtval_remove(
 	blkcnt = args->rmtblkcnt;
 	done = 0;
 	while (!done) {
-		xfs_defer_init(args->trans, args->trans->t_dfops);
 		error = xfs_bunmapi(args->trans, args->dp, lblkno, blkcnt,
 				    XFS_BMAPI_ATTRFORK, 1, &done);
 		if (error)
-- 
2.17.1

[PATCH 14/14] xfs: drop unnecessary xfs_defer_finish() dfops parameter

[Brian Foster]

Every caller of xfs_defer_finish() now passes the transaction and
its associated ->t_dfops. The xfs_defer_ops parameter is therefore
no longer necessary and can be removed.

Since most xfs_defer_finish() callers also have to consider
xfs_defer_cancel() on error, update the latter to also receive the
transaction for consistency. The log recovery code contains an
outlier case that cancels a dfops directly without an available
transaction. Retain an internal wrapper to support this outlier case
for the time being.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_attr.c        | 27 +++++++++++++--------------
 fs/xfs/libxfs/xfs_attr_remote.c |  8 ++++----
 fs/xfs/libxfs/xfs_defer.c       |  7 +++----
 fs/xfs/libxfs/xfs_defer.h       |  4 ++--
 fs/xfs/xfs_bmap_util.c          |  4 ++--
 fs/xfs/xfs_dquot.c              |  4 ++--
 fs/xfs/xfs_inode.c              |  4 ++--
 fs/xfs/xfs_log_recover.c        |  2 +-
 fs/xfs/xfs_reflink.c            |  8 ++++----
 fs/xfs/xfs_trans.c              |  6 +++---
 fs/xfs/xfs_trans.h              |  8 ++++++++
 11 files changed, 44 insertions(+), 38 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 3e98f0af389c..3deb5cdadf08 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -322,7 +322,7 @@ xfs_attr_set(
 		xfs_trans_bhold(args.trans, leaf_bp);
 		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);
+		error = xfs_defer_finish(&args.trans);
 		if (error)
 			goto out;
 
@@ -591,7 +591,7 @@ xfs_attr_leaf_addname(
 		if (error)
 			goto out_defer_cancel;
 		xfs_defer_ijoin(args->trans->t_dfops, dp);
-		error = xfs_defer_finish(&args->trans, args->trans->t_dfops);
+		error = xfs_defer_finish(&args->trans);
 		if (error)
 			goto out_defer_cancel;
 
@@ -680,7 +680,7 @@ xfs_attr_leaf_addname(
 			if (error)
 				goto out_defer_cancel;
 			xfs_defer_ijoin(args->trans->t_dfops, dp);
-			error = xfs_defer_finish(&args->trans, args->trans->t_dfops);
+			error = xfs_defer_finish(&args->trans);
 			if (error)
 				goto out_defer_cancel;
 		}
@@ -698,7 +698,7 @@ xfs_attr_leaf_addname(
 	}
 	return error;
 out_defer_cancel:
-	xfs_defer_cancel(args->trans->t_dfops);
+	xfs_defer_cancel(args->trans);
 	return error;
 }
 
@@ -744,13 +744,13 @@ xfs_attr_leaf_removename(
 		if (error)
 			goto out_defer_cancel;
 		xfs_defer_ijoin(args->trans->t_dfops, dp);
-		error = xfs_defer_finish(&args->trans, args->trans->t_dfops);
+		error = xfs_defer_finish(&args->trans);
 		if (error)
 			goto out_defer_cancel;
 	}
 	return 0;
 out_defer_cancel:
-	xfs_defer_cancel(args->trans->t_dfops);
+	xfs_defer_cancel(args->trans);
 	return error;
 }
 
@@ -871,8 +871,7 @@ xfs_attr_node_addname(
 			if (error)
 				goto out_defer_cancel;
 			xfs_defer_ijoin(args->trans->t_dfops, dp);
-			error = xfs_defer_finish(&args->trans,
-						 args->trans->t_dfops);
+			error = xfs_defer_finish(&args->trans);
 			if (error)
 				goto out_defer_cancel;
 
@@ -897,7 +896,7 @@ xfs_attr_node_addname(
 		if (error)
 			goto out_defer_cancel;
 		xfs_defer_ijoin(args->trans->t_dfops, dp);
-		error = xfs_defer_finish(&args->trans, args->trans->t_dfops);
+		error = xfs_defer_finish(&args->trans);
 		if (error)
 			goto out_defer_cancel;
 	} else {
@@ -994,7 +993,7 @@ xfs_attr_node_addname(
 			if (error)
 				goto out_defer_cancel;
 			xfs_defer_ijoin(args->trans->t_dfops, dp);
-			error = xfs_defer_finish(&args->trans, args->trans->t_dfops);
+			error = xfs_defer_finish(&args->trans);
 			if (error)
 				goto out_defer_cancel;
 		}
@@ -1023,7 +1022,7 @@ xfs_attr_node_addname(
 		return error;
 	return retval;
 out_defer_cancel:
-	xfs_defer_cancel(args->trans->t_dfops);
+	xfs_defer_cancel(args->trans);
 	goto out;
 }
 
@@ -1118,7 +1117,7 @@ xfs_attr_node_removename(
 		if (error)
 			goto out_defer_cancel;
 		xfs_defer_ijoin(args->trans->t_dfops, dp);
-		error = xfs_defer_finish(&args->trans, args->trans->t_dfops);
+		error = xfs_defer_finish(&args->trans);
 		if (error)
 			goto out_defer_cancel;
 		/*
@@ -1150,7 +1149,7 @@ xfs_attr_node_removename(
 			if (error)
 				goto out_defer_cancel;
 			xfs_defer_ijoin(args->trans->t_dfops, dp);
-			error = xfs_defer_finish(&args->trans, args->trans->t_dfops);
+			error = xfs_defer_finish(&args->trans);
 			if (error)
 				goto out_defer_cancel;
 		} else
@@ -1162,7 +1161,7 @@ xfs_attr_node_removename(
 	xfs_da_state_free(state);
 	return error;
 out_defer_cancel:
-	xfs_defer_cancel(args->trans->t_dfops);
+	xfs_defer_cancel(args->trans);
 	goto out;
 }
 
diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index 0fbfb740949e..77ca38586913 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -487,7 +487,7 @@ xfs_attr_rmtval_set(
 		if (error)
 			goto out_defer_cancel;
 		xfs_defer_ijoin(args->trans->t_dfops, dp);
-		error = xfs_defer_finish(&args->trans, args->trans->t_dfops);
+		error = xfs_defer_finish(&args->trans);
 		if (error)
 			goto out_defer_cancel;
 
@@ -555,7 +555,7 @@ xfs_attr_rmtval_set(
 	ASSERT(valuelen == 0);
 	return 0;
 out_defer_cancel:
-	xfs_defer_cancel(args->trans->t_dfops);
+	xfs_defer_cancel(args->trans);
 	return error;
 }
 
@@ -628,7 +628,7 @@ xfs_attr_rmtval_remove(
 		if (error)
 			goto out_defer_cancel;
 		xfs_defer_ijoin(args->trans->t_dfops, args->dp);
-		error = xfs_defer_finish(&args->trans, args->trans->t_dfops);
+		error = xfs_defer_finish(&args->trans);
 		if (error)
 			goto out_defer_cancel;
 
@@ -641,6 +641,6 @@ xfs_attr_rmtval_remove(
 	}
 	return 0;
 out_defer_cancel:
-	xfs_defer_cancel(args->trans->t_dfops);
+	xfs_defer_cancel(args->trans);
 	return error;
 }
diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 415fcf720cc9..a46cf25b85db 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -352,9 +352,9 @@ xfs_defer_reset(
  */
 int
 xfs_defer_finish(
-	struct xfs_trans		**tp,
-	struct xfs_defer_ops		*dop)
+	struct xfs_trans		**tp)
 {
+	struct xfs_defer_ops		*dop = (*tp)->t_dfops;
 	struct xfs_defer_pending	*dfp;
 	struct list_head		*li;
 	struct list_head		*n;
@@ -363,7 +363,6 @@ xfs_defer_finish(
 	void				(*cleanup_fn)(struct xfs_trans *, void *, int);
 
 	ASSERT((*tp)->t_flags & XFS_TRANS_PERM_LOG_RES);
-	ASSERT((*tp)->t_dfops == dop);
 
 	trace_xfs_defer_finish((*tp)->t_mountp, dop, _RET_IP_);
 
@@ -456,7 +455,7 @@ xfs_defer_finish(
  * Free up any items left in the list.
  */
 void
-xfs_defer_cancel(
+__xfs_defer_cancel(
 	struct xfs_defer_ops		*dop)
 {
 	struct xfs_defer_pending	*dfp;
diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index 349b4d906fb2..b9687a0a5086 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -61,8 +61,8 @@ struct xfs_defer_ops {
 
 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);
-void xfs_defer_cancel(struct xfs_defer_ops *dop);
+int xfs_defer_finish(struct xfs_trans **tp);
+void __xfs_defer_cancel(struct xfs_defer_ops *dop);
 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);
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 6b0f31402a81..7b40e9fef381 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1624,7 +1624,7 @@ xfs_swap_extent_rmap(
 				goto out_defer;
 
 			xfs_defer_ijoin(tp->t_dfops, ip);
-			error = xfs_defer_finish(tpp, tp->t_dfops);
+			error = xfs_defer_finish(tpp);
 			tp = *tpp;
 			if (error)
 				goto out_defer;
@@ -1645,7 +1645,7 @@ xfs_swap_extent_rmap(
 	return 0;
 
 out_defer:
-	xfs_defer_cancel(tp->t_dfops);
+	xfs_defer_cancel(tp);
 out:
 	trace_xfs_swap_extent_rmap_error(ip, error, _RET_IP_);
 	tip->i_d.di_flags2 = tip_flags2;
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index a57d5e8c3118..da5c55cec966 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -368,7 +368,7 @@ xfs_dquot_disk_alloc(
 		xfs_trans_brelse(tp, bp);
 		goto error1;
 	}
-	error = xfs_defer_finish(tpp, tp->t_dfops);
+	error = xfs_defer_finish(tpp);
 	tp = *tpp;
 	if (error) {
 		xfs_buf_relse(bp);
@@ -378,7 +378,7 @@ xfs_dquot_disk_alloc(
 	return 0;
 
 error1:
-	xfs_defer_cancel(tp->t_dfops);
+	xfs_defer_cancel(tp);
 error0:
 	return error;
 }
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index c47183a2f167..0e4bd559a6a7 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1570,7 +1570,7 @@ xfs_itruncate_extents_flags(
 		 * reservation and commit the old transaction.
 		 */
 		xfs_defer_ijoin(tp->t_dfops, ip);
-		error = xfs_defer_finish(&tp, tp->t_dfops);
+		error = xfs_defer_finish(&tp);
 		if (error)
 			goto out_bmap_cancel;
 
@@ -1606,7 +1606,7 @@ xfs_itruncate_extents_flags(
 	 * the transaction can be properly aborted.  We just need to make sure
 	 * we're not holding any resources that we were not when we came in.
 	 */
-	xfs_defer_cancel(tp->t_dfops);
+	xfs_defer_cancel(tp);
 	goto out;
 }
 
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 265e1f561157..94908a4019e1 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -4946,7 +4946,7 @@ xlog_recover_process_intents(
 	xfs_trans_ail_cursor_done(&cur);
 	spin_unlock(&ailp->ail_lock);
 	if (error)
-		xfs_defer_cancel(&dfops);
+		__xfs_defer_cancel(&dfops);
 	else
 		error = xlog_finish_defer_ops(log->l_mp, &dfops);
 
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index f46983b1b302..8b394b60d9be 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -518,9 +518,9 @@ xfs_reflink_cancel_cow_blocks(
 
 			/* Roll the transaction */
 			xfs_defer_ijoin((*tpp)->t_dfops, ip);
-			error = xfs_defer_finish(tpp, (*tpp)->t_dfops);
+			error = xfs_defer_finish(tpp);
 			if (error) {
-				xfs_defer_cancel((*tpp)->t_dfops);
+				xfs_defer_cancel(*tpp);
 				break;
 			}
 
@@ -716,7 +716,7 @@ xfs_reflink_end_cow(
 		xfs_bmap_del_extent_cow(ip, &icur, &got, &del);
 
 		xfs_defer_ijoin(tp->t_dfops, ip);
-		error = xfs_defer_finish(&tp, tp->t_dfops);
+		error = xfs_defer_finish(&tp);
 		if (error)
 			goto out_cancel;
 		if (!xfs_iext_get_extent(ifp, &icur, &got))
@@ -1077,7 +1077,7 @@ xfs_reflink_remap_extent(
 next_extent:
 		/* Process all the deferred stuff. */
 		xfs_defer_ijoin(tp->t_dfops, ip);
-		error = xfs_defer_finish(&tp, tp->t_dfops);
+		error = xfs_defer_finish(&tp);
 		if (error)
 			goto out_cancel;
 	}
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 142410632a36..5c0ba0ed0878 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -933,9 +933,9 @@ __xfs_trans_commit(
 
 	/* finish deferred items on final commit */
 	if (!regrant && tp->t_dfops) {
-		error = xfs_defer_finish(&tp, tp->t_dfops);
+		error = xfs_defer_finish(&tp);
 		if (error) {
-			xfs_defer_cancel(tp->t_dfops);
+			xfs_defer_cancel(tp);
 			goto out_unreserve;
 		}
 	}
@@ -1030,7 +1030,7 @@ xfs_trans_cancel(
 	trace_xfs_trans_cancel(tp, _RET_IP_);
 
 	if (tp->t_dfops)
-		xfs_defer_cancel(tp->t_dfops);
+		xfs_defer_cancel(tp);
 
 	/*
 	 * See if the caller is relying on us to shut down the
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index bab857c36d3a..7badbe03c342 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -199,6 +199,14 @@ xfs_trans_read_buf(
 				      flags, bpp, ops);
 }
 
+/* cancel dfops associated with a transaction */
+static inline void
+xfs_defer_cancel(
+	struct xfs_trans *tp)
+{
+	__xfs_defer_cancel(tp->t_dfops);
+}
+
 struct xfs_buf	*xfs_trans_getsb(xfs_trans_t *, struct xfs_mount *, int);
 
 void		xfs_trans_brelse(xfs_trans_t *, struct xfs_buf *);
-- 
2.17.1

Re: [PATCH 08/14] xfs: support embedded dfops in transaction

[Brian Foster]

On Thu, Jul 19, 2018 at 09:49:13AM -0400, Brian Foster wrote:
> 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 <bfoster@redhat.com>
> ---

FYI, this accidentally leaves off libxfs/xfs_rtbitmap.c, which also
requires the xfs_defer.h include treatment.

Brian

>  fs/xfs/libxfs/xfs_alloc_btree.c    |  1 +
>  fs/xfs/libxfs/xfs_attr_leaf.c      |  1 +
>  fs/xfs/libxfs/xfs_da_btree.c       |  1 +
>  fs/xfs/libxfs/xfs_defer.c          | 33 ++++++++++++++++++++++++++++++
>  fs/xfs/libxfs/xfs_defer.h          |  1 +
>  fs/xfs/libxfs/xfs_dir2_block.c     |  1 +
>  fs/xfs/libxfs/xfs_dir2_data.c      |  1 +
>  fs/xfs/libxfs/xfs_dir2_leaf.c      |  1 +
>  fs/xfs/libxfs/xfs_dir2_node.c      |  1 +
>  fs/xfs/libxfs/xfs_dir2_sf.c        |  1 +
>  fs/xfs/libxfs/xfs_dquot_buf.c      |  1 +
>  fs/xfs/libxfs/xfs_ialloc_btree.c   |  1 +
>  fs/xfs/libxfs/xfs_inode_fork.c     |  1 +
>  fs/xfs/libxfs/xfs_refcount_btree.c |  1 +
>  fs/xfs/libxfs/xfs_symlink_remote.c |  1 +
>  fs/xfs/libxfs/xfs_trans_resv.c     |  1 +
>  fs/xfs/xfs_aops.c                  |  1 +
>  fs/xfs/xfs_attr_inactive.c         |  2 +-
>  fs/xfs/xfs_attr_list.c             |  1 +
>  fs/xfs/xfs_buf_item.c              |  1 +
>  fs/xfs/xfs_dir2_readdir.c          |  1 +
>  fs/xfs/xfs_dquot_item.c            |  1 +
>  fs/xfs/xfs_export.c                |  1 +
>  fs/xfs/xfs_extent_busy.c           |  1 +
>  fs/xfs/xfs_extfree_item.c          |  1 +
>  fs/xfs/xfs_file.c                  |  1 +
>  fs/xfs/xfs_icache.c                |  1 +
>  fs/xfs/xfs_icreate_item.c          |  1 +
>  fs/xfs/xfs_inode_item.c            |  1 +
>  fs/xfs/xfs_ioctl.c                 |  1 +
>  fs/xfs/xfs_iops.c                  |  2 +-
>  fs/xfs/xfs_log.c                   |  1 +
>  fs/xfs/xfs_log_cil.c               |  1 +
>  fs/xfs/xfs_pnfs.c                  |  1 +
>  fs/xfs/xfs_qm.c                    |  1 +
>  fs/xfs/xfs_qm_bhv.c                |  1 +
>  fs/xfs/xfs_qm_syscalls.c           |  2 +-
>  fs/xfs/xfs_quotaops.c              |  1 +
>  fs/xfs/xfs_super.c                 |  1 +
>  fs/xfs/xfs_trans.c                 | 32 ++++++++++++++++++++++++-----
>  fs/xfs/xfs_trans.h                 |  2 +-
>  fs/xfs/xfs_trans_ail.c             |  1 +
>  fs/xfs/xfs_trans_buf.c             |  1 +
>  fs/xfs/xfs_trans_dquot.c           |  1 +
>  fs/xfs/xfs_trans_inode.c           |  1 +
>  45 files changed, 103 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
> index 4e59cc8a2802..a2ecb6e94cfe 100644
> --- a/fs/xfs/libxfs/xfs_alloc_btree.c
> +++ b/fs/xfs/libxfs/xfs_alloc_btree.c
> @@ -18,6 +18,7 @@
>  #include "xfs_error.h"
>  #include "xfs_trace.h"
>  #include "xfs_cksum.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  
>  
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 251304f3bc5d..56156c985e4d 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -16,6 +16,7 @@
>  #include "xfs_da_format.h"
>  #include "xfs_da_btree.h"
>  #include "xfs_inode.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_inode_item.h"
>  #include "xfs_bmap_btree.h"
> diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> index 9efbd2038ffb..b5c8dbf5c037 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.c
> +++ b/fs/xfs/libxfs/xfs_da_btree.c
> @@ -17,6 +17,7 @@
>  #include "xfs_dir2.h"
>  #include "xfs_dir2_priv.h"
>  #include "xfs_inode.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_inode_item.h"
>  #include "xfs_alloc.h"
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index e6baa27a690b..415fcf720cc9 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -557,3 +557,36 @@ 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)
> +{
> +	int			i;
> +
> +	ASSERT(dst != src);
> +
> +	list_splice_init(&src->dop_intake, &dst->dop_intake);
> +	list_splice_init(&src->dop_pending, &dst->dop_pending);
> +
> +	for (i = 0; i < XFS_DEFER_OPS_NR_INODES; i++) {
> +		if (!src->dop_inodes[i])
> +			break;
> +		dst->dop_inodes[i] = src->dop_inodes[i];
> +	}
> +	for (i = 0; i< XFS_DEFER_OPS_NR_BUFS; i++) {
> +		if (!src->dop_bufs[i])
> +			break;
> +		dst->dop_bufs[i] = src->dop_bufs[i];
> +	}
> +
> +	dst->dop_low = src->dop_low;
> +
> +	xfs_defer_reset(src);
> +}
> diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
> index 8f58f217fdff..349b4d906fb2 100644
> --- a/fs/xfs/libxfs/xfs_defer.h
> +++ b/fs/xfs/libxfs/xfs_defer.h
> @@ -67,6 +67,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 {
> diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
> index 30ed5919da72..76b45a309a23 100644
> --- a/fs/xfs/libxfs/xfs_dir2_block.c
> +++ b/fs/xfs/libxfs/xfs_dir2_block.c
> @@ -13,6 +13,7 @@
>  #include "xfs_da_format.h"
>  #include "xfs_da_btree.h"
>  #include "xfs_inode.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_inode_item.h"
>  #include "xfs_bmap.h"
> diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
> index 01162c62ec8f..d4e2318cf235 100644
> --- a/fs/xfs/libxfs/xfs_dir2_data.c
> +++ b/fs/xfs/libxfs/xfs_dir2_data.c
> @@ -16,6 +16,7 @@
>  #include "xfs_dir2.h"
>  #include "xfs_dir2_priv.h"
>  #include "xfs_error.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_buf_item.h"
>  #include "xfs_cksum.h"
> diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> index 1728a3e6f5cf..c514f116812e 100644
> --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> @@ -18,6 +18,7 @@
>  #include "xfs_dir2_priv.h"
>  #include "xfs_error.h"
>  #include "xfs_trace.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_buf_item.h"
>  #include "xfs_cksum.h"
> diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
> index 2daf874969ab..1a904d2d130e 100644
> --- a/fs/xfs/libxfs/xfs_dir2_node.c
> +++ b/fs/xfs/libxfs/xfs_dir2_node.c
> @@ -18,6 +18,7 @@
>  #include "xfs_dir2_priv.h"
>  #include "xfs_error.h"
>  #include "xfs_trace.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_buf_item.h"
>  #include "xfs_cksum.h"
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index 585dfdb7b6b6..26ae3e0a257f 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -12,6 +12,7 @@
>  #include "xfs_da_format.h"
>  #include "xfs_da_btree.h"
>  #include "xfs_inode.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_inode_item.h"
>  #include "xfs_error.h"
> diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
> index d293f371dd54..1ddd5860fbf8 100644
> --- a/fs/xfs/libxfs/xfs_dquot_buf.c
> +++ b/fs/xfs/libxfs/xfs_dquot_buf.c
> @@ -13,6 +13,7 @@
>  #include "xfs_mount.h"
>  #include "xfs_inode.h"
>  #include "xfs_quota.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_qm.h"
>  #include "xfs_error.h"
> diff --git a/fs/xfs/libxfs/xfs_ialloc_btree.c b/fs/xfs/libxfs/xfs_ialloc_btree.c
> index a5237afec5ab..e91919764cad 100644
> --- a/fs/xfs/libxfs/xfs_ialloc_btree.c
> +++ b/fs/xfs/libxfs/xfs_ialloc_btree.c
> @@ -19,6 +19,7 @@
>  #include "xfs_error.h"
>  #include "xfs_trace.h"
>  #include "xfs_cksum.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_rmap.h"
>  
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 183ec0cb8921..4c1f4a2af811 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -12,6 +12,7 @@
>  #include "xfs_trans_resv.h"
>  #include "xfs_mount.h"
>  #include "xfs_inode.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_inode_item.h"
>  #include "xfs_btree.h"
> diff --git a/fs/xfs/libxfs/xfs_refcount_btree.c b/fs/xfs/libxfs/xfs_refcount_btree.c
> index 26d2300ed865..3e3e7509a518 100644
> --- a/fs/xfs/libxfs/xfs_refcount_btree.c
> +++ b/fs/xfs/libxfs/xfs_refcount_btree.c
> @@ -18,6 +18,7 @@
>  #include "xfs_error.h"
>  #include "xfs_trace.h"
>  #include "xfs_cksum.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_bit.h"
>  #include "xfs_rmap.h"
> diff --git a/fs/xfs/libxfs/xfs_symlink_remote.c b/fs/xfs/libxfs/xfs_symlink_remote.c
> index 95374ab2dee7..f231dd836cd5 100644
> --- a/fs/xfs/libxfs/xfs_symlink_remote.c
> +++ b/fs/xfs/libxfs/xfs_symlink_remote.c
> @@ -17,6 +17,7 @@
>  #include "xfs_trace.h"
>  #include "xfs_symlink.h"
>  #include "xfs_cksum.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_buf_item.h"
>  #include "xfs_log.h"
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index f99a7aefe418..db356294707b 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -17,6 +17,7 @@
>  #include "xfs_bmap_btree.h"
>  #include "xfs_ialloc.h"
>  #include "xfs_quota.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_qm.h"
>  #include "xfs_trans_space.h"
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index f4d3252236c1..9c121168dd9b 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -11,6 +11,7 @@
>  #include "xfs_trans_resv.h"
>  #include "xfs_mount.h"
>  #include "xfs_inode.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_inode_item.h"
>  #include "xfs_alloc.h"
> diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
> index d3055972d3a6..cb0ad9b6678e 100644
> --- a/fs/xfs/xfs_attr_inactive.c
> +++ b/fs/xfs/xfs_attr_inactive.c
> @@ -17,6 +17,7 @@
>  #include "xfs_inode.h"
>  #include "xfs_alloc.h"
>  #include "xfs_attr_remote.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_inode_item.h"
>  #include "xfs_bmap.h"
> @@ -26,7 +27,6 @@
>  #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,
> diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
> index f9ca80154c9c..8c6acc994419 100644
> --- a/fs/xfs/xfs_attr_list.c
> +++ b/fs/xfs/xfs_attr_list.c
> @@ -14,6 +14,7 @@
>  #include "xfs_da_format.h"
>  #include "xfs_da_btree.h"
>  #include "xfs_inode.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_inode_item.h"
>  #include "xfs_bmap.h"
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 1c9d1398980b..335d05b5d1e7 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -11,6 +11,7 @@
>  #include "xfs_bit.h"
>  #include "xfs_sb.h"
>  #include "xfs_mount.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_buf_item.h"
>  #include "xfs_trans_priv.h"
> diff --git a/fs/xfs/xfs_dir2_readdir.c b/fs/xfs/xfs_dir2_readdir.c
> index 5142e64e2345..a4b38c94effe 100644
> --- a/fs/xfs/xfs_dir2_readdir.c
> +++ b/fs/xfs/xfs_dir2_readdir.c
> @@ -19,6 +19,7 @@
>  #include "xfs_error.h"
>  #include "xfs_trace.h"
>  #include "xfs_bmap.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  
>  /*
> diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
> index 7dedd17c4813..8fb101ad8b91 100644
> --- a/fs/xfs/xfs_dquot_item.c
> +++ b/fs/xfs/xfs_dquot_item.c
> @@ -12,6 +12,7 @@
>  #include "xfs_inode.h"
>  #include "xfs_quota.h"
>  #include "xfs_error.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_buf_item.h"
>  #include "xfs_trans_priv.h"
> diff --git a/fs/xfs/xfs_export.c b/fs/xfs/xfs_export.c
> index 3cf4682e2510..ef5b7959b18b 100644
> --- a/fs/xfs/xfs_export.c
> +++ b/fs/xfs/xfs_export.c
> @@ -13,6 +13,7 @@
>  #include "xfs_dir2.h"
>  #include "xfs_export.h"
>  #include "xfs_inode.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_inode_item.h"
>  #include "xfs_trace.h"
> diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
> index 0ed68379e551..3558a832f483 100644
> --- a/fs/xfs/xfs_extent_busy.c
> +++ b/fs/xfs/xfs_extent_busy.c
> @@ -16,6 +16,7 @@
>  #include "xfs_alloc.h"
>  #include "xfs_extent_busy.h"
>  #include "xfs_trace.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_log.h"
>  
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index d9da66c718bb..55cc4f4fae36 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -10,6 +10,7 @@
>  #include "xfs_trans_resv.h"
>  #include "xfs_bit.h"
>  #include "xfs_mount.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_trans_priv.h"
>  #include "xfs_buf_item.h"
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 6b31f41eafa2..8e06b19ce8c0 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -13,6 +13,7 @@
>  #include "xfs_da_format.h"
>  #include "xfs_da_btree.h"
>  #include "xfs_inode.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_inode_item.h"
>  #include "xfs_bmap.h"
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 47f417d20a30..873113913d4a 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -12,6 +12,7 @@
>  #include "xfs_mount.h"
>  #include "xfs_inode.h"
>  #include "xfs_error.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_trans_priv.h"
>  #include "xfs_inode_item.h"
> diff --git a/fs/xfs/xfs_icreate_item.c b/fs/xfs/xfs_icreate_item.c
> index 8381d34cb102..04f86ede18c7 100644
> --- a/fs/xfs/xfs_icreate_item.c
> +++ b/fs/xfs/xfs_icreate_item.c
> @@ -11,6 +11,7 @@
>  #include "xfs_trans_resv.h"
>  #include "xfs_bit.h"
>  #include "xfs_mount.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_trans_priv.h"
>  #include "xfs_error.h"
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 2389c34c172d..423c2183a383 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -10,6 +10,7 @@
>  #include "xfs_trans_resv.h"
>  #include "xfs_mount.h"
>  #include "xfs_inode.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_inode_item.h"
>  #include "xfs_error.h"
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 0ef5ece5634c..d816eaec685d 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -26,6 +26,7 @@
>  #include "xfs_trace.h"
>  #include "xfs_icache.h"
>  #include "xfs_symlink.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_acl.h"
>  #include "xfs_btree.h"
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 704b57a8b99e..32bbd5001b9f 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -18,6 +18,7 @@
>  #include "xfs_quota.h"
>  #include "xfs_error.h"
>  #include "xfs_attr.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_trace.h"
>  #include "xfs_icache.h"
> @@ -26,7 +27,6 @@
>  #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>
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 5e56f3b93d4b..b6a89e4ff847 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -12,6 +12,7 @@
>  #include "xfs_mount.h"
>  #include "xfs_errortag.h"
>  #include "xfs_error.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_trans_priv.h"
>  #include "xfs_log.h"
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index d3884e08b43c..92050c99c430 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -14,6 +14,7 @@
>  #include "xfs_alloc.h"
>  #include "xfs_extent_busy.h"
>  #include "xfs_discard.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_trans_priv.h"
>  #include "xfs_log.h"
> diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
> index f44c3599527d..f3fe6ed2da96 100644
> --- a/fs/xfs/xfs_pnfs.c
> +++ b/fs/xfs/xfs_pnfs.c
> @@ -10,6 +10,7 @@
>  #include "xfs_sb.h"
>  #include "xfs_mount.h"
>  #include "xfs_inode.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_log.h"
>  #include "xfs_bmap.h"
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 9ceb85cce33a..166ca6054179 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -20,6 +20,7 @@
>  #include "xfs_bmap.h"
>  #include "xfs_bmap_btree.h"
>  #include "xfs_bmap_util.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_trans_space.h"
>  #include "xfs_qm.h"
> diff --git a/fs/xfs/xfs_qm_bhv.c b/fs/xfs/xfs_qm_bhv.c
> index 73a1d77ec187..1ebcb9e9418f 100644
> --- a/fs/xfs/xfs_qm_bhv.c
> +++ b/fs/xfs/xfs_qm_bhv.c
> @@ -12,6 +12,7 @@
>  #include "xfs_mount.h"
>  #include "xfs_inode.h"
>  #include "xfs_error.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_qm.h"
>  
> diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> index df0783303887..94ca3bff8d07 100644
> --- a/fs/xfs/xfs_qm_syscalls.c
> +++ b/fs/xfs/xfs_qm_syscalls.c
> @@ -16,13 +16,13 @@
>  #include "xfs_sb.h"
>  #include "xfs_mount.h"
>  #include "xfs_inode.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_error.h"
>  #include "xfs_quota.h"
>  #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 *,
> diff --git a/fs/xfs/xfs_quotaops.c b/fs/xfs/xfs_quotaops.c
> index 205fbb2a77e4..a698b964d13a 100644
> --- a/fs/xfs/xfs_quotaops.c
> +++ b/fs/xfs/xfs_quotaops.c
> @@ -10,6 +10,7 @@
>  #include "xfs_mount.h"
>  #include "xfs_inode.h"
>  #include "xfs_quota.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_trace.h"
>  #include "xfs_icache.h"
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index f9f8dc490d3d..25a6d227d1fe 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -18,6 +18,7 @@
>  #include "xfs_alloc.h"
>  #include "xfs_error.h"
>  #include "xfs_fsops.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_buf_item.h"
>  #include "xfs_log.h"
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index de00f79ff698..142410632a36 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -14,12 +14,12 @@
>  #include "xfs_inode.h"
>  #include "xfs_extent_busy.h"
>  #include "xfs_quota.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_trans_priv.h"
>  #include "xfs_log.h"
>  #include "xfs_trace.h"
>  #include "xfs_error.h"
> -#include "xfs_defer.h"
>  
>  kmem_zone_t	*xfs_trans_zone;
>  
> @@ -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
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 6f857af61455..bab857c36d3a 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -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;
>  
> @@ -130,6 +129,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;
>  
>  /*
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 55326f971cb3..91aa25a753fc 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -10,6 +10,7 @@
>  #include "xfs_log_format.h"
>  #include "xfs_trans_resv.h"
>  #include "xfs_mount.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_trans_priv.h"
>  #include "xfs_trace.h"
> diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> index 15919f67a88f..165044d7b6c3 100644
> --- a/fs/xfs/xfs_trans_buf.c
> +++ b/fs/xfs/xfs_trans_buf.c
> @@ -11,6 +11,7 @@
>  #include "xfs_trans_resv.h"
>  #include "xfs_mount.h"
>  #include "xfs_inode.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_buf_item.h"
>  #include "xfs_trans_priv.h"
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index c23257a26c2b..d737a5c00148 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -12,6 +12,7 @@
>  #include "xfs_mount.h"
>  #include "xfs_inode.h"
>  #include "xfs_error.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_trans_priv.h"
>  #include "xfs_quota.h"
> diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
> index 542927321a61..7848911f5e98 100644
> --- a/fs/xfs/xfs_trans_inode.c
> +++ b/fs/xfs/xfs_trans_inode.c
> @@ -11,6 +11,7 @@
>  #include "xfs_trans_resv.h"
>  #include "xfs_mount.h"
>  #include "xfs_inode.h"
> +#include "xfs_defer.h"
>  #include "xfs_trans.h"
>  #include "xfs_trans_priv.h"
>  #include "xfs_inode_item.h"
> -- 
> 2.17.1
> 
> --
> 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

Re: [PATCH 08/14] xfs: support embedded dfops in transaction

[Christoph Hellwig]

On Thu, Jul 19, 2018 at 12:18:26PM -0400, Brian Foster wrote:
> FYI, this accidentally leaves off libxfs/xfs_rtbitmap.c, which also
> requires the xfs_defer.h include treatment.

I wonder if we shouldn't just move struct xfs_defer_ops to xfs_trans.h
instead.

Re: [PATCH 01/14] xfs: pull up dfops from xfs_itruncate_extents()

[Christoph Hellwig]

On Thu, Jul 19, 2018 at 09:49:06AM -0400, Brian Foster wrote:
> 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.

Yes, this looks pretty sensible:

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

Re: [PATCH 02/14] xfs: use ->t_dfops in log recovery intent processing

[Christoph Hellwig]

On Thu, Jul 19, 2018 at 09:49:07AM -0400, Brian Foster wrote:
> 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.

Looks good,

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

Re: [PATCH 03/14] xfs: fix transaction leak on remote attr set/remove failure

[Christoph Hellwig]

On Thu, Jul 19, 2018 at 09:49:08AM -0400, Brian Foster wrote:
> 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.

Looks good,

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

Re: [PATCH 04/14] xfs: make deferred processing safe for embedded dfops

[Christoph Hellwig]

Yes, this looks pretty sensible:

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

Re: [PATCH 05/14] xfs: remove unused deferred ops committed field

[Christoph Hellwig]

On Thu, Jul 19, 2018 at 09:49:10AM -0400, Brian Foster wrote:
> 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 <bfoster@redhat.com>

Looks good,

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

Re: [PATCH 06/14] xfs: reset dfops to initial state after finish

[Christoph Hellwig]

On Thu, Jul 19, 2018 at 09:49:11AM -0400, Brian Foster wrote:
> 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 <bfoster@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_defer.c | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index d3466087db16..e6baa27a690b 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -319,6 +319,29 @@ xfs_defer_bjoin(
>  	return -EFSCORRUPTED;
>  }
>  
> +/*
> + * Reset an already used dfops after finish.
> + */
> +static void
> +xfs_defer_reset(
> +	struct xfs_defer_ops	*dop)
> +{
> +	int 			i;
> +
> +	ASSERT(!xfs_defer_has_unfinished_work(dop));
> +	dop->dop_low = false;
> +	for (i = 0; i < XFS_DEFER_OPS_NR_INODES; i++) {
> +		if (!dop->dop_inodes[i])
> +			break;
> +		dop->dop_inodes[i] = NULL;
> +	}
> +	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS; i++) {
> +		if (!dop->dop_bufs[i])
> +			break;
> +		dop->dop_bufs[i] = NULL;
> +	}
> +}

Are these loops really more efficient than just doing memsets
of dop_inodes and dop_bufs?

Otherwise looks good:

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

Re: [PATCH 07/14] xfs: pack holes in xfs_defer_ops and xfs_trans

[Christoph Hellwig]

On Thu, Jul 19, 2018 at 09:49:12AM -0400, Brian Foster wrote:
> 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 <bfoster@redhat.com>

Looks good,

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

Re: [PATCH 08/14] xfs: support embedded dfops in transaction

[Christoph Hellwig]

> +/*
> + * 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)
> +{
> +	int			i;
> +
> +	ASSERT(dst != src);
> +
> +	list_splice_init(&src->dop_intake, &dst->dop_intake);
> +	list_splice_init(&src->dop_pending, &dst->dop_pending);
> +
> +	for (i = 0; i < XFS_DEFER_OPS_NR_INODES; i++) {
> +		if (!src->dop_inodes[i])
> +			break;
> +		dst->dop_inodes[i] = src->dop_inodes[i];
> +	}
> +	for (i = 0; i< XFS_DEFER_OPS_NR_BUFS; i++) {
> +		if (!src->dop_bufs[i])
> +			break;
> +		dst->dop_bufs[i] = src->dop_bufs[i];
> +	}

I suspect simply memcpy()ing the state over would be faster..

Re: [PATCH 09/14] xfs: use internal dfops in cow blocks cancel

[Christoph Hellwig]

> +			ASSERT((*tpp)->t_dfops &&
> +			       (*tpp)->t_firstblock == NULLFSBLOCK);

I'd make this two separate asserts so that we can see which one did
caused it if it triggers.

Re: [PATCH 00/14] xfs: embed dfops in the transaction

[Christoph Hellwig]

On Thu, Jul 19, 2018 at 09:49:05AM -0400, Brian Foster wrote:
> return a clean transaction. Other things to consider might be to do away
> with support for external dfops and the ->t_dfops pointer indirection,
> or perhaps even consider going the other direction: allocate dfops from
> a separate zone to save some memory on non-permanent transactions (note
> that 16 of 28 transactions use a permanent log res. last I looked, so it
> may not be worth it atm).

The defer_ops aren't really that big, and allocations are relatively
costly, so I don't think a separate allocation is a good idea.  If we
really want to optimize the non-permanent transaction case we could do
something like:

struct xfs_trans {
	...
	struct xfs_defer_ops dfops[];
};

and then have two caches for the with an without dfops case.  But
I can't believe that would be worth it, especially in face of...


> I know Christoph also had thoughts around condensing some of the items
> joined to the dfops to those with the transaction.

... this.

> I have yet to think
> about that one, but I do have an RFC quality patch laying around that
> replaces the ->dop_low flag with a transaction flag (->t_flags),
> eliminating the need for that extra byte in xfs_defer_ops. The one quirk
> associated with that is the question of whether we want to preserve the
> behavior where low mode remains active across the series of transactions
> associated with the traditional (on-stack) dfops or is reset on
> transaction roll (a la firstblock). I'll post that RFC separately for a
> more proper discussion..

That sounds like a good enough start.  For now I'd keep the existing
behavior because it really is deep magic and needs a deep audit.  I
had started on that a long time ago but dropped the ball, but mixing
it with this work is probably not helpful.

Re: [PATCH 08/14] xfs: support embedded dfops in transaction

[Brian Foster]

On Thu, Jul 19, 2018 at 12:47:59PM -0700, Christoph Hellwig wrote:
> On Thu, Jul 19, 2018 at 12:18:26PM -0400, Brian Foster wrote:
> > FYI, this accidentally leaves off libxfs/xfs_rtbitmap.c, which also
> > requires the xfs_defer.h include treatment.
> 
> I wonder if we shouldn't just move struct xfs_defer_ops to xfs_trans.h
> instead.

I thought about doing that, was curious how many files I'd have to
modify and ended up just plowing through them all. :P I'm happy to move
the struct def over if nobody objects.

Brian

Re: [PATCH 08/14] xfs: support embedded dfops in transaction

[Brian Foster]

On Thu, Jul 19, 2018 at 12:56:53PM -0700, Christoph Hellwig wrote:
> > +/*
> > + * 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)
> > +{
> > +	int			i;
> > +
> > +	ASSERT(dst != src);
> > +
> > +	list_splice_init(&src->dop_intake, &dst->dop_intake);
> > +	list_splice_init(&src->dop_pending, &dst->dop_pending);
> > +
> > +	for (i = 0; i < XFS_DEFER_OPS_NR_INODES; i++) {
> > +		if (!src->dop_inodes[i])
> > +			break;
> > +		dst->dop_inodes[i] = src->dop_inodes[i];
> > +	}
> > +	for (i = 0; i< XFS_DEFER_OPS_NR_BUFS; i++) {
> > +		if (!src->dop_bufs[i])
> > +			break;
> > +		dst->dop_bufs[i] = src->dop_bufs[i];
> > +	}
> 
> I suspect simply memcpy()ing the state over would be faster..

Hm, I figured open-coding it would be faster than function calls since
there are only 2 pointers, but tbh I'm not sure either way and don't
have a strong preference. That said, I've been thinking a bit about how
to get rid of dop_bufs/inodes entirely (see my reply to the cover letter
thread). If that looks promising then it's probably not worth changing
this over since the code will go away (but otherwise I'll change it to
memcpy() if that's preferred)..

Brian

Re: [PATCH 06/14] xfs: reset dfops to initial state after finish

[Brian Foster]

On Thu, Jul 19, 2018 at 12:54:41PM -0700, Christoph Hellwig wrote:
> On Thu, Jul 19, 2018 at 09:49:11AM -0400, Brian Foster wrote:
> > 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 <bfoster@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_defer.c | 30 ++++++++++++++++++++++++++++--
> >  1 file changed, 28 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> > index d3466087db16..e6baa27a690b 100644
> > --- a/fs/xfs/libxfs/xfs_defer.c
> > +++ b/fs/xfs/libxfs/xfs_defer.c
> > @@ -319,6 +319,29 @@ xfs_defer_bjoin(
> >  	return -EFSCORRUPTED;
> >  }
> >  
> > +/*
> > + * Reset an already used dfops after finish.
> > + */
> > +static void
> > +xfs_defer_reset(
> > +	struct xfs_defer_ops	*dop)
> > +{
> > +	int 			i;
> > +
> > +	ASSERT(!xfs_defer_has_unfinished_work(dop));
> > +	dop->dop_low = false;
> > +	for (i = 0; i < XFS_DEFER_OPS_NR_INODES; i++) {
> > +		if (!dop->dop_inodes[i])
> > +			break;
> > +		dop->dop_inodes[i] = NULL;
> > +	}
> > +	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS; i++) {
> > +		if (!dop->dop_bufs[i])
> > +			break;
> > +		dop->dop_bufs[i] = NULL;
> > +	}
> > +}
> 
> Are these loops really more efficient than just doing memsets
> of dop_inodes and dop_bufs?
> 

Not sure, I suppose we could just check dop_bufs[0] and memset() if !=
NULL..? I can do that as well if we don't otherwise lose these pointers.

Brian

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

Re: [PATCH 09/14] xfs: use internal dfops in cow blocks cancel

[Brian Foster]

On Thu, Jul 19, 2018 at 12:57:48PM -0700, Christoph Hellwig wrote:
> > +			ASSERT((*tpp)->t_dfops &&
> > +			       (*tpp)->t_firstblock == NULLFSBLOCK);
> 
> I'd make this two separate asserts so that we can see which one did
> caused it if it triggers.

Ok.

Brian

Re: [PATCH 00/14] xfs: embed dfops in the transaction

[Brian Foster]

On Thu, Jul 19, 2018 at 01:05:57PM -0700, Christoph Hellwig wrote:
> On Thu, Jul 19, 2018 at 09:49:05AM -0400, Brian Foster wrote:
> > return a clean transaction. Other things to consider might be to do away
> > with support for external dfops and the ->t_dfops pointer indirection,
> > or perhaps even consider going the other direction: allocate dfops from
> > a separate zone to save some memory on non-permanent transactions (note
> > that 16 of 28 transactions use a permanent log res. last I looked, so it
> > may not be worth it atm).
> 
> The defer_ops aren't really that big, and allocations are relatively
> costly, so I don't think a separate allocation is a good idea.  If we
> really want to optimize the non-permanent transaction case we could do
> something like:
> 
> struct xfs_trans {
> 	...
> 	struct xfs_defer_ops dfops[];
> };
> 
> and then have two caches for the with an without dfops case.  But
> I can't believe that would be worth it, especially in face of...
> 
> 
> > I know Christoph also had thoughts around condensing some of the items
> > joined to the dfops to those with the transaction.
> 
> ... this.
> 

Yeah. I was actually poking around today after writing this up and
thought that we might be able to replace both dop_inodes/dop_bufs with
checks in the transaction item list for either held buffers or inode
items where lock_flags == 0. I _think_ both of those states may be
essentially equivalent to joined dfops items, but I have to verify that.
If so, we can probably make the dfops inode/buf relogging "automatic,"
drop both pointer lists and the whole memory thing becomes kind of moot.

> > I have yet to think
> > about that one, but I do have an RFC quality patch laying around that
> > replaces the ->dop_low flag with a transaction flag (->t_flags),
> > eliminating the need for that extra byte in xfs_defer_ops. The one quirk
> > associated with that is the question of whether we want to preserve the
> > behavior where low mode remains active across the series of transactions
> > associated with the traditional (on-stack) dfops or is reset on
> > transaction roll (a la firstblock). I'll post that RFC separately for a
> > more proper discussion..
> 
> That sounds like a good enough start.  For now I'd keep the existing
> behavior because it really is deep magic and needs a deep audit.  I
> had started on that a long time ago but dropped the ball, but mixing
> it with this work is probably not helpful.

That sounds reasonable to me. We can always change behavior in a
subsequent patch. IIRC the only issue is that intent recovery code has
no way to carry dop_low mode around without a transaction. It currently
passes around a dfops for each intent. Hmm, perhaps we can have the
caller just allocate a transaction, pass it to the recovery helpers for
reservation and then just keep rolling it rather than have each helper
allocate a transaction anew. I'll look into it, or let me know if you
have any other thoughts/ideas.

Brian

Re: [PATCH 00/14] xfs: embed dfops in the transaction

[Darrick J. Wong]

On Thu, Jul 19, 2018 at 04:36:43PM -0400, Brian Foster wrote:
> On Thu, Jul 19, 2018 at 01:05:57PM -0700, Christoph Hellwig wrote:
> > On Thu, Jul 19, 2018 at 09:49:05AM -0400, Brian Foster wrote:
> > > return a clean transaction. Other things to consider might be to do away
> > > with support for external dfops and the ->t_dfops pointer indirection,
> > > or perhaps even consider going the other direction: allocate dfops from
> > > a separate zone to save some memory on non-permanent transactions (note
> > > that 16 of 28 transactions use a permanent log res. last I looked, so it
> > > may not be worth it atm).
> > 
> > The defer_ops aren't really that big, and allocations are relatively
> > costly, so I don't think a separate allocation is a good idea.  If we
> > really want to optimize the non-permanent transaction case we could do
> > something like:
> > 
> > struct xfs_trans {
> > 	...
> > 	struct xfs_defer_ops dfops[];
> > };
> > 
> > and then have two caches for the with an without dfops case.  But
> > I can't believe that would be worth it, especially in face of...
> > 
> > 
> > > I know Christoph also had thoughts around condensing some of the items
> > > joined to the dfops to those with the transaction.
> > 
> > ... this.
> > 
> 
> Yeah. I was actually poking around today after writing this up and
> thought that we might be able to replace both dop_inodes/dop_bufs with
> checks in the transaction item list for either held buffers or inode
> items where lock_flags == 0. I _think_ both of those states may be
> essentially equivalent to joined dfops items, but I have to verify that.
> If so, we can probably make the dfops inode/buf relogging "automatic,"
> drop both pointer lists and the whole memory thing becomes kind of moot.

<nod>

> > > I have yet to think
> > > about that one, but I do have an RFC quality patch laying around that
> > > replaces the ->dop_low flag with a transaction flag (->t_flags),
> > > eliminating the need for that extra byte in xfs_defer_ops. The one quirk
> > > associated with that is the question of whether we want to preserve the
> > > behavior where low mode remains active across the series of transactions
> > > associated with the traditional (on-stack) dfops or is reset on
> > > transaction roll (a la firstblock). I'll post that RFC separately for a
> > > more proper discussion..
> > 
> > That sounds like a good enough start.  For now I'd keep the existing
> > behavior because it really is deep magic and needs a deep audit.  I
> > had started on that a long time ago but dropped the ball, but mixing
> > it with this work is probably not helpful.
> 
> That sounds reasonable to me. We can always change behavior in a
> subsequent patch. IIRC the only issue is that intent recovery code has
> no way to carry dop_low mode around without a transaction. It currently
> passes around a dfops for each intent. Hmm, perhaps we can have the
> caller just allocate a transaction, pass it to the recovery helpers for
> reservation and then just keep rolling it rather than have each helper
> allocate a transaction anew. I'll look into it, or let me know if you
> have any other thoughts/ideas.

That could get tricky, since each log intent item type allocates its own
transaction with some context-dependent reservation and resblks. Rolling
our way through the intent items would require us to calculate the max
reservation size and resblks for all the items beforehand for the
initial _trans_alloc, which would be kinda messy to avoid having a flags
field.

--D

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

Re: [PATCH 00/14] xfs: embed dfops in the transaction

[Brian Foster]

On Thu, Jul 19, 2018 at 02:36:34PM -0700, Darrick J. Wong wrote:
> On Thu, Jul 19, 2018 at 04:36:43PM -0400, Brian Foster wrote:
> > On Thu, Jul 19, 2018 at 01:05:57PM -0700, Christoph Hellwig wrote:
> > > On Thu, Jul 19, 2018 at 09:49:05AM -0400, Brian Foster wrote:
> > > > return a clean transaction. Other things to consider might be to do away
> > > > with support for external dfops and the ->t_dfops pointer indirection,
> > > > or perhaps even consider going the other direction: allocate dfops from
> > > > a separate zone to save some memory on non-permanent transactions (note
> > > > that 16 of 28 transactions use a permanent log res. last I looked, so it
> > > > may not be worth it atm).
> > > 
> > > The defer_ops aren't really that big, and allocations are relatively
> > > costly, so I don't think a separate allocation is a good idea.  If we
> > > really want to optimize the non-permanent transaction case we could do
> > > something like:
> > > 
> > > struct xfs_trans {
> > > 	...
> > > 	struct xfs_defer_ops dfops[];
> > > };
> > > 
> > > and then have two caches for the with an without dfops case.  But
> > > I can't believe that would be worth it, especially in face of...
> > > 
> > > 
> > > > I know Christoph also had thoughts around condensing some of the items
> > > > joined to the dfops to those with the transaction.
> > > 
> > > ... this.
> > > 
> > 
> > Yeah. I was actually poking around today after writing this up and
> > thought that we might be able to replace both dop_inodes/dop_bufs with
> > checks in the transaction item list for either held buffers or inode
> > items where lock_flags == 0. I _think_ both of those states may be
> > essentially equivalent to joined dfops items, but I have to verify that.
> > If so, we can probably make the dfops inode/buf relogging "automatic,"
> > drop both pointer lists and the whole memory thing becomes kind of moot.
> 
> <nod>
> 
> > > > I have yet to think
> > > > about that one, but I do have an RFC quality patch laying around that
> > > > replaces the ->dop_low flag with a transaction flag (->t_flags),
> > > > eliminating the need for that extra byte in xfs_defer_ops. The one quirk
> > > > associated with that is the question of whether we want to preserve the
> > > > behavior where low mode remains active across the series of transactions
> > > > associated with the traditional (on-stack) dfops or is reset on
> > > > transaction roll (a la firstblock). I'll post that RFC separately for a
> > > > more proper discussion..
> > > 
> > > That sounds like a good enough start.  For now I'd keep the existing
> > > behavior because it really is deep magic and needs a deep audit.  I
> > > had started on that a long time ago but dropped the ball, but mixing
> > > it with this work is probably not helpful.
> > 
> > That sounds reasonable to me. We can always change behavior in a
> > subsequent patch. IIRC the only issue is that intent recovery code has
> > no way to carry dop_low mode around without a transaction. It currently
> > passes around a dfops for each intent. Hmm, perhaps we can have the
> > caller just allocate a transaction, pass it to the recovery helpers for
> > reservation and then just keep rolling it rather than have each helper
> > allocate a transaction anew. I'll look into it, or let me know if you
> > have any other thoughts/ideas.
> 
> That could get tricky, since each log intent item type allocates its own
> transaction with some context-dependent reservation and resblks. Rolling
> our way through the intent items would require us to calculate the max
> reservation size and resblks for all the items beforehand for the
> initial _trans_alloc, which would be kinda messy to avoid having a flags
> field.
> 

Yeah, I was more thinking of creating a zero reservation transaction in
the caller and (re)exporting xfs_trans_reserve() so each helper could
reserve commit/roll.

Taking a look at the code, I wonder if the simplest thing is to allocate
an empty transaction in the caller (so nesting shouldn't be an issue, I
think, and since we already have that mechanism for scrub) instead of a
raw dfops, pass that new tp around and then open-code the transfer of
low mode back and forth the same way dfops are moved back and forth
until intent recovery completion. In fact, we could probably create a
little xlog_recover_tp_move() wrapper to do both and (more importantly)
document the hackery in one place until we determine whether to kill the
multi transaction low mode behavior. A bit ugly, but probably simpler
than trying to bake this one-off corner case (that can hopefully be
killed off) into the core transaction infrastructure via trans dups and
reservation games. Thoughts?

Brian

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

Re: [PATCH 00/14] xfs: embed dfops in the transaction

[Brian Foster]

On Thu, Jul 19, 2018 at 02:36:34PM -0700, Darrick J. Wong wrote:
> On Thu, Jul 19, 2018 at 04:36:43PM -0400, Brian Foster wrote:
> > On Thu, Jul 19, 2018 at 01:05:57PM -0700, Christoph Hellwig wrote:
> > > On Thu, Jul 19, 2018 at 09:49:05AM -0400, Brian Foster wrote:
> > > > return a clean transaction. Other things to consider might be to do away
> > > > with support for external dfops and the ->t_dfops pointer indirection,
> > > > or perhaps even consider going the other direction: allocate dfops from
> > > > a separate zone to save some memory on non-permanent transactions (note
> > > > that 16 of 28 transactions use a permanent log res. last I looked, so it
> > > > may not be worth it atm).
> > > 
> > > The defer_ops aren't really that big, and allocations are relatively
> > > costly, so I don't think a separate allocation is a good idea.  If we
> > > really want to optimize the non-permanent transaction case we could do
> > > something like:
> > > 
> > > struct xfs_trans {
> > > 	...
> > > 	struct xfs_defer_ops dfops[];
> > > };
> > > 
> > > and then have two caches for the with an without dfops case.  But
> > > I can't believe that would be worth it, especially in face of...
> > > 
> > > 
> > > > I know Christoph also had thoughts around condensing some of the items
> > > > joined to the dfops to those with the transaction.
> > > 
> > > ... this.
> > > 
> > 
> > Yeah. I was actually poking around today after writing this up and
> > thought that we might be able to replace both dop_inodes/dop_bufs with
> > checks in the transaction item list for either held buffers or inode
> > items where lock_flags == 0. I _think_ both of those states may be
> > essentially equivalent to joined dfops items, but I have to verify that.
> > If so, we can probably make the dfops inode/buf relogging "automatic,"
> > drop both pointer lists and the whole memory thing becomes kind of moot.
> 
> <nod>
> 

Ok.. on this one, running some tests shows that pretty much all dfops
joined bufs/inodes are essentially held in the transaction (which would
probably be a bug if that were not the case). The opposite is also true
for buffers, because a held buffers must also be held in subsequent
transactions to ensure the caller still has a reference after dfops
completion. OTOH, there are some places where inodes are joined to a
transaction with lock_flags == 0 and dfops are run without the inode
joined to the dfops. I don't necessarily think this is a bug for the
inode case because the caller still owns the inode lock, we just don't
relog it across the series of transactions required for dfops
completion.

I don't think it would necessarily be a problem if we did relog the
inode in these cases, however. In fact I think the argument has been
made in the past to do that explicitly (with xfs_defer_[i|b]join()) in
one or two cases. So given that, the overall approach still seems
reasonable enough to me. FWIW, the appended patch shows the cases I've
caught on a quick xfstests run that would be affected (i.e., where we
don't currently defer_ijoin() but implicitly would with an "automatic"
relog mechanism). I'm running a longer test to try and catch any others,
but let me know if anybody sees problems with this or has other ideas...

Brian

--- 8< ---

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 60138514ea86..c3cbe05cdb1b 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1120,6 +1120,7 @@ xfs_bmap_add_attrfork(
 			xfs_log_sb(tp);
 	}
 
+	xfs_defer_ijoin(tp->t_dfops, ip);
 	error = xfs_trans_commit(tp);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	return error;
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 7b40e9fef381..e63712e67fd8 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -979,6 +979,7 @@ xfs_alloc_file_space(
 		/*
 		 * Complete the transaction
 		 */
+		xfs_defer_ijoin(tp->t_dfops, ip);
 		error = xfs_trans_commit(tp);
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);
 		if (error)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 0e4bd559a6a7..24fdca90b588 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1810,6 +1810,7 @@ xfs_inactive_ifree(
 	 * Just ignore errors at this point.  There is nothing we can do except
 	 * to try to keep going. Make sure it's not a silent error.
 	 */
+	xfs_defer_ijoin(tp->t_dfops, ip);
 	error = xfs_trans_commit(tp);
 	if (error)
 		xfs_notice(mp, "%s: xfs_trans_commit returned error %d",
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 8e8ca9f03f0e..464b4b3225bb 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -261,6 +261,7 @@ xfs_iomap_write_direct(
 	/*
 	 * Complete the transaction
 	 */
+	xfs_defer_ijoin(tp->t_dfops, ip);
 	error = xfs_trans_commit(tp);
 	if (error)
 		goto out_unlock;
@@ -762,6 +763,7 @@ xfs_iomap_write_allocate(
 			if (error)
 				goto trans_cancel;
 
+			xfs_defer_ijoin(tp->t_dfops, ip);
 			error = xfs_trans_commit(tp);
 			if (error)
 				goto error0;
@@ -879,6 +881,7 @@ xfs_iomap_write_unwritten(
 			xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 		}
 
+		xfs_defer_ijoin(tp->t_dfops, ip);
 		error = xfs_trans_commit(tp);
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);
 		if (error)
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index cddde219630a..8984f283da7b 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -435,6 +435,7 @@ xfs_reflink_allocate_cow(
 	xfs_inode_set_cowblocks_tag(ip);
 
 	/* Finish up. */
+	xfs_defer_ijoin(tp->t_dfops, ip);
 	error = xfs_trans_commit(tp);
 	if (error)
 		return error;

Re: [PATCH 08/14] xfs: support embedded dfops in transaction

[Christoph Hellwig]

On Thu, Jul 19, 2018 at 04:31:20PM -0400, Brian Foster wrote:
> On Thu, Jul 19, 2018 at 12:47:59PM -0700, Christoph Hellwig wrote:
> > On Thu, Jul 19, 2018 at 12:18:26PM -0400, Brian Foster wrote:
> > > FYI, this accidentally leaves off libxfs/xfs_rtbitmap.c, which also
> > > requires the xfs_defer.h include treatment.
> > 
> > I wonder if we shouldn't just move struct xfs_defer_ops to xfs_trans.h
> > instead.
> 
> I thought about doing that, was curious how many files I'd have to
> modify and ended up just plowing through them all. :P I'm happy to move
> the struct def over if nobody objects.

To me moving it feels like the right thing.  In the end xfs_trans will be
the prime user of xfs_defer_ops, assuming we even keep it as a separate
structure at all.

Re: [PATCH 08/14] xfs: support embedded dfops in transaction

[Christoph Hellwig]

On Thu, Jul 19, 2018 at 04:32:49PM -0400, Brian Foster wrote:
> Hm, I figured open-coding it would be faster than function calls since
> there are only 2 pointers, but tbh I'm not sure either way and don't
> have a strong preference.

We should not even generate a function call for a small memset thanks
to gcc builtins.

Re: [PATCH 06/14] xfs: reset dfops to initial state after finish

[Christoph Hellwig]

On Thu, Jul 19, 2018 at 04:33:38PM -0400, Brian Foster wrote:
> Not sure, I suppose we could just check dop_bufs[0] and memset() if !=
> NULL..? I can do that as well if we don't otherwise lose these pointers.

I don't understand why we even want a conditional here.  In general
copying one or two additional pointers should always be cheaper than
having a conditional.

Re: [PATCH 00/14] xfs: embed dfops in the transaction

[Christoph Hellwig]

On Thu, Jul 19, 2018 at 02:36:34PM -0700, Darrick J. Wong wrote:
> > That sounds reasonable to me. We can always change behavior in a
> > subsequent patch. IIRC the only issue is that intent recovery code has
> > no way to carry dop_low mode around without a transaction. It currently
> > passes around a dfops for each intent. Hmm, perhaps we can have the
> > caller just allocate a transaction, pass it to the recovery helpers for
> > reservation and then just keep rolling it rather than have each helper
> > allocate a transaction anew. I'll look into it, or let me know if you
> > have any other thoughts/ideas.
> 
> That could get tricky, since each log intent item type allocates its own
> transaction with some context-dependent reservation and resblks. Rolling
> our way through the intent items would require us to calculate the max
> reservation size and resblks for all the items beforehand for the
> initial _trans_alloc, which would be kinda messy to avoid having a flags
> field.

For now we can just keep that state in a separate variable, e.g.:

	bool low_space_mode = (tp->t_flags & XFS_TRANS_LOW_SPACE);

	...

	if (low_space_mode)
		other_tp->t_flags |= XFS_TRANS_LOW_SPACE;

Not too pretty but I wouldn't spend to much time on this low space
mode, which fundamentally is a horribly band aid and should eventually
go away once we've fixed up our space reservations for real.

Re: [PATCH 00/14] xfs: embed dfops in the transaction

[Christoph Hellwig]

On Fri, Jul 20, 2018 at 10:41:07AM -0400, Brian Foster wrote:
> Ok.. on this one, running some tests shows that pretty much all dfops
> joined bufs/inodes are essentially held in the transaction (which would
> probably be a bug if that were not the case).

That's what I'd expect.

> The opposite is also true
> for buffers, because a held buffers must also be held in subsequent
> transactions to ensure the caller still has a reference after dfops
> completion. OTOH, there are some places where inodes are joined to a
> transaction with lock_flags == 0 and dfops are run without the inode
> joined to the dfops. I don't necessarily think this is a bug for the
> inode case because the caller still owns the inode lock, we just don't
> relog it across the series of transactions required for dfops
> completion.

I suspect many of those are a bug.  Yes, we could have deferred ops
that don't need the inode, but in general we'd want the same objects
for the deferred inode.

That being said we could always have an indicator for that case, but
I'd rather make it opt-in than opt-out.

Re: [PATCH 08/14] xfs: support embedded dfops in transaction

[Darrick J. Wong]

On Fri, Jul 20, 2018 at 09:05:50AM -0700, Christoph Hellwig wrote:
> On Thu, Jul 19, 2018 at 04:31:20PM -0400, Brian Foster wrote:
> > On Thu, Jul 19, 2018 at 12:47:59PM -0700, Christoph Hellwig wrote:
> > > On Thu, Jul 19, 2018 at 12:18:26PM -0400, Brian Foster wrote:
> > > > FYI, this accidentally leaves off libxfs/xfs_rtbitmap.c, which also
> > > > requires the xfs_defer.h include treatment.
> > > 
> > > I wonder if we shouldn't just move struct xfs_defer_ops to xfs_trans.h
> > > instead.
> > 
> > I thought about doing that, was curious how many files I'd have to
> > modify and ended up just plowing through them all. :P I'm happy to move
> > the struct def over if nobody objects.
> 
> To me moving it feels like the right thing.  In the end xfs_trans will be
> the prime user of xfs_defer_ops, assuming we even keep it as a separate
> structure at all.

I don't have a problem with moving it either, but if you do please make
sure it ports to xfsprogs (it probably will without much fuss).

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