Re: [v2] ext4: Add a test for rename with RENAME_WHITEOUT

[Zorro Lang <zlang@redhat.com>]

On Mon, Feb 01, 2021 at 02:12:46PM +0800, Sun Ke wrote:
> hi, Zorro
> 
> 在 2021/1/30 19:12, Zorro Lang 写道:
> > On Thu, Jan 28, 2021 at 02:12:02PM +0800, Sun Ke wrote:
> > > Fill the disk space, try to create some files and rename a file, mount
> > > again, list directory contents and triggers some errors. It is a
> > > regression test for kernel commit 6b4b8e6b4ad8 ("ext4: ext4: fix bug for
> > > rename with RENAME_WHITEOUT")
> > > 
> > > Signed-off-by: Sun Ke <sunke32@huawei.com>
> > > ---
> > > v1 -> v3:
> > > Use the original in src and modify 048.out
> > > ---
> > > ---
> > >   tests/ext4/048     | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >   tests/ext4/048.out |  2 ++
> > >   tests/ext4/group   |  1 +
> > >   3 files changed, 79 insertions(+)
> > >   create mode 100755 tests/ext4/048
> > >   create mode 100644 tests/ext4/048.out
> > > 
> > > diff --git a/tests/ext4/048 b/tests/ext4/048
> > > new file mode 100755
> > > index 00000000..0311d1a2
> > > --- /dev/null
> > > +++ b/tests/ext4/048
> > > @@ -0,0 +1,76 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (c) 2021 HUAWEI.  All Rights Reserved.
> > > +#
> > > +# FS QA Test 048
> > > +#
> > > +# This is a regression test for kernel patch:
> > > +# commit 6b4b8e6b4ad8 ("ext4: ext4: fix bug for rename with RENAME_WHITEOUT")
> > > +
> > > +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.*
> > > +}
> > > +
> > > +# get standard environment, filters and checks
> > > +. ./common/rc
> > > +. ./common/filter
> > > +
> > > +# remove previous $seqres.full before test
> > > +rm -f $seqres.full
> > > +
> > > +# real QA test starts here
> > > +
> > > +# Modify as appropriate.
> > > +_supported_fs ext4
> > > +_supported_fs generic
> > ext4 or generic? Tell the truth, according to below testing steps, I think it
> > can be a generic test case, not only for ext4.
> 
> When reproducing the bug, demsg show :
> 
>     EXT4-fs error (device loop0): ext4_lookup:1440: inode #2049: comm ls:
> deleted inode referenced: 139
> 
> It was printed in fs/ext4/super.c in funcation __ext4_error_inode. I search
> "deleted inode referenced" in linux kernel
> 
> and find 4 places:
> 
>     fs/ext2/namei.c: "deleted inode referenced: %lu",
>     fs/ext4/namei.c: "deleted inode referenced: %u",
>     fs/minix/inode.c:               printk("MINIX-fs: deleted inode
> referenced: %lu\n",
>     fs/minix/inode.c:               printk("MINIX-fs: deleted inode
> referenced: %lu\n",
> 
> If it is generic, I am not very sure how to distinguish success from
> failure.

Except this dmesg check, all other steps aren't extN related. So I'd like to
recommand:
1) Checking if we have a better way to avoid checking this ext4 only dmesg
output? Likes if that "ls -l test/dir/file1" can find this bug? Does it have
different output with/without the bug fix?

2) If we can't do the 1), and must check the ext4-only dmesg, we can use a
judgement to check it for extN only:

if [[ $FSTYP =~ ext ]];then
	check that dmesg
fi

then let other filesystems do dmesg checking by default.

But the 1) is still the best way I think.

Thanks,
Zorro

> 
> 
> > > +_require_scratch
> > > +_require_xfs_io_command "falloc"
> > > +
> > > +dmesg -c > /dev/null
> > > +
> > > +_scratch_mkfs > $seqres.full 2>&1
> > > +_scratch_mount >> $seqres.full 2>&1
> > > +
> > > +testdir=$SCRATCH_MNT
> > > +cd ${testdir}
> > > +
> > > +mkdir test
> > > +$XFS_IO_PROG -f -c "falloc 0 128M" img >> $seqres.full
> > > +$MKFS_EXT4_PROG  img > /dev/null 2>&1
> > > +$MOUNT_PROG img test
> > Can we reproduce this bug by $SCRATCH_DEV directly, not through a loopdev.
> > Likes:
> > 
> > _scratch_mkfs_sized 128M (maybe bigger ?)
> > _scratch_mount
> > 
> > > +
> > > +# fill the disk space
> > > +dd if=/dev/zero of=test/foo bs=1M count=128 > /dev/null 2>&1
> > Can _fill_fs() helper help you that?
> > 
> > > +
> > > +# create 1000 files, not all the files will be created successfully
> > > +mkdir test/dir
> > > +cd test/dir
> > > +for ((i = 0; i < 1000; i++))
> > > +do
> > > +	touch file$i > /dev/null 2>&1
> > > +done
> > I don't know if it can be 100% sure there's at least 1 file will be created at
> > here, after you tried to fill the fs.
> > 
> > So I think maybe you can create a file with known name, then use _fill_fs()
> > to fill the whole fs. Then try to rename the file you know its name. Can that
> > reproduce the bug?
> OK, I am not sure, let me have a try.
> > 
> > > +
> > > +# try to rename, but now no space left on device
> > > +$here/src/renameat2 -w $testdir/test/dir/file1 $testdir/test/dir/dst_file
> > > +
> > > +cd $testdir
> > > +$UMOUNT_PROG test
> > > +$MOUNT_PROG img test
> > > +ls -l test/dir/file1 > /dev/null 2>&1
> > > +$UMOUNT_PROG test
> > > +
> > > +dmesg -c | grep "deleted inode referenced"
> > Can _check_dmesg_for() help you?
> > 
> > > +# success, all done
> > > +status=0
> > > +exit
> > > diff --git a/tests/ext4/048.out b/tests/ext4/048.out
> > > new file mode 100644
> > > index 00000000..5b3990da
> > > --- /dev/null
> > > +++ b/tests/ext4/048.out
> > > @@ -0,0 +1,2 @@
> > > +QA output created by 048
> > > +No space left on device
> > > diff --git a/tests/ext4/group b/tests/ext4/group
> > > index ceda2ba6..6140dd7e 100644
> > > --- a/tests/ext4/group
> > > +++ b/tests/ext4/group
> > > @@ -50,6 +50,7 @@
> > >   045 auto dir
> > >   046 auto prealloc quick
> > >   047 auto quick dax
> > > +048 other
> > I think your test case is a general test case, "auto"and "rename" groups
> > are good to it, if it's quick enough, add "quick" group.
> 
> Thanks for your suggestion.
> 
> 
> Thanks,
> 
> Sun Ke
> 
> > 
> > >   271 auto rw quick
> > >   301 aio auto ioctl rw stress defrag
> > >   302 aio auto ioctl rw stress defrag
> > > -- 
> > > 2.13.6
> > > 
> > .
>