Re: [PATCH 21/28] xfs: refactor releasing finished intents during log recovery

[Chandan Babu R <chandanrlinux@gmail.com>]

On Tuesday 5 May 2020 6:42:47 AM IST Darrick J. Wong wrote:
> 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.
>

The functionality is the same as was before applying this patch.

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

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_log_recover.h |    3 +++
>  fs/xfs/xfs_bmap_item.c          |   42 +++++++++------------------------------
>  fs/xfs/xfs_extfree_item.c       |   42 +++++++++------------------------------
>  fs/xfs/xfs_log_recover.c        |   35 ++++++++++++++++++++++++++++++++-
>  fs/xfs/xfs_refcount_item.c      |   42 +++++++++------------------------------
>  fs/xfs/xfs_rmap_item.c          |   42 +++++++++------------------------------
>  fs/xfs/xfs_trans.h              |    1 +
>  7 files changed, 78 insertions(+), 129 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h
> index d4d6d4f84fda..b875819a1c04 100644
> --- a/fs/xfs/libxfs/xfs_log_recover.h
> +++ b/fs/xfs/libxfs/xfs_log_recover.h
> @@ -126,4 +126,7 @@ bool xlog_put_buffer_cancelled(struct xlog *log, xfs_daddr_t blkno, uint len);
>  void xlog_recover_iodone(struct xfs_buf *bp);
>  int xlog_recover_process_unlinked(struct xlog *log);
>  
> +void xlog_recover_release_intent(struct xlog *log, unsigned short intent_type,
> +		uint64_t intent_id);
> +
>  #endif	/* __XFS_LOG_RECOVER_H__ */
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index f88ebf8634c4..96627ea800c8 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -578,12 +578,21 @@ xfs_bui_item_recover(
>  	return error;
>  }
>  
> +STATIC bool
> +xfs_bui_item_match(
> +	struct xfs_log_item	*lip,
> +	uint64_t		intent_id)
> +{
> +	return BUI_ITEM(lip)->bui_format.bui_id == intent_id;
> +}
> +
>  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,
> +	.iop_match	= xfs_bui_item_match,
>  };
>  
>  /*
> @@ -675,45 +684,14 @@ xlog_recover_bmap_done_commit_pass2(
>  	xfs_lsn_t			lsn)
>  {
>  	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);
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 3fc8a9864217..4e1b10ab17a5 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -665,12 +665,21 @@ xfs_efi_item_recover(
>  	return error;
>  }
>  
> +STATIC bool
> +xfs_efi_item_match(
> +	struct xfs_log_item	*lip,
> +	uint64_t		intent_id)
> +{
> +	return EFI_ITEM(lip)->efi_format.efi_id == intent_id;
> +}
> +
>  static const struct xfs_item_ops xfs_efi_item_ops = {
>  	.iop_size	= xfs_efi_item_size,
>  	.iop_format	= xfs_efi_item_format,
>  	.iop_unpin	= xfs_efi_item_unpin,
>  	.iop_release	= xfs_efi_item_release,
>  	.iop_recover	= xfs_efi_item_recover,
> +	.iop_match	= xfs_efi_item_match,
>  };
>  
>  
> @@ -734,46 +743,15 @@ xlog_recover_extfree_done_commit_pass2(
>  	struct xlog_recover_item	*item,
>  	xfs_lsn_t			lsn)
>  {
> -	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 = (struct xfs_efi_log_item *)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);
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 0ccc09c004f1..55477b9b9311 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1779,6 +1779,38 @@ xlog_clear_stale_blocks(
>  	return 0;
>  }
>  
> +/*
> + * 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)
> +{
> +	struct xfs_ail_cursor	cur;
> +	struct xfs_log_item	*lip;
> +	struct xfs_ail		*ailp = log->l_ailp;
> +
> +	spin_lock(&ailp->ail_lock);
> +	for (lip = xfs_trans_ail_cursor_first(ailp, &cur, 0); lip != NULL;
> +	     lip = xfs_trans_ail_cursor_next(ailp, &cur)) {
> +		if (lip->li_type != intent_type)
> +			continue;
> +		if (!lip->li_ops->iop_match(lip, intent_id))
> +			continue;
> +
> +		spin_unlock(&ailp->ail_lock);
> +		lip->li_ops->iop_release(lip);
> +		spin_lock(&ailp->ail_lock);
> +		break;
> +	}
> +
> +	xfs_trans_ail_cursor_done(&cur);
> +	spin_unlock(&ailp->ail_lock);
> +}
> +
>  /******************************************************************************
>   *
>   *		Log recover routines
> @@ -2590,7 +2622,8 @@ xlog_finish_defer_ops(
>  /* 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;
> +	return lip->li_ops->iop_recover != NULL &&
> +	       lip->li_ops->iop_match != NULL;
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index 5b72eebd8764..27126b136b5a 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -591,12 +591,21 @@ xfs_cui_item_recover(
>  	return error;
>  }
>  
> +STATIC bool
> +xfs_cui_item_match(
> +	struct xfs_log_item	*lip,
> +	uint64_t		intent_id)
> +{
> +	return CUI_ITEM(lip)->cui_format.cui_id == intent_id;
> +}
> +
>  static const struct xfs_item_ops xfs_cui_item_ops = {
>  	.iop_size	= xfs_cui_item_size,
>  	.iop_format	= xfs_cui_item_format,
>  	.iop_unpin	= xfs_cui_item_unpin,
>  	.iop_release	= xfs_cui_item_release,
>  	.iop_recover	= xfs_cui_item_recover,
> +	.iop_match	= xfs_cui_item_match,
>  };
>  
>  /*
> @@ -684,45 +693,14 @@ xlog_recover_refcount_done_commit_pass2(
>  	xfs_lsn_t			lsn)
>  {
>  	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);
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index e763dd8ed0a6..3987f217415c 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -606,12 +606,21 @@ xfs_rui_item_recover(
>  	return error;
>  }
>  
> +STATIC bool
> +xfs_rui_item_match(
> +	struct xfs_log_item	*lip,
> +	uint64_t		intent_id)
> +{
> +	return RUI_ITEM(lip)->rui_format.rui_id == intent_id;
> +}
> +
>  static const struct xfs_item_ops xfs_rui_item_ops = {
>  	.iop_size	= xfs_rui_item_size,
>  	.iop_format	= xfs_rui_item_format,
>  	.iop_unpin	= xfs_rui_item_unpin,
>  	.iop_release	= xfs_rui_item_release,
>  	.iop_recover	= xfs_rui_item_recover,
> +	.iop_match	= xfs_rui_item_match,
>  };
>  
>  /*
> @@ -675,42 +684,11 @@ xlog_recover_rmap_done_commit_pass2(
>  	xfs_lsn_t			lsn)
>  {
>  	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);
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 3f6a79108991..3e8808bb07c5 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -78,6 +78,7 @@ struct xfs_item_ops {
>  	xfs_lsn_t (*iop_committed)(struct xfs_log_item *, xfs_lsn_t);
>  	void (*iop_error)(struct xfs_log_item *, xfs_buf_t *);
>  	int (*iop_recover)(struct xfs_log_item *lip, struct xfs_trans *tp);
> +	bool (*iop_match)(struct xfs_log_item *item, uint64_t id);
>  };
>  
>  /*
> 
> 


-- 
chandan