[17/28] xfs: refactor recovered BUI log item playback
diff mbox series

Message ID 158864114272.182683.11138860973756666002.stgit@magnolia
State Superseded
Headers show
Series
  • xfs: refactor log recovery
Related show

Commit Message

Darrick J. Wong May 5, 2020, 1:12 a.m. UTC
From: Darrick J. Wong <darrick.wong@oracle.com>

Move the code that processes the log items created from the recovered
log items into the per-item source code files and use dispatch functions
to call them.  No functional changes.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_bmap_item.c   |   44 ++++++++++++++++++----
 fs/xfs/xfs_bmap_item.h   |    3 --
 fs/xfs/xfs_log_recover.c |   91 ++++++----------------------------------------
 3 files changed, 47 insertions(+), 91 deletions(-)

Comments

Chandan Babu R May 5, 2020, 9:49 a.m. UTC | #1
On Tuesday 5 May 2020 6:42:22 AM IST Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Move the code that processes the log items created from the recovered
> log items into the per-item source code files and use dispatch functions
> to call them.  No functional changes.
>

BUI log item playback is consistent with what was done before the patch is
applied.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_bmap_item.c   |   44 ++++++++++++++++++----
>  fs/xfs/xfs_bmap_item.h   |    3 --
>  fs/xfs/xfs_log_recover.c |   91 ++++++----------------------------------------
>  3 files changed, 47 insertions(+), 91 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index 0fbebef69e26..f88ebf8634c4 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -28,6 +28,8 @@
>  kmem_zone_t	*xfs_bui_zone;
>  kmem_zone_t	*xfs_bud_zone;
>  
> +static const struct xfs_item_ops xfs_bui_item_ops;
> +
>  static inline struct xfs_bui_log_item *BUI_ITEM(struct xfs_log_item *lip)
>  {
>  	return container_of(lip, struct xfs_bui_log_item, bui_item);
> @@ -47,7 +49,7 @@ xfs_bui_item_free(
>   * committed vs unpin operations in bulk insert operations. Hence the reference
>   * count to ensure only the last caller frees the BUI.
>   */
> -void
> +STATIC void
>  xfs_bui_release(
>  	struct xfs_bui_log_item	*buip)
>  {
> @@ -126,13 +128,6 @@ xfs_bui_item_release(
>  	xfs_bui_release(BUI_ITEM(lip));
>  }
>  
> -static const struct xfs_item_ops xfs_bui_item_ops = {
> -	.iop_size	= xfs_bui_item_size,
> -	.iop_format	= xfs_bui_item_format,
> -	.iop_unpin	= xfs_bui_item_unpin,
> -	.iop_release	= xfs_bui_item_release,
> -};
> -
>  /*
>   * Allocate and initialize an bui item with the given number of extents.
>   */
> @@ -425,7 +420,7 @@ const struct xfs_defer_op_type xfs_bmap_update_defer_type = {
>   * Process a bmap update intent item that was recovered from the log.
>   * We need to update some inode's bmbt.
>   */
> -int
> +STATIC int
>  xfs_bui_recover(
>  	struct xfs_trans		*parent_tp,
>  	struct xfs_bui_log_item		*buip)
> @@ -560,6 +555,37 @@ xfs_bui_recover(
>  	return error;
>  }
>  
> +/* Recover the BUI if necessary. */
> +STATIC int
> +xfs_bui_item_recover(
> +	struct xfs_log_item		*lip,
> +	struct xfs_trans		*tp)
> +{
> +	struct xfs_ail			*ailp = lip->li_ailp;
> +	struct xfs_bui_log_item		*buip = BUI_ITEM(lip);
> +	int				error;
> +
> +	/*
> +	 * Skip BUIs that we've already processed.
> +	 */
> +	if (test_bit(XFS_BUI_RECOVERED, &buip->bui_flags))
> +		return 0;
> +
> +	spin_unlock(&ailp->ail_lock);
> +	error = xfs_bui_recover(tp, buip);
> +	spin_lock(&ailp->ail_lock);
> +
> +	return error;
> +}
> +
> +static const struct xfs_item_ops xfs_bui_item_ops = {
> +	.iop_size	= xfs_bui_item_size,
> +	.iop_format	= xfs_bui_item_format,
> +	.iop_unpin	= xfs_bui_item_unpin,
> +	.iop_release	= xfs_bui_item_release,
> +	.iop_recover	= xfs_bui_item_recover,
> +};
> +
>  /*
>   * Copy an BUI format buffer from the given buf, and into the destination
>   * BUI format structure.  The BUI/BUD items were designed not to need any
> diff --git a/fs/xfs/xfs_bmap_item.h b/fs/xfs/xfs_bmap_item.h
> index 515b1d5d6ab7..44d06e62f8f9 100644
> --- a/fs/xfs/xfs_bmap_item.h
> +++ b/fs/xfs/xfs_bmap_item.h
> @@ -74,7 +74,4 @@ struct xfs_bud_log_item {
>  extern struct kmem_zone	*xfs_bui_zone;
>  extern struct kmem_zone	*xfs_bud_zone;
>  
> -void xfs_bui_release(struct xfs_bui_log_item *);
> -int xfs_bui_recover(struct xfs_trans *parent_tp, struct xfs_bui_log_item *buip);
> -
>  #endif	/* __XFS_BMAP_ITEM_H__ */
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index ad5ac97ed0c7..20ee32c2652d 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2553,60 +2553,6 @@ xlog_recover_process_data(
>  	return 0;
>  }
>  
> -/* Recover the BUI if necessary. */
> -STATIC int
> -xlog_recover_process_bui(
> -	struct xfs_trans		*parent_tp,
> -	struct xfs_ail			*ailp,
> -	struct xfs_log_item		*lip)
> -{
> -	struct xfs_bui_log_item		*buip;
> -	int				error;
> -
> -	/*
> -	 * Skip BUIs that we've already processed.
> -	 */
> -	buip = container_of(lip, struct xfs_bui_log_item, bui_item);
> -	if (test_bit(XFS_BUI_RECOVERED, &buip->bui_flags))
> -		return 0;
> -
> -	spin_unlock(&ailp->ail_lock);
> -	error = xfs_bui_recover(parent_tp, buip);
> -	spin_lock(&ailp->ail_lock);
> -
> -	return error;
> -}
> -
> -/* Release the BUI since we're cancelling everything. */
> -STATIC void
> -xlog_recover_cancel_bui(
> -	struct xfs_mount		*mp,
> -	struct xfs_ail			*ailp,
> -	struct xfs_log_item		*lip)
> -{
> -	struct xfs_bui_log_item		*buip;
> -
> -	buip = container_of(lip, struct xfs_bui_log_item, bui_item);
> -
> -	spin_unlock(&ailp->ail_lock);
> -	xfs_bui_release(buip);
> -	spin_lock(&ailp->ail_lock);
> -}
> -
> -/* Is this log item a deferred action intent? */
> -static inline bool xlog_item_is_intent(struct xfs_log_item *lip)
> -{
> -	switch (lip->li_type) {
> -	case XFS_LI_EFI:
> -	case XFS_LI_RUI:
> -	case XFS_LI_CUI:
> -	case XFS_LI_BUI:
> -		return true;
> -	default:
> -		return false;
> -	}
> -}
> -
>  /* Take all the collected deferred ops and finish them in order. */
>  static int
>  xlog_finish_defer_ops(
> @@ -2641,6 +2587,12 @@ xlog_finish_defer_ops(
>  	return xfs_trans_commit(tp);
>  }
>  
> +/* Is this log item a deferred action intent? */
> +static inline bool xlog_item_is_intent(struct xfs_log_item *lip)
> +{
> +	return lip->li_ops->iop_recover != NULL;
> +}
> +
>  /*
>   * When this is called, all of the log intent items which did not have
>   * corresponding log done items should be in the AIL.  What we do now
> @@ -2711,20 +2663,11 @@ xlog_recover_process_intents(
>  
>  		/*
>  		 * NOTE: If your intent processing routine can create more
> -		 * deferred ops, you /must/ attach them to the dfops in this
> -		 * routine or else those subsequent intents will get
> +		 * deferred ops, you /must/ attach them to the transaction in
> +		 * this routine or else those subsequent intents will get
>  		 * replayed in the wrong order!
>  		 */
> -		switch (lip->li_type) {
> -		case XFS_LI_EFI:
> -		case XFS_LI_RUI:
> -		case XFS_LI_CUI:
> -			error = lip->li_ops->iop_recover(lip, parent_tp);
> -			break;
> -		case XFS_LI_BUI:
> -			error = xlog_recover_process_bui(parent_tp, ailp, lip);
> -			break;
> -		}
> +		error = lip->li_ops->iop_recover(lip, parent_tp);
>  		if (error)
>  			goto out;
>  		lip = xfs_trans_ail_cursor_next(ailp, &cur);
> @@ -2767,19 +2710,9 @@ xlog_recover_cancel_intents(
>  			break;
>  		}
>  
> -		switch (lip->li_type) {
> -		case XFS_LI_EFI:
> -		case XFS_LI_RUI:
> -		case XFS_LI_CUI:
> -			spin_unlock(&ailp->ail_lock);
> -			lip->li_ops->iop_release(lip);
> -			spin_lock(&ailp->ail_lock);
> -			break;
> -		case XFS_LI_BUI:
> -			xlog_recover_cancel_bui(log->l_mp, ailp, lip);
> -			break;
> -		}
> -
> +		spin_unlock(&ailp->ail_lock);
> +		lip->li_ops->iop_release(lip);
> +		spin_lock(&ailp->ail_lock);
>  		lip = xfs_trans_ail_cursor_next(ailp, &cur);
>  	}
>  
> 
>
Christoph Hellwig May 6, 2020, 3:21 p.m. UTC | #2
The xlog_item_is_intent change needs to be documented, or even better
split into a separate patch.

Otherwise looks good:

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

Patch
diff mbox series

diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 0fbebef69e26..f88ebf8634c4 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -28,6 +28,8 @@ 
 kmem_zone_t	*xfs_bui_zone;
 kmem_zone_t	*xfs_bud_zone;
 
+static const struct xfs_item_ops xfs_bui_item_ops;
+
 static inline struct xfs_bui_log_item *BUI_ITEM(struct xfs_log_item *lip)
 {
 	return container_of(lip, struct xfs_bui_log_item, bui_item);
@@ -47,7 +49,7 @@  xfs_bui_item_free(
  * committed vs unpin operations in bulk insert operations. Hence the reference
  * count to ensure only the last caller frees the BUI.
  */
-void
+STATIC void
 xfs_bui_release(
 	struct xfs_bui_log_item	*buip)
 {
@@ -126,13 +128,6 @@  xfs_bui_item_release(
 	xfs_bui_release(BUI_ITEM(lip));
 }
 
-static const struct xfs_item_ops xfs_bui_item_ops = {
-	.iop_size	= xfs_bui_item_size,
-	.iop_format	= xfs_bui_item_format,
-	.iop_unpin	= xfs_bui_item_unpin,
-	.iop_release	= xfs_bui_item_release,
-};
-
 /*
  * Allocate and initialize an bui item with the given number of extents.
  */
@@ -425,7 +420,7 @@  const struct xfs_defer_op_type xfs_bmap_update_defer_type = {
  * Process a bmap update intent item that was recovered from the log.
  * We need to update some inode's bmbt.
  */
-int
+STATIC int
 xfs_bui_recover(
 	struct xfs_trans		*parent_tp,
 	struct xfs_bui_log_item		*buip)
@@ -560,6 +555,37 @@  xfs_bui_recover(
 	return error;
 }
 
+/* Recover the BUI if necessary. */
+STATIC int
+xfs_bui_item_recover(
+	struct xfs_log_item		*lip,
+	struct xfs_trans		*tp)
+{
+	struct xfs_ail			*ailp = lip->li_ailp;
+	struct xfs_bui_log_item		*buip = BUI_ITEM(lip);
+	int				error;
+
+	/*
+	 * Skip BUIs that we've already processed.
+	 */
+	if (test_bit(XFS_BUI_RECOVERED, &buip->bui_flags))
+		return 0;
+
+	spin_unlock(&ailp->ail_lock);
+	error = xfs_bui_recover(tp, buip);
+	spin_lock(&ailp->ail_lock);
+
+	return error;
+}
+
+static const struct xfs_item_ops xfs_bui_item_ops = {
+	.iop_size	= xfs_bui_item_size,
+	.iop_format	= xfs_bui_item_format,
+	.iop_unpin	= xfs_bui_item_unpin,
+	.iop_release	= xfs_bui_item_release,
+	.iop_recover	= xfs_bui_item_recover,
+};
+
 /*
  * Copy an BUI format buffer from the given buf, and into the destination
  * BUI format structure.  The BUI/BUD items were designed not to need any
diff --git a/fs/xfs/xfs_bmap_item.h b/fs/xfs/xfs_bmap_item.h
index 515b1d5d6ab7..44d06e62f8f9 100644
--- a/fs/xfs/xfs_bmap_item.h
+++ b/fs/xfs/xfs_bmap_item.h
@@ -74,7 +74,4 @@  struct xfs_bud_log_item {
 extern struct kmem_zone	*xfs_bui_zone;
 extern struct kmem_zone	*xfs_bud_zone;
 
-void xfs_bui_release(struct xfs_bui_log_item *);
-int xfs_bui_recover(struct xfs_trans *parent_tp, struct xfs_bui_log_item *buip);
-
 #endif	/* __XFS_BMAP_ITEM_H__ */
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index ad5ac97ed0c7..20ee32c2652d 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2553,60 +2553,6 @@  xlog_recover_process_data(
 	return 0;
 }
 
-/* Recover the BUI if necessary. */
-STATIC int
-xlog_recover_process_bui(
-	struct xfs_trans		*parent_tp,
-	struct xfs_ail			*ailp,
-	struct xfs_log_item		*lip)
-{
-	struct xfs_bui_log_item		*buip;
-	int				error;
-
-	/*
-	 * Skip BUIs that we've already processed.
-	 */
-	buip = container_of(lip, struct xfs_bui_log_item, bui_item);
-	if (test_bit(XFS_BUI_RECOVERED, &buip->bui_flags))
-		return 0;
-
-	spin_unlock(&ailp->ail_lock);
-	error = xfs_bui_recover(parent_tp, buip);
-	spin_lock(&ailp->ail_lock);
-
-	return error;
-}
-
-/* Release the BUI since we're cancelling everything. */
-STATIC void
-xlog_recover_cancel_bui(
-	struct xfs_mount		*mp,
-	struct xfs_ail			*ailp,
-	struct xfs_log_item		*lip)
-{
-	struct xfs_bui_log_item		*buip;
-
-	buip = container_of(lip, struct xfs_bui_log_item, bui_item);
-
-	spin_unlock(&ailp->ail_lock);
-	xfs_bui_release(buip);
-	spin_lock(&ailp->ail_lock);
-}
-
-/* Is this log item a deferred action intent? */
-static inline bool xlog_item_is_intent(struct xfs_log_item *lip)
-{
-	switch (lip->li_type) {
-	case XFS_LI_EFI:
-	case XFS_LI_RUI:
-	case XFS_LI_CUI:
-	case XFS_LI_BUI:
-		return true;
-	default:
-		return false;
-	}
-}
-
 /* Take all the collected deferred ops and finish them in order. */
 static int
 xlog_finish_defer_ops(
@@ -2641,6 +2587,12 @@  xlog_finish_defer_ops(
 	return xfs_trans_commit(tp);
 }
 
+/* Is this log item a deferred action intent? */
+static inline bool xlog_item_is_intent(struct xfs_log_item *lip)
+{
+	return lip->li_ops->iop_recover != NULL;
+}
+
 /*
  * When this is called, all of the log intent items which did not have
  * corresponding log done items should be in the AIL.  What we do now
@@ -2711,20 +2663,11 @@  xlog_recover_process_intents(
 
 		/*
 		 * NOTE: If your intent processing routine can create more
-		 * deferred ops, you /must/ attach them to the dfops in this
-		 * routine or else those subsequent intents will get
+		 * deferred ops, you /must/ attach them to the transaction in
+		 * this routine or else those subsequent intents will get
 		 * replayed in the wrong order!
 		 */
-		switch (lip->li_type) {
-		case XFS_LI_EFI:
-		case XFS_LI_RUI:
-		case XFS_LI_CUI:
-			error = lip->li_ops->iop_recover(lip, parent_tp);
-			break;
-		case XFS_LI_BUI:
-			error = xlog_recover_process_bui(parent_tp, ailp, lip);
-			break;
-		}
+		error = lip->li_ops->iop_recover(lip, parent_tp);
 		if (error)
 			goto out;
 		lip = xfs_trans_ail_cursor_next(ailp, &cur);
@@ -2767,19 +2710,9 @@  xlog_recover_cancel_intents(
 			break;
 		}
 
-		switch (lip->li_type) {
-		case XFS_LI_EFI:
-		case XFS_LI_RUI:
-		case XFS_LI_CUI:
-			spin_unlock(&ailp->ail_lock);
-			lip->li_ops->iop_release(lip);
-			spin_lock(&ailp->ail_lock);
-			break;
-		case XFS_LI_BUI:
-			xlog_recover_cancel_bui(log->l_mp, ailp, lip);
-			break;
-		}
-
+		spin_unlock(&ailp->ail_lock);
+		lip->li_ops->iop_release(lip);
+		spin_lock(&ailp->ail_lock);
 		lip = xfs_trans_ail_cursor_next(ailp, &cur);
 	}