Re: [PATCH v2] btrfs-progs: convert-ext2: insert a dummy inode item before inode ref

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2024/1/17 05:17, David Sterba wrote:
> On Sat, Jan 13, 2024 at 07:07:06PM +1030, Qu Wenruo wrote:
>> [BUG]
>> There is a report about failed btrfs-convert, which shows the following
>> error:
>>
>>    Create btrfs metadata
>>    corrupt leaf: root=5 block=5001931145216 slot=1 ino=89911763, invalid previous key objectid, have 89911762 expect 89911763
>>    leaf 5001931145216 items 336 free space 7 generation 90 owner FS_TREE
>>    leaf 5001931145216 flags 0x1(WRITTEN) backref revision 1
>>    fs uuid 8b69f018-37c3-4b30-b859-42ccfcbe2449
>>    chunk uuid 448ce78c-ea41-49f6-99dc-46ad80b93da9
>>            item 0 key (89911762 INODE_REF 3858733) itemoff 16222 itemsize 61
>>                    index 171 namelen 51 name: [FILENAME1]
>>            item 1 key (89911763 INODE_REF 3858733) itemoff 16161 itemsize 61
>>                    index 103 namelen 51 name: [FILENAME2]
>>
>> [CAUSE]
>> When iterating a directory, btrfs-convert would insert the DIR_ITEMs,
>> along with the INODE_REF of that inode.
>>
>> This leads to above stray INODE_REFs, and trigger the tree-checker.
>>
>> This can only happen for large fs, as for most cases we have all these
>> modified tree blocks cached, thus tree-checker won't be triggered.
>> But when the tree block cache is not hit, and we have to read from disk,
>> then such behavior can lead to above tree-checker error.
>>
>> [FIX]
>> Insert a dummy INODE_ITEM for the INODE_REF first, the inode items would
>> be updated when iterating the child inode of the directory.
>>
>> Issue: #731
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> Thanks, the cached data are uncovering some bugs, I wonder if
> https://github.com/kdave/btrfs-progs/issues/349 could be also caused by
> that.

Unfortunately the csum is not the same problem at all.

I don't have any clue yet, but can take sometime to look into it since
there is a reproducer.

>
>> ---
>>   check/mode-common.h   | 15 ---------------
>>   common/utils.h        | 16 ++++++++++++++++
>>   convert/source-ext2.c | 30 ++++++++++++++++++++----------
>>   convert/source-fs.c   | 20 ++++++++++++++++++++
>>   4 files changed, 56 insertions(+), 25 deletions(-)
>>
>> ---
>> Changelog:
>> v2:
>> - Initialized dummy inodes' mode/generation/transid
>>    As the mode can still trigger tree-checker warnings.
>>
>> diff --git a/check/mode-common.h b/check/mode-common.h
>> index 894bbbb8141b..80672e51e870 100644
>> --- a/check/mode-common.h
>> +++ b/check/mode-common.h
>> @@ -167,21 +167,6 @@ static inline bool is_valid_imode(u32 imode)
>>
>>   int recow_extent_buffer(struct btrfs_root *root, struct extent_buffer *eb);
>>
>> -static inline u32 btrfs_type_to_imode(u8 type)
>> -{
>> -	static u32 imode_by_btrfs_type[] = {
>> -		[BTRFS_FT_REG_FILE]	= S_IFREG,
>> -		[BTRFS_FT_DIR]		= S_IFDIR,
>> -		[BTRFS_FT_CHRDEV]	= S_IFCHR,
>> -		[BTRFS_FT_BLKDEV]	= S_IFBLK,
>> -		[BTRFS_FT_FIFO]		= S_IFIFO,
>> -		[BTRFS_FT_SOCK]		= S_IFSOCK,
>> -		[BTRFS_FT_SYMLINK]	= S_IFLNK,
>> -	};
>> -
>> -	return imode_by_btrfs_type[(type)];
>> -}
>
> Why did you move this helper to utils.h? Here it's available for
> anything that needs it. Mkfs and convert share some code, no style
> problem to cross include from each other. Also moving it to utils.h is
> going the opposite way, it's a header that's a default if there's no
> better place. Lot of code has been factored out of it.

OK, my initial problem is about including headers from check/, but since
it's not a problem then I'm totally fine.

Would update the patch and reflect that.

Thanks,
Qu
>