Re: [PATCH v2] btrfs-progs: convert: Add 64 bit block numbers support

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

在 2024/6/13 09:32, David Sterba 写道:
> On Wed, Jun 12, 2024 at 10:37:43PM +0200, David Sterba wrote:
>> On Tue, Jun 11, 2024 at 06:03:30AM +0000, Srivathsa Dara wrote:
>>>> 在 2024/6/6 19:52, Srivathsa Dara 写道:
>>>>>    	if (err)
>>>>>    		goto error;
>>>>> diff --git a/convert/source-ext2.h b/convert/source-ext2.h index
>>>>> d204aac5..62c9b1fa 100644
>>>>> --- a/convert/source-ext2.h
>>>>> +++ b/convert/source-ext2.h
>>>>> @@ -46,7 +46,7 @@ struct btrfs_trans_handle;
>>>>>    #define ext2fs_get_block_bitmap_range2 ext2fs_get_block_bitmap_range
>>>>>    #define ext2fs_inode_data_blocks2 ext2fs_inode_data_blocks
>>>>>    #define ext2fs_read_ext_attr2 ext2fs_read_ext_attr
>>>>> -#define ext2fs_blocks_count(s)		((s)->s_blocks_count)
>>>>> +#define ext2fs_blocks_count(s)		(((s)->s_blocks_count_hi << 32) | (s)->s_blocks_count)
>>>>
>>>> This is definitely needed, or it would trigger the ASSERT().
>>>>
>>>> But again, the newer btrfs-progs no longer go with internally defined ext2fs_blocks_count(), but using the one from e2fsprogs headers, and the library version is already returning blk64_t.
>>>
>>> Okay, got it.
>>>
>>> Tested the code base with the commit c23e068aaf91, it does handle 64 bit block numbers.
>>
>> So, is this patch still needed? I'm not sure after Qu fixed the
>> iteration, the tests pass without the patch too.
>
> Locally the test succeeds (e2fsprogs 1.47.0), however in the github CI
> it fails with:
>
> mke2fs -t ext4 -b 4096 -F /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img
> mke2fs 1.46.5 (30-Dec-2021)
> mke2fs: Device size reported to be zero.  Invalid partition specified, or
> 	partition table wasn't reread after running fdisk, due to
> 	a modified partition being busy and in use.  You may need to reboot
> 	to re-read your partition table.

The error is from mke2fs, and considering how old the version is in CI,
it may be e2fsprogs itself unable to properly detect the 16T file.

If we begin to start add checks on e2fsprogs, it's going to end up ugly...

Any good idea to go forward?
Update CI (which seems impossible) or make the mke2fs part as mayfail
and skip it?

Thanks,
Qu
>
> This is the updated test case 018-fs-size-overflow and in the code the change
> is the updated definition of ext2fs_blocks_count.
>
> I'll put the test case to a topic branch and remove it from devel so the
> CI can be used. You can open a pull request to verify if things work.
>