[PATCH v1] ext4: add test case 061 for ext3 to ext4 conversion

[PATCH v1] ext4: add test case 061 for ext3 to ext4 conversion

[Boyang Xue]

Signed-off-by: Boyang Xue <bxue@redhat.com>
---
 tests/ext4/061     | 63 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/ext4/061.out | 17 +++++++++++++
 2 files changed, 80 insertions(+)
 create mode 100755 tests/ext4/061
 create mode 100644 tests/ext4/061.out

diff --git a/tests/ext4/061 b/tests/ext4/061
new file mode 100755
index 00000000..f42f2a92
--- /dev/null
+++ b/tests/ext4/061
@@ -0,0 +1,63 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2025 Red Hat Inc.  All Rights Reserved.
+#
+# FS QA Test 061
+#
+# Test conversion from ext3 to ext4 filesystem with various mount options
+#
+. ./common/preamble
+_begin_fstest auto
+
+# Import common functions.
+. ./common/filter
+
+_supported_fs ext3 ext4
+
+_require_scratch
+
+MOUNT_OPTS_LIST=(
+    ""
+    "noatime"
+    "data=journal"
+    "data=writeback"
+)
+
+_cleanup()
+{
+    if _is_dev_mounted "$SCRATCH_DEV" > /dev/null 2>&1; then
+        $UMOUNT_PROG "$SCRATCH_DEV" || _fail "_cleanup: umount failed"
+    fi
+}
+
+
+fill_fs() {
+    echo "Filling filesystem..."
+    while true; do
+        dd if=/dev/urandom of="${SCRATCH_MNT}/file$(date +%s)" bs=1M count=4 \
+> /dev/null 2>&1 || _fail "dd failed"
+        df -h ${SCRATCH_MNT} | awk 'NR==2 {print $5}' | grep -q '[9][0-9]%' \
+&& break
+    done
+    echo "Filesystem almost full."
+}
+
+for mount_opt in "${MOUNT_OPTS_LIST[@]}"; do
+    echo "Starting test with mount options: '$mount_opt'"
+    mkfs.ext3 -F "$SCRATCH_DEV" 1g >> $seqres.full 2>&1 \
+|| _fail "mkfs.ext3 failed"
+    mount -t ext3 -o "$mount_opt" "$SCRATCH_DEV" "$SCRATCH_MNT" \
+ >> $seqres.full 2>&1 || _fail "mount ext3 failed"
+    fill_fs
+    umount "$SCRATCH_MNT" >> $seqres.full 2>&1 || _fail "umount ext3 failed"
+    tune2fs -O extents,uninit_bg,dir_index "$SCRATCH_DEV" \
+>> $seqres.full 2>&1 || _fail "tune2fs failed"
+    e2fsck -f -y "$SCRATCH_DEV" >> $seqres.full 2>&1 || _fail "e2fsck failed"
+    mount -t ext4 "$SCRATCH_DEV" "$SCRATCH_MNT" >> $seqres.full 2>&1 \
+|| _fail "mount ext4 failed"
+    umount "$SCRATCH_MNT" >> $seqres.full 2>&1 || _fail "umount ext3 failed"
+    echo "Test with mount options: '$mount_opt' completed successfully."
+done
+
+status=0
+exit
diff --git a/tests/ext4/061.out b/tests/ext4/061.out
new file mode 100644
index 00000000..ed2997a0
--- /dev/null
+++ b/tests/ext4/061.out
@@ -0,0 +1,17 @@
+QA output created by 061
+Starting test with mount options: ''
+Filling filesystem...
+Filesystem almost full.
+Test with mount options: '' completed successfully.
+Starting test with mount options: 'noatime'
+Filling filesystem...
+Filesystem almost full.
+Test with mount options: 'noatime' completed successfully.
+Starting test with mount options: 'data=journal'
+Filling filesystem...
+Filesystem almost full.
+Test with mount options: 'data=journal' completed successfully.
+Starting test with mount options: 'data=writeback'
+Filling filesystem...
+Filesystem almost full.
+Test with mount options: 'data=writeback' completed successfully.
-- 
2.48.1

Re: [PATCH v1] ext4: add test case 061 for ext3 to ext4 conversion

[Zorro Lang]

On Thu, Feb 20, 2025 at 03:20:29PM +0800, Boyang Xue wrote:

About the subject "ext4: add test case 061 for ext3 to ext4 conversion",
the case number is not fixed before merging, so better to change it as:
"ext4: test conversion from ext3 to ext4".

And do you have more detailed commit log at here?

> Signed-off-by: Boyang Xue <bxue@redhat.com>
> ---
>  tests/ext4/061     | 63 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/ext4/061.out | 17 +++++++++++++
>  2 files changed, 80 insertions(+)
>  create mode 100755 tests/ext4/061
>  create mode 100644 tests/ext4/061.out
> 
> diff --git a/tests/ext4/061 b/tests/ext4/061
> new file mode 100755
> index 00000000..f42f2a92
> --- /dev/null
> +++ b/tests/ext4/061
> @@ -0,0 +1,63 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2025 Red Hat Inc.  All Rights Reserved.
> +#
> +# FS QA Test 061
> +#
> +# Test conversion from ext3 to ext4 filesystem with various mount options
> +#
> +. ./common/preamble
> +_begin_fstest auto

                      quick?

And the doc/group-names.txt has one line:
  convert                   btrfs ext[34] conversion tool

currently the "convert" group is only used for btrfs, but it metions ext[34].
So I'm wondering if we should add this group to this test.

CC ext4 list to get more reviewing.

> +
> +# Import common functions.
> +. ./common/filter

Do you use any filter helpers ?

> +
> +_supported_fs ext3 ext4

There's not _supported_fs helper anymore, if you're sure ext2 isn't suit for
this test, you can use _exclude_fs. But from your below codes, you don't need
it.

> +
> +_require_scratch
> +
> +MOUNT_OPTS_LIST=(
> +    ""
> +    "noatime"
> +    "data=journal"
> +    "data=writeback"

Why does this case test these 3 mount options particularly, not others?

> +)
> +
> +_cleanup()
> +{
> +    if _is_dev_mounted "$SCRATCH_DEV" > /dev/null 2>&1; then
> +        $UMOUNT_PROG "$SCRATCH_DEV" || _fail "_cleanup: umount failed"

Is this a test step or a cleanup step? If it's a test step, please move
it to offical test context. If it's a cleanup step, it's useless, due to
xfstests always trys to umount $SCRATCH_DEV.

This looks not like a _cleanup step, more likes a test step. So better
to move it to offical test context.

> +    fi
> +}
> +
> +
> +fill_fs() {
> +    echo "Filling filesystem..."
> +    while true; do
> +        dd if=/dev/urandom of="${SCRATCH_MNT}/file$(date +%s)" bs=1M count=4 \
> +> /dev/null 2>&1 || _fail "dd failed"
> +        df -h ${SCRATCH_MNT} | awk 'NR==2 {print $5}' | grep -q '[9][0-9]%' \
> +&& break
> +    done
> +    echo "Filesystem almost full."
> +}

Can _fill_fs (in common/populate) help that?

> +
> +for mount_opt in "${MOUNT_OPTS_LIST[@]}"; do
> +    echo "Starting test with mount options: '$mount_opt'"
> +    mkfs.ext3 -F "$SCRATCH_DEV" 1g >> $seqres.full 2>&1 \
> +|| _fail "mkfs.ext3 failed"
> +    mount -t ext3 -o "$mount_opt" "$SCRATCH_DEV" "$SCRATCH_MNT" \
> + >> $seqres.full 2>&1 || _fail "mount ext3 failed"
> +    fill_fs
> +    umount "$SCRATCH_MNT" >> $seqres.full 2>&1 || _fail "umount ext3 failed"
> +    tune2fs -O extents,uninit_bg,dir_index "$SCRATCH_DEV" \
> +>> $seqres.full 2>&1 || _fail "tune2fs failed"

_require_command "$TUNE2FS_PROG" tune2fs

> +    e2fsck -f -y "$SCRATCH_DEV" >> $seqres.full 2>&1 || _fail "e2fsck failed"
> +    mount -t ext4 "$SCRATCH_DEV" "$SCRATCH_MNT" >> $seqres.full 2>&1 \
> +|| _fail "mount ext4 failed"
> +    umount "$SCRATCH_MNT" >> $seqres.full 2>&1 || _fail "umount ext3 failed"
> +    echo "Test with mount options: '$mount_opt' completed successfully."
> +done

How about using common helpers. Something likes:

# prepare an ext3
_scratch_mkfs -t ext3 >> $seqres.full 2>&1 || _fail "Fail to create ext3"
_scratch_mount
_fill_fs
_scratch_unmount
# convert ext3 to ext4
$TUNE2FS_PROG -O extents,uninit_bg,dir_index $SCRATCH_DEV
_check_scratch_fs
_scratch_mount
_scratch_unmount

(CC ext4 list too)

Thanks,
Zorro

> +
> +status=0
> +exit
> diff --git a/tests/ext4/061.out b/tests/ext4/061.out
> new file mode 100644
> index 00000000..ed2997a0
> --- /dev/null
> +++ b/tests/ext4/061.out
> @@ -0,0 +1,17 @@
> +QA output created by 061
> +Starting test with mount options: ''
> +Filling filesystem...
> +Filesystem almost full.
> +Test with mount options: '' completed successfully.
> +Starting test with mount options: 'noatime'
> +Filling filesystem...
> +Filesystem almost full.
> +Test with mount options: 'noatime' completed successfully.
> +Starting test with mount options: 'data=journal'
> +Filling filesystem...
> +Filesystem almost full.
> +Test with mount options: 'data=journal' completed successfully.
> +Starting test with mount options: 'data=writeback'
> +Filling filesystem...
> +Filesystem almost full.
> +Test with mount options: 'data=writeback' completed successfully.
> -- 
> 2.48.1
> 
> 

Re: [PATCH v1] ext4: add test case 061 for ext3 to ext4 conversion

[Theodore Ts'o]

I'm curious why this is something that you care about this particular
case?  Historically, Red Hat (nor any other enterprise distribution)
has supported changing or adding ext4 feature flags.  What users were
told to do was to create a new ext4 file system, and then copy the
files (or do a backup/restore) to the new file system.

Over a decade ago, when I was at Google, we did do a transition of
file sytems from ext2 to ext4 (in no-journal mode) for all files
stored in our cluster file systems.  Our experience for our partcular
workload is that the performance delta between an existing file system
that was formatted w/o extents, and uninit_bg, and then converted to
"ext4" using tune2fs, was roughly half the performance improvement of
a file system that was freshly formatted to use ext4 once the file
system was filled to roughly the same level, so that file system aging
wastaken into account.

So users who do the conversion won't get the full benefits of ext4.
Also, the exact set of file system features that we changed was
different from from what you are testing.  I'd also note that this
test is just filling a 1G file system, then changing the file system
features, and then running fsck on the file system, and then trying to
mount the file system.

This isn't actually a very exhaustive test of doing a "conversion".
If you really want to do a "coversion" test, what I'd recommend is to
fill the TEST_DEV to be about 50% full, and then make the changes to
the file system features, and then running the xfstests using the ext4
file system type without SCRATCH_DEV being set up.  This would do a
much better job of testing this case, without needing to add a new
test to xfstest that to be honest, isn't really all that effective at
testing this case.

What did we do, 10+ year ago?  Well, we ran xfstests using the a file
system config[1] that didn't have flex_bg set, since that's one of the
primary differences when using a "converted" file system.  We then
using tune2fs to convert 1% of the file systems in a single failure
domain of the cluster file system, and watched looking for potential
problems, and measuring for any performances gains (or losses).  We
then gradually increased the percentage of file systems converted to
2%, 5%, etc which is considered best practice[2] when doing this kind
of feature rollout.

[1] https://github.com/tytso/xfstests-bld/blob/master/test-appliance/files/root/fs/ext4/cfg/ext3conv
[2] https://books.google.com/books?id=81UrjwEACAAJ

Cheers,

						- Ted

Re: [PATCH v1] ext4: add test case 061 for ext3 to ext4 conversion

[Boyang Xue]

Hi Ted,

Thanks for your reply. I learned a lot from the text.

On Thu, Feb 20, 2025 at 11:16 PM Theodore Ts'o <tytso@mit.edu> wrote:
>
> I'm curious why this is something that you care about this particular
> case?  Historically, Red Hat (nor any other enterprise distribution)
> has supported changing or adding ext4 feature flags.  What users were
> told to do was to create a new ext4 file system, and then copy the
> files (or do a backup/restore) to the new file system.

We had learned we did have customers insist on doing the conversion
rather than creating a fresh new ext4 and copying files for some
reason, so we hope our test can cover this use case.

> Over a decade ago, when I was at Google, we did do a transition of
> file sytems from ext2 to ext4 (in no-journal mode) for all files
> stored in our cluster file systems.  Our experience for our partcular
> workload is that the performance delta between an existing file system
> that was formatted w/o extents, and uninit_bg, and then converted to
> "ext4" using tune2fs, was roughly half the performance improvement of
> a file system that was freshly formatted to use ext4 once the file
> system was filled to roughly the same level, so that file system aging
> wastaken into account.
>
> So users who do the conversion won't get the full benefits of ext4.
> Also, the exact set of file system features that we changed was
> different from from what you are testing.  I'd also note that this
> test is just filling a 1G file system, then changing the file system
> features, and then running fsck on the file system, and then trying to
> mount the file system.
>
> This isn't actually a very exhaustive test of doing a "conversion".
> If you really want to do a "coversion" test, what I'd recommend is to
> fill the TEST_DEV to be about 50% full, and then make the changes to
> the file system features, and then running the xfstests using the ext4
> file system type without SCRATCH_DEV being set up.  This would do a
> much better job of testing this case, without needing to add a new
> test to xfstest that to be honest, isn't really all that effective at
> testing this case.

I'm curious why the TEST_DEV is about 50% full is more effective at
testing this case?

>
> What did we do, 10+ year ago?  Well, we ran xfstests using the a file
> system config[1] that didn't have flex_bg set, since that's one of the
> primary differences when using a "converted" file system.  We then
> using tune2fs to convert 1% of the file systems in a single failure
> domain of the cluster file system, and watched looking for potential
> problems, and measuring for any performances gains (or losses).  We
> then gradually increased the percentage of file systems converted to
> 2%, 5%, etc which is considered best practice[2] when doing this kind
> of feature rollout.
>

I'm wondering is the [1] ext4 config can be used to simulated a
tune2fs converted ext3 fs?

> [1] https://github.com/tytso/xfstests-bld/blob/master/test-appliance/files/root/fs/ext4/cfg/ext3conv
> [2] https://books.google.com/books?id=81UrjwEACAAJ
>
> Cheers,
>
>                                                 - Ted
>

Cheers,
Boyang

Re: [PATCH v1] ext4: add test case 061 for ext3 to ext4 conversion

[Theodore Ts'o]

On Fri, Feb 21, 2025 at 01:06:04AM +0800, Boyang Xue wrote:
> > This isn't actually a very exhaustive test of doing a "conversion".
> > If you really want to do a "coversion" test, what I'd recommend is to
> > fill the TEST_DEV to be about 50% full, and then make the changes to
> > the file system features, and then running the xfstests using the ext4
> > file system type without SCRATCH_DEV being set up.  This would do a
> > much better job of testing this case, without needing to add a new
> > test to xfstest that to be honest, isn't really all that effective at
> > testing this case.
> 
> I'm curious why the TEST_DEV is about 50% full is more effective at
> testing this case?

The reason why this is useful is because we are starting with a file
system that 50% of the space filled with files using indirect files,
and when you run the tests, you are created and deleting files using
extents.  That's actualy not perfect.  What we would want to do is to
create ext2/ext3-style files using indirect block maps, that would
then be mutated using fstress and fsx in parallel with files being
created, mutated, and deleted using extent-mapped files.

The basic idea is that you want to actually exercise *using* the file
system after it is converted -- reading and writing files, allocating
and dealocating blocks and inodes, after converting the file sytem --
and then making sure that the kernel doesn't crash or result in a
corrupted file system.

Your proposed test just fills the file system, unmounts it, uses
tune2fs to add the extents feature flag, and then runs e2fsck on the
file system, and then tries mounting the file system.  That's pretty
much guaranteed to work, since running fsck.ext4 on a file system that
was originally created as ext3, with lots of indirect mapped files,
and then adding the extent flag, essentially exercises *zero*
new or different code paths.

Cheers,

					- Ted