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

[David Sterba <dsterba@suse.cz>]

On Wed, Jan 17, 2024 at 06:43:50AM +1030, Qu Wenruo wrote:
> 
> 
> 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.

No need to, I've added it to devel already, thanks.