[Qemu-devel] [PATCH] qemu-iotests: make a few more tests generic

[Qemu-devel] [PATCH] qemu-iotests: make a few more tests generic

[Christoph Hellwig]

Pretend that a non-implemented check is always successful and thus allow
various tests that were qcow2-specific before to be generic.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: qemu-iotests/009
===================================================================
--- qemu-iotests.orig/009	2009-07-08 21:03:29.268364164 +0200
+++ qemu-iotests/009	2009-07-08 21:10:26.103364516 +0200
@@ -40,7 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 1
 . ./common.rc
 . ./common.filter
 
-_supported_fmt qcow2
+_supported_fmt generic
 _supported_os Linux
 
 
Index: qemu-iotests/010
===================================================================
--- qemu-iotests.orig/010	2009-07-08 21:03:29.274364788 +0200
+++ qemu-iotests/010	2009-07-08 21:10:26.104364852 +0200
@@ -40,7 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 1
 . ./common.rc
 . ./common.filter
 
-_supported_fmt qcow2
+_supported_fmt generic
 _supported_os Linux
 
 
Index: qemu-iotests/011
===================================================================
--- qemu-iotests.orig/011	2009-07-08 21:03:29.281364421 +0200
+++ qemu-iotests/011	2009-07-08 21:10:26.104364852 +0200
@@ -40,7 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 1
 . ./common.rc
 . ./common.filter
 
-_supported_fmt qcow2
+_supported_fmt generic
 _supported_os Linux
 
 
Index: qemu-iotests/013
===================================================================
--- qemu-iotests.orig/013	2009-07-08 21:10:19.394364074 +0200
+++ qemu-iotests/013	2009-07-08 21:29:03.448264691 +0200
@@ -42,7 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 1
 . ./common.pattern
 
 # much of this could be generic for any format supporting compression.
-_supported_fmt qcow2
+_supported_fmt qcow qcow2
 _supported_os Linux
 
 TEST_OFFSETS="0 4294967296"
@@ -66,7 +66,7 @@ echo "Compressing image"
 echo
 
 mv $TEST_IMG $TEST_IMG.orig
-$QEMU_IMG convert -f qcow2 -O qcow2 -c $TEST_IMG.orig $TEST_IMG
+$QEMU_IMG convert -f $IMGFMT -O $IMGFMT -c $TEST_IMG.orig $TEST_IMG
 
 echo "Testing compressed image"
 echo
Index: qemu-iotests/014
===================================================================
--- qemu-iotests.orig/014	2009-07-08 21:03:29.384364533 +0200
+++ qemu-iotests/014	2009-07-08 21:10:26.109364301 +0200
@@ -42,7 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 1
 . ./common.filter
 . ./common.pattern
 
-# much of this could be generic for any format supporting compression.
+# much of this could be generic for any format supporting snapshots
 _supported_fmt qcow2
 _supported_os Linux
 
Index: qemu-iotests/015
===================================================================
--- qemu-iotests.orig/015	2009-07-08 21:10:24.862372246 +0200
+++ qemu-iotests/015	2009-07-08 21:10:26.110364568 +0200
@@ -41,7 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 1
 . ./common.rc
 . ./common.filter
 
-# currently only qcow2 allows for consistency checks using qemu-img
+# actually any format that supports snapshots
 _supported_fmt qcow2
 _supported_os Linux
 
Index: qemu-iotests/common.rc
===================================================================
--- qemu-iotests.orig/common.rc	2009-07-08 21:03:29.393364072 +0200
+++ qemu-iotests/common.rc	2009-07-08 21:10:26.111364626 +0200
@@ -75,7 +75,8 @@ _cleanup_test_img()
 
 _check_test_img()
 {
-    $QEMU_IMG check -f $IMGFMT $TEST_IMG
+    $QEMU_IMG check -f $IMGFMT $TEST_IMG 2>&1 | \
+    	sed -e 's/qemu-img\: This image format does not support checks/No errors were found on the image./'
 }
 
 _get_pids_by_name()
Index: qemu-iotests/common.pattern
===================================================================
--- qemu-iotests.orig/common.pattern	2009-07-08 21:03:29.398364429 +0200
+++ qemu-iotests/common.pattern	2009-07-08 21:10:26.112364683 +0200
@@ -96,7 +96,7 @@ function io_test2() {
     io_pattern writev $((offset + 8 * 4096)) 4096 $((9 * 4096)) 256 165
 
     mv $TEST_IMG $TEST_IMG.orig
-    $QEMU_IMG convert -f qcow2 -O qcow2 -c $TEST_IMG.orig $TEST_IMG
+    $QEMU_IMG convert -f $IMGFMT -O $IMGFMT -c $TEST_IMG.orig $TEST_IMG
 
     # Write the used clusters
     echo === Used clusters [1]

Re: [Qemu-devel] [PATCH] qemu-iotests: make a few more tests generic

[Kevin Wolf]

Christoph Hellwig schrieb:
> Pretend that a non-implemented check is always successful and thus allow
> various tests that were qcow2-specific before to be generic.

Looks good in general. However, vmdk and vpc fail test case 011 for me.
Not sure yet where the problem actually is, there seems to be no qemu-io
output at all for these.

Kevin

Re: [Qemu-devel] [PATCH] qemu-iotests: make a few more tests generic

[Christoph Hellwig]

On Thu, Jul 09, 2009 at 11:58:58AM +0200, Kevin Wolf wrote:
> Christoph Hellwig schrieb:
> > Pretend that a non-implemented check is always successful and thus allow
> > various tests that were qcow2-specific before to be generic.
> 
> Looks good in general. However, vmdk and vpc fail test case 011 for me.
> Not sure yet where the problem actually is, there seems to be no qemu-io
> output at all for these.

They pass for me.  So I guess we have a problem somewhere that we'll
eventually need to investigate.

Re: [Qemu-devel] [PATCH] qemu-iotests: make a few more tests generic

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]

Christoph Hellwig schrieb:
> On Thu, Jul 09, 2009 at 11:58:58AM +0200, Kevin Wolf wrote:
>> Christoph Hellwig schrieb:
>>> Pretend that a non-implemented check is always successful and thus allow
>>> various tests that were qcow2-specific before to be generic.
>> Looks good in general. However, vmdk and vpc fail test case 011 for me.
>> Not sure yet where the problem actually is, there seems to be no qemu-io
>> output at all for these.
> 
> They pass for me.  So I guess we have a problem somewhere that we'll
> eventually need to investigate.

This problem has already be found. Avi's patch from almost three weeks
ago fixes it.

It really starts to get annoying. How am I supposed to work with commits
only every other week (which is bad enough) and then patches are
forgotten and probably won't be merged before another two weeks? I guess
I should manage some local tree with fixes myself and move away from
basing my work on git master...

Attaching Avi's patch for reference.

Kevin

[-- Attachment #2: 0001-block-Clean-up-after-deleting-BHs.patch --]
[-- Type: text/plain, Size: 1174 bytes --]

>From 3458d28d2a1eec3b867f7caa65ba177aa5b77da5 Mon Sep 17 00:00:00 2001
From: Avi Kivity <avi@redhat.com>
Date: Tue, 23 Jun 2009 16:20:36 +0300
Subject: [PATCH] block: Clean up after deleting BHs

Commit 6a7ad299 ("Call qemu_bh_delete at bdrv_aio_bh_cb") deletes emulated
aio bottom halves to prevent endless accumulation.  However, it leaves a
stale ->bh pointer, which is then waited on when the aio is reused.

Zeroing the pointer fixes the issue, allowing vmdk format images to be used.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 block.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index aca5a6d..cefbe77 100644
--- a/block.c
+++ b/block.c
@@ -1374,6 +1374,7 @@ static void bdrv_aio_cancel_em(BlockDriverAIOCB *blockacb)
 {
     BlockDriverAIOCBSync *acb = (BlockDriverAIOCBSync *)blockacb;
     qemu_bh_delete(acb->bh);
+    acb->bh = NULL;
     qemu_aio_release(acb);
 }
 
@@ -1391,6 +1392,7 @@ static void bdrv_aio_bh_cb(void *opaque)
     qemu_vfree(acb->bounce);
     acb->common.cb(acb->common.opaque, acb->ret);
     qemu_bh_delete(acb->bh);
+    acb->bh = NULL;
     qemu_aio_release(acb);
 }
 
-- 
1.6.0.6

Re: [Qemu-devel] [PATCH] qemu-iotests: make a few more tests generic

[Christoph Hellwig]

On Fri, Jul 10, 2009 at 09:47:54AM +0200, Kevin Wolf wrote:
> Christoph Hellwig schrieb:
> > On Thu, Jul 09, 2009 at 11:58:58AM +0200, Kevin Wolf wrote:
> >> Christoph Hellwig schrieb:
> >>> Pretend that a non-implemented check is always successful and thus allow
> >>> various tests that were qcow2-specific before to be generic.
> >> Looks good in general. However, vmdk and vpc fail test case 011 for me.
> >> Not sure yet where the problem actually is, there seems to be no qemu-io
> >> output at all for these.
> > 
> > They pass for me.  So I guess we have a problem somewhere that we'll
> > eventually need to investigate.
> 
> This problem has already be found. Avi's patch from almost three weeks
> ago fixes it.

Ah, that explains why I don't see it.  I kept my local alternative fix
around in my quilt stack.

> It really starts to get annoying. How am I supposed to work with commits
> only every other week (which is bad enough) and then patches are
> forgotten and probably won't be merged before another two weeks? I guess
> I should manage some local tree with fixes myself and move away from
> basing my work on git master...

Yeah, working on qemu has been quite a challenge for me due to this,
too.  We can try to setup some sort of staging development tree.

Re: [Qemu-devel] [PATCH] qemu-iotests: make a few more tests generic

[Anthony Liguori]

Kevin Wolf wrote:
> This problem has already be found. Avi's patch from almost three weeks
> ago fixes it.
>   

Christoph posted an alternative patch and there didn't seem to be a 
consensus on the thread about what solution was the best one.  See 
http://article.gmane.org/gmane.comp.emulators.qemu/46032/match=block+clean+up+after.  
It's also up to the submitter to keep track of their patches.  If they 
think one should be applied that hasn't been, they need to follow up on 
it.  The only way to scale here is to push as much work as possible to 
the outer-most nodes.

> It really starts to get annoying. How am I supposed to work with commits
> only every other week (which is bad enough) and then patches are
> forgotten and probably won't be merged before another two weeks? I guess
> I should manage some local tree with fixes myself and move away from
> basing my work on git master...
>   

Complaining is all well and good but it doesn't help the problem.  Your 
particular compliant is about a patch that fixes a problem another patch 
introduced.  Greater patch velocity == greater instability because there 
isn't an adequate amount of testing going on.  It takes time to do 
proper testing and review of patches.

It would be very helpful if you proactively tested/reviewed patches on 
the mailing list and then commented appropriately.  People adding 
Tested-by tags to patches on the list would be a great help for instance.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] qemu-iotests: make a few more tests generic

[Kevin Wolf]

Anthony Liguori schrieb:
> Kevin Wolf wrote:
>> This problem has already be found. Avi's patch from almost three weeks
>> ago fixes it.
>>   
> 
> Christoph posted an alternative patch and there didn't seem to be a 
> consensus on the thread about what solution was the best one.  See 
> http://article.gmane.org/gmane.comp.emulators.qemu/46032/match=block+clean+up+after.  

I know. I really don't care which one is used, but I want to have that
bug fixed.

> It's also up to the submitter to keep track of their patches.  If they 
> think one should be applied that hasn't been, they need to follow up on 
> it.  The only way to scale here is to push as much work as possible to 
> the outer-most nodes.

We can try to help here, but we need to know what you want the process
to look like. When do you want us to resend patches? When no patches
have been submitted for two weeks, I hesitate to resend a patch that is
only a week old. You don't want to be flooded with resends of patches
that you have already taken care of or that you have planned to act on
later, right?

>> It really starts to get annoying. How am I supposed to work with commits
>> only every other week (which is bad enough) and then patches are
>> forgotten and probably won't be merged before another two weeks? I guess
>> I should manage some local tree with fixes myself and move away from
>> basing my work on git master...
> 
> Complaining is all well and good but it doesn't help the problem.  Your 
> particular compliant is about a patch that fixes a problem another patch 
> introduced.  Greater patch velocity == greater instability because there 
> isn't an adequate amount of testing going on.  It takes time to do 
> proper testing and review of patches.

Well, yes and no. If a few patches come into git master each day and a
patch breaks something (and the breakage is really annoying), we'll most
probably have the fix on the next day. On the other hand, if the patches
are two weeks in your queue first, maybe it's a bit less likely that
they break things, but if they do the breakage persists until the next
big patch round comes two weeks later.

Also with each day of patches not committed we're more and more likely
to post patches that apply on master but conflict with your queue.

I would really like to see your queue flushed more often (it has been
flushed more frequently in the past, so why did this change in the first
place?)

> It would be very helpful if you proactively tested/reviewed patches on 
> the mailing list and then commented appropriately.  People adding 
> Tested-by tags to patches on the list would be a great help for instance.

I'm trying to do my share here. If some formalized things like Acked-by
or Tested-by tags are more helpful than "Looks good to me" comments, let
me know.

Kevin

Re: [Qemu-devel] [PATCH] qemu-iotests: make a few more tests generic

[Christoph Hellwig]

On Fri, Jul 10, 2009 at 07:50:16AM -0500, Anthony Liguori wrote:
> Christoph posted an alternative patch and there didn't seem to be a 
> consensus on the thread about what solution was the best one.  See 
> http://article.gmane.org/gmane.comp.emulators.qemu/46032/match=block+clean+up+after.  

either one is much much better than nothing.

> It's also up to the submitter to keep track of their patches.  If they 
> think one should be applied that hasn't been, they need to follow up on it. 
> The only way to scale here is to push as much work as possible to the 
> outer-most nodes.

It's just really hard to track stuff without feedback.  You only flush
your queue very sproadicly, so some kind of indicator that it is in the
queue would be extremly helpful.  Just an ok, I'll queue it to mean that
it's not lost means we know it's been deal with in some way, and we can
ping patches that haven't gotten any feedback for say a week.

Re: [Qemu-devel] [PATCH] qemu-iotests: make a few more tests generic

[Anthony Liguori]

Christoph Hellwig wrote:
>> It's also up to the submitter to keep track of their patches.  If they 
>> think one should be applied that hasn't been, they need to follow up on it. 
>> The only way to scale here is to push as much work as possible to the 
>> outer-most nodes.
>>     
>
> It's just really hard to track stuff without feedback.  You only flush
> your queue very sproadicly, so some kind of indicator that it is in the
> queue would be extremly helpful.  Just an ok, I'll queue it to mean that
> it's not lost means we know it's been deal with in some way, and we can
> ping patches that haven't gotten any feedback for say a week.
>   

What about akpm-style automated notifications that a patch has been 
queued and/or rejected from queue?  These would just go to the sender, 
not the list.

It wouldn't be very hard for me to add this.

And yeah, more frequent queue flushing is a separate problem.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] qemu-iotests: make a few more tests generic

[Christoph Hellwig]

On Fri, Jul 10, 2009 at 08:39:37AM -0500, Anthony Liguori wrote:
> What about akpm-style automated notifications that a patch has been 
> queued and/or rejected from queue?  These would just go to the sender, 
> not the list.

That would work for me.