Yan, Zheng wrote: > On Thu, Aug 25, 2011 at 3:56 PM, Li Zefan <l...@cn.fujitsu.com> wrote: >> We have an offset in file extent to indicate its position in the >> corresponding extent item in extent tree. We also have an offset in >> extent item to indicate the start position of the file extent that >> uses this item. >> >> The math is: >> >> extent_item.extent_data_ref.offset = file_pos - file_extent.extent_offset. >> >> e1 >> disk extents: |--------------| >> ^ >> | e2 >> | |-----------------| >> | | ^ >> | | | >> v v | >> file extents: |----- f1 -----|----- f2 -----| >> >> So it looks like e2.offset points to f1 not f2. Therefore given an extent >> item, >> we'll have to search through all the file extents in an inode to find the >> relative file extent in the worst case, which makes this field somewhat >> useless. >> > > The reason for this is reducing number of file extent backref itmes.
It seems to me a rare case, which isn't worth the complexity and inconvenience it brings, and it requires an extra field (.count). > we don't have to search all the file extents because the file extent size > is limited and we have extent_data_ref.count. Yes we have to, and for a big file with many small file extents, the extent number is not trivial. > >> What makes things worse is the above fomula can make the offset a negative >> value (cast to u64): >> >> # touch /mnt/dst >> # clone_range -s 8192 -d 4096 /mnt/src /mnt/dst >> # umount /mnt >> # btrfs-debug-tree /dev/sda7 >> ... >> item 2 key (12582912 EXTENT_ITEM 49152) itemoff 3865 itemsize 82 >> extent refs 2 gen 8 flags 1 >> extent data backref root 5 objectid 258 offset >> 18446744073709543424 count 1 >> extent data backref root 5 objectid 257 offset 0 count 1 >> ... >> >> and relocation won't work in this case: >> >> # mount /dev/sda7 /mnt >> # rm /mnt/src >> # sync >> # btrfs fi bal /mnt >> (kernel warning !!) >> (hung up !!) >> >> I don't see the necessity or benefit of the substraction in the fomula, >> and I think the correct one is: >> >> extent_item.extent_data_ref.offset = file_pos >> >> (As a side effect thereafter we don't need extent_data_ref.count) >> >> That's what this patch does. Unfornately it is an incompatable change >> in disk format. >> >> So I think we have to live with this defect, just fix relocation for >> the negative offset case ? > > I prefer fixing relocation. > Sure, though I would prefer the alternative if not for the stablity of disk format. -- 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
