Btrfs: allow cloning ranges within the same file
PATCH 1/1 Btrfs: allow cloning ranges within the same file
It's safe and easy to do so, provided the ranges don't overlap.
--- fs/btrfs/ioctl.c | 23 ++++++++++++++++------- 1 files changed, 16 insertions(+), 7 deletions(-)
fs/btrfs/ioctl.c +16 -7
@@ -2223,8 +2223,6 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd,* decompress into destination's address_space (the file offset * may change, so source mapping won't do), then recompress (or * otherwise reinsert) a subrange. -* - allow ranges within the same file to be cloned (provided -* they don't overlap)? */ /* the destination must be opened for writing */
@@ -2247,8 +2245,6 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd, src = src_file->f_dentry->d_inode; ret = -EINVAL ; -if (src == inode) - goto out_fput;/* the src must be open for reading */ if (!(src_file->f_mode &FMODE_READ ))
@@ -2282,9 +2278,11 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd,if (inode < src) {mutex_lock_nested (&inode->i_mutex,I_MUTEX_PARENT );mutex_lock_nested (&src->i_mutex,I_MUTEX_CHILD ); - }else { + }else if (inode > src) {mutex_lock_nested (&src->i_mutex,I_MUTEX_PARENT );mutex_lock_nested (&inode->i_mutex,I_MUTEX_CHILD ); + }else { +mutex_lock (&inode->i_mutex); }/* determine range to clone */
@@ -2302,6 +2300,13 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd, !IS_ALIGNED(destoff, bs)) goto out_unlock; +/* +* allow ranges within the same file to be cloned only if +* they don't overlap +*/ +if (src == inode && !(off + len <= destoff || destoff + len <= off)) + goto out_unlock; +if (destoff > inode->i_size) { ret =btrfs_cont_expand (inode, inode->i_size, destoff);if (ret)
@@ -2543,8 +2548,12 @@ out:btrfs_release_path (path);unlock_extent (&BTRFS_I (src)->io_tree, off, off+len,GFP_NOFS ); out_unlock: -mutex_unlock (&src->i_mutex); -mutex_unlock (&inode->i_mutex); +if (src != inode) { +mutex_unlock (&src->i_mutex); +mutex_unlock (&inode->i_mutex); + }else { +mutex_unlock (&inode->i_mutex); + }vfree (buf);btrfs_free_path (path); out_fput:
Signature · 1 line
1.7.3.1
I like allowing those clones. However ...
Quoted 43 lines · Li Zefan
It's safe and easy to do so, provided the ranges don't overlap.
Signed-off-by Li Zefan <[email protected]>--- fs/btrfs/ioctl.c | 23 ++++++++++++++++------- 1 files changed, 16 insertions(+), 7 deletions(-)fs/btrfs/ioctl.c +3
-5@@ -2223,8 +2223,6 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd,* decompress into destination's address_space (the file offset * may change, so source mapping won't do), then recompress (or * otherwise reinsert) a subrange. -* - allow ranges within the same file to be cloned (provided -* they don't overlap)? */ /* the destination must be opened for writing */ @@ -2247,8 +2245,6 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd, src = src_file->f_dentry->d_inode; ret = -EINVAL ; -if (src == inode) - goto out_fput;/* the src must be open for reading */ if (!(src_file->f_mode &FMODE_READ ))@@ -2282,9 +2278,11 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd,if (inode < src) {mutex_lock_nested (&inode->i_mutex,I_MUTEX_PARENT );mutex_lock_nested (&src->i_mutex,I_MUTEX_CHILD ); - }else { + }else if (inode > src) {mutex_lock_nested (&src->i_mutex,I_MUTEX_PARENT );mutex_lock_nested (&inode->i_mutex,I_MUTEX_CHILD ); + }else { +mutex_lock (&inode->i_mutex); }/* determine range to clone */
fs/btrfs/ioctl.c static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd, · line 2307
Quoted 8 lines
fs/btrfs/ioctl.c +5
-0@@ -2302,6 +2300,13 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd, !IS_ALIGNED(destoff, bs)) goto out_unlock; +/* +* allow ranges within the same file to be cloned only if +* they don't overlap +*/ +if (src == inode && !(off + len <= destoff || destoff + len <= off))
^^^^^^^^^^^^
... this check isn't sufficient. Two different inodes can reference the same data extent and we must prevent overlap cloning there as well.
-Jan
Quoted 20 lines
fs/btrfs/ioctl.c +8
-2+ goto out_unlock; +if (destoff > inode->i_size) { ret =btrfs_cont_expand (inode, inode->i_size, destoff);if (ret)@@ -2543,8 +2548,12 @@ out:btrfs_release_path (path);unlock_extent (&BTRFS_I (src)->io_tree, off, off+len,GFP_NOFS ); out_unlock: -mutex_unlock (&src->i_mutex); -mutex_unlock (&inode->i_mutex); +if (src != inode) { +mutex_unlock (&src->i_mutex); +mutex_unlock (&inode->i_mutex); + }else { +mutex_unlock (&inode->i_mutex); + }vfree (buf);btrfs_free_path (path); out_fput:
Quoted 46 lines
Quoted 46 lines
It's safe and easy to do so, provided the ranges don't overlap.
Signed-off-by Li Zefan <[email protected]>--- fs/btrfs/ioctl.c | 23 ++++++++++++++++------- 1 files changed, 16 insertions(+), 7 deletions(-)fs/btrfs/ioctl.c +3
-5@@ -2223,8 +2223,6 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd,* decompress into destination's address_space (the file offset * may change, so source mapping won't do), then recompress (or * otherwise reinsert) a subrange. -* - allow ranges within the same file to be cloned (provided -* they don't overlap)? */ /* the destination must be opened for writing */ @@ -2247,8 +2245,6 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd, src = src_file->f_dentry->d_inode; ret = -EINVAL ; -if (src == inode) - goto out_fput;/* the src must be open for reading */ if (!(src_file->f_mode &FMODE_READ ))@@ -2282,9 +2278,11 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd,if (inode < src) {mutex_lock_nested (&inode->i_mutex,I_MUTEX_PARENT );mutex_lock_nested (&src->i_mutex,I_MUTEX_CHILD ); - }else { + }else if (inode > src) {mutex_lock_nested (&src->i_mutex,I_MUTEX_PARENT );mutex_lock_nested (&inode->i_mutex,I_MUTEX_CHILD ); + }else { +mutex_lock (&inode->i_mutex); }/* determine range to clone */ @@ -2302,6 +2300,13 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd, !IS_ALIGNED(destoff, bs)) goto out_unlock;
Quoted 9 lines
fs/btrfs/ioctl.c static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd, · line 2307
Quoted 5 lines
fs/btrfs/ioctl.c +5
-0+/* +* allow ranges within the same file to be cloned only if +* they don't overlap +*/ +if (src == inode && !(off + len <= destoff || destoff + len <= off))^^^^^^^^^^^^... this check isn't sufficient. Two different inodes can reference the same data extent and we must prevent overlap cloning there as well.
Could you be more elaborate on this? I don't know what's the problem you're refering to.
Quoted 56 lines · Li Zefan
Quoted 46 lines
Quoted 46 lines
It's safe and easy to do so, provided the ranges don't overlap.
Signed-off-by Li Zefan <[email protected]>--- fs/btrfs/ioctl.c | 23 ++++++++++++++++------- 1 files changed, 16 insertions(+), 7 deletions(-)fs/btrfs/ioctl.c +3
-5@@ -2223,8 +2223,6 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd,* decompress into destination's address_space (the file offset * may change, so source mapping won't do), then recompress (or * otherwise reinsert) a subrange. -* - allow ranges within the same file to be cloned (provided -* they don't overlap)? */ /* the destination must be opened for writing */ @@ -2247,8 +2245,6 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd, src = src_file->f_dentry->d_inode; ret = -EINVAL ; -if (src == inode) - goto out_fput;/* the src must be open for reading */ if (!(src_file->f_mode &FMODE_READ ))@@ -2282,9 +2278,11 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd,if (inode < src) {mutex_lock_nested (&inode->i_mutex,I_MUTEX_PARENT );mutex_lock_nested (&src->i_mutex,I_MUTEX_CHILD ); - }else { + }else if (inode > src) {mutex_lock_nested (&src->i_mutex,I_MUTEX_PARENT );mutex_lock_nested (&inode->i_mutex,I_MUTEX_CHILD ); + }else { +mutex_lock (&inode->i_mutex); }/* determine range to clone */ @@ -2302,6 +2300,13 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd, !IS_ALIGNED(destoff, bs)) goto out_unlock;Quoted 9 lines
fs/btrfs/ioctl.c static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd, · line 2307
Quoted 5 lines
fs/btrfs/ioctl.c +5
-0+/* +* allow ranges within the same file to be cloned only if +* they don't overlap +*/ +if (src == inode && !(off + len <= destoff || destoff + len <= off))^^^^^^^^^^^^... this check isn't sufficient. Two different inodes can reference the same data extent and we must prevent overlap cloning there as well.
Quoted 2 lines
Could you be more elaborate on this? I don't know what's the problem you're refering to.
I'll try to rephrase:
Why do you disallow cloning ranges within the same file if they overlap? For me, it would be because I've got fear of the headache that occurred if I thought about cloning 80k at offset 16k to the offset 8k.
Original file, let's say, single extent, 128k in size
+-------------------------------------------------------+
| |xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx| |
+-------------------------------------------------------+
^ ^ ^
16k 96k 128kWhat should happen when you clone the said x-range to offset 8k in the same file? I think, we can find a solution for this and thus allow cloning even overlapping ranges within the same file (perhaps at the expense of a headache).
However, if there is no good (agreed) solution for this scenario, I'm saying that we can't solve it for two files (inodes) both referring to the above extent, either.
I'm not sure if my example is helping to illustrate my point. If not, could you please give an example file with an example clone command with overlapping ranges that you believe should fail with EINVAL? I could make my point clearer when building on top of your example, I hope :-)
Thanks,
-Jan