xfs
[Top] [All Lists]

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

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH v3 03/11] xfs: support the XFS_BTNUM_FINOBT free inode btree type
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 11 Feb 2014 17:22:11 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1391536182-9048-4-git-send-email-bfoster@xxxxxxxxxx>
References: <1391536182-9048-1-git-send-email-bfoster@xxxxxxxxxx> <1391536182-9048-4-git-send-email-bfoster@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>