Re: [PATCH RESEND 6/7] xfstest: Add test case to test multiplecollapse range call

[Namjae Jeon]

>> +. ./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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/