Re: [PATCH v3 03/11] xfs: support the XFS_BTNUM_FINOBT free inode btree type

[Dave Chinner <david@fromorbit.com>]

On Tue, Feb 04, 2014 at 12:49:34PM -0500, Brian Foster wrote:
> Define the AGI fields for the finobt root/level and add magic
> numbers. Update the btree code to add support for the new
> XFS_BTNUM_FINOBT inode btree.
> 
> The finobt root block is reserved immediately following the inobt
> root block in the AG. Update XFS_PREALLOC_BLOCKS() to determine the
> starting AG data block based on whether finobt support is enabled.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_ag.h           | 32 +++++++++++++++----------
>  fs/xfs/xfs_btree.c        |  6 +++--
>  fs/xfs/xfs_btree.h        |  3 +++
>  fs/xfs/xfs_format.h       | 14 ++++++++++-
>  fs/xfs/xfs_ialloc.c       | 37 +++++++++++++++++++++++++----
>  fs/xfs/xfs_ialloc_btree.c | 60 +++++++++++++++++++++++++++++++++++++++++++++--
>  fs/xfs/xfs_log_recover.c  |  2 ++
>  fs/xfs/xfs_stats.c        |  1 +
>  fs/xfs/xfs_stats.h        | 18 +++++++++++++-
>  fs/xfs/xfs_types.h        |  2 +-
>  10 files changed, 150 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ag.h b/fs/xfs/xfs_ag.h
> index 3fc1098..5d3011f 100644
> --- a/fs/xfs/xfs_ag.h
> +++ b/fs/xfs/xfs_ag.h
> @@ -164,22 +164,28 @@ typedef struct xfs_agi {
>  	__be32		agi_pad32;
>  	__be64		agi_lsn;	/* last write sequence */
>  
> +	__be32		agi_free_root; /* root of the free inode btree */
> +	__be32		agi_free_level;/* levels in free inode btree */
> +
>  	/* structure must be padded to 64 bit alignment */
>  } xfs_agi_t;

Can you add comments to the agi structure that indicate where the
contiguous logging regions start and end?

.....

>  #ifdef DEBUG
> @@ -1515,14 +1517,39 @@ xfs_ialloc_log_agi(
>  	ASSERT(agi->agi_magicnum == cpu_to_be32(XFS_AGI_MAGIC));
>  #endif
>  	/*
> -	 * Compute byte offsets for the first and last fields.
> +	 * The growth of the agi buffer over time now requires that we interpret
> +	 * the buffer as two logical regions delineated at the end of the unlinked
> +	 * list. This is due to the size of the hash table and its location in the
> +	 * middle of the agi.
> +	 *
> +	 * For example, a request to log a field before agi_unlinked and a field
> +	 * after agi_unlinked could cause us to log the entire hash table and use
> +	 * an excessive amount of log space. To avoid this behavior, log the
> +	 * region up through agi_unlinked in one call and the region after
> +	 * agi_unlinked through the end of the structure in another.
>  	 */

This comment should be at the head of the function as it describes
the constraints the function works under, rather than explaining
particular piece of the code.

....
> @@ -334,11 +384,17 @@ xfs_inobt_init_cursor(
>  
>  	cur->bc_tp = tp;
>  	cur->bc_mp = mp;
> -	cur->bc_nlevels = be32_to_cpu(agi->agi_level);
>  	cur->bc_btnum = btnum;
> +	if (btnum == XFS_BTNUM_INO) {
> +		cur->bc_nlevels = be32_to_cpu(agi->agi_level);
> +		cur->bc_ops = &xfs_inobt_ops;
> +	} else {
> +		cur->bc_nlevels = be32_to_cpu(agi->agi_free_level);
> +		cur->bc_ops = &xfs_finobt_ops;

		ASSERT(btnum == XFS_BTNUM_FINO);
....

Otherwise looks pretty good.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs