diff mbox series

[15/19] xfs: refactor releasing finished intents during log recovery

Message ID 158752125867.2140829.718007064092831514.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs: refactor log recovery | expand

Commit Message

Darrick J. Wong April 22, 2020, 2:07 a.m. UTC
From: Darrick J. Wong <darrick.wong@oracle.com>

Replace the open-coded AIL item walking with a proper helper when we're
trying to release an intent item that has been finished.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_log_recover.h |    5 +++
 fs/xfs/xfs_bmap_item.c          |   56 +++++++++++++++++----------------------
 fs/xfs/xfs_extfree_item.c       |   56 +++++++++++++++++----------------------
 fs/xfs/xfs_log_recover.c        |   27 +++++++++++++++++++
 fs/xfs/xfs_refcount_item.c      |   56 +++++++++++++++++----------------------
 fs/xfs/xfs_rmap_item.c          |   56 +++++++++++++++++----------------------
 6 files changed, 128 insertions(+), 128 deletions(-)

Comments

Christoph Hellwig April 25, 2020, 6:34 p.m. UTC | #1
> +STATIC bool
> +xlog_release_bui(
> +	struct xlog		*log,
> +	struct xfs_log_item	*lip,
> +	uint64_t		intent_id)
> +{
> +	struct xfs_bui_log_item	*buip = BUI_ITEM(lip);
> +	struct xfs_ail		*ailp = log->l_ailp;
> +
> +	if (buip->bui_format.bui_id == intent_id) {
> +		/*
> +		 * Drop the BUD reference to the BUI. This
> +		 * removes the BUI from the AIL and frees it.
> +		 */
> +		spin_unlock(&ailp->ail_lock);
> +		xfs_bui_release(buip);
> +		spin_lock(&ailp->ail_lock);
> +		return true;
> +	}
> +
> +	return false;

Early returns for the no match case would seem a little more clear for
all the callbacks.  Also the boilerplate comments in all the callback
seem rather pointless.  If you think they have enough value I'd
consolidate that information once in the xlog_recover_release_intent
description.

> +	spin_lock(&ailp->ail_lock);
> +	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
> +	while (lip != NULL) {
> +		if (lip->li_type == intent_type && fn(log, lip, intent_id))
> +			break;
> +		lip = xfs_trans_ail_cursor_next(ailp, &cur);
> +	}

What about:

	for (lip = xfs_trans_ail_cursor_first(ailp, &cur, 0); lip;
	     lip = xfs_trans_ail_cursor_next(ailp, &cur) {
		if (lip->li_type != intent_type)
			continue;
		if (fn(log, lip, intent_id))
			break;
	}
Darrick J. Wong April 28, 2020, 10:40 p.m. UTC | #2
On Sat, Apr 25, 2020 at 11:34:10AM -0700, Christoph Hellwig wrote:
> > +STATIC bool
> > +xlog_release_bui(
> > +	struct xlog		*log,
> > +	struct xfs_log_item	*lip,
> > +	uint64_t		intent_id)
> > +{
> > +	struct xfs_bui_log_item	*buip = BUI_ITEM(lip);
> > +	struct xfs_ail		*ailp = log->l_ailp;
> > +
> > +	if (buip->bui_format.bui_id == intent_id) {
> > +		/*
> > +		 * Drop the BUD reference to the BUI. This
> > +		 * removes the BUI from the AIL and frees it.
> > +		 */
> > +		spin_unlock(&ailp->ail_lock);
> > +		xfs_bui_release(buip);
> > +		spin_lock(&ailp->ail_lock);
> > +		return true;
> > +	}
> > +
> > +	return false;
> 
> Early returns for the no match case would seem a little more clear for
> all the callbacks.

Done.

> Also the boilerplate comments in all the callback
> seem rather pointless.  If you think they have enough value I'd
> consolidate that information once in the xlog_recover_release_intent
> description.

Yeah, I'll move it.

> 
> > +	spin_lock(&ailp->ail_lock);
> > +	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
> > +	while (lip != NULL) {
> > +		if (lip->li_type == intent_type && fn(log, lip, intent_id))
> > +			break;
> > +		lip = xfs_trans_ail_cursor_next(ailp, &cur);
> > +	}
> 
> What about:
> 
> 	for (lip = xfs_trans_ail_cursor_first(ailp, &cur, 0); lip;
> 	     lip = xfs_trans_ail_cursor_next(ailp, &cur) {
> 		if (lip->li_type != intent_type)
> 			continue;
> 		if (fn(log, lip, intent_id))
> 			break;
> 	}

Done.

--D
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h
index 5bdba7ee98c5..ac1adccc8451 100644
--- a/fs/xfs/libxfs/xfs_log_recover.h
+++ b/fs/xfs/libxfs/xfs_log_recover.h
@@ -169,4 +169,9 @@  extern const struct xlog_recover_intent_type xlog_recover_rmap_type;
 extern const struct xlog_recover_intent_type xlog_recover_refcount_type;
 extern const struct xlog_recover_intent_type xlog_recover_bmap_type;
 
+typedef bool (*xlog_recover_release_intent_fn)(struct xlog *log,
+		struct xfs_log_item *item, uint64_t intent_id);
+void xlog_recover_release_intent(struct xlog *log, unsigned short intent_type,
+		uint64_t intent_id, xlog_recover_release_intent_fn fn);
+
 #endif	/* __XFS_LOG_RECOVER_H__ */
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index b4fbc58b6906..cd593b98f102 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -634,6 +634,28 @@  xlog_recover_bui(
 	return 0;
 }
 
+STATIC bool
+xlog_release_bui(
+	struct xlog		*log,
+	struct xfs_log_item	*lip,
+	uint64_t		intent_id)
+{
+	struct xfs_bui_log_item	*buip = BUI_ITEM(lip);
+	struct xfs_ail		*ailp = log->l_ailp;
+
+	if (buip->bui_format.bui_id == intent_id) {
+		/*
+		 * Drop the BUD reference to the BUI. This
+		 * removes the BUI from the AIL and frees it.
+		 */
+		spin_unlock(&ailp->ail_lock);
+		xfs_bui_release(buip);
+		spin_lock(&ailp->ail_lock);
+		return true;
+	}
+
+	return false;
+}
 
 /*
  * This routine is called when an BUD format structure is found in a committed
@@ -648,45 +670,15 @@  xlog_recover_bud(
 	struct xlog_recover_item	*item)
 {
 	struct xfs_bud_log_format	*bud_formatp;
-	struct xfs_bui_log_item		*buip = NULL;
-	struct xfs_log_item		*lip;
-	uint64_t			bui_id;
-	struct xfs_ail_cursor		cur;
-	struct xfs_ail			*ailp = log->l_ailp;
 
 	bud_formatp = item->ri_buf[0].i_addr;
 	if (item->ri_buf[0].i_len != sizeof(struct xfs_bud_log_format)) {
 		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
 		return -EFSCORRUPTED;
 	}
-	bui_id = bud_formatp->bud_bui_id;
-
-	/*
-	 * Search for the BUI with the id in the BUD format structure in the
-	 * AIL.
-	 */
-	spin_lock(&ailp->ail_lock);
-	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
-	while (lip != NULL) {
-		if (lip->li_type == XFS_LI_BUI) {
-			buip = (struct xfs_bui_log_item *)lip;
-			if (buip->bui_format.bui_id == bui_id) {
-				/*
-				 * Drop the BUD reference to the BUI. This
-				 * removes the BUI from the AIL and frees it.
-				 */
-				spin_unlock(&ailp->ail_lock);
-				xfs_bui_release(buip);
-				spin_lock(&ailp->ail_lock);
-				break;
-			}
-		}
-		lip = xfs_trans_ail_cursor_next(ailp, &cur);
-	}
-
-	xfs_trans_ail_cursor_done(&cur);
-	spin_unlock(&ailp->ail_lock);
 
+	xlog_recover_release_intent(log, XFS_LI_BUI, bud_formatp->bud_bui_id,
+			 xlog_release_bui);
 	return 0;
 }
 
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index cd2beb581b40..6a873309f3bc 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -695,6 +695,28 @@  xlog_recover_efi(
 	return 0;
 }
 
+STATIC bool
+xlog_release_efi(
+	struct xlog		*log,
+	struct xfs_log_item	*lip,
+	uint64_t		intent_id)
+{
+	struct xfs_efi_log_item	*efip = EFI_ITEM(lip);
+	struct xfs_ail		*ailp = log->l_ailp;
+
+	if (efip->efi_format.efi_id == intent_id) {
+		/*
+		 * Drop the EFD reference to the EFI. This
+		 * removes the EFI from the AIL and frees it.
+		 */
+		spin_unlock(&ailp->ail_lock);
+		xfs_efi_release(efip);
+		spin_lock(&ailp->ail_lock);
+		return true;
+	}
+
+	return false;
+}
 
 /*
  * This routine is called when an EFD format structure is found in a committed
@@ -708,46 +730,16 @@  xlog_recover_efd(
 	struct xlog			*log,
 	struct xlog_recover_item	*item)
 {
-	struct xfs_ail_cursor		cur;
 	struct xfs_efd_log_format	*efd_formatp;
-	struct xfs_efi_log_item		*efip = NULL;
-	struct xfs_log_item		*lip;
-	struct xfs_ail			*ailp = log->l_ailp;
-	uint64_t			efi_id;
 
 	efd_formatp = item->ri_buf[0].i_addr;
 	ASSERT((item->ri_buf[0].i_len == (sizeof(xfs_efd_log_format_32_t) +
 		((efd_formatp->efd_nextents - 1) * sizeof(xfs_extent_32_t)))) ||
 	       (item->ri_buf[0].i_len == (sizeof(xfs_efd_log_format_64_t) +
 		((efd_formatp->efd_nextents - 1) * sizeof(xfs_extent_64_t)))));
-	efi_id = efd_formatp->efd_efi_id;
-
-	/*
-	 * Search for the EFI with the id in the EFD format structure in the
-	 * AIL.
-	 */
-	spin_lock(&ailp->ail_lock);
-	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
-	while (lip != NULL) {
-		if (lip->li_type == XFS_LI_EFI) {
-			efip = (xfs_efi_log_item_t *)lip;
-			if (efip->efi_format.efi_id == efi_id) {
-				/*
-				 * Drop the EFD reference to the EFI. This
-				 * removes the EFI from the AIL and frees it.
-				 */
-				spin_unlock(&ailp->ail_lock);
-				xfs_efi_release(efip);
-				spin_lock(&ailp->ail_lock);
-				break;
-			}
-		}
-		lip = xfs_trans_ail_cursor_next(ailp, &cur);
-	}
-
-	xfs_trans_ail_cursor_done(&cur);
-	spin_unlock(&ailp->ail_lock);
 
+	xlog_recover_release_intent(log, XFS_LI_EFI, efd_formatp->efd_efi_id,
+			 xlog_release_efi);
 	return 0;
 }
 
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 08066fa32b80..460f836de963 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1770,6 +1770,33 @@  xlog_clear_stale_blocks(
 
 /* Log intent item dispatching. */
 
+/*
+ * Release the recovered intent item in the AIL that matches the given intent
+ * type and intent id.
+ */
+void
+xlog_recover_release_intent(
+	struct xlog		*log,
+	unsigned short		intent_type,
+	uint64_t		intent_id,
+	xlog_recover_release_intent_fn	fn)
+{
+	struct xfs_ail_cursor	cur;
+	struct xfs_log_item	*lip;
+	struct xfs_ail		*ailp = log->l_ailp;
+
+	spin_lock(&ailp->ail_lock);
+	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
+	while (lip != NULL) {
+		if (lip->li_type == intent_type && fn(log, lip, intent_id))
+			break;
+		lip = xfs_trans_ail_cursor_next(ailp, &cur);
+	}
+
+	xfs_trans_ail_cursor_done(&cur);
+	spin_unlock(&ailp->ail_lock);
+}
+
 STATIC int xlog_recover_intent_pass2(struct xlog *log,
 		struct list_head *buffer_list, struct xlog_recover_item *item,
 		xfs_lsn_t current_lsn);
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index a1dac3d60a2a..6eef1523078c 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -657,6 +657,28 @@  xlog_recover_cui(
 	return 0;
 }
 
+STATIC bool
+xlog_release_cui(
+	struct xlog		*log,
+	struct xfs_log_item	*lip,
+	uint64_t		intent_id)
+{
+	struct xfs_cui_log_item	*cuip = CUI_ITEM(lip);
+	struct xfs_ail		*ailp = log->l_ailp;
+
+	if (cuip->cui_format.cui_id == intent_id) {
+		/*
+		 * Drop the CUD reference to the CUI. This
+		 * removes the CUI from the AIL and frees it.
+		 */
+		spin_unlock(&ailp->ail_lock);
+		xfs_cui_release(cuip);
+		spin_lock(&ailp->ail_lock);
+		return true;
+	}
+
+	return false;
+}
 
 /*
  * This routine is called when an CUD format structure is found in a committed
@@ -671,45 +693,15 @@  xlog_recover_cud(
 	struct xlog_recover_item	*item)
 {
 	struct xfs_cud_log_format	*cud_formatp;
-	struct xfs_cui_log_item		*cuip = NULL;
-	struct xfs_log_item		*lip;
-	uint64_t			cui_id;
-	struct xfs_ail_cursor		cur;
-	struct xfs_ail			*ailp = log->l_ailp;
 
 	cud_formatp = item->ri_buf[0].i_addr;
 	if (item->ri_buf[0].i_len != sizeof(struct xfs_cud_log_format)) {
 		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
 		return -EFSCORRUPTED;
 	}
-	cui_id = cud_formatp->cud_cui_id;
-
-	/*
-	 * Search for the CUI with the id in the CUD format structure in the
-	 * AIL.
-	 */
-	spin_lock(&ailp->ail_lock);
-	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
-	while (lip != NULL) {
-		if (lip->li_type == XFS_LI_CUI) {
-			cuip = (struct xfs_cui_log_item *)lip;
-			if (cuip->cui_format.cui_id == cui_id) {
-				/*
-				 * Drop the CUD reference to the CUI. This
-				 * removes the CUI from the AIL and frees it.
-				 */
-				spin_unlock(&ailp->ail_lock);
-				xfs_cui_release(cuip);
-				spin_lock(&ailp->ail_lock);
-				break;
-			}
-		}
-		lip = xfs_trans_ail_cursor_next(ailp, &cur);
-	}
-
-	xfs_trans_ail_cursor_done(&cur);
-	spin_unlock(&ailp->ail_lock);
 
+	xlog_recover_release_intent(log, XFS_LI_CUI, cud_formatp->cud_cui_id,
+			 xlog_release_cui);
 	return 0;
 }
 
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 1ef752563f37..b60fb141c22e 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -650,6 +650,28 @@  xlog_recover_rui(
 	return 0;
 }
 
+STATIC bool
+xlog_release_rui(
+	struct xlog		*log,
+	struct xfs_log_item	*lip,
+	uint64_t		intent_id)
+{
+	struct xfs_rui_log_item	*ruip = RUI_ITEM(lip);
+	struct xfs_ail		*ailp = log->l_ailp;
+
+	if (ruip->rui_format.rui_id == intent_id) {
+		/*
+		 * Drop the RUD reference to the RUI. This
+		 * removes the RUI from the AIL and frees it.
+		 */
+		spin_unlock(&ailp->ail_lock);
+		xfs_rui_release(ruip);
+		spin_lock(&ailp->ail_lock);
+		return true;
+	}
+
+	return false;
+}
 
 /*
  * This routine is called when an RUD format structure is found in a committed
@@ -664,42 +686,12 @@  xlog_recover_rud(
 	struct xlog_recover_item	*item)
 {
 	struct xfs_rud_log_format	*rud_formatp;
-	struct xfs_rui_log_item		*ruip = NULL;
-	struct xfs_log_item		*lip;
-	uint64_t			rui_id;
-	struct xfs_ail_cursor		cur;
-	struct xfs_ail			*ailp = log->l_ailp;
 
 	rud_formatp = item->ri_buf[0].i_addr;
 	ASSERT(item->ri_buf[0].i_len == sizeof(struct xfs_rud_log_format));
-	rui_id = rud_formatp->rud_rui_id;
-
-	/*
-	 * Search for the RUI with the id in the RUD format structure in the
-	 * AIL.
-	 */
-	spin_lock(&ailp->ail_lock);
-	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
-	while (lip != NULL) {
-		if (lip->li_type == XFS_LI_RUI) {
-			ruip = (struct xfs_rui_log_item *)lip;
-			if (ruip->rui_format.rui_id == rui_id) {
-				/*
-				 * Drop the RUD reference to the RUI. This
-				 * removes the RUI from the AIL and frees it.
-				 */
-				spin_unlock(&ailp->ail_lock);
-				xfs_rui_release(ruip);
-				spin_lock(&ailp->ail_lock);
-				break;
-			}
-		}
-		lip = xfs_trans_ail_cursor_next(ailp, &cur);
-	}
-
-	xfs_trans_ail_cursor_done(&cur);
-	spin_unlock(&ailp->ail_lock);
 
+	xlog_recover_release_intent(log, XFS_LI_RUI, rud_formatp->rud_rui_id,
+			 xlog_release_rui);
 	return 0;
 }