search

kdave / btrfs-progs

Development of userspace BTRFS tools
GNU General Public License v2.0
527 stars 239 forks source link

btrfs-progs: convert: proper ext4 unwritten extents handling #789

Closed adam900710 closed 3 weeks ago

adam900710 commented 1 month ago

There is a bug report that ext4 unwritten extents (extent with EXT2_EXTENT_FLAGS_UNINIT, acts the same as BTRFS_FILE_EXTENT_PREALLOC) would result a regular btrfs file extent.

Instead of filling the range with zero, converted btrfs would read whatever from the disk, causing corruption.

Although there are some attempts trying to fix the problem, the resulted patches are not meeting our standard, so I have to come up a fix by myself.

The root cause is that, ext2fs_block_iterate2() won't report any extent info, thus it's more or less only useful for ext2 filesystems. For ext4 filesystems, inodes can have a feature called EXT4_EXTENTS_FL, allowing a proper extent based iteratioin.

So this patch would keep the old ext2fs_block_iterate2() as fallback (as for older ext2 fses, they do not support fallocate anyway), while for newer ext4 fses, go with ect2fsextent*() APIs to iterate the file extents.

The new APIs provides the extra extent.e_flags to indicate whether the extent is unwritten or not. For unwritten extents, we just puch it as a hole for now, since even if we create a correct preallocated file extent, the space can not be utlized as it's shared between the file extent and ext2 image.

So just punching a hole would be the simplest workaround for now.

kdave commented 3 weeks ago

Merged to devel, thanks.

adam900710 commented 3 weeks ago

Nope, you only merged the last two patches, but dropped the first one:

Revert "btrfs-progs: convert: add raid-stripe-tree to allowed features"

This is already causing problems: https://lore.kernel.org/linux-btrfs/2f8d2b44-8958-4312-bea2-8c53c2312c7c@gmail.com/

kdave commented 2 weeks ago

Sorry I did not mention it, yes the "RST revert to convert" patch was deleted before merge. I don't find the argument strong enough. RST is for zoned and will be the fix for RAID56 and can be also used independently (even if not bringing a huge improvement). The failing tests will need to handle it, we have multiple testing environments so this is a standard thing.

adam900710 commented 2 weeks ago

There are two problems:

  1. Normal kernel won't support RST by default
  2. RST is not even stable (you should already know that) for zoned devices

So why you put such a immature feature so early that the most common use-case (btrfs-convert) won't even need bother the RST feature?

If RST is stable enough, no longer requires zone device and no longer hides behind CONFIG_BTRFS_DEBUG, then sure we can move it to regular features.

But it's definitely not the case, not to mention which is already the cause of a lot of false failures.