Re: [PATCH 12/12] Btrfs: Fix file clone when source offset is not 0

[Li Zefan <lizf@cn.fujitsu.com>]

Jan Schmidt wrote:
> I was looking at the clone range ioctl and have some remarks:
> 
> On 27.01.2011 09:46, Li Zefan wrote:
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index f87552a..1b61dab 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -1788,7 +1788,10 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd,
>>  
>>  			memcpy(&new_key, &key, sizeof(new_key));
>>  			new_key.objectid = inode->i_ino;
>> -			new_key.offset = key.offset + destoff - off;
>> +			if (off <= key.offset)
>> +				new_key.offset = key.offset + destoff - off;
>> +			else
>> +				new_key.offset = destoff;
> 						 ^^^^^^^
> 1) This looks spurious to me. What if destoff isn't aligned? That's what
> the "key.offset - off" code above is for. Before the patch, the code
> didn't work at all, I agree. But this fix can only work for aligned
> requests.
> 
> 2) The error in new_key also has propagated to the extent item's backref
> and wasn't fixed there. I did a range clone and ended up with an extent
> item like that:
>         item 30 key (1318842368 EXTENT_ITEM 131072) itemoff 1047
> itemsize 169
>                 extent refs 8 gen 11103 flags 1
> [...]
>                 extent data backref root 257 objectid 272 offset
> 18446744073709494272 count 1
> 
> The last offset (equal to -14 * 4k) is obviously wrong. I didn't figure
> out how the variables are computed, but it looks like there's something
> wrong with the "datao" u64 to me.
> 

Unfortunately this is expected. The calculation is:

extent_item.extent_data_ref.offset = file_pos - file_extent.extent_offset

so you may get negative offset.

The design idea was to reduce the number of extent backrefs in that
a data backref can point to different file extents in the same file
(in this case the "count" field > 1). We didn't expect nagetive
offset until range clone was implemented.