xfs: Pull EFI/EFD handling out from under the AIL lock

EFI/EFD interactions are protected from races by the AIL lock. They are the only type of log items that require the the AIL lock to serialise internal state, so they need to be separated from the AIL lock before we can do bulk insert operations on the AIL. To acheive this, convert the counter of the number of extents in the EFI to an atomic so it can be safely manipulated by EFD processing without locks. Also, convert the EFI state flag manipulations to use atomic bit operations so no locks are needed to record state changes. Finally, use the state bits to determine when it is safe to free the EFI and clean up the code to do this neatly. Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Christoph Hellwig <hch@lst.de>

xfs: Pull EFI/EFD handling out from under the AIL lock
EFI/EFD interactions are protected from races by the AIL lock. They are the only type of log items that require the the AIL lock to serialise internal state, so they need to be separated from the AIL lock before we can do bulk insert operations on the AIL. To acheive this, convert the counter of the number of extents in the EFI to an atomic so it can be safely manipulated by EFD processing without locks. Also, convert the EFI state flag manipulations to use atomic bit operations so no locks are needed to record state changes. Finally, use the state bits to determine when it is safe to free the EFI and clean up the code to do this neatly. Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Christoph Hellwig <hch@lst.de>
Dave Chinner · Dave Chinner
1 parent 9c5f8414ef
Showing 4 changed files with 59 additions and 49 deletions Side-by-side Diff
fs/xfs/xfs_extfree_item.c
fs/xfs/xfs_extfree_item.h
fs/xfs/xfs_log_recover.c
fs/xfs/xfs_trans_extfree.c
@@ -48,6 +48,28 @@
 }
  
 /*
+ * Freeing the efi requires that we remove it from the AIL if it has already
+ * been placed there. However, the EFI may not yet have been placed in the AIL
+ * when called by xfs_efi_release() from EFD processing due to the ordering of
+ * committed vs unpin operations in bulk insert operations. Hence the
+ * test_and_clear_bit(XFS_EFI_COMMITTED) to ensure only the last caller frees
+ * the EFI.
+ */
+STATIC void
+__xfs_efi_release(
+	struct xfs_efi_log_item	*efip)
+{
+	struct xfs_ail		*ailp = efip->efi_item.li_ailp;
+
+	if (!test_and_clear_bit(XFS_EFI_COMMITTED, &efip->efi_flags)) {
+		spin_lock(&ailp->xa_lock);
+		/* xfs_trans_ail_delete() drops the AIL lock. */
+		xfs_trans_ail_delete(ailp, &efip->efi_item);
+		xfs_efi_item_free(efip);
+	}
+}
+
+/*
  * This returns the number of iovecs needed to log the given efi item.
  * We only need 1 iovec for an efi item.  It just logs the efi_log_format
  * structure.
@@ -74,7 +96,8 @@
 	struct xfs_efi_log_item	*efip = EFI_ITEM(lip);
 	uint			size;
  
-	ASSERT(efip->efi_next_extent == efip->efi_format.efi_nextents);
+	ASSERT(atomic_read(&efip->efi_next_extent) ==
+				efip->efi_format.efi_nextents);
  
 	efip->efi_format.efi_type = XFS_LI_EFI;
  
@@ -103,7 +126,8 @@
  * which the EFI is manipulated during a transaction.  If we are being asked to
  * remove the EFI it's because the transaction has been cancelled and by
  * definition that means the EFI cannot be in the AIL so remove it from the
- * transaction and free it.
+ * transaction and free it.  Otherwise coordinate with xfs_efi_release() (via
+ * XFS_EFI_COMMITTED) to determine who gets to free the EFI.
  */
 STATIC void
 xfs_efi_item_unpin(
  
  
  
@@ -111,17 +135,14 @@
 	int			remove)
 {
 	struct xfs_efi_log_item	*efip = EFI_ITEM(lip);
-	struct xfs_ail		*ailp = lip->li_ailp;
  
-	spin_lock(&ailp->xa_lock);
 	if (remove) {
 		ASSERT(!(lip->li_flags & XFS_LI_IN_AIL));
 		xfs_trans_del_item(lip);
 		xfs_efi_item_free(efip);
-	} else {
-		efip->efi_flags |= XFS_EFI_COMMITTED;
+		return;
 	}
-	spin_unlock(&ailp->xa_lock);
+	__xfs_efi_release(efip);
 }
  
 /*
  
@@ -150,16 +171,20 @@
 }
  
 /*
- * The EFI is logged only once and cannot be moved in the log, so
- * simply return the lsn at which it's been logged.  The canceled
- * flag is not paid any attention here.  Checking for that is delayed
- * until the EFI is unpinned.
+ * The EFI is logged only once and cannot be moved in the log, so simply return
+ * the lsn at which it's been logged.  For bulk transaction committed
+ * processing, the EFI may be processed but not yet unpinned prior to the EFD
+ * being processed. Set the XFS_EFI_COMMITTED flag so this case can be detected
+ * when processing the EFD.
  */
 STATIC xfs_lsn_t
 xfs_efi_item_committed(
 	struct xfs_log_item	*lip,
 	xfs_lsn_t		lsn)
 {
+	struct xfs_efi_log_item	*efip = EFI_ITEM(lip);
+
+	set_bit(XFS_EFI_COMMITTED, &efip->efi_flags);
 	return lsn;
 }
  
@@ -228,6 +253,7 @@
 	xfs_log_item_init(mp, &efip->efi_item, XFS_LI_EFI, &xfs_efi_item_ops);
 	efip->efi_format.efi_nextents = nextents;
 	efip->efi_format.efi_id = (__psint_t)(void*)efip;
+	atomic_set(&efip->efi_next_extent, 0);
  
 	return efip;
 }
  
@@ -287,37 +313,18 @@
 }
  
 /*
- * This is called by the efd item code below to release references to
- * the given efi item.  Each efd calls this with the number of
- * extents that it has logged, and when the sum of these reaches
- * the total number of extents logged by this efi item we can free
- * the efi item.
- *
- * Freeing the efi item requires that we remove it from the AIL.
- * We'll use the AIL lock to protect our counters as well as
- * the removal from the AIL.
+ * This is called by the efd item code below to release references to the given
+ * efi item.  Each efd calls this with the number of extents that it has
+ * logged, and when the sum of these reaches the total number of extents logged
+ * by this efi item we can free the efi item.
  */
 void
 xfs_efi_release(xfs_efi_log_item_t	*efip,
 		uint			nextents)
 {
-	struct xfs_ail		*ailp = efip->efi_item.li_ailp;
-	int			extents_left;
-
-	ASSERT(efip->efi_next_extent > 0);
-	ASSERT(efip->efi_flags & XFS_EFI_COMMITTED);
-
-	spin_lock(&ailp->xa_lock);
-	ASSERT(efip->efi_next_extent >= nextents);
-	efip->efi_next_extent -= nextents;
-	extents_left = efip->efi_next_extent;
-	if (extents_left == 0) {
-		/* xfs_trans_ail_delete() drops the AIL lock. */
-		xfs_trans_ail_delete(ailp, (xfs_log_item_t *)efip);
-		xfs_efi_item_free(efip);
-	} else {
-		spin_unlock(&ailp->xa_lock);
-	}
+	ASSERT(atomic_read(&efip->efi_next_extent) >= nextents);
+	if (atomic_sub_and_test(nextents, &efip->efi_next_extent))
+		__xfs_efi_release(efip);
 }
  
 static inline struct xfs_efd_log_item *EFD_ITEM(struct xfs_log_item *lip)
@@ -111,10 +111,10 @@
 #define	XFS_EFI_MAX_FAST_EXTENTS	16
  
 /*
- * Define EFI flags.
+ * Define EFI flag bits. Manipulated by set/clear/test_bit operators.
  */
-#define	XFS_EFI_RECOVERED	0x1
-#define	XFS_EFI_COMMITTED	0x2
+#define	XFS_EFI_RECOVERED	1
+#define	XFS_EFI_COMMITTED	2
  
 /*
  * This is the "extent free intention" log item.  It is used
@@ -124,8 +124,8 @@
  */
 typedef struct xfs_efi_log_item {
 	xfs_log_item_t		efi_item;
-	uint			efi_flags;	/* misc flags */
-	uint			efi_next_extent;
+	atomic_t		efi_next_extent;
+	unsigned long		efi_flags;	/* misc flags */
 	xfs_efi_log_format_t	efi_format;
 } xfs_efi_log_item_t;
  
@@ -2567,8 +2567,7 @@
 		xfs_efi_item_free(efip);
 		return error;
 	}
-	efip->efi_next_extent = efi_formatp->efi_nextents;
-	efip->efi_flags |= XFS_EFI_COMMITTED;
+	atomic_set(&efip->efi_next_extent, efi_formatp->efi_nextents);
  
 	spin_lock(&log->l_ailp->xa_lock);
 	/*
@@ -2878,7 +2877,7 @@
 	xfs_extent_t		*extp;
 	xfs_fsblock_t		startblock_fsb;
  
-	ASSERT(!(efip->efi_flags & XFS_EFI_RECOVERED));
+	ASSERT(!test_bit(XFS_EFI_RECOVERED, &efip->efi_flags));
  
 	/*
 	 * First check the validity of the extents described by the
@@ -2917,7 +2916,7 @@
 					 extp->ext_len);
 	}
  
-	efip->efi_flags |= XFS_EFI_RECOVERED;
+	set_bit(XFS_EFI_RECOVERED, &efip->efi_flags);
 	error = xfs_trans_commit(tp, 0);
 	return error;
  
@@ -2974,7 +2973,7 @@
 		 * Skip EFIs that we've already processed.
 		 */
 		efip = (xfs_efi_log_item_t *)lip;
-		if (efip->efi_flags & XFS_EFI_RECOVERED) {
+		if (test_bit(XFS_EFI_RECOVERED, &efip->efi_flags)) {
 			lip = xfs_trans_ail_cursor_next(ailp, &cur);
 			continue;
 		}
@@ -69,12 +69,16 @@
 	tp->t_flags |= XFS_TRANS_DIRTY;
 	efip->efi_item.li_desc->lid_flags |= XFS_LID_DIRTY;
  
-	next_extent = efip->efi_next_extent;
+	/*
+	 * atomic_inc_return gives us the value after the increment;
+	 * we want to use it as an array index so we need to subtract 1 from
+	 * it.
+	 */
+	next_extent = atomic_inc_return(&efip->efi_next_extent) - 1;
 	ASSERT(next_extent < efip->efi_format.efi_nextents);
 	extp = &(efip->efi_format.efi_extents[next_extent]);
 	extp->ext_start = start_block;
 	extp->ext_len = ext_len;
-	efip->efi_next_extent++;
 }
...	...	@@ -48,6 +48,28 @@
48	48	}
49	49
50	50	/*
	51	+ * Freeing the efi requires that we remove it from the AIL if it has already
	52	+ * been placed there. However, the EFI may not yet have been placed in the AIL
	53	+ * when called by xfs_efi_release() from EFD processing due to the ordering of
	54	+ * committed vs unpin operations in bulk insert operations. Hence the
	55	+ * test_and_clear_bit(XFS_EFI_COMMITTED) to ensure only the last caller frees
	56	+ * the EFI.
	57	+ */
	58	+STATIC void
	59	+__xfs_efi_release(
	60	+ struct xfs_efi_log_item *efip)
	61	+{
	62	+ struct xfs_ail *ailp = efip->efi_item.li_ailp;
	63	+
	64	+ if (!test_and_clear_bit(XFS_EFI_COMMITTED, &efip->efi_flags)) {
	65	+ spin_lock(&ailp->xa_lock);
	66	+ /* xfs_trans_ail_delete() drops the AIL lock. */
	67	+ xfs_trans_ail_delete(ailp, &efip->efi_item);
	68	+ xfs_efi_item_free(efip);
	69	+ }
	70	+}
	71	+
	72	+/*
51	73	* This returns the number of iovecs needed to log the given efi item.
52	74	* We only need 1 iovec for an efi item. It just logs the efi_log_format
53	75	* structure.
...	...	@@ -74,7 +96,8 @@
74	96	struct xfs_efi_log_item *efip = EFI_ITEM(lip);
75	97	uint size;
76	98
77		- ASSERT(efip->efi_next_extent == efip->efi_format.efi_nextents);
	99	+ ASSERT(atomic_read(&efip->efi_next_extent) ==
	100	+ efip->efi_format.efi_nextents);
78	101
79	102	efip->efi_format.efi_type = XFS_LI_EFI;
80	103
...	...	@@ -103,7 +126,8 @@
103	126	* which the EFI is manipulated during a transaction. If we are being asked to
104	127	* remove the EFI it's because the transaction has been cancelled and by
105	128	* definition that means the EFI cannot be in the AIL so remove it from the
106		- * transaction and free it.
	129	+ * transaction and free it. Otherwise coordinate with xfs_efi_release() (via
	130	+ * XFS_EFI_COMMITTED) to determine who gets to free the EFI.
107	131	*/
108	132	STATIC void
109	133	xfs_efi_item_unpin(
110	134
111	135
112	136
...	...	@@ -111,17 +135,14 @@
111	135	int remove)
112	136	{
113	137	struct xfs_efi_log_item *efip = EFI_ITEM(lip);
114		- struct xfs_ail *ailp = lip->li_ailp;
115	138
116		- spin_lock(&ailp->xa_lock);
117	139	if (remove) {
118	140	ASSERT(!(lip->li_flags & XFS_LI_IN_AIL));
119	141	xfs_trans_del_item(lip);
120	142	xfs_efi_item_free(efip);
121		- } else {
122		- efip->efi_flags \|= XFS_EFI_COMMITTED;
	143	+ return;
123	144	}
124		- spin_unlock(&ailp->xa_lock);
	145	+ __xfs_efi_release(efip);
125	146	}
126	147
127	148	/*
128	149
...	...	@@ -150,16 +171,20 @@
150	171	}
151	172
152	173	/*
153		- * The EFI is logged only once and cannot be moved in the log, so
154		- * simply return the lsn at which it's been logged. The canceled
155		- * flag is not paid any attention here. Checking for that is delayed
156		- * until the EFI is unpinned.
	174	+ * The EFI is logged only once and cannot be moved in the log, so simply return
	175	+ * the lsn at which it's been logged. For bulk transaction committed
	176	+ * processing, the EFI may be processed but not yet unpinned prior to the EFD
	177	+ * being processed. Set the XFS_EFI_COMMITTED flag so this case can be detected
	178	+ * when processing the EFD.
157	179	*/
158	180	STATIC xfs_lsn_t
159	181	xfs_efi_item_committed(
160	182	struct xfs_log_item *lip,
161	183	xfs_lsn_t lsn)
162	184	{
	185	+ struct xfs_efi_log_item *efip = EFI_ITEM(lip);
	186	+
	187	+ set_bit(XFS_EFI_COMMITTED, &efip->efi_flags);
163	188	return lsn;
164	189	}
165	190
...	...	@@ -228,6 +253,7 @@
228	253	xfs_log_item_init(mp, &efip->efi_item, XFS_LI_EFI, &xfs_efi_item_ops);
229	254	efip->efi_format.efi_nextents = nextents;
230	255	efip->efi_format.efi_id = (__psint_t)(void*)efip;
	256	+ atomic_set(&efip->efi_next_extent, 0);
231	257
232	258	return efip;
233	259	}
234	260
...	...	@@ -287,37 +313,18 @@
287	313	}
288	314
289	315	/*
290		- * This is called by the efd item code below to release references to
291		- * the given efi item. Each efd calls this with the number of
292		- * extents that it has logged, and when the sum of these reaches
293		- * the total number of extents logged by this efi item we can free
294		- * the efi item.
295		- *
296		- * Freeing the efi item requires that we remove it from the AIL.
297		- * We'll use the AIL lock to protect our counters as well as
298		- * the removal from the AIL.
	316	+ * This is called by the efd item code below to release references to the given
	317	+ * efi item. Each efd calls this with the number of extents that it has
	318	+ * logged, and when the sum of these reaches the total number of extents logged
	319	+ * by this efi item we can free the efi item.
299	320	*/
300	321	void
301	322	xfs_efi_release(xfs_efi_log_item_t *efip,
302	323	uint nextents)
303	324	{
304		- struct xfs_ail *ailp = efip->efi_item.li_ailp;
305		- int extents_left;
306		-
307		- ASSERT(efip->efi_next_extent > 0);
308		- ASSERT(efip->efi_flags & XFS_EFI_COMMITTED);
309		-
310		- spin_lock(&ailp->xa_lock);
311		- ASSERT(efip->efi_next_extent >= nextents);
312		- efip->efi_next_extent -= nextents;
313		- extents_left = efip->efi_next_extent;
314		- if (extents_left == 0) {
315		- /* xfs_trans_ail_delete() drops the AIL lock. */
316		- xfs_trans_ail_delete(ailp, (xfs_log_item_t *)efip);
317		- xfs_efi_item_free(efip);
318		- } else {
319		- spin_unlock(&ailp->xa_lock);
320		- }
	325	+ ASSERT(atomic_read(&efip->efi_next_extent) >= nextents);
	326	+ if (atomic_sub_and_test(nextents, &efip->efi_next_extent))
	327	+ __xfs_efi_release(efip);
321	328	}
322	329
323	330	static inline struct xfs_efd_log_item EFD_ITEM(struct xfs_log_item lip)
...	...	@@ -111,10 +111,10 @@
111	111	#define XFS_EFI_MAX_FAST_EXTENTS 16
112	112
113	113	/*
114		- * Define EFI flags.
	114	+ * Define EFI flag bits. Manipulated by set/clear/test_bit operators.
115	115	*/
116		-#define XFS_EFI_RECOVERED 0x1
117		-#define XFS_EFI_COMMITTED 0x2
	116	+#define XFS_EFI_RECOVERED 1
	117	+#define XFS_EFI_COMMITTED 2
118	118
119	119	/*
120	120	* This is the "extent free intention" log item. It is used
...	...	@@ -124,8 +124,8 @@
124	124	*/
125	125	typedef struct xfs_efi_log_item {
126	126	xfs_log_item_t efi_item;
127		- uint efi_flags; /* misc flags */
128		- uint efi_next_extent;
	127	+ atomic_t efi_next_extent;
	128	+ unsigned long efi_flags; /* misc flags */
129	129	xfs_efi_log_format_t efi_format;
130	130	} xfs_efi_log_item_t;
131	131
...	...	@@ -2567,8 +2567,7 @@
2567	2567	xfs_efi_item_free(efip);
2568	2568	return error;
2569	2569	}
2570		- efip->efi_next_extent = efi_formatp->efi_nextents;
2571		- efip->efi_flags \|= XFS_EFI_COMMITTED;
	2570	+ atomic_set(&efip->efi_next_extent, efi_formatp->efi_nextents);
2572	2571
2573	2572	spin_lock(&log->l_ailp->xa_lock);
2574	2573	/*
...	...	@@ -2878,7 +2877,7 @@
2878	2877	xfs_extent_t *extp;
2879	2878	xfs_fsblock_t startblock_fsb;
2880	2879
2881		- ASSERT(!(efip->efi_flags & XFS_EFI_RECOVERED));
	2880	+ ASSERT(!test_bit(XFS_EFI_RECOVERED, &efip->efi_flags));
2882	2881
2883	2882	/*
2884	2883	* First check the validity of the extents described by the
...	...	@@ -2917,7 +2916,7 @@
2917	2916	extp->ext_len);
2918	2917	}
2919	2918
2920		- efip->efi_flags \|= XFS_EFI_RECOVERED;
	2919	+ set_bit(XFS_EFI_RECOVERED, &efip->efi_flags);
2921	2920	error = xfs_trans_commit(tp, 0);
2922	2921	return error;
2923	2922
...	...	@@ -2974,7 +2973,7 @@
2974	2973	* Skip EFIs that we've already processed.
2975	2974	*/
2976	2975	efip = (xfs_efi_log_item_t *)lip;
2977		- if (efip->efi_flags & XFS_EFI_RECOVERED) {
	2976	+ if (test_bit(XFS_EFI_RECOVERED, &efip->efi_flags)) {
2978	2977	lip = xfs_trans_ail_cursor_next(ailp, &cur);
2979	2978	continue;
2980	2979	}
...	...	@@ -69,12 +69,16 @@
69	69	tp->t_flags \|= XFS_TRANS_DIRTY;
70	70	efip->efi_item.li_desc->lid_flags \|= XFS_LID_DIRTY;
71	71
72		- next_extent = efip->efi_next_extent;
	72	+ /*
	73	+ * atomic_inc_return gives us the value after the increment;
	74	+ * we want to use it as an array index so we need to subtract 1 from
	75	+ * it.
	76	+ */
	77	+ next_extent = atomic_inc_return(&efip->efi_next_extent) - 1;
73	78	ASSERT(next_extent < efip->efi_format.efi_nextents);
74	79	extp = &(efip->efi_format.efi_extents[next_extent]);
75	80	extp->ext_start = start_block;
76	81	extp->ext_len = ext_len;
77		- efip->efi_next_extent++;
78	82	}
79	83
80	84