> + 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@xxxxxx>
|