On Fri, Aug 26, 2011 at 10:00 AM, Li Zefan <l...@cn.fujitsu.com> wrote: > 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). > Random write workload isn't a rare case.
>> 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. > Max file extent size is 128M, so only need to scan a 128M range in the worst case. >> >>> 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
