Re: [PATCH] xfs: xfs_swap_extents needs to handle dynamic fork offsets

[Christoph Hellwig <hch@infradead.org>]

> +	TP_printk("dev %d:%d %s inode 0x%llx, %s format, num_extents %d, "
> +		  "Max in-fork extents %d, broot size %d, fork offset %d",

It would be nice to keep the

	"dev %d:%d ino 0x%llx"

prefix as a convention so that all trace records are similar at their
beginning.

>  #undef TRACE_INCLUDE_PATH
> diff --git a/fs/xfs/xfs_dfrag.c b/fs/xfs/xfs_dfrag.c
> @@ -53,7 +53,7 @@ xfs_swapext(
>  	xfs_swapext_t	*sxp)
>  {
>  	xfs_inode_t     *ip, *tip;
> -	struct file	*file, *target_file;
> +	struct file	*file, *tmp_file;

I think these xfs_swapext belong into a separate patch.  While they
make the code quite a bit more redable they're purely cleanups and
can wait for 2.6.34.  And while you're at it you might also want to
merge xfs_swap_extents into xfs_swapext - there's no need for that
split at all.

> +static int
> +xfs_swap_extents_check_format(
> +	xfs_inode_t	*ip,	/* target inode */
> +	xfs_inode_t	*tip)	/* tmp inode */

What about just using struct xfs_inode * for new code?

> +	/* Should never get a local format */
> +	if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL ||
> +	    tip->i_d.di_format == XFS_DINODE_FMT_LOCAL)
> +		return EINVAL;

Ok, same check as in the old code.  Any reason we drop the XFS_ERROR
here?

> +	/*
> +	 * if the target inode has less extents that then temporary inode then
> +	 * why did userspace call us?
> +	 */
> +	if (ip->i_d.di_nextents < tip->i_d.di_nextents)
> +		return EINVAL;

Ok.
> +	/*
> +	 * if the target inode is in extent form and the temp inode is in btree
> +	 * form then we will end up with the target inode in the wrong format
> +	 * as we already know there are less extents in the temp inode.
> +	 */
> +	if (ip->i_d.di_format == XFS_DINODE_FMT_EXTENTS &&
> +	    tip->i_d.di_format == XFS_DINODE_FMT_BTREE)
> +		return EINVAL;

Ok.

> +	/* 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)
> +		return EINVAL;
> +
> +	/* 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)
> +		return EINVAL;

So we check this either way just to be sure, ok.

> +	/* Check root block of temp in btree form to max in target */
> +	if (tip->i_d.di_format == XFS_DINODE_FMT_BTREE &&
> +	    XFS_IFORK_BOFF(ip) &&
> +	    tip->i_df.if_broot_bytes > XFS_IFORK_BOFF(ip))
> +		return EINVAL;
> +
> +	/* Check root block of target in btree form to max in temp */
> +	if (ip->i_d.di_format == XFS_DINODE_FMT_BTREE &&
> +	    XFS_IFORK_BOFF(tip) &&
> +	    ip->i_df.if_broot_bytes > XFS_IFORK_BOFF(tip))
> +		return EINVAL;

Same here.


Patch looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

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