Re: [PATCH 3/4] xfs: drop all altpath buffers at the end of the sibling check

[Chandan Rajendra <chandan@linux.ibm.com>]

On Wednesday, March 25, 2020 8:54 AM Darrick J. Wong wrote: 
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The dirattr btree checking code uses the altpath substructure of the
> dirattr state structure to check the sibling pointers of dir/attr tree
> blocks.  At the end of sibling checks, xfs_da3_path_shift could have
> changed multiple levels of buffer pointers in the altpath structure.
> Although we release the leaf level buffer, this isn't enough -- we also
> need to release the node buffers that are unique to the altpath.
> 
> Not releasing all of the altpath buffers leaves them locked to the
> transaction.  This is suboptimal because we should release resources
> when we don't need them anymore.  Fix the function to loop all levels of
> the altpath, and fix the return logic so that we always run the loop.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

The patch indeed releases 'altpath' block buffers that are not referenced by
by block buffers in 'path' after the sibling block is checked for
inconsistencies.

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> ---
>  fs/xfs/scrub/dabtree.c |   42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/dabtree.c b/fs/xfs/scrub/dabtree.c
> index 97a15b6f2865..9a2e27ac1300 100644
> --- a/fs/xfs/scrub/dabtree.c
> +++ b/fs/xfs/scrub/dabtree.c
> @@ -219,19 +219,21 @@ xchk_da_btree_block_check_sibling(
>  	int			direction,
>  	xfs_dablk_t		sibling)
>  {
> +	struct xfs_da_state_path *path = &ds->state->path;
> +	struct xfs_da_state_path *altpath = &ds->state->altpath;
>  	int			retval;
> +	int			plevel;
>  	int			error;
>  
> -	memcpy(&ds->state->altpath, &ds->state->path,
> -			sizeof(ds->state->altpath));
> +	memcpy(altpath, path, sizeof(ds->state->altpath));
>  
>  	/*
>  	 * If the pointer is null, we shouldn't be able to move the upper
>  	 * level pointer anywhere.
>  	 */
>  	if (sibling == 0) {
> -		error = xfs_da3_path_shift(ds->state, &ds->state->altpath,
> -				direction, false, &retval);
> +		error = xfs_da3_path_shift(ds->state, altpath, direction,
> +				false, &retval);
>  		if (error == 0 && retval == 0)
>  			xchk_da_set_corrupt(ds, level);
>  		error = 0;
> @@ -239,27 +241,33 @@ xchk_da_btree_block_check_sibling(
>  	}
>  
>  	/* Move the alternate cursor one block in the direction given. */
> -	error = xfs_da3_path_shift(ds->state, &ds->state->altpath,
> -			direction, false, &retval);
> +	error = xfs_da3_path_shift(ds->state, altpath, direction, false,
> +			&retval);
>  	if (!xchk_da_process_error(ds, level, &error))
> -		return error;
> +		goto out;
>  	if (retval) {
>  		xchk_da_set_corrupt(ds, level);
> -		return error;
> +		goto out;
>  	}
> -	if (ds->state->altpath.blk[level].bp)
> -		xchk_buffer_recheck(ds->sc,
> -				ds->state->altpath.blk[level].bp);
> +	if (altpath->blk[level].bp)
> +		xchk_buffer_recheck(ds->sc, altpath->blk[level].bp);
>  
>  	/* Compare upper level pointer to sibling pointer. */
> -	if (ds->state->altpath.blk[level].blkno != sibling)
> +	if (altpath->blk[level].blkno != sibling)
>  		xchk_da_set_corrupt(ds, level);
> -	if (ds->state->altpath.blk[level].bp) {
> -		xfs_trans_brelse(ds->dargs.trans,
> -				ds->state->altpath.blk[level].bp);
> -		ds->state->altpath.blk[level].bp = NULL;
> -	}
> +
>  out:
> +	/* Free all buffers in the altpath that aren't referenced from path. */
> +	for (plevel = 0; plevel < altpath->active; plevel++) {
> +		if (altpath->blk[plevel].bp == NULL ||
> +		    (plevel < path->active &&
> +		     altpath->blk[plevel].bp == path->blk[plevel].bp))
> +			continue;
> +
> +		xfs_trans_brelse(ds->dargs.trans, altpath->blk[plevel].bp);
> +		altpath->blk[plevel].bp = NULL;
> +	}
> +
>  	return error;
>  }
>  
> 
> 


-- 
chandan