xfs: embed dfops in the transaction
cover xfs: embed dfops in the transaction
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(-)
Signature · 1 line
2.17.1
PATCH 1/14 xfs: pull up dfops from xfs_itruncate_extents()
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.
--- fs/xfs/xfs_attr_inactive.c | 3 +++ fs/xfs/xfs_bmap_util.c | 2 ++ fs/xfs/xfs_inode.c | 8 +++----- fs/xfs/xfs_iops.c | 3 +++ fs/xfs/xfs_qm_syscalls.c | 3 +++ 5 files changed, 14 insertions(+), 5 deletions(-)
fs/xfs/xfs_attr_inactive.c +3 -0
@@ -26,6 +26,7 @@#include "xfs_quota.h" #include "xfs_trace.h" #include "xfs_dir2.h" +#include "xfs_defer.h" /* * Look at all the extents for this logical region,
@@ -381,6 +382,7 @@ xfs_attr_inactive( {struct xfs_trans *trans;struct xfs_mount *mp; +struct xfs_defer_ops dfops;int lock_mode =XFS_ILOCK_SHARED ;int error =0 ;
@@ -397,6 +399,7 @@ xfs_attr_inactive( error =xfs_trans_alloc (mp, &M_RES (mp)->tr_attrinval,0 ,0 ,0 , &trans);if (error) goto out_destroy_fork; +xfs_defer_init (trans, &dfops); lock_mode =XFS_ILOCK_EXCL ;xfs_ilock (dp, lock_mode);
fs/xfs/xfs_bmap_util.c +2 -0
@@ -792,6 +792,7 @@ xfs_free_eofblocks(int nimaps;struct xfs_bmbt_irec imap;struct xfs_mount *mp = ip->i_mount; +struct xfs_defer_ops dfops;/* * Figure out if there are any blocks beyond the end
@@ -831,6 +832,7 @@ xfs_free_eofblocks(ASSERT (XFS_FORCED_SHUTDOWN (mp));return error; } +xfs_defer_init (tp, &dfops);xfs_ilock (ip,XFS_ILOCK_EXCL );xfs_trans_ijoin (tp, ip,0 );
fs/xfs/xfs_inode.c +3 -5
@@ -1541,8 +1541,6 @@ xfs_itruncate_extents_flags( {struct xfs_mount *mp = ip->i_mount;struct xfs_trans *tp = *tpp; -struct xfs_defer_ops *odfops = tp->t_dfops; -struct xfs_defer_ops dfops;xfs_fileoff_t first_unmap_block;xfs_fileoff_t last_block;xfs_filblks_t unmap_len;
@@ -1579,7 +1577,7 @@ xfs_itruncate_extents_flags(ASSERT (first_unmap_block < last_block); unmap_len = last_block - first_unmap_block +1 ;while (!done) { -xfs_defer_init (tp, &dfops); +ASSERT (tp->t_firstblock ==NULLFSBLOCK ); error =xfs_bunmapi (tp, ip, first_unmap_block, unmap_len, flags,XFS_ITRUNC_MAX_EXTENTS , &done);if (error)
@@ -1618,8 +1616,6 @@ xfs_itruncate_extents_flags(trace_xfs_itruncate_extents_end (ip, new_size); out: -/* ->t_dfops points to local stack, don't leak it! */ - tp->t_dfops = odfops; *tpp = tp;return error; out_bmap_cancel:
@@ -1723,6 +1719,7 @@ xfs_inactive_truncate( {struct xfs_mount *mp = ip->i_mount;struct xfs_trans *tp; +struct xfs_defer_ops dfops;int error; error =xfs_trans_alloc (mp, &M_RES (mp)->tr_itruncate,0 ,0 ,0 , &tp);
@@ -1730,6 +1727,7 @@ xfs_inactive_truncate(ASSERT (XFS_FORCED_SHUTDOWN (mp));return error; } +xfs_defer_init (tp, &dfops);xfs_ilock (ip,XFS_ILOCK_EXCL );xfs_trans_ijoin (tp, ip,0 );
fs/xfs/xfs_iops.c +3 -0
@@ -26,6 +26,7 @@#include "xfs_dir2.h" #include "xfs_trans_space.h" #include "xfs_iomap.h" +#include "xfs_defer.h" #include <linux/capability.h> #include <linux/xattr.h>
@@ -812,6 +813,7 @@ xfs_setattr_size(struct inode *inode =VFS_I (ip);xfs_off_t oldsize, newsize;struct xfs_trans *tp; +struct xfs_defer_ops dfops;int error;uint lock_flags =0 ;bool did_zeroing = false;
@@ -915,6 +917,7 @@ xfs_setattr_size( error =xfs_trans_alloc (mp, &M_RES (mp)->tr_itruncate,0 ,0 ,0 , &tp);if (error)return error; +xfs_defer_init (tp, &dfops); lock_flags |=XFS_ILOCK_EXCL ;xfs_ilock (ip,XFS_ILOCK_EXCL );
fs/xfs/xfs_qm_syscalls.c +3 -0
@@ -22,6 +22,7 @@#include "xfs_qm.h" #include "xfs_trace.h" #include "xfs_icache.h" +#include "xfs_defer.h" STATIC int xfs_qm_log_quotaoff(xfs_mount_t *, xfs_qoff_logitem_t **, uint); STATIC int xfs_qm_log_quotaoff_end(xfs_mount_t *, xfs_qoff_logitem_t *,
@@ -213,6 +214,7 @@ xfs_qm_scall_trunc_qfile( {struct xfs_inode *ip;struct xfs_trans *tp; +struct xfs_defer_ops dfops;int error;if (ino ==NULLFSINO )
@@ -229,6 +231,7 @@ xfs_qm_scall_trunc_qfile(xfs_iunlock (ip,XFS_IOLOCK_EXCL ); goto out_put; } +xfs_defer_init (tp, &dfops);xfs_ilock (ip,XFS_ILOCK_EXCL );xfs_trans_ijoin (tp, ip,0 );
Signature · 1 line
2.17.1
Re: [PATCH 01/14] xfs: pull up dfops from xfs_itruncate_extents()
Quoted 5 lines · 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.
Quoted 9 lines
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 thexfs_defer_init()in thexfs_itruncate_extents_flags()loop with an assert. Both dfops and firstblock should be in a valid state afterxfs_defer_finish()and the inode joined to the dfops is fixed throughout the loop.
Yes, this looks pretty sensible:
PATCH 2/14 xfs: use ->t_dfops in log recovery intent processing
In reply to Brian Fosterxlog_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.
--- fs/xfs/xfs_log_recover.c | 2 ++ 1 file changed, 2 insertions(+)
fs/xfs/xfs_log_recover.c +2 -0
@@ -4854,6 +4854,8 @@ xlog_finish_defer_ops( 0, XFS_TRANS_RESERVE, &tp);if (error)return error; +/* dfops is already populated so assign it manually */ + tp->t_dfops = dfops; error =xfs_defer_finish (&tp, dfops);if (error)
Signature · 1 line
2.17.1
Re: [PATCH 02/14] xfs: use ->t_dfops in log recovery intent processing
Quoted 5 lines · Brian Foster
xlog_finish_defer_ops()processes the deferred operations collected over the entire intent recovery sequence. We can'txfs_defer_init()here because the dfops is already populated. Attach it manually and eliminate the last caller ofxfs_defer_finish()that doesn't pass ->t_dfops.
Looks good,
PATCH 3/14 xfs: fix transaction leak on remote attr set/remove failure
In reply to Brian FosterThe 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.
--- fs/xfs/libxfs/xfs_attr_remote.c | 2 -- 1 file changed, 2 deletions(-)
fs/xfs/libxfs/xfs_attr_remote.c +0 -2
@@ -558,7 +558,6 @@ xfs_attr_rmtval_set(return 0 ; out_defer_cancel:xfs_defer_cancel (args->trans->t_dfops); - args->trans =NULL ;return error; }
@@ -646,6 +645,5 @@ xfs_attr_rmtval_remove(return 0 ; out_defer_cancel:xfs_defer_cancel (args->trans->t_dfops); - args->trans =NULL ;return error; }
Signature · 1 line
2.17.1
Re: [PATCH 03/14] xfs: fix transaction leak on remote attr set/remove failure
Quoted 9 lines · 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.
Looks good,
PATCH 4/14 xfs: make deferred processing safe for embedded dfops
In reply to Brian Fosterxfs_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.
--- fs/xfs/libxfs/xfs_defer.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-)
fs/xfs/libxfs/xfs_defer.c +10 -16
@@ -225,9 +225,9 @@ xfs_defer_trans_abort(/* Roll a transaction so we can do some deferred op processing. */ STATIC intxfs_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
Signature · 1 line
2.17.1
Re: [PATCH 04/14] xfs: make deferred processing safe for embedded dfops
Yes, this looks pretty sensible:
PATCH 5/14 xfs: remove unused deferred ops committed field
In reply to Brian Fosterdop_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.
--- fs/xfs/libxfs/xfs_defer.c | 1 - fs/xfs/libxfs/xfs_defer.h | 1 - fs/xfs/xfs_trace.h | 8 ++------ 3 files changed, 2 insertions(+), 8 deletions(-)
fs/xfs/libxfs/xfs_defer.c +0 -1
@@ -249,7 +249,6 @@ xfs_defer_trans_roll(xfs_defer_trans_abort (*tp, dop, error);return error; } - dop->dop_committed = true;/* Rejoin the joined inodes. */ for (i =0 ; i <XFS_DEFER_OPS_NR_INODES && dop->dop_inodes[i]; i++)
fs/xfs/libxfs/xfs_defer.h +0 -1
@@ -49,7 +49,6 @@ enum xfs_defer_ops_type {#define XFS_DEFER_OPS_NR_BUFS 2/* join up to two buffers */ struct xfs_defer_ops { -bool dop_committed;/* did any trans commit? */ bool dop_low;/* alloc in low mode */ struct list_head dop_intake;/* unlogged pending work */ struct list_head dop_pending;/* logged pending work */
fs/xfs/xfs_trace.h +2 -6
@@ -2229,14 +2229,12 @@ DECLARE_EVENT_CLASS(xfs_defer_class,TP_fast_assign ( __entry->dev = mp ? mp->m_super->s_dev : 0; __entry->dop = dop; - __entry->committed = dop->dop_committed; __entry->low = dop->dop_low; __entry->caller_ip = caller_ip; ), -TP_printk ("dev %d:%d ops %p committed %d low %d, caller %pS" , +TP_printk ("dev %d:%d ops %p low %d, caller %pS" ,MAJOR (__entry->dev),MINOR (__entry->dev), __entry->dop, - __entry->committed, __entry->low, (char *)__entry->caller_ip) )
@@ -2259,14 +2257,12 @@ DECLARE_EVENT_CLASS(xfs_defer_error_class,TP_fast_assign ( __entry->dev = mp ? mp->m_super->s_dev : 0; __entry->dop = dop; - __entry->committed = dop->dop_committed; __entry->low = dop->dop_low; __entry->error = error; ), -TP_printk ("dev %d:%d ops %p committed %d low %d err %d" , +TP_printk ("dev %d:%d ops %p low %d err %d" ,MAJOR (__entry->dev),MINOR (__entry->dev), __entry->dop, - __entry->committed, __entry->low, __entry->error) )
Signature · 1 line
2.17.1
Re: [PATCH 05/14] xfs: remove unused deferred ops committed field
Quoted 6 lines · Brian Foster
dop_committedis set when deferred item processing rolls the transaction at least once, but is only ever accessed in tracepoints. The transaction roll/commit events are already available via independent tracepoints, so remove the otherwise unused field.Signed-off-by Brian Foster <[email protected]>
Looks good,
PATCH 6/14 xfs: reset dfops to initial state after finish
In reply to Brian Fosterxfs_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.
--- fs/xfs/libxfs/xfs_defer.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)
fs/xfs/libxfs/xfs_defer.c +28 -2
@@ -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; }
Signature · 1 line
2.17.1
Re: [PATCH 06/14] xfs: reset dfops to initial state after finish
Quoted 44 lines · Brian Foster
xfs_defer_init()is currently used in two particular situations. The first and most obvious case is raw initialization of anxfs_defer_opsstruct. The other case is partial reinit ofxfs_defer_opson 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()orxfs_defer_init()if dfops is to be reused, we can shift some of the init logic intoxfs_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 anxfs_defer_finish()without the need to change any calling code.Signed-off-by Brian Foster <[email protected]>--- fs/xfs/libxfs/xfs_defer.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)fs/xfs/libxfs/xfs_defer.c +12
-0@@ -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++) {
fs/xfs/libxfs/xfs_defer.c xfs_defer_bjoin( · line 343
Quoted 10 lines
fs/xfs/libxfs/xfs_defer.c +10
-0+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:
Re: [PATCH 06/14] xfs: reset dfops to initial state after finish
Quoted 50 lines · Christoph Hellwig
Quoted 49 lines · Brian Foster
xfs_defer_init()is currently used in two particular situations. The first and most obvious case is raw initialization of anxfs_defer_opsstruct. The other case is partial reinit ofxfs_defer_opson 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()orxfs_defer_init()if dfops is to be reused, we can shift some of the init logic intoxfs_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 anxfs_defer_finish()without the need to change any calling code.Signed-off-by Brian Foster <[email protected]>--- fs/xfs/libxfs/xfs_defer.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)fs/xfs/libxfs/xfs_defer.c +17
-0@@ -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++) {
Quoted 9 lines
fs/xfs/libxfs/xfs_defer.c xfs_defer_bjoin( · line 343
Quoted 5 lines
fs/xfs/libxfs/xfs_defer.c +5
-0+if (!dop->dop_bufs[i]) +break ; + dop->dop_bufs[i] =NULL ; + } +}Are these loops really more efficient than just doing memsets of
dop_inodesanddop_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
Quoted 3 lines
Otherwise looks good:
Reviewed-by Christoph Hellwig <[email protected]>
Re: [PATCH 06/14] xfs: reset dfops to initial state after finish
Quoted 2 lines · Brian Foster
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.
PATCH 7/14 xfs: pack holes in xfs_defer_ops and xfs_trans
In reply to Brian FosterBoth 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.
--- fs/xfs/libxfs/xfs_defer.h | 3 ++- fs/xfs/xfs_trans.h | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-)
fs/xfs/libxfs/xfs_defer.h +2 -1
@@ -49,13 +49,14 @@ enum xfs_defer_ops_type {#define XFS_DEFER_OPS_NR_BUFS 2/* join up to two buffers */ struct xfs_defer_ops { -bool dop_low;/* alloc in low mode */ struct list_head dop_intake;/* unlogged pending work */ struct list_head dop_pending;/* logged pending work */ /* relog these with each roll */ struct xfs_inode *dop_inodes[XFS_DEFER_OPS_NR_INODES ];struct xfs_buf *dop_bufs[XFS_DEFER_OPS_NR_BUFS ]; + +bool dop_low;/* alloc in low mode */ }; void xfs_defer_add(struct xfs_defer_ops *dop, enum xfs_defer_ops_type type,
fs/xfs/xfs_trans.h +1 -1
@@ -102,12 +102,12 @@ typedef struct xfs_trans {unsigned int t_blk_res_used;/* # of resvd blocks used */ unsigned int t_rtx_res;/* # of rt extents resvd */ unsigned int t_rtx_res_used;/* # of resvd rt extents used */ +unsigned int t_flags;/* misc flags */ xfs_fsblock_t t_firstblock;/* first block allocated */ struct xlog_ticket *t_ticket;/* log mgr ticket */ struct xfs_mount *t_mountp;/* ptr to fs mount struct */ struct xfs_dquot_acct *t_dqinfo;/* acctg info for dquots */ struct xfs_defer_ops *t_dfops;/* dfops reference */ -unsigned int t_flags;/* misc flags */ int64_t t_icount_delta;/* superblock icount change */ int64_t t_ifree_delta;/* superblock ifree change */ int64_t t_fdblocks_delta;/* superblock fdblocks chg */
Signature · 1 line
2.17.1
Re: [PATCH 07/14] xfs: pack holes in xfs_defer_ops and xfs_trans
Quoted 5 lines · Brian Foster
Both structures have holes due to member alignment. Move
dop_lowto the end ofxfs_deferops to sanitize the cache line alignment and movet_flagsto save 8 bytes inxfs_trans.Signed-off-by Brian Foster <[email protected]>
Looks good,
PATCH 8/14 xfs: support embedded dfops in transaction
In reply to Brian FosterThe 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.
--- 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(-)
fs/xfs/libxfs/xfs_alloc_btree.c +1 -0
@@ -18,6 +18,7 @@#include "xfs_error.h" #include "xfs_trace.h" #include "xfs_cksum.h" +#include "xfs_defer.h" #include "xfs_trans.h"
fs/xfs/libxfs/xfs_attr_leaf.c +1 -0
@@ -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"
fs/xfs/libxfs/xfs_da_btree.c +1 -0
@@ -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"
fs/xfs/libxfs/xfs_defer.c +33 -0
@@ -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); +}
fs/xfs/libxfs/xfs_defer.h +1 -0
@@ -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 {
fs/xfs/libxfs/xfs_dir2_block.c +1 -0
@@ -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"
fs/xfs/libxfs/xfs_dir2_data.c +1 -0
@@ -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"
fs/xfs/libxfs/xfs_dir2_leaf.c +1 -0
@@ -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"
fs/xfs/libxfs/xfs_dir2_node.c +1 -0
@@ -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"
fs/xfs/libxfs/xfs_dir2_sf.c +1 -0
@@ -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"
fs/xfs/libxfs/xfs_dquot_buf.c +1 -0
@@ -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"
fs/xfs/libxfs/xfs_ialloc_btree.c +1 -0
@@ -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"
fs/xfs/libxfs/xfs_inode_fork.c +1 -0
@@ -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"
fs/xfs/libxfs/xfs_refcount_btree.c +1 -0
@@ -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"
fs/xfs/libxfs/xfs_symlink_remote.c +1 -0
@@ -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"
fs/xfs/libxfs/xfs_trans_resv.c +1 -0
@@ -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"
fs/xfs/xfs_aops.c +1 -0
@@ -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"
fs/xfs/xfs_attr_inactive.c +1 -1
@@ -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,
fs/xfs/xfs_attr_list.c +1 -0
@@ -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"
fs/xfs/xfs_buf_item.c +1 -0
@@ -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"
fs/xfs/xfs_dir2_readdir.c +1 -0
@@ -19,6 +19,7 @@#include "xfs_error.h" #include "xfs_trace.h" #include "xfs_bmap.h" +#include "xfs_defer.h" #include "xfs_trans.h" /*
fs/xfs/xfs_dquot_item.c +1 -0
@@ -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"
fs/xfs/xfs_export.c +1 -0
@@ -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"
fs/xfs/xfs_extent_busy.c +1 -0
@@ -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"
fs/xfs/xfs_extfree_item.c +1 -0
@@ -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"
fs/xfs/xfs_file.c +1 -0
@@ -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"
fs/xfs/xfs_icache.c +1 -0
@@ -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"
fs/xfs/xfs_icreate_item.c +1 -0
@@ -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"
fs/xfs/xfs_inode_item.c +1 -0
@@ -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"
fs/xfs/xfs_ioctl.c +1 -0
@@ -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"
fs/xfs/xfs_iops.c +1 -1
@@ -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>
fs/xfs/xfs_log.c +1 -0
@@ -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"
fs/xfs/xfs_log_cil.c +1 -0
@@ -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"
fs/xfs/xfs_pnfs.c +1 -0
@@ -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"
fs/xfs/xfs_qm.c +1 -0
@@ -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"
fs/xfs/xfs_qm_bhv.c +1 -0
@@ -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"
fs/xfs/xfs_qm_syscalls.c +1 -1
@@ -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 *,
fs/xfs/xfs_quotaops.c +1 -0
@@ -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"
fs/xfs/xfs_super.c +1 -0
@@ -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"
fs/xfs/xfs_trans.c +27 -5
@@ -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
fs/xfs/xfs_trans.h +1 -1
@@ -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;/*
fs/xfs/xfs_trans_ail.c +1 -0
@@ -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"
fs/xfs/xfs_trans_buf.c +1 -0
@@ -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"
fs/xfs/xfs_trans_dquot.c +1 -0
@@ -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"
fs/xfs/xfs_trans_inode.c +1 -0
@@ -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"
Signature · 1 line
2.17.1
Re: [PATCH 08/14] xfs: support embedded dfops in transaction
Quoted 30 lines · 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_opsin 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.
Quoted 7 lines
This patch adds the fundamental ability to support an internal dfops. All codepaths that perform deferred processing continue to override the internal dfops until they are converted over in subsequent patches.
Signed-off-by Brian Foster <[email protected]>---
FYI, this accidentally leaves off libxfs/xfs_rtbitmap.c, which also requires the xfs_defer.h include treatment.
Brian
Quoted 720 lines
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(-)fs/xfs/libxfs/xfs_alloc_btree.c +1
-0@@ -18,6 +18,7 @@#include "xfs_error.h" #include "xfs_trace.h" #include "xfs_cksum.h" +#include "xfs_defer.h" #include "xfs_trans.h" fs/xfs/libxfs/xfs_attr_leaf.c +1
-0@@ -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" fs/xfs/libxfs/xfs_da_btree.c +1
-0@@ -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" fs/xfs/libxfs/xfs_defer.c +33
-0@@ -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); +}fs/xfs/libxfs/xfs_defer.h +1
-0@@ -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 {fs/xfs/libxfs/xfs_dir2_block.c +1
-0@@ -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" fs/xfs/libxfs/xfs_dir2_data.c +1
-0@@ -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" fs/xfs/libxfs/xfs_dir2_leaf.c +1
-0@@ -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" fs/xfs/libxfs/xfs_dir2_node.c +1
-0@@ -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" fs/xfs/libxfs/xfs_dir2_sf.c +1
-0@@ -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" fs/xfs/libxfs/xfs_dquot_buf.c +1
-0@@ -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" fs/xfs/libxfs/xfs_ialloc_btree.c +1
-0@@ -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" fs/xfs/libxfs/xfs_inode_fork.c +1
-0@@ -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" fs/xfs/libxfs/xfs_refcount_btree.c +1
-0@@ -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" fs/xfs/libxfs/xfs_symlink_remote.c +1
-0@@ -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" fs/xfs/libxfs/xfs_trans_resv.c +1
-0@@ -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" fs/xfs/xfs_aops.c +1
-0@@ -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" fs/xfs/xfs_attr_inactive.c +1
-1@@ -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, fs/xfs/xfs_attr_list.c +1
-0@@ -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" fs/xfs/xfs_buf_item.c +1
-0@@ -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" fs/xfs/xfs_dir2_readdir.c +1
-0@@ -19,6 +19,7 @@#include "xfs_error.h" #include "xfs_trace.h" #include "xfs_bmap.h" +#include "xfs_defer.h" #include "xfs_trans.h" /* fs/xfs/xfs_dquot_item.c +1
-0@@ -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" fs/xfs/xfs_export.c +1
-0@@ -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" fs/xfs/xfs_extent_busy.c +1
-0@@ -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" fs/xfs/xfs_extfree_item.c +1
-0@@ -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" fs/xfs/xfs_file.c +1
-0@@ -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" fs/xfs/xfs_icache.c +1
-0@@ -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" fs/xfs/xfs_icreate_item.c +1
-0@@ -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" fs/xfs/xfs_inode_item.c +1
-0@@ -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" fs/xfs/xfs_ioctl.c +1
-0@@ -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" fs/xfs/xfs_iops.c +1
-1@@ -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> fs/xfs/xfs_log.c +1
-0@@ -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" fs/xfs/xfs_log_cil.c +1
-0@@ -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" fs/xfs/xfs_pnfs.c +1
-0@@ -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" fs/xfs/xfs_qm.c +1
-0@@ -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" fs/xfs/xfs_qm_bhv.c +1
-0@@ -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" fs/xfs/xfs_qm_syscalls.c +1
-1@@ -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 *,fs/xfs/xfs_quotaops.c +1
-0@@ -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" fs/xfs/xfs_super.c +1
-0@@ -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" fs/xfs/xfs_trans.c +27
-5@@ -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 fs/xfs/xfs_trans.h +1
-1@@ -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;/* fs/xfs/xfs_trans_ail.c +1
-0@@ -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" fs/xfs/xfs_trans_buf.c +1
-0@@ -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" fs/xfs/xfs_trans_dquot.c +1
-0@@ -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" fs/xfs/xfs_trans_inode.c +1
-0@@ -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" Signature · 6 lines
2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/14] xfs: support embedded dfops in transaction
Quoted 2 lines · Brian Foster
FYI, this accidentally leaves off
libxfs/xfs_rtbitmap.c, which also requires thexfs_defer.hinclude treatment.
I wonder if we shouldn't just move struct xfs_defer_ops to xfs_trans.h instead.
Re: [PATCH 08/14] xfs: support embedded dfops in transaction
Quoted 6 lines · Christoph Hellwig
Quoted 2 lines · Brian Foster
FYI, this accidentally leaves off
libxfs/xfs_rtbitmap.c, which also requires thexfs_defer.hinclude treatment.I wonder if we shouldn't just move struct
xfs_defer_opstoxfs_trans.hinstead.
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
Quoted 11 lines · Brian Foster
Quoted 6 lines · Christoph Hellwig
Quoted 2 lines · Brian Foster
FYI, this accidentally leaves off
libxfs/xfs_rtbitmap.c, which also requires thexfs_defer.hinclude treatment.I wonder if we shouldn't just move struct
xfs_defer_opstoxfs_trans.hinstead.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
Quoted 9 lines · Christoph Hellwig
Quoted 8 lines · Brian Foster
Quoted 6 lines · Christoph Hellwig
Quoted 2 lines · Brian Foster
FYI, this accidentally leaves off
libxfs/xfs_rtbitmap.c, which also requires thexfs_defer.hinclude treatment.I wonder if we shouldn't just move struct
xfs_defer_opstoxfs_trans.hinstead.
Quoted 7 lines
Quoted 3 lines
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_transwill be the prime user ofxfs_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
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/14] xfs: support embedded dfops in transaction
In reply to Brian FosterQuoted 18 lines
fs/xfs/libxfs/xfs_defer.c +17
-0+/* +* 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); +
fs/xfs/libxfs/xfs_defer.c xfs_defer_init( · line 587
Quoted 10 lines
fs/xfs/libxfs/xfs_defer.c +10
-0+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 08/14] xfs: support embedded dfops in transaction
Quoted 22 lines · Christoph Hellwig
Quoted 22 lines
fs/xfs/libxfs/xfs_defer.c +21
-0+/* +* 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];
Quoted 8 lines
fs/xfs/libxfs/xfs_defer.c xfs_defer_init( · line 587
Quoted 6 lines
fs/xfs/libxfs/xfs_defer.c +6
-0+ } +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 08/14] xfs: support embedded dfops in transaction
Quoted 3 lines · Brian Foster
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.
PATCH 9/14 xfs: use internal dfops in cow blocks cancel
In reply to Brian FosterAll 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.
--- fs/xfs/xfs_reflink.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
fs/xfs/xfs_reflink.c +2 -4
@@ -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; }
Signature · 1 line
2.17.1
Re: [PATCH 09/14] xfs: use internal dfops in cow blocks cancel
fs/xfs/xfs_reflink.c xfs_reflink_cancel_cow_blocks( · line 513
Quoted 2 lines
fs/xfs/xfs_reflink.c +2
-0+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 09/14] xfs: use internal dfops in cow blocks cancel
Quoted 5 lines · Christoph Hellwig
fs/xfs/xfs_reflink.c xfs_reflink_cancel_cow_blocks( · line 513
Quoted 2 lines
fs/xfs/xfs_reflink.c +2
-0+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
PATCH 10/14 xfs: use internal dfops in attr code
In reply to Brian FosterRemove the unnecessary on-stack dfops structure and use the internal transaction dfops instead. The lower level xattr code already appropriately accesses ->t_dfops throughout.
--- fs/xfs/libxfs/xfs_attr.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
fs/xfs/libxfs/xfs_attr.c +5 -11
@@ -202,7 +202,6 @@ xfs_attr_set(struct xfs_mount *mp = dp->i_mount;struct xfs_buf *leaf_bp =NULL ;struct xfs_da_args args; -struct xfs_defer_ops dfops;struct xfs_trans_res tres;int rsvd = (flags &ATTR_ROOT ) !=0 ;int error, err2, local;
@@ -251,7 +250,6 @@ xfs_attr_set( rsvd ? XFS_TRANS_RESERVE : 0, &args.trans);if (error)return error; -xfs_defer_init (args.trans, &dfops);xfs_ilock (dp,XFS_ILOCK_EXCL ); error = xfs_trans_reserve_quota_nblks(args.trans, dp, args.total, 0,
@@ -315,18 +313,18 @@ xfs_attr_set(*/ error =xfs_attr_shortform_to_leaf (&args, &leaf_bp);if (error) - goto out_defer_cancel; + goto out;/* * Prevent the leaf buffer from being unlocked so that a * concurrent AIL push cannot grab the half-baked leaf * buffer and run into problems with the write verifier. */ xfs_trans_bhold (args.trans, leaf_bp); -xfs_defer_bjoin (&dfops, leaf_bp); -xfs_defer_ijoin (&dfops, dp); - error =xfs_defer_finish (&args.trans, &dfops); +xfs_defer_bjoin (args.trans->t_dfops, leaf_bp); +xfs_defer_ijoin (args.trans->t_dfops, dp); + error =xfs_defer_finish (&args.trans, args.trans->t_dfops);if (error) - goto out_defer_cancel; + goto out;/* * Commit the leaf transformation. We'll need another (linked)
@@ -366,8 +364,6 @@ xfs_attr_set(return error; -out_defer_cancel: -xfs_defer_cancel (&dfops); out:if (leaf_bp)xfs_trans_brelse (args.trans, leaf_bp);
@@ -389,7 +385,6 @@ xfs_attr_remove( {struct xfs_mount *mp = dp->i_mount;struct xfs_da_args args; -struct xfs_defer_ops dfops;int error;XFS_STATS_INC (mp, xs_attr_remove);
@@ -422,7 +417,6 @@ xfs_attr_remove( &args.trans);if (error)return error; -xfs_defer_init (args.trans, &dfops);xfs_ilock (dp,XFS_ILOCK_EXCL );/*
Signature · 1 line
2.17.1
PATCH 11/14 xfs: use internal dfops during [b|c]ui recovery
In reply to Brian Fosterbmap 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.
--- fs/xfs/xfs_bmap_item.c | 21 +++++++++++---------- fs/xfs/xfs_log_recover.c | 6 +++--- fs/xfs/xfs_refcount_item.c | 30 ++++++++++++++++-------------- 3 files changed, 30 insertions(+), 27 deletions(-)
fs/xfs/xfs_bmap_item.c +11 -10
@@ -441,7 +441,12 @@ xfs_bui_recover( XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK), 0, 0, &tp);if (error)return error; - tp->t_dfops = dfops; +/* +* Recovery stashes all deferred ops during intent processing and +* finishes them on completion. Transfer current dfops state to this +* transaction and transfer the result back before we return. +*/ +xfs_defer_move (tp->t_dfops, dfops); budp =xfs_trans_get_bud (tp, buip);/* Grab the inode. */
@@ -470,7 +475,7 @@ xfs_bui_recover(xfs_trans_ijoin (tp, ip,0 ); count = bmap->me_len; - error =xfs_trans_log_finish_bmap_update (tp, budp, dfops, type, + error =xfs_trans_log_finish_bmap_update (tp, budp, tp->t_dfops, type, ip, whichfork, bmap->me_startoff, bmap->me_startblock, &count, state);if (error)
@@ -482,18 +487,14 @@ xfs_bui_recover( irec.br_blockcount = count; irec.br_startoff = bmap->me_startoff; irec.br_state = state; - error =xfs_bmap_unmap_extent (tp->t_mountp, dfops, ip, &irec); + error =xfs_bmap_unmap_extent (tp->t_mountp, tp->t_dfops, ip, + &irec);if (error) goto err_inode; }set_bit (XFS_BUI_RECOVERED , &buip->bui_flags); -/* -* Recovery finishes all deferred ops once intent processing is -* complete. Reset the trans reference because commit expects a finished -* dfops or none at all. -*/ - tp->t_dfops =NULL ; +xfs_defer_move (dfops, tp->t_dfops); error =xfs_trans_commit (tp);xfs_iunlock (ip,XFS_ILOCK_EXCL );IRELE (ip);
@@ -501,7 +502,7 @@ xfs_bui_recover(return error; err_inode: - tp->t_dfops =NULL ; +xfs_defer_move (dfops, tp->t_dfops);xfs_trans_cancel (tp);if (ip) {xfs_iunlock (ip,XFS_ILOCK_EXCL );
fs/xfs/xfs_log_recover.c +3 -3
@@ -4854,10 +4854,10 @@ xlog_finish_defer_ops( 0, XFS_TRANS_RESERVE, &tp);if (error)return error; -/* dfops is already populated so assign it manually */ - tp->t_dfops = dfops; +/* transfer all collected dfops to this transaction */ +xfs_defer_move (tp->t_dfops, dfops); - error =xfs_defer_finish (&tp, dfops); + error =xfs_defer_finish (&tp, tp->t_dfops);if (error) goto out_cancel;
fs/xfs/xfs_refcount_item.c +16 -14
@@ -452,7 +452,12 @@ xfs_cui_recover( mp->m_refc_maxlevels * 2, 0, XFS_TRANS_RESERVE, &tp);if (error)return error; - tp->t_dfops = dfops; +/* +* Recovery stashes all deferred ops during intent processing and +* finishes them on completion. Transfer current dfops state to this +* transaction and transfer the result back before we return. +*/ +xfs_defer_move (tp->t_dfops, dfops); cudp =xfs_trans_get_cud (tp, cuip);for (i =0 ; i < cuip->cui_format.cui_nextents; i++) {
@@ -474,8 +479,8 @@ xfs_cui_recover( new_len = refc->pe_len; }else error =xfs_trans_log_finish_refcount_update (tp, cudp, - dfops, type, refc->pe_startblock, refc->pe_len, - &new_fsb, &new_len, &rcur); + tp->t_dfops, type, refc->pe_startblock, + refc->pe_len, &new_fsb, &new_len, &rcur);if (error) goto abort_error;
@@ -486,21 +491,23 @@ xfs_cui_recover(switch (type) {case XFS_REFCOUNT_INCREASE : error =xfs_refcount_increase_extent ( - tp->t_mountp, dfops, &irec); + tp->t_mountp, tp->t_dfops, + &irec);break ;case XFS_REFCOUNT_DECREASE : error =xfs_refcount_decrease_extent ( - tp->t_mountp, dfops, &irec); + tp->t_mountp, tp->t_dfops, + &irec);break ;case XFS_REFCOUNT_ALLOC_COW : error =xfs_refcount_alloc_cow_extent ( - tp->t_mountp, dfops, + tp->t_mountp, tp->t_dfops, irec.br_startblock, irec.br_blockcount);break ;case XFS_REFCOUNT_FREE_COW : error =xfs_refcount_free_cow_extent ( - tp->t_mountp, dfops, + tp->t_mountp, tp->t_dfops, irec.br_startblock, irec.br_blockcount);break ;
@@ -515,18 +522,13 @@ xfs_cui_recover(xfs_refcount_finish_one_cleanup (tp, rcur, error);set_bit (XFS_CUI_RECOVERED , &cuip->cui_flags); -/* -* Recovery finishes all deferred ops once intent processing is -* complete. Reset the trans reference because commit expects a finished -* dfops or none at all. -*/ - tp->t_dfops =NULL ; +xfs_defer_move (dfops, tp->t_dfops); error =xfs_trans_commit (tp);return error; abort_error:xfs_refcount_finish_one_cleanup (tp, rcur, error); - tp->t_dfops =NULL ; +xfs_defer_move (dfops, tp->t_dfops);xfs_trans_cancel (tp);return error; }
Signature · 1 line
2.17.1
PATCH 12/14 xfs: remove all boilerplate defer init/finish code
In reply to Brian FosterAt 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.
--- 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(-)
fs/xfs/libxfs/xfs_bmap.c +1 -15
@@ -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; }
fs/xfs/libxfs/xfs_refcount.c +1 -9
@@ -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:
fs/xfs/xfs_attr_inactive.c +0 -2
@@ -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);
fs/xfs/xfs_bmap_util.c +6 -37
161 lines not on this page · raw message
fs/xfs/xfs_dquot.c +0 -4
@@ -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)
fs/xfs/xfs_inode.c +13 -66
279 lines not on this page · raw message
fs/xfs/xfs_iomap.c +2 -24
@@ -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;
fs/xfs/xfs_iops.c +0 -2
@@ -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 );
fs/xfs/xfs_log_recover.c +0 -8
@@ -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; }/*
fs/xfs/xfs_qm_syscalls.c +0 -2
@@ -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 );
fs/xfs/xfs_reflink.c +12 -25
@@ -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 );
fs/xfs/xfs_rtalloc.c +1 -8
@@ -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;
fs/xfs/xfs_symlink.c +8 -30
@@ -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:
Signature · 1 line
2.17.1
PATCH 13/14 xfs: remove unnecessary dfops init calls in xattr code
In reply to Brian FosterEach 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.
--- fs/xfs/libxfs/xfs_attr.c | 8 -------- fs/xfs/libxfs/xfs_attr_remote.c | 3 --- 2 files changed, 11 deletions(-)
fs/xfs/libxfs/xfs_attr.c +0 -8
@@ -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)
fs/xfs/libxfs/xfs_attr_remote.c +0 -3
@@ -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)
Signature · 1 line
2.17.1
PATCH 14/14 xfs: drop unnecessary xfs_defer_finish() dfops parameter
In reply to Brian FosterEvery 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.
--- 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(-)
fs/xfs/libxfs/xfs_attr.c +13 -14
@@ -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; }
fs/xfs/libxfs/xfs_attr_remote.c +4 -4
@@ -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; }
fs/xfs/libxfs/xfs_defer.c +3 -4
@@ -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;
fs/xfs/libxfs/xfs_defer.h +2 -2
@@ -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);
fs/xfs/xfs_bmap_util.c +2 -2
@@ -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;
fs/xfs/xfs_dquot.c +2 -2
@@ -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; }
fs/xfs/xfs_inode.c +2 -2
@@ -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; }
fs/xfs/xfs_log_recover.c +1 -1
@@ -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);
fs/xfs/xfs_reflink.c +4 -4
@@ -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; }
fs/xfs/xfs_trans.c +3 -3
@@ -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
fs/xfs/xfs_trans.h +8 -0
@@ -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 *);
Signature · 1 line
2.17.1
Quoted 6 lines · Brian Foster
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...
Quoted 2 lines
I know Christoph also had thoughts around condensing some of the items joined to the dfops to those with the transaction.
... this.
Quoted 9 lines
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 inxfs_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.
Quoted 18 lines · Christoph Hellwig
Quoted 6 lines · Brian Foster
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_opsaren'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[]; };
Quoted 9 lines
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...
Quoted 2 lines
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.
Quoted 10 lines
Quoted 9 lines
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 inxfs_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..
Quoted 4 lines
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
Quoted 29 lines · Brian Foster
Quoted 18 lines · Christoph Hellwig
Quoted 6 lines · Brian Foster
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_opsaren'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[]; };Quoted 9 lines
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...
Quoted 2 lines
I know Christoph also had thoughts around condensing some of the items joined to the dfops to those with the transaction.
... this.
Quoted 7 lines
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>
Quoted 15 lines
Quoted 10 lines
Quoted 9 lines
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 inxfs_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..Quoted 4 lines
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.
Quoted 8 lines
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_lowmode 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
Quoted 5 lines
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 64 lines · Darrick J. Wong
Quoted 29 lines · Brian Foster
Quoted 18 lines · Christoph Hellwig
Quoted 6 lines · Brian Foster
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_opsaren'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[]; };Quoted 9 lines
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...
Quoted 2 lines
I know Christoph also had thoughts around condensing some of the items joined to the dfops to those with the transaction.
... this.
Quoted 7 lines
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>
Quoted 15 lines
Quoted 10 lines
Quoted 9 lines
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 inxfs_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..Quoted 4 lines
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.
Quoted 8 lines
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_lowmode 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.
Quoted 7 lines
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
Quoted 11 lines
--D
Quoted 5 lines
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 38 lines · Darrick J. Wong
Quoted 29 lines · Brian Foster
Quoted 18 lines · Christoph Hellwig
Quoted 6 lines · Brian Foster
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_opsaren'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[]; };Quoted 9 lines
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...
Quoted 2 lines
I know Christoph also had thoughts around condensing some of the items joined to the dfops to those with the transaction.
... this.
Quoted 7 lines
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.
Quoted 2 lines
<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< ---
fs/xfs/libxfs/xfs_bmap.c +1 -0
@@ -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;
fs/xfs/xfs_bmap_util.c +1 -0
@@ -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)
fs/xfs/xfs_inode.c +1 -0
@@ -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",
fs/xfs/xfs_iomap.c +3 -0
@@ -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)
fs/xfs/xfs_reflink.c +1 -0
@@ -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;
Quoted 3 lines · Brian Foster
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.
Quoted 9 lines
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 withlock_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.
Quoted 9 lines · Darrick J. Wong
Quoted 8 lines
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_lowmode 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.
Quoted 6 lines
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.