* [PATCH] Btrfs: fix a crash of clone with inline extents's split @ 2014-03-10 10:56 Liu Bo 2014-03-17 14:41 ` David Sterba 2014-04-09 17:42 ` David Disseldorp 0 siblings, 2 replies; 6+ messages in thread From: Liu Bo @ 2014-03-10 10:56 UTC (permalink / raw) To: linux-btrfs; +Cc: David Disseldorp 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.liu@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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Btrfs: fix a crash of clone with inline extents's split 2014-03-10 10:56 [PATCH] Btrfs: fix a crash of clone with inline extents's split Liu Bo @ 2014-03-17 14:41 ` David Sterba 2014-03-18 10:55 ` Liu Bo 2014-04-09 17:42 ` David Disseldorp 1 sibling, 1 reply; 6+ messages in thread From: David Sterba @ 2014-03-17 14:41 UTC (permalink / raw) To: Liu Bo; +Cc: linux-btrfs, David Disseldorp 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.liu@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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Btrfs: fix a crash of clone with inline extents's split 2014-03-17 14:41 ` David Sterba @ 2014-03-18 10:55 ` Liu Bo 2014-03-19 18:47 ` David Sterba 0 siblings, 1 reply; 6+ messages in thread From: Liu Bo @ 2014-03-18 10:55 UTC (permalink / raw) To: dsterba, linux-btrfs, David Disseldorp 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.liu@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Btrfs: fix a crash of clone with inline extents's split 2014-03-18 10:55 ` Liu Bo @ 2014-03-19 18:47 ` David Sterba 0 siblings, 0 replies; 6+ messages in thread From: David Sterba @ 2014-03-19 18:47 UTC (permalink / raw) To: Liu Bo; +Cc: linux-btrfs, David Disseldorp 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". ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Btrfs: fix a crash of clone with inline extents's split 2014-03-10 10:56 [PATCH] Btrfs: fix a crash of clone with inline extents's split Liu Bo 2014-03-17 14:41 ` David Sterba @ 2014-04-09 17:42 ` David Disseldorp 2014-04-09 17:42 ` [PATCH] btrfs/035: update clone test to expect EOPNOTSUPP David Disseldorp 1 sibling, 1 reply; 6+ messages in thread From: David Disseldorp @ 2014-04-09 17:42 UTC (permalink / raw) To: Liu Bo; +Cc: linux-btrfs 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] btrfs/035: update clone test to expect EOPNOTSUPP 2014-04-09 17:42 ` David Disseldorp @ 2014-04-09 17:42 ` David Disseldorp 0 siblings, 0 replies; 6+ messages in thread From: David Disseldorp @ 2014-04-09 17:42 UTC (permalink / raw) To: Liu Bo; +Cc: linux-btrfs, David Disseldorp With kernel commit 00fdf13a2e9f313a044288aa59d3b8ec29ff904a, the first clone-range overwrite attempt now fails with EOPNOTSUPP. FIXME: The second clone-range causes EIO on subsequent read attempts. Signed-off-by: David Disseldorp <ddiss@suse.de> --- tests/btrfs/035 | 10 ++++++++++ tests/btrfs/035.out | 5 +++++ 2 files changed, 15 insertions(+) diff --git a/tests/btrfs/035 b/tests/btrfs/035 index 6808179..21a9059 100755 --- a/tests/btrfs/035 +++ b/tests/btrfs/035 @@ -57,21 +57,31 @@ src_str="aaaaaaaaaa" echo -n "$src_str" > $SCRATCH_MNT/src $CLONER_PROG $SCRATCH_MNT/src $SCRATCH_MNT/src.clone1 +cat $SCRATCH_MNT/src.clone1 +echo src_str="bbbbbbbbbbcccccccccc" echo -n "$src_str" > $SCRATCH_MNT/src $CLONER_PROG $SCRATCH_MNT/src $SCRATCH_MNT/src.clone2 +cat $SCRATCH_MNT/src.clone2 +echo +# Prior to kernel commit 00fdf13a2e9f313a044288aa59d3b8ec29ff904a, this clone +# resulted in a BUG_ON in __btrfs_drop_extents(). The kernel now returns EINVAL +# up to userspace. snap_src_sz=`ls -lah $SCRATCH_MNT/src.clone1 | awk '{print $5}'` echo "attempting ioctl (src.clone1 src)" $CLONER_PROG -s 0 -d 0 -l ${snap_src_sz} \ $SCRATCH_MNT/src.clone1 $SCRATCH_MNT/src +cat $SCRATCH_MNT/src +echo snap_src_sz=`ls -lah $SCRATCH_MNT/src.clone2 | awk '{print $5}'` echo "attempting ioctl (src.clone2 src)" $CLONER_PROG -s 0 -d 0 -l ${snap_src_sz} \ $SCRATCH_MNT/src.clone2 $SCRATCH_MNT/src +cat $SCRATCH_MNT/src status=0 ; exit diff --git a/tests/btrfs/035.out b/tests/btrfs/035.out index f86cadf..0ea2c4f 100644 --- a/tests/btrfs/035.out +++ b/tests/btrfs/035.out @@ -1,3 +1,8 @@ QA output created by 035 +aaaaaaaaaa +bbbbbbbbbbcccccccccc attempting ioctl (src.clone1 src) +clone failed: Operation not supported +bbbbbbbbbbcccccccccc attempting ioctl (src.clone2 src) +bbbbbbbbbbcccccccccc -- 1.8.4.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-04-09 17:34 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-03-10 10:56 [PATCH] Btrfs: fix a crash of clone with inline extents's split Liu Bo 2014-03-17 14:41 ` David Sterba 2014-03-18 10:55 ` Liu Bo 2014-03-19 18:47 ` David Sterba 2014-04-09 17:42 ` David Disseldorp 2014-04-09 17:42 ` [PATCH] btrfs/035: update clone test to expect EOPNOTSUPP David Disseldorp
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).