[Qemu-devel] active block commit bug?

[Qemu-devel] active block commit bug?

[Eric Blake]

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

Testing on Fedora 20 (JSON output slightly modified for legibility):

$ qemu-kvm --version
QEMU emulator version 2.0.0, Copyright (c) 2003-2008 Fabrice Bellard
$ touch f
$ qemu-kvm -qmp stdio -drive file=f
 <= {"QMP": {"version": {"qemu": {"micro": 0, "minor": 0, "major": 2},
"package": ""}, "capabilities": []}}

{"execute":"qmp_capabilities"}
 <= {"return": {}}

{"execute":"blockdev-snapshot-sync","arguments":{"device":"ide0-hd0","snapshot-file":"g"}}
Formatting 'g', fmt=qcow2 size=0 backing_file='f' backing_fmt='raw'
encryption=off cluster_size=65536 lazy_refcounts=off
 <= {"return": {}}

{"execute":"block-commit","arguments":{"device":"ide0-hd0","top":"g"}}
{"timestamp": {"seconds": 1401921011, "microseconds": 498888}, "event":
"BLOCK_JOB_COMPLETED", "data": {"device": "ide0-hd0", "len": 0,
"offset": 0, "speed": 0, "type": "commit"}}
 <= {"return": {}}

{"execute":"query-block-jobs"}
 <= {"return": []}

Huh? I thought that an active commit was not supposed to complete
automatically, but that the job would remain around until I either
'block-job-cancel' or 'block-job-complete' it.  That is, I should have
gotten a BLOCK_JOB_READY event and still see the job when I query for
it.  Where am I going wrong, or did I uncover a bug in active commit?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] active block commit bug?

[Jeff Cody]

On Wed, Jun 04, 2014 at 04:55:04PM -0600, Eric Blake wrote:
> Testing on Fedora 20 (JSON output slightly modified for legibility):
> 
> $ qemu-kvm --version
> QEMU emulator version 2.0.0, Copyright (c) 2003-2008 Fabrice Bellard
> $ touch f
> $ qemu-kvm -qmp stdio -drive file=f
>  <= {"QMP": {"version": {"qemu": {"micro": 0, "minor": 0, "major": 2},
> "package": ""}, "capabilities": []}}
> 
> {"execute":"qmp_capabilities"}
>  <= {"return": {}}
> 
> {"execute":"blockdev-snapshot-sync","arguments":{"device":"ide0-hd0","snapshot-file":"g"}}
> Formatting 'g', fmt=qcow2 size=0 backing_file='f' backing_fmt='raw'
> encryption=off cluster_size=65536 lazy_refcounts=off
>  <= {"return": {}}
> 
> {"execute":"block-commit","arguments":{"device":"ide0-hd0","top":"g"}}
> {"timestamp": {"seconds": 1401921011, "microseconds": 498888}, "event":
> "BLOCK_JOB_COMPLETED", "data": {"device": "ide0-hd0", "len": 0,
> "offset": 0, "speed": 0, "type": "commit"}}
>  <= {"return": {}}
> 
> {"execute":"query-block-jobs"}
>  <= {"return": []}
> 
> Huh? I thought that an active commit was not supposed to complete
> automatically, but that the job would remain around until I either
> 'block-job-cancel' or 'block-job-complete' it.  That is, I should have
> gotten a BLOCK_JOB_READY event and still see the job when I query for
> it.  Where am I going wrong, or did I uncover a bug in active commit?
>

I tried repeating your findings, but I couldn't, until I noticed that
'f' was just a 0-length raw image in your test.

The snapshot file will be the same size, 0.  So when we go to perform
the active commit, we short-circuit at the beginning, since we are
committing a zero-length image:

from block/mirror.c (active commit is a special mirror case):

 static void coroutine_fn mirror_run(void *opaque)
 {
     MirrorBlockJob *s = opaque;
     BlockDriverState *bs = s->common.bs;
     int64_t sector_num, end, sectors_per_chunk, length;
     uint64_t last_pause_ns;
     BlockDriverInfo bdi;
     char backing_filename[1024];
     int ret = 0;
     int n;
 
     if (block_job_is_cancelled(&s->common)) {
         goto immediate_exit;
     }
 
     s->common.len = bdrv_getlength(bs);
     if (s->common.len <= 0) {
         block_job_completed(&s->common, s->common.len);
         return;
     }
     ^^^^^
     we exit early here, with a completed message, since there is
     nothing to do.

If 'g' had increased to non-zero size, then you would have received a
BLOCK_JOB_READY instead.


-Jeff

Re: [Qemu-devel] active block commit bug?

[Eric Blake]

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

On 06/04/2014 06:12 PM, Jeff Cody wrote:

>> Huh? I thought that an active commit was not supposed to complete
>> automatically, but that the job would remain around until I either
>> 'block-job-cancel' or 'block-job-complete' it.  That is, I should have
>> gotten a BLOCK_JOB_READY event and still see the job when I query for
>> it.  Where am I going wrong, or did I uncover a bug in active commit?
>>
> 
> I tried repeating your findings, but I couldn't, until I noticed that
> 'f' was just a 0-length raw image in your test.
> 
> The snapshot file will be the same size, 0.  So when we go to perform
> the active commit, we short-circuit at the beginning, since we are
> committing a zero-length image:

That explains it.

>  
>      s->common.len = bdrv_getlength(bs);
>      if (s->common.len <= 0) {
>          block_job_completed(&s->common, s->common.len);
>          return;
>      }
>      ^^^^^
>      we exit early here, with a completed message, since there is
>      nothing to do.
> 
> If 'g' had increased to non-zero size, then you would have received a
> BLOCK_JOB_READY instead.

Sounds like we have an off-by-one condition if empty files behave
differently from other files.  We ought to fix that bug (not that your
normal guest will ever have a 0-length backing file, but this was what I
was trying to use for libvirt's probing of whether active commit is
supported)

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] active block commit bug?

[Fam Zheng]

On Wed, 06/04 19:54, Eric Blake wrote:
> On 06/04/2014 06:12 PM, Jeff Cody wrote:
> 
> >> Huh? I thought that an active commit was not supposed to complete
> >> automatically, but that the job would remain around until I either
> >> 'block-job-cancel' or 'block-job-complete' it.  That is, I should have
> >> gotten a BLOCK_JOB_READY event and still see the job when I query for
> >> it.  Where am I going wrong, or did I uncover a bug in active commit?
> >>
> > 
> > I tried repeating your findings, but I couldn't, until I noticed that
> > 'f' was just a 0-length raw image in your test.
> > 
> > The snapshot file will be the same size, 0.  So when we go to perform
> > the active commit, we short-circuit at the beginning, since we are
> > committing a zero-length image:
> 
> That explains it.
> 
> >  
> >      s->common.len = bdrv_getlength(bs);
> >      if (s->common.len <= 0) {
> >          block_job_completed(&s->common, s->common.len);
> >          return;
> >      }
> >      ^^^^^
> >      we exit early here, with a completed message, since there is
> >      nothing to do.
> > 
> > If 'g' had increased to non-zero size, then you would have received a
> > BLOCK_JOB_READY instead.
> 
> Sounds like we have an off-by-one condition if empty files behave
> differently from other files.  We ought to fix that bug (not that your
> normal guest will ever have a 0-length backing file, but this was what I
> was trying to use for libvirt's probing of whether active commit is
> supported)
> 

Yes, agreed, this special case is only going to make management confused. I
will send a patch to fix this.

Eric, is this a good way to probe the active commit? I was expecting full
instrospection of QMP could do it, but I don't know about the status of that
piece of work. Amos, any ideas?

Fam

Re: [Qemu-devel] active block commit bug?

[Eric Blake]

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

On 06/04/2014 08:09 PM, Fam Zheng wrote:

>> Sounds like we have an off-by-one condition if empty files behave
>> differently from other files.  We ought to fix that bug (not that your
>> normal guest will ever have a 0-length backing file, but this was what I
>> was trying to use for libvirt's probing of whether active commit is
>> supported)
>>
> 
> Yes, agreed, this special case is only going to make management confused. I
> will send a patch to fix this.

Thanks.

> 
> Eric, is this a good way to probe the active commit? I was expecting full
> instrospection of QMP could do it, but I don't know about the status of that
> piece of work. Amos, any ideas?

Introspection already missed qemu 2.0 when active commit was added; and
we're close enough to soft freeze for 2.1 that I'm guessing it will miss
2.1 as well :(

So yes, I'm experimenting with how to learn if active commit works by
seeing what error message differences I can trigger with minimum effort;
libvirt will cache what it learns so that it only has to ask once per
qemu binary/timestamp, then let the user know up front whether active
commit will work.  Since there are existing qemu versions that have
active commit but not introspection, I'm stuck using this harder probe
to avoid a false negative for those older qemu.  My other option is to
just wait for introspection, or even something intermediate like Jeff's
patch to make 'top' optional, and just declare qemu 2.0 active commit as
not working - but since it is only the special case of a 0-size file
(which is fairly unlikely for any real client use, and certainly
something I can avoid in the libvirt probing), it feels a bit harsh to
reject 2.0 just for this corner-case bug.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] active block commit bug?

[Fam Zheng]

On Wed, 06/04 20:55, Eric Blake wrote:
> On 06/04/2014 08:09 PM, Fam Zheng wrote:
> 
> >> Sounds like we have an off-by-one condition if empty files behave
> >> differently from other files.  We ought to fix that bug (not that your
> >> normal guest will ever have a 0-length backing file, but this was what I
> >> was trying to use for libvirt's probing of whether active commit is
> >> supported)
> >>
> > 
> > Yes, agreed, this special case is only going to make management confused. I
> > will send a patch to fix this.
> 
> Thanks.
> 
> > 
> > Eric, is this a good way to probe the active commit? I was expecting full
> > instrospection of QMP could do it, but I don't know about the status of that
> > piece of work. Amos, any ideas?
> 
> Introspection already missed qemu 2.0 when active commit was added; and
> we're close enough to soft freeze for 2.1 that I'm guessing it will miss
> 2.1 as well :(
> 
> So yes, I'm experimenting with how to learn if active commit works by
> seeing what error message differences I can trigger with minimum effort;
> libvirt will cache what it learns so that it only has to ask once per
> qemu binary/timestamp, then let the user know up front whether active
> commit will work.  Since there are existing qemu versions that have
> active commit but not introspection, I'm stuck using this harder probe
> to avoid a false negative for those older qemu.  My other option is to
> just wait for introspection, or even something intermediate like Jeff's
> patch to make 'top' optional, and just declare qemu 2.0 active commit as
> not working - but since it is only the special case of a 0-size file
> (which is fairly unlikely for any real client use, and certainly
> something I can avoid in the libvirt probing), it feels a bit harsh to
> reject 2.0 just for this corner-case bug.
> 

Thanks, I'll keep in mind the feature probing necessity in the future.

Fam

Re: [Qemu-devel] active block commit bug?

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 06/04/2014 08:09 PM, Fam Zheng wrote:
>
>>> Sounds like we have an off-by-one condition if empty files behave
>>> differently from other files.  We ought to fix that bug (not that your
>>> normal guest will ever have a 0-length backing file, but this was what I
>>> was trying to use for libvirt's probing of whether active commit is
>>> supported)
>>>
>> 
>> Yes, agreed, this special case is only going to make management confused. I
>> will send a patch to fix this.
>
> Thanks.
>
>> 
>> Eric, is this a good way to probe the active commit? I was expecting full
>> instrospection of QMP could do it, but I don't know about the status of that
>> piece of work. Amos, any ideas?
>
> Introspection already missed qemu 2.0 when active commit was added; and
> we're close enough to soft freeze for 2.1 that I'm guessing it will miss
> 2.1 as well :(

Almost certainly.  It has non-trivial design issues.  To have a chance
to make it into 2.x, it needs to be posted for review early in the 2.x
cycle.

[...]

Re: [Qemu-devel] active block commit bug?

[Kevin Wolf]

Am 05.06.2014 um 09:06 hat Markus Armbruster geschrieben:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 06/04/2014 08:09 PM, Fam Zheng wrote:
> >
> >>> Sounds like we have an off-by-one condition if empty files behave
> >>> differently from other files.  We ought to fix that bug (not that your
> >>> normal guest will ever have a 0-length backing file, but this was what I
> >>> was trying to use for libvirt's probing of whether active commit is
> >>> supported)
> >>>
> >> 
> >> Yes, agreed, this special case is only going to make management confused. I
> >> will send a patch to fix this.
> >
> > Thanks.
> >
> >> 
> >> Eric, is this a good way to probe the active commit? I was expecting full
> >> instrospection of QMP could do it, but I don't know about the status of that
> >> piece of work. Amos, any ideas?
> >
> > Introspection already missed qemu 2.0 when active commit was added; and
> > we're close enough to soft freeze for 2.1 that I'm guessing it will miss
> > 2.1 as well :(
> 
> Almost certainly.  It has non-trivial design issues.  To have a chance
> to make it into 2.x, it needs to be posted for review early in the 2.x
> cycle.

Why is nobody discussing these non-trivial design issues then? Patches
have been posted even in a 1.x development phase, so that's not what is
blocking us. If anything, it's the missing discussion.

Kevin

Re: [Qemu-devel] active block commit bug?

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 05.06.2014 um 09:06 hat Markus Armbruster geschrieben:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>> > On 06/04/2014 08:09 PM, Fam Zheng wrote:
>> >
>> >>> Sounds like we have an off-by-one condition if empty files behave
>> >>> differently from other files.  We ought to fix that bug (not that your
>> >>> normal guest will ever have a 0-length backing file, but this was what I
>> >>> was trying to use for libvirt's probing of whether active commit is
>> >>> supported)
>> >>>
>> >> 
>> >> Yes, agreed, this special case is only going to make management confused. I
>> >> will send a patch to fix this.
>> >
>> > Thanks.
>> >
>> >> 
>> >> Eric, is this a good way to probe the active commit? I was expecting full
>> >> instrospection of QMP could do it, but I don't know about the
>> >> status of that
>> >> piece of work. Amos, any ideas?
>> >
>> > Introspection already missed qemu 2.0 when active commit was added; and
>> > we're close enough to soft freeze for 2.1 that I'm guessing it will miss
>> > 2.1 as well :(
>> 
>> Almost certainly.  It has non-trivial design issues.  To have a chance
>> to make it into 2.x, it needs to be posted for review early in the 2.x
>> cycle.
>
> Why is nobody discussing these non-trivial design issues then? Patches
> have been posted even in a 1.x development phase, so that's not what is
> blocking us. If anything, it's the missing discussion.

If I remember correctly, review of the patches made us realize
introspection is more complex than initially thought.  We discussed a
proper solution at some length, but concluded implementing it in time
for 2.0 was not in the cards (we were running close to the hard freeze
already).  Unfortunately, nobody has stepped up to implement it in time
for 2.1 either.

To make further progress, we need a sucker^Wvolunteer pushing the
feature.

Re: [Qemu-devel] active block commit bug?

[Amos Kong]

On Thu, Jun 05, 2014 at 09:06:42AM +0200, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 06/04/2014 08:09 PM, Fam Zheng wrote:
> >
> >>> Sounds like we have an off-by-one condition if empty files behave
> >>> differently from other files.  We ought to fix that bug (not that your
> >>> normal guest will ever have a 0-length backing file, but this was what I
> >>> was trying to use for libvirt's probing of whether active commit is
> >>> supported)
> >>>
> >> 
> >> Yes, agreed, this special case is only going to make management confused. I
> >> will send a patch to fix this.
> >
> > Thanks.
> >
> >> 
> >> Eric, is this a good way to probe the active commit? I was expecting full
> >> instrospection of QMP could do it, but I don't know about the status of that
> >> piece of work. Amos, any ideas?
> >
> > Introspection already missed qemu 2.0 when active commit was added; and
> > we're close enough to soft freeze for 2.1 that I'm guessing it will miss
> > 2.1 as well :(
> 
> Almost certainly.  It has non-trivial design issues.  To have a chance
> to make it into 2.x, it needs to be posted for review early in the 2.x
> cycle.

I already started to respin the new version, let's see the new version
next week.
 
> [...]

-- 
			Amos.