Re: [PATCH 17/28] xfs: refactor recovered BUI log item playback

Chandan Babu R
2020-05-05T09:49:54Z

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);
> }
>
>
>

--
chandan