* [PATCH 1/3] Revert "btrfs-progs: convert: add raid-stripe-tree to allowed features"
2024-05-07 3:22 [PATCH 0/3] btrfs-progs: convert: proper ext4 unwritten extents handling Qu Wenruo
@ 2024-05-07 3:22 ` Qu Wenruo
2024-05-07 3:22 ` [PATCH 2/3] btrfs-progs: convert: rework file extent iteration to handle unwritten extents Qu Wenruo
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2024-05-07 3:22 UTC (permalink / raw)
To: linux-btrfs
This reverts commit 346a3819237b319985ad6042d6302f67b040a803.
The RST feature (at least for now) is mostly for zoned devices to
support extra data profiles.
Thus btrfs-convert is never designed to handle RST, because there is no
way an ext4/reiserfs can even be created on a zoned device, or it would
create the correct RST tree root at all
Revert this unsupported feature to prevent false alerts.
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
common/fsfeatures.h | 3 +-
tests/common.convert | 44 ++++---------------
tests/convert-tests/001-ext2-basic/test.sh | 2 +-
tests/convert-tests/003-ext4-basic/test.sh | 2 +-
.../005-delete-all-rollback/test.sh | 6 +--
.../convert-tests/010-reiserfs-basic/test.sh | 2 +-
.../011-reiserfs-delete-all-rollback/test.sh | 6 +--
tests/convert-tests/024-ntfs-basic/test.sh | 2 +-
8 files changed, 15 insertions(+), 52 deletions(-)
diff --git a/common/fsfeatures.h b/common/fsfeatures.h
index 5b241bdf9122..f636171f6bd9 100644
--- a/common/fsfeatures.h
+++ b/common/fsfeatures.h
@@ -68,8 +68,7 @@ static const struct btrfs_mkfs_features btrfs_convert_allowed_features = {
BTRFS_FEATURE_INCOMPAT_BIG_METADATA |
BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF |
BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA |
- BTRFS_FEATURE_INCOMPAT_NO_HOLES |
- BTRFS_FEATURE_INCOMPAT_RAID_STRIPE_TREE,
+ BTRFS_FEATURE_INCOMPAT_NO_HOLES,
.runtime_flags = BTRFS_FEATURE_RUNTIME_QUOTA,
};
diff --git a/tests/common.convert b/tests/common.convert
index 06eec9b2a853..50d49a0d32da 100644
--- a/tests/common.convert
+++ b/tests/common.convert
@@ -214,47 +214,19 @@ convert_test_post_check_checksums() {
grep -q 'FAILED' && _fail "file validation failed"
}
-# Check if convert can mount the image based on features
-# $1: name of the feature
-convert_can_mount() {
- local features="$1"
-
- if [ "$features" = 'block-group-tree' ]; then
- if ! [ -f "/sys/fs/btrfs/features/block_group_tree" ]; then
- _log "Skip mount checks due to missing support of block-group-tree"
- return 1
- fi
- fi
- if [ "$features" = 'raid-stripe-tree' ]; then
- if ! [ -f "/sys/fs/btrfs/features/raid_stripe_tree" ]; then
- _log "Skip mount checks due to missing support of raid-stripe-tree"
- return 1
- fi
- fi
-
- return 0
-}
-
# post conversion checks, all three in one call, on an unmounted image
-# $1: features for mount compatibility checks
-# $2: file with checksums
-# $3: file with permissions.
-# $4: file with acl entries.
+# $1: file with checksums
+# $2: file with permissions.
+# $3: file with acl entries.
convert_test_post_checks_all() {
- local features="$1"
-
+ _assert_path "$1"
_assert_path "$2"
_assert_path "$3"
- _assert_path "$4"
-
- if ! convert_can_mount "$features"; then
- return 0
- fi
run_check_mount_test_dev
- convert_test_post_check_checksums "$2"
- convert_test_post_check_permissions "$3"
- convert_test_post_check_acl "$4"
+ convert_test_post_check_checksums "$1"
+ convert_test_post_check_permissions "$2"
+ convert_test_post_check_acl "$3"
# Create a large file to trigger data chunk allocation
generate_dataset "large"
@@ -313,7 +285,7 @@ convert_test() {
run_check_umount_test_dev
convert_test_do_convert "$features" "$nodesize" "$fstype"
- convert_test_post_checks_all "$features" "$CHECKSUMTMP" "$EXT_PERMTMP" "$EXT_ACLTMP"
+ convert_test_post_checks_all "$CHECKSUMTMP" "$EXT_PERMTMP" "$EXT_ACLTMP"
rm -- "$CHECKSUMTMP"
rm -- "$EXT_PERMTMP"
rm -- "$EXT_ACLTMP"
diff --git a/tests/convert-tests/001-ext2-basic/test.sh b/tests/convert-tests/001-ext2-basic/test.sh
index 461ff4a5f1a9..6dc105b55e27 100755
--- a/tests/convert-tests/001-ext2-basic/test.sh
+++ b/tests/convert-tests/001-ext2-basic/test.sh
@@ -12,7 +12,7 @@ check_kernel_support_acl
# Iterate over defaults and options that are not tied to hardware capabilities
# or number of devices
-for feature in '' 'block-group-tree' 'raid-stripe-tree'; do
+for feature in '' 'block-group-tree' ; do
convert_test ext2 "$feature" "ext2 4k nodesize" 4096 mke2fs -b 4096
convert_test ext2 "$feature" "ext2 16k nodesize" 16384 mke2fs -b 4096
convert_test ext2 "$feature" "ext2 64k nodesize" 65536 mke2fs -b 4096
diff --git a/tests/convert-tests/003-ext4-basic/test.sh b/tests/convert-tests/003-ext4-basic/test.sh
index 14637fc852db..8ae5264cb340 100755
--- a/tests/convert-tests/003-ext4-basic/test.sh
+++ b/tests/convert-tests/003-ext4-basic/test.sh
@@ -12,7 +12,7 @@ check_kernel_support_acl
# Iterate over defaults and options that are not tied to hardware capabilities
# or number of devices
-for feature in '' 'block-group-tree' 'raid-stripe-tree' ; do
+for feature in '' 'block-group-tree' ; do
convert_test ext4 "$feature" "ext4 4k nodesize" 4096 mke2fs -t ext4 -b 4096
convert_test ext4 "$feature" "ext4 16k nodesize" 16384 mke2fs -t ext4 -b 4096
convert_test ext4 "$feature" "ext4 64k nodesize" 65536 mke2fs -t ext4 -b 4096
diff --git a/tests/convert-tests/005-delete-all-rollback/test.sh b/tests/convert-tests/005-delete-all-rollback/test.sh
index 5603d3078bc8..fa56ca292bfc 100755
--- a/tests/convert-tests/005-delete-all-rollback/test.sh
+++ b/tests/convert-tests/005-delete-all-rollback/test.sh
@@ -38,10 +38,6 @@ do_test() {
convert_test_do_convert "$features" "$nodesize"
- if ! convert_can_mount "$features"; then
- return 0
- fi
-
run_check_mount_test_dev
convert_test_post_check_checksums "$CHECKSUMTMP"
@@ -68,7 +64,7 @@ do_test() {
# Iterate over defaults and options that are not tied to hardware capabilities
# or number of devices
-for feature in '' 'block-group-tree' 'raid-stripe-tree' ; do
+for feature in '' 'block-group-tree' ; do
do_test "$feature" "ext4 4k nodesize" 4096 mke2fs -t ext4 -b 4096
do_test "$feature" "ext4 16k nodesize" 16384 mke2fs -t ext4 -b 4096
do_test "$feature" "ext4 64k nodesize" 65536 mke2fs -t ext4 -b 4096
diff --git a/tests/convert-tests/010-reiserfs-basic/test.sh b/tests/convert-tests/010-reiserfs-basic/test.sh
index 6ab02b31d176..86b1826783b3 100755
--- a/tests/convert-tests/010-reiserfs-basic/test.sh
+++ b/tests/convert-tests/010-reiserfs-basic/test.sh
@@ -15,7 +15,7 @@ prepare_test_dev
# Iterate over defaults and options that are not tied to hardware capabilities
# or number of devices
-for feature in '' 'block-group-tree' 'raid-stripe-tree'; do
+for feature in '' 'block-group-tree' ; do
convert_test reiserfs "$feature" "reiserfs 4k nodesize" 4096 mkreiserfs -b 4096
convert_test reiserfs "$feature" "reiserfs 16k nodesize" 16384 mkreiserfs -b 4096
convert_test reiserfs "$feature" "reiserfs 64k nodesize" 65536 mkreiserfs -b 4096
diff --git a/tests/convert-tests/011-reiserfs-delete-all-rollback/test.sh b/tests/convert-tests/011-reiserfs-delete-all-rollback/test.sh
index 688613c2c6ad..e1c3e02eb7a0 100755
--- a/tests/convert-tests/011-reiserfs-delete-all-rollback/test.sh
+++ b/tests/convert-tests/011-reiserfs-delete-all-rollback/test.sh
@@ -40,10 +40,6 @@ do_test() {
convert_test_do_convert "$features" "$nodesize"
- if ! convert_can_mount "$features"; then
- return 0
- fi
-
run_check_mount_test_dev
convert_test_post_check_checksums "$CHECKSUMTMP"
@@ -70,7 +66,7 @@ do_test() {
# Iterate over defaults and options that are not tied to hardware capabilities
# or number of devices
-for feature in '' 'block-group-tree' 'raid-stripe-tree'; do
+for feature in '' 'block-group-tree' ; do
do_test "$feature" "reiserfs 4k nodesize" 4096 mkreiserfs -b 4096
do_test "$feature" "reiserfs 16k nodesize" 16384 mkreiserfs -b 4096
do_test "$feature" "reiserfs 64k nodesize" 65536 mkreiserfs -b 4096
diff --git a/tests/convert-tests/024-ntfs-basic/test.sh b/tests/convert-tests/024-ntfs-basic/test.sh
index a93f60070674..d2e1be79ef6b 100755
--- a/tests/convert-tests/024-ntfs-basic/test.sh
+++ b/tests/convert-tests/024-ntfs-basic/test.sh
@@ -17,6 +17,6 @@ prepare_test_dev
# Iterate over defaults and options that are not tied to hardware capabilities
# or number of devices. Test only 4K block size as minimum.
-for feature in '' 'block-group-tree' 'raid-stripe-tree'; do
+for feature in '' 'block-group-tree' ; do
convert_test ntfs "$feature" "ntfs 4k nodesize" 4096 mkfs.ntfs -s 4096
done
--
2.45.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/3] btrfs-progs: convert: rework file extent iteration to handle unwritten extents
2024-05-07 3:22 [PATCH 0/3] btrfs-progs: convert: proper ext4 unwritten extents handling Qu Wenruo
2024-05-07 3:22 ` [PATCH 1/3] Revert "btrfs-progs: convert: add raid-stripe-tree to allowed features" Qu Wenruo
@ 2024-05-07 3:22 ` Qu Wenruo
2024-05-07 7:48 ` Yordan
2024-05-07 3:22 ` [PATCH 3/3] btrfs-progs: tests/convert: add test case for ext4 " Qu Wenruo
2024-06-03 20:14 ` [PATCH 0/3] btrfs-progs: convert: proper ext4 unwritten extents handling David Sterba
3 siblings, 1 reply; 6+ messages in thread
From: Qu Wenruo @ 2024-05-07 3:22 UTC (permalink / raw)
To: linux-btrfs; +Cc: Yordan
[BUG]
There is a bug report that btrfs-convert can not handle unwritten
extents (EXT2_EXTENT_FLAGS_UNINIT set, which is pretty much the same as
BTRFS_FILE_EXTENT_PREALLOC), which can cause the converted image to have
incorrect contents.
[CAUSE]
Currently we use ext2fs_block_iterate2() to go through all data extents
of an ext2 inode, but it doesn't provide the info on if the range is
unwritten or not.
Thus for unwritten extents, the results btrfs would just treat it as
regular extents, and read the contents from disk other than setting the
contents to zero.
[FIX]
Instead of the ext2fs_block_iterate2(), here we follow the debugfs'
"dump_extents" command, to use ext2fs_extent_*() helpers to go through
every data extent of the inode, that's if the inode support the
EXT4_EXTENTS_FL flag.
Now we can properly get the info of which extents are unwritten, and use
holes to replace those unwritten extents.
Reported-by: Yordan <y16267966@gmail.com>
Link: https://lore.kernel.org/all/d34c7d77a7f00c93bea6a4d6e83c7caf.mailbg@mail.bg/
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
convert/source-ext2.c | 116 +++++++++++++++++++++++++++++++++++++++---
1 file changed, 109 insertions(+), 7 deletions(-)
diff --git a/convert/source-ext2.c b/convert/source-ext2.c
index bba81e4034fd..029fa198fc24 100644
--- a/convert/source-ext2.c
+++ b/convert/source-ext2.c
@@ -303,6 +303,88 @@ static int ext2_block_iterate_proc(ext2_filsys fs, blk_t *blocknr,
return 0;
}
+static int iterate_one_file_extent(struct blk_iterate_data *data,
+ u64 filepos, u64 len, u64 disk_bytenr,
+ bool prealloced)
+{
+ const int sectorsize = data->trans->fs_info->sectorsize;
+ const int sectorbits = ilog2(sectorsize);
+ int ret;
+
+ UASSERT(len > 0);
+ for (int i = 0; i < len; i += sectorsize) {
+ /*
+ * Just treat preallocated extent as hole.
+ *
+ * As there is no way to utilize the preallocated space, since
+ * any file extent would also be shared by ext2 image.
+ */
+ if (prealloced)
+ ret = block_iterate_proc(0,
+ (filepos + i) >> sectorbits, data);
+ else
+ ret = block_iterate_proc(
+ (disk_bytenr + i) >> sectorbits,
+ (filepos + i) >> sectorbits, data);
+
+ if (ret < 0)
+ return ret;
+ }
+ return 0;
+}
+
+static int iterate_file_extents(struct blk_iterate_data *data,
+ ext2_filsys ext2fs, ext2_ino_t ext2_ino,
+ u32 convert_flags)
+{
+ ext2_extent_handle_t handle = NULL;
+ struct ext2fs_extent extent;
+ const int sectorsize = data->trans->fs_info->sectorsize;
+ const int sectorbits = ilog2(sectorsize);
+ int op = EXT2_EXTENT_ROOT;
+ errcode_t errcode;
+ int ret = 0;
+
+ errcode = ext2fs_extent_open(ext2fs, ext2_ino, &handle);
+ if (errcode) {
+ error("failed to open ext2 inode %u: %s", ext2_ino,
+ error_message(errcode));
+ return -EIO;
+ }
+ while (1) {
+ u64 disk_bytenr;
+ u64 filepos;
+ u64 len;
+
+ errcode = ext2fs_extent_get(handle, op, &extent);
+ if (errcode == EXT2_ET_EXTENT_NO_NEXT)
+ break;
+ if (errcode) {
+ data->errcode = errcode;
+ ret = -EIO;
+ goto out;
+ }
+ op = EXT2_EXTENT_NEXT;
+
+ if (extent.e_flags & EXT2_EXTENT_FLAGS_SECOND_VISIT)
+ continue;
+ if (!(extent.e_flags & EXT2_EXTENT_FLAGS_LEAF))
+ continue;
+
+ filepos = extent.e_lblk << sectorbits;
+ len = extent.e_len << sectorbits;
+ disk_bytenr = extent.e_pblk << sectorbits;
+
+ ret = iterate_one_file_extent(data, filepos, len, disk_bytenr,
+ extent.e_flags & EXT2_EXTENT_FLAGS_UNINIT);
+ if (ret < 0)
+ goto out;
+ }
+out:
+ ext2fs_extent_free(handle);
+ return ret;
+}
+
/*
* traverse file's data blocks, record these data blocks as file extents.
*/
@@ -315,6 +397,7 @@ static int ext2_create_file_extents(struct btrfs_trans_handle *trans,
int ret;
char *buffer = NULL;
errcode_t err;
+ struct ext2_inode ext2_inode = { 0 };
u32 last_block;
u32 sectorsize = root->fs_info->sectorsize;
u64 inode_size = btrfs_stack_inode_size(btrfs_inode);
@@ -323,10 +406,32 @@ static int ext2_create_file_extents(struct btrfs_trans_handle *trans,
init_blk_iterate_data(&data, trans, root, btrfs_inode, objectid,
convert_flags & CONVERT_FLAG_DATACSUM);
- err = ext2fs_block_iterate2(ext2_fs, ext2_ino, BLOCK_FLAG_DATA_ONLY,
- NULL, ext2_block_iterate_proc, &data);
- if (err)
- goto error;
+ err = ext2fs_read_inode(ext2_fs, ext2_ino, &ext2_inode);
+ if (err) {
+ error("failed to read ext2 inode %u: %s",
+ ext2_ino, error_message(err));
+ return -EIO;
+ }
+ /*
+ * For inodes without extent block maps, go with the older
+ * ext2fs_block_iterate2().
+ * Otherwise use ext2fs_extent_*() based solution, as that can provide
+ * UNINIT extent flags.
+ */
+ if ((ext2_inode.i_flags & EXT4_EXTENTS_FL) == 0) {
+ err = ext2fs_block_iterate2(ext2_fs, ext2_ino,
+ BLOCK_FLAG_DATA_ONLY, NULL,
+ ext2_block_iterate_proc, &data);
+ if (err) {
+ error("ext2fs_block_iterate2: %s", error_message(err));
+ return -EIO;
+ }
+ } else {
+ ret = iterate_file_extents(&data, ext2_fs, ext2_ino,
+ convert_flags);
+ if (ret < 0)
+ goto fail;
+ }
ret = data.errcode;
if (ret)
goto fail;
@@ -366,9 +471,6 @@ static int ext2_create_file_extents(struct btrfs_trans_handle *trans,
fail:
free(buffer);
return ret;
-error:
- error("ext2fs_block_iterate2: %s", error_message(err));
- return -1;
}
static int ext2_create_symlink(struct btrfs_trans_handle *trans,
--
2.45.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 0/3] btrfs-progs: convert: proper ext4 unwritten extents handling
2024-05-07 3:22 [PATCH 0/3] btrfs-progs: convert: proper ext4 unwritten extents handling Qu Wenruo
` (2 preceding siblings ...)
2024-05-07 3:22 ` [PATCH 3/3] btrfs-progs: tests/convert: add test case for ext4 " Qu Wenruo
@ 2024-06-03 20:14 ` David Sterba
3 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2024-06-03 20:14 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs
On Tue, May 07, 2024 at 12:52:52PM +0930, Qu Wenruo wrote:
> [REPO]
> https://github.com/adam900710/btrfs-progs/tree/unwritten_extent
>
> 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 ect2fs_extent_*() 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.
>
> Qu Wenruo (3):
> Revert "btrfs-progs: convert: add raid-stripe-tree to allowed
> features"
> btrfs-progs: convert: rework file extent iteration to handle unwritten
> extents
> btrfs-progs: tests/convert: add test case for ext4 unwritten extents
Added to devel, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread