From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53727) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vjt1X-0005TW-Jk for qemu-devel@nongnu.org; Fri, 22 Nov 2013 10:52:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vjt1Q-0000YA-0l for qemu-devel@nongnu.org; Fri, 22 Nov 2013 10:52:03 -0500 Received: from mail-we0-x235.google.com ([2a00:1450:400c:c03::235]:43344) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vjt1P-0000Xk-MG for qemu-devel@nongnu.org; Fri, 22 Nov 2013 10:51:55 -0500 Received: by mail-we0-f181.google.com with SMTP id x55so1349801wes.12 for ; Fri, 22 Nov 2013 07:51:54 -0800 (PST) Date: Fri, 22 Nov 2013 16:51:51 +0100 From: Stefan Hajnoczi Message-ID: <20131122155151.GB3232@stefanha-thinkpad.redhat.com> References: <1384933457-26953-1-git-send-email-famz@redhat.com> <20131121124340.GC7466@stefanha-thinkpad.redhat.com> <20131121141623.GH3454@dhcp-200-207.str.redhat.com> <20131122085735.GB18096@stefanha-thinkpad.redhat.com> <20131122093030.GB3720@dhcp-200-207.str.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20131122093030.GB3720@dhcp-200-207.str.redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 0/6] Add cache mode option to qemu-iotests, and change default mode to "writeback" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Fam Zheng , qemu-devel@nongnu.org, Stefan Hajnoczi , WenchaoXia 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 " 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