xfs
[Top] [All Lists]

Re: [PATCH 04/11] xfs: remove the if_ext_max field in struct xfs_ifork

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 04/11] xfs: remove the if_ext_max field in struct xfs_ifork
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 14 Dec 2011 08:59:58 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20111208155918.162879660@xxxxxxxxxxxxxxxxxxxxxx>
References: <20111208155755.323930705@xxxxxxxxxxxxxxxxxxxxxx> <20111208155918.162879660@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Dec 08, 2011 at 10:57:59AM -0500, Christoph Hellwig wrote:
> We spent a lot of effort to maintain this field, but it always equalts to the
> fork size divided by the constant size of an extent.  The prime use of it is
> to assert that the two stay in sync.  Just divide the fork size by the extent
> size in the few places that we actually use it and remove the overhead
> of maintaining it.

Ok, so you are trading off the overhead of initialising once with
runtime overhead of a read, against a runtime overhead read and a
integer division. Some platforms have slow integer division, so at
face value this migh tbe a bit slower.

However, sizeof(struct xfs_bmbt_rec) == 16, which is determined at
compile time so the compiler can optimise that to a shift, which has
basically no overhead compared to a division on platforms where
division is slow.

>From taht perspective, this seems like a good tradeoff to make -
very little additional runtime overhead, much simpler code.

> Also introduce a few helpers to consolidate the places
> where we actually care about the value.

The helpers are a vast improvement.

> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

Couple of small formatting comments below, but otherwise consider it

Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>

> Index: xfs/fs/xfs/xfs_dfrag.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_dfrag.c       2011-12-02 19:39:31.437161062 +0100
> +++ xfs/fs/xfs/xfs_dfrag.c    2011-12-07 11:17:02.342984256 +0100
> @@ -163,12 +163,14 @@ xfs_swap_extents_check_format(
>  
>       /* Check temp in extent form to max in target */
>       if (tip->i_d.di_format == XFS_DINODE_FMT_EXTENTS &&
> -         XFS_IFORK_NEXTENTS(tip, XFS_DATA_FORK) > ip->i_df.if_ext_max)
> +         XFS_IFORK_NEXTENTS(tip, XFS_DATA_FORK) >
> +         XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK))
>               return EINVAL;

I'd indent the XFS_IFORK_MAXEXT() so it's obvious it's part of a
conditional and not a new conditional expression. Maybe something
like:

+           XFS_IFORK_NEXTENTS(tip, XFS_DATA_FORK) >
+                       XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK))

Otherwise it's not exactly obvious how the logic flows here.

>  
>       /* Check target in extent form to max in temp */
>       if (ip->i_d.di_format == XFS_DINODE_FMT_EXTENTS &&
> -         XFS_IFORK_NEXTENTS(ip, XFS_DATA_FORK) > tip->i_df.if_ext_max)
> +         XFS_IFORK_NEXTENTS(ip, XFS_DATA_FORK) >
> +         XFS_IFORK_MAXEXT(tip, XFS_DATA_FORK))
>               return EINVAL;

Same here.

>  
>       /*
> @@ -180,18 +182,25 @@ xfs_swap_extents_check_format(
>        * (a common defrag case) which will occur when the temp inode is in
>        * extent format...
>        */
> -     if (tip->i_d.di_format == XFS_DINODE_FMT_BTREE &&
> -         ((XFS_IFORK_BOFF(ip) &&
> -           tip->i_df.if_broot_bytes > XFS_IFORK_BOFF(ip)) ||
> -          XFS_IFORK_NEXTENTS(tip, XFS_DATA_FORK) <= ip->i_df.if_ext_max))
> +     if (tip->i_d.di_format == XFS_DINODE_FMT_BTREE) {
> +             if (XFS_IFORK_BOFF(ip) &&
> +                 tip->i_df.if_broot_bytes > XFS_IFORK_BOFF(ip))
> +                     return EINVAL;
> +             if (XFS_IFORK_NEXTENTS(tip, XFS_DATA_FORK) <=
> +                 XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK))
>               return EINVAL;
                ^ needs another indent.
> +     }
>  
>       /* Reciprocal target->temp btree format checks */
> -     if (ip->i_d.di_format == XFS_DINODE_FMT_BTREE &&
> -         ((XFS_IFORK_BOFF(tip) &&
> -           ip->i_df.if_broot_bytes > XFS_IFORK_BOFF(tip)) ||
> -          XFS_IFORK_NEXTENTS(ip, XFS_DATA_FORK) <= tip->i_df.if_ext_max))
> +     if (ip->i_d.di_format == XFS_DINODE_FMT_BTREE) {
> +             if (XFS_IFORK_BOFF(tip) &&
> +                 ip->i_df.if_broot_bytes > XFS_IFORK_BOFF(tip))
> +                     return EINVAL;
> +
> +             if (XFS_IFORK_NEXTENTS(ip, XFS_DATA_FORK) <=
> +                 XFS_IFORK_MAXEXT(tip, XFS_DATA_FORK))
>               return EINVAL;

Same here.


-- 
Dave Chinner
david@xxxxxxxxxxxxx

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