xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: xfs_swap_extents needs to handle dynamic fork offsets
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Tue, 12 Jan 2010 08:09:22 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1263272506-15085-1-git-send-email-david@xxxxxxxxxxxxx>
References: <1263272506-15085-1-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.19 (2009-01-05)
> +     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>

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