Re: [Qemu-devel] [PATCH v2 20/21] qemu-iotests: Test cache mode option inheritance

[Max Reitz]

On 23.11.2015 16:59, Kevin Wolf wrote:
> This is doing a more complete test on setting cache modes both while
> opening an image (i.e. in a -drive command line) and in reopen
> situations. It checks that reopen can specify options for child nodes
> and that cache modes are correctly inherited from parent nodes where
> they are not specified.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  tests/qemu-iotests/142     | 354 ++++++++++++++++++++
>  tests/qemu-iotests/142.out | 788 
> +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 1143 insertions(+)
>  create mode 100755 tests/qemu-iotests/142
>  create mode 100644 tests/qemu-iotests/142.out

Comments below, to make it short: With %s/bs->backing_hd/bs->backing/
and a comment about why you commented 'out | grep "Cache"':

Reviewed-by: Max Reitz <address@hidden>

> diff --git a/tests/qemu-iotests/142 b/tests/qemu-iotests/142
> new file mode 100755
> index 0000000..58daf26
> --- /dev/null
> +++ b/tests/qemu-iotests/142
> @@ -0,0 +1,354 @@
> +#!/bin/bash
> +#
> +# Test for configuring cache modes of arbitrary nodes (requires O_DIRECT)
> +#
> +# Copyright (C) 2015 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +# creator
> address@hidden
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1     # failure is the default!
> +
> +_cleanup()
> +{
> +    _cleanup_test_img
> +    rm -f $TEST_IMG.snap
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +_supported_fmt qcow2
> +_supported_proto file
> +_supported_os Linux
> +
> +# We test all cache modes anyway, but O_DIRECT needs to be supported
> +_default_cache_mode none
> +_supported_cache_modes none directsync
> +
> +function do_run_qemu()
> +{
> +    echo Testing: "$@"
> +    (
> +        if ! test -t 0; then
> +            while read cmd; do
> +                echo $cmd
> +            done
> +        fi
> +        echo quit
> +    ) | $QEMU -nographic -monitor stdio -nodefaults "$@"
> +    echo
> +}
> +
> +function run_qemu()
> +{
> +    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu
> +}
> +
> +size=128M
> +
> +TEST_IMG="$TEST_IMG.base" _make_test_img $size
> +TEST_IMG="$TEST_IMG.snap" _make_test_img $size
> +_make_test_img -b "$TEST_IMG.base" $size
> +
> +echo
> +echo === Simple test for all cache modes ===
> +echo
> +
> +run_qemu -drive file="$TEST_IMG",cache=none
> +run_qemu -drive file="$TEST_IMG",cache=directsync
> +run_qemu -drive file="$TEST_IMG",cache=writeback
> +run_qemu -drive file="$TEST_IMG",cache=writethrough
> +run_qemu -drive file="$TEST_IMG",cache=unsafe
> +run_qemu -drive file="$TEST_IMG",cache=invalid_value
> +
> +echo
> +echo === Check inheritance of cache modes ===
> +echo
> +
> +files="if=none,file=$TEST_IMG,backing.file.filename=$TEST_IMG.base"
> +ids="node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file"
> +
> +function check_cache_all()
> +{
> +    # cache.direct is supposed to be inherited by both bs->file and
> +    # bs->backing_hd

%s/bs->backing_hd/bs->backing/g

> +
> +    echo -e "cache.direct=on on none0"
> +    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",cache.direct=on | 
> grep "Cache"
> +    echo -e "\ncache.direct=on on file"
> +    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",file.cache.direct=on 
> | grep "Cache"
> +    echo -e "\ncache.direct=on on backing"
> +    echo "$hmp_cmds" | run_qemu -drive 
> "$files","$ids",backing.cache.direct=on | grep "Cache"
> +    echo -e "\ncache.direct=on on backing-file"
> +    echo "$hmp_cmds" | run_qemu -drive 
> "$files","$ids",backing.file.cache.direct=on | grep "Cache"
> +
> +    # cache.writeback is supposed to be inherited by bs->backing_hd; bs->file
> +    # always gets cache.writeback=on

I don't know whether having the backing file inherit cache.writeback
makes any sense, but I guess it's the default behavior and overriding it
wouldn't make any sense either.

> +    echo -e "\n\ncache.writeback=off on none0"
> +    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",cache.writeback=off | 
> grep "Cache"
> +    echo -e "\ncache.writeback=off on file"
> +    echo "$hmp_cmds" | run_qemu -drive 
> "$files","$ids",file.cache.writeback=off | grep "Cache"
> +    echo -e "\ncache.writeback=off on backing"
> +    echo "$hmp_cmds" | run_qemu -drive 
> "$files","$ids",backing.cache.writeback=off | grep "Cache"
> +    echo -e "\ncache.writeback=off on backing-file"
> +    echo "$hmp_cmds" | run_qemu -drive 
> "$files","$ids",backing.file.cache.writeback=off | grep "Cache"
> +
> +    # cache.no-flush is supposed to be inherited by both bs->file and 
> bs->backing_hd
> +
> +    echo -e "\n\ncache.no-flush=on on none0"
> +    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",cache.no-flush=on | 
> grep "Cache"
> +    echo -e "\ncache.no-flush=on on file"
> +    echo "$hmp_cmds" | run_qemu -drive 
> "$files","$ids",file.cache.no-flush=on | grep "Cache"
> +    echo -e "\ncache.no-flush=on on backing"
> +    echo "$hmp_cmds" | run_qemu -drive 
> "$files","$ids",backing.cache.no-flush=on | grep "Cache"
> +    echo -e "\ncache.no-flush=on on backing-file"
> +    echo "$hmp_cmds" | run_qemu -drive 
> "$files","$ids",backing.file.cache.no-flush=on | grep "Cache"
> +}

[...]

> +echo
> +echo "--- Change cache mode in parent, child has explicit option in JSON ---"
> +echo
> +
> +# This checks that children with options explicitly set by the json:
> +# pseudo-protocol don't inherit these options from their parents.
> +#
> +# Yes, blkdebug::json:... is criminal, but I can't see another way to have a
> +# BDS initialised with the json: pseudo-protocol, but still have it inherit
> +# options from its parent node.

I'd suggest a json:{} backing file name (stored in the overlay, and thus
implicitly opened). This is what we originally intended to have json:{}
for, after all. ;-)

> +hmp_cmds="qemu-io none0 \"reopen -o 
> cache.writeback=off,cache.direct=on,cache.no-flush=on\"
> +info block image
> +info block blkdebug
> +info block file"
> +
> +echo "$hmp_cmds" | run_qemu -drive 
> if=none,file="blkdebug::json:{\"filename\":\"$TEST_IMG\",,\"cache\":{\"writeback\":false,,\"direct\":false}}",node-name=image,file.node-name=blkdebug,file.image.node-name=file
>  #| grep "Cache"

Why is the grep commented out? If it wasn't, I wouldn't have had to fix
my patch fix script (see below)...

[...]

> diff --git a/tests/qemu-iotests/142.out b/tests/qemu-iotests/142.out
> new file mode 100644
> index 0000000..c141fb8
> --- /dev/null
> +++ b/tests/qemu-iotests/142.out
> @@ -0,0 +1,788 @@

[...]

> +
> +--- Change cache mode in parent, child has explicit option in JSON ---
> +
> +Testing: -drive 
> if=none,file=blkdebug::json:{"filename":"TEST_DIR/t.qcow2",,"cache":{"writeback":false,,"direct":false}},node-name=image,file.node-name=blkdebug,file.image.node-name=file
> +QEMU X.Y.Z monitor - type 'help' for more information
> +(qemu) 
> qqeqemqemuqemu-qemu-iqemu-ioqemu-io
>  qemu-io nqemu-io 
> noqemu-io 
> nonqemu-io 
> noneqemu-io 
> none0qemu-io none0 
> qemu-io none0 
> "qemu-io none0 
> "rqemu-io none0 
> "reqemu-io none0 
> "reoqemu-io none0 
> "reopqemu-io 
> none0 
> "reopeqemu-io 
> none0 "reopen!
>  qemu-io none0 "reopen 
> qemu-io 
> none0 "reopen 
> -qemu-io
>  none0 "reopen 
> -oqemu-io
>  none0 "reopen -o 
> qemu-io
>  none0 "reopen -o 
> cqemu-io
>  none0 "reopen -o 
> caqemu-io
>  none0 "reopen -o 
> cacqemu-io
>  none0 "reopen -o 
> cachqemu-io
>  none0 "reopen -o 
> cacheqemu-io
>  none0 "reop!
>  en -o 
> cache.qemu-io
>  none0 "reopen -o 
> cache.wqemu-io
>  none0 "reopen -o 
> cache.wrqemu-io
>  none0 "reopen -o 
> cache.wriqemu-io
>  none0 "reopen -o 
> cache.writqemu-io
>  none0 "reopen -o 
> cache.writeqemu-io
>  none0 "reopen -o 
> cache.writebqemu-io
>  none0 "reopen -o cache.writeba
[
>  D!
>  [Dqemu-io none0 "reopen -o 
> cache.writebacqemu-io
>  none0 "reopen -o 
> cache.writebackqemu-io
>  none0 "reopen -o 
> cache.writeback=qemu-io
>  none0 "reopen -o 
> cache.writeback=oqemu-io
>  none0 "reopen -o 
> cache.writeback=ofqemu-io
>  none0 "reopen -o 
> cache.writeback=off[!
>  
> Dqemu-io
>  none0 "reopen -o 
> cache.writeback=off,qemu-io
>  none0 "reopen -o 
> cache.writeback=off,cqemu-io
>  none0 "reopen -o 
> cache.writeback=off,caqemu-io
>  none0 "reopen -o 
> cache.writeback=off,cacqemu-io
>  none0 "reopen -o 
> cache.writeback=off,cach
[
>  Dqemu-io none0 "reopen -o cac!
>  
> he.writeback=off,cacheqemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.qemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.dqemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.diqemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.dir!
>  qemu-io none0 "reopen -o 
> cache.writeback=off,cache.direqemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.direcqemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.directqemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.direct=qemu-io
>  none0 "reopen -o cache.writeback=
o
>  ff,cache.direct=o!
>  
> qemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.direct=onqemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.direct=on,qemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.direct=on,cqemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.direct=on,ca!
>  
> [Dqemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.direct=on,cacqemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.direct=on,cachqemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.direct=on,cacheqemu-io
>  none0 "reopen -o cache.writeback=off,cache.direct=on,cac
h
>  e.!
>  
> qemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.direct=on,cache.nqemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.direct=on,cache.noqemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.direct=on,cache.no-qemu-io!
>   none0 "reopen -o 
> cache.writeback=off,cache.direct=on,cache.no-fqemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.direct=on,cache.no-flqemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.direct=on,cache.no-fluqemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.direct=on,cache.no-flus[
D
>  [!
>  
> Dqemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.direct=on,cache.no-flushqemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.direct=on,cache.no-flush=qemu-io
>  none0 "reopen -o 
> cache.writeback=off,cache.direct=on,cache.no-flush=o!
>  qemu-io none0 "reopen -o 
> cache.writeback=off,cache.direct=on,cache.no-flush=onqemu-io
>  none0 "reopen -o cache.writeback=off,cache.direct=on,cache.no-flush=on"

Congratulations, you win the "Patch has a line so long it breaks my
large-patch-fixer.rb" prize. Or, alternatively, the "Patch has a line so
long with so many special characters it breaks git's per-line character
counter" (I think).

Some split lines don't end in an exclamation mark (probably because they
are so long), so my script doesn't know whether to merge them or not,
and others have an exclamation mark at the end (and need to be merged)
but have actually less than 78 characters.

(Apparently, git sometimes generates lines with 998 characters and then
cannot insert a ! anymore (because CRLF alone takes 2 bytes). The lines
ending in ! have 990 characters. So I guess it's a bug in git. And the
fact that it's always lines following a 998 character line that are
actually too short to be merged makes it look like git didn't even
notice it split a line there, so somehow its counting seems to be off.)

((Based on the 998-character information, I was now able to amend my
large-patch-fixer.rb so it is able to reconstruct this line.))

Max

> +(qemu) iininfinfoinfo 
> info binfo 
> blinfo bloinfo 
> blocinfo 
> blockinfo block 
> info block 
> iinfo block 
> iminfo block 
> imainfo block 
> imaginfo block image
> +
> +image: blkdebug::TEST_DIR/t.qcow2 (qcow2)
> +    Cache mode:       writethrough, direct, ignore flushes
> +    Backing file:     TEST_DIR/t.qcow2.base (chain depth: 1)
> +(qemu) iininfinfoinfo 
> info binfo 
> blinfo bloinfo 
> blocinfo 
> blockinfo block 
> info block 
> binfo block 
> blinfo block 
> blkinfo block 
> blkdinfo block 
> blkdeinfo block 
> blkdebinfo block 
> blkdebuinfo block 
> blkdebug
> +
> +blkdebug: blkdebug::TEST_DIR/t.qcow2 (blkdebug)
> +    Cache mode:       writeback, direct, ignore flushes
> +(qemu) iininfinfoinfo 
> info binfo 
> blinfo bloinfo 
> blocinfo 
> blockinfo block 
> info block 
> finfo block 
> fiinfo block 
> filinfo block file
> +
> +file: TEST_DIR/t.qcow2 (file)
> +    Cache mode:       writethrough, ignore flushes
> +(qemu) qququiquit

signature.asc
Description: OpenPGP digital signature