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