xfs
[Top] [All Lists]

Re: [PATCH RESEND 5/10] xfstest: shared/001: Standard collapse range tes

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH RESEND 5/10] xfstest: shared/001: Standard collapse range tests
From: Namjae Jeon <linkinjeon@xxxxxxxxx>
Date: Tue, 4 Feb 2014 12:49:33 +0900
Cc: viro@xxxxxxxxxxxxxxxxxx, bpm@xxxxxxx, tytso@xxxxxxx, adilger.kernel@xxxxxxxxx, jack@xxxxxxx, mtk.manpages@xxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx, xfs@xxxxxxxxxxx, linux-ext4@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, Namjae Jeon <namjae.jeon@xxxxxxxxxxx>, Ashish Sangwan <a.sangwan@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=2t+9bKGMqwaS5z2Bv3a04eivRhZtLBD2H+eGLhtJJEA=; b=K+/j/44VqCSwGL6CduV0lbUCbruvAjBpHG0L5GeTGfWTNz7v31Erl6oefDKUA9oG+W YSolA8w792La2XgwkQ5t2/JkBViNfCKFdHIna1NCVncPgED8LiQ7mUeo1HoeebatBkyu QBnYxx3iEuVnBQcgn0WfgxvzxjpMM4kC2Y24qoVqBpRYuAkNpROja26VzqthXyBUMYjb USU22DtIbMOHRPQbubDW971rOqorwitidyHJDfBQH/sScq6HiZy2FJejUPOd+Rj9tYVb WVIZ9C7YItFznXLFRsjxc5e53kLzw6jCuDzV7QDxSRJkPU6PnYJpQwdU+00nDsj79LhY bK/g==
In-reply-to: <20140203231737.GV13997@dastard>
References: <1391319958-3275-1-git-send-email-linkinjeon@xxxxxxxxx> <20140203231737.GV13997@dastard>
2014-02-04, Dave Chinner <david@xxxxxxxxxxxxx>:
> On Sun, Feb 02, 2014 at 02:45:58PM +0900, Namjae Jeon wrote:
>> From: Namjae Jeon <namjae.jeon@xxxxxxxxxxx>
>>
>> This testcase(001) tries to test various corner cases
>> for fcollapse range functionality over different type of extents.
>>
>> Signed-off-by: Namjae Jeon <namjae.jeon@xxxxxxxxxxx>
>> Signed-off-by: Ashish Sangwan <a.sangwan@xxxxxxxxxxx>
>
> Couple of things:
>
>>                      -c "$map_cmd -v" $testfile | $filter_cmd
>>              [ $? -ne 0 ] && die_now
>>              _md5_checksum $testfile
>> @@ -415,10 +425,10 @@ _test_generic_punch()
>>              if [ "$remove_testfile" ]; then
>>                      rm -f $testfile
>>              fi
>> -            $XFS_IO_PROG -f -c "truncate 20k" \
>> -                    -c "$alloc_cmd 0 8k" \
>> -                    -c "pwrite 8k 8k" $sync_cmd \
>> -                    -c "$zero_cmd 4k 8k" \
>> +            $XFS_IO_PROG -f -c "truncate $(($multiple * 20))k" \
>> +                    -c "$alloc_cmd 0 $(($multiple * 8))k" \
>> +                    -c "pwrite $(($multiple * 8))k $(($multiple * 8))k" 
>> $sync_cmd \
>> +                    -c "$zero_cmd $(($multiple * 4))k $(($multiple * 8))k" \
>>                      -c "$map_cmd -v" $testfile | $filter_cmd
>
Hi. Dave.
> This is unreadable, and therefore I'd consider that these changes
> render _test_generic_punch unmaintainable.
>
> Either it needs tobe factored to be more readable, or we need a more
> readable way of representing the offsets and sizes if we want them
> to be variable. For example:
>
> _4k="$((multiple * 4))k"
> _8k="$((multiple * 8))k"
> _20k="$((multiple * 20))k"
>
> leads to:
>
>               $XFS_IO_PROG -f -c "truncate $_20k" \
>                       -c "$alloc_cmd 0 $_8k" \
>                       -c "pwrite $_8k $_8k" $sync_cmd \
>                       -c "$zero_cmd $_4k $_8k" \
>                       -c "$map_cmd -v" $testfile | $filter_cmd
>
> which is still readable and allows us to arbitrarily scale the
> parameters. It even allows us to handle different filesystem block
> sizes if we really want to....
Okay, I will change it as you suggest.
>
>>              -c "$map_cmd -v" $testfile | $filter_cmd
>>      [ $? -ne 0 ] && die_now
>>      _md5_checksum $testfile
>>
>> +    # If zero_cmd is fcollpase, don't check unaligned offsets
>> +    if [ "$zero_cmd" == "fcollapse" ]; then
>> +            if [ "$remove_testfile" ]; then
>> +                    rm -f $testfile
>> +                    rm -f $testfile.2
>> +            fi
>> +            return
>> +    fi
>
> No need to remove the test files here - we remove them at
> test startup to ensure we have a known initial state....
Okay.
>
>> +0: [0..63]: extent
>> +bb7df04e1b0a2570657527a7e108ae23
>> +    13. data -> unwritten -> data
>> +0: [0..63]: extent
>> +0f0151cbed83e4bf6e5bde26e82ab115
>> +    14. data -> hole @ EOF
>> +fallocate: Invalid argument
>> +0: [0..159]: extent
>
> This error appears in all the golden outputs. If it's correct, then
> perhaps it should be filtered out or commented somewhere to explain
> why it is expected.
Okay, I will add the comments to explain about this.

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

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