xfs
[Top] [All Lists]

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

To: Namjae Jeon <linkinjeon@xxxxxxxxx>
Subject: Re: [PATCH RESEND 5/10] xfstest: shared/001: Standard collapse range tests
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 4 Feb 2014 10:17:37 +1100
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
In-reply-to: <1391319958-3275-1-git-send-email-linkinjeon@xxxxxxxxx>
References: <1391319958-3275-1-git-send-email-linkinjeon@xxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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

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....

>               -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....

> +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.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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