Re: [PATCH 19/28] xfs: refactor xlog_recover_process_unlinked

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

On Tuesday 5 May 2020 6:42:35 AM IST Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Hoist the unlinked inode processing logic out of the AG loop and into
> its own function.  No functional changes.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_unlink_recover.c |   91 +++++++++++++++++++++++++------------------
>  1 file changed, 52 insertions(+), 39 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_unlink_recover.c b/fs/xfs/xfs_unlink_recover.c
> index 2a19d096e88d..413b34085640 100644
> --- a/fs/xfs/xfs_unlink_recover.c
> +++ b/fs/xfs/xfs_unlink_recover.c
> @@ -145,54 +145,67 @@ xlog_recover_process_one_iunlink(
>   * scheduled on this CPU to ensure other scheduled work can run without undue
>   * latency.
>   */
> -void
> -xlog_recover_process_unlinked(
> -	struct xlog		*log)
> +STATIC int
> +xlog_recover_process_iunlinked(
> +	struct xfs_mount	*mp,
> +	xfs_agnumber_t		agno)
>  {
> -	struct xfs_mount	*mp;
>  	struct xfs_agi		*agi;
>  	struct xfs_buf		*agibp;
> -	xfs_agnumber_t		agno;
>  	xfs_agino_t		agino;
>  	int			bucket;
>  	int			error;
>  
> -	mp = log->l_mp;
> -
> -	for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
> -		/*
> -		 * Find the agi for this ag.
> -		 */
> -		error = xfs_read_agi(mp, NULL, 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....
> -			 */
> -			continue;
> -		}
> +	/*
> +	 * Find the agi for this ag.
> +	 */
> +	error = xfs_read_agi(mp, NULL, agno, &agibp);
> +	if (error) {
>  		/*
> -		 * 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 is b0rked. Don't process it.
> +		 *
> +		 * We should probably mark the filesystem as corrupt
> +		 * after we've recovered all the ag's we can....
>  		 */
> -		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,
> -							agno, agino, bucket);
> -				cond_resched();
> -			}
> +		return error;


This causes a change in behaviour i.e. an error return from here would cause
xlog_recover_process_unlinked() to break "loop on all AGs". Before this
change, XFS would continue to process all the remaining AGs as described by
the above comment.


> +	}
> +
> +	/*
> +	 * 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,
> +						agno, agino, bucket);
> +			cond_resched();
>  		}
> -		xfs_buf_rele(agibp);
> +	}
> +	xfs_buf_rele(agibp);
> +
> +	return 0;
> +}
> +
> +void
> +xlog_recover_process_unlinked(
> +	struct xlog		*log)
> +{
> +	struct xfs_mount	*mp = log->l_mp;
> +	xfs_agnumber_t		agno;
> +	int			error;
> +
> +	for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
> +		error = xlog_recover_process_iunlinked(mp, agno);
> +		if (error)
> +			break;
>  	}
>  }
> 
> 


-- 
chandan