xfs
[Top] [All Lists]

Re: [PATCH RESEND 6/7] xfstest: Add test case to test multiple collapse

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH RESEND 6/7] xfstest: Add test case to test multiple collapse range call
From: Namjae Jeon <linkinjeon@xxxxxxxxx>
Date: Thu, 10 Oct 2013 19:20:43 +0900
Cc: viro@xxxxxxxxxxxxxxxxxx, mtk.manpages@xxxxxxxxx, tytso@xxxxxxx, adilger.kernel@xxxxxxxxx, bpm@xxxxxxx, elder@xxxxxxxxxx, hch@xxxxxxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, linux-ext4@xxxxxxxxxxxxxxx, xfs@xxxxxxxxxxx, a.sangwan@xxxxxxxxxxx, Namjae Jeon <namjae.jeon@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=vo5hj8B349hPuqtjZoz7mFLy1OuW8f6orVwMp0KO7eQ=; b=eM5Kpd3GeEFJm+AxiaRRqZ/mG0+JRpd8q1nRMrCg2ThgDhxBdgBsN96sFdaEym5ndV Dl/pGRlgZYvWfc2E8MLuQQ/copqhEs1B1EZ6gy9fiNkumQd6YnPUKo3Bx0EdB4aEtdG9 CyYhB+4lgQPbd2u7wvjqMo9OP36pMmzOXGVMaknvA2ig+8v6QUa7EreVsD308tam/IaJ IF5Hoa1HSPk5Zo6v2JgZp5i3CaCiQ7DwZ+7CP6Iypfo6QsxI2gGMwbRwMg1EdSs5mVY5 1RdSbZmCig8zF3Vl/gxGCrCPOUvBeH1SikAxqmYDEcDK9f7gF44A3mmk/W7k4GJN87Rb HUyw==
In-reply-to: <20131009235828.GR4446@dastard>
References: <1381090446-2897-1-git-send-email-linkinjeon@xxxxxxxxx> <20131009235828.GR4446@dastard>
>> +. ./common/rc
>> +. ./common/filter
>> +
>> +# real QA test starts here
>> +_supported_fs xfs ext4
>> +_supported_os Linux
>> +
>> +_require_scratch
>> +_require_xfs_io_fiemap
>> +_require_xfs_io_falloc_collapse
>> +_do_die_on_error=y
>> +test=$SCRATCH_MNT/test
>
> Not used.
Okay.

>
>> +testfile=$SCRATCH_MNT/317.$$
>> +BSIZE=4096
>> +BLOCKS=10240
>> +
>> +# Filters fiemap output
>> +_filter_fiemap()
>> +{
>> +     awk --posix '
>> +             $3 ~ /hole/ {
>> +                     print $1, $2, $3;
>> +                     next;
>> +             }
>> +             $5 ~ /0x[[:xdigit:]]+/ {
>> +                     print $1, $2, "extent";
>> +             }'
>> +}
>
> There's already a function in common/punch of this name, and it does
> pretty much the same thing. Why not use that?
Ah, Okay, I will check.

>
>> +
>> +case $FSTYP in
>> +     ext4)
>> +             export MKFS_OPTIONS="-F -b $BSIZE"
>> +             ;;
>> +     xfs)
>> +             export MKFS_OPTIONS="-f -b size=$BSIZE"
>> +             ;;
>> +esac
>
> _scratch_mkfs takes options on the command line - there is no need
> to do this.
Okay.

>
> In fact, this test needs to run on all block sizes that filesystems
> are capable of using, not just 4k and different architectures
> exercise different code paths and so we must be able to test the
> case where block size is smaller than page size on x86-64 so when
> the code is run on an ia64 or ppc64 box with a 64k page size we know
> that it's not completely broken...
Okay, I will update to test block size is smaller than page size.

>
> Anyway, if you really need to make a 4k block size filesystem, then
> _scratch_mkfs_sized() is the generic way of doing this.
>
>> +# make filesystem on scratch with 4KB blocksize
>> +_do 'make filesystem on $SCRATCH_DEV' '_scratch_mkfs'
>> +_do 'mount filesytem' '_scratch_mount'
>
> I really dislike this "_do" wrapper. The text does not add anything
> to the test, and it makes it hard to see the command being run and
> harder to modify it when necessary. It is used only by a couple of
> old tests, and we'd do better to remove it than to propagate it
> further.  This:
>
> _scratch_mkfs >> $seqres.full 2>&1 || _fail "scratch_mkfs failed."
> _scratch_mount >> $seqres.full 2>&1 || _fail "scratch_mount failed."
>
> does everything that the _do wrapper does.
Okay.

>
>> +
>> +# Write file
>> +length=$(($BLOCKS*$BSIZE))
>> +$XFS_IO_PROG -f -c "pwrite 0 $length" -c fsync $testfile > /dev/null
>> +
>> +# Collapse alternate blocks
>> +for (( i = 1; i <= 7; i++ )); do
>> +     for(( j=0 ; j < $(($BLOCKS/(2**$i))) ; j++ )); do
>> +             offset=$(($j*$BSIZE))
>> +             $XFS_IO_PROG -c "fcollapse $offset $BSIZE" $testfile > 
>> /dev/null
>> +     done
>> +done
>> +
>> +# Check if 80 extents are present
>> +$XFS_IO_PROG -c "fiemap -v" $testfile | _filter_fiemap
>
> If all you care about is that there are 80 extents, then why not
> just something like:
>
> $XFS_IO_PROG -c "fiemap -v" $testfile |grep "^ *[0-9]*:" |wc -l
Okay, I will check.

>
>> +
>> +_do 'unmount $SCRATCH_DEV' 'umount $SCRATCH_DEV'
>> +_do 'repair filesystem' '_check_scratch_fs'
>
> _check_scratch_fs is all you need to call here.
Yes, right. I will update.

>
>> index 3a69294..80ff7ec 100644
>> --- a/tests/shared/group
>> +++ b/tests/shared/group
>> @@ -12,3 +12,4 @@
>>  298 auto trim
>>  305 aio dangerous enospc rw stress
>>  316 auto quick collapse
>> +317 auto collapse
>
> Again, I think the prealloc group is better for this.
Okay, I will add collpase range cases to prealloc group.

Thanks for review.
I will post patches included your all review points soon.
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>