xfs: defer agfl block frees

PATCH In review Brian Foster 2 participants 21 messages Apr 18, 2018 – May 9, 2018 XFS filesystem

lore

Share & tools

lore

Brian Foster

author

2018-04-18 13:31 UTC #0 lore raw

v2 cover xfs: defer agfl block frees

Hi all,

This version drops all refactoring and cleanup changes related to the tp pointer to avoid confusion between functional fixes and cleanup work that was somewhat intertwined in the previous versions. Hence, the purpose of this series is now purely to enable deferred AGFL block frees. Because this behavior is enabled at transaction granularity, carry the dfops to the allocator via the transaction.

This avoids the need for the extra plumbing that was implemented in rfcv1 (and removed in rfcv2) and facilitates cleanups like those demonstrated in the previous version. I'll rename ->t_agfl_dfops to ->t_dfops and do the broader cleanup work in a separate series once this series is complete.

Thoughts, reviews, flames appreciated.

Brian

v2:
- Drop all refactoring/cleanup bits.
- Rename dfops pointer to ->t_agfl_dfops for explicit use by AGFL fixup code.
v1: https://marc.info/?l=linux-xfs&m=152338240213442&w=2
- Rebased to for-next.
- Attach dfops to tp via xfs_defer_init().
- Fix up xfs_defer_finish() to restore original pointer.
- Dropped debug/test patches.
rfcv2: https://marc.info/?l=linux-xfs&m=151681946702093&w=2
- Use transaction to carry deferred ops struct to allocation context.
- Remove dfops param from dir interfaces where possible.
- Defer AGFL block frees from more contexts.
- Define runtime stat for transaction regrant and logcount patch (to be potentially removed).
rfcv1: https://marc.info/?l=linux-xfs&m=151267309423883&w=2

Brian Foster (6):
xfs: create agfl block free helper function
xfs: defer agfl block frees when dfops is available
xfs: defer agfl block frees from deferred ops processing context
xfs: defer agfl frees from inode inactivation
xfs: defer frees from common inode allocation paths
xfs: defer agfl frees from directory op transactions

 fs/xfs/libxfs/xfs_alloc.c  | 82 +++++++++++++++++++++++++++++++++++++++-------
 fs/xfs/libxfs/xfs_alloc.h  |  2 ++
 fs/xfs/libxfs/xfs_defer.c  | 12 +++++++
 fs/xfs/libxfs/xfs_defer.h  |  1 +
 fs/xfs/xfs_inode.c         |  5 +++
 fs/xfs/xfs_symlink.c       |  1 +
 fs/xfs/xfs_trace.h         |  2 ++
 fs/xfs/xfs_trans.c         | 11 +++++--
 fs/xfs/xfs_trans.h         |  1 +
 fs/xfs/xfs_trans_extfree.c | 70 +++++++++++++++++++++++++++++++++++++++
 10 files changed, 173 insertions(+), 14 deletions(-)

Signature · 1 line

2.13.6

Brian Foster

author

2018-04-18 13:31 UTC #1 lore raw

PATCH v2 1/6 xfs: create agfl block free helper function

Refactor the AGFL block free code into a new helper such that it can be invoked from deferred context. No functional changes.

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

Reviewed-by Dave Chinner <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/libxfs/xfs_alloc.c | 37 +++++++++++++++++++++++++++----------
 fs/xfs/libxfs/xfs_alloc.h |  2 ++
 2 files changed, 29 insertions(+), 10 deletions(-)

fs/xfs/libxfs/xfs_alloc.c +27 ~~-10~~

index 4bcc095fe44a..193a5b4909c5 100644
@@ -2060,6 +2060,30 @@ xfs_alloc_space_available(
 	return true;
 }
 
+int
+xfs_free_agfl_block(
+	struct xfs_trans	*tp,
+	xfs_agnumber_t		agno,
+	xfs_agblock_t		agbno,
+	struct xfs_buf		*agbp,
+	struct xfs_owner_info	*oinfo)
+{
+	int			error;
+	struct xfs_buf		*bp;
+
+	error = xfs_free_ag_extent(tp, agbp, agno, agbno, 1, oinfo,
+				   XFS_AG_RESV_AGFL);
+	if (error)
+		return error;
+
+	bp = xfs_btree_get_bufs(tp->t_mountp, tp, agno, agbno, 0);
+	if (!bp)
+		return -EFSCORRUPTED;
+	xfs_trans_binval(tp, bp);
+
+	return 0;
+}
+
 /*
  * Check the agfl fields of the agf for inconsistency or corruption. The purpose
  * is to detect an agfl header padding mismatch between current and early v5

@@ -2247,21 +2271,14 @@ xfs_alloc_fix_freelist(
 	else
 		xfs_rmap_ag_owner(&targs.oinfo, XFS_RMAP_OWN_AG);
 	while (!(flags & XFS_ALLOC_FLAG_NOSHRINK) && pag->pagf_flcount > need) {
-		struct xfs_buf	*bp;
-
 		error = xfs_alloc_get_freelist(tp, agbp, &bno, 0);
 		if (error)
 			goto out_agbp_relse;
-		error = xfs_free_ag_extent(tp, agbp, args->agno, bno, 1,
-					   &targs.oinfo, XFS_AG_RESV_AGFL);
+
+		error = xfs_free_agfl_block(tp, args->agno, bno, agbp,
+					    &targs.oinfo);
 		if (error)
 			goto out_agbp_relse;
-		bp = xfs_btree_get_bufs(mp, tp, args->agno, bno, 0);
-		if (!bp) {
-			error = -EFSCORRUPTED;
-			goto out_agbp_relse;
-		}
-		xfs_trans_binval(tp, bp);
 	}
 
 	targs.tp = tp;

fs/xfs/libxfs/xfs_alloc.h +2 -0

index cbf789ea5a4e..949e21326066 100644
@@ -223,6 +223,8 @@ int xfs_read_agf(struct xfs_mount *mp, struct xfs_trans *tp,
 			xfs_agnumber_t agno, int flags, struct xfs_buf **bpp);
 int xfs_alloc_read_agfl(struct xfs_mount *mp, struct xfs_trans *tp,
 			xfs_agnumber_t agno, struct xfs_buf **bpp);
+int xfs_free_agfl_block(struct xfs_trans *, xfs_agnumber_t, xfs_agblock_t,
+			struct xfs_buf *, struct xfs_owner_info *);
 int xfs_alloc_fix_freelist(struct xfs_alloc_arg *args, int flags);
 int xfs_free_extent_fix_freelist(struct xfs_trans *tp, xfs_agnumber_t agno,
 		struct xfs_buf **agbp);

Signature · 1 line

2.13.6

Darrick J. Wong

Reviewed-by on v2 1/6

2018-05-08 00:35 UTC #2 lore raw

Re: [PATCH v2 1/6] xfs: create agfl block free helper function

Quoted 6 lines · Brian Foster

Refactor the AGFL block free code into a new helper such that it can be invoked from deferred context. No functional changes.
Signed-off-by Brian Foster <[email protected]>
Reviewed-by Dave Chinner <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>

Looks ok,

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 85 lines

---
 fs/xfs/libxfs/xfs_alloc.c | 37 +++++++++++++++++++++++++++----------
 fs/xfs/libxfs/xfs_alloc.h |  2 ++
 2 files changed, 29 insertions(+), 10 deletions(-)

fs/xfs/libxfs/xfs_alloc.c +27 ~~-10~~

index 4bcc095fe44a..193a5b4909c5 100644
@@ -2060,6 +2060,30 @@ xfs_alloc_space_available(
 	return true;
 }
 
+int
+xfs_free_agfl_block(
+	struct xfs_trans	*tp,
+	xfs_agnumber_t		agno,
+	xfs_agblock_t		agbno,
+	struct xfs_buf		*agbp,
+	struct xfs_owner_info	*oinfo)
+{
+	int			error;
+	struct xfs_buf		*bp;
+
+	error = xfs_free_ag_extent(tp, agbp, agno, agbno, 1, oinfo,
+				   XFS_AG_RESV_AGFL);
+	if (error)
+		return error;
+
+	bp = xfs_btree_get_bufs(tp->t_mountp, tp, agno, agbno, 0);
+	if (!bp)
+		return -EFSCORRUPTED;
+	xfs_trans_binval(tp, bp);
+
+	return 0;
+}
+
 /*
  * Check the agfl fields of the agf for inconsistency or corruption. The purpose
  * is to detect an agfl header padding mismatch between current and early v5

@@ -2247,21 +2271,14 @@ xfs_alloc_fix_freelist(
 	else
 		xfs_rmap_ag_owner(&targs.oinfo, XFS_RMAP_OWN_AG);
 	while (!(flags & XFS_ALLOC_FLAG_NOSHRINK) && pag->pagf_flcount > need) {
-		struct xfs_buf	*bp;
-
 		error = xfs_alloc_get_freelist(tp, agbp, &bno, 0);
 		if (error)
 			goto out_agbp_relse;
-		error = xfs_free_ag_extent(tp, agbp, args->agno, bno, 1,
-					   &targs.oinfo, XFS_AG_RESV_AGFL);
+
+		error = xfs_free_agfl_block(tp, args->agno, bno, agbp,
+					    &targs.oinfo);
 		if (error)
 			goto out_agbp_relse;
-		bp = xfs_btree_get_bufs(mp, tp, args->agno, bno, 0);
-		if (!bp) {
-			error = -EFSCORRUPTED;
-			goto out_agbp_relse;
-		}
-		xfs_trans_binval(tp, bp);
 	}
 
 	targs.tp = tp;

fs/xfs/libxfs/xfs_alloc.h +2 -0

index cbf789ea5a4e..949e21326066 100644
@@ -223,6 +223,8 @@ int xfs_read_agf(struct xfs_mount *mp, struct xfs_trans *tp,
 			xfs_agnumber_t agno, int flags, struct xfs_buf **bpp);
 int xfs_alloc_read_agfl(struct xfs_mount *mp, struct xfs_trans *tp,
 			xfs_agnumber_t agno, struct xfs_buf **bpp);
+int xfs_free_agfl_block(struct xfs_trans *, xfs_agnumber_t, xfs_agblock_t,
+			struct xfs_buf *, struct xfs_owner_info *);
 int xfs_alloc_fix_freelist(struct xfs_alloc_arg *args, int flags);
 int xfs_free_extent_fix_freelist(struct xfs_trans *tp, xfs_agnumber_t agno,
 		struct xfs_buf **agbp);

Signature · 6 lines

2.13.6

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

Brian Foster

author

2018-04-18 13:31 UTC #3 lore raw

PATCH v2 2/6 xfs: defer agfl block frees when dfops is available

In reply to Brian Foster

The AGFL fixup code executes before every block allocation/free and rectifies the AGFL based on the current, dynamic allocation requirements of the fs. The AGFL must hold a minimum number of blocks to satisfy a worst case split of the free space btrees caused by the impending allocation operation. The AGFL is also updated to maintain the implicit requirement for a minimum number of free slots to satisfy a worst case join of the free space btrees.

Since the AGFL caches individual blocks, AGFL reduction typically involves multiple, single block frees. We've had reports of transaction overrun problems during certain workloads that boil down to AGFL reduction freeing multiple blocks and consuming more space in the log than was reserved for the transaction.

Since the objective of freeing AGFL blocks is to ensure free AGFL free slots are available for the upcoming allocation, one way to address this problem is to release surplus blocks from the AGFL immediately but defer the free of those blocks (similar to how file-mapped blocks are unmapped from the file in one transaction and freed via a deferred operation) until the transaction is rolled. This turns AGFL reduction into an operation with predictable log reservation consumption.

Add the capability to defer AGFL block frees when a deferred ops list is available to the AGFL fixup code. Add a dfops pointer to the transaction to carry dfops through various contexts to the allocator context. Deferring AGFL frees is conditional behavior based on whether the transaction pointer is populated. The long term objective is to reuse the transaction pointer to clean up all unrelated callchains that pass dfops on the stack along with a transaction and in doing so, consistently defer AGFL blocks from the allocator.

A bit of customization is required to handle deferred completion processing because AGFL blocks are accounted against a per-ag reservation pool and AGFL blocks are not inserted into the extent busy list when freed (they are inserted when used and released back to the AGFL). Reuse the majority of the existing deferred extent free infrastructure and customize it appropriately to handle AGFL blocks.

Note that this patch only adds infrastructure. It does not change behavior because no callers have been updated to pass ->t_agfl_dfops into the allocation code.

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

---
 fs/xfs/libxfs/xfs_alloc.c  | 51 ++++++++++++++++++++++++++++++---
 fs/xfs/libxfs/xfs_defer.h  |  1 +
 fs/xfs/xfs_trace.h         |  2 ++
 fs/xfs/xfs_trans.c         | 11 ++++++--
 fs/xfs/xfs_trans.h         |  1 +
 fs/xfs/xfs_trans_extfree.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 129 insertions(+), 7 deletions(-)

fs/xfs/libxfs/xfs_alloc.c +47 -4

index 193a5b4909c5..350ad203b082 100644
@@ -39,6 +39,9 @@
 #include "xfs_buf_item.h"
 #include "xfs_log.h"
 #include "xfs_ag_resv.h"
+#include "xfs_bmap.h"
+
+extern kmem_zone_t	*xfs_bmap_free_item_zone;
 
 struct workqueue_struct *xfs_alloc_wq;

@@ -2172,6 +2175,40 @@ xfs_agfl_reset(
 }
 
 /*
+ * Defer an AGFL block free. This is effectively equivalent to
+ * xfs_bmap_add_free() with some special handling particular to AGFL blocks.
+ *
+ * Deferring AGFL frees helps prevent log reservation overruns due to too many
+ * allocation operations in a transaction. AGFL frees are prone to this problem
+ * because for one they are always freed one at a time. Further, an immediate
+ * AGFL block free can cause a btree join and require another block free before
+ * the real allocation can proceed. Deferring the free disconnects freeing up
+ * the AGFL slot from freeing the block.
+ */
+STATIC void
+xfs_defer_agfl_block(
+	struct xfs_mount		*mp,
+	struct xfs_defer_ops		*dfops,
+	xfs_agnumber_t			agno,
+	xfs_fsblock_t			agbno,
+	struct xfs_owner_info		*oinfo)
+{
+	struct xfs_extent_free_item	*new;		/* new element */
+
+	ASSERT(xfs_bmap_free_item_zone != NULL);
+	ASSERT(oinfo != NULL);
+
+	new = kmem_zone_alloc(xfs_bmap_free_item_zone, KM_SLEEP);
+	new->xefi_startblock = XFS_AGB_TO_FSB(mp, agno, agbno);
+	new->xefi_blockcount = 1;
+	new->xefi_oinfo = *oinfo;
+
+	trace_xfs_agfl_free_defer(mp, agno, 0, agbno, 1);
+
+	xfs_defer_add(dfops, XFS_DEFER_OPS_TYPE_AGFL_FREE, &new->xefi_list);
+}
+
+/*
  * Decide whether to use this allocation group for this allocation.
  * If so, fix up the btree freelist's size.
  */

@@ -2275,10 +2312,16 @@ xfs_alloc_fix_freelist(
 		if (error)
 			goto out_agbp_relse;
 
-		error = xfs_free_agfl_block(tp, args->agno, bno, agbp,
-					    &targs.oinfo);
-		if (error)
-			goto out_agbp_relse;
+		/* defer agfl frees if dfops is provided */
+		if (tp->t_agfl_dfops) {
+			xfs_defer_agfl_block(mp, tp->t_agfl_dfops, args->agno,
+					     bno, &targs.oinfo);
+		} else {
+			error = xfs_free_agfl_block(tp, args->agno, bno, agbp,
+						    &targs.oinfo);
+			if (error)
+				goto out_agbp_relse;
+		}
 	}
 
 	targs.tp = tp;

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

index 045beacdd37d..e70725ba1f5f 100644
@@ -55,6 +55,7 @@ enum xfs_defer_ops_type {
 	XFS_DEFER_OPS_TYPE_REFCOUNT,
 	XFS_DEFER_OPS_TYPE_RMAP,
 	XFS_DEFER_OPS_TYPE_FREE,
+	XFS_DEFER_OPS_TYPE_AGFL_FREE,
 	XFS_DEFER_OPS_TYPE_MAX,
 };

fs/xfs/xfs_trace.h +2 -0

index 8955254b900e..00e6aaea6565 100644
@@ -2433,6 +2433,8 @@ DEFINE_DEFER_PENDING_EVENT(xfs_defer_pending_abort);
 #define DEFINE_BMAP_FREE_DEFERRED_EVENT DEFINE_PHYS_EXTENT_DEFERRED_EVENT
 DEFINE_BMAP_FREE_DEFERRED_EVENT(xfs_bmap_free_defer);
 DEFINE_BMAP_FREE_DEFERRED_EVENT(xfs_bmap_free_deferred);
+DEFINE_BMAP_FREE_DEFERRED_EVENT(xfs_agfl_free_defer);
+DEFINE_BMAP_FREE_DEFERRED_EVENT(xfs_agfl_free_deferred);
 
 /* rmap tracepoints */
 DECLARE_EVENT_CLASS(xfs_rmap_class,

fs/xfs/xfs_trans.c +8 -3

index d6d8f9d129a7..06adb1a3e31f 100644
@@ -31,6 +31,7 @@
 #include "xfs_log.h"
 #include "xfs_trace.h"
 #include "xfs_error.h"
+#include "xfs_defer.h"
 
 kmem_zone_t	*xfs_trans_zone;
 kmem_zone_t	*xfs_log_item_desc_zone;

@@ -94,11 +95,11 @@ xfs_trans_free(
  * blocks.  Locks and log items, however, are no inherited.  They must
  * be added to the new transaction explicitly.
  */
-STATIC xfs_trans_t *
+STATIC struct xfs_trans *
 xfs_trans_dup(
-	xfs_trans_t	*tp)
+	struct xfs_trans	*tp)
 {
-	xfs_trans_t	*ntp;
+	struct xfs_trans	*ntp;
 
 	ntp = kmem_zone_zalloc(xfs_trans_zone, KM_SLEEP);

@@ -127,6 +128,7 @@ 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_agfl_dfops = tp->t_agfl_dfops;
 
 	xfs_trans_dup_dqinfo(tp, ntp);

@@ -936,6 +938,9 @@ __xfs_trans_commit(
 	int			error = 0;
 	int			sync = tp->t_flags & XFS_TRANS_SYNC;
 
+	ASSERT(!tp->t_agfl_dfops ||
+	       !xfs_defer_has_unfinished_work(tp->t_agfl_dfops) || regrant);
+
 	/*
 	 * If there is nothing to be logged by the transaction,
 	 * then unlock all of the items associated with the

fs/xfs/xfs_trans.h +1 -0

index 9d542dfe0052..834388c2c9de 100644
@@ -111,6 +111,7 @@ typedef struct xfs_trans {
 	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_agfl_dfops;	/* optional agfl fixup dfops */
 	unsigned int		t_flags;	/* misc flags */
 	int64_t			t_icount_delta;	/* superblock icount change */
 	int64_t			t_ifree_delta;	/* superblock ifree change */

fs/xfs/xfs_trans_extfree.c +70 -0

79 lines not on this page · raw message

Signature · 1 line

2.13.6

Darrick J. Wong

Reviewed-by on v2 2/6

2018-05-08 01:10 UTC #4 lore raw

Re: [PATCH v2 2/6] xfs: defer agfl block frees when dfops is available

Quoted 206 lines · Brian Foster

The AGFL fixup code executes before every block allocation/free and rectifies the AGFL based on the current, dynamic allocation requirements of the fs. The AGFL must hold a minimum number of blocks to satisfy a worst case split of the free space btrees caused by the impending allocation operation. The AGFL is also updated to maintain the implicit requirement for a minimum number of free slots to satisfy a worst case join of the free space btrees.
Since the AGFL caches individual blocks, AGFL reduction typically involves multiple, single block frees. We've had reports of transaction overrun problems during certain workloads that boil down to AGFL reduction freeing multiple blocks and consuming more space in the log than was reserved for the transaction.
Since the objective of freeing AGFL blocks is to ensure free AGFL free slots are available for the upcoming allocation, one way to address this problem is to release surplus blocks from the AGFL immediately but defer the free of those blocks (similar to how file-mapped blocks are unmapped from the file in one transaction and freed via a deferred operation) until the transaction is rolled. This turns AGFL reduction into an operation with predictable log reservation consumption.
Add the capability to defer AGFL block frees when a deferred ops list is available to the AGFL fixup code. Add a dfops pointer to the transaction to carry dfops through various contexts to the allocator context. Deferring AGFL frees is conditional behavior based on whether the transaction pointer is populated. The long term objective is to reuse the transaction pointer to clean up all unrelated callchains that pass dfops on the stack along with a transaction and in doing so, consistently defer AGFL blocks from the allocator.
A bit of customization is required to handle deferred completion processing because AGFL blocks are accounted against a per-ag reservation pool and AGFL blocks are not inserted into the extent busy list when freed (they are inserted when used and released back to the AGFL). Reuse the majority of the existing deferred extent free infrastructure and customize it appropriately to handle AGFL blocks.
Note that this patch only adds infrastructure. It does not change behavior because no callers have been updated to pass ->t_agfl_dfops into the allocation code.
Signed-off-by Brian Foster <[email protected]>
---
 fs/xfs/libxfs/xfs_alloc.c  | 51 ++++++++++++++++++++++++++++++---
 fs/xfs/libxfs/xfs_defer.h  |  1 +
 fs/xfs/xfs_trace.h         |  2 ++
 fs/xfs/xfs_trans.c         | 11 ++++++--
 fs/xfs/xfs_trans.h         |  1 +
 fs/xfs/xfs_trans_extfree.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 129 insertions(+), 7 deletions(-)
fs/xfs/libxfs/xfs_alloc.c +47 -4
index 193a5b4909c5..350ad203b082 100644
@@ -39,6 +39,9 @@
 #include "xfs_buf_item.h"
 #include "xfs_log.h"
 #include "xfs_ag_resv.h"
+#include "xfs_bmap.h"
+
+extern kmem_zone_t	*xfs_bmap_free_item_zone;
 
 struct workqueue_struct *xfs_alloc_wq;
 
@@ -2172,6 +2175,40 @@ xfs_agfl_reset(
 }
 
 /*
+ * Defer an AGFL block free. This is effectively equivalent to
+ * xfs_bmap_add_free() with some special handling particular to AGFL blocks.
+ *
+ * Deferring AGFL frees helps prevent log reservation overruns due to too many
+ * allocation operations in a transaction. AGFL frees are prone to this problem
+ * because for one they are always freed one at a time. Further, an immediate
+ * AGFL block free can cause a btree join and require another block free before
+ * the real allocation can proceed. Deferring the free disconnects freeing up
+ * the AGFL slot from freeing the block.
+ */
+STATIC void
+xfs_defer_agfl_block(
+	struct xfs_mount		*mp,
+	struct xfs_defer_ops		*dfops,
+	xfs_agnumber_t			agno,
+	xfs_fsblock_t			agbno,
+	struct xfs_owner_info		*oinfo)
+{
+	struct xfs_extent_free_item	*new;		/* new element */
+
+	ASSERT(xfs_bmap_free_item_zone != NULL);
+	ASSERT(oinfo != NULL);
+
+	new = kmem_zone_alloc(xfs_bmap_free_item_zone, KM_SLEEP);
+	new->xefi_startblock = XFS_AGB_TO_FSB(mp, agno, agbno);
+	new->xefi_blockcount = 1;
+	new->xefi_oinfo = *oinfo;
+
+	trace_xfs_agfl_free_defer(mp, agno, 0, agbno, 1);
+
+	xfs_defer_add(dfops, XFS_DEFER_OPS_TYPE_AGFL_FREE, &new->xefi_list);
+}
+
+/*
  * Decide whether to use this allocation group for this allocation.
  * If so, fix up the btree freelist's size.
  */
@@ -2275,10 +2312,16 @@ xfs_alloc_fix_freelist(
 		if (error)
 			goto out_agbp_relse;
 
-		error = xfs_free_agfl_block(tp, args->agno, bno, agbp,
-					    &targs.oinfo);
-		if (error)
-			goto out_agbp_relse;
+		/* defer agfl frees if dfops is provided */
+		if (tp->t_agfl_dfops) {
+			xfs_defer_agfl_block(mp, tp->t_agfl_dfops, args->agno,
+					     bno, &targs.oinfo);
+		} else {
+			error = xfs_free_agfl_block(tp, args->agno, bno, agbp,
+						    &targs.oinfo);
+			if (error)
+				goto out_agbp_relse;
+		}
 	}
 
 	targs.tp = tp;
fs/xfs/libxfs/xfs_defer.h +1 -0
index 045beacdd37d..e70725ba1f5f 100644
@@ -55,6 +55,7 @@ enum xfs_defer_ops_type {
 	XFS_DEFER_OPS_TYPE_REFCOUNT,
 	XFS_DEFER_OPS_TYPE_RMAP,
 	XFS_DEFER_OPS_TYPE_FREE,
+	XFS_DEFER_OPS_TYPE_AGFL_FREE,
 	XFS_DEFER_OPS_TYPE_MAX,
 };
 
fs/xfs/xfs_trace.h +2 -0
index 8955254b900e..00e6aaea6565 100644
@@ -2433,6 +2433,8 @@ DEFINE_DEFER_PENDING_EVENT(xfs_defer_pending_abort);
 #define DEFINE_BMAP_FREE_DEFERRED_EVENT DEFINE_PHYS_EXTENT_DEFERRED_EVENT
 DEFINE_BMAP_FREE_DEFERRED_EVENT(xfs_bmap_free_defer);
 DEFINE_BMAP_FREE_DEFERRED_EVENT(xfs_bmap_free_deferred);
+DEFINE_BMAP_FREE_DEFERRED_EVENT(xfs_agfl_free_defer);
+DEFINE_BMAP_FREE_DEFERRED_EVENT(xfs_agfl_free_deferred);
 
 /* rmap tracepoints */
 DECLARE_EVENT_CLASS(xfs_rmap_class,
fs/xfs/xfs_trans.c +8 -3
index d6d8f9d129a7..06adb1a3e31f 100644
@@ -31,6 +31,7 @@
 #include "xfs_log.h"
 #include "xfs_trace.h"
 #include "xfs_error.h"
+#include "xfs_defer.h"
 
 kmem_zone_t	*xfs_trans_zone;
 kmem_zone_t	*xfs_log_item_desc_zone;
@@ -94,11 +95,11 @@ xfs_trans_free(
  * blocks.  Locks and log items, however, are no inherited.  They must
  * be added to the new transaction explicitly.
  */
-STATIC xfs_trans_t *
+STATIC struct xfs_trans *
 xfs_trans_dup(
-	xfs_trans_t	*tp)
+	struct xfs_trans	*tp)
 {
-	xfs_trans_t	*ntp;
+	struct xfs_trans	*ntp;
 
 	ntp = kmem_zone_zalloc(xfs_trans_zone, KM_SLEEP);
 
@@ -127,6 +128,7 @@ 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_agfl_dfops = tp->t_agfl_dfops;
 
 	xfs_trans_dup_dqinfo(tp, ntp);
 
@@ -936,6 +938,9 @@ __xfs_trans_commit(
 	int			error = 0;
 	int			sync = tp->t_flags & XFS_TRANS_SYNC;
 
+	ASSERT(!tp->t_agfl_dfops ||
+	       !xfs_defer_has_unfinished_work(tp->t_agfl_dfops) || regrant);
+
 	/*
 	 * If there is nothing to be logged by the transaction,
 	 * then unlock all of the items associated with the
fs/xfs/xfs_trans.h +0 -0
index 9d542dfe0052..834388c2c9de 100644

fs/xfs/xfs_trans.h typedef struct xfs_trans { · line 114

Quoted 4 lines

fs/xfs/xfs_trans.h +1 -0

@@ -111,6 +111,7 @@ typedef struct xfs_trans {
 	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_agfl_dfops;	/* optional agfl fixup dfops */

This more or less looks fine to me, with ^^^^ this one (incoherent) quibble:

A thread can only have one transaction and one dfops in play at a time, so could we just call this t_dfops and allow anyone to access it who wants to add their own deferred operations? Then we can stop passing both tp and dfops all over the stack. Can you think of a situation where we would want access to t_dfops for deferred agfl frees but not for any other deferred ops?

Separate cleanup, of course, and sorry if this has already been asked.

Other than that this looks ok enough to test,

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 94 lines

fs/xfs/xfs_trans.h +0 -0

 	unsigned int		t_flags;	/* misc flags */
 	int64_t			t_icount_delta;	/* superblock icount change */
 	int64_t			t_ifree_delta;	/* superblock ifree change */

fs/xfs/xfs_trans_extfree.c +70 -0

index ab438647592a..f5620796ae25 100644
@@ -231,9 +231,79 @@ static const struct xfs_defer_op_type xfs_extent_free_defer_type = {
 	.cancel_item	= xfs_extent_free_cancel_item,
 };
 
+/*
+ * AGFL blocks are accounted differently in the reserve pools and are not
+ * inserted into the busy extent list.
+ */
+STATIC int
+xfs_agfl_free_finish_item(
+	struct xfs_trans		*tp,
+	struct xfs_defer_ops		*dop,
+	struct list_head		*item,
+	void				*done_item,
+	void				**state)
+{
+	struct xfs_mount		*mp = tp->t_mountp;
+	struct xfs_efd_log_item		*efdp = done_item;
+	struct xfs_extent_free_item	*free;
+	struct xfs_extent		*extp;
+	struct xfs_buf			*agbp;
+	int				error;
+	xfs_agnumber_t			agno;
+	xfs_agblock_t			agbno;
+	uint				next_extent;
+
+	free = container_of(item, struct xfs_extent_free_item, xefi_list);
+	ASSERT(free->xefi_blockcount == 1);
+	agno = XFS_FSB_TO_AGNO(mp, free->xefi_startblock);
+	agbno = XFS_FSB_TO_AGBNO(mp, free->xefi_startblock);
+
+	trace_xfs_agfl_free_deferred(mp, agno, 0, agbno, free->xefi_blockcount);
+
+	error = xfs_alloc_read_agf(mp, tp, agno, 0, &agbp);
+	if (!error)
+		error = xfs_free_agfl_block(tp, agno, agbno, agbp,
+					    &free->xefi_oinfo);
+
+	/*
+	 * Mark the transaction dirty, even on error. This ensures the
+	 * transaction is aborted, which:
+	 *
+	 * 1.) releases the EFI and frees the EFD
+	 * 2.) shuts down the filesystem
+	 */
+	tp->t_flags |= XFS_TRANS_DIRTY;
+	efdp->efd_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+
+	next_extent = efdp->efd_next_extent;
+	ASSERT(next_extent < efdp->efd_format.efd_nextents);
+	extp = &(efdp->efd_format.efd_extents[next_extent]);
+	extp->ext_start = free->xefi_startblock;
+	extp->ext_len = free->xefi_blockcount;
+	efdp->efd_next_extent++;
+
+	kmem_free(free);
+	return error;
+}
+
+
+/* sub-type with special handling for AGFL deferred frees */
+static const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
+	.type		= XFS_DEFER_OPS_TYPE_AGFL_FREE,
+	.max_items	= XFS_EFI_MAX_FAST_EXTENTS,
+	.diff_items	= xfs_extent_free_diff_items,
+	.create_intent	= xfs_extent_free_create_intent,
+	.abort_intent	= xfs_extent_free_abort_intent,
+	.log_item	= xfs_extent_free_log_item,
+	.create_done	= xfs_extent_free_create_done,
+	.finish_item	= xfs_agfl_free_finish_item,
+	.cancel_item	= xfs_extent_free_cancel_item,
+};
+
 /* Register the deferred op type. */
 void
 xfs_extent_free_init_defer_op(void)
 {
 	xfs_defer_init_op_type(&xfs_extent_free_defer_type);
+	xfs_defer_init_op_type(&xfs_agfl_free_defer_type);
 }

Signature · 6 lines

2.13.6

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

Brian Foster

author

2018-05-08 12:31 UTC #5 lore raw

Re: [PATCH v2 2/6] xfs: defer agfl block frees when dfops is available

Quoted 1 line · Darrick J. Wong

On Wed, Apr 18, 2018 at 09:31:15AM -0400, Brian Foster wrote:

...

Quoted 11 lines

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

---
 fs/xfs/libxfs/xfs_alloc.c  | 51 ++++++++++++++++++++++++++++++---
 fs/xfs/libxfs/xfs_defer.h  |  1 +
 fs/xfs/xfs_trace.h         |  2 ++
 fs/xfs/xfs_trans.c         | 11 ++++++--
 fs/xfs/xfs_trans.h         |  1 +
 fs/xfs/xfs_trans_extfree.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 129 insertions(+), 7 deletions(-)

...

Quoted 12 lines

fs/xfs/xfs_trans.h typedef struct xfs_trans { · line 114

Quoted 9 lines

fs/xfs/xfs_trans.h +1 -0

index 9d542dfe0052..834388c2c9de 100644
@@ -111,6 +111,7 @@ typedef struct xfs_trans {
 	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_agfl_dfops;	/* optional agfl fixup dfops */

This more or less looks fine to me, with ^^^^ this one (incoherent) quibble:

I wouldn't call it incoherent...

Quoted 7 lines

A thread can only have one transaction and one dfops in play at a time, so could we just call this t_dfops and allow anyone to access it who wants to add their own deferred operations? Then we can stop passing both tp and dfops all over the stack. Can you think of a situation where we would want access to t_dfops for deferred agfl frees but not for any other deferred ops?

... because that is precisely the secondary goal of this approach. :) Dave suggested this approach in a previous version and the last post actually used ->t_dfops rather than ->t_agfl_dfops and included some associated dfops parameter elimination cleanup I did along the way.

Christoph objected to the partial refactoring, however, so I'm trying to do this in two parts where the first is primarily focused on fixing the problems resolved by deferred agfl frees. The challenge is that I don't want to manually plumb dfops through new codepaths to control deferred agfl frees only to immediately turn around and clean that code out, and I certainly don't want to refactor all of our dfops code just to enable deferred agfl frees because it creates an unnecessary dependency for backports. As a result, I've stripped out as much refactoring as possible to try and make it clear that this is a bug fix series that happens to create a new xfs_trans field that will open wider cleanups in a follow on series.

IOW, I have a series in progress where patch 1 renames ->t_agfl_dfops back to ->t_dfops to essentially open it for general use and then starts to incorporate the broader cleanups like removing dfops from callstacks, alloc/bmap data structures, etc.

BTW, something I'm curious of your (and Dave, Christoph?) opinion on... another thing that was dropped (permanently) from the previous series was an update to xfs_defer_init() to do the ->t_dfops = &dfops assignment that is otherwise open-coded throughout here. While I don't have a strong preference on that for this series, the more broader cleanup I do the less I like the open-coded approach because it creates a tx setup requirement that's easy to miss (case in point: a function signature change is an easy way to verify all dfops have been associated with transactions). Instead, I'd prefer code that looks something like the following as the end result of the follow-on cleanup series:

	struct xfs_trans	*tp;
	struct xfs_defer_ops	dfops;

	xfs_trans_alloc(... &tp);
	xfs_defer_init(&dfops, &first_block, tp);

	...

	xfs_defer_finish(&tp);
	xfs_trans_commit(tp);
	...

Note that once we require association of dfops with tp, we don't have to pass dfops around to anywhere the tp isn't already needed, including xfs_defer_finish(). This also opens the possibility of doing things like replacing on-stack dfops with something that's just embedded in xfs_trans itself (optionally pushing down the init/finish calls into the trans infrastructure), but that would require changing how dfops are transferred to rolled transactions and whatnot. I haven't really got far enough to give that proper consideration, but that may be a possible 3rd step cleanup if there's value.

Anyways, I'd really like to avoid having to switch back and forth between the xfs_defer_init() tweak and open-coded assignment if possible. Any thoughts/preferences on either approach?

Brian

Quoted 106 lines

Separate cleanup, of course, and sorry if this has already been asked.

Other than that this looks ok enough to test,

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 94 lines

fs/xfs/xfs_trans.h +0 -0

 	unsigned int		t_flags;	/* misc flags */
 	int64_t			t_icount_delta;	/* superblock icount change */
 	int64_t			t_ifree_delta;	/* superblock ifree change */

fs/xfs/xfs_trans_extfree.c +70 -0

index ab438647592a..f5620796ae25 100644
@@ -231,9 +231,79 @@ static const struct xfs_defer_op_type xfs_extent_free_defer_type = {
 	.cancel_item	= xfs_extent_free_cancel_item,
 };
 
+/*
+ * AGFL blocks are accounted differently in the reserve pools and are not
+ * inserted into the busy extent list.
+ */
+STATIC int
+xfs_agfl_free_finish_item(
+	struct xfs_trans		*tp,
+	struct xfs_defer_ops		*dop,
+	struct list_head		*item,
+	void				*done_item,
+	void				**state)
+{
+	struct xfs_mount		*mp = tp->t_mountp;
+	struct xfs_efd_log_item		*efdp = done_item;
+	struct xfs_extent_free_item	*free;
+	struct xfs_extent		*extp;
+	struct xfs_buf			*agbp;
+	int				error;
+	xfs_agnumber_t			agno;
+	xfs_agblock_t			agbno;
+	uint				next_extent;
+
+	free = container_of(item, struct xfs_extent_free_item, xefi_list);
+	ASSERT(free->xefi_blockcount == 1);
+	agno = XFS_FSB_TO_AGNO(mp, free->xefi_startblock);
+	agbno = XFS_FSB_TO_AGBNO(mp, free->xefi_startblock);
+
+	trace_xfs_agfl_free_deferred(mp, agno, 0, agbno, free->xefi_blockcount);
+
+	error = xfs_alloc_read_agf(mp, tp, agno, 0, &agbp);
+	if (!error)
+		error = xfs_free_agfl_block(tp, agno, agbno, agbp,
+					    &free->xefi_oinfo);
+
+	/*
+	 * Mark the transaction dirty, even on error. This ensures the
+	 * transaction is aborted, which:
+	 *
+	 * 1.) releases the EFI and frees the EFD
+	 * 2.) shuts down the filesystem
+	 */
+	tp->t_flags |= XFS_TRANS_DIRTY;
+	efdp->efd_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+
+	next_extent = efdp->efd_next_extent;
+	ASSERT(next_extent < efdp->efd_format.efd_nextents);
+	extp = &(efdp->efd_format.efd_extents[next_extent]);
+	extp->ext_start = free->xefi_startblock;
+	extp->ext_len = free->xefi_blockcount;
+	efdp->efd_next_extent++;
+
+	kmem_free(free);
+	return error;
+}
+
+
+/* sub-type with special handling for AGFL deferred frees */
+static const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
+	.type		= XFS_DEFER_OPS_TYPE_AGFL_FREE,
+	.max_items	= XFS_EFI_MAX_FAST_EXTENTS,
+	.diff_items	= xfs_extent_free_diff_items,
+	.create_intent	= xfs_extent_free_create_intent,
+	.abort_intent	= xfs_extent_free_abort_intent,
+	.log_item	= xfs_extent_free_log_item,
+	.create_done	= xfs_extent_free_create_done,
+	.finish_item	= xfs_agfl_free_finish_item,
+	.cancel_item	= xfs_extent_free_cancel_item,
+};
+
 /* Register the deferred op type. */
 void
 xfs_extent_free_init_defer_op(void)
 {
 	xfs_defer_init_op_type(&xfs_extent_free_defer_type);
+	xfs_defer_init_op_type(&xfs_agfl_free_defer_type);
 }

Signature · 6 lines

2.13.6

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

Darrick J. Wong

2018-05-08 18:04 UTC #6 lore raw

Re: [PATCH v2 2/6] xfs: defer agfl block frees when dfops is available

Quoted 55 lines · Brian Foster

Quoted 1 line · Darrick J. Wong
On Wed, Apr 18, 2018 at 09:31:15AM -0400, Brian Foster wrote:
...
Quoted 11 lines
Quoted 11 lines
Signed-off-by Brian Foster <[email protected]>
---
 fs/xfs/libxfs/xfs_alloc.c  | 51 ++++++++++++++++++++++++++++++---
 fs/xfs/libxfs/xfs_defer.h  |  1 +
 fs/xfs/xfs_trace.h         |  2 ++
 fs/xfs/xfs_trans.c         | 11 ++++++--
 fs/xfs/xfs_trans.h         |  1 +
 fs/xfs/xfs_trans_extfree.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 129 insertions(+), 7 deletions(-)
...
Quoted 12 lines
fs/xfs/xfs_trans.h typedef struct xfs_trans { · line 114
Quoted 9 lines
fs/xfs/xfs_trans.h +1 -0
index 9d542dfe0052..834388c2c9de 100644
@@ -111,6 +111,7 @@ typedef struct xfs_trans {
 	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_agfl_dfops;	/* optional agfl fixup dfops */
This more or less looks fine to me, with ^^^^ this one (incoherent) quibble:
I wouldn't call it incoherent...
Quoted 7 lines
A thread can only have one transaction and one dfops in play at a time, so could we just call this t_dfops and allow anyone to access it who wants to add their own deferred operations? Then we can stop passing both tp and dfops all over the stack. Can you think of a situation where we would want access to t_dfops for deferred agfl frees but not for any other deferred ops?
... because that is precisely the secondary goal of this approach. :) Dave suggested this approach in a previous version and the last post actually used ->t_dfops rather than ->t_agfl_dfops and included some associated dfops parameter elimination cleanup I did along the way.
Christoph objected to the partial refactoring, however, so I'm trying to do this in two parts where the first is primarily focused on fixing the problems resolved by deferred agfl frees. The challenge is that I don't want to manually plumb dfops through new codepaths to control deferred agfl frees only to immediately turn around and clean that code out, and I certainly don't want to refactor all of our dfops code just to enable deferred agfl frees because it creates an unnecessary dependency for backports. As a result, I've stripped out as much refactoring as possible to try and make it clear that this is a bug fix series that happens to create a new xfs_trans field that will open wider cleanups in a follow on series.

Quoted 4 lines

IOW, I have a series in progress where patch 1 renames ->t_agfl_dfops back to ->t_dfops to essentially open it for general use and then starts to incorporate the broader cleanups like removing dfops from callstacks, alloc/bmap data structures, etc.

Seems reasonable. I've occasionally thought that passing around (tp, dfops, firstblock) is kinda bulky and silly.

Quoted 23 lines

BTW, something I'm curious of your (and Dave, Christoph?) opinion on... another thing that was dropped (permanently) from the previous series was an update to xfs_defer_init() to do the ->t_dfops = &dfops assignment that is otherwise open-coded throughout here. While I don't have a strong preference on that for this series, the more broader cleanup I do the less I like the open-coded approach because it creates a tx setup requirement that's easy to miss (case in point: a function signature change is an easy way to verify all dfops have been associated with transactions). Instead, I'd prefer code that looks something like the following as the end result of the follow-on cleanup series:
	struct xfs_trans	*tp;
	struct xfs_defer_ops	dfops;

	xfs_trans_alloc(... &tp);
	xfs_defer_init(&dfops, &first_block, tp);

	...

	xfs_defer_finish(&tp);
	xfs_trans_commit(tp);
	...

Quoted 9 lines

Note that once we require association of dfops with tp, we don't have to pass dfops around to anywhere the tp isn't already needed, including xfs_defer_finish(). This also opens the possibility of doing things like replacing on-stack dfops with something that's just embedded in xfs_trans itself (optionally pushing down the init/finish calls into the trans infrastructure), but that would require changing how dfops are transferred to rolled transactions and whatnot. I haven't really got far enough to give that proper consideration, but that may be a possible 3rd step cleanup if there's value.

Not sure. There are places where it seems to make sense to be able to _defer_finish but still have a transaction around to continue making changes afterwards. Or at least, the attr and quota code seems to do that. :P

Also, there's one funny case where we can have a dfops but no transaction, which is xlog_recover_process_intents -> xlog_finish_defer_ops. We create a dfops to collect new intents created as part of replaying unfinished intents that were in the dirty log, but since the intent recovery functions create and commit their own transactions, we don't create the transaction that goes with the dfops until we're done replaying old intents and ready to finish the new intents.

--D

Quoted 115 lines

Anyways, I'd really like to avoid having to switch back and forth between the xfs_defer_init() tweak and open-coded assignment if possible. Any thoughts/preferences on either approach? Brian

Quoted 98 lines

Separate cleanup, of course, and sorry if this has already been asked.

Other than that this looks ok enough to test,

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 90 lines

fs/xfs/xfs_trans.h +0 -0

 	unsigned int		t_flags;	/* misc flags */
 	int64_t			t_icount_delta;	/* superblock icount change */
 	int64_t			t_ifree_delta;	/* superblock ifree change */

fs/xfs/xfs_trans_extfree.c +70 -0

index ab438647592a..f5620796ae25 100644
@@ -231,9 +231,79 @@ static const struct xfs_defer_op_type xfs_extent_free_defer_type = {
 	.cancel_item	= xfs_extent_free_cancel_item,
 };
 
+/*
+ * AGFL blocks are accounted differently in the reserve pools and are not
+ * inserted into the busy extent list.
+ */
+STATIC int
+xfs_agfl_free_finish_item(
+	struct xfs_trans		*tp,
+	struct xfs_defer_ops		*dop,
+	struct list_head		*item,
+	void				*done_item,
+	void				**state)
+{
+	struct xfs_mount		*mp = tp->t_mountp;
+	struct xfs_efd_log_item		*efdp = done_item;
+	struct xfs_extent_free_item	*free;
+	struct xfs_extent		*extp;
+	struct xfs_buf			*agbp;
+	int				error;
+	xfs_agnumber_t			agno;
+	xfs_agblock_t			agbno;
+	uint				next_extent;
+
+	free = container_of(item, struct xfs_extent_free_item, xefi_list);
+	ASSERT(free->xefi_blockcount == 1);
+	agno = XFS_FSB_TO_AGNO(mp, free->xefi_startblock);
+	agbno = XFS_FSB_TO_AGBNO(mp, free->xefi_startblock);
+
+	trace_xfs_agfl_free_deferred(mp, agno, 0, agbno, free->xefi_blockcount);
+
+	error = xfs_alloc_read_agf(mp, tp, agno, 0, &agbp);
+	if (!error)
+		error = xfs_free_agfl_block(tp, agno, agbno, agbp,
+					    &free->xefi_oinfo);
+
+	/*
+	 * Mark the transaction dirty, even on error. This ensures the
+	 * transaction is aborted, which:
+	 *
+	 * 1.) releases the EFI and frees the EFD
+	 * 2.) shuts down the filesystem
+	 */
+	tp->t_flags |= XFS_TRANS_DIRTY;
+	efdp->efd_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+
+	next_extent = efdp->efd_next_extent;
+	ASSERT(next_extent < efdp->efd_format.efd_nextents);
+	extp = &(efdp->efd_format.efd_extents[next_extent]);
+	extp->ext_start = free->xefi_startblock;
+	extp->ext_len = free->xefi_blockcount;
+	efdp->efd_next_extent++;
+
+	kmem_free(free);
+	return error;
+}
+
+
+/* sub-type with special handling for AGFL deferred frees */
+static const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
+	.type		= XFS_DEFER_OPS_TYPE_AGFL_FREE,
+	.max_items	= XFS_EFI_MAX_FAST_EXTENTS,
+	.diff_items	= xfs_extent_free_diff_items,
+	.create_intent	= xfs_extent_free_create_intent,
+	.abort_intent	= xfs_extent_free_abort_intent,
+	.log_item	= xfs_extent_free_log_item,
+	.create_done	= xfs_extent_free_create_done,
+	.finish_item	= xfs_agfl_free_finish_item,
+	.cancel_item	= xfs_extent_free_cancel_item,
+};
+
 /* Register the deferred op type. */
 void
 xfs_extent_free_init_defer_op(void)
 {
 	xfs_defer_init_op_type(&xfs_extent_free_defer_type);
+	xfs_defer_init_op_type(&xfs_agfl_free_defer_type);
 }

Signature · 1 line

2.13.6

Quoted 8 lines

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

Brian Foster

author

2018-05-09 11:19 UTC #7 lore raw

Re: [PATCH v2 2/6] xfs: defer agfl block frees when dfops is available

Quoted 97 lines · Darrick J. Wong

Quoted 55 lines · Brian Foster
Quoted 1 line · Darrick J. Wong
On Wed, Apr 18, 2018 at 09:31:15AM -0400, Brian Foster wrote:
...
Quoted 11 lines
Quoted 11 lines
Signed-off-by Brian Foster <[email protected]>
---
 fs/xfs/libxfs/xfs_alloc.c  | 51 ++++++++++++++++++++++++++++++---
 fs/xfs/libxfs/xfs_defer.h  |  1 +
 fs/xfs/xfs_trace.h         |  2 ++
 fs/xfs/xfs_trans.c         | 11 ++++++--
 fs/xfs/xfs_trans.h         |  1 +
 fs/xfs/xfs_trans_extfree.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 129 insertions(+), 7 deletions(-)
...
Quoted 12 lines
fs/xfs/xfs_trans.h typedef struct xfs_trans { · line 114
Quoted 9 lines
fs/xfs/xfs_trans.h +1 -0
index 9d542dfe0052..834388c2c9de 100644
@@ -111,6 +111,7 @@ typedef struct xfs_trans {
 	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_agfl_dfops;	/* optional agfl fixup dfops */
This more or less looks fine to me, with ^^^^ this one (incoherent) quibble:
I wouldn't call it incoherent...
Quoted 7 lines
A thread can only have one transaction and one dfops in play at a time, so could we just call this t_dfops and allow anyone to access it who wants to add their own deferred operations? Then we can stop passing both tp and dfops all over the stack. Can you think of a situation where we would want access to t_dfops for deferred agfl frees but not for any other deferred ops?
... because that is precisely the secondary goal of this approach. :) Dave suggested this approach in a previous version and the last post actually used ->t_dfops rather than ->t_agfl_dfops and included some associated dfops parameter elimination cleanup I did along the way.
Christoph objected to the partial refactoring, however, so I'm trying to do this in two parts where the first is primarily focused on fixing the problems resolved by deferred agfl frees. The challenge is that I don't want to manually plumb dfops through new codepaths to control deferred agfl frees only to immediately turn around and clean that code out, and I certainly don't want to refactor all of our dfops code just to enable deferred agfl frees because it creates an unnecessary dependency for backports. As a result, I've stripped out as much refactoring as possible to try and make it clear that this is a bug fix series that happens to create a new xfs_trans field that will open wider cleanups in a follow on series.
Quoted 4 lines
IOW, I have a series in progress where patch 1 renames ->t_agfl_dfops back to ->t_dfops to essentially open it for general use and then starts to incorporate the broader cleanups like removing dfops from callstacks, alloc/bmap data structures, etc.
Seems reasonable. I've occasionally thought that passing around (tp, dfops, firstblock) is kinda bulky and silly.
Quoted 23 lines
BTW, something I'm curious of your (and Dave, Christoph?) opinion on... another thing that was dropped (permanently) from the previous series was an update to xfs_defer_init() to do the ->t_dfops = &dfops assignment that is otherwise open-coded throughout here. While I don't have a strong preference on that for this series, the more broader cleanup I do the less I like the open-coded approach because it creates a tx setup requirement that's easy to miss (case in point: a function signature change is an easy way to verify all dfops have been associated with transactions). Instead, I'd prefer code that looks something like the following as the end result of the follow-on cleanup series:
	struct xfs_trans	*tp;
	struct xfs_defer_ops	dfops;

	xfs_trans_alloc(... &tp);
	xfs_defer_init(&dfops, &first_block, tp);

	...

	xfs_defer_finish(&tp);
	xfs_trans_commit(tp);
	...
Quoted 9 lines
Note that once we require association of dfops with tp, we don't have to pass dfops around to anywhere the tp isn't already needed, including xfs_defer_finish(). This also opens the possibility of doing things like replacing on-stack dfops with something that's just embedded in xfs_trans itself (optionally pushing down the init/finish calls into the trans infrastructure), but that would require changing how dfops are transferred to rolled transactions and whatnot. I haven't really got far enough to give that proper consideration, but that may be a possible 3rd step cleanup if there's value.

Quoted 5 lines

Not sure. There are places where it seems to make sense to be able to _defer_finish but still have a transaction around to continue making changes afterwards. Or at least, the attr and quota code seems to do that. :P

Right.. I do think xfs_defer_finish() should continue to exist even if we were to eventually do something like embed a dfops in the tp and allow a transaction commit to do the dfops completion. Something like that would just essentially clean out some of the simple trans_alloc() -> do_stuff() -> xfs_defer_finish() -> xfs_trans_commit() boilerplate code. Code that does looping xfs_defer_finish() calls and whatnot should continue to look the same.

But I don't think that has much of a bearing on tweaking xfs_defer_init() vs. tp->t_dfops = &dfops..?

Quoted 8 lines

Also, there's one funny case where we can have a dfops but no transaction, which is xlog_recover_process_intents -> xlog_finish_defer_ops. We create a dfops to collect new intents created as part of replaying unfinished intents that were in the dirty log, but since the intent recovery functions create and commit their own transactions, we don't create the transaction that goes with the dfops until we're done replaying old intents and ready to finish the new intents.

I should have pointed out that the tp parameter is intended to be optional. So xfs_defer_init() simply does the tp->t_dfops = &dfops assignment if tp != NULL. If tp == NULL, the associated dfops could certainly already be associated with some transaction and be reused (which appears to be the use case down in some of the xfs_da_args code IIRC), or not at all. So the recovery code you refer to could simply go unchanged or be an exception where we do open-code the ->t_dfops assignment.

The part I'm curious about here is really just whether it's worth tweaking xfs_defer_init() so we don't have to add an additional tp->t_dfops = &dfops assignment (almost) everywhere we already call xfs_defer_init(). I suppose I could try that change after the fact so it's easier to factor in or out...

Brian

Quoted 123 lines

--D

Quoted 111 lines

Anyways, I'd really like to avoid having to switch back and forth between the xfs_defer_init() tweak and open-coded assignment if possible. Any thoughts/preferences on either approach? Brian

Quoted 98 lines

Separate cleanup, of course, and sorry if this has already been asked.

Other than that this looks ok enough to test,

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 90 lines

fs/xfs/xfs_trans.h +0 -0

 	unsigned int		t_flags;	/* misc flags */
 	int64_t			t_icount_delta;	/* superblock icount change */
 	int64_t			t_ifree_delta;	/* superblock ifree change */

fs/xfs/xfs_trans_extfree.c +70 -0

index ab438647592a..f5620796ae25 100644
@@ -231,9 +231,79 @@ static const struct xfs_defer_op_type xfs_extent_free_defer_type = {
 	.cancel_item	= xfs_extent_free_cancel_item,
 };
 
+/*
+ * AGFL blocks are accounted differently in the reserve pools and are not
+ * inserted into the busy extent list.
+ */
+STATIC int
+xfs_agfl_free_finish_item(
+	struct xfs_trans		*tp,
+	struct xfs_defer_ops		*dop,
+	struct list_head		*item,
+	void				*done_item,
+	void				**state)
+{
+	struct xfs_mount		*mp = tp->t_mountp;
+	struct xfs_efd_log_item		*efdp = done_item;
+	struct xfs_extent_free_item	*free;
+	struct xfs_extent		*extp;
+	struct xfs_buf			*agbp;
+	int				error;
+	xfs_agnumber_t			agno;
+	xfs_agblock_t			agbno;
+	uint				next_extent;
+
+	free = container_of(item, struct xfs_extent_free_item, xefi_list);
+	ASSERT(free->xefi_blockcount == 1);
+	agno = XFS_FSB_TO_AGNO(mp, free->xefi_startblock);
+	agbno = XFS_FSB_TO_AGBNO(mp, free->xefi_startblock);
+
+	trace_xfs_agfl_free_deferred(mp, agno, 0, agbno, free->xefi_blockcount);
+
+	error = xfs_alloc_read_agf(mp, tp, agno, 0, &agbp);
+	if (!error)
+		error = xfs_free_agfl_block(tp, agno, agbno, agbp,
+					    &free->xefi_oinfo);
+
+	/*
+	 * Mark the transaction dirty, even on error. This ensures the
+	 * transaction is aborted, which:
+	 *
+	 * 1.) releases the EFI and frees the EFD
+	 * 2.) shuts down the filesystem
+	 */
+	tp->t_flags |= XFS_TRANS_DIRTY;
+	efdp->efd_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+
+	next_extent = efdp->efd_next_extent;
+	ASSERT(next_extent < efdp->efd_format.efd_nextents);
+	extp = &(efdp->efd_format.efd_extents[next_extent]);
+	extp->ext_start = free->xefi_startblock;
+	extp->ext_len = free->xefi_blockcount;
+	efdp->efd_next_extent++;
+
+	kmem_free(free);
+	return error;
+}
+
+
+/* sub-type with special handling for AGFL deferred frees */
+static const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
+	.type		= XFS_DEFER_OPS_TYPE_AGFL_FREE,
+	.max_items	= XFS_EFI_MAX_FAST_EXTENTS,
+	.diff_items	= xfs_extent_free_diff_items,
+	.create_intent	= xfs_extent_free_create_intent,
+	.abort_intent	= xfs_extent_free_abort_intent,
+	.log_item	= xfs_extent_free_log_item,
+	.create_done	= xfs_extent_free_create_done,
+	.finish_item	= xfs_agfl_free_finish_item,
+	.cancel_item	= xfs_extent_free_cancel_item,
+};
+
 /* Register the deferred op type. */
 void
 xfs_extent_free_init_defer_op(void)
 {
 	xfs_defer_init_op_type(&xfs_extent_free_defer_type);
+	xfs_defer_init_op_type(&xfs_agfl_free_defer_type);
 }

Signature · 1 line

2.13.6

Quoted 8 lines

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

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

Darrick J. Wong

2018-05-09 14:35 UTC #8 lore raw

Re: [PATCH v2 2/6] xfs: defer agfl block frees when dfops is available

Quoted 112 lines · Brian Foster

Quoted 97 lines · Darrick J. Wong
Quoted 55 lines · Brian Foster
Quoted 1 line · Darrick J. Wong
On Wed, Apr 18, 2018 at 09:31:15AM -0400, Brian Foster wrote:
...
Quoted 11 lines
Quoted 11 lines
Signed-off-by Brian Foster <[email protected]>
---
 fs/xfs/libxfs/xfs_alloc.c  | 51 ++++++++++++++++++++++++++++++---
 fs/xfs/libxfs/xfs_defer.h  |  1 +
 fs/xfs/xfs_trace.h         |  2 ++
 fs/xfs/xfs_trans.c         | 11 ++++++--
 fs/xfs/xfs_trans.h         |  1 +
 fs/xfs/xfs_trans_extfree.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 129 insertions(+), 7 deletions(-)
...
Quoted 12 lines
fs/xfs/xfs_trans.h typedef struct xfs_trans { · line 114
Quoted 9 lines
fs/xfs/xfs_trans.h +1 -0
index 9d542dfe0052..834388c2c9de 100644
@@ -111,6 +111,7 @@ typedef struct xfs_trans {
 	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_agfl_dfops;	/* optional agfl fixup dfops */
This more or less looks fine to me, with ^^^^ this one (incoherent) quibble:
I wouldn't call it incoherent...
Quoted 7 lines
A thread can only have one transaction and one dfops in play at a time, so could we just call this t_dfops and allow anyone to access it who wants to add their own deferred operations? Then we can stop passing both tp and dfops all over the stack. Can you think of a situation where we would want access to t_dfops for deferred agfl frees but not for any other deferred ops?
... because that is precisely the secondary goal of this approach. :) Dave suggested this approach in a previous version and the last post actually used ->t_dfops rather than ->t_agfl_dfops and included some associated dfops parameter elimination cleanup I did along the way.
Christoph objected to the partial refactoring, however, so I'm trying to do this in two parts where the first is primarily focused on fixing the problems resolved by deferred agfl frees. The challenge is that I don't want to manually plumb dfops through new codepaths to control deferred agfl frees only to immediately turn around and clean that code out, and I certainly don't want to refactor all of our dfops code just to enable deferred agfl frees because it creates an unnecessary dependency for backports. As a result, I've stripped out as much refactoring as possible to try and make it clear that this is a bug fix series that happens to create a new xfs_trans field that will open wider cleanups in a follow on series.
Quoted 4 lines
IOW, I have a series in progress where patch 1 renames ->t_agfl_dfops back to ->t_dfops to essentially open it for general use and then starts to incorporate the broader cleanups like removing dfops from callstacks, alloc/bmap data structures, etc.
Seems reasonable. I've occasionally thought that passing around (tp, dfops, firstblock) is kinda bulky and silly.
Quoted 23 lines
BTW, something I'm curious of your (and Dave, Christoph?) opinion on... another thing that was dropped (permanently) from the previous series was an update to xfs_defer_init() to do the ->t_dfops = &dfops assignment that is otherwise open-coded throughout here. While I don't have a strong preference on that for this series, the more broader cleanup I do the less I like the open-coded approach because it creates a tx setup requirement that's easy to miss (case in point: a function signature change is an easy way to verify all dfops have been associated with transactions). Instead, I'd prefer code that looks something like the following as the end result of the follow-on cleanup series:
	struct xfs_trans	*tp;
	struct xfs_defer_ops	dfops;

	xfs_trans_alloc(... &tp);
	xfs_defer_init(&dfops, &first_block, tp);

	...

	xfs_defer_finish(&tp);
	xfs_trans_commit(tp);
	...
Quoted 9 lines
Note that once we require association of dfops with tp, we don't have to pass dfops around to anywhere the tp isn't already needed, including xfs_defer_finish(). This also opens the possibility of doing things like replacing on-stack dfops with something that's just embedded in xfs_trans itself (optionally pushing down the init/finish calls into the trans infrastructure), but that would require changing how dfops are transferred to rolled transactions and whatnot. I haven't really got far enough to give that proper consideration, but that may be a possible 3rd step cleanup if there's value.
Quoted 5 lines
Not sure. There are places where it seems to make sense to be able to _defer_finish but still have a transaction around to continue making changes afterwards. Or at least, the attr and quota code seems to do that. :P
Right.. I do think xfs_defer_finish() should continue to exist even if we were to eventually do something like embed a dfops in the tp and allow a transaction commit to do the dfops completion. Something like that would just essentially clean out some of the simple trans_alloc() -> do_stuff() -> xfs_defer_finish() -> xfs_trans_commit() boilerplate code. Code that does looping xfs_defer_finish() calls and whatnot should continue to look the same.

Quoted 2 lines

But I don't think that has much of a bearing on tweaking xfs_defer_init() vs. tp->t_dfops = &dfops..?

It doesn't; I had gone off into the weeds thinking about the _finish cases. I guess we're talking about splitting the dfops usages into scenario (a) where the transaction owns the dfops and _finishes it as part of _trans_commit; and (b) the existing situation where the calling function owns the dfops and has to take care of _init and _finish explicitly. I think that would work and it seems like a reasonable cleanup since there are a lot of places where the code does _finish and _commit and that's it.

Quoted 18 lines

Quoted 8 lines
Also, there's one funny case where we can have a dfops but no transaction, which is xlog_recover_process_intents -> xlog_finish_defer_ops. We create a dfops to collect new intents created as part of replaying unfinished intents that were in the dirty log, but since the intent recovery functions create and commit their own transactions, we don't create the transaction that goes with the dfops until we're done replaying old intents and ready to finish the new intents.
I should have pointed out that the tp parameter is intended to be optional. So xfs_defer_init() simply does the tp->t_dfops = &dfops assignment if tp != NULL. If tp == NULL, the associated dfops could certainly already be associated with some transaction and be reused (which appears to be the use case down in some of the xfs_da_args code IIRC), or not at all. So the recovery code you refer to could simply go unchanged or be an exception where we do open-code the ->t_dfops assignment.

Quoted 5 lines

The part I'm curious about here is really just whether it's worth tweaking xfs_defer_init() so we don't have to add an additional tp->t_dfops = &dfops assignment (almost) everywhere we already call xfs_defer_init(). I suppose I could try that change after the fact so it's easier to factor in or out...

Hm, I think I like this idea where the transaction can set up an internal dfops and manage it for you unless you call _defer_init to signal that you're going to manage the dfops yourself and want to override the internal dfops. Would that fit well with all this? I think it would...

(And now that I think about it, no, you can't really have nested dfops because _defer_[bi]join means that you want the specified buffer/inode relogged and held across transaction rolls, which doesn't happen if a nested dfops rolls the transaction.)

--D

Quoted 130 lines

Brian

Quoted 115 lines

--D

Quoted 111 lines

Anyways, I'd really like to avoid having to switch back and forth between the xfs_defer_init() tweak and open-coded assignment if possible. Any thoughts/preferences on either approach? Brian

Quoted 106 lines

Separate cleanup, of course, and sorry if this has already been asked.

Other than that this looks ok enough to test,

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 94 lines

fs/xfs/xfs_trans.h +0 -0

 	unsigned int		t_flags;	/* misc flags */
 	int64_t			t_icount_delta;	/* superblock icount change */
 	int64_t			t_ifree_delta;	/* superblock ifree change */

fs/xfs/xfs_trans_extfree.c +70 -0

index ab438647592a..f5620796ae25 100644
@@ -231,9 +231,79 @@ static const struct xfs_defer_op_type xfs_extent_free_defer_type = {
 	.cancel_item	= xfs_extent_free_cancel_item,
 };
 
+/*
+ * AGFL blocks are accounted differently in the reserve pools and are not
+ * inserted into the busy extent list.
+ */
+STATIC int
+xfs_agfl_free_finish_item(
+	struct xfs_trans		*tp,
+	struct xfs_defer_ops		*dop,
+	struct list_head		*item,
+	void				*done_item,
+	void				**state)
+{
+	struct xfs_mount		*mp = tp->t_mountp;
+	struct xfs_efd_log_item		*efdp = done_item;
+	struct xfs_extent_free_item	*free;
+	struct xfs_extent		*extp;
+	struct xfs_buf			*agbp;
+	int				error;
+	xfs_agnumber_t			agno;
+	xfs_agblock_t			agbno;
+	uint				next_extent;
+
+	free = container_of(item, struct xfs_extent_free_item, xefi_list);
+	ASSERT(free->xefi_blockcount == 1);
+	agno = XFS_FSB_TO_AGNO(mp, free->xefi_startblock);
+	agbno = XFS_FSB_TO_AGBNO(mp, free->xefi_startblock);
+
+	trace_xfs_agfl_free_deferred(mp, agno, 0, agbno, free->xefi_blockcount);
+
+	error = xfs_alloc_read_agf(mp, tp, agno, 0, &agbp);
+	if (!error)
+		error = xfs_free_agfl_block(tp, agno, agbno, agbp,
+					    &free->xefi_oinfo);
+
+	/*
+	 * Mark the transaction dirty, even on error. This ensures the
+	 * transaction is aborted, which:
+	 *
+	 * 1.) releases the EFI and frees the EFD
+	 * 2.) shuts down the filesystem
+	 */
+	tp->t_flags |= XFS_TRANS_DIRTY;
+	efdp->efd_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+
+	next_extent = efdp->efd_next_extent;
+	ASSERT(next_extent < efdp->efd_format.efd_nextents);
+	extp = &(efdp->efd_format.efd_extents[next_extent]);
+	extp->ext_start = free->xefi_startblock;
+	extp->ext_len = free->xefi_blockcount;
+	efdp->efd_next_extent++;
+
+	kmem_free(free);
+	return error;
+}
+
+
+/* sub-type with special handling for AGFL deferred frees */
+static const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
+	.type		= XFS_DEFER_OPS_TYPE_AGFL_FREE,
+	.max_items	= XFS_EFI_MAX_FAST_EXTENTS,
+	.diff_items	= xfs_extent_free_diff_items,
+	.create_intent	= xfs_extent_free_create_intent,
+	.abort_intent	= xfs_extent_free_abort_intent,
+	.log_item	= xfs_extent_free_log_item,
+	.create_done	= xfs_extent_free_create_done,
+	.finish_item	= xfs_agfl_free_finish_item,
+	.cancel_item	= xfs_extent_free_cancel_item,
+};
+
 /* Register the deferred op type. */
 void
 xfs_extent_free_init_defer_op(void)
 {
 	xfs_defer_init_op_type(&xfs_extent_free_defer_type);
+	xfs_defer_init_op_type(&xfs_agfl_free_defer_type);
 }

Signature · 6 lines

2.13.6

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

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

Brian Foster

author

2018-05-09 15:41 UTC #9 lore raw

Re: [PATCH v2 2/6] xfs: defer agfl block frees when dfops is available

Quoted 116 lines · Darrick J. Wong

Quoted 112 lines · Brian Foster
Quoted 97 lines · Darrick J. Wong
Quoted 55 lines · Brian Foster
Quoted 1 line · Darrick J. Wong
On Wed, Apr 18, 2018 at 09:31:15AM -0400, Brian Foster wrote:
...
Quoted 11 lines
Quoted 11 lines
Signed-off-by Brian Foster <[email protected]>
---
 fs/xfs/libxfs/xfs_alloc.c  | 51 ++++++++++++++++++++++++++++++---
 fs/xfs/libxfs/xfs_defer.h  |  1 +
 fs/xfs/xfs_trace.h         |  2 ++
 fs/xfs/xfs_trans.c         | 11 ++++++--
 fs/xfs/xfs_trans.h         |  1 +
 fs/xfs/xfs_trans_extfree.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 129 insertions(+), 7 deletions(-)
...
Quoted 12 lines
fs/xfs/xfs_trans.h typedef struct xfs_trans { · line 114
Quoted 9 lines
fs/xfs/xfs_trans.h +1 -0
index 9d542dfe0052..834388c2c9de 100644
@@ -111,6 +111,7 @@ typedef struct xfs_trans {
 	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_agfl_dfops;	/* optional agfl fixup dfops */
This more or less looks fine to me, with ^^^^ this one (incoherent) quibble:
I wouldn't call it incoherent...
Quoted 7 lines
A thread can only have one transaction and one dfops in play at a time, so could we just call this t_dfops and allow anyone to access it who wants to add their own deferred operations? Then we can stop passing both tp and dfops all over the stack. Can you think of a situation where we would want access to t_dfops for deferred agfl frees but not for any other deferred ops?
... because that is precisely the secondary goal of this approach. :) Dave suggested this approach in a previous version and the last post actually used ->t_dfops rather than ->t_agfl_dfops and included some associated dfops parameter elimination cleanup I did along the way.
Christoph objected to the partial refactoring, however, so I'm trying to do this in two parts where the first is primarily focused on fixing the problems resolved by deferred agfl frees. The challenge is that I don't want to manually plumb dfops through new codepaths to control deferred agfl frees only to immediately turn around and clean that code out, and I certainly don't want to refactor all of our dfops code just to enable deferred agfl frees because it creates an unnecessary dependency for backports. As a result, I've stripped out as much refactoring as possible to try and make it clear that this is a bug fix series that happens to create a new xfs_trans field that will open wider cleanups in a follow on series.
Quoted 4 lines
IOW, I have a series in progress where patch 1 renames ->t_agfl_dfops back to ->t_dfops to essentially open it for general use and then starts to incorporate the broader cleanups like removing dfops from callstacks, alloc/bmap data structures, etc.
Seems reasonable. I've occasionally thought that passing around (tp, dfops, firstblock) is kinda bulky and silly.
Quoted 23 lines
BTW, something I'm curious of your (and Dave, Christoph?) opinion on... another thing that was dropped (permanently) from the previous series was an update to xfs_defer_init() to do the ->t_dfops = &dfops assignment that is otherwise open-coded throughout here. While I don't have a strong preference on that for this series, the more broader cleanup I do the less I like the open-coded approach because it creates a tx setup requirement that's easy to miss (case in point: a function signature change is an easy way to verify all dfops have been associated with transactions). Instead, I'd prefer code that looks something like the following as the end result of the follow-on cleanup series:
	struct xfs_trans	*tp;
	struct xfs_defer_ops	dfops;

	xfs_trans_alloc(... &tp);
	xfs_defer_init(&dfops, &first_block, tp);

	...

	xfs_defer_finish(&tp);
	xfs_trans_commit(tp);
	...
Quoted 9 lines
Note that once we require association of dfops with tp, we don't have to pass dfops around to anywhere the tp isn't already needed, including xfs_defer_finish(). This also opens the possibility of doing things like replacing on-stack dfops with something that's just embedded in xfs_trans itself (optionally pushing down the init/finish calls into the trans infrastructure), but that would require changing how dfops are transferred to rolled transactions and whatnot. I haven't really got far enough to give that proper consideration, but that may be a possible 3rd step cleanup if there's value.
Quoted 5 lines
Not sure. There are places where it seems to make sense to be able to _defer_finish but still have a transaction around to continue making changes afterwards. Or at least, the attr and quota code seems to do that. :P
Right.. I do think xfs_defer_finish() should continue to exist even if we were to eventually do something like embed a dfops in the tp and allow a transaction commit to do the dfops completion. Something like that would just essentially clean out some of the simple trans_alloc() -> do_stuff() -> xfs_defer_finish() -> xfs_trans_commit() boilerplate code. Code that does looping xfs_defer_finish() calls and whatnot should continue to look the same.
Quoted 2 lines
But I don't think that has much of a bearing on tweaking xfs_defer_init() vs. tp->t_dfops = &dfops..?

Quoted 9 lines

It doesn't; I had gone off into the weeds thinking about the _finish cases. I guess we're talking about splitting the dfops usages into scenario (a) where the transaction owns the dfops and _finishes it as part of _trans_commit; and (b) the existing situation where the calling function owns the dfops and has to take care of _init and _finish explicitly. I think that would work and it seems like a reasonable cleanup since there are a lot of places where the code does _finish and _commit and that's it.

Ok.

Quoted 24 lines

Quoted 18 lines
Quoted 8 lines
Also, there's one funny case where we can have a dfops but no transaction, which is xlog_recover_process_intents -> xlog_finish_defer_ops. We create a dfops to collect new intents created as part of replaying unfinished intents that were in the dirty log, but since the intent recovery functions create and commit their own transactions, we don't create the transaction that goes with the dfops until we're done replaying old intents and ready to finish the new intents.
I should have pointed out that the tp parameter is intended to be optional. So xfs_defer_init() simply does the tp->t_dfops = &dfops assignment if tp != NULL. If tp == NULL, the associated dfops could certainly already be associated with some transaction and be reused (which appears to be the use case down in some of the xfs_da_args code IIRC), or not at all. So the recovery code you refer to could simply go unchanged or be an exception where we do open-code the ->t_dfops assignment.
Quoted 5 lines
The part I'm curious about here is really just whether it's worth tweaking xfs_defer_init() so we don't have to add an additional tp->t_dfops = &dfops assignment (almost) everywhere we already call xfs_defer_init(). I suppose I could try that change after the fact so it's easier to factor in or out...

Quoted 6 lines

Hm, I think I like this idea where the transaction can set up an internal dfops and manage it for you unless you call _defer_init to signal that you're going to manage the dfops yourself and want to override the internal dfops. Would that fit well with all this? I think it would...

I think it could..

Quoted 5 lines

(And now that I think about it, no, you can't really have nested dfops because _defer_[bi]join means that you want the specified buffer/inode relogged and held across transaction rolls, which doesn't happen if a nested dfops rolls the transaction.)

... but in the simplest form it may just depend on the context. Some transactions may be able to use the in-tx model, others may have to exclusively use something local or otherwise reference the embedded dfops directly.

Anyways, I haven't really thought far enough ahead to consider changing how dfops might be allocated/used directly in a transaction. I wanted to leave that as a last step once the low hanging fruit is cleaned up and the transaction reference is used consistently.

Brian

Quoted 136 lines

--D
Quoted 122 lines
Brian
Quoted 119 lines
--D
Quoted 115 lines
Anyways, I'd really like to avoid having to switch back and forth between the xfs_defer_init() tweak and open-coded assignment if possible. Any thoughts/preferences on either approach? Brian
Quoted 98 lines
Separate cleanup, of course, and sorry if this has already been asked.
Other than that this looks ok enough to test,
Reviewed-by Darrick J. Wong <[email protected]>
--D
Quoted 90 lines
fs/xfs/xfs_trans.h +0 -0
 	unsigned int		t_flags;	/* misc flags */
 	int64_t			t_icount_delta;	/* superblock icount change */
 	int64_t			t_ifree_delta;	/* superblock ifree change */
fs/xfs/xfs_trans_extfree.c +70 -0
index ab438647592a..f5620796ae25 100644
@@ -231,9 +231,79 @@ static const struct xfs_defer_op_type xfs_extent_free_defer_type = {
 	.cancel_item	= xfs_extent_free_cancel_item,
 };
 
+/*
+ * AGFL blocks are accounted differently in the reserve pools and are not
+ * inserted into the busy extent list.
+ */
+STATIC int
+xfs_agfl_free_finish_item(
+	struct xfs_trans		*tp,
+	struct xfs_defer_ops		*dop,
+	struct list_head		*item,
+	void				*done_item,
+	void				**state)
+{
+	struct xfs_mount		*mp = tp->t_mountp;
+	struct xfs_efd_log_item		*efdp = done_item;
+	struct xfs_extent_free_item	*free;
+	struct xfs_extent		*extp;
+	struct xfs_buf			*agbp;
+	int				error;
+	xfs_agnumber_t			agno;
+	xfs_agblock_t			agbno;
+	uint				next_extent;
+
+	free = container_of(item, struct xfs_extent_free_item, xefi_list);
+	ASSERT(free->xefi_blockcount == 1);
+	agno = XFS_FSB_TO_AGNO(mp, free->xefi_startblock);
+	agbno = XFS_FSB_TO_AGBNO(mp, free->xefi_startblock);
+
+	trace_xfs_agfl_free_deferred(mp, agno, 0, agbno, free->xefi_blockcount);
+
+	error = xfs_alloc_read_agf(mp, tp, agno, 0, &agbp);
+	if (!error)
+		error = xfs_free_agfl_block(tp, agno, agbno, agbp,
+					    &free->xefi_oinfo);
+
+	/*
+	 * Mark the transaction dirty, even on error. This ensures the
+	 * transaction is aborted, which:
+	 *
+	 * 1.) releases the EFI and frees the EFD
+	 * 2.) shuts down the filesystem
+	 */
+	tp->t_flags |= XFS_TRANS_DIRTY;
+	efdp->efd_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+
+	next_extent = efdp->efd_next_extent;
+	ASSERT(next_extent < efdp->efd_format.efd_nextents);
+	extp = &(efdp->efd_format.efd_extents[next_extent]);
+	extp->ext_start = free->xefi_startblock;
+	extp->ext_len = free->xefi_blockcount;
+	efdp->efd_next_extent++;
+
+	kmem_free(free);
+	return error;
+}
+
+
+/* sub-type with special handling for AGFL deferred frees */
+static const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
+	.type		= XFS_DEFER_OPS_TYPE_AGFL_FREE,
+	.max_items	= XFS_EFI_MAX_FAST_EXTENTS,
+	.diff_items	= xfs_extent_free_diff_items,
+	.create_intent	= xfs_extent_free_create_intent,
+	.abort_intent	= xfs_extent_free_abort_intent,
+	.log_item	= xfs_extent_free_log_item,
+	.create_done	= xfs_extent_free_create_done,
+	.finish_item	= xfs_agfl_free_finish_item,
+	.cancel_item	= xfs_extent_free_cancel_item,
+};
+
 /* Register the deferred op type. */
 void
 xfs_extent_free_init_defer_op(void)
 {
 	xfs_defer_init_op_type(&xfs_extent_free_defer_type);
+	xfs_defer_init_op_type(&xfs_agfl_free_defer_type);
 }
Signature · 1 line
2.13.6
Quoted 8 lines
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
--
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 8 lines
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
--
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

Darrick J. Wong

2018-05-09 15:57 UTC #10 lore raw

Re: [PATCH v2 2/6] xfs: defer agfl block frees when dfops is available

Quoted 173 lines · Brian Foster

Quoted 116 lines · Darrick J. Wong
Quoted 112 lines · Brian Foster
Quoted 97 lines · Darrick J. Wong
Quoted 55 lines · Brian Foster
Quoted 1 line · Darrick J. Wong
On Wed, Apr 18, 2018 at 09:31:15AM -0400, Brian Foster wrote:
...
Quoted 11 lines
Quoted 11 lines
Signed-off-by Brian Foster <[email protected]>
---
 fs/xfs/libxfs/xfs_alloc.c  | 51 ++++++++++++++++++++++++++++++---
 fs/xfs/libxfs/xfs_defer.h  |  1 +
 fs/xfs/xfs_trace.h         |  2 ++
 fs/xfs/xfs_trans.c         | 11 ++++++--
 fs/xfs/xfs_trans.h         |  1 +
 fs/xfs/xfs_trans_extfree.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 129 insertions(+), 7 deletions(-)
...
Quoted 12 lines
fs/xfs/xfs_trans.h typedef struct xfs_trans { · line 114
Quoted 9 lines
fs/xfs/xfs_trans.h +1 -0
index 9d542dfe0052..834388c2c9de 100644
@@ -111,6 +111,7 @@ typedef struct xfs_trans {
 	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_agfl_dfops;	/* optional agfl fixup dfops */
This more or less looks fine to me, with ^^^^ this one (incoherent) quibble:
I wouldn't call it incoherent...
Quoted 7 lines
A thread can only have one transaction and one dfops in play at a time, so could we just call this t_dfops and allow anyone to access it who wants to add their own deferred operations? Then we can stop passing both tp and dfops all over the stack. Can you think of a situation where we would want access to t_dfops for deferred agfl frees but not for any other deferred ops?
... because that is precisely the secondary goal of this approach. :) Dave suggested this approach in a previous version and the last post actually used ->t_dfops rather than ->t_agfl_dfops and included some associated dfops parameter elimination cleanup I did along the way.
Christoph objected to the partial refactoring, however, so I'm trying to do this in two parts where the first is primarily focused on fixing the problems resolved by deferred agfl frees. The challenge is that I don't want to manually plumb dfops through new codepaths to control deferred agfl frees only to immediately turn around and clean that code out, and I certainly don't want to refactor all of our dfops code just to enable deferred agfl frees because it creates an unnecessary dependency for backports. As a result, I've stripped out as much refactoring as possible to try and make it clear that this is a bug fix series that happens to create a new xfs_trans field that will open wider cleanups in a follow on series.
Quoted 4 lines
IOW, I have a series in progress where patch 1 renames ->t_agfl_dfops back to ->t_dfops to essentially open it for general use and then starts to incorporate the broader cleanups like removing dfops from callstacks, alloc/bmap data structures, etc.
Seems reasonable. I've occasionally thought that passing around (tp, dfops, firstblock) is kinda bulky and silly.
Quoted 23 lines
BTW, something I'm curious of your (and Dave, Christoph?) opinion on... another thing that was dropped (permanently) from the previous series was an update to xfs_defer_init() to do the ->t_dfops = &dfops assignment that is otherwise open-coded throughout here. While I don't have a strong preference on that for this series, the more broader cleanup I do the less I like the open-coded approach because it creates a tx setup requirement that's easy to miss (case in point: a function signature change is an easy way to verify all dfops have been associated with transactions). Instead, I'd prefer code that looks something like the following as the end result of the follow-on cleanup series:
	struct xfs_trans	*tp;
	struct xfs_defer_ops	dfops;

	xfs_trans_alloc(... &tp);
	xfs_defer_init(&dfops, &first_block, tp);

	...

	xfs_defer_finish(&tp);
	xfs_trans_commit(tp);
	...
Quoted 9 lines
Note that once we require association of dfops with tp, we don't have to pass dfops around to anywhere the tp isn't already needed, including xfs_defer_finish(). This also opens the possibility of doing things like replacing on-stack dfops with something that's just embedded in xfs_trans itself (optionally pushing down the init/finish calls into the trans infrastructure), but that would require changing how dfops are transferred to rolled transactions and whatnot. I haven't really got far enough to give that proper consideration, but that may be a possible 3rd step cleanup if there's value.
Quoted 5 lines
Not sure. There are places where it seems to make sense to be able to _defer_finish but still have a transaction around to continue making changes afterwards. Or at least, the attr and quota code seems to do that. :P
Right.. I do think xfs_defer_finish() should continue to exist even if we were to eventually do something like embed a dfops in the tp and allow a transaction commit to do the dfops completion. Something like that would just essentially clean out some of the simple trans_alloc() -> do_stuff() -> xfs_defer_finish() -> xfs_trans_commit() boilerplate code. Code that does looping xfs_defer_finish() calls and whatnot should continue to look the same.
Quoted 2 lines
But I don't think that has much of a bearing on tweaking xfs_defer_init() vs. tp->t_dfops = &dfops..?
Quoted 9 lines
It doesn't; I had gone off into the weeds thinking about the _finish cases. I guess we're talking about splitting the dfops usages into scenario (a) where the transaction owns the dfops and _finishes it as part of _trans_commit; and (b) the existing situation where the calling function owns the dfops and has to take care of _init and _finish explicitly. I think that would work and it seems like a reasonable cleanup since there are a lot of places where the code does _finish and _commit and that's it.
Ok.
Quoted 24 lines
Quoted 18 lines
Quoted 8 lines
Also, there's one funny case where we can have a dfops but no transaction, which is xlog_recover_process_intents -> xlog_finish_defer_ops. We create a dfops to collect new intents created as part of replaying unfinished intents that were in the dirty log, but since the intent recovery functions create and commit their own transactions, we don't create the transaction that goes with the dfops until we're done replaying old intents and ready to finish the new intents.
I should have pointed out that the tp parameter is intended to be optional. So xfs_defer_init() simply does the tp->t_dfops = &dfops assignment if tp != NULL. If tp == NULL, the associated dfops could certainly already be associated with some transaction and be reused (which appears to be the use case down in some of the xfs_da_args code IIRC), or not at all. So the recovery code you refer to could simply go unchanged or be an exception where we do open-code the ->t_dfops assignment.
Quoted 5 lines
The part I'm curious about here is really just whether it's worth tweaking xfs_defer_init() so we don't have to add an additional tp->t_dfops = &dfops assignment (almost) everywhere we already call xfs_defer_init(). I suppose I could try that change after the fact so it's easier to factor in or out...
Quoted 6 lines
Hm, I think I like this idea where the transaction can set up an internal dfops and manage it for you unless you call _defer_init to signal that you're going to manage the dfops yourself and want to override the internal dfops. Would that fit well with all this? I think it would...
I think it could..
Quoted 5 lines
(And now that I think about it, no, you can't really have nested dfops because _defer_[bi]join means that you want the specified buffer/inode relogged and held across transaction rolls, which doesn't happen if a nested dfops rolls the transaction.)
... but in the simplest form it may just depend on the context. Some transactions may be able to use the in-tx model, others may have to exclusively use something local or otherwise reference the embedded dfops directly.

Quoted 4 lines

Anyways, I haven't really thought far enough ahead to consider changing how dfops might be allocated/used directly in a transaction. I wanted to leave that as a last step once the low hanging fruit is cleaned up and the transaction reference is used consistently.

<nod> FWIW I pulled this in for testing.

--D

Quoted 142 lines

Brian
Quoted 128 lines
--D
Quoted 126 lines
Brian
Quoted 123 lines
--D
Quoted 111 lines
Anyways, I'd really like to avoid having to switch back and forth between the xfs_defer_init() tweak and open-coded assignment if possible. Any thoughts/preferences on either approach? Brian
Quoted 98 lines
Separate cleanup, of course, and sorry if this has already been asked.
Other than that this looks ok enough to test,
Reviewed-by Darrick J. Wong <[email protected]>
--D
Quoted 90 lines
fs/xfs/xfs_trans.h +0 -0
 	unsigned int		t_flags;	/* misc flags */
 	int64_t			t_icount_delta;	/* superblock icount change */
 	int64_t			t_ifree_delta;	/* superblock ifree change */
fs/xfs/xfs_trans_extfree.c +70 -0
index ab438647592a..f5620796ae25 100644
@@ -231,9 +231,79 @@ static const struct xfs_defer_op_type xfs_extent_free_defer_type = {
 	.cancel_item	= xfs_extent_free_cancel_item,
 };
 
+/*
+ * AGFL blocks are accounted differently in the reserve pools and are not
+ * inserted into the busy extent list.
+ */
+STATIC int
+xfs_agfl_free_finish_item(
+	struct xfs_trans		*tp,
+	struct xfs_defer_ops		*dop,
+	struct list_head		*item,
+	void				*done_item,
+	void				**state)
+{
+	struct xfs_mount		*mp = tp->t_mountp;
+	struct xfs_efd_log_item		*efdp = done_item;
+	struct xfs_extent_free_item	*free;
+	struct xfs_extent		*extp;
+	struct xfs_buf			*agbp;
+	int				error;
+	xfs_agnumber_t			agno;
+	xfs_agblock_t			agbno;
+	uint				next_extent;
+
+	free = container_of(item, struct xfs_extent_free_item, xefi_list);
+	ASSERT(free->xefi_blockcount == 1);
+	agno = XFS_FSB_TO_AGNO(mp, free->xefi_startblock);
+	agbno = XFS_FSB_TO_AGBNO(mp, free->xefi_startblock);
+
+	trace_xfs_agfl_free_deferred(mp, agno, 0, agbno, free->xefi_blockcount);
+
+	error = xfs_alloc_read_agf(mp, tp, agno, 0, &agbp);
+	if (!error)
+		error = xfs_free_agfl_block(tp, agno, agbno, agbp,
+					    &free->xefi_oinfo);
+
+	/*
+	 * Mark the transaction dirty, even on error. This ensures the
+	 * transaction is aborted, which:
+	 *
+	 * 1.) releases the EFI and frees the EFD
+	 * 2.) shuts down the filesystem
+	 */
+	tp->t_flags |= XFS_TRANS_DIRTY;
+	efdp->efd_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+
+	next_extent = efdp->efd_next_extent;
+	ASSERT(next_extent < efdp->efd_format.efd_nextents);
+	extp = &(efdp->efd_format.efd_extents[next_extent]);
+	extp->ext_start = free->xefi_startblock;
+	extp->ext_len = free->xefi_blockcount;
+	efdp->efd_next_extent++;
+
+	kmem_free(free);
+	return error;
+}
+
+
+/* sub-type with special handling for AGFL deferred frees */
+static const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
+	.type		= XFS_DEFER_OPS_TYPE_AGFL_FREE,
+	.max_items	= XFS_EFI_MAX_FAST_EXTENTS,
+	.diff_items	= xfs_extent_free_diff_items,
+	.create_intent	= xfs_extent_free_create_intent,
+	.abort_intent	= xfs_extent_free_abort_intent,
+	.log_item	= xfs_extent_free_log_item,
+	.create_done	= xfs_extent_free_create_done,
+	.finish_item	= xfs_agfl_free_finish_item,
+	.cancel_item	= xfs_extent_free_cancel_item,
+};
+
 /* Register the deferred op type. */
 void
 xfs_extent_free_init_defer_op(void)
 {
 	xfs_defer_init_op_type(&xfs_extent_free_defer_type);
+	xfs_defer_init_op_type(&xfs_agfl_free_defer_type);
 }
Signature · 1 line
2.13.6
Quoted 8 lines
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
--
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 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
--
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 8 lines
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
--
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

Darrick J. Wong

2018-05-09 16:04 UTC #11 lore raw

Re: [PATCH v2 2/6] xfs: defer agfl block frees when dfops is available

In reply to Brian Foster

Quoted 254 lines · Brian Foster

Note that this patch only adds infrastructure. It does not change behavior because no callers have been updated to pass ->t_agfl_dfops into the allocation code.

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

---
 fs/xfs/libxfs/xfs_alloc.c  | 51 ++++++++++++++++++++++++++++++---
 fs/xfs/libxfs/xfs_defer.h  |  1 +
 fs/xfs/xfs_trace.h         |  2 ++
 fs/xfs/xfs_trans.c         | 11 ++++++--
 fs/xfs/xfs_trans.h         |  1 +
 fs/xfs/xfs_trans_extfree.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 129 insertions(+), 7 deletions(-)

fs/xfs/libxfs/xfs_alloc.c +47 -4

index 193a5b4909c5..350ad203b082 100644
@@ -39,6 +39,9 @@
 #include "xfs_buf_item.h"
 #include "xfs_log.h"
 #include "xfs_ag_resv.h"
+#include "xfs_bmap.h"
+
+extern kmem_zone_t	*xfs_bmap_free_item_zone;
 
 struct workqueue_struct *xfs_alloc_wq;

@@ -2172,6 +2175,40 @@ xfs_agfl_reset(
 }
 
 /*
+ * Defer an AGFL block free. This is effectively equivalent to
+ * xfs_bmap_add_free() with some special handling particular to AGFL blocks.
+ *
+ * Deferring AGFL frees helps prevent log reservation overruns due to too many
+ * allocation operations in a transaction. AGFL frees are prone to this problem
+ * because for one they are always freed one at a time. Further, an immediate
+ * AGFL block free can cause a btree join and require another block free before
+ * the real allocation can proceed. Deferring the free disconnects freeing up
+ * the AGFL slot from freeing the block.
+ */
+STATIC void
+xfs_defer_agfl_block(
+	struct xfs_mount		*mp,
+	struct xfs_defer_ops		*dfops,
+	xfs_agnumber_t			agno,
+	xfs_fsblock_t			agbno,
+	struct xfs_owner_info		*oinfo)
+{
+	struct xfs_extent_free_item	*new;		/* new element */
+
+	ASSERT(xfs_bmap_free_item_zone != NULL);
+	ASSERT(oinfo != NULL);
+
+	new = kmem_zone_alloc(xfs_bmap_free_item_zone, KM_SLEEP);
+	new->xefi_startblock = XFS_AGB_TO_FSB(mp, agno, agbno);
+	new->xefi_blockcount = 1;
+	new->xefi_oinfo = *oinfo;
+
+	trace_xfs_agfl_free_defer(mp, agno, 0, agbno, 1);
+
+	xfs_defer_add(dfops, XFS_DEFER_OPS_TYPE_AGFL_FREE, &new->xefi_list);
+}
+
+/*
  * Decide whether to use this allocation group for this allocation.
  * If so, fix up the btree freelist's size.
  */

@@ -2275,10 +2312,16 @@ xfs_alloc_fix_freelist(
 		if (error)
 			goto out_agbp_relse;
 
-		error = xfs_free_agfl_block(tp, args->agno, bno, agbp,
-					    &targs.oinfo);
-		if (error)
-			goto out_agbp_relse;
+		/* defer agfl frees if dfops is provided */
+		if (tp->t_agfl_dfops) {
+			xfs_defer_agfl_block(mp, tp->t_agfl_dfops, args->agno,
+					     bno, &targs.oinfo);
+		} else {
+			error = xfs_free_agfl_block(tp, args->agno, bno, agbp,
+						    &targs.oinfo);
+			if (error)
+				goto out_agbp_relse;
+		}
 	}
 
 	targs.tp = tp;

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

index 045beacdd37d..e70725ba1f5f 100644
@@ -55,6 +55,7 @@ enum xfs_defer_ops_type {
 	XFS_DEFER_OPS_TYPE_REFCOUNT,
 	XFS_DEFER_OPS_TYPE_RMAP,
 	XFS_DEFER_OPS_TYPE_FREE,
+	XFS_DEFER_OPS_TYPE_AGFL_FREE,
 	XFS_DEFER_OPS_TYPE_MAX,
 };

fs/xfs/xfs_trace.h +2 -0

index 8955254b900e..00e6aaea6565 100644
@@ -2433,6 +2433,8 @@ DEFINE_DEFER_PENDING_EVENT(xfs_defer_pending_abort);
 #define DEFINE_BMAP_FREE_DEFERRED_EVENT DEFINE_PHYS_EXTENT_DEFERRED_EVENT
 DEFINE_BMAP_FREE_DEFERRED_EVENT(xfs_bmap_free_defer);
 DEFINE_BMAP_FREE_DEFERRED_EVENT(xfs_bmap_free_deferred);
+DEFINE_BMAP_FREE_DEFERRED_EVENT(xfs_agfl_free_defer);
+DEFINE_BMAP_FREE_DEFERRED_EVENT(xfs_agfl_free_deferred);
 
 /* rmap tracepoints */
 DECLARE_EVENT_CLASS(xfs_rmap_class,

fs/xfs/xfs_trans.c +8 -3

index d6d8f9d129a7..06adb1a3e31f 100644
@@ -31,6 +31,7 @@
 #include "xfs_log.h"
 #include "xfs_trace.h"
 #include "xfs_error.h"
+#include "xfs_defer.h"
 
 kmem_zone_t	*xfs_trans_zone;
 kmem_zone_t	*xfs_log_item_desc_zone;

@@ -94,11 +95,11 @@ xfs_trans_free(
  * blocks.  Locks and log items, however, are no inherited.  They must
  * be added to the new transaction explicitly.
  */
-STATIC xfs_trans_t *
+STATIC struct xfs_trans *
 xfs_trans_dup(
-	xfs_trans_t	*tp)
+	struct xfs_trans	*tp)
 {
-	xfs_trans_t	*ntp;
+	struct xfs_trans	*ntp;
 
 	ntp = kmem_zone_zalloc(xfs_trans_zone, KM_SLEEP);

@@ -127,6 +128,7 @@ 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_agfl_dfops = tp->t_agfl_dfops;
 
 	xfs_trans_dup_dqinfo(tp, ntp);

@@ -936,6 +938,9 @@ __xfs_trans_commit(
 	int			error = 0;
 	int			sync = tp->t_flags & XFS_TRANS_SYNC;
 
+	ASSERT(!tp->t_agfl_dfops ||
+	       !xfs_defer_has_unfinished_work(tp->t_agfl_dfops) || regrant);
+
 	/*
 	 * If there is nothing to be logged by the transaction,
 	 * then unlock all of the items associated with the

fs/xfs/xfs_trans.h +1 -0

index 9d542dfe0052..834388c2c9de 100644
@@ -111,6 +111,7 @@ typedef struct xfs_trans {
 	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_agfl_dfops;	/* optional agfl fixup dfops */
 	unsigned int		t_flags;	/* misc flags */
 	int64_t			t_icount_delta;	/* superblock icount change */
 	int64_t			t_ifree_delta;	/* superblock ifree change */

fs/xfs/xfs_trans_extfree.c +33 -0

index ab438647592a..f5620796ae25 100644
@@ -231,9 +231,79 @@ static const struct xfs_defer_op_type xfs_extent_free_defer_type = {
 	.cancel_item	= xfs_extent_free_cancel_item,
 };
 
+/*
+ * AGFL blocks are accounted differently in the reserve pools and are not
+ * inserted into the busy extent list.
+ */
+STATIC int
+xfs_agfl_free_finish_item(
+	struct xfs_trans		*tp,
+	struct xfs_defer_ops		*dop,
+	struct list_head		*item,
+	void				*done_item,
+	void				**state)
+{
+	struct xfs_mount		*mp = tp->t_mountp;
+	struct xfs_efd_log_item		*efdp = done_item;
+	struct xfs_extent_free_item	*free;
+	struct xfs_extent		*extp;
+	struct xfs_buf			*agbp;
+	int				error;
+	xfs_agnumber_t			agno;
+	xfs_agblock_t			agbno;
+	uint				next_extent;
+
+	free = container_of(item, struct xfs_extent_free_item, xefi_list);
+	ASSERT(free->xefi_blockcount == 1);
+	agno = XFS_FSB_TO_AGNO(mp, free->xefi_startblock);
+	agbno = XFS_FSB_TO_AGBNO(mp, free->xefi_startblock);
+
+	trace_xfs_agfl_free_deferred(mp, agno, 0, agbno, free->xefi_blockcount);
+
+	error = xfs_alloc_read_agf(mp, tp, agno, 0, &agbp);
+	if (!error)
+		error = xfs_free_agfl_block(tp, agno, agbno, agbp,
+					    &free->xefi_oinfo);

fs/xfs/xfs_trans_extfree.c static const struct xfs_defer_op_type xfs_extent_free_defer_type = { · line 276

Quoted 10 lines

fs/xfs/xfs_trans_extfree.c +10 -0

+
+	/*
+	 * Mark the transaction dirty, even on error. This ensures the
+	 * transaction is aborted, which:
+	 *
+	 * 1.) releases the EFI and frees the EFD
+	 * 2.) shuts down the filesystem
+	 */
+	tp->t_flags |= XFS_TRANS_DIRTY;
+	efdp->efd_item.li_desc->lid_flags |= XFS_LID_DIRTY;

FWIW I think this becomes:

	set_bit(XFS_LI_DIRTY, &efdp->efd_item.li_flags);

after Dave's series to remove log item descriptors, afaict. Will fix it up when I pull that into the branch.

--D

Quoted 40 lines

fs/xfs/xfs_trans_extfree.c +27 -0

+
+	next_extent = efdp->efd_next_extent;
+	ASSERT(next_extent < efdp->efd_format.efd_nextents);
+	extp = &(efdp->efd_format.efd_extents[next_extent]);
+	extp->ext_start = free->xefi_startblock;
+	extp->ext_len = free->xefi_blockcount;
+	efdp->efd_next_extent++;
+
+	kmem_free(free);
+	return error;
+}
+
+
+/* sub-type with special handling for AGFL deferred frees */
+static const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
+	.type		= XFS_DEFER_OPS_TYPE_AGFL_FREE,
+	.max_items	= XFS_EFI_MAX_FAST_EXTENTS,
+	.diff_items	= xfs_extent_free_diff_items,
+	.create_intent	= xfs_extent_free_create_intent,
+	.abort_intent	= xfs_extent_free_abort_intent,
+	.log_item	= xfs_extent_free_log_item,
+	.create_done	= xfs_extent_free_create_done,
+	.finish_item	= xfs_agfl_free_finish_item,
+	.cancel_item	= xfs_extent_free_cancel_item,
+};
+
 /* Register the deferred op type. */
 void
 xfs_extent_free_init_defer_op(void)
 {
 	xfs_defer_init_op_type(&xfs_extent_free_defer_type);
+	xfs_defer_init_op_type(&xfs_agfl_free_defer_type);
 }

Signature · 6 lines

2.13.6

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

Brian Foster

author

2018-05-09 16:31 UTC #12 lore raw

Re: [PATCH v2 2/6] xfs: defer agfl block frees when dfops is available

Quoted 1 line · Darrick J. Wong

On Wed, Apr 18, 2018 at 09:31:15AM -0400, Brian Foster wrote:

...

Quoted 11 lines

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

---
 fs/xfs/libxfs/xfs_alloc.c  | 51 ++++++++++++++++++++++++++++++---
 fs/xfs/libxfs/xfs_defer.h  |  1 +
 fs/xfs/xfs_trace.h         |  2 ++
 fs/xfs/xfs_trans.c         | 11 ++++++--
 fs/xfs/xfs_trans.h         |  1 +
 fs/xfs/xfs_trans_extfree.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 129 insertions(+), 7 deletions(-)

...

Quoted 54 lines

Quoted 41 lines

fs/xfs/xfs_trans_extfree.c +33 -0

index ab438647592a..f5620796ae25 100644
@@ -231,9 +231,79 @@ static const struct xfs_defer_op_type xfs_extent_free_defer_type = {
 	.cancel_item	= xfs_extent_free_cancel_item,
 };
 
+/*
+ * AGFL blocks are accounted differently in the reserve pools and are not
+ * inserted into the busy extent list.
+ */
+STATIC int
+xfs_agfl_free_finish_item(
+	struct xfs_trans		*tp,
+	struct xfs_defer_ops		*dop,
+	struct list_head		*item,
+	void				*done_item,
+	void				**state)
+{
+	struct xfs_mount		*mp = tp->t_mountp;
+	struct xfs_efd_log_item		*efdp = done_item;
+	struct xfs_extent_free_item	*free;
+	struct xfs_extent		*extp;
+	struct xfs_buf			*agbp;
+	int				error;
+	xfs_agnumber_t			agno;
+	xfs_agblock_t			agbno;
+	uint				next_extent;
+
+	free = container_of(item, struct xfs_extent_free_item, xefi_list);
+	ASSERT(free->xefi_blockcount == 1);
+	agno = XFS_FSB_TO_AGNO(mp, free->xefi_startblock);
+	agbno = XFS_FSB_TO_AGBNO(mp, free->xefi_startblock);
+
+	trace_xfs_agfl_free_deferred(mp, agno, 0, agbno, free->xefi_blockcount);
+
+	error = xfs_alloc_read_agf(mp, tp, agno, 0, &agbp);
+	if (!error)
+		error = xfs_free_agfl_block(tp, agno, agbno, agbp,
+					    &free->xefi_oinfo);

fs/xfs/xfs_trans_extfree.c static const struct xfs_defer_op_type xfs_extent_free_defer_type = { · line 276

Quoted 10 lines

fs/xfs/xfs_trans_extfree.c +10 -0

+
+	/*
+	 * Mark the transaction dirty, even on error. This ensures the
+	 * transaction is aborted, which:
+	 *
+	 * 1.) releases the EFI and frees the EFD
+	 * 2.) shuts down the filesystem
+	 */
+	tp->t_flags |= XFS_TRANS_DIRTY;
+	efdp->efd_item.li_desc->lid_flags |= XFS_LID_DIRTY;

FWIW I think this becomes:

Quoted 5 lines

	set_bit(XFS_LI_DIRTY, &efdp->efd_item.li_flags);
after Dave's series to remove log item descriptors, afaict. Will fix it up when I pull that into the branch.

Looks right, thanks!

Brian

Quoted 46 lines

--D

Quoted 40 lines

fs/xfs/xfs_trans_extfree.c +27 -0

+
+	next_extent = efdp->efd_next_extent;
+	ASSERT(next_extent < efdp->efd_format.efd_nextents);
+	extp = &(efdp->efd_format.efd_extents[next_extent]);
+	extp->ext_start = free->xefi_startblock;
+	extp->ext_len = free->xefi_blockcount;
+	efdp->efd_next_extent++;
+
+	kmem_free(free);
+	return error;
+}
+
+
+/* sub-type with special handling for AGFL deferred frees */
+static const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
+	.type		= XFS_DEFER_OPS_TYPE_AGFL_FREE,
+	.max_items	= XFS_EFI_MAX_FAST_EXTENTS,
+	.diff_items	= xfs_extent_free_diff_items,
+	.create_intent	= xfs_extent_free_create_intent,
+	.abort_intent	= xfs_extent_free_abort_intent,
+	.log_item	= xfs_extent_free_log_item,
+	.create_done	= xfs_extent_free_create_done,
+	.finish_item	= xfs_agfl_free_finish_item,
+	.cancel_item	= xfs_extent_free_cancel_item,
+};
+
 /* Register the deferred op type. */
 void
 xfs_extent_free_init_defer_op(void)
 {
 	xfs_defer_init_op_type(&xfs_extent_free_defer_type);
+	xfs_defer_init_op_type(&xfs_agfl_free_defer_type);
 }

Signature · 6 lines

2.13.6

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

Brian Foster

author

2018-04-18 13:31 UTC #13 lore raw

PATCH v2 3/6 xfs: defer agfl block frees from deferred ops processing context

In reply to Brian Foster

Now that AGFL block frees are deferred when dfops is set in the transaction, start deferring AGFL block frees from contexts that are known to push the limits of existing log reservations.

The first such context is deferred operation processing itself. This primarily targets deferred extent frees (such as file extents and inode chunks), but in doing so covers all allocation operations that occur in deferred operation processing context.

Update xfs_defer_finish() to set and reset ->t_agfl_dfops across the processing sequence. This means that any AGFL block frees due to allocation events result in the addition of new EFIs to the dfops rather than being processed immediately. xfs_defer_finish() rolls the transaction at least once more to process the frees of the AGFL blocks back to the allocation btrees and returns once the AGFL is rectified.

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

---
 fs/xfs/libxfs/xfs_defer.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

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

index 087fea02c389..4d87d9661907 100644
@@ -352,11 +352,22 @@ 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);
 
 	trace_xfs_defer_finish((*tp)->t_mountp, dop);
 
+	/*
+	 * 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_agfl_dfops || ((*tp)->t_agfl_dfops == dop));
+	orig_dop = (*tp)->t_agfl_dfops;
+	(*tp)->t_agfl_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. */

@@ -428,6 +439,7 @@ xfs_defer_finish(
 	}
 
 out:
+	(*tp)->t_agfl_dfops = orig_dop;
 	if (error)
 		trace_xfs_defer_finish_error((*tp)->t_mountp, dop, error);
 	else

Signature · 1 line

2.13.6

Darrick J. Wong

Reviewed-by on v2 3/6

2018-05-08 01:11 UTC #14 lore raw

Re: [PATCH v2 3/6] xfs: defer agfl block frees from deferred ops processing context

Quoted 17 lines · Brian Foster

Now that AGFL block frees are deferred when dfops is set in the transaction, start deferring AGFL block frees from contexts that are known to push the limits of existing log reservations.
The first such context is deferred operation processing itself. This primarily targets deferred extent frees (such as file extents and inode chunks), but in doing so covers all allocation operations that occur in deferred operation processing context.
Update xfs_defer_finish() to set and reset ->t_agfl_dfops across the processing sequence. This means that any AGFL block frees due to allocation events result in the addition of new EFIs to the dfops rather than being processed immediately. xfs_defer_finish() rolls the transaction at least once more to process the frees of the AGFL blocks back to the allocation btrees and returns once the AGFL is rectified.

Quoted 1 line

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

Looks ok, will test...

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 46 lines

---
 fs/xfs/libxfs/xfs_defer.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

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

index 087fea02c389..4d87d9661907 100644
@@ -352,11 +352,22 @@ 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);
 
 	trace_xfs_defer_finish((*tp)->t_mountp, dop);
 
+	/*
+	 * 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_agfl_dfops || ((*tp)->t_agfl_dfops == dop));
+	orig_dop = (*tp)->t_agfl_dfops;
+	(*tp)->t_agfl_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. */

@@ -428,6 +439,7 @@ xfs_defer_finish(
 	}
 
 out:
+	(*tp)->t_agfl_dfops = orig_dop;
 	if (error)
 		trace_xfs_defer_finish_error((*tp)->t_mountp, dop, error);
 	else

Signature · 6 lines

2.13.6

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

Brian Foster

author

2018-04-18 13:31 UTC #15 lore raw

PATCH v2 4/6 xfs: defer agfl frees from inode inactivation

In reply to Brian Foster

XFS inode chunks are already freed via deferred operations (which now also defer AGFL block frees), but inode btree blocks are freed directly in the associated context. This has been known to lead to log reservation overruns in particular workloads where an inobt block free may require several AGFL block frees (and thus several allocation btree modifications) before the inobt block itself is actually freed.

To avoid this problem, defer the frees of any AGFL blocks before the inobt block free takes place. This requires passing the dfops from xfs_inactive_ifree() down through the inobt ->[alloc|free]_block() callouts, which essentially only requires to attach the dfops to the transaction since it is already carried all the way through to the inobt update and allocation.

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

---
 fs/xfs/xfs_inode.c | 1 +
 1 file changed, 1 insertion(+)

fs/xfs/xfs_inode.c +1 -0

index 2b70c8b4cee2..33e36bb019b6 100644
@@ -1811,6 +1811,7 @@ xfs_inactive_ifree(
 	xfs_trans_ijoin(tp, ip, 0);
 
 	xfs_defer_init(&dfops, &first_block);
+	tp->t_agfl_dfops = &dfops;
 	error = xfs_ifree(tp, ip, &dfops);
 	if (error) {
 		/*

Signature · 1 line

2.13.6

Brian Foster

author

2018-04-18 13:31 UTC #16 lore raw

PATCH v2 5/6 xfs: defer frees from common inode allocation paths

In reply to Brian Foster

Inode allocation can require block allocation for physical inode chunk allocation, inode btree record insertion, and/or directory block allocation for entry insertion. Any of these block allocation requests can require AGFL fixups prior to the actual allocation. Update the common file creation transacions to defer AGFL frees from these contexts to avoid too much log reservation consumption per-transaction.

Since these transactions are already passed down through the btree cursors and da_args structure, this simply requires to attach dfops to the transaction. Note that this covers tr_create, tr_mkdir and tr_symlink. Other transactions such as tr_create_tmpfile do not already make use of deferred operations and so are left alone for the time being.

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

---
 fs/xfs/xfs_inode.c   | 1 +
 fs/xfs/xfs_symlink.c | 1 +
 2 files changed, 2 insertions(+)

fs/xfs/xfs_inode.c +1 -0

index 33e36bb019b6..484ebef36fe4 100644
@@ -1196,6 +1196,7 @@ xfs_create(
 	unlock_dp_on_error = true;
 
 	xfs_defer_init(&dfops, &first_block);
+	tp->t_agfl_dfops = &dfops;
 
 	/*
 	 * Reserve disk quota and the inode.

fs/xfs/xfs_symlink.c +1 -0

index 5b66ac12913c..88643a8fd487 100644
@@ -259,6 +259,7 @@ xfs_symlink(
 	 * bmapi or the directory create code.
 	 */
 	xfs_defer_init(&dfops, &first_block);
+	tp->t_agfl_dfops = &dfops;
 
 	/*
 	 * Allocate an inode for the symlink.

Signature · 1 line

2.13.6

Brian Foster

author

2018-04-18 13:31 UTC #17 lore raw

PATCH v2 6/6 xfs: defer agfl frees from directory op transactions

In reply to Brian Foster

Directory operations can perform block allocations as entries are added/removed from directories. Defer AGFL block frees from the remaining directory operation transactions. This covers the hard link, remove and rename operations.

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

---
 fs/xfs/xfs_inode.c | 3 +++
 1 file changed, 3 insertions(+)

fs/xfs/xfs_inode.c +3 -0

index 484ebef36fe4..47aa124e4744 100644
@@ -1452,6 +1452,7 @@ xfs_link(
 	}
 
 	xfs_defer_init(&dfops, &first_block);
+	tp->t_agfl_dfops = &dfops;
 
 	/*
 	 * Handle initial link state of O_TMPFILE inode

@@ -2649,6 +2650,7 @@ xfs_remove(
 		goto out_trans_cancel;
 
 	xfs_defer_init(&dfops, &first_block);
+	tp->t_agfl_dfops = &dfops;
 	error = xfs_dir_removename(tp, dp, name, ip->i_ino,
 					&first_block, &dfops, resblks);
 	if (error) {

@@ -3016,6 +3018,7 @@ xfs_rename(
 	}
 
 	xfs_defer_init(&dfops, &first_block);
+	tp->t_agfl_dfops = &dfops;
 
 	/* RENAME_EXCHANGE is unique from here on. */
 	if (flags & RENAME_EXCHANGE)

Signature · 1 line

2.13.6

Darrick J. Wong

Reviewed-by on v2 6/6

2018-05-08 01:12 UTC #18 lore raw

Re: [PATCH v2 6/6] xfs: defer agfl frees from directory op transactions

Quoted 31 lines · Brian Foster

Directory operations can perform block allocations as entries are added/removed from directories. Defer AGFL block frees from the remaining directory operation transactions. This covers the hard link, remove and rename operations.
Signed-off-by Brian Foster <[email protected]>
---
 fs/xfs/xfs_inode.c | 3 +++
 1 file changed, 3 insertions(+)
fs/xfs/xfs_inode.c +2 -0
index 484ebef36fe4..47aa124e4744 100644
@@ -1452,6 +1452,7 @@ xfs_link(
 	}
 
 	xfs_defer_init(&dfops, &first_block);
+	tp->t_agfl_dfops = &dfops;
 
 	/*
 	 * Handle initial link state of O_TMPFILE inode
@@ -2649,6 +2650,7 @@ xfs_remove(
 		goto out_trans_cancel;
 
 	xfs_defer_init(&dfops, &first_block);
+	tp->t_agfl_dfops = &dfops;
 	error = xfs_dir_removename(tp, dp, name, ip->i_ino,
 					&first_block, &dfops, resblks);
 	if (error) {

fs/xfs/xfs_inode.c xfs_rename( · line 3021

Quoted 4 lines

fs/xfs/xfs_inode.c +1 -0

@@ -3016,6 +3018,7 @@ xfs_rename(
 	}
 
 	xfs_defer_init(&dfops, &first_block);
+	tp->t_agfl_dfops = &dfops;

Hmm, do you have a reproducer xfstest for any of the last three patches?

Codewise,

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 10 lines

fs/xfs/xfs_inode.c +0 -0

 	/* RENAME_EXCHANGE is unique from here on. */
 	if (flags & RENAME_EXCHANGE)

Signature · 6 lines

2.13.6

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

Brian Foster

author

2018-05-08 12:33 UTC #19 lore raw

Re: [PATCH v2 6/6] xfs: defer agfl frees from directory op transactions

Quoted 34 lines · Darrick J. Wong

Quoted 33 lines · Brian Foster
Directory operations can perform block allocations as entries are added/removed from directories. Defer AGFL block frees from the remaining directory operation transactions. This covers the hard link, remove and rename operations.
Signed-off-by Brian Foster <[email protected]>
---
 fs/xfs/xfs_inode.c | 3 +++
 1 file changed, 3 insertions(+)
fs/xfs/xfs_inode.c +2 -0
index 484ebef36fe4..47aa124e4744 100644
@@ -1452,6 +1452,7 @@ xfs_link(
 	}
 
 	xfs_defer_init(&dfops, &first_block);
+	tp->t_agfl_dfops = &dfops;
 
 	/*
 	 * Handle initial link state of O_TMPFILE inode
@@ -2649,6 +2650,7 @@ xfs_remove(
 		goto out_trans_cancel;
 
 	xfs_defer_init(&dfops, &first_block);
+	tp->t_agfl_dfops = &dfops;
 	error = xfs_dir_removename(tp, dp, name, ip->i_ino,
 					&first_block, &dfops, resblks);
 	if (error) {
@@ -3016,6 +3018,7 @@ xfs_rename(
 	}
 

Quoted 5 lines

fs/xfs/xfs_inode.c xfs_rename( · line 3021
Quoted 2 lines
fs/xfs/xfs_inode.c +1 -0
 	xfs_defer_init(&dfops, &first_block);
+	tp->t_agfl_dfops = &dfops;
Hmm, do you have a reproducer xfstest for any of the last three patches?

Unfortunately, no. The only reproducer I've managed so far was a (huge) private/customer metadump that consistently reproduced log reservation overruns over a long running file removal operation. Even then, that particular instance was worked around by the previous reworks of the inode transaction reservation calculations.

While we ended up with a workaround for that problem, it was still known that the agfl fixups were unpredictable and we couldn't guarantee coverage by the reservation calculations without making them excessively large. So this series aims to make the agfl fixup (freeing, specifically) behavior itself predictable such that it always fits in an allocation log reservation unit.

Quoted 3 lines

Codewise,
Reviewed-by Darrick J. Wong <[email protected]>

Thanks.. does this refer to just this patch, btw, or the previous 3?

Brian

Quoted 16 lines

--D
Quoted 10 lines
fs/xfs/xfs_inode.c +0 -0
 	/* RENAME_EXCHANGE is unique from here on. */
 	if (flags & RENAME_EXCHANGE)
Signature · 6 lines
2.13.6

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

Darrick J. Wong

2018-05-08 14:53 UTC #20 lore raw

Re: [PATCH v2 6/6] xfs: defer agfl frees from directory op transactions

Quoted 47 lines · Brian Foster

Quoted 34 lines · Darrick J. Wong
Quoted 33 lines · Brian Foster
Directory operations can perform block allocations as entries are added/removed from directories. Defer AGFL block frees from the remaining directory operation transactions. This covers the hard link, remove and rename operations.
Signed-off-by Brian Foster <[email protected]>
---
 fs/xfs/xfs_inode.c | 3 +++
 1 file changed, 3 insertions(+)
fs/xfs/xfs_inode.c +2 -0
index 484ebef36fe4..47aa124e4744 100644
@@ -1452,6 +1452,7 @@ xfs_link(
 	}
 
 	xfs_defer_init(&dfops, &first_block);
+	tp->t_agfl_dfops = &dfops;
 
 	/*
 	 * Handle initial link state of O_TMPFILE inode
@@ -2649,6 +2650,7 @@ xfs_remove(
 		goto out_trans_cancel;
 
 	xfs_defer_init(&dfops, &first_block);
+	tp->t_agfl_dfops = &dfops;
 	error = xfs_dir_removename(tp, dp, name, ip->i_ino,
 					&first_block, &dfops, resblks);
 	if (error) {
@@ -3016,6 +3018,7 @@ xfs_rename(
 	}
 
Quoted 5 lines
fs/xfs/xfs_inode.c xfs_rename( · line 3021
Quoted 2 lines
fs/xfs/xfs_inode.c +1 -0
 	xfs_defer_init(&dfops, &first_block);
+	tp->t_agfl_dfops = &dfops;
Hmm, do you have a reproducer xfstest for any of the last three patches?
Unfortunately, no. The only reproducer I've managed so far was a (huge) private/customer metadump that consistently reproduced log reservation overruns over a long running file removal operation. Even then, that particular instance was worked around by the previous reworks of the inode transaction reservation calculations.

Quoted 6 lines

While we ended up with a workaround for that problem, it was still known that the agfl fixups were unpredictable and we couldn't guarantee coverage by the reservation calculations without making them excessively large. So this series aims to make the agfl fixup (freeing, specifically) behavior itself predictable such that it always fits in an allocation log reservation unit.

<nod>

Quoted 5 lines

Quoted 3 lines
Codewise,
Reviewed-by Darrick J. Wong <[email protected]>
Thanks.. does this refer to just this patch, btw, or the previous 3?

All three.

--D

Quoted 22 lines

Brian
Quoted 8 lines
--D
Quoted 6 lines
fs/xfs/xfs_inode.c +0 -0
 	/* RENAME_EXCHANGE is unique from here on. */
 	if (flags & RENAME_EXCHANGE)
Signature · 1 line
2.13.6
Quoted 8 lines
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
--
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