[Qemu-devel] Supporting unsafe create when backing file is not accessible

[Qemu-devel] Supporting unsafe create when backing file is not accessible

[Ala Hino]

Hi,

We encountered a performance issue when creating a volume for a running VM
and we'd like to share the info with you. The root cause of the issue is in
our code but we found a workaround that relies on qemu-img create
undocumented behavior.

During our tests, we found that in order to create a volume with a backing
file, the baking file has to be valid and accessible. This requires us to
activate the entire chain before creating the volume, and deactivate the
chain after the operation completes. Activating and deactivating the chain
are expensive operations that we prefer to avoid when possible. Below is
the command we use to create volumes:

qemu-img create -f qcow2 -o compat=1.1 -b
8a28cda2-548d-47da-bbba-faa81284f6ba -F raw
/rhev/data-center/e6b272af-84cb-43fc-ae5b-7fe18bc469a2/f47c980a-fd76-44a9-8b78-00d3ab2ffd2f/images/2ff0a3c0-f145-4f83-b668-fc0c39ba191f/d3b69657-892f-499c-9ac3-9c443ead7d9b
1073741824

We also found that when providing the size and the backing format for qemu,
qemu doesn't open the backing chain, and in this case we don't have to
activate/deactivate the entire chain - exactly the behavior we wish to have.
We we'd like to get your confirmation of the above behavior as it isn't
documented, and whether it can be documented.

In addition, we are aware of https://bugzilla.redhat.com/1213786, where a
-u (unsafe) option is planned to be added (see comment #4 in the BZ). Can
you please confirm that once that support is released, it won't break
existing, i.e. code that provides size and backing format assuming that
"unsafe" create is supported?

Thanks,
Ala

Re: [Qemu-devel] Supporting unsafe create when backing file is not accessible

[Eric Blake]

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

On 07/12/2017 03:56 AM, Ala Hino wrote:
> Hi,
> 
> We encountered a performance issue when creating a volume for a running VM
> and we'd like to share the info with you. The root cause of the issue is in
> our code but we found a workaround that relies on qemu-img create
> undocumented behavior.
> 
> During our tests, we found that in order to create a volume with a backing
> file, the baking file has to be valid and accessible.

In general, that's a good thing. But you're also right that it is nice
to have a mode where the backing file is not probed.

> This requires us to
> activate the entire chain before creating the volume, and deactivate the
> chain after the operation completes. Activating and deactivating the chain
> are expensive operations that we prefer to avoid when possible. Below is
> the command we use to create volumes:
> 
> qemu-img create -f qcow2 -o compat=1.1 -b
> 8a28cda2-548d-47da-bbba-faa81284f6ba -F raw
> /rhev/data-center/e6b272af-84cb-43fc-ae5b-7fe18bc469a2/f47c980a-fd76-44a9-8b78-00d3ab2ffd2f/images/2ff0a3c0-f145-4f83-b668-fc0c39ba191f/d3b69657-892f-499c-9ac3-9c443ead7d9b
> 1073741824
> 
> We also found that when providing the size and the backing format for qemu,
> qemu doesn't open the backing chain, and in this case we don't have to
> activate/deactivate the entire chain - exactly the behavior we wish to have.

Yes, that is currently the case. You are correct that patches have been
proposed to tighten things so that we would probe the existence of the
backing file in more cases (even when the size is provided); and that if
we do so, we'd also have to provide a backdoor for creating an image
without probing the backing file.  But it is also the case that you can
create an image with NO backing file, and then use 'qemu-img rebase -u'
to add the backing file after the fact, without waiting for any proposed
patches to land.

> We we'd like to get your confirmation of the above behavior as it isn't
> documented, and whether it can be documented.

The fact that you are asking does mean that we should revive John's
proposed patches, in some form or another.

> 
> In addition, we are aware of https://bugzilla.redhat.com/1213786, where a
> -u (unsafe) option is planned to be added (see comment #4 in the BZ). Can
> you please confirm that once that support is released, it won't break
> existing, i.e. code that provides size and backing format assuming that
> "unsafe" create is supported?

If we tighten things to require the backing file to exist unless -u is
provided, then providing the size alone will no longer be sufficient to
prevent the probe - you'd have to use -u to prevent the probe, or change
your workflow to create the image without a backing file then add in the
backing information via 'qemu-img rebase -u'.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] Supporting unsafe create when backing file is not accessible

[Nir Soffer]

On Wed, Jul 12, 2017 at 5:40 PM Eric Blake <eblake@redhat.com> wrote:

> On 07/12/2017 03:56 AM, Ala Hino wrote:
> > Hi,
> >
> > We encountered a performance issue when creating a volume for a running
> VM
> > and we'd like to share the info with you. The root cause of the issue is
> in
> > our code but we found a workaround that relies on qemu-img create
> > undocumented behavior.
> >
> > During our tests, we found that in order to create a volume with a
> backing
> > file, the baking file has to be valid and accessible.
>
> In general, that's a good thing. But you're also right that it is nice
> to have a mode where the backing file is not probed.
>
> > This requires us to
> > activate the entire chain before creating the volume, and deactivate the
> > chain after the operation completes. Activating and deactivating the
> chain
> > are expensive operations that we prefer to avoid when possible. Below is
> > the command we use to create volumes:
> >
> > qemu-img create -f qcow2 -o compat=1.1 -b
> > 8a28cda2-548d-47da-bbba-faa81284f6ba -F raw
> >
> /rhev/data-center/e6b272af-84cb-43fc-ae5b-7fe18bc469a2/f47c980a-fd76-44a9-8b78-00d3ab2ffd2f/images/2ff0a3c0-f145-4f83-b668-fc0c39ba191f/d3b69657-892f-499c-9ac3-9c443ead7d9b
> > 1073741824
> >
> > We also found that when providing the size and the backing format for
> qemu,
> > qemu doesn't open the backing chain, and in this case we don't have to
> > activate/deactivate the entire chain - exactly the behavior we wish to
> have.
>
> Yes, that is currently the case. You are correct that patches have been
> proposed to tighten things so that we would probe the existence of the
> backing file in more cases (even when the size is provided); and that if
> we do so, we'd also have to provide a backdoor for creating an image
> without probing the backing file.  But it is also the case that you can
> create an image with NO backing file, and then use 'qemu-img rebase -u'
> to add the backing file after the fact, without waiting for any proposed
> patches to land.
>
> > We we'd like to get your confirmation of the above behavior as it isn't
> > documented, and whether it can be documented.
>
> The fact that you are asking does mean that we should revive John's
> proposed patches, in some form or another.
>

Eric, we are more concerned about using the current qemu version.

We can use the fact that providing both size and backing format,
qemu does not open the backing file, but this is not documented, and
we don't want to base oVirt code on undocumented behavior.

What we would like to have is:
- qemu blessing for using this undocumented behaviour
- and documenting this behavior in qemu-img(1)

With this we can fix https://bugzilla.redhat.com/1395941
<https://bugzilla.redhat.com/show_bug.cgi?id=1395941>
now, instead of waiting for qemu 2.10.

For future version, having a explicit way to allow unsafe create
is of course better.

Nir

>
> > In addition, we are aware of https://bugzilla.redhat.com/1213786, where
> a
> > -u (unsafe) option is planned to be added (see comment #4 in the BZ). Can
> > you please confirm that once that support is released, it won't break
> > existing, i.e. code that provides size and backing format assuming that
> > "unsafe" create is supported?
>
> If we tighten things to require the backing file to exist unless -u is
> provided, then providing the size alone will no longer be sufficient to
> prevent the probe - you'd have to use -u to prevent the probe, or change
> your workflow to create the image without a backing file then add in the
> backing information via 'qemu-img rebase -u'.
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266 <(919)%20301-3266>
> Virtualization:  qemu.org | libvirt.org
>
>

Re: [Qemu-devel] Supporting unsafe create when backing file is not accessible

[Kevin Wolf]

Am 14.07.2017 um 21:13 hat Nir Soffer geschrieben:
> On Wed, Jul 12, 2017 at 5:40 PM Eric Blake <eblake@redhat.com> wrote:
> 
> > On 07/12/2017 03:56 AM, Ala Hino wrote:
> > > Hi,
> > >
> > > We encountered a performance issue when creating a volume for a running
> > VM
> > > and we'd like to share the info with you. The root cause of the issue is
> > in
> > > our code but we found a workaround that relies on qemu-img create
> > > undocumented behavior.
> > >
> > > During our tests, we found that in order to create a volume with a
> > backing
> > > file, the baking file has to be valid and accessible.
> >
> > In general, that's a good thing. But you're also right that it is nice
> > to have a mode where the backing file is not probed.
> >
> > > This requires us to
> > > activate the entire chain before creating the volume, and deactivate the
> > > chain after the operation completes. Activating and deactivating the
> > chain
> > > are expensive operations that we prefer to avoid when possible. Below is
> > > the command we use to create volumes:
> > >
> > > qemu-img create -f qcow2 -o compat=1.1 -b
> > > 8a28cda2-548d-47da-bbba-faa81284f6ba -F raw
> > >
> > /rhev/data-center/e6b272af-84cb-43fc-ae5b-7fe18bc469a2/f47c980a-fd76-44a9-8b78-00d3ab2ffd2f/images/2ff0a3c0-f145-4f83-b668-fc0c39ba191f/d3b69657-892f-499c-9ac3-9c443ead7d9b
> > > 1073741824
> > >
> > > We also found that when providing the size and the backing format for
> > qemu,
> > > qemu doesn't open the backing chain, and in this case we don't have to
> > > activate/deactivate the entire chain - exactly the behavior we wish to
> > have.
> >
> > Yes, that is currently the case. You are correct that patches have been
> > proposed to tighten things so that we would probe the existence of the
> > backing file in more cases (even when the size is provided); and that if
> > we do so, we'd also have to provide a backdoor for creating an image
> > without probing the backing file.  But it is also the case that you can
> > create an image with NO backing file, and then use 'qemu-img rebase -u'
> > to add the backing file after the fact, without waiting for any proposed
> > patches to land.
> >
> > > We we'd like to get your confirmation of the above behavior as it isn't
> > > documented, and whether it can be documented.
> >
> > The fact that you are asking does mean that we should revive John's
> > proposed patches, in some form or another.
> >
> 
> Eric, we are more concerned about using the current qemu version.
> 
> We can use the fact that providing both size and backing format,
> qemu does not open the backing file, but this is not documented, and
> we don't want to base oVirt code on undocumented behavior.
> 
> What we would like to have is:
> - qemu blessing for using this undocumented behaviour
> - and documenting this behavior in qemu-img(1)

You already asked me whether you could rely on this behaviour in a
private email and my answer was a pretty clear no.

I'm not sure what you're hoping to get from asking someone else for the
same thing, but if you want an accurate answer, it won't change.

We won't document this undocumented behaviour because it isn't
intentional in the first place. It is simply a missing check of user
input in qemu-img.

> With this we can fix https://bugzilla.redhat.com/1395941
> <https://bugzilla.redhat.com/show_bug.cgi?id=1395941>
> now, instead of waiting for qemu 2.10.

Upstream doesn't compromise on interfaces to make life more convenient
for downstreams. The correct procedure here is to get a -u added for
2.10 and then backport it into the downstream.

Kevin

Re: [Qemu-devel] Supporting unsafe create when backing file is not accessible

[Eric Blake]

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

On 07/14/2017 02:13 PM, Nir Soffer wrote:

> Eric, we are more concerned about using the current qemu version.
> 
> We can use the fact that providing both size and backing format,
> qemu does not open the backing file, but this is not documented, and
> we don't want to base oVirt code on undocumented behavior.
> 
> What we would like to have is:
> - qemu blessing for using this undocumented behaviour

But how are you going to get that? By the time someone writes a
documentation patch, it won't land until qemu 2.10, but by then, they
might as well have written the -u patch instead.

Older versions will continue to have the older behavior, unless someone
backports a patch to give them the newer behavior - but if someone is
backporting -u, presumably they will also backport whatever
introspection mechanisms you would also use against upstream qemu to
learn if -u is present.  If the introspection mechanism is not present
or gives the answer that -u is not present, then you can safely assume
the old behavior (because the new behavior is only going to be present
intentionally).  But that is STILL not something that we are going to
explicitly document, because it makes more sense to implement the
working feature than to document the workaround to a missing feature.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] Supporting unsafe create when backing file is not accessible

[Kevin Wolf]

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

Am 17.07.2017 um 13:36 hat Eric Blake geschrieben:
> On 07/14/2017 02:13 PM, Nir Soffer wrote:
> > Eric, we are more concerned about using the current qemu version.
> > 
> > We can use the fact that providing both size and backing format,
> > qemu does not open the backing file, but this is not documented, and
> > we don't want to base oVirt code on undocumented behavior.
> > 
> > What we would like to have is:
> > - qemu blessing for using this undocumented behaviour
> 
> But how are you going to get that? By the time someone writes a
> documentation patch, it won't land until qemu 2.10, but by then, they
> might as well have written the -u patch instead.

In fact, soft freeze is tomorrow, so if we want to have -u in 2.10, it
would be good to have a patch ready today.

> Older versions will continue to have the older behavior, unless someone
> backports a patch to give them the newer behavior - but if someone is
> backporting -u, presumably they will also backport whatever
> introspection mechanisms you would also use against upstream qemu to
> learn if -u is present.  If the introspection mechanism is not present
> or gives the answer that -u is not present, then you can safely assume
> the old behavior (because the new behavior is only going to be present
> intentionally).  But that is STILL not something that we are going to
> explicitly document, because it makes more sense to implement the
> working feature than to document the workaround to a missing feature.

While I think adding -u today is reasonably realistic, I'm doubtful that
we can get an introspection mechanism in place today. Perhaps we can
declare it a bug fix, but I'd rather not rush something like that.

How does libvirt detect qemu-img features today? Just try and then check
the error message?

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] Supporting unsafe create when backing file is not accessible

[Eric Blake]

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

On 07/17/2017 07:19 AM, Kevin Wolf wrote:

> While I think adding -u today is reasonably realistic, I'm doubtful that
> we can get an introspection mechanism in place today. Perhaps we can
> declare it a bug fix, but I'd rather not rush something like that.
> 
> How does libvirt detect qemu-img features today? Just try and then check
> the error message?

Yeah, when it comes to non-advertised feature detection, the best you
can do is try using the feature, and fall back to the older approach if
the feature was not present (this approach only works for features that
gracefully fail without side effects when attempted on older versions,
but fortunately that's the case for most true feature additions).

And note that creating an image without a backing file, then using
'qemu-img rebase -u' to give it a backing image without probing the
backing image, IS currently documented and supported (so we DO have a
safe fallback, regardless of when 'qemu-img create -u ... size' actually
lands).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] Supporting unsafe create when backing file is not accessible

[Kevin Wolf]

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

Am 17.07.2017 um 14:25 hat Eric Blake geschrieben:
> On 07/17/2017 07:19 AM, Kevin Wolf wrote:
> 
> > While I think adding -u today is reasonably realistic, I'm doubtful that
> > we can get an introspection mechanism in place today. Perhaps we can
> > declare it a bug fix, but I'd rather not rush something like that.
> > 
> > How does libvirt detect qemu-img features today? Just try and then check
> > the error message?
> 
> Yeah, when it comes to non-advertised feature detection, the best you
> can do is try using the feature, and fall back to the older approach if
> the feature was not present (this approach only works for features that
> gracefully fail without side effects when attempted on older versions,
> but fortunately that's the case for most true feature additions).

Yes, for adding a new option like -u this should work.

> And note that creating an image without a backing file, then using
> 'qemu-img rebase -u' to give it a backing image without probing the
> backing image, IS currently documented and supported (so we DO have a
> safe fallback, regardless of when 'qemu-img create -u ... size' actually
> lands).

I thought so, too, but in fact this is not documented behaviour (yet):

   Unsafe mode

   qemu-img uses the unsafe mode if "-u" is specified. In this mode,
   only the backing file name and format of filename is changed without
   any checks on the file contents. The user must take care of
   specifying the correct new backing file, or the guest-visible content
   of the image will be corrupted.

   This mode is useful for renaming or moving the backing file to
   somewhere else.  It can be used without an accessible old backing
   file, i.e. you can use it to fix an image whose backing file has
   already been moved/renamed.

So we explicitly allow the old backing file to be missing, and we
guarantee that rebase -u doesn't check the image content, but we don't
say anything yet about whether the new backing file must exist.

As this is already unsafe mode, I think for rebase we can indeed just
clarify the documentation.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]