On Mon, Jan 30, 2012 at 6:03 PM, Jan Schmidt <list.bt...@jan-o-sch.net> wrote: > On 30.01.2012 07:33, Li Zefan wrote: >> 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. > > I see where the negative offset comes from. But what can this offset be > used for? >
The offset in backref isn't equal to the offset of the file extent, it's just a hint for searching file extents, Negative offset isn't that bad if you consider it as value that is close to zero. >> 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. > > Reducing the number of backrefs is a good thing. In case of count > 1, > it's clear that the offset cannot reference all of the extent data > items. We have different design choices: > > a) Use the above computation and leave the filesystem with an unusable > offset value for extent backrefs. > > b) Use either one of the extent data item offsets this backref references. > > c) Always use a predefined constant (like 0 or -1) when count > 1. > > d) Disallow count > 1 for those refs and turn them into shared refs as > soon as count gets > 1. > > I don't like a :-) > > -Jan > -- > 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 -- 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
