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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <20250220095812.t5pwikrf7ccsj3kr@dell-per750-06-vm-08.rhts.eng.pek2.redhat.com>
Date: Thu, 20 Feb 2025 17:58:12 +0800
From: Zorro Lang <zlang@...hat.com>
To: Boyang Xue <bxue@...hat.com>
Cc: fstests@...r.kernel.org, linux-ext4@...r.kernel.org
Subject: Re: [PATCH v1] ext4: add test case 061 for ext3 to ext4 conversion

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@...hat.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
> 
>