Re: [PATCH 3/3] btrfs: Perform locking/unlocking in btrfs_remap_file_range()

[Filipe Manana <fdmanana@gmail.com>]

On Mon, Feb 25, 2019 at 7:08 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>
> Moves code to make it more readable, so as locking and unlocking is
> done in the same function.
>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/btrfs/ioctl.c | 53 +++++++++++++++++++++--------------------------------
>  1 file changed, 21 insertions(+), 32 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 9c8e1734429c..f0ae1af91ff3 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3975,22 +3975,6 @@ static int btrfs_remap_file_range_prep(struct file *file_in, loff_t pos_in,
>         u64 wb_len;
>         int ret;
>
> -       if (!(remap_flags & REMAP_FILE_DEDUP)) {
> -               struct btrfs_root *root_out = BTRFS_I(inode_out)->root;
> -
> -               if (btrfs_root_readonly(root_out))xfs_reflink_remap_prep
> -                       return -EROFS;
> -
> -               if (file_in->f_path.mnt != file_out->f_path.mnt ||
> -                   inode_in->i_sb != inode_out->i_sb)
> -                       return -EXDEV;
> -       }

Why move these checks?
The goal of the _prep function (both btrfs and vfs)  is to have the
checks for all needed conditions in one place.

As for the lock/unlock, it follows the same pattern from xfs
(xfs_reflink_remap_prep and xfs_file_remap_range).
No complaints about changing this, I'm just neutral about it.

> -
> -       if (same_inode)
> -               inode_lock(inode_in);
> -       else
> -               btrfs_double_inode_lock(inode_in, inode_out);
> -
>         /*
>          * Now that the inodes are locked, we need to start writeback ourselves
>          * and can not rely on the writeback from the VFS's generic helper
> @@ -4022,26 +4006,14 @@ static int btrfs_remap_file_range_prep(struct file *file_in, loff_t pos_in,
>         ret = btrfs_wait_ordered_range(inode_in, ALIGN_DOWN(pos_in, bs),
>                                        wb_len);
>         if (ret < 0)
> -               goto out_unlock;
> +               return ret;
>         ret = btrfs_wait_ordered_range(inode_out, ALIGN_DOWN(pos_out, bs),
>                                        wb_len);
>         if (ret < 0)
> -               goto out_unlock;
> +               return ret;
>
> -       ret = generic_remap_file_range_prep(file_in, pos_in, file_out, pos_out,
> +       return generic_remap_file_range_prep(file_in, pos_in, file_out, pos_out,
>                                             len, remap_flags);
> -       if (ret < 0 || *len == 0)
> -               goto out_unlock;
> -
> -       return 0;
> -
> - out_unlock:
> -       if (same_inode)
> -               inode_unlock(inode_in);
> -       else
> -               btrfs_double_inode_unlock(inode_in, inode_out);
> -
> -       return ret;
>  }
>
>  loff_t btrfs_remap_file_range(struct file *src_file, loff_t off,
> @@ -4056,16 +4028,33 @@ loff_t btrfs_remap_file_range(struct file *src_file, loff_t off,
>         if (remap_flags & ~(REMAP_FILE_DEDUP | REMAP_FILE_ADVISORY))
>                 return -EINVAL;
>
> +       if (!(remap_flags & REMAP_FILE_DEDUP)) {
> +               struct btrfs_root *root_out = BTRFS_I(dst_inode)->root;
> +
> +               if (btrfs_root_readonly(root_out))
> +                       return -EROFS;
> +
> +               if (src_file->f_path.mnt != dst_file->f_path.mnt ||
> +                   src_inode->i_sb != dst_inode->i_sb)
> +                       return -EXDEV;
> +       }
> +
> +       if (same_inode)
> +               inode_lock(src_inode);
> +       else
> +               btrfs_double_inode_lock(src_inode, dst_inode);
> +
>         ret = btrfs_remap_file_range_prep(src_file, off, dst_file, destoff,
>                                           &len, remap_flags);
>         if (ret < 0 || len == 0)
> -               return ret;
> +               goto out_unlock;
>
>         if (remap_flags & REMAP_FILE_DEDUP)
>                 ret = btrfs_extent_same(src_inode, off, len, dst_inode, destoff);
>         else
>                 ret = btrfs_clone_files(dst_file, src_file, off, len, destoff);
>
> +out_unlock:
>         if (same_inode)
>                 inode_unlock(src_inode);
>         else
> --
> 2.16.4
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”