Re: [PATCH] Btrfs: fix a crash of clone with inline extents's split

[Liu Bo <bo.li.liu@oracle.com>]

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