Re: [Qemu-block] [Qemu-devel] Options for 4.0

[Kevin Wolf]

Am 01.04.2019 um 18:22 hat Markus Armbruster geschrieben:
> Peter Krempa <address@hidden> writes:
> > On Mon, Apr 01, 2019 at 16:55:50 +0200, Markus Armbruster wrote:
> >> Peter Krempa <address@hidden> writes:
> >> > That works with -drive as qemu reopens the files internally, but does
> >> > not work when using -blockdev.
> >> 
> >> Happy to take your word for it.

The difference is actually not -blockdev vs. -drive, but whether you
define the 'file' node separately in its own -blockdev option or whether
you create it in the context of the format node (using file.* dotted
syntax for its options).

Effectively this doesn't change what Peter says because -drive only
supports the latter, and for -blockdev we only want to use the former.

> >> > Since we need to keep the images RO until doing  the blockjob there
> >> > needs to be a way when either qemu automagically does what libvirt needs
> >> > even if the image did not have the write permission originally.
> >> >
> >> > auto-read-only was a naive impl when qemu attempted to keep the storage
> >> > access nodes RW. At the point when I was testing it I didn't enable
> >> > s-virt as it's a hassle when you want to run git versions of libvirt and
> >> > qemu and thus didn't see the problem with missing permissions.
> >> 
> >> Yes, fallback read-only is useless for the scenario above.
> >> 
> >> Kevin wrote "libvirt requires dynamic read-only for file-posix. It
> >> prefers dynamic read-only for other drivers, but can live with fallback
> >> read-only there."  I'm not sure how it can live with fallback read-only.
> >> Is it because only *files* can labelled with SELinux?
> >> 
> >> If yes, then my next question is how libvirt makes use of fallback
> >> read-only for non-files.  Can you give me an example?
> >
> > For non-files libvirt usually does not have means to modify the access
> > permissions.
> >
> > A simple example may be a NBD export. If the NBD server exports the
> > image readonly libvirt can't do much about it. Arguably you can consider
> > a situation where the user modifies the access to read-write manually
> > and then invokes the libvirt virDomainBlockCommit API and thus still
> > expects the automagic reopen to happen correctly. At this point I'm not
> > really certain to which extend this would work currently with -drive as
> > there are known issues when attempting to point to a non-file image by
> > path rather than node-name.
> 
> Both automagic reopen and explicit reopen work here.  Both succeed when
> the user has granted access manually.  Libvirt would have to try the
> explicit reopen even in cases where it can't grant access itself.
> 
> Sounds like you don't have a use case for fallback read-only.  Correct?

If everything supports dynamic read-only, I don't think anyone would
have a use for fallback read-only. The long-term vision was that we
would remove the read-only option and add a new force-read-only option
that would allow selecting between "force read-only" and "do whatever is
needed to support what the attached parents requested". This should be
all we really need. I don't think there is even a use case for a hard
read-write option.

It's just that dynamic read-only is a bit harder to implement, so we
started with fallback read-only as "good enough for libvirt" (seemingly,
turned out to be wrong for file-posix).

The definition of the option was kept intentionally vague so that it
would allow switching to dynamic read-only. The idea was that it only
turns an error into working cases, so we can safely do the switch and
libvirt would automatically be upgraded from an error case to a working
case, too.

Of course, in this reasoning we forgot that libvirt might want to just
switch back to -drive in the cases that would error out instead of
actually receiving that error.

> >> > The dynamic version attempts to fix that. That is still the automagic
> >> > approach. I don't really mind also doing a hands-on approach where we'd
> >> > need to tell qemu to reopen given nodes.
> >> 
> >> Aha!  You just corrected my overly narrow idea of the solution space.
> >> 
> >> > I'm not sure what ends up being less work.
> >> 
> >> "Less work" is an important consideration.
> >> 
> >> We may be able to accept some extra work to get a saner interface.
> >> Depends on how much saner and how much extra exactly, and on who needs
> >> to do the work.
> >
> > I agree. While the 'explicit reopen' approach may be more work for
> > libvirt I'll happily accept that solution.
> 
> Would x-blockdev-reopen do?
> 
> What exactly is hold it in x-space?
> 
> I guess a specialized blockdev-set-read-only would do as well.

x-blockdev-reopen was only just added. It provides the same options as
blockdev-add, so a first estimation would be that it's roughly the same
complexity. Experience from blockdev-add tells us that getting it stable
the next release was completely unrealistic because we didn't even fully
understand the problems yet. I expect the same for x-blockdev-reopen.

Maybe what impresses you more is that x-blockdev-reopen doesn't only
change options like read-only, but also references to other nodes. So
it's the dynamic graph reconfiguration thing we've talking about for
years. This seems hard to get right in the first attempt.

But we also know a few more concrete shortcomings. For example, changing
child nodes doesn't work if iothreads are involved.

Also: Not all option that could theoretically be changed in
x-blockdev-reopen are actually supported yet. If we mark it stable
before everything is covered, we'll need some kind of schema annotations
again that describe in which version changing which option was added.
Maybe this time we'd better figure out how to do that before creating
the problem.

For blockdev-add, we decided to only mark it stable as soon as full
feature parity with -drive was achieved (except for options that we
_really_ don't want to have in QAPI).

So I'm very doubtful of marking x-blockdev-reopen stable in 4.1.

A specialised blockdev-set-read-only seems more realistic for 4.1, but
it still wouldn't solve the problem now in 4.0 and once we have a stable
blockdev-reopen, it's duplicated functionality.

Kevin