On Mon, Jun 27, 2022 at 10:43:30AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> For upcoming changes to the way inode unlinked list processing is
> done, the structure of recovery needs to change slightly. We also
> really need to untangle the messy error handling in list recovery
> so that actions like emptying the bucket on inode lookup failure
> are associated with the bucket list walk failing, not failing
> to look up the inode.
>
> Refactor the recovery code now to keep the re-organisation seperate
> to the algorithm changes.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/xfs_log_recover.c | 162 ++++++++++++++++++++-------------------
> 1 file changed, 84 insertions(+), 78 deletions(-)
>
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index f360b46533a6..7d0f530d7e3c 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2627,23 +2627,23 @@ xlog_recover_cancel_intents(
> * This routine performs a transaction to null out a bad inode pointer
> * in an agi unlinked inode hash bucket.
> */
> -STATIC void
> +static void
> xlog_recover_clear_agi_bucket(
> - xfs_mount_t *mp,
> - xfs_agnumber_t agno,
> - int bucket)
> + struct xfs_mount *mp,
> + struct xfs_perag *pag,
Why even pass in *mp when you've got *pag?
> + int bucket)
> {
> - xfs_trans_t *tp;
> - xfs_agi_t *agi;
> - struct xfs_buf *agibp;
> - int offset;
> - int error;
> + struct xfs_trans *tp;
> + struct xfs_agi *agi;
> + struct xfs_buf *agibp;
> + int offset;
> + int error;
>
> error = xfs_trans_alloc(mp, &M_RES(mp)->tr_clearagi, 0, 0, 0, &tp);
> if (error)
> goto out_error;
>
> - error = xfs_read_agi(mp, tp, agno, &agibp);
> + error = xfs_read_agi(mp, tp, pag->pag_agno, &agibp);
> if (error)
> goto out_abort;
>
> @@ -2662,46 +2662,40 @@ xlog_recover_clear_agi_bucket(
> out_abort:
> xfs_trans_cancel(tp);
> out_error:
> - xfs_warn(mp, "%s: failed to clear agi %d. Continuing.", __func__, agno);
> + xfs_warn(mp, "%s: failed to clear agi %d. Continuing.",
> + __func__, pag->pag_agno);
> return;
Nit: Don't need the return here.
Otherwise looks good.
--D
> }
>
> -STATIC xfs_agino_t
> -xlog_recover_process_one_iunlink(
> - struct xfs_mount *mp,
> - xfs_agnumber_t agno,
> - xfs_agino_t agino,
> - int bucket)
> +static int
> +xlog_recover_iunlink_bucket(
> + struct xfs_mount *mp,
> + struct xfs_perag *pag,
> + struct xfs_agi *agi,
> + int bucket)
> {
> - struct xfs_inode *ip;
> - xfs_ino_t ino;
> - int error;
> + struct xfs_inode *ip;
> + xfs_agino_t agino;
>
> - ino = XFS_AGINO_TO_INO(mp, agno, agino);
> - error = xfs_iget(mp, NULL, ino, 0, 0, &ip);
> - if (error)
> - goto fail;
> + agino = be32_to_cpu(agi->agi_unlinked[bucket]);
> + while (agino != NULLAGINO) {
> + int error;
>
> - xfs_iflags_clear(ip, XFS_IRECOVERY);
> - ASSERT(VFS_I(ip)->i_nlink == 0);
> - ASSERT(VFS_I(ip)->i_mode != 0);
> + error = xfs_iget(mp, NULL,
> + XFS_AGINO_TO_INO(mp, pag->pag_agno, agino),
> + 0, 0, &ip);
> + if (error)
> + return error;;
>
> - /* setup for the next pass */
> - agino = ip->i_next_unlinked;
> - xfs_irele(ip);
> - return agino;
> + ASSERT(VFS_I(ip)->i_nlink == 0);
> + ASSERT(VFS_I(ip)->i_mode != 0);
> + xfs_iflags_clear(ip, XFS_IRECOVERY);
> + agino = ip->i_next_unlinked;
>
> - fail:
> - /*
> - * We can't read in the inode this bucket points to, or this inode
> - * is messed up. Just ditch this bucket of inodes. We will lose
> - * some inodes and space, but at least we won't hang.
> - *
> - * Call xlog_recover_clear_agi_bucket() to perform a transaction to
> - * clear the inode pointer in the bucket.
> - */
> - xlog_recover_clear_agi_bucket(mp, agno, bucket);
> - return NULLAGINO;
> + xfs_irele(ip);
> + cond_resched();
> + }
> + return 0;
> }
>
> /*
> @@ -2727,51 +2721,63 @@ xlog_recover_process_one_iunlink(
> * scheduled on this CPU to ensure other scheduled work can run without undue
> * latency.
> */
> -STATIC void
> -xlog_recover_process_iunlinks(
> - struct xlog *log)
> +static void
> +xlog_recover_iunlink_ag(
> + struct xfs_mount *mp,
> + struct xfs_perag *pag)
> {
> - struct xfs_mount *mp = log->l_mp;
> - struct xfs_perag *pag;
> - xfs_agnumber_t agno;
> struct xfs_agi *agi;
> struct xfs_buf *agibp;
> - xfs_agino_t agino;
> int bucket;
> int error;
>
> - for_each_perag(mp, agno, pag) {
> - error = xfs_read_agi(mp, NULL, pag->pag_agno, &agibp);
> + error = xfs_read_agi(mp, NULL, pag->pag_agno, &agibp);
> + if (error) {
> + /*
> + * AGI is b0rked. Don't process it.
> + *
> + * We should probably mark the filesystem as corrupt after we've
> + * recovered all the ag's we can....
> + */
> + return;
> + }
> +
> + /*
> + * Unlock the buffer so that it can be acquired in the normal course of
> + * the transaction to truncate and free each inode. Because we are not
> + * racing with anyone else here for the AGI buffer, we don't even need
> + * to hold it locked to read the initial unlinked bucket entries out of
> + * the buffer. We keep buffer reference though, so that it stays pinned
> + * in memory while we need the buffer.
> + */
> + agi = agibp->b_addr;
> + xfs_buf_unlock(agibp);
> +
> + for (bucket = 0; bucket < XFS_AGI_UNLINKED_BUCKETS; bucket++) {
> + error = xlog_recover_iunlink_bucket(mp, pag, agi, bucket);
> if (error) {
> /*
> - * AGI is b0rked. Don't process it.
> - *
> - * We should probably mark the filesystem as corrupt
> - * after we've recovered all the ag's we can....
> + * Bucket is unrecoverable, so only a repair scan can
> + * free the remaining unlinked inodes. Just empty the
> + * bucket and remaining inodes on it unreferenced and
> + * unfreeable.
> */
> - continue;
> + xlog_recover_clear_agi_bucket(mp, pag, bucket);
> }
> - /*
> - * Unlock the buffer so that it can be acquired in the normal
> - * course of the transaction to truncate and free each inode.
> - * Because we are not racing with anyone else here for the AGI
> - * buffer, we don't even need to hold it locked to read the
> - * initial unlinked bucket entries out of the buffer. We keep
> - * buffer reference though, so that it stays pinned in memory
> - * while we need the buffer.
> - */
> - agi = agibp->b_addr;
> - xfs_buf_unlock(agibp);
> -
> - for (bucket = 0; bucket < XFS_AGI_UNLINKED_BUCKETS; bucket++) {
> - agino = be32_to_cpu(agi->agi_unlinked[bucket]);
> - while (agino != NULLAGINO) {
> - agino = xlog_recover_process_one_iunlink(mp,
> - pag->pag_agno, agino, bucket);
> - cond_resched();
> - }
> - }
> - xfs_buf_rele(agibp);
> + }
> +
> + xfs_buf_rele(agibp);
> +}
> +
> +static void
> +xlog_recover_process_iunlinks(
> + struct xlog *log)
> +{
> + struct xfs_perag *pag;
> + xfs_agnumber_t agno;
> +
> + for_each_perag(log->l_mp, agno, pag) {
> + xlog_recover_iunlink_ag(log->l_mp, pag);
> }
>
> /*
> @@ -2779,7 +2785,7 @@ xlog_recover_process_iunlinks(
> * are fully completed on disk and the incore inodes can be reclaimed
> * before we signal that recovery is complete.
> */
> - xfs_inodegc_flush(mp);
> + xfs_inodegc_flush(log->l_mp);
> }
>
> STATIC void
> --
> 2.36.1
>
