Message-ID: <d8eaab72-d917-4dab-aa3a-a3946450a5e4@suse.com>
Date: Mon, 6 May 2024 19:55:03 +0930
From: Qu Wenruo <wqu@...e.com>
To: Anand Jain <anand.jain@...cle.com>, Qu Wenruo <quwenruo.btrfs@....com>,
 linux-btrfs@...r.kernel.org
Cc: dsterba@...e.com, y16267966@...il.com, linux-ext4@...r.kernel.org
Subject: Re: [PATCH v2 4/4] btrfs-progs: convert: support ext2 unwritten file
 data extents



在 2024/5/6 19:26, Anand Jain 写道:
> 
>>>> . Remove RFC
>>>> . Identify the block with a merged preallocated extent and call 
>>>> fail-safe
>>>> . Qu has an idea that it could be marked as a hole, which may be 
>>>> based on
>>>>    top of this patch.
>>>
>>> Well, my idea of going holes other than preallocated extents is mostly
>>> to avoid the extra @prealloc flag parameter.
>>>
>>> But that's not a big deal for now, as I found the following way to
>>> easily crack your v2 patchset:
> 
> 
> This patch and the below test case are working as designed it is not
> a bug/crack, with the current limitation that it should fail (safer
> than silent corruption) (as shown below) when there is a merged 
> unwritten data extent.
> 
> 
>    ERROR: inode 13 index 0: identified unsupported merged block length 1 
> wanted 4
> 
> 
> This is an intermediary stage while the full support is being added.
> 
> 
> Given this option, the user will have a choice to work on the identified
> inode and make it a non-unwritten extent so that btrfs-convert shall be
> successful.

Nope, this is not acceptable.

If a completely valid ext4 (with enough space) can not be converted to 
btrfs, it's a bug in btrfs-convert and that's why we're here fixing the bug.

Requiring interruption from end user is NOT a solution.

Please update the patchset to handle such case, especially this is not 
impossible to solve.

Just mark the written part as regular data file extents, and mark the 
really unwritten one as preallocated.

If you really find it too hard to do, just let me take over.

Thanks,
Qu

> 
> 
>>>
>>>   # fallocate -l 1G test.img
>>>   # mkfs.ext4 -F test.img
>>>   # mount test.img $mnt
>>>   # xfs_io -f -c "falloc 0 16K" $mnt/file
>>>   # sync
>>>   # xfs_io -f -c "pwrite 0 4k" -c "pwrite 12k 4k" $mnt/file
>>>   # umount $mnt
>>>   # ./btrfs-convert test.img
>>> btrfs-convert from btrfs-progs v6.8
>>>
>>> Source filesystem:
>>>    Type:           ext2
>>>    Label:
>>>    Blocksize:      4096
>>>    UUID:           0f98aa2a-b1ee-4e91-8815-9b9a7b4af00a
>>> Target filesystem:
>>>    Label:
>>>    Blocksize:      4096
>>>    Nodesize:       16384
>>>    UUID:           3b8db399-8e25-495b-a41c-47afcb672020
>>>    Checksum:       crc32c
>>>    Features:       extref, skinny-metadata, no-holes, free-space-tree
>>> (default)
>>>      Data csum:    yes
>>>      Inline data:  yes
>>>      Copy xattr:   yes
>>> Reported stats:
>>>    Total space:      1073741824
>>>    Free space:        872349696 (81.24%)
>>>    Inode count:           65536
>>>    Free inodes:           65523
>>>    Block count:          262144
>>> Create initial btrfs filesystem
>>> Create ext2 image file
>>> Create btrfs metadata
>>> ERROR: inode 13 index 0: identified unsupported merged block length 1
>>> wanted 4
>>> ERROR: failed to copy ext2 inode 13: -22
>>> ERROR: error during copy_inodes -22
>>> WARNING: error during conversion, the original filesystem is not 
>>> modified
>>>
> 
> 
> 
>>> [...]
>>>> +
>>>> +    memset(&extent, 0, sizeof(struct ext2fs_extent));
>>>> +    if (ext2fs_extent_get(handle, EXT2_EXTENT_CURRENT, &extent)) {
>>>> +        error("ext2fs_extent_get EXT2_EXTENT_CURRENT failed inode %d",
>>>> +               src->ext2_ino);
>>>> +        ext2fs_extent_free(handle);
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    if (extent.e_pblk != data->disk_block) {
>>>> +    error("inode %d index %d found wrong extent e_pblk %llu wanted 
>>>> disk_block %llu",
>>>> +               src->ext2_ino, index, extent.e_pblk, data->disk_block);
>>>> +        ext2fs_extent_free(handle);
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    if (extent.e_len != data->num_blocks) {
>>>> +    error("inode %d index %d: identified unsupported merged block 
>>>> length %u wanted %llu",
>>>> +            src->ext2_ino, index, extent.e_len, data->num_blocks);
>>>> +        ext2fs_extent_free(handle);
>>>> +        return -EINVAL;
>>>> +    }
>>>
>>> You have to split the extent in this case. As the example I gave, part
>>> of the extent can have been written.
>>> (And I'm not sure if the e_pblk check is also correct)
>>>
>>> I believe the example I gave could be a pretty good test case.
>>> (Or you can go one step further to interleave every 4K)
>>
>> Furthermore, I have to consider what is the best way to iterate all 
>> data extents of an ext2 inode.
>>
>> Instead of ext2fs_block_iterate2(), I'm wondering if 
>> ext2fs_extent_goto() would be a better solution. (As long as if it can 
>> handle holes).
>>
>> Another thing is, please Cc this series to ext4 mailing list if possible.
>> I hope to get some feedback from the ext4 exports as they may have a 
>> much better idea than us.
>>
> 
> I've tried fixes without success. Empirically, I found
> that the main issue is extent optimization and merging,
> which ignores the unwritten flag, idk where is this
> happening. I think it is during writing the ext4 image
> at the inode BTRFS_FIRST_FREE_OBJECTID + 1.
> 
> If avoiding this optimization possible, the extent boundary
> will align with ext4 and thus its flags.
> 
> Thanks, Anand
> 
> 

Powered by blists - more mailing lists