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