Re: [PATCH 3/9] xfs: refactor xlog_recover_process_iunlinks()

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Jul 08, 2022 at 09:43:39AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Seems pretty simple
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--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
>