No subject

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 at lst.de>

Couple of small formatting comments below, but otherwise consider it

Reviewed-by: Dave Chinner <dchinner at redhat.com>

> 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 at fromorbit.com