[PATCH v2 0/4] btrfs-progs: add support ext4 unwritten file extent

[PATCH v2 0/4] btrfs-progs: add support ext4 unwritten file extent

[Anand Jain]

v2:
 Fix per review comments.
 Identify and fail safe prealloc merged with regular blocks.

These patches add support for the ext4 file data unwritten/preallocated
extents. Patches 1-3 are preparatory patches, and patch 4 adds the
missing feature.

Patch 4 is marked as RFC because this patch is tested with limited
variants of the file extents with unwritten flag.


Anand Jain (4):
  btrfs-progs: convert: refactor ext2_create_file_extents add argument
    ext2_inode
  btrfs-progs: convert: struct blk_iterate_data, add ext2-specific file
    inode pointers
  btrfs-progs: convert: refactor __btrfs_record_file_extent to add a
    prealloc flag
  btrfs-progs: convert: support ext2 unwritten file data extents

 common/extent-tree-utils.c |  8 +++---
 common/extent-tree-utils.h |  2 +-
 convert/main.c             | 10 ++++---
 convert/source-ext2.c      | 59 +++++++++++++++++++++++++++++++++++++-
 convert/source-ext2.h      |  9 ++++++
 convert/source-fs.c        | 26 +++++++++++++++--
 convert/source-fs.h        |  3 ++
 convert/source-reiserfs.c  |  2 +-
 mkfs/rootdir.c             |  3 +-
 9 files changed, 108 insertions(+), 14 deletions(-)

-- 
2.39.3

[PATCH v2 1/4] btrfs-progs: convert: refactor ext2_create_file_extents add argument ext2_inode

[Anand Jain]

This is a preparatory patch adds an argument '%ext2_inode' for the
function __btrfs_record_file_extent(); to be used in the following patches.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: -
 convert/source-ext2.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/convert/source-ext2.c b/convert/source-ext2.c
index 2186b2526e38..a3f61bb01171 100644
--- a/convert/source-ext2.c
+++ b/convert/source-ext2.c
@@ -310,6 +310,7 @@ static int ext2_create_file_extents(struct btrfs_trans_handle *trans,
 			       struct btrfs_root *root, u64 objectid,
 			       struct btrfs_inode_item *btrfs_inode,
 			       ext2_filsys ext2_fs, ext2_ino_t ext2_ino,
+			       struct ext2_inode *ext2_inode,
 			       u32 convert_flags)
 {
 	int ret;
@@ -384,6 +385,7 @@ static int ext2_create_symlink(struct btrfs_trans_handle *trans,
 		btrfs_set_stack_inode_size(btrfs_inode, inode_size + 1);
 		ret = ext2_create_file_extents(trans, root, objectid,
 				btrfs_inode, ext2_fs, ext2_ino,
+				ext2_inode,
 				CONVERT_FLAG_DATACSUM |
 				CONVERT_FLAG_INLINE_DATA);
 		btrfs_set_stack_inode_size(btrfs_inode, inode_size);
@@ -903,7 +905,7 @@ static int ext2_copy_single_inode(struct btrfs_trans_handle *trans,
 	switch (ext2_inode->i_mode & S_IFMT) {
 	case S_IFREG:
 		ret = ext2_create_file_extents(trans, root, objectid,
-			&btrfs_inode, ext2_fs, ext2_ino, convert_flags);
+			&btrfs_inode, ext2_fs, ext2_ino, ext2_inode, convert_flags);
 		break;
 	case S_IFDIR:
 		ret = ext2_create_dir_entries(trans, root, objectid,
-- 
2.39.3

[PATCH v2 2/4] btrfs-progs: convert: struct blk_iterate_data, add ext2-specific file inode pointers

[Anand Jain]

To obtain the file data extent flags, we require the use of ext2 helper
functions, pass these pointer in the 'struct blk_iterate_data'. However,
this struct is a common function across both 'reiserfs' and 'ext4'
filesystems. Since there is no further development on 'convert-reiserfs',
this patch avoids creating a mess which won't be used.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: split struct blk_iterate_data  containing the source private.

 convert/source-ext2.c | 7 +++++++
 convert/source-ext2.h | 6 ++++++
 convert/source-fs.h   | 2 ++
 3 files changed, 15 insertions(+)

diff --git a/convert/source-ext2.c b/convert/source-ext2.c
index a3f61bb01171..477c39d9d658 100644
--- a/convert/source-ext2.c
+++ b/convert/source-ext2.c
@@ -320,10 +320,17 @@ static int ext2_create_file_extents(struct btrfs_trans_handle *trans,
 	u32 sectorsize = root->fs_info->sectorsize;
 	u64 inode_size = btrfs_stack_inode_size(btrfs_inode);
 	struct blk_iterate_data data;
+	struct ext2_source_fs  source_fs;
 
 	init_blk_iterate_data(&data, trans, root, btrfs_inode, objectid,
 			convert_flags & CONVERT_FLAG_DATACSUM);
 
+	source_fs.ext2_fs = ext2_fs;
+	source_fs.ext2_ino = ext2_ino;
+	source_fs.ext2_inode = ext2_inode;
+
+	data.source_fs = &source_fs;
+
 	err = ext2fs_block_iterate2(ext2_fs, ext2_ino, BLOCK_FLAG_DATA_ONLY,
 				    NULL, ext2_block_iterate_proc, &data);
 	if (err)
diff --git a/convert/source-ext2.h b/convert/source-ext2.h
index d204aac504e7..026a7cad8ac8 100644
--- a/convert/source-ext2.h
+++ b/convert/source-ext2.h
@@ -76,6 +76,12 @@ struct dir_iterate_data {
 	int errcode;
 };
 
+struct ext2_source_fs {
+	ext2_filsys ext2_fs;
+	struct ext2_inode *ext2_inode;
+	ext2_ino_t ext2_ino;
+};
+
 #define EXT2_ACL_VERSION	0x0001
 
 #endif	/* BTRFSCONVERT_EXT2 */
diff --git a/convert/source-fs.h b/convert/source-fs.h
index b26e1842941d..25916c65681b 100644
--- a/convert/source-fs.h
+++ b/convert/source-fs.h
@@ -118,6 +118,8 @@ struct btrfs_convert_operations {
 };
 
 struct blk_iterate_data {
+	void *source_fs;
+
 	struct btrfs_trans_handle *trans;
 	struct btrfs_root *root;
 	struct btrfs_root *convert_root;
-- 
2.39.3

[PATCH v2 3/4] btrfs-progs: convert: refactor __btrfs_record_file_extent to add a prealloc flag

[Anand Jain]

This preparatory patch adds an argument '%prealloc' to the function
__btrfs_record_file_extent(), to be used in the following patches.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: pass btrfs_record_file_extent() actual flags instead of has_prealloc boolean.

 common/extent-tree-utils.c |  8 ++++----
 common/extent-tree-utils.h |  2 +-
 convert/main.c             | 10 ++++++----
 convert/source-fs.c        |  5 +++--
 convert/source-reiserfs.c  |  2 +-
 mkfs/rootdir.c             |  3 ++-
 6 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/common/extent-tree-utils.c b/common/extent-tree-utils.c
index 34c7e5095160..53c10734408d 100644
--- a/common/extent-tree-utils.c
+++ b/common/extent-tree-utils.c
@@ -122,7 +122,7 @@ static int __btrfs_record_file_extent(struct btrfs_trans_handle *trans,
 				      struct btrfs_root *root, u64 objectid,
 				      struct btrfs_inode_item *inode,
 				      u64 file_pos, u64 disk_bytenr,
-				      u64 *ret_num_bytes)
+				      u64 *ret_num_bytes, u64 flags)
 {
 	int ret;
 	struct btrfs_fs_info *info = root->fs_info;
@@ -229,7 +229,7 @@ static int __btrfs_record_file_extent(struct btrfs_trans_handle *trans,
 	leaf = path->nodes[0];
 	fi = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_file_extent_item);
 	btrfs_set_file_extent_generation(leaf, fi, trans->transid);
-	btrfs_set_file_extent_type(leaf, fi, BTRFS_FILE_EXTENT_REG);
+	btrfs_set_file_extent_type(leaf, fi, flags);
 	btrfs_set_file_extent_disk_bytenr(leaf, fi, extent_bytenr);
 	btrfs_set_file_extent_disk_num_bytes(leaf, fi, extent_num_bytes);
 	btrfs_set_file_extent_offset(leaf, fi, extent_offset);
@@ -265,7 +265,7 @@ int btrfs_record_file_extent(struct btrfs_trans_handle *trans,
 			     struct btrfs_root *root, u64 objectid,
 			     struct btrfs_inode_item *inode,
 			     u64 file_pos, u64 disk_bytenr,
-			     u64 num_bytes)
+			     u64 num_bytes, u64 flags)
 {
 	u64 cur_disk_bytenr = disk_bytenr;
 	u64 cur_file_pos = file_pos;
@@ -276,7 +276,7 @@ int btrfs_record_file_extent(struct btrfs_trans_handle *trans,
 		ret = __btrfs_record_file_extent(trans, root, objectid,
 						 inode, cur_file_pos,
 						 cur_disk_bytenr,
-						 &cur_num_bytes);
+						 &cur_num_bytes, flags);
 		if (ret < 0)
 			break;
 		cur_disk_bytenr += cur_num_bytes;
diff --git a/common/extent-tree-utils.h b/common/extent-tree-utils.h
index f03d9c438375..dce48c43faf5 100644
--- a/common/extent-tree-utils.h
+++ b/common/extent-tree-utils.h
@@ -31,6 +31,6 @@ int btrfs_record_file_extent(struct btrfs_trans_handle *trans,
 			     struct btrfs_root *root, u64 objectid,
 			     struct btrfs_inode_item *inode,
 			     u64 file_pos, u64 disk_bytenr,
-			     u64 num_bytes);
+			     u64 num_bytes, u64 flags);
 
 #endif
diff --git a/convert/main.c b/convert/main.c
index f18fab4a236c..fb0f97d949d4 100644
--- a/convert/main.c
+++ b/convert/main.c
@@ -337,7 +337,7 @@ static int create_image_file_range(struct btrfs_trans_handle *trans,
 		return -EINVAL;
 	}
 	ret = btrfs_record_file_extent(trans, root, ino, inode, bytenr,
-				       disk_bytenr, len);
+				       disk_bytenr, len, BTRFS_FILE_EXTENT_REG);
 	if (ret < 0)
 		return ret;
 
@@ -426,7 +426,8 @@ static int migrate_one_reserved_range(struct btrfs_trans_handle *trans,
 
 		/* Now handle extent item and file extent things */
 		ret = btrfs_record_file_extent(trans, root, ino, inode, cur_off,
-					       key.objectid, key.offset);
+					       key.objectid, key.offset,
+					       BTRFS_FILE_EXTENT_REG);
 		if (ret < 0)
 			break;
 		/* Finally, insert csum items */
@@ -438,7 +439,7 @@ static int migrate_one_reserved_range(struct btrfs_trans_handle *trans,
 		hole_len = cur_off - hole_start;
 		if (hole_len) {
 			ret = btrfs_record_file_extent(trans, root, ino, inode,
-					hole_start, 0, hole_len);
+					hole_start, 0, hole_len, BTRFS_FILE_EXTENT_REG);
 			if (ret < 0)
 				break;
 		}
@@ -455,7 +456,8 @@ static int migrate_one_reserved_range(struct btrfs_trans_handle *trans,
 	 */
 	if (range_end(range) - hole_start > 0)
 		ret = btrfs_record_file_extent(trans, root, ino, inode,
-				hole_start, 0, range_end(range) - hole_start);
+				hole_start, 0, range_end(range) - hole_start,
+				BTRFS_FILE_EXTENT_REG);
 	return ret;
 }
 
diff --git a/convert/source-fs.c b/convert/source-fs.c
index 66561438866e..df5ce66caf7f 100644
--- a/convert/source-fs.c
+++ b/convert/source-fs.c
@@ -262,7 +262,7 @@ int record_file_blocks(struct blk_iterate_data *data,
 	if (old_disk_bytenr == 0)
 		return btrfs_record_file_extent(data->trans, root,
 				data->objectid, data->inode, file_pos, 0,
-				num_bytes);
+				num_bytes, BTRFS_FILE_EXTENT_REG);
 
 	/*
 	 * Search real disk bytenr from convert root
@@ -316,7 +316,8 @@ int record_file_blocks(struct blk_iterate_data *data,
 			      old_disk_bytenr + num_bytes) - cur_off;
 		ret = btrfs_record_file_extent(data->trans, data->root,
 					data->objectid, data->inode, file_pos,
-					real_disk_bytenr, cur_len);
+					real_disk_bytenr, cur_len,
+					BTRFS_FILE_EXTENT_REG);
 		if (ret < 0)
 			break;
 		cur_off += cur_len;
diff --git a/convert/source-reiserfs.c b/convert/source-reiserfs.c
index 3edc72ed08a5..746892ff0a8d 100644
--- a/convert/source-reiserfs.c
+++ b/convert/source-reiserfs.c
@@ -365,7 +365,7 @@ static int convert_direct(struct btrfs_trans_handle *trans,
 		return ret;
 
 	return btrfs_record_file_extent(trans, root, objectid, inode, offset,
-					key.objectid, sectorsize);
+					key.objectid, sectorsize, BTRFS_FILE_EXTENT_REG);
 }
 
 static int reiserfs_convert_tail(struct btrfs_trans_handle *trans,
diff --git a/mkfs/rootdir.c b/mkfs/rootdir.c
index 4ae9f435a7b7..cb6659319b7d 100644
--- a/mkfs/rootdir.c
+++ b/mkfs/rootdir.c
@@ -411,7 +411,8 @@ again:
 
 	if (bytes_read) {
 		ret = btrfs_record_file_extent(trans, root, objectid,
-				btrfs_inode, file_pos, first_block, cur_bytes);
+				btrfs_inode, file_pos, first_block, cur_bytes,
+				false);
 		if (ret)
 			goto end;
 
-- 
2.39.3

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

[Anand Jain]

This patch, along with the dependent patches below, adds support for
ext4 unmerged  unwritten file extents as preallocated file extent in btrfs.

 btrfs-progs: convert: refactor ext2_create_file_extents add argument ext2_inode
 btrfs-progs: convert: struct blk_iterate_data, add ext2-specific file inode pointers
 btrfs-progs: convert: refactor __btrfs_record_file_extent to add a prealloc flag

The patch is developed with POV of portability with the current
e2fsprogs library.

This patch will handle independent unwritten extents by marking them with prealloc
flag and will identify merged unwritten extents, triggering a fail.

Testcase:

     $ dd if=/dev/urandom of=/mnt/test/foo bs=4K count=1 conv=fsync status=none
     $ dd if=/dev/urandom of=/mnt/test/foo bs=4K count=2 conv=fsync seek=1 status=none
     $ xfs_io -f -c 'falloc -k 12K 12K' /mnt/test/foo
     $ dd if=/dev/zero of=/mnt/test/foo bs=4K count=1 conv=fsync seek=6 status=none

     $ filefrag -v /mnt/test/foo
     Filesystem type is: ef53
     File size of /mnt/test/foo is 28672 (7 blocks of 4096 bytes)
	 ext:     logical_offset:        physical_offset: length:   expected: flags:
	   0:        0..       0:      33280..     33280:      1:
	   1:        1..       2:      33792..     33793:      2:      33281:
	   2:        3..       5:      33281..     33283:      3:      33794: unwritten
	   3:        6..       6:      33794..     33794:      1:      33284: last,eof

     $ sha256sum /mnt/test/foo
     18619b678a5c207a971a0aa931604f48162e307c57ecdec450d5f095fe9f32c7  /mnt/test/foo

   Convert and compare the checksum

   Before:

     $ filefrag -v /mnt/test/foo
     Filesystem type is: 9123683e
     File size of /mnt/test/foo is 28672 (7 blocks of 4096 bytes)
      ext:     logical_offset:        physical_offset: length:   expected: flags:
      0:        0..       0:      33280..     33280:      1:             shared
      1:        1..       2:      33792..     33793:      2:      33281: shared
      2:        3..       6:      33281..     33284:      4:      33794: last,shared,eof
     /mnt/test/foo: 3 extents found

     $ sha256sum /mnt/test/foo
     6874a1733e5785682210d69c07f256f684cf5433c7235ed29848b4a4d52030e0  /mnt/test/foo

   After:

     $ filefrag -v /mnt/test/foo
     Filesystem type is: 9123683e
     File size of /mnt/test/foo is 28672 (7 blocks of 4096 bytes)
	 ext:     logical_offset:        physical_offset: length:   expected: flags:
	   0:        0..       0:      33280..     33280:      1:             shared
	   1:        1..       2:      33792..     33793:      2:      33281: shared
	   2:        3..       5:      33281..     33283:      3:      33794: unwritten,shared
	   3:        6..       6:      33794..     33794:      1:      33284: last,shared,eof
     /mnt/test/foo: 4 extents found

     $ sha256sum /mnt/test/foo
     18619b678a5c207a971a0aa931604f48162e307c57ecdec450d5f095fe9f32c7  /mnt/test/foo

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2:

. 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.
. Updated the change log.

 convert/source-ext2.c | 48 +++++++++++++++++++++++++++++++++++++++++++
 convert/source-ext2.h |  3 +++
 convert/source-fs.c   | 25 ++++++++++++++++++++--
 convert/source-fs.h   |  1 +
 4 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/convert/source-ext2.c b/convert/source-ext2.c
index 477c39d9d658..9de591e34c18 100644
--- a/convert/source-ext2.c
+++ b/convert/source-ext2.c
@@ -208,6 +208,54 @@ static u8 ext2_filetype_conversion_table[EXT2_FT_MAX] = {
 	[EXT2_FT_SYMLINK]	= BTRFS_FT_SYMLINK,
 };
 
+int ext2_find_unwritten(struct blk_iterate_data *data, int index,
+			bool *has_unwritten)
+{
+	ext2_extent_handle_t handle;
+	struct ext2fs_extent extent;
+	struct ext2_source_fs  *src = (struct ext2_source_fs *) data->source_fs;
+
+	if (ext2fs_extent_open2(src->ext2_fs, src->ext2_ino,
+				src->ext2_inode, &handle)) {
+		error("ext2fs_extent_open2 failed, inode %d", src->ext2_ino);
+		return -EINVAL;
+	}
+
+	if (ext2fs_extent_goto2(handle, 0, index)) {
+		error("ext2fs_extent_goto2 failed, inode %d num_blocks %llu",
+		       src->ext2_ino, data->num_blocks);
+		ext2fs_extent_free(handle);
+		return -EINVAL;
+	}
+
+	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;
+	}
+
+	if (extent.e_flags & EXT2_EXTENT_FLAGS_UNINIT)
+		*has_unwritten = true;
+
+	return 0;
+}
+
 static int ext2_dir_iterate_proc(ext2_ino_t dir, int entry,
 			    struct ext2_dir_entry *dirent,
 			    int offset, int blocksize,
diff --git a/convert/source-ext2.h b/convert/source-ext2.h
index 026a7cad8ac8..19014d3c25e6 100644
--- a/convert/source-ext2.h
+++ b/convert/source-ext2.h
@@ -82,6 +82,9 @@ struct ext2_source_fs {
 	ext2_ino_t ext2_ino;
 };
 
+int ext2_find_unwritten(struct blk_iterate_data *data, int index,
+			bool *has_unwritten);
+
 #define EXT2_ACL_VERSION	0x0001
 
 #endif	/* BTRFSCONVERT_EXT2 */
diff --git a/convert/source-fs.c b/convert/source-fs.c
index df5ce66caf7f..88a6ceaf41f6 100644
--- a/convert/source-fs.c
+++ b/convert/source-fs.c
@@ -31,6 +31,7 @@
 #include "common/extent-tree-utils.h"
 #include "convert/common.h"
 #include "convert/source-fs.h"
+#include "convert/source-ext2.h"
 
 const struct simple_range btrfs_reserved_ranges[3] = {
 	{ 0,			     SZ_1M },
@@ -239,6 +240,15 @@ fail:
 	return ret;
 }
 
+int find_prealloc(struct blk_iterate_data *data, int index,
+		  bool *has_prealloc)
+{
+	if (data->source_fs)
+		return ext2_find_unwritten(data, index, has_prealloc);
+
+	return -EINVAL;
+}
+
 /*
  * Record a file extent in original filesystem into btrfs one.
  * The special point is, old disk_block can point to a reserved range.
@@ -257,6 +267,7 @@ int record_file_blocks(struct blk_iterate_data *data,
 	u64 old_disk_bytenr = disk_block * sectorsize;
 	u64 num_bytes = num_blocks * sectorsize;
 	u64 cur_off = old_disk_bytenr;
+	int index = data->first_block;
 
 	/* Hole, pass it to record_file_extent directly */
 	if (old_disk_bytenr == 0)
@@ -276,6 +287,16 @@ int record_file_blocks(struct blk_iterate_data *data,
 		u64 extent_num_bytes;
 		u64 real_disk_bytenr;
 		u64 cur_len;
+		u64 flags = BTRFS_FILE_EXTENT_REG;
+		bool has_prealloc = false;
+
+		if (find_prealloc(data, index, &has_prealloc)) {
+			data->errcode = -1;
+			return -EINVAL;
+		}
+
+		if (has_prealloc)
+			flags = BTRFS_FILE_EXTENT_PREALLOC;
 
 		key.objectid = data->convert_ino;
 		key.type = BTRFS_EXTENT_DATA_KEY;
@@ -316,12 +337,12 @@ int record_file_blocks(struct blk_iterate_data *data,
 			      old_disk_bytenr + num_bytes) - cur_off;
 		ret = btrfs_record_file_extent(data->trans, data->root,
 					data->objectid, data->inode, file_pos,
-					real_disk_bytenr, cur_len,
-					BTRFS_FILE_EXTENT_REG);
+					real_disk_bytenr, cur_len, flags);
 		if (ret < 0)
 			break;
 		cur_off += cur_len;
 		file_pos += cur_len;
+		index++;
 
 		/*
 		 * No need to care about csum
diff --git a/convert/source-fs.h b/convert/source-fs.h
index 25916c65681b..db7ead422585 100644
--- a/convert/source-fs.h
+++ b/convert/source-fs.h
@@ -153,5 +153,6 @@ int read_disk_extent(struct btrfs_root *root, u64 bytenr,
 		            u32 num_bytes, char *buffer);
 int record_file_blocks(struct blk_iterate_data *data,
 			      u64 file_block, u64 disk_block, u64 num_blocks);
+int find_prealloc(struct blk_iterate_data *data, int index, bool *has_prealloc);
 
 #endif
-- 
2.39.3

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

[Qu Wenruo]

在 2024/5/6 12:34, Anand Jain 写道:
> This patch, along with the dependent patches below, adds support for
> ext4 unmerged  unwritten file extents as preallocated file extent in btrfs.
>
>   btrfs-progs: convert: refactor ext2_create_file_extents add argument ext2_inode
>   btrfs-progs: convert: struct blk_iterate_data, add ext2-specific file inode pointers
>   btrfs-progs: convert: refactor __btrfs_record_file_extent to add a prealloc flag
>
> The patch is developed with POV of portability with the current
> e2fsprogs library.
>
> This patch will handle independent unwritten extents by marking them with prealloc
> flag and will identify merged unwritten extents, triggering a fail.
>
> Testcase:
>
>       $ dd if=/dev/urandom of=/mnt/test/foo bs=4K count=1 conv=fsync status=none
>       $ dd if=/dev/urandom of=/mnt/test/foo bs=4K count=2 conv=fsync seek=1 status=none
>       $ xfs_io -f -c 'falloc -k 12K 12K' /mnt/test/foo
>       $ dd if=/dev/zero of=/mnt/test/foo bs=4K count=1 conv=fsync seek=6 status=none
>
>       $ filefrag -v /mnt/test/foo
>       Filesystem type is: ef53
>       File size of /mnt/test/foo is 28672 (7 blocks of 4096 bytes)
> 	 ext:     logical_offset:        physical_offset: length:   expected: flags:
> 	   0:        0..       0:      33280..     33280:      1:
> 	   1:        1..       2:      33792..     33793:      2:      33281:
> 	   2:        3..       5:      33281..     33283:      3:      33794: unwritten
> 	   3:        6..       6:      33794..     33794:      1:      33284: last,eof
>
>       $ sha256sum /mnt/test/foo
>       18619b678a5c207a971a0aa931604f48162e307c57ecdec450d5f095fe9f32c7  /mnt/test/foo
>
>     Convert and compare the checksum
>
>     Before:
>
>       $ filefrag -v /mnt/test/foo
>       Filesystem type is: 9123683e
>       File size of /mnt/test/foo is 28672 (7 blocks of 4096 bytes)
>        ext:     logical_offset:        physical_offset: length:   expected: flags:
>        0:        0..       0:      33280..     33280:      1:             shared
>        1:        1..       2:      33792..     33793:      2:      33281: shared
>        2:        3..       6:      33281..     33284:      4:      33794: last,shared,eof
>       /mnt/test/foo: 3 extents found
>
>       $ sha256sum /mnt/test/foo
>       6874a1733e5785682210d69c07f256f684cf5433c7235ed29848b4a4d52030e0  /mnt/test/foo
>
>     After:
>
>       $ filefrag -v /mnt/test/foo
>       Filesystem type is: 9123683e
>       File size of /mnt/test/foo is 28672 (7 blocks of 4096 bytes)
> 	 ext:     logical_offset:        physical_offset: length:   expected: flags:
> 	   0:        0..       0:      33280..     33280:      1:             shared
> 	   1:        1..       2:      33792..     33793:      2:      33281: shared
> 	   2:        3..       5:      33281..     33283:      3:      33794: unwritten,shared
> 	   3:        6..       6:      33794..     33794:      1:      33284: last,shared,eof
>       /mnt/test/foo: 4 extents found
>
>       $ sha256sum /mnt/test/foo
>       18619b678a5c207a971a0aa931604f48162e307c57ecdec450d5f095fe9f32c7  /mnt/test/foo
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2:
>
> . 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:

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

Thanks,
Qu

> +
> +	if (extent.e_flags & EXT2_EXTENT_FLAGS_UNINIT)
> +		*has_unwritten = true;
> +
> +	return 0;
> +}
> +
>   static int ext2_dir_iterate_proc(ext2_ino_t dir, int entry,
>   			    struct ext2_dir_entry *dirent,
>   			    int offset, int blocksize,
> diff --git a/convert/source-ext2.h b/convert/source-ext2.h
> index 026a7cad8ac8..19014d3c25e6 100644
> --- a/convert/source-ext2.h
> +++ b/convert/source-ext2.h
> @@ -82,6 +82,9 @@ struct ext2_source_fs {
>   	ext2_ino_t ext2_ino;
>   };
>
> +int ext2_find_unwritten(struct blk_iterate_data *data, int index,
> +			bool *has_unwritten);
> +
>   #define EXT2_ACL_VERSION	0x0001
>
>   #endif	/* BTRFSCONVERT_EXT2 */
> diff --git a/convert/source-fs.c b/convert/source-fs.c
> index df5ce66caf7f..88a6ceaf41f6 100644
> --- a/convert/source-fs.c
> +++ b/convert/source-fs.c
> @@ -31,6 +31,7 @@
>   #include "common/extent-tree-utils.h"
>   #include "convert/common.h"
>   #include "convert/source-fs.h"
> +#include "convert/source-ext2.h"
>
>   const struct simple_range btrfs_reserved_ranges[3] = {
>   	{ 0,			     SZ_1M },
> @@ -239,6 +240,15 @@ fail:
>   	return ret;
>   }
>
> +int find_prealloc(struct blk_iterate_data *data, int index,
> +		  bool *has_prealloc)
> +{
> +	if (data->source_fs)
> +		return ext2_find_unwritten(data, index, has_prealloc);
> +
> +	return -EINVAL;
> +}
> +
>   /*
>    * Record a file extent in original filesystem into btrfs one.
>    * The special point is, old disk_block can point to a reserved range.
> @@ -257,6 +267,7 @@ int record_file_blocks(struct blk_iterate_data *data,
>   	u64 old_disk_bytenr = disk_block * sectorsize;
>   	u64 num_bytes = num_blocks * sectorsize;
>   	u64 cur_off = old_disk_bytenr;
> +	int index = data->first_block;
>
>   	/* Hole, pass it to record_file_extent directly */
>   	if (old_disk_bytenr == 0)
> @@ -276,6 +287,16 @@ int record_file_blocks(struct blk_iterate_data *data,
>   		u64 extent_num_bytes;
>   		u64 real_disk_bytenr;
>   		u64 cur_len;
> +		u64 flags = BTRFS_FILE_EXTENT_REG;
> +		bool has_prealloc = false;
> +
> +		if (find_prealloc(data, index, &has_prealloc)) {
> +			data->errcode = -1;
> +			return -EINVAL;
> +		}
> +
> +		if (has_prealloc)
> +			flags = BTRFS_FILE_EXTENT_PREALLOC;
>
>   		key.objectid = data->convert_ino;
>   		key.type = BTRFS_EXTENT_DATA_KEY;
> @@ -316,12 +337,12 @@ int record_file_blocks(struct blk_iterate_data *data,
>   			      old_disk_bytenr + num_bytes) - cur_off;
>   		ret = btrfs_record_file_extent(data->trans, data->root,
>   					data->objectid, data->inode, file_pos,
> -					real_disk_bytenr, cur_len,
> -					BTRFS_FILE_EXTENT_REG);
> +					real_disk_bytenr, cur_len, flags);
>   		if (ret < 0)
>   			break;
>   		cur_off += cur_len;
>   		file_pos += cur_len;
> +		index++;
>
>   		/*
>   		 * No need to care about csum
> diff --git a/convert/source-fs.h b/convert/source-fs.h
> index 25916c65681b..db7ead422585 100644
> --- a/convert/source-fs.h
> +++ b/convert/source-fs.h
> @@ -153,5 +153,6 @@ int read_disk_extent(struct btrfs_root *root, u64 bytenr,
>   		            u32 num_bytes, char *buffer);
>   int record_file_blocks(struct blk_iterate_data *data,
>   			      u64 file_block, u64 disk_block, u64 num_blocks);
> +int find_prealloc(struct blk_iterate_data *data, int index, bool *has_prealloc);
>
>   #endif

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

[Qu Wenruo]

在 2024/5/6 15:11, Qu Wenruo 写道:
> 
> 
> 在 2024/5/6 12:34, Anand Jain 写道:
>> This patch, along with the dependent patches below, adds support for
>> ext4 unmerged  unwritten file extents as preallocated file extent in 
>> btrfs.
>>
>>   btrfs-progs: convert: refactor ext2_create_file_extents add argument 
>> ext2_inode
>>   btrfs-progs: convert: struct blk_iterate_data, add ext2-specific 
>> file inode pointers
>>   btrfs-progs: convert: refactor __btrfs_record_file_extent to add a 
>> prealloc flag
>>
>> The patch is developed with POV of portability with the current
>> e2fsprogs library.
>>
>> This patch will handle independent unwritten extents by marking them 
>> with prealloc
>> flag and will identify merged unwritten extents, triggering a fail.
>>
>> Testcase:
>>
>>       $ dd if=/dev/urandom of=/mnt/test/foo bs=4K count=1 conv=fsync 
>> status=none
>>       $ dd if=/dev/urandom of=/mnt/test/foo bs=4K count=2 conv=fsync 
>> seek=1 status=none
>>       $ xfs_io -f -c 'falloc -k 12K 12K' /mnt/test/foo
>>       $ dd if=/dev/zero of=/mnt/test/foo bs=4K count=1 conv=fsync 
>> seek=6 status=none
>>
>>       $ filefrag -v /mnt/test/foo
>>       Filesystem type is: ef53
>>       File size of /mnt/test/foo is 28672 (7 blocks of 4096 bytes)
>>      ext:     logical_offset:        physical_offset: length:   
>> expected: flags:
>>        0:        0..       0:      33280..     33280:      1:
>>        1:        1..       2:      33792..     33793:      2:      33281:
>>        2:        3..       5:      33281..     33283:      3:      
>> 33794: unwritten
>>        3:        6..       6:      33794..     33794:      1:      
>> 33284: last,eof
>>
>>       $ sha256sum /mnt/test/foo
>>       
>> 18619b678a5c207a971a0aa931604f48162e307c57ecdec450d5f095fe9f32c7  
>> /mnt/test/foo
>>
>>     Convert and compare the checksum
>>
>>     Before:
>>
>>       $ filefrag -v /mnt/test/foo
>>       Filesystem type is: 9123683e
>>       File size of /mnt/test/foo is 28672 (7 blocks of 4096 bytes)
>>        ext:     logical_offset:        physical_offset: length:   
>> expected: flags:
>>        0:        0..       0:      33280..     33280:      
>> 1:             shared
>>        1:        1..       2:      33792..     33793:      2:      
>> 33281: shared
>>        2:        3..       6:      33281..     33284:      4:      
>> 33794: last,shared,eof
>>       /mnt/test/foo: 3 extents found
>>
>>       $ sha256sum /mnt/test/foo
>>       
>> 6874a1733e5785682210d69c07f256f684cf5433c7235ed29848b4a4d52030e0  
>> /mnt/test/foo
>>
>>     After:
>>
>>       $ filefrag -v /mnt/test/foo
>>       Filesystem type is: 9123683e
>>       File size of /mnt/test/foo is 28672 (7 blocks of 4096 bytes)
>>      ext:     logical_offset:        physical_offset: length:   
>> expected: flags:
>>        0:        0..       0:      33280..     33280:      
>> 1:             shared
>>        1:        1..       2:      33792..     33793:      2:      
>> 33281: shared
>>        2:        3..       5:      33281..     33283:      3:      
>> 33794: unwritten,shared
>>        3:        6..       6:      33794..     33794:      1:      
>> 33284: last,shared,eof
>>       /mnt/test/foo: 4 extents found
>>
>>       $ sha256sum /mnt/test/foo
>>       
>> 18619b678a5c207a971a0aa931604f48162e307c57ecdec450d5f095fe9f32c7  
>> /mnt/test/foo
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v2:
>>
>> . 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:
> 
>   # 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.

Thanks,
Qu
> 
> Thanks,
> Qu
> 
>> +
>> +    if (extent.e_flags & EXT2_EXTENT_FLAGS_UNINIT)
>> +        *has_unwritten = true;
>> +
>> +    return 0;
>> +}
>> +
>>   static int ext2_dir_iterate_proc(ext2_ino_t dir, int entry,
>>                   struct ext2_dir_entry *dirent,
>>                   int offset, int blocksize,
>> diff --git a/convert/source-ext2.h b/convert/source-ext2.h
>> index 026a7cad8ac8..19014d3c25e6 100644
>> --- a/convert/source-ext2.h
>> +++ b/convert/source-ext2.h
>> @@ -82,6 +82,9 @@ struct ext2_source_fs {
>>       ext2_ino_t ext2_ino;
>>   };
>>
>> +int ext2_find_unwritten(struct blk_iterate_data *data, int index,
>> +            bool *has_unwritten);
>> +
>>   #define EXT2_ACL_VERSION    0x0001
>>
>>   #endif    /* BTRFSCONVERT_EXT2 */
>> diff --git a/convert/source-fs.c b/convert/source-fs.c
>> index df5ce66caf7f..88a6ceaf41f6 100644
>> --- a/convert/source-fs.c
>> +++ b/convert/source-fs.c
>> @@ -31,6 +31,7 @@
>>   #include "common/extent-tree-utils.h"
>>   #include "convert/common.h"
>>   #include "convert/source-fs.h"
>> +#include "convert/source-ext2.h"
>>
>>   const struct simple_range btrfs_reserved_ranges[3] = {
>>       { 0,                 SZ_1M },
>> @@ -239,6 +240,15 @@ fail:
>>       return ret;
>>   }
>>
>> +int find_prealloc(struct blk_iterate_data *data, int index,
>> +          bool *has_prealloc)
>> +{
>> +    if (data->source_fs)
>> +        return ext2_find_unwritten(data, index, has_prealloc);
>> +
>> +    return -EINVAL;
>> +}
>> +
>>   /*
>>    * Record a file extent in original filesystem into btrfs one.
>>    * The special point is, old disk_block can point to a reserved range.
>> @@ -257,6 +267,7 @@ int record_file_blocks(struct blk_iterate_data *data,
>>       u64 old_disk_bytenr = disk_block * sectorsize;
>>       u64 num_bytes = num_blocks * sectorsize;
>>       u64 cur_off = old_disk_bytenr;
>> +    int index = data->first_block;
>>
>>       /* Hole, pass it to record_file_extent directly */
>>       if (old_disk_bytenr == 0)
>> @@ -276,6 +287,16 @@ int record_file_blocks(struct blk_iterate_data 
>> *data,
>>           u64 extent_num_bytes;
>>           u64 real_disk_bytenr;
>>           u64 cur_len;
>> +        u64 flags = BTRFS_FILE_EXTENT_REG;
>> +        bool has_prealloc = false;
>> +
>> +        if (find_prealloc(data, index, &has_prealloc)) {
>> +            data->errcode = -1;
>> +            return -EINVAL;
>> +        }
>> +
>> +        if (has_prealloc)
>> +            flags = BTRFS_FILE_EXTENT_PREALLOC;
>>
>>           key.objectid = data->convert_ino;
>>           key.type = BTRFS_EXTENT_DATA_KEY;
>> @@ -316,12 +337,12 @@ int record_file_blocks(struct blk_iterate_data 
>> *data,
>>                     old_disk_bytenr + num_bytes) - cur_off;
>>           ret = btrfs_record_file_extent(data->trans, data->root,
>>                       data->objectid, data->inode, file_pos,
>> -                    real_disk_bytenr, cur_len,
>> -                    BTRFS_FILE_EXTENT_REG);
>> +                    real_disk_bytenr, cur_len, flags);
>>           if (ret < 0)
>>               break;
>>           cur_off += cur_len;
>>           file_pos += cur_len;
>> +        index++;
>>
>>           /*
>>            * No need to care about csum
>> diff --git a/convert/source-fs.h b/convert/source-fs.h
>> index 25916c65681b..db7ead422585 100644
>> --- a/convert/source-fs.h
>> +++ b/convert/source-fs.h
>> @@ -153,5 +153,6 @@ int read_disk_extent(struct btrfs_root *root, u64 
>> bytenr,
>>                       u32 num_bytes, char *buffer);
>>   int record_file_blocks(struct blk_iterate_data *data,
>>                     u64 file_block, u64 disk_block, u64 num_blocks);
>> +int find_prealloc(struct blk_iterate_data *data, int index, bool 
>> *has_prealloc);
>>
>>   #endif

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

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


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

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

[Qu Wenruo]

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

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

[Qu Wenruo]

在 2024/5/6 12:34, Anand Jain 写道:
[...]
>       18619b678a5c207a971a0aa931604f48162e307c57ecdec450d5f095fe9f32c7  /mnt/test/foo
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2:
>
> . Remove RFC

Nope, you didn't even test the patch using the selftests.
(Despite the RST introduced failure, the first ext2 is enough to expose
your bug).

Not to mention the lack of functionality to handle part written part
unwritten extents.

This is NOT acceptable at all.

> +	if (ext2fs_extent_open2(src->ext2_fs, src->ext2_ino,
> +				src->ext2_inode, &handle)) {
> +		error("ext2fs_extent_open2 failed, inode %d", src->ext2_ino);
> +		return -EINVAL;
> +	}

Have you tried ext2?

ext2fs_extent_open()/open2() all needs the inode to have EXT4_EXTENTS_FL
flag, or it would immediately return error.

> +
> +	if (ext2fs_extent_goto2(handle, 0, index)) {
> +		error("ext2fs_extent_goto2 failed, inode %d num_blocks %llu",
> +		       src->ext2_ino, data->num_blocks);
> +		ext2fs_extent_free(handle);
> +		return -EINVAL;
> +	}
> +
> +	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 didn't handle the halfwritten extent correctly.

And it looks like you didn't even understand how to properly iterate
through all the extents.

In fact, if you really spent your time to check how debugfs
"dump_extents" command works, it would be clear that we can easily
iterate the split extents.

I strongly doubt if you're handling the case correctly or with any
responsibly.

I already have a small patch to properly handle all the legacy and newer
EXTENTS_FL features, and handle the uninit extents correctly.
I'll do cleanup the RST test case first, run the full convert tests at
least, with proper test case for it.

I appreciate your effort so far, but this is really below our standard.

Thanks,
Qu

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

[Anand Jain]

On 5/7/24 09:25, Qu Wenruo wrote:
> 
> 
> 在 2024/5/6 12:34, Anand Jain 写道:
> [...]
>>       
>> 18619b678a5c207a971a0aa931604f48162e307c57ecdec450d5f095fe9f32c7  
>> /mnt/test/foo
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v2:
>>
>> . Remove RFC
> 
> Nope, you didn't even test the patch using the selftests.
> (Despite the RST introduced failure, the first ext2 is enough to expose
> your bug).
> 

Realizing the existing test case missed testing unwritten extents,
I wrote my own, and they passed as expected. Later, I completely
forgot running the existing test cases, my bad.

Overall, the initial work inspired better designs. I'm glad the
issue was resolved.