On Tue, Feb 26, 2019 at 06:57:17AM -0600, Goldwyn Rodrigues wrote:
> On 12:08 26/02, Filipe Manana wrote:
> > 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.
>
> In the original flow, these checks were done without locks.
> But I suppose they can be done with locks held as well.
The locking does not affect the above checks, so no problem here.
> >
> > 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.
> >
>
> I just read the xfs code and yes it is similar. Locking and unlocking
> in separate functions makes it difficult to read, especially
> when it can be done in the same function.
>
> > > -
> > > - if (same_inode)
> > > - inode_lock(inode_in);
> > > - else
> > > - btrfs_double_inode_lock(inode_in, inode_out);
> > > -
But removing the checks from here can't be done because there's inode
compatibility flag check done right here (in current code, since commit
500710d3b872) but it's not in this diff.
Otherwise there's a race with chatter, once fixed by
b5c40d598f5408bd0ca22dfffa82f03cd9433f23 "Btrfs: fix clone vs chattr
NODATASUM race".
