Re: [PATCH 2/2] simplify xfs_lock_for_rename

["Barry Naujok" <bnaujok@xxxxxxx>]

On Fri, 11 Apr 2008 04:44:13 +1000, Christoph Hellwig <hch@xxxxxx> wrote:

> as with lookup merge what's left of xfs_dir_lookup_int into it, kill
> a useless lock roundtrip left over from IRIX and kill a bunch of useless
> variables.
>
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
>
> Index: linux-2.6-xfs/fs/xfs/xfs_rename.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_rename.c	2008-04-09 20:30:08.000000000  
> +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_rename.c   2008-04-10 08:58:50.000000000 +0200
> @@ -67,10 +67,6 @@ xfs_rename_unlock4(
>         }
>  }
> -#ifdef DEBUG
> -int xfs_rename_skip, xfs_rename_nskip;
> -#endif
> -
>  /*
>   * The following routine will acquire the locks required for a rename
>   * operation. The code understands the semantics of renames and will
> @@ -92,47 +88,33 @@ xfs_lock_for_rename(
>         xfs_inode_t     **i_tab,/* out: array of inode returned, sorted */
>         int             *num_inodes)  /* out: number of inodes in array */
>  {
> -       xfs_inode_t             *ip2 = NULL;
>         xfs_inode_t             *temp;
> -       xfs_ino_t               inum1, inum2;
> +       xfs_ino_t               inum2;
>         int                     error;
>         int                     i, j;
>         uint                    lock_mode;
> -       int                     diff_dirs = (dp1 != dp2);
> -       /*
> -        * First, find out the current inums of the entries so that we
> -        * can determine the initial locking order.  We'll have to
> -        * sanity check stuff after all the locks have been acquired
> -        * to see if we still have the right inodes, directories, etc.
> -        */
> -       lock_mode = xfs_ilock_map_shared(dp1);
> +       *ipp2 = NULL;
> +
>         IHOLD(ip1);
>         xfs_itrace_ref(ip1);
> -       inum1 = ip1->i_ino;
> -
> -       /*
> -        * Unlock dp1 and lock dp2 if they are different.
> -        */
> -       if (diff_dirs) {
> -               xfs_iunlock_map_shared(dp1, lock_mode);
> -               lock_mode = xfs_ilock_map_shared(dp2);
> +       lock_mode = xfs_ilock_map_shared(dp2);
> +       error = xfs_dir_lookup(NULL, dp2, name2, &inum2);
> +       if (!error) {
> +               xfs_iunlock(dp2, lock_mode);
> +               error = xfs_iget(dp2->i_mount, NULL, inum2, 0, 0, ipp2, 0);
> +               xfs_ilock(dp2, lock_mode);

Is this xfs_dir_lookup call actually needed?

It seems that do_rename() in namei.c calls lookup_hash/__lookup_hash
which does an inode->i_op->lookup() to get the dentry. This will do
the xfs_lookup anyway, taking a refernence if the target exists.

        error = do_path_lookup(newdfd, newname, LOOKUP_PARENT, &newnd);
        ...
        new_dentry = lookup_hash(&newnd);
        ...
        error = vfs_rename(old_dir->d_inode, old_dentry,
                                   new_dir->d_inode, new_dentry);

Therefore, target_ip/ipp2 is already known then when xfs_rename/
xfs_lock_for_rename is called.

Barry.