[PATCH] Btrfs: fix a crash of clone with inline extents's split
Thanks for the BUG_ON() fix here. Strangely, I'm now seeing EIO returned for reads following the second clone-range. Please see the subsequent xfstests patch. Cheers, David -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: fix a crash of clone with inline extents's split
On Tue, Mar 18, 2014 at 06:55:13PM +0800, Liu Bo wrote: On Mon, Mar 17, 2014 at 03:41:31PM +0100, David Sterba wrote: There are enough EINVAL's that verify correcntess of the input parameters and it's not always clear which one fails. The EOPNOTSUPP errocode is close to the true reason of the failure, but it could be misinterpreted as if the whole clone operation is not supported, so it's not all correct but IMO better than EINVAL. Yep, I was hesitating on these two errors while making the patch, but I prefer EINVAL rather than EOPNOTSUPP because of the reason you've stated. I think it'd be good to add one more btrfs_printk message to clarify what's happening here, agree? I don't think a printk is the right thing here, this means that if an error happens somebody has to look into the log what happened and act accordingly. The EOPNOTSUPP errorcode would allow an application to do a fallback action, ie. copy the data instead of cloning. The same as if the clone ioctl would not exist at all. EINVAL says you didn't give me valid arguments to work with. -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: fix a crash of clone with inline extents's split
On Mon, Mar 17, 2014 at 03:41:31PM +0100, David Sterba wrote:
On Mon, Mar 10, 2014 at 06:56:07PM +0800, Liu Bo wrote:
xfstests's btrfs/035 triggers a BUG_ON, which we use to detect the split
of inline extents in __btrfs_drop_extents().
For inline extents, we cannot duplicate another EXTENT_DATA item, because
it breaks the rule of inline extents, that is, 'start offset' needs to be 0.
We have set limitations for the source inode's compressed inline extents,
because it needs to decompress and recompress. Now the destination inode's
inline extents also need similar limitations.
The limitation (by lack of implementation, not by design) of compressed
inline extents is there, but it's impossible to reach. The inline
extents are never longer than the 'inline limit' (the ~3916 size), so
the comment is more a note to the future.
You're adding another limitation to avoid a crash, but I don't agree
that EINVAL is right here, due to the fact that it's lack of
implementation, not a real error.
There are enough EINVAL's that verify correcntess of the input
parameters and it's not always clear which one fails. The EOPNOTSUPP
errocode is close to the true reason of the failure, but it could be
misinterpreted as if the whole clone operation is not supported, so it's
not all correct but IMO better than EINVAL.
Yep, I was hesitating on these two errors while making the patch, but I
prefer EINVAL rather than EOPNOTSUPP because of the reason you've stated.
I think it'd be good to add one more btrfs_printk message to clarify what's
happening here, agree?
The most common case of 'cp --reflink' is not affected by this.
With this, xfstests btrfs/035 doesn't run into panic.
Signed-off-by: Liu Bo bo.li@oracle.com
---
fs/btrfs/file.c | 15 ---
fs/btrfs/ioctl.c | 10 ++
2 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 0165b86..2c34a04 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3090,8 +3090,9 @@ process_slot:
new_key.offset + datal,
1);
if (ret) {
- btrfs_abort_transaction(trans, root,
- ret);
+ if (ret != -EINVAL)
+ btrfs_abort_transaction(trans,
+ root, ret);
The error comes from __btrfs_drop_extents and all callers would need to
be updated (or at least reviewed) with the 'ret != ...' check as well,
because it changes the semantics. And I'm not sure if to the right
direction.
Good point, Dave, actually I missed this part before, just checked for
callers of __btrfs_drop_extents() and btrfs_drop_extents(), luckily EINVAL is
not a special one at these places, the error is just returned to upper callers.
btrfs_end_transaction(trans, root);
goto out;
}
@@ -3175,8 +3176,9 @@ 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)?
True, but unrelated.
yep, that's right, will clean it up.
Thanks for the comments!
-liubo
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: fix a crash of clone with inline extents's split
On Mon, Mar 10, 2014 at 06:56:07PM +0800, Liu Bo wrote:
xfstests's btrfs/035 triggers a BUG_ON, which we use to detect the split
of inline extents in __btrfs_drop_extents().
For inline extents, we cannot duplicate another EXTENT_DATA item, because
it breaks the rule of inline extents, that is, 'start offset' needs to be 0.
We have set limitations for the source inode's compressed inline extents,
because it needs to decompress and recompress. Now the destination inode's
inline extents also need similar limitations.
The limitation (by lack of implementation, not by design) of compressed
inline extents is there, but it's impossible to reach. The inline
extents are never longer than the 'inline limit' (the ~3916 size), so
the comment is more a note to the future.
You're adding another limitation to avoid a crash, but I don't agree
that EINVAL is right here, due to the fact that it's lack of
implementation, not a real error.
There are enough EINVAL's that verify correcntess of the input
parameters and it's not always clear which one fails. The EOPNOTSUPP
errocode is close to the true reason of the failure, but it could be
misinterpreted as if the whole clone operation is not supported, so it's
not all correct but IMO better than EINVAL.
The most common case of 'cp --reflink' is not affected by this.
With this, xfstests btrfs/035 doesn't run into panic.
Signed-off-by: Liu Bo bo.li@oracle.com
---
fs/btrfs/file.c | 15 ---
fs/btrfs/ioctl.c | 10 ++
2 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 0165b86..2c34a04 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3090,8 +3090,9 @@ process_slot:
new_key.offset + datal,
1);
if (ret) {
- btrfs_abort_transaction(trans, root,
- ret);
+ if (ret != -EINVAL)
+ btrfs_abort_transaction(trans,
+ root, ret);
The error comes from __btrfs_drop_extents and all callers would need to
be updated (or at least reviewed) with the 'ret != ...' check as well,
because it changes the semantics. And I'm not sure if to the right
direction.
btrfs_end_transaction(trans, root);
goto out;
}
@@ -3175,8 +3176,9 @@ 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)?
True, but unrelated.
+ *
+ * - split destination inode's inline extents. The inline extents can
+ * be either compressed or non-compressed.
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Btrfs: fix a crash of clone with inline extents's split
xfstests's btrfs/035 triggers a BUG_ON, which we use to detect the split
of inline extents in __btrfs_drop_extents().
For inline extents, we cannot duplicate another EXTENT_DATA item, because
it breaks the rule of inline extents, that is, 'start offset' needs to be 0.
We have set limitations for the source inode's compressed inline extents,
because it needs to decompress and recompress. Now the destination inode's
inline extents also need similar limitations.
With this, xfstests btrfs/035 doesn't run into panic.
Signed-off-by: Liu Bo bo.li@oracle.com
---
fs/btrfs/file.c | 15 ---
fs/btrfs/ioctl.c | 10 ++
2 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 0165b86..2c34a04 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -798,7 +798,10 @@ next_slot:
*/
if (start key.offset end extent_end) {
BUG_ON(del_nr 0);
- BUG_ON(extent_type == BTRFS_FILE_EXTENT_INLINE);
+ if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
+ ret = -EINVAL;
+ break;
+ }
memcpy(new_key, key, sizeof(new_key));
new_key.offset = start;
@@ -841,7 +844,10 @@ next_slot:
* | extent |
*/
if (start = key.offset end extent_end) {
- BUG_ON(extent_type == BTRFS_FILE_EXTENT_INLINE);
+ if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
+ ret = -EINVAL;
+ break;
+ }
memcpy(new_key, key, sizeof(new_key));
new_key.offset = end;
@@ -864,7 +870,10 @@ next_slot:
*/
if (start key.offset end = extent_end) {
BUG_ON(del_nr 0);
- BUG_ON(extent_type == BTRFS_FILE_EXTENT_INLINE);
+ if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
+ ret = -EINVAL;
+ break;
+ }
btrfs_set_file_extent_num_bytes(leaf, fi,
start - key.offset);
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 9a90445..49f03ab 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3090,8 +3090,9 @@ process_slot:
new_key.offset + datal,
1);
if (ret) {
- btrfs_abort_transaction(trans, root,
- ret);
+ if (ret != -EINVAL)
+ btrfs_abort_transaction(trans,
+ root, ret);
btrfs_end_transaction(trans, root);
goto out;
}
@@ -3175,8 +3176,9 @@ 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)?
+*
+* - split destination inode's inline extents. The inline extents can
+* be either compressed or non-compressed.
*/
/* the destination must be opened for writing */
--
1.8.2.1
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
