Re: [Qemu-block] [PATCH v8 03/26] block: Add BDS.backing_overridden

[Max Reitz]

On 2018-06-26 19:34, Max Reitz wrote:

[...]

> So I suppose I'll rename my BDS.backing_file_canonical attempt to
> BDS.auto_backing_file, and this will be what the image header contains
> (unless we have opened a BDS from it, in which case it will be that
> BDS's filename, so it is canonicalized).

Update: That seems to go down the drain.  We (stupidly) allow taking a
backing filename from the image header and then enriching it with
options from the command line (so you could take null-co:// from the
image header and then the user could give backing.size.  I won't comment
on how little sense that makes (apart from [1]), but in any case, this
means that "we have opened a BDS from it" is very hard to reliably
recognize, because you need to find out whether the user has given any
strong options for the backing file.  That pretty much brings the
complexity back to backing_overridden, I'd say.

Well, or: Whenever the user has given any backing options, the resulting
BDS is categorized as overridden and the filename is not used as a
canonicalized version of auto_backing_file.

I now pretty much fail to see how any of this is better than
backing_overridden, but whatever.

Max

[1] In fact, it's completely broken, because the user-given backing
options only override the backing filename once the user-given options
contain "file.filename".  So you actually cannot really override
anything unless your new backing file has a protocol driver with a
"filename" option.  Well, you can, but that means creating an own BDS
and then giving a reference to that...



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v8 03/26] block: Add BDS.backing_overridden

[Max Reitz]

So, while tackling this once more, this is the current state, in short.

I wanted to add a new field BDS.backing_file_canonical which basically
gets set to bs->backing->bs->filename whenever we know that bs->backing
was opened through bs->backing_file.  That is,
bs->backing_file_canonical would be bs->backing_file, but after going
through the whole hoop of bdrv_open() and bdrv_refresh_filename().

With such a field, we could compare bs->backing->bs->filename against
bs->backing_file_canonical, and thus see whether the backing BDS matches
the image header filename.  I hope this would do the right thing
whenever qemu tries to figure out backing filenames on its own; it will
break, though, when the user just uses some filename and that doesn't
happen to coincide with qemu's generated filenames.

(For instance, if you create a new overlay manually, give it some
backing filename, and that backing filename isn't 1:1 what qemu would
reconstruct; and then you open that file with backing=null, and do a
blockdev-snapshot; then qemu will give you a json:{} filename for the
overlay because due to backing=null, it didn't open the overlay's
backing_file and thus doesn't know how it would look reconstructed.  But
I think that's something we could live with.)

((The only real way to fix this I can imagine is to clone the whole
bdrv_parse_filename()/bdrv_open()/bdrv_refresh_filename()
infrastructure, but just for filenames instead of real BDSs.  Yeah, no,
I won't do that.))

OK, so that was an idea, but now it turns out that bs->backing_file
actually isn't that image header's idea of the backing file.  It appears
to mostly be a clone of bs->backing->bs->filename, but not really.
Honestly, I have no idea what it is, but as a matter of fact, it is
modified by bdrv_backing_attach(), so it will probably usually match
bs->backing->bs->filename (unless there is no bs->backing, in which case
it just retains its old value?).  So it pretty much is completely
useless for any comparison here.

So I suppose I'll rename my BDS.backing_file_canonical attempt to
BDS.auto_backing_file, and this will be what the image header contains
(unless we have opened a BDS from it, in which case it will be that
BDS's filename, so it is canonicalized).

Well, or I could just go with backing_overridden, because honestly that
didn't seem so bad.

Max


PS: Or, we could argue that nobody needs filenames anyway and that
they're just for show and debugging nowadays, so nobody actually needs
backing chain information in them.  May sound a bit stupid, but then
again nobody has ever complained that that's in fact the current state.



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v8 03/26] block: Add BDS.backing_overridden

[Max Reitz]

On 2018-02-22 17:21, Kevin Wolf wrote:
> Am 22.02.2018 um 16:17 hat Max Reitz geschrieben:
>> On 2018-02-22 16:12, Kevin Wolf wrote:
>>> Am 22.02.2018 um 15:55 hat Max Reitz geschrieben:
 On 2018-02-22 14:39, Kevin Wolf wrote:
> Am 05.02.2018 um 16:18 hat Max Reitz geschrieben:
>> If the backing file is overridden, this most probably does change the
>> guest-visible data of a BDS. Therefore, we will need to consider this in
>> bdrv_refresh_filename().
>>
>> Adding a new field to the BDS is not nice, but it is very simple and
>> exactly keeps track of whether the backing file has been overridden.
>
> ...as long as we manage to actually keep it up to date all the time.
>
> First of all, what I'm missing here (or in fact in the comment in the
> code) is a definition what "overridden" really means. "specified by the
> user" is kind of vague: You consider the backing file relationship for
> snapshot=on as user specified, even though the user wasn't explicit
> about this. On the other hand, creating a live snapshot results in a
> node that isn't user specified.
>
> Isn't the real question to ask whether the default backing file (taken
> from the image header) would result in the same tree? The answer to this
> changes after more operations, like qmp_change_backing_file().

 With you so far.

> Considering that there are so many ways to change the answer, I think
> the simplest reliable option isn't a new BDS field that needs to updated
> everywhere, but looking at the current value of bs->options and
> bs->backing_file and see if they match.

 I don't see how that is simple.  First, bs->options does not necessarily
 reflect the "current options", those would be bs->full_open_options.
 And for generating that, we need a way to determine whether the backing
 file has been overridden or not, so whether we need to put the backing
 options into it or whether we do not.
>>>
>>> For the purpose of this comparison, we need a set of options that
>>> contains the backing file options unconditionally.
>>>
 (I am right that bs->backing_file is what the image header says, right?
 So we need to compare it against something that reflects the runtime 
 state.)
>>>
>>> I think so, yes.
>>>
 What I could see would be comparing bs->backing_file to
 bs->backing->bs->filename.  But this sounds very hacky to me.

 One thing the comes to mind is that it can break whenever
 bdrv_refresh_filename() is clever.  So you specify
 'json:{"driver":"null-co"}' in the image header, and
 bdrv_refresh_filename() optimizes that to "null-co://".  Now the
 filenames differ even though it's still the original filename.  So this
 wouldn't work very well either.
> 
> So what's the full effect here?
> 
> You example says that if you use an overcomplicated way to specify an
> image (by using json: instead of an URL), you get back an
> overcomplicated filename for the parent image (which includes the
> backing file even though it's not really necessary). Sounds fair enough
> to me.

OK, but one issue is that you've used an overcomplicated way for the
backing file; but you get an overcomplicated filename for the overlay.

> Can bad things happen with absolute vs. relative paths?

"Can"?  Absolutely.  "Do"?  I don't know? :-)

Another thing are non-unique URLs.  For instance, nbd allows you to
specify "nbd:localhost:10809", but it will generate
"nbd://localhost:10809".  (Same for just "nbd://localhost".)

Or of course the good old "file:foo.qcow2".

>>> On the other hand, the problem with your current approach is that it
>>> results in a JSON filename even if you override the backing file and
>>> specify the same file name as we already have in the image header.
>>
>> Yes.
>>
>>> In the future, libvirt is going to manually build the graph, so we will
>>> always have the backing file overridden according to the logic in this
>>> patch. I don't think we want to get JSON filenames for all libvirt
>>> managed VMs, so can we realistically do without any kind of comparison?
>>
>> libvirt doesn't need to query the filename, though, does it?
> 
> I know that libvirt uses the output in qemu-img info. And I learnt about
> that because they were surprised that json: filenames you get there
> can't necessarily be fed to QMP (because they contain only strings).
> 
> Other than that, I hope they don't. I suppose the filename can end up in
> error messages in logfiles, though.

Fair point.  Although it isn't impossible to decrypt json:{} filenames,
it isn't very nice.

(And I suspect most error messages today actually contain the node name,
which is even less useful than json:{} filenames.)

>> In my mind, we wanted to phase out filenames and basically only present
>> them as convenience/legacy information to users who use qemu directly.
>>
>> I really don't see the point of burdening qemu 

Re: [Qemu-block] [PATCH v8 03/26] block: Add BDS.backing_overridden

[Kevin Wolf]

Am 22.02.2018 um 16:17 hat Max Reitz geschrieben:
> On 2018-02-22 16:12, Kevin Wolf wrote:
> > Am 22.02.2018 um 15:55 hat Max Reitz geschrieben:
> >> On 2018-02-22 14:39, Kevin Wolf wrote:
> >>> Am 05.02.2018 um 16:18 hat Max Reitz geschrieben:
>  If the backing file is overridden, this most probably does change the
>  guest-visible data of a BDS. Therefore, we will need to consider this in
>  bdrv_refresh_filename().
> 
>  Adding a new field to the BDS is not nice, but it is very simple and
>  exactly keeps track of whether the backing file has been overridden.
> >>>
> >>> ...as long as we manage to actually keep it up to date all the time.
> >>>
> >>> First of all, what I'm missing here (or in fact in the comment in the
> >>> code) is a definition what "overridden" really means. "specified by the
> >>> user" is kind of vague: You consider the backing file relationship for
> >>> snapshot=on as user specified, even though the user wasn't explicit
> >>> about this. On the other hand, creating a live snapshot results in a
> >>> node that isn't user specified.
> >>>
> >>> Isn't the real question to ask whether the default backing file (taken
> >>> from the image header) would result in the same tree? The answer to this
> >>> changes after more operations, like qmp_change_backing_file().
> >>
> >> With you so far.
> >>
> >>> Considering that there are so many ways to change the answer, I think
> >>> the simplest reliable option isn't a new BDS field that needs to updated
> >>> everywhere, but looking at the current value of bs->options and
> >>> bs->backing_file and see if they match.
> >>
> >> I don't see how that is simple.  First, bs->options does not necessarily
> >> reflect the "current options", those would be bs->full_open_options.
> >> And for generating that, we need a way to determine whether the backing
> >> file has been overridden or not, so whether we need to put the backing
> >> options into it or whether we do not.
> > 
> > For the purpose of this comparison, we need a set of options that
> > contains the backing file options unconditionally.
> > 
> >> (I am right that bs->backing_file is what the image header says, right?
> >> So we need to compare it against something that reflects the runtime 
> >> state.)
> > 
> > I think so, yes.
> > 
> >> What I could see would be comparing bs->backing_file to
> >> bs->backing->bs->filename.  But this sounds very hacky to me.
> >>
> >> One thing the comes to mind is that it can break whenever
> >> bdrv_refresh_filename() is clever.  So you specify
> >> 'json:{"driver":"null-co"}' in the image header, and
> >> bdrv_refresh_filename() optimizes that to "null-co://".  Now the
> >> filenames differ even though it's still the original filename.  So this
> >> wouldn't work very well either.

So what's the full effect here?

You example says that if you use an overcomplicated way to specify an
image (by using json: instead of an URL), you get back an
overcomplicated filename for the parent image (which includes the
backing file even though it's not really necessary). Sounds fair enough
to me.

Can bad things happen with absolute vs. relative paths?

> > On the other hand, the problem with your current approach is that it
> > results in a JSON filename even if you override the backing file and
> > specify the same file name as we already have in the image header.
> 
> Yes.
> 
> > In the future, libvirt is going to manually build the graph, so we will
> > always have the backing file overridden according to the logic in this
> > patch. I don't think we want to get JSON filenames for all libvirt
> > managed VMs, so can we realistically do without any kind of comparison?
> 
> libvirt doesn't need to query the filename, though, does it?

I know that libvirt uses the output in qemu-img info. And I learnt about
that because they were surprised that json: filenames you get there
can't necessarily be fed to QMP (because they contain only strings).

Other than that, I hope they don't. I suppose the filename can end up in
error messages in logfiles, though.

> In my mind, we wanted to phase out filenames and basically only present
> them as convenience/legacy information to users who use qemu directly.
> 
> I really don't see the point of burdening qemu with simplifying and
> niceifying filenames when you want to use node names for everything
> anyway.

But if you essentially say "filenames are only for those who don't use
advanced features", then why bother with overridden backing files?

There are two problems I have with this patch: The first is that it
introduces additional state that needs to be managed correctly in all
future patches that modify the graph, and the second (and worse one) is
that it fails to manage this state correctly even now.

I mentioned snapshots and change-backing-file that can result in a wrong
bs->backing_overridden, and those were only the obvious first places I
had a look at. Even if you fix them, I wouldn't trust my 

Re: [Qemu-block] [PATCH v8 03/26] block: Add BDS.backing_overridden

[Max Reitz]

On 2018-02-22 16:12, Kevin Wolf wrote:
> Am 22.02.2018 um 15:55 hat Max Reitz geschrieben:
>> On 2018-02-22 14:39, Kevin Wolf wrote:
>>> Am 05.02.2018 um 16:18 hat Max Reitz geschrieben:
 If the backing file is overridden, this most probably does change the
 guest-visible data of a BDS. Therefore, we will need to consider this in
 bdrv_refresh_filename().

 Adding a new field to the BDS is not nice, but it is very simple and
 exactly keeps track of whether the backing file has been overridden.
>>>
>>> ...as long as we manage to actually keep it up to date all the time.
>>>
>>> First of all, what I'm missing here (or in fact in the comment in the
>>> code) is a definition what "overridden" really means. "specified by the
>>> user" is kind of vague: You consider the backing file relationship for
>>> snapshot=on as user specified, even though the user wasn't explicit
>>> about this. On the other hand, creating a live snapshot results in a
>>> node that isn't user specified.
>>>
>>> Isn't the real question to ask whether the default backing file (taken
>>> from the image header) would result in the same tree? The answer to this
>>> changes after more operations, like qmp_change_backing_file().
>>
>> With you so far.
>>
>>> Considering that there are so many ways to change the answer, I think
>>> the simplest reliable option isn't a new BDS field that needs to updated
>>> everywhere, but looking at the current value of bs->options and
>>> bs->backing_file and see if they match.
>>
>> I don't see how that is simple.  First, bs->options does not necessarily
>> reflect the "current options", those would be bs->full_open_options.
>> And for generating that, we need a way to determine whether the backing
>> file has been overridden or not, so whether we need to put the backing
>> options into it or whether we do not.
> 
> For the purpose of this comparison, we need a set of options that
> contains the backing file options unconditionally.
> 
>> (I am right that bs->backing_file is what the image header says, right?
>> So we need to compare it against something that reflects the runtime state.)
> 
> I think so, yes.
> 
>> What I could see would be comparing bs->backing_file to
>> bs->backing->bs->filename.  But this sounds very hacky to me.
>>
>> One thing the comes to mind is that it can break whenever
>> bdrv_refresh_filename() is clever.  So you specify
>> 'json:{"driver":"null-co"}' in the image header, and
>> bdrv_refresh_filename() optimizes that to "null-co://".  Now the
>> filenames differ even though it's still the original filename.  So this
>> wouldn't work very well either.
> 
> On the other hand, the problem with your current approach is that it
> results in a JSON filename even if you override the backing file and
> specify the same file name as we already have in the image header.

Yes.

> In the future, libvirt is going to manually build the graph, so we will
> always have the backing file overridden according to the logic in this
> patch. I don't think we want to get JSON filenames for all libvirt
> managed VMs, so can we realistically do without any kind of comparison?

libvirt doesn't need to query the filename, though, does it?

In my mind, we wanted to phase out filenames and basically only present
them as convenience/legacy information to users who use qemu directly.

I really don't see the point of burdening qemu with simplifying and
niceifying filenames when you want to use node names for everything anyway.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v8 03/26] block: Add BDS.backing_overridden

[Kevin Wolf]

Am 22.02.2018 um 15:55 hat Max Reitz geschrieben:
> On 2018-02-22 14:39, Kevin Wolf wrote:
> > Am 05.02.2018 um 16:18 hat Max Reitz geschrieben:
> >> If the backing file is overridden, this most probably does change the
> >> guest-visible data of a BDS. Therefore, we will need to consider this in
> >> bdrv_refresh_filename().
> >>
> >> Adding a new field to the BDS is not nice, but it is very simple and
> >> exactly keeps track of whether the backing file has been overridden.
> > 
> > ...as long as we manage to actually keep it up to date all the time.
> > 
> > First of all, what I'm missing here (or in fact in the comment in the
> > code) is a definition what "overridden" really means. "specified by the
> > user" is kind of vague: You consider the backing file relationship for
> > snapshot=on as user specified, even though the user wasn't explicit
> > about this. On the other hand, creating a live snapshot results in a
> > node that isn't user specified.
> > 
> > Isn't the real question to ask whether the default backing file (taken
> > from the image header) would result in the same tree? The answer to this
> > changes after more operations, like qmp_change_backing_file().
> 
> With you so far.
> 
> > Considering that there are so many ways to change the answer, I think
> > the simplest reliable option isn't a new BDS field that needs to updated
> > everywhere, but looking at the current value of bs->options and
> > bs->backing_file and see if they match.
> 
> I don't see how that is simple.  First, bs->options does not necessarily
> reflect the "current options", those would be bs->full_open_options.
> And for generating that, we need a way to determine whether the backing
> file has been overridden or not, so whether we need to put the backing
> options into it or whether we do not.

For the purpose of this comparison, we need a set of options that
contains the backing file options unconditionally.

> (I am right that bs->backing_file is what the image header says, right?
> So we need to compare it against something that reflects the runtime state.)

I think so, yes.

> What I could see would be comparing bs->backing_file to
> bs->backing->bs->filename.  But this sounds very hacky to me.
> 
> One thing the comes to mind is that it can break whenever
> bdrv_refresh_filename() is clever.  So you specify
> 'json:{"driver":"null-co"}' in the image header, and
> bdrv_refresh_filename() optimizes that to "null-co://".  Now the
> filenames differ even though it's still the original filename.  So this
> wouldn't work very well either.

On the other hand, the problem with your current approach is that it
results in a JSON filename even if you override the backing file and
specify the same file name as we already have in the image header.

In the future, libvirt is going to manually build the graph, so we will
always have the backing file overridden according to the logic in this
patch. I don't think we want to get JSON filenames for all libvirt
managed VMs, so can we realistically do without any kind of comparison?

Kevin


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v8 03/26] block: Add BDS.backing_overridden

[Max Reitz]

On 2018-02-22 14:39, Kevin Wolf wrote:
> Am 05.02.2018 um 16:18 hat Max Reitz geschrieben:
>> If the backing file is overridden, this most probably does change the
>> guest-visible data of a BDS. Therefore, we will need to consider this in
>> bdrv_refresh_filename().
>>
>> Adding a new field to the BDS is not nice, but it is very simple and
>> exactly keeps track of whether the backing file has been overridden.
> 
> ...as long as we manage to actually keep it up to date all the time.
> 
> First of all, what I'm missing here (or in fact in the comment in the
> code) is a definition what "overridden" really means. "specified by the
> user" is kind of vague: You consider the backing file relationship for
> snapshot=on as user specified, even though the user wasn't explicit
> about this. On the other hand, creating a live snapshot results in a
> node that isn't user specified.
> 
> Isn't the real question to ask whether the default backing file (taken
> from the image header) would result in the same tree? The answer to this
> changes after more operations, like qmp_change_backing_file().

With you so far.

> Considering that there are so many ways to change the answer, I think
> the simplest reliable option isn't a new BDS field that needs to updated
> everywhere, but looking at the current value of bs->options and
> bs->backing_file and see if they match.

I don't see how that is simple.  First, bs->options does not necessarily
reflect the "current options", those would be bs->full_open_options.
And for generating that, we need a way to determine whether the backing
file has been overridden or not, so whether we need to put the backing
options into it or whether we do not.

(I am right that bs->backing_file is what the image header says, right?
So we need to compare it against something that reflects the runtime state.)

What I could see would be comparing bs->backing_file to
bs->backing->bs->filename.  But this sounds very hacky to me.

One thing the comes to mind is that it can break whenever
bdrv_refresh_filename() is clever.  So you specify
'json:{"driver":"null-co"}' in the image header, and
bdrv_refresh_filename() optimizes that to "null-co://".  Now the
filenames differ even though it's still the original filename.  So this
wouldn't work very well either.

Max

>> This commit adds a FIXME which will be remedied by a follow-up commit.
>> Until then, the respective piece of code will not result in any behavior
>> that is worse than what we currently have.
>>
>> Signed-off-by: Max Reitz 
>> Reviewed-by: Eric Blake 
>> Reviewed-by: Alberto Garcia 
> 
> Kevin
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v8 03/26] block: Add BDS.backing_overridden

[Kevin Wolf]

Am 05.02.2018 um 16:18 hat Max Reitz geschrieben:
> If the backing file is overridden, this most probably does change the
> guest-visible data of a BDS. Therefore, we will need to consider this in
> bdrv_refresh_filename().
> 
> Adding a new field to the BDS is not nice, but it is very simple and
> exactly keeps track of whether the backing file has been overridden.

...as long as we manage to actually keep it up to date all the time.

First of all, what I'm missing here (or in fact in the comment in the
code) is a definition what "overridden" really means. "specified by the
user" is kind of vague: You consider the backing file relationship for
snapshot=on as user specified, even though the user wasn't explicit
about this. On the other hand, creating a live snapshot results in a
node that isn't user specified.

Isn't the real question to ask whether the default backing file (taken
from the image header) would result in the same tree? The answer to this
changes after more operations, like qmp_change_backing_file().

Considering that there are so many ways to change the answer, I think
the simplest reliable option isn't a new BDS field that needs to updated
everywhere, but looking at the current value of bs->options and
bs->backing_file and see if they match.

> This commit adds a FIXME which will be remedied by a follow-up commit.
> Until then, the respective piece of code will not result in any behavior
> that is worse than what we currently have.
> 
> Signed-off-by: Max Reitz 
> Reviewed-by: Eric Blake 
> Reviewed-by: Alberto Garcia 

Kevin