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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 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