xfs
[Top] [All Lists]

Re: [PATCH RESEND 5/7] xfstest: Add test case to check various corner ca

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH RESEND 5/7] xfstest: Add test case to check various corner cases for collapsing range
From: Namjae Jeon <linkinjeon@xxxxxxxxx>
Date: Thu, 10 Oct 2013 19:01:01 +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=Vg18ZhD502czGzn5jhmJclcBuz5nLlJZIhkQ6Ucg5/k=; b=bNiJ1Lsg0IiElt/VZdy6x2y7Qc8/iQZ1T/3L2cVBFgCehqwM/BQRCW34PGBKUUDdsG mlGjPy62KTGPQgjP6fgNFFi+s1sNAn1vlCud8p2pEZFoVBZxoV5Sns4eQDcj7o1srq1S nqiRK5jzaXjCqaDal43K369O8EMMYnC3UD7VeKT/ZQ/w3yb9Zx9kX2QfYq5XMR6Yjm+D TgOdS/pairoZfOdfO771wUah+YlBLBcwjWkmEBx+LOatIAkqI3eAZjtfG/XoPnwtwFLZ T9yGK6bnuQnGfyMPI6cT4W35TiMmaHYDJ02vl45SStZeP+xKRS1z7IvoESWeVY2sJbu1 or5g==
In-reply-to: <20131009233421.GQ4446@dastard>
References: <1381090432-2863-1-git-send-email-linkinjeon@xxxxxxxxx> <20131009233421.GQ4446@dastard>
>> +#
>> +_test_generic_collapse()
>> +{
>
> This function is just a copy and paste of _test_generic_punch() with
> all the ranges increased by a factor of 4. Why can't you simply use
> _test_generic_punch() and pass in a different $zero_cmd?
Yes, you're right. We can use test_generic_punch as you said.
I will update.
>
>
>>
>> +# check that xfs_io, kernel and filesystem all support fallocate with 
>> collapse
>> +# range
>> +_require_xfs_io_falloc_collapse()
>> +{
>> +     testfile=$TEST_DIR/$$.falloc
>> +     testio=`$XFS_IO_PROG -F -f -c "pwrite 0 20k" -c "fsync" \
>> +             -c "fcollapse 4k 8k" $testfile 2>&1`
>
> No need for the -F parameter anymore.
Okay.

>
>> +     rm -f $testfile 2>&1 > /dev/null
>> +     echo $testio | grep -q "not found" && \
>> +             _notrun "xfs_io fallocate collapse range support is missing"
>> +     echo $testio | grep -q "Operation not supported" && \
>> +             _notrun "xfs_io fallocate collapse range failed (no fs 
>> support?)"
>> +}
>> +
>>  # check that xfs_io, kernel and filesystem support fiemap
>>  _require_xfs_io_fiemap()
>>  {
>> diff --git a/tests/shared/316 b/tests/shared/316
>> new file mode 100644
>> index 0000000..66a8489
>> --- /dev/null
>> +++ b/tests/shared/316
>
> You don't need to number this 316. shared/001 is not taken, so start
> there....
okay.

>
>> @@ -0,0 +1,70 @@
>> +#! /bin/bash
>> +# FS QA Test No. 316
>> +#
>> +# Test fallocate collapse range
>
> A more verbose test description is preferred. A paragraph describing
> that the test exercises boundary conditions across different extent
> types for the FALLOC_FL_COLLAPSE_RANGE operation would be ideal.
Okay.

>
>> +trap "_cleanup ; exit \$status" 0 1 2 3 15
>> +
>> +# get standard environment, filters and checks
>> +# we need to include common/punch to get defination fo filter functions
>> +. ./common/rc
>> +. ./common/filter
>> +. ./common/punch
>> +. ./common/collapse
>> +
>> +# real QA test starts here
>> +_supported_fs xfs ext4
>> +_supported_os Linux
>> +
>> +_require_xfs_io_falloc_punch
>> +_require_xfs_io_falloc
>> +_require_xfs_io_fiemap
>> +_require_xfs_io_falloc_collapse
>> +
>> +testfile=$TEST_DIR/316.$$
>
> If you are going to use the test number to identify the file, you
> should use $seq rather than hard coding the number....
Okay.

>
>> +# Standard collapse range tests
>> +_test_generic_collapse falloc fcollapse fpunch fiemap _filter_hole_fiemap 
>> $testfile
>> +
>> +# Delayed allocation collapse range tests
>> +_test_generic_collapse -d falloc fcollapse fpunch fiemap 
>> _filter_hole_fiemap $testfile
>> +
>> +# Multi collapse tests
>> +_test_generic_collapse -k falloc fcollapse fpunch fiemap 
>> _filter_hole_fiemap $testfile
>> +
>> +# Delayed allocation multi collapse range tests
>> +_test_generic_collapse -d -k falloc fcollapse fpunch fiemap 
>> _filter_hole_fiemap $testfile
>
> What I'd prefer is each of these is a separate unit test. i.e:
>
> shared/001: Standard collapse range tests
> shared/002: Delayed allocation collapse range tests
> shared/003: Multi collapse tests
> shared/004: Delayed allocation multi collapse range tests
>
> The reason for doing this is that it means that:
>
>         a) we can track failures of the different types of tests
>            independently; and
>         b) the filesystem the tests are being run on is checked for
>            consistency between each test
>
> I know, the punch tests you copied this from lump them all into the
> one test, but I'd really like to get away from the complex tests
> that aggregate lots of different things into a single pass/fail test
> as it makes it hard to isolate failures and track them over the long
> term...
>
Okay, I will do it as your suggestion.

>> +
>> +status=0 ; exit
>
> separate lines
>
>> diff --git a/tests/shared/group b/tests/shared/group
>> index 0ad640b..3a69294 100644
>> --- a/tests/shared/group
>> +++ b/tests/shared/group
>> @@ -11,4 +11,4 @@
>>  289 auto quick
>>  298 auto trim
>>  305 aio dangerous enospc rw stress
>> -
>> +316 auto quick collapse
>
> I'd put this in the prealloc group rather than create a new one
> called "collapse".
Okay.

Thanks for review.
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx

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