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

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, May 05, 2020 at 06:49:17PM +0530, Chandan Babu R wrote:
> 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.

Hm, you're right.  I'll make this function return void and then mess
with the return values and whatnot later.

--D

> 
> > +	}
> > +
> > +	/*
> > +	 * 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
> 
> 
>