Re: [PATCH v2 4/4] btrfs-progs: convert: support ext2 unwritten file data extents

[Anand Jain <anand.jain@oracle.com>]

>>> . 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.


>>
>>   # 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