Hi Ted / Andreas,
Theodore Tso wrote:
> On Fri, Sep 25, 2009 at 04:34:53AM -0600, Andreas Dilger wrote:
>> Wouldn't it make more sense to just return an error (or no error
>> but do nothing) in the case of source/target inode being the same?
>> I don't see how that would be good to "defragment" the inode onto
>> itself, just like "cp f f" and "rename f f" don't make sense either.
>
> The code actually checks to make sure the source and target inodes are
> different, but it does so too late. So the following patch should fix
> the problem which Akira-san was attempting to solve, without
> introducing any extra code complexity (we just move some lines of code
> around instead.)
I thought the argument check (orig and donor inodes are different)
should be done in mext_check_arguments()
because this function checks the arguments whether they are fine
(according to its name).
So mext_double_{up, down}_{read, write} which may be called earlier than
mext_check_arguments() should take/release one inode of them,
if orig and donor inodes are same.
And in the point of view of each function (mext_double_{up, down}_{read, write}),
I thought that we have had better to care about the situation
that same inode is passed to it, they are "static" function though.
Anyway, I tested Ted's patch fixes the problem.
Thanks for your work. :-)
Tested-by: Akira Fujita <a-fujita@xxxxxxxxxxxxx>
Regards,
Akira Fujita
>
> commit 7cdf27b7241ef616b4158a26d7d85bd36f499462
> Author: Theodore Ts'o <tytso@xxxxxxx>
> Date: Mon Sep 28 15:58:29 2009 -0400
>
> ext4: EXT4_IOC_MOVE_EXT: Check for different original and donor inodes first
>
> Move the check to make sure the original and donor inodes are
> different earlier, to avoid a potential deadlock by trying to lock the
> same inode twice.
>
> Signed-off-by: "Theodore Ts'o" <tytso@xxxxxxx>
>
> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
> index 5332fd4..25b6b14 100644
> --- a/fs/ext4/move_extent.c
> +++ b/fs/ext4/move_extent.c
> @@ -1001,14 +1001,6 @@ mext_check_arguments(struct inode *orig_inode,
> return -EINVAL;
> }
>
> - /* orig and donor should be different file */
> - if (orig_inode->i_ino == donor_inode->i_ino) {
> - ext4_debug("ext4 move extent: The argument files should not "
> - "be same file [ino:orig %lu, donor %lu]\n",
> - orig_inode->i_ino, donor_inode->i_ino);
> - return -EINVAL;
> - }
> -
> /* Ext4 move extent supports only extent based file */
> if (!(EXT4_I(orig_inode)->i_flags & EXT4_EXTENTS_FL)) {
> ext4_debug("ext4 move extent: orig file is not extents "
> @@ -1232,6 +1224,14 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp,
> int block_len_in_page;
> int uninit;
>
> + /* orig and donor should be different file */
> + if (orig_inode->i_ino == donor_inode->i_ino) {
> + ext4_debug("ext4 move extent: The argument files should not "
> + "be same file [ino:orig %lu, donor %lu]\n",
> + orig_inode->i_ino, donor_inode->i_ino);
> + return -EINVAL;
> + }
> +
> /* protect orig and donor against a truncate */
> ret1 = mext_inode_double_lock(orig_inode, donor_inode);
> if (ret1 < 0)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
