* [PATCH] Btrfs: allow cloning ranges within the same file
@ 2012-01-30 9:14 Li Zefan
2012-01-30 9:56 ` Jan Schmidt
0 siblings, 1 reply; 4+ messages in thread
From: Li Zefan @ 2012-01-30 9:14 UTC (permalink / raw)
To: linux-btrfs@vger.kernel.org; +Cc: Jan Schmidt
It's safe and easy to do so, provided the ranges don't overlap.
Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
fs/btrfs/ioctl.c | 23 ++++++++++++++++-------
1 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 0b06a5c..8fcd671 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -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:
--
1.7.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] Btrfs: allow cloning ranges within the same file
2012-01-30 9:14 [PATCH] Btrfs: allow cloning ranges within the same file Li Zefan
@ 2012-01-30 9:56 ` Jan Schmidt
2012-02-01 9:34 ` Li Zefan
0 siblings, 1 reply; 4+ messages in thread
From: Jan Schmidt @ 2012-01-30 9:56 UTC (permalink / raw)
To: Li Zefan; +Cc: linux-btrfs@vger.kernel.org
I like allowing those clones. However ...
On 30.01.2012 10:14, Li Zefan wrote:
> It's safe and easy to do so, provided the ranges don't overlap.
>
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
> fs/btrfs/ioctl.c | 23 ++++++++++++++++-------
> 1 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 0b06a5c..8fcd671 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -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))
^^^^^^^^^^^^
... this check isn't sufficient. Two different inodes can reference the
same data extent and we must prevent overlap cloning there as well.
-Jan
> + 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:
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Btrfs: allow cloning ranges within the same file
2012-01-30 9:56 ` Jan Schmidt
@ 2012-02-01 9:34 ` Li Zefan
2012-02-02 21:07 ` Jan Schmidt
0 siblings, 1 reply; 4+ messages in thread
From: Li Zefan @ 2012-02-01 9:34 UTC (permalink / raw)
To: Jan Schmidt; +Cc: linux-btrfs@vger.kernel.org
>> It's safe and easy to do so, provided the ranges don't overlap.
>>
>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>> ---
>> fs/btrfs/ioctl.c | 23 ++++++++++++++++-------
>> 1 files changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 0b06a5c..8fcd671 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -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))
> ^^^^^^^^^^^^
> ... 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.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Btrfs: allow cloning ranges within the same file
2012-02-01 9:34 ` Li Zefan
@ 2012-02-02 21:07 ` Jan Schmidt
0 siblings, 0 replies; 4+ messages in thread
From: Jan Schmidt @ 2012-02-02 21:07 UTC (permalink / raw)
To: Li Zefan; +Cc: linux-btrfs@vger.kernel.org
On 01.02.2012 10:34, Li Zefan wrote:
>>> It's safe and easy to do so, provided the ranges don't overlap.
>>>
>>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>>> ---
>>> fs/btrfs/ioctl.c | 23 ++++++++++++++++-------
>>> 1 files changed, 16 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>> index 0b06a5c..8fcd671 100644
>>> --- a/fs/btrfs/ioctl.c
>>> +++ b/fs/btrfs/ioctl.c
>>> @@ -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))
>> ^^^^^^^^^^^^
>> ... 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.
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 128k
What 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-02-02 21:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-01-30 9:14 [PATCH] Btrfs: allow cloning ranges within the same file Li Zefan
2012-01-30 9:56 ` Jan Schmidt
2012-02-01 9:34 ` Li Zefan
2012-02-02 21:07 ` Jan Schmidt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).