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. -- 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
