On Mon, Feb 25, 2019 at 7:08 PM Goldwyn Rodrigues <rgoldwyn@xxxxxxx> wrote:
>
> From: Goldwyn Rodrigues <rgoldwyn@xxxxxxxx>
>
> Moves code to make it more readable, so as locking and unlocking is
> done in the same function.
>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@xxxxxxxx>
> ---
> 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.”
