On Wed, Feb 27, 2019 at 03:52:42PM +0100, David Sterba wrote: > > > > - 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". So the unchecked access to inode flags does not happen, I did not have a clear picture of the change. The locks are only moved outside of _prep to the caller. The lock/unlock look better in the same function, they're close toe each other on the same page, so ok from me.
