Re: [Qemu-devel] [PATCH v3 0/6] Add cache mode option to qemu-iotests, and change default mode to writeback
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
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
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
Re: [Qemu-devel] [PATCH v3 0/6] Add cache mode option to qemu-iotests, and change default mode to writeback
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
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
