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