[Forwarding this RVB to the list for more permanent recording...]
--D
----- Forwarded message from Chandan Babu R <chandanrlinux@xxxxxxxxx> -----
Date: Thu, 07 May 2020 09:32:27 +0530
From: Chandan Babu R <chandanrlinux@xxxxxxxxx>
To: "Darrick J. Wong" <darrick.wong@xxxxxxxxxx>
Subject: Re: [PATCH 20/25] xfs: refactor adding recovered intent items to the log
On Thursday 7 May 2020 6:33:49 AM IST you wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> During recovery, every intent that we recover from the log has to be
> added to the AIL. Replace the open-coded addition with a helper.
>
The changes look good to me.
Reviewed-by: Chandan Babu R <chandanrlinux@xxxxxxxxx>
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> fs/xfs/xfs_bmap_item.c | 10 +++-------
> fs/xfs/xfs_extfree_item.c | 10 +++-------
> fs/xfs/xfs_refcount_item.c | 10 +++-------
> fs/xfs/xfs_rmap_item.c | 10 +++-------
> fs/xfs/xfs_trans_ail.c | 11 +++++++++++
> fs/xfs/xfs_trans_priv.h | 3 +++
> 6 files changed, 26 insertions(+), 28 deletions(-)
>
>
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index b3996f361b87..1e9bc8d15f51 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -651,15 +651,11 @@ xlog_recover_bui_commit_pass2(
> return error;
> }
> atomic_set(&buip->bui_next_extent, bui_formatp->bui_nextents);
> -
> - spin_lock(&log->l_ailp->ail_lock);
> /*
> - * The RUI has two references. One for the RUD and one for RUI to ensure
> - * it makes it into the AIL. Insert the RUI into the AIL directly and
> - * drop the RUI reference. Note that xfs_trans_ail_update() drops the
> - * AIL lock.
> + * Insert the intent into the AIL directly and drop one reference so
> + * that finishing or canceling the work will drop the other.
> */
> - xfs_trans_ail_update(log->l_ailp, &buip->bui_item, lsn);
> + xfs_trans_ail_insert(log->l_ailp, &buip->bui_item, lsn);
> xfs_bui_release(buip);
> return 0;
> }
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 3855e30109bf..99c4643d0ae8 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -710,15 +710,11 @@ xlog_recover_efi_commit_pass2(
> return error;
> }
> atomic_set(&efip->efi_next_extent, efi_formatp->efi_nextents);
> -
> - spin_lock(&log->l_ailp->ail_lock);
> /*
> - * The EFI has two references. One for the EFD and one for EFI to ensure
> - * it makes it into the AIL. Insert the EFI into the AIL directly and
> - * drop the EFI reference. Note that xfs_trans_ail_update() drops the
> - * AIL lock.
> + * Insert the intent into the AIL directly and drop one reference so
> + * that finishing or canceling the work will drop the other.
> */
> - xfs_trans_ail_update(log->l_ailp, &efip->efi_item, lsn);
> + xfs_trans_ail_insert(log->l_ailp, &efip->efi_item, lsn);
> xfs_efi_release(efip);
> return 0;
> }
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index c03836e1a6d7..a9c513338ddc 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -660,15 +660,11 @@ xlog_recover_cui_commit_pass2(
> return error;
> }
> atomic_set(&cuip->cui_next_extent, cui_formatp->cui_nextents);
> -
> - spin_lock(&log->l_ailp->ail_lock);
> /*
> - * The CUI has two references. One for the CUD and one for CUI to ensure
> - * it makes it into the AIL. Insert the CUI into the AIL directly and
> - * drop the CUI reference. Note that xfs_trans_ail_update() drops the
> - * AIL lock.
> + * Insert the intent into the AIL directly and drop one reference so
> + * that finishing or canceling the work will drop the other.
> */
> - xfs_trans_ail_update(log->l_ailp, &cuip->cui_item, lsn);
> + xfs_trans_ail_insert(log->l_ailp, &cuip->cui_item, lsn);
> xfs_cui_release(cuip);
> return 0;
> }
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index 31d35de518d1..ee0be4310c7c 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -651,15 +651,11 @@ xlog_recover_rui_commit_pass2(
> return error;
> }
> atomic_set(&ruip->rui_next_extent, rui_formatp->rui_nextents);
> -
> - spin_lock(&log->l_ailp->ail_lock);
> /*
> - * The RUI has two references. One for the RUD and one for RUI to ensure
> - * it makes it into the AIL. Insert the RUI into the AIL directly and
> - * drop the RUI reference. Note that xfs_trans_ail_update() drops the
> - * AIL lock.
> + * Insert the intent into the AIL directly and drop one reference so
> + * that finishing or canceling the work will drop the other.
> */
> - xfs_trans_ail_update(log->l_ailp, &ruip->rui_item, lsn);
> + xfs_trans_ail_insert(log->l_ailp, &ruip->rui_item, lsn);
> xfs_rui_release(ruip);
> return 0;
> }
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index bf09d4b4df58..ac5019361a13 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -815,6 +815,17 @@ xfs_trans_ail_update_bulk(
> xfs_ail_update_finish(ailp, tail_lsn);
> }
>
> +/* Insert a log item into the AIL. */
> +void
> +xfs_trans_ail_insert(
> + struct xfs_ail *ailp,
> + struct xfs_log_item *lip,
> + xfs_lsn_t lsn)
> +{
> + spin_lock(&ailp->ail_lock);
> + xfs_trans_ail_update_bulk(ailp, NULL, &lip, 1, lsn);
> +}
> +
> /*
> * Delete one log item from the AIL.
> *
> diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
> index cc046d9557ae..3004aeac9110 100644
> --- a/fs/xfs/xfs_trans_priv.h
> +++ b/fs/xfs/xfs_trans_priv.h
> @@ -91,6 +91,9 @@ xfs_trans_ail_update(
> xfs_trans_ail_update_bulk(ailp, NULL, &lip, 1, lsn);
> }
>
> +void xfs_trans_ail_insert(struct xfs_ail *ailp, struct xfs_log_item *lip,
> + xfs_lsn_t lsn);
> +
> xfs_lsn_t xfs_ail_delete_one(struct xfs_ail *ailp, struct xfs_log_item *lip);
> void xfs_ail_update_finish(struct xfs_ail *ailp, xfs_lsn_t old_lsn)
> __releases(ailp->ail_lock);
>
>
--
chandan
----- End forwarded message -----
