Re: [Qemu-devel] [PATCH v2 7/7] qemu-iotests: add 039 qcow2 lazy refcounts test

[Stefan Hajnoczi]

On Fri, Jul 27, 2012 at 9:07 AM, Kevin Wolf  wrote:
> Am 27.07.2012 09:56, schrieb Stefan Hajnoczi:
>> On Thu, Jul 26, 2012 at 2:28 PM, Kevin Wolf  wrote:
>>> Am 25.07.2012 14:21, schrieb Stefan Hajnoczi:
 +== Read-only access must still work ==
 +read 512/512 bytes at offset 0
 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 +incompatible_features 0x1
 +
 +== Repairing the image file must succeed ==
 +ERROR OFLAG_COPIED: offset=8005 refcount=0
 +Repairing cluster 5 refcount=0 reference=1
 +No errors were found on the image.
 +incompatible_features 0x0
>>>
>>> I wonder what happened to the "The following inconsistencies were found
>>> and repaired" message. Most likely not a problem with qemu-iotests,
>>> though, but something unexpected in qemu-img.
>>
>> It's because opening a qcow2 image read/write when the dirty flag is
>> set causes a repair.  This accounts for the "Repairing cluster 5 ..."
>> message.
>>
>> Then qemu-img check -r all calls bdrv_check() on an already repaired
>> image file and we get the "No errors were found on the image".
>
> I see. Not exactly how it was intended... Do we need a BDRV_O_CHECK flag
> that prevents the automatic repair or should we just live with the
> suboptimal output when lazy refcounting is enabled?

You noticed the issue so others might notice it too.  I'll send a
patch including qcow2 and qed changes to fix this using BDRV_O_CHECK.

Stefan

Re: [Qemu-devel] [PATCH v2 7/7] qemu-iotests: add 039 qcow2 lazy refcounts test

[Kevin Wolf]

Am 27.07.2012 09:56, schrieb Stefan Hajnoczi:
> On Thu, Jul 26, 2012 at 2:28 PM, Kevin Wolf  wrote:
>> Am 25.07.2012 14:21, schrieb Stefan Hajnoczi:
>>> +== Read-only access must still work ==
>>> +read 512/512 bytes at offset 0
>>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>> +incompatible_features 0x1
>>> +
>>> +== Repairing the image file must succeed ==
>>> +ERROR OFLAG_COPIED: offset=8005 refcount=0
>>> +Repairing cluster 5 refcount=0 reference=1
>>> +No errors were found on the image.
>>> +incompatible_features 0x0
>>
>> I wonder what happened to the "The following inconsistencies were found
>> and repaired" message. Most likely not a problem with qemu-iotests,
>> though, but something unexpected in qemu-img.
> 
> It's because opening a qcow2 image read/write when the dirty flag is
> set causes a repair.  This accounts for the "Repairing cluster 5 ..."
> message.
> 
> Then qemu-img check -r all calls bdrv_check() on an already repaired
> image file and we get the "No errors were found on the image".

I see. Not exactly how it was intended... Do we need a BDRV_O_CHECK flag
that prevents the automatic repair or should we just live with the
suboptimal output when lazy refcounting is enabled?

Kevin

Re: [Qemu-devel] [PATCH v2 7/7] qemu-iotests: add 039 qcow2 lazy refcounts test

[Stefan Hajnoczi]

On Thu, Jul 26, 2012 at 2:28 PM, Kevin Wolf  wrote:
> Am 25.07.2012 14:21, schrieb Stefan Hajnoczi:
>> This tests establishes the basic post-conditions of the qcow2 lazy
>> refcounts features:
>>
>>   1. If the image was closed normally, it is marked clean.
>>
>>   2. If an allocating write was performed and the image was not close
>>  normally, then it is marked dirty.
>>
>>  a. Written data can be read back successfully.
>>  b. The image file can be repaired and will be marked clean again.
>>
>> Signed-off-by: Stefan Hajnoczi 
>
> I think an important case that is missing here is opening a dirty image
> rw without having run qemu-img check -r first.

I have added that test case.

>> +== Read-only access must still work ==
>> +read 512/512 bytes at offset 0
>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +incompatible_features 0x1
>> +
>> +== Repairing the image file must succeed ==
>> +ERROR OFLAG_COPIED: offset=8005 refcount=0
>> +Repairing cluster 5 refcount=0 reference=1
>> +No errors were found on the image.
>> +incompatible_features 0x0
>
> I wonder what happened to the "The following inconsistencies were found
> and repaired" message. Most likely not a problem with qemu-iotests,
> though, but something unexpected in qemu-img.

It's because opening a qcow2 image read/write when the dirty flag is
set causes a repair.  This accounts for the "Repairing cluster 5 ..."
message.

Then qemu-img check -r all calls bdrv_check() on an already repaired
image file and we get the "No errors were found on the image".

Stefan

Re: [Qemu-devel] [PATCH v2 7/7] qemu-iotests: add 039 qcow2 lazy refcounts test

[Kevin Wolf]

Am 25.07.2012 14:21, schrieb Stefan Hajnoczi:
> This tests establishes the basic post-conditions of the qcow2 lazy
> refcounts features:
> 
>   1. If the image was closed normally, it is marked clean.
> 
>   2. If an allocating write was performed and the image was not close
>  normally, then it is marked dirty.
> 
>  a. Written data can be read back successfully.
>  b. The image file can be repaired and will be marked clean again.
> 
> Signed-off-by: Stefan Hajnoczi 

I think an important case that is missing here is opening a dirty image
rw without having run qemu-img check -r first.

> +== Read-only access must still work ==
> +read 512/512 bytes at offset 0
> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +incompatible_features 0x1
> +
> +== Repairing the image file must succeed ==
> +ERROR OFLAG_COPIED: offset=8005 refcount=0
> +Repairing cluster 5 refcount=0 reference=1
> +No errors were found on the image.
> +incompatible_features 0x0

I wonder what happened to the "The following inconsistencies were found
and repaired" message. Most likely not a problem with qemu-iotests,
though, but something unexpected in qemu-img.

Kevin

Re: [Qemu-devel] [PATCH v2 7/7] qemu-iotests: add 039 qcow2 lazy refcounts test

[Eric Blake]

On 07/25/2012 04:45 PM, Stefan Hajnoczi wrote:

>> Since you are assuming bash (and even if you were to assume POSIX
>> /bin/sh)...
>>
>>> +
>>> +seq=`basename $0`
>>
>> I prefer $() over ``.
>>
>>> +echo "QA output created by $seq"
>>> +
>>> +here=`pwd`
>>
>> POSIX (and therefore bash) guarantees that $PWD is sane, and faster to
>> access than $(pwd).
>>
>>> +tmp=/tmp/$$
>>
>> That's not very secure.  It may be worth using bash's $RANDOM, or using
>> mkstemp(1).
>>
>> Beyond that, the series seemed reasonable to me.
> 
> All qemu-iotests scripts do these things in the same way and I'd like
> for them to be consistent.

Good argument.

> 
> If we make these changes they should be applied to all qemu-iotests
> scripts.  I agree with your points but also think the value in making
> the change now is small.

Indeed - what you have is technically correct, even if not the most
efficient.  Any such cleanups should, as you say, be a separate patch
globally applied to the qemu-iotests, and not this test in isolation.

> 
> Do you want to send a patch that fixes these issues in qemu-iotests?

Up to you; or read another way, it bothered me enough to comment, but
not enough to write the patch myself, so I'm fine living with status quo
if it doesn't bother anyone else either.  Old-school techniques aren't
wrong, per se, just inefficient; and while the insecure temp file name
could be exploited, people running the testsuite tend to be on personal
platforms rather than enterprise systems, and the cost of exploiting a
testsuite is not as severe as the cost of exploiting an installed script.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 7/7] qemu-iotests: add 039 qcow2 lazy refcounts test

[Stefan Hajnoczi]

On Wed, Jul 25, 2012 at 11:54:50AM -0600, Eric Blake wrote:
> On 07/25/2012 06:21 AM, Stefan Hajnoczi wrote:
> > This tests establishes the basic post-conditions of the qcow2 lazy
> > refcounts features:
> > 
> >   1. If the image was closed normally, it is marked clean.
> > 
> >   2. If an allocating write was performed and the image was not close
> >  normally, then it is marked dirty.
> > 
> >  a. Written data can be read back successfully.
> >  b. The image file can be repaired and will be marked clean again.
> > 
> > Signed-off-by: Stefan Hajnoczi 
> 
> > +++ b/tests/qemu-iotests/039
> > @@ -0,0 +1,117 @@
> > +#!/bin/bash
> 
> Since you are assuming bash (and even if you were to assume POSIX
> /bin/sh)...
> 
> > +
> > +seq=`basename $0`
> 
> I prefer $() over ``.
> 
> > +echo "QA output created by $seq"
> > +
> > +here=`pwd`
> 
> POSIX (and therefore bash) guarantees that $PWD is sane, and faster to
> access than $(pwd).
> 
> > +tmp=/tmp/$$
> 
> That's not very secure.  It may be worth using bash's $RANDOM, or using
> mkstemp(1).
> 
> Beyond that, the series seemed reasonable to me.

All qemu-iotests scripts do these things in the same way and I'd like
for them to be consistent.

If we make these changes they should be applied to all qemu-iotests
scripts.  I agree with your points but also think the value in making
the change now is small.

Do you want to send a patch that fixes these issues in qemu-iotests?
The general shell scripting style used there is quite old school and
makes use of backquotes often.

Stefan

Re: [Qemu-devel] [PATCH v2 7/7] qemu-iotests: add 039 qcow2 lazy refcounts test

[Eric Blake]

On 07/25/2012 06:21 AM, Stefan Hajnoczi wrote:
> This tests establishes the basic post-conditions of the qcow2 lazy
> refcounts features:
> 
>   1. If the image was closed normally, it is marked clean.
> 
>   2. If an allocating write was performed and the image was not close
>  normally, then it is marked dirty.
> 
>  a. Written data can be read back successfully.
>  b. The image file can be repaired and will be marked clean again.
> 
> Signed-off-by: Stefan Hajnoczi 

> +++ b/tests/qemu-iotests/039
> @@ -0,0 +1,117 @@
> +#!/bin/bash

Since you are assuming bash (and even if you were to assume POSIX
/bin/sh)...

> +
> +seq=`basename $0`

I prefer $() over ``.

> +echo "QA output created by $seq"
> +
> +here=`pwd`

POSIX (and therefore bash) guarantees that $PWD is sane, and faster to
access than $(pwd).

> +tmp=/tmp/$$

That's not very secure.  It may be worth using bash's $RANDOM, or using
mkstemp(1).

Beyond that, the series seemed reasonable to me.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH v2 7/7] qemu-iotests: add 039 qcow2 lazy refcounts test

[Stefan Hajnoczi]

This tests establishes the basic post-conditions of the qcow2 lazy
refcounts features:

  1. If the image was closed normally, it is marked clean.

  2. If an allocating write was performed and the image was not close
 normally, then it is marked dirty.

 a. Written data can be read back successfully.
 b. The image file can be repaired and will be marked clean again.

Signed-off-by: Stefan Hajnoczi 
---
 tests/qemu-iotests/039 |  117 
 tests/qemu-iotests/039.out |   42 
 tests/qemu-iotests/group   |1 +
 3 files changed, 160 insertions(+)
 create mode 100755 tests/qemu-iotests/039
 create mode 100644 tests/qemu-iotests/039.out

diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
new file mode 100755
index 000..4bf792f
--- /dev/null
+++ b/tests/qemu-iotests/039
@@ -0,0 +1,117 @@
+#!/bin/bash
+#
+# Test qcow2 lazy refcounts
+#
+# Copyright (C) 2012 Red Hat, Inc.
+# Copyright IBM, Corp. 2010
+#
+# Based on test 038.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=stefa...@linux.vnet.ibm.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+size=128M
+
+echo
+echo "== Checking that image is clean on shutdown =="
+
+IMGOPTS="compat=1.1,lazy_refcounts=on"
+_make_test_img $size
+
+$QEMU_IO -c "write -P 0x5a 0 512" $TEST_IMG | _filter_qemu_io
+
+# The dirty bit must not be set
+./qcow2.py $TEST_IMG dump-header | grep incompatible_features
+_check_test_img
+
+echo
+echo "== Creating a dirty image file =="
+
+IMGOPTS="compat=1.1,lazy_refcounts=on"
+_make_test_img $size
+
+old_ulimit=$(ulimit -c)
+ulimit -c 0 # do not produce a core dump on abort(3)
+$QEMU_IO -c "write -P 0x5a 0 512" -c "abort" $TEST_IMG | _filter_qemu_io
+ulimit -c "$old_ulimit"
+
+# The dirty bit must be set
+./qcow2.py $TEST_IMG dump-header | grep incompatible_features
+_check_test_img
+
+echo
+echo "== Read-only access must still work =="
+
+$QEMU_IO -r -c "read -P 0x5a 0 512" $TEST_IMG | _filter_qemu_io
+
+# The dirty bit must be set
+./qcow2.py $TEST_IMG dump-header | grep incompatible_features
+
+echo
+echo "== Repairing the image file must succeed =="
+
+$QEMU_IMG check -r all $TEST_IMG
+
+# The dirty bit must not be set
+./qcow2.py $TEST_IMG dump-header | grep incompatible_features
+
+echo
+echo "== Data should still be accessible after repair =="
+
+$QEMU_IO -c "read -P 0x5a 0 512" $TEST_IMG | _filter_qemu_io
+
+echo
+echo "== Creating an image file with lazy_refcounts=off =="
+
+IMGOPTS="compat=1.1,lazy_refcounts=off"
+_make_test_img $size
+
+old_ulimit=$(ulimit -c)
+ulimit -c 0 # do not produce a core dump on abort(3)
+$QEMU_IO -c "write -P 0x5a 0 512" -c "abort" $TEST_IMG | _filter_qemu_io
+ulimit -c "$old_ulimit"
+
+# The dirty bit must not be set since lazy_refcounts=off
+./qcow2.py $TEST_IMG dump-header | grep incompatible_features
+_check_test_img
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
+
diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
new file mode 100644
index 000..6be47c0
--- /dev/null
+++ b/tests/qemu-iotests/039.out
@@ -0,0 +1,42 @@
+QA output created by 039
+
+== Checking that image is clean on shutdown ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 lazy_refcounts=on 
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+incompatible_features 0x0
+No errors were found on the image.
+
+== Creating a dirty image file ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 lazy_refcounts=on 
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+incompatible_features 0x1
+ERROR OFLAG_COPIED: offset=8005 refcount=0
+ERROR cluster 5 refcount=0 reference=1
+
+2 errors were found on the image.
+Data may be corrupted, or further writes to the image may corrupt it.
+
+== Read-only access must still work ==
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+incompatible_features 0x1
+
+== Repairing the imag