[Qemu-devel] [PATCH v3 0/6] Add cache mode option to qemu-iotests, and change default mode to "writeback"

[Qemu-devel] [PATCH v3 0/6] Add cache mode option to qemu-iotests, and change default mode to "writeback"

[Fam Zheng]

This series adds cache mode option in the iotests framework. Test cases are
updated to make use of cache mode and mask supported modes.

v3: Change _unsupported_qemu_io_options to _supported_cache_modes.
    Change default mode to "writeback".
    Clean up some whitespaces in the end of series.
    Fix "026.out.nocache" case.
    Fix 048 case on tmpfs.


Fam Zheng (6):
  qemu-iotests: Add "-c <cache-mode>" option
  qemu-iotests: Honour cache mode in iotests.py
  qemu-iotests: Add _supported_cache_modes
  qemu-iotests: Change default cache mode to "writeback"
  qemu-iotests: Force qcow2 in error path test in 048
  qemu-iotests: Clean up spaces in usage output

 tests/qemu-iotests/026        |  2 +-
 tests/qemu-iotests/039        |  2 +-
 tests/qemu-iotests/048        |  8 +++++++-
 tests/qemu-iotests/052        |  3 +--
 tests/qemu-iotests/check      |  2 +-
 tests/qemu-iotests/common     | 37 +++++++++++++++++++++++++++----------
 tests/qemu-iotests/common.rc  | 20 ++++++++++----------
 tests/qemu-iotests/iotests.py |  3 ++-
 8 files changed, 50 insertions(+), 27 deletions(-)

-- 
1.8.4.2

[Qemu-devel] [PATCH v3 1/6] qemu-iotests: Add "-c <cache-mode>" option

[Fam Zheng]

The option sets cache mode used in the tests. "-nocache" is changed to
an alias to "-c none", and internally passes "-t none" to qemu-io.

Python scripts will make use of option this in the next commit.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/check  |  2 +-
 tests/qemu-iotests/common | 21 +++++++++++++++++++--
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index f5f328f..dc0105c 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -242,7 +242,7 @@ do
             fi
 
             reference=$seq.out
-            if (echo $QEMU_IO_OPTIONS | grep -s -- '--nocache' > /dev/null); then
+            if [ "$CACHEMODE" = "none" ]; then
                 [ -f $seq.out.nocache ] && reference=$seq.out.nocache
             fi
 
diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
index 8cde7f1..e25e13b 100644
--- a/tests/qemu-iotests/common
+++ b/tests/qemu-iotests/common
@@ -42,13 +42,16 @@ expunge=true
 have_test_arg=false
 randomize=false
 valgrind=false
+cachemode=false
 rm -f $tmp.list $tmp.tmp $tmp.sed
 
 export IMGFMT=raw
 export IMGFMT_GENERIC=true
 export IMGPROTO=file
 export IMGOPTS=""
+export CACHEMODE="writethrough"
 export QEMU_IO_OPTIONS=""
+export CACHEMODE_IS_DEFAULT=true
 
 for r
 do
@@ -113,7 +116,12 @@ s/ .*//p
         IMGOPTS="$r"
         imgopts=false
         continue
-
+    elif $cachemode
+    then
+        CACHEMODE="$r"
+        CACHEMODE_IS_DEFAULT=false
+        cachemode=false
+        continue
     fi
 
     xpand=true
@@ -147,6 +155,7 @@ check options
     -o options          -o options to pass to qemu-img create/convert
     -T                        output timestamps
     -r                         randomize test order
+    -c                  cache mode
 
 testlist options
     -g group[,group...]        include tests from these groups
@@ -219,7 +228,8 @@ testlist options
             xpand=false
             ;;
         -nocache)
-            QEMU_IO_OPTIONS="$QEMU_IO_OPTIONS --nocache"
+            CACHEMODE="none"
+            CACHEMODE_IS_DEFAULT=false
             xpand=false
             ;;
 
@@ -258,6 +268,10 @@ testlist options
             imgopts=true
             xpand=false
             ;;
+        -c)
+            cachemode=true
+            xpand=false
+            ;;
         -r)        # randomize test order
             randomize=true
             xpand=false
@@ -334,6 +348,9 @@ BEGIN        { for (t='$start'; t<='$end'; t++) printf "%03d\n",t }' \
 
 done
 
+# Set qemu-io cache mode with $CACHEMODE we have
+QEMU_IO_OPTIONS="$QEMU_IO_OPTIONS -t $CACHEMODE"
+
 # Set default options for qemu-img create -o if they were not specified
 _set_default_imgopts
 
-- 
1.8.4.2

[Qemu-devel] [PATCH v3 2/6] qemu-iotests: Honour cache mode in iotests.py

[Fam Zheng]

This will allow overriding cache mode from the "-c mode" option.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/iotests.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index fb10ff4..c84a1a5 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -37,6 +37,7 @@ qemu_args = os.environ.get('QEMU', 'qemu').strip().split(' ')
 imgfmt = os.environ.get('IMGFMT', 'raw')
 imgproto = os.environ.get('IMGPROTO', 'file')
 test_dir = os.environ.get('TEST_DIR', '/var/tmp')
+cachemode = os.environ.get('CACHEMODE')
 
 socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper')
 
@@ -96,7 +97,7 @@ class VM(object):
         '''Add a virtio-blk drive to the VM'''
         options = ['if=virtio',
                    'format=%s' % imgfmt,
-                   'cache=none',
+                   'cache=%s' % cachemode,
                    'file=%s' % path,
                    'id=drive%d' % self._num_drives]
         if opts:
-- 
1.8.4.2

[Qemu-devel] [PATCH v3 3/6] qemu-iotests: Add _supported_cache_modes

[Fam Zheng]

This replaces _unsupported_qemu_io_options and check for support of
current cache mode.

If user dosen't give "-c <mode>" or "-nocache", the first supported
cache mode is used in qemu-io.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/026       |  2 +-
 tests/qemu-iotests/039       |  2 +-
 tests/qemu-iotests/052       |  3 +--
 tests/qemu-iotests/common.rc | 20 ++++++++++----------
 4 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/tests/qemu-iotests/026 b/tests/qemu-iotests/026
index ebe29d0..9078cd8 100755
--- a/tests/qemu-iotests/026
+++ b/tests/qemu-iotests/026
@@ -44,7 +44,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto generic
 _supported_os Linux
-
+_supported_cache_modes "writethrough" "none"
 
 echo "Errors while writing 128 kB"
 echo
diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
index 8bade92..a298b50 100755
--- a/tests/qemu-iotests/039
+++ b/tests/qemu-iotests/039
@@ -44,7 +44,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto generic
 _supported_os Linux
-_unsupported_qemu_io_options --nocache
+_supported_cache_modes "writethrough"
 
 size=128M
 
diff --git a/tests/qemu-iotests/052 b/tests/qemu-iotests/052
index f5f9683..6d96c99 100755
--- a/tests/qemu-iotests/052
+++ b/tests/qemu-iotests/052
@@ -41,8 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt generic
 _supported_proto generic
 _supported_os Linux
-_unsupported_qemu_io_options --nocache
-
+_supported_cache_modes "writethrough"
 
 size=128M
 _make_test_img $size
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 7f62457..15d5bbd 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -387,18 +387,18 @@ _supported_os()
     _notrun "not suitable for this OS: $HOSTOS"
 }
 
-_unsupported_qemu_io_options()
+_supported_cache_modes()
 {
-    for bad_opt
-    do
-        for opt in $QEMU_IO_OPTIONS
-        do
-            if [ "$bad_opt" = "$opt" ]
-            then
-                _notrun "not suitable for qemu-io option: $bad_opt"
-            fi
-        done
+    if $CACHEMODE_IS_DEFAULT; then
+        QEMU_IO="$QEMU_IO -t $1"
+        return
+    fi
+    for mode; do
+        if [ "$mode" = "$CACHEMODE" ]; then
+            return
+        fi
     done
+    _notrun "not suitable for cache mode: $CACHEMODE"
 }
 
 # this test requires that a specified command (executable) exists
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v3 3/6] qemu-iotests: Add _supported_cache_modes

[Stefan Hajnoczi]

On Wed, Nov 20, 2013 at 03:44:14PM +0800, Fam Zheng wrote:
> -_unsupported_qemu_io_options()
> +_supported_cache_modes()
>  {
> -    for bad_opt
> -    do
> -        for opt in $QEMU_IO_OPTIONS
> -        do
> -            if [ "$bad_opt" = "$opt" ]
> -            then
> -                _notrun "not suitable for qemu-io option: $bad_opt"
> -            fi
> -        done
> +    if $CACHEMODE_IS_DEFAULT; then
> +        QEMU_IO="$QEMU_IO -t $1"
> +        return
> +    fi
> +    for mode; do
> +        if [ "$mode" = "$CACHEMODE" ]; then
> +            return
> +        fi
>      done
> +    _notrun "not suitable for cache mode: $CACHEMODE"
>  }

This seems weird to me:
By default tests run with CACHEMODE=writethrough but test cases can use
_supported_cache_modes() to switch to a different "default" behind the
scenes?

Why not keep it simple:
If a test doesn't support CACHEMODE, it gets skipped.

Stefan

Re: [Qemu-devel] [PATCH v3 3/6] qemu-iotests: Add _supported_cache_modes

[Fam Zheng]

On 2013年11月21日 20:39, Stefan Hajnoczi wrote:
> On Wed, Nov 20, 2013 at 03:44:14PM +0800, Fam Zheng wrote:
>> -_unsupported_qemu_io_options()
>> +_supported_cache_modes()
>>   {
>> -    for bad_opt
>> -    do
>> -        for opt in $QEMU_IO_OPTIONS
>> -        do
>> -            if [ "$bad_opt" = "$opt" ]
>> -            then
>> -                _notrun "not suitable for qemu-io option: $bad_opt"
>> -            fi
>> -        done
>> +    if $CACHEMODE_IS_DEFAULT; then
>> +        QEMU_IO="$QEMU_IO -t $1"
>> +        return
>> +    fi
>> +    for mode; do
>> +        if [ "$mode" = "$CACHEMODE" ]; then
>> +            return
>> +        fi
>>       done
>> +    _notrun "not suitable for cache mode: $CACHEMODE"
>>   }
>
> This seems weird to me:
> By default tests run with CACHEMODE=writethrough but test cases can use
> _supported_cache_modes() to switch to a different "default" behind the
> scenes?
>
> Why not keep it simple:
> If a test doesn't support CACHEMODE, it gets skipped.
>

I thought Kevin wanted to override the cache mode (if not given by 
user), so the cases doesn't get skipped by default. I also feel that a 
test case used to actually run by default shouldn't be skipped unnoticed.

I'm basically adding a meta cache mode "default" here, which means we 
will not have a "implicit yet forced" cache mode any more.

On 2013年11月19日 18:21, Kevin Wolf wrote:
 >
 > Okay, I gave it some testing, too, and it looks like we need some
 > additional changes. There are some test cases that use:
 >
 > _unsupported_qemu_io_options --nocache
 >
 > Which obviously doesn't work any more. We need to replace it by a check
 > against $CACHEMODE (or, perhaps preferably, even override it
 > automatically, so that test cases aren't left out just because of the
 > cache mode)
 >
 > Test case 026 uses the option of having a 026.out.nocache, which differs
 > from the normal output. I suspect the correct differentiation is here
 > between writethrough and writeback modes. And of course, grepping for
 > '--nocache' to detect the condition doesn't work any more.
 >
 > Kevin
 >

Fam

[Qemu-devel] [PATCH v3 4/6] qemu-iotests: Change default cache mode to "writeback"

[Fam Zheng]

So that the tests can run faster.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/common | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
index e25e13b..7db6bde 100644
--- a/tests/qemu-iotests/common
+++ b/tests/qemu-iotests/common
@@ -49,7 +49,7 @@ export IMGFMT=raw
 export IMGFMT_GENERIC=true
 export IMGPROTO=file
 export IMGOPTS=""
-export CACHEMODE="writethrough"
+export CACHEMODE="writeback"
 export QEMU_IO_OPTIONS=""
 export CACHEMODE_IS_DEFAULT=true
 
-- 
1.8.4.2

[Qemu-devel] [PATCH v3 5/6] qemu-iotests: Force qcow2 in error path test in 048

[Fam Zheng]

The "raw" doesn't always work on certain file systems (e.g. tmpfs). Use
qcow2 to make the allocation status explicit.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/048 | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/048 b/tests/qemu-iotests/048
index 9def7fc..0fb5d2c 100755
--- a/tests/qemu-iotests/048
+++ b/tests/qemu-iotests/048
@@ -88,7 +88,13 @@ event = "read_aio"
 errno = "5"
 once ="off"
 EOF
-_make_test_img $size
+if [ "$IMGFMT" = "raw" ]; then
+    # For raw the allocation status for new image depends on file system, force
+    # qcow2 in this case, since the tested code is in block layer
+    IMGFMT=qcow2 _make_test_img $size | _filter_imgfmt
+else
+    _make_test_img $size | _filter_imgfmt
+fi
 cp "$TEST_IMG" "$TEST_IMG2"
 io_pattern write 512 512 0 1 102
 TEST_IMG="blkdebug:$TEST_DIR/blkdebug.conf:$TEST_IMG" _compare 2>&1 |\
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v3 5/6] qemu-iotests: Force qcow2 in error path test in 048

[Stefan Hajnoczi]

On Wed, Nov 20, 2013 at 03:44:16PM +0800, Fam Zheng wrote:
> The "raw" doesn't always work on certain file systems (e.g. tmpfs). Use
> qcow2 to make the allocation status explicit.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/048 | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/048 b/tests/qemu-iotests/048
> index 9def7fc..0fb5d2c 100755
> --- a/tests/qemu-iotests/048
> +++ b/tests/qemu-iotests/048
> @@ -88,7 +88,13 @@ event = "read_aio"
>  errno = "5"
>  once ="off"
>  EOF
> -_make_test_img $size
> +if [ "$IMGFMT" = "raw" ]; then
> +    # For raw the allocation status for new image depends on file system, force
> +    # qcow2 in this case, since the tested code is in block layer
> +    IMGFMT=qcow2 _make_test_img $size | _filter_imgfmt

If it can't be tested, skip the test.  Don't silently test something
else.

Stefan

Re: [Qemu-devel] [PATCH v3 5/6] qemu-iotests: Force qcow2 in error path test in 048

[Fam Zheng]

On 2013年11月21日 20:41, Stefan Hajnoczi wrote:
> On Wed, Nov 20, 2013 at 03:44:16PM +0800, Fam Zheng wrote:
>> The "raw" doesn't always work on certain file systems (e.g. tmpfs). Use
>> qcow2 to make the allocation status explicit.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>   tests/qemu-iotests/048 | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/qemu-iotests/048 b/tests/qemu-iotests/048
>> index 9def7fc..0fb5d2c 100755
>> --- a/tests/qemu-iotests/048
>> +++ b/tests/qemu-iotests/048
>> @@ -88,7 +88,13 @@ event = "read_aio"
>>   errno = "5"
>>   once ="off"
>>   EOF
>> -_make_test_img $size
>> +if [ "$IMGFMT" = "raw" ]; then
>> +    # For raw the allocation status for new image depends on file system, force
>> +    # qcow2 in this case, since the tested code is in block layer
>> +    IMGFMT=qcow2 _make_test_img $size | _filter_imgfmt
>
> If it can't be tested, skip the test.  Don't silently test something
> else.
>

OK. Do you think I should split the test? Since we don't have 
conditional reference output.

Fam

[Qemu-devel] [PATCH v3 6/6] qemu-iotests: Clean up spaces in usage output

[Fam Zheng]

Whitespace changes to align columns.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/common | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
index 7db6bde..75dcdd8 100644
--- a/tests/qemu-iotests/common
+++ b/tests/qemu-iotests/common
@@ -132,7 +132,7 @@ s/ .*//p
             echo "Usage: $0 [options] [testlist]"'
 
 common options
-    -v                        verbose
+    -v                  verbose
 
 check options
     -raw                test raw (default)
@@ -148,20 +148,20 @@ check options
     -sheepdog           test sheepdog
     -nbd                test nbd
     -ssh                test ssh
-    -xdiff                graphical mode diff
-    -nocache                use O_DIRECT on backing file
-    -misalign                misalign memory allocations
-    -n                        show me, do not run tests
+    -xdiff              graphical mode diff
+    -nocache            use O_DIRECT on backing file
+    -misalign           misalign memory allocations
+    -n                  show me, do not run tests
     -o options          -o options to pass to qemu-img create/convert
-    -T                        output timestamps
-    -r                         randomize test order
+    -T                  output timestamps
+    -r                  randomize test order
     -c                  cache mode
 
 testlist options
     -g group[,group...]        include tests from these groups
     -x group[,group...]        exclude tests from these groups
     NNN                        include test NNN
-    NNN-NNN                include test range (eg. 012-021)
+    NNN-NNN                    include test range (eg. 012-021)
 '
             exit 0
             ;;
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v3 0/6] Add cache mode option to qemu-iotests, and change default mode to "writeback"

[Stefan Hajnoczi]

On Wed, Nov 20, 2013 at 03:44:11PM +0800, Fam Zheng wrote:
> This series adds cache mode option in the iotests framework. Test cases are
> updated to make use of cache mode and mask supported modes.
> 
> v3: Change _unsupported_qemu_io_options to _supported_cache_modes.
>     Change default mode to "writeback".
>     Clean up some whitespaces in the end of series.
>     Fix "026.out.nocache" case.
>     Fix 048 case on tmpfs.
> 
> 
> Fam Zheng (6):
>   qemu-iotests: Add "-c <cache-mode>" option
>   qemu-iotests: Honour cache mode in iotests.py
>   qemu-iotests: Add _supported_cache_modes
>   qemu-iotests: Change default cache mode to "writeback"
>   qemu-iotests: Force qcow2 in error path test in 048
>   qemu-iotests: Clean up spaces in usage output
> 
>  tests/qemu-iotests/026        |  2 +-
>  tests/qemu-iotests/039        |  2 +-
>  tests/qemu-iotests/048        |  8 +++++++-
>  tests/qemu-iotests/052        |  3 +--
>  tests/qemu-iotests/check      |  2 +-
>  tests/qemu-iotests/common     | 37 +++++++++++++++++++++++++++----------
>  tests/qemu-iotests/common.rc  | 20 ++++++++++----------
>  tests/qemu-iotests/iotests.py |  3 ++-
>  8 files changed, 50 insertions(+), 27 deletions(-)

In principle I'm happy with the series but there are two instances where
you are silently changing what the test does (overriding cache mode and
image format).

Please skip tests that cannot run instead of silently testing something
else.

Stefan

Re: [Qemu-devel] [PATCH v3 0/6] Add cache mode option to qemu-iotests, and change default mode to "writeback"

[Kevin Wolf]

Am 21.11.2013 um 13:43 hat Stefan Hajnoczi geschrieben:
> On Wed, Nov 20, 2013 at 03:44:11PM +0800, Fam Zheng wrote:
> > This series adds cache mode option in the iotests framework. Test cases are
> > updated to make use of cache mode and mask supported modes.
> > 
> > v3: Change _unsupported_qemu_io_options to _supported_cache_modes.
> >     Change default mode to "writeback".
> >     Clean up some whitespaces in the end of series.
> >     Fix "026.out.nocache" case.
> >     Fix 048 case on tmpfs.
> > 
> > 
> > Fam Zheng (6):
> >   qemu-iotests: Add "-c <cache-mode>" option
> >   qemu-iotests: Honour cache mode in iotests.py
> >   qemu-iotests: Add _supported_cache_modes
> >   qemu-iotests: Change default cache mode to "writeback"
> >   qemu-iotests: Force qcow2 in error path test in 048
> >   qemu-iotests: Clean up spaces in usage output
> > 
> >  tests/qemu-iotests/026        |  2 +-
> >  tests/qemu-iotests/039        |  2 +-
> >  tests/qemu-iotests/048        |  8 +++++++-
> >  tests/qemu-iotests/052        |  3 +--
> >  tests/qemu-iotests/check      |  2 +-
> >  tests/qemu-iotests/common     | 37 +++++++++++++++++++++++++++----------
> >  tests/qemu-iotests/common.rc  | 20 ++++++++++----------
> >  tests/qemu-iotests/iotests.py |  3 ++-
> >  8 files changed, 50 insertions(+), 27 deletions(-)
> 
> In principle I'm happy with the series but there are two instances where
> you are silently changing what the test does (overriding cache mode and
> image format).
> 
> Please skip tests that cannot run instead of silently testing something
> else.

Skipping isn't really a good solution either. For example, I almost
never test 039, because I always run ./check -nocache. What would be
most useful is to use a working cache mode for this test rather than
leaving it untested over weeks and months.

I mean we can make things as sophisticated as we want, and introduce a
list of cache modes in the order of their preference, so that I can say
./check -c none,writethrough, but that's probably a bit overengineered.

Kevin

Re: [Qemu-devel] [PATCH v3 0/6] Add cache mode option to qemu-iotests, and change default mode to "writeback"

[Stefan Hajnoczi]

On Thu, Nov 21, 2013 at 03:16:23PM +0100, Kevin Wolf wrote:
> Am 21.11.2013 um 13:43 hat Stefan Hajnoczi geschrieben:
> > On Wed, Nov 20, 2013 at 03:44:11PM +0800, Fam Zheng wrote:
> > > This series adds cache mode option in the iotests framework. Test cases are
> > > updated to make use of cache mode and mask supported modes.
> > > 
> > > v3: Change _unsupported_qemu_io_options to _supported_cache_modes.
> > >     Change default mode to "writeback".
> > >     Clean up some whitespaces in the end of series.
> > >     Fix "026.out.nocache" case.
> > >     Fix 048 case on tmpfs.
> > > 
> > > 
> > > Fam Zheng (6):
> > >   qemu-iotests: Add "-c <cache-mode>" option
> > >   qemu-iotests: Honour cache mode in iotests.py
> > >   qemu-iotests: Add _supported_cache_modes
> > >   qemu-iotests: Change default cache mode to "writeback"
> > >   qemu-iotests: Force qcow2 in error path test in 048
> > >   qemu-iotests: Clean up spaces in usage output
> > > 
> > >  tests/qemu-iotests/026        |  2 +-
> > >  tests/qemu-iotests/039        |  2 +-
> > >  tests/qemu-iotests/048        |  8 +++++++-
> > >  tests/qemu-iotests/052        |  3 +--
> > >  tests/qemu-iotests/check      |  2 +-
> > >  tests/qemu-iotests/common     | 37 +++++++++++++++++++++++++++----------
> > >  tests/qemu-iotests/common.rc  | 20 ++++++++++----------
> > >  tests/qemu-iotests/iotests.py |  3 ++-
> > >  8 files changed, 50 insertions(+), 27 deletions(-)
> > 
> > In principle I'm happy with the series but there are two instances where
> > you are silently changing what the test does (overriding cache mode and
> > image format).
> > 
> > Please skip tests that cannot run instead of silently testing something
> > else.
> 
> Skipping isn't really a good solution either. For example, I almost
> never test 039, because I always run ./check -nocache. What would be
> most useful is to use a working cache mode for this test rather than
> leaving it untested over weeks and months.
> 
> I mean we can make things as sophisticated as we want, and introduce a
> list of cache modes in the order of their preference, so that I can say
> ./check -c none,writethrough, but that's probably a bit overengineered.

Why not just:

# Before sending a block pull request
./check -c none
./check -c writeback

That covers both O_DIRECT and buffered I/O.

I think 1:1 behavior is important to keep it easy to reason about test
cases.  If test cases go off an change behavior behind the scenes it's
possible to get confused or draw the wrong conclusions.

Stefan

Re: [Qemu-devel] [PATCH v3 0/6] Add cache mode option to qemu-iotests, and change default mode to "writeback"

[Kevin Wolf]

Am 22.11.2013 um 09:57 hat Stefan Hajnoczi geschrieben:
> On Thu, Nov 21, 2013 at 03:16:23PM +0100, Kevin Wolf wrote:
> > Am 21.11.2013 um 13:43 hat Stefan Hajnoczi geschrieben:
> > > On Wed, Nov 20, 2013 at 03:44:11PM +0800, Fam Zheng wrote:
> > > > This series adds cache mode option in the iotests framework. Test cases are
> > > > updated to make use of cache mode and mask supported modes.
> > > > 
> > > > v3: Change _unsupported_qemu_io_options to _supported_cache_modes.
> > > >     Change default mode to "writeback".
> > > >     Clean up some whitespaces in the end of series.
> > > >     Fix "026.out.nocache" case.
> > > >     Fix 048 case on tmpfs.
> > > > 
> > > > 
> > > > Fam Zheng (6):
> > > >   qemu-iotests: Add "-c <cache-mode>" option
> > > >   qemu-iotests: Honour cache mode in iotests.py
> > > >   qemu-iotests: Add _supported_cache_modes
> > > >   qemu-iotests: Change default cache mode to "writeback"
> > > >   qemu-iotests: Force qcow2 in error path test in 048
> > > >   qemu-iotests: Clean up spaces in usage output
> > > > 
> > > >  tests/qemu-iotests/026        |  2 +-
> > > >  tests/qemu-iotests/039        |  2 +-
> > > >  tests/qemu-iotests/048        |  8 +++++++-
> > > >  tests/qemu-iotests/052        |  3 +--
> > > >  tests/qemu-iotests/check      |  2 +-
> > > >  tests/qemu-iotests/common     | 37 +++++++++++++++++++++++++++----------
> > > >  tests/qemu-iotests/common.rc  | 20 ++++++++++----------
> > > >  tests/qemu-iotests/iotests.py |  3 ++-
> > > >  8 files changed, 50 insertions(+), 27 deletions(-)
> > > 
> > > In principle I'm happy with the series but there are two instances where
> > > you are silently changing what the test does (overriding cache mode and
> > > image format).
> > > 
> > > Please skip tests that cannot run instead of silently testing something
> > > else.
> > 
> > Skipping isn't really a good solution either. For example, I almost
> > never test 039, because I always run ./check -nocache. What would be
> > most useful is to use a working cache mode for this test rather than
> > leaving it untested over weeks and months.
> > 
> > I mean we can make things as sophisticated as we want, and introduce a
> > list of cache modes in the order of their preference, so that I can say
> > ./check -c none,writethrough, but that's probably a bit overengineered.
> 
> Why not just:
> 
> # Before sending a block pull request
> ./check -c none
> ./check -c writeback
> 
> That covers both O_DIRECT and buffered I/O.

And takes about twice the time. Sorry, no, in practice it means that 039
(and I think one or two more) stays untested by me, because running all
test cases a second time with writethrough (not writeback) would take
forever.

> I think 1:1 behavior is important to keep it easy to reason about test
> cases.  If test cases go off an change behavior behind the scenes it's
> possible to get confused or draw the wrong conclusions.

I can understand your position and would agree in theory. I just think
that running the test with more difficult reasoning is preferable to
not running the test and therefore not reasoning about it even though
the reasoning would have been easy.

Kevin

Re: [Qemu-devel] [PATCH v3 0/6] Add cache mode option to qemu-iotests, and change default mode to "writeback"

[Stefan Hajnoczi]

On Fri, Nov 22, 2013 at 10:30:30AM +0100, Kevin Wolf wrote:
> Am 22.11.2013 um 09:57 hat Stefan Hajnoczi geschrieben:
> > On Thu, Nov 21, 2013 at 03:16:23PM +0100, Kevin Wolf wrote:
> > > Am 21.11.2013 um 13:43 hat Stefan Hajnoczi geschrieben:
> > > > On Wed, Nov 20, 2013 at 03:44:11PM +0800, Fam Zheng wrote:
> > > > > This series adds cache mode option in the iotests framework. Test cases are
> > > > > updated to make use of cache mode and mask supported modes.
> > > > > 
> > > > > v3: Change _unsupported_qemu_io_options to _supported_cache_modes.
> > > > >     Change default mode to "writeback".
> > > > >     Clean up some whitespaces in the end of series.
> > > > >     Fix "026.out.nocache" case.
> > > > >     Fix 048 case on tmpfs.
> > > > > 
> > > > > 
> > > > > Fam Zheng (6):
> > > > >   qemu-iotests: Add "-c <cache-mode>" option
> > > > >   qemu-iotests: Honour cache mode in iotests.py
> > > > >   qemu-iotests: Add _supported_cache_modes
> > > > >   qemu-iotests: Change default cache mode to "writeback"
> > > > >   qemu-iotests: Force qcow2 in error path test in 048
> > > > >   qemu-iotests: Clean up spaces in usage output
> > > > > 
> > > > >  tests/qemu-iotests/026        |  2 +-
> > > > >  tests/qemu-iotests/039        |  2 +-
> > > > >  tests/qemu-iotests/048        |  8 +++++++-
> > > > >  tests/qemu-iotests/052        |  3 +--
> > > > >  tests/qemu-iotests/check      |  2 +-
> > > > >  tests/qemu-iotests/common     | 37 +++++++++++++++++++++++++++----------
> > > > >  tests/qemu-iotests/common.rc  | 20 ++++++++++----------
> > > > >  tests/qemu-iotests/iotests.py |  3 ++-
> > > > >  8 files changed, 50 insertions(+), 27 deletions(-)
> > > > 
> > > > In principle I'm happy with the series but there are two instances where
> > > > you are silently changing what the test does (overriding cache mode and
> > > > image format).
> > > > 
> > > > Please skip tests that cannot run instead of silently testing something
> > > > else.
> > > 
> > > Skipping isn't really a good solution either. For example, I almost
> > > never test 039, because I always run ./check -nocache. What would be
> > > most useful is to use a working cache mode for this test rather than
> > > leaving it untested over weeks and months.
> > > 
> > > I mean we can make things as sophisticated as we want, and introduce a
> > > list of cache modes in the order of their preference, so that I can say
> > > ./check -c none,writethrough, but that's probably a bit overengineered.
> > 
> > Why not just:
> > 
> > # Before sending a block pull request
> > ./check -c none
> > ./check -c writeback
> > 
> > That covers both O_DIRECT and buffered I/O.
> 
> And takes about twice the time. Sorry, no, in practice it means that 039
> (and I think one or two more) stays untested by me, because running all
> test cases a second time with writethrough (not writeback) would take
> forever.
> 
> > I think 1:1 behavior is important to keep it easy to reason about test
> > cases.  If test cases go off an change behavior behind the scenes it's
> > possible to get confused or draw the wrong conclusions.
> 
> I can understand your position and would agree in theory. I just think
> that running the test with more difficult reasoning is preferable to
> not running the test and therefore not reasoning about it even though
> the reasoning would have been easy.

I think we've gotten some good ideas from an IRC discussion with Fam and
Wenchao.

A "./check" default mode that runs every test case with appropriate
settings is useful.  This makes it easy to run the tests and gives us
good coverage.

Stefan