Re: [PATCH] ext4/035: Verify directory shrinking

[Eryu Guan <guaneryu@gmail.com>]

Hi,

On Wed, Feb 27, 2019 at 04:03:16PM -0800, Harshad Shirwadkar wrote:
> Add test to verify that directory shrinking works for ext4 and also
> doing so repeatedly does not introduce any errors in file system.
> 
> Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>

Thanks for the test! But a couple of issues in line :)

But one major problem is that, we need a way to check if the ext4 being
tested supports shrinking directory, and skip the test if it doesn't, as
shrink directory is a new feature, we don't want the test to report
failure on unpatched ext4, which gives false failure.

And it seems like this test is not ext4-specific, some other filesystems
support shrink directory as well, e.g. xfs and btrfs, so the test could
be a generic test, as long as we could detect if the filesystem supports
the feature or not.

> ---
>  tests/ext4/035     | 109 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/ext4/035.out |   9 ++++
>  tests/ext4/group   |   1 +

If adding a new fs-specific test, it'd be better to cc the fs's mailing
list too, e.g. linux-ext4@vger.kernel.org in this case.

>  3 files changed, 119 insertions(+)
>  create mode 100644 tests/ext4/035

Add new test with 0755 permission. It's highly recommended to use the
'new' script to create new test template, it's handling such issues for
you already. e.g.

./new ext4

>  create mode 100644 tests/ext4/035.out
> 
> diff --git a/tests/ext4/035 b/tests/ext4/035
> new file mode 100644
> index 00000000..46898d99
> --- /dev/null
> +++ b/tests/ext4/035
> @@ -0,0 +1,109 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2019 Google, Inc.  All Rights Reserved.
> +#
> +# FS QA Test No. 035
> +#
> +# Verify that directory shrinking works and that it does not introduce any
> +# errors.
> +#
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +    cd /
> +    #rm -f $tmp.*

Use tab as indention, not spaces, and remove $tmp.* even if you're not
using it in test explicitly, as the common functions may create $tmp.*
files too. Again, the 'new' script sets up template correctly.

> +}
> +
> +_make_files()

Local functions don't need the leading underscore.

> +{
> +    dirname=$1
> +    num_files=$2

And declare local variables as 'local'.

> +    for x in `seq 1 $num_files`; do
> +        fname="$(printf "%.255s\n" "$(perl -e "print \"${x}_\" x 500;")")"

Looks like you're copying test from ext4/017 and modifying based on
that. But I don't think this test doesn't need a specific pattern for file name, as
long as its name is long enough to take much directory space. So just do
$(perl -e 'print "a" x 255;') ?

> +        touch "${dirname}/${fname}"

echo > ${dirname}/${fname} would be a little faster, especially when
you're creating many files, that saves test time.

> +    done
> +}
> +
> +_make_dirs_and_files()
> +{
> +    parent_dir=$1
> +    num_dirs=$2
> +    num_files=$3
> +    for x in `seq 1 $num_dirs`; do
> +	mkdir ${parent_dir}/${x}
> +        _make_files "${parent_dir}/${x}" $num_files
> +    done
> +}
> +
> +_remove_dirs()
> +{
> +    parent_dir=$1
> +    num_dirs=$2
> +    for x in `seq 1 $num_dirs`; do
> +        find ${parent_dir}/${x} -type f -exec rm '{}' \;
> +    done
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +. ./common/attr

attr is not used, no need to source common/attr.

> +
> +# real QA test starts here
> +_supported_fs ext4
> +_supported_os Linux
> +
> +_require_scratch
> +# test -n "${FORCE_FUZZ}" || _require_scratch_ext4_crc
> +_require_attrs

Above two lines could be removed.

> +
> +rm -f $seqres.full
> +TESTDIR="${SCRATCH_MNT}/scratchdir"
> +TESTFILE="${TESTDIR}/testfile"

TESTDIR is only used in TESTFILE, but TESTFILE is not used in this test.

And we have a global TEST_DIR variable (mount point for $TEST_DEV), so
TESTDIR is a bad name, which may make people think it's TEST_DIR.

> +
> +echo "+ create scratch fs"
> +_scratch_mkfs_ext4 > /dev/null 2>&1

_scratch_mkfs, no need to call _scratch_mkfs_ext4 explicitly.

> +
> +echo "+ mount fs image"
> +_scratch_mount
> +
> +mkdir -p ${SCRATCH_MNT}/test
> +
> +echo "+ create files"
> +blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
> +_make_files ${SCRATCH_MNT}/test $((blksz * 4 / 256))

Any reason to create $((blksz * 4 / 256)) files?

> +
> +size_before=$(du -s "${SCRATCH_MNT}/test" | gawk '{print $1}')

I think you're going to take the size of the "test" dir inode not the
size of all the files within it? So it should be
"stat -c %s $SCRATCH_MNT/test"?

> +
> +echo "+ remove files"
> +rm ${SCRATCH_MNT}/test/*
> +
> +size_after=$(du -s "${SCRATCH_MNT}/test" | gawk '{print $1}')
> +
> +if [ $size_after -lt $size_before ]; then
> +  echo "+ directory shrinked"
> +else
> +  _fail "directory did not shrink"

Just echo "directory did not shrink", no need to _fail, the test harness
would capture the output and compare it with the golden output and fails
the test if they don't match.

> +fi
> +
> +echo "+ create many directories and files"
> +_make_dirs_and_files ${SCRATCH_MNT} 4 10240
> +
> +echo "+ empty dirs"
> +_remove_dirs ${SCRATCH_MNT} 4
> +
> +umount "${SCRATCH_MNT}"

_scratch_umount

> +
> +echo "+ check fs"
> +e2fsck -fn ${SCRATCH_DEV} >> $seqres.full 2>&1 || _fail "fsck should not fail"

No need to umount & check fs explicitly, test harness would do it for
you as well.

Thanks,
Eryu

> +
> +status=0
> +exit
> diff --git a/tests/ext4/035.out b/tests/ext4/035.out
> new file mode 100644
> index 00000000..2d9a34e1
> --- /dev/null
> +++ b/tests/ext4/035.out
> @@ -0,0 +1,9 @@
> +QA output created by 035
> ++ create scratch fs
> ++ mount fs image
> ++ create files
> ++ remove files
> ++ directory shrinked
> ++ create many directories and files
> ++ empty dirs
> ++ check fs
> diff --git a/tests/ext4/group b/tests/ext4/group
> index eb744a12..c25205dd 100644
> --- a/tests/ext4/group
> +++ b/tests/ext4/group
> @@ -37,6 +37,7 @@
>  032 auto quick ioctl resize
>  033 auto ioctl resize
>  034 auto quick quota
> +035 auto
>  271 auto rw quick
>  301 aio auto ioctl rw stress defrag
>  302 aio auto ioctl rw stress defrag
> -- 
> 2.21.0.rc2.261.ga7da99ff1b-goog
>