On Fri, Jul 08, 2022 at 09:43:39AM +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>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
Seems pretty simple
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--D
> ---
> fs/xfs/xfs_log_recover.c | 135 ++++++++++++++++++++-------------------
> 1 file changed, 70 insertions(+), 65 deletions(-)
>
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index e9dfd7102312..c3fff566ae7e 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2667,40 +2667,35 @@ xlog_recover_clear_agi_bucket(
> return;
> }
>
> -STATIC xfs_agino_t
> -xlog_recover_process_one_iunlink(
> - struct xfs_perag *pag,
> - xfs_agino_t agino,
> - int bucket)
> +static int
> +xlog_recover_iunlink_bucket(
> + struct xfs_perag *pag,
> + struct xfs_agi *agi,
> + int bucket)
> {
> - struct xfs_inode *ip;
> - xfs_ino_t ino;
> - int error;
> + struct xfs_mount *mp = pag->pag_mount;
> + struct xfs_inode *ip;
> + xfs_agino_t agino;
>
> - ino = XFS_AGINO_TO_INO(pag->pag_mount, pag->pag_agno, agino);
> - error = xfs_iget(pag->pag_mount, 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;;
>
> - 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(pag, bucket);
> - return NULLAGINO;
> + xfs_irele(ip);
> + cond_resched();
> + }
> + return 0;
> }
>
> /*
> @@ -2726,59 +2721,69 @@ 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_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(pag, NULL, &agibp);
> + error = xfs_read_agi(pag, NULL, &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(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;
> - }
> - /*
> - * 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(pag,
> - agino, bucket);
> - cond_resched();
> - }
> + xlog_recover_clear_agi_bucket(pag, bucket);
> }
> - 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(pag);
> +
> /*
> * Flush the pending unlinked inodes to ensure that the inactivations
> * 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
>
