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

[Markus Armbruster]

If in a hurry, skip ahead to "bottom line".

Kevin Wolf <address@hidden> writes:

> Am 01.04.2019 um 22:02 hat Markus Armbruster geschrieben:
>> Markus Armbruster <address@hidden> writes:
>> 
>> > Let's review our options for 4.0.
>> >
>> > Please note my analysis is handicapped by incomplete information, in
>> > particular on libvirt's needs.
>> >
>> > Terminology:
>> >
>> > * "Hard read-write" semantics: open read/write.
>> >
>> > * "Hard read-only" semantics: open read-only.
>> >
>> > * "Dynamic read-only" semantics: open read-only, reopen read/write when
>> >   a writer attaches, reopen read-only when the last writer detaches.
>> >
>> > * "Fallback read-only" semantics:. try to open read/write, fall back to
>> >   read-only.
>> >
>> > We have a use case for dynamic read-only: libvirt.  I'm not aware of a
>> > use case for fallback read-only.
>> >
>> > Behavior before 3.1:
>> >
>> > * read-only=on: hard read-only
>> >
>> > * read-only=off: hard read-write
>> >
>> > Behavior in 3.1: new parameter auto-read-only, default off.
>> >
>> > * read-only=on: hard read-only (no change)
>> >
>> > * read-only=on,auto-read-only=on: hard read-only (auto-read-only=on
>> >   silently ignored)
>> >
>> > * read-only=off: hard read-write
>> >
>> > * read-only=off,auto-read-only=on: depends on the driver
>> >
>> >   - file-posix.c's drivers: fallback read-only
>> >   - some other drivers: fallback read-only
>> >   - remaining drivers: hard read/write
>> >
>> > Behavior in 4.0 so far:
>> >
>> > * read-only=on: hard read-only (no change)
>> >
>> > * read-only=on,auto-read-only=on: hard read-only (no change)
>> >
>> > * read-only=off: hard read-write (no change)
>> >
>> > * read-only=off,auto-read-only=on: depends on the driver
>> >
>> >   - file-posix.c's drivers: dynamic read-only
>> >   - some other drivers: fallback read-only (no change)
>> >   - remaining drivers: hard read/write (no change)
>> >
>> >
>> > Option 1: Rename @auto-read-only.
>> >
>> > Rationale: we released auto-read-only in v3.1 with unproven fallback
>> > read-only semantics.  It turned out not to be useful.  Admit the
>> > mistake, retract it.  Release our next attempt in 4.0 under a suitable
>> > new name with fallback read-only semantics.
>> >
>> > CON:
>> >
>> > * Compatibility break.  No-go if there are users.  Users seem quite
>> >   unlikely, though.
>> >
>> > * Still unproven, albeit less so: this time we have some unreleased
>> >   (unfinished?) libvirt code.
>> >
>> > * Semantics are still largely left to drivers, and the schema can't tell
>> >   which one does what.  Awkward.
>> >
>> > * Unless we're fairly confident we won't upgrade drivers from "hard" to
>> >   "fallback" to "dynamic", this merely kicks the can down the road:
>> >   we'll face the exact same "how can libvirt find out" problem on every
>> >   upgrade.
>> >
>> > PRO:
>> >
>> > * When libvirt sees the new name, it can rely on file-posix.c's drivers
>> >   providing dynamic read-only semantics.
>> >
>> >
>> > Option 2: Add query-qemu-features command, return
>> > file-dynamic-auto-read-only #ifdef CONFIG_POSIX.
>> >
>> > Rationale: we released auto-read-only in v3.1 with unproven fallback
>> > read-only semantics.  It turned out not to be useful.  Admit the
>> > mistake, and patch it up in 4.0.  Libirt needs to know whether it's
>> > patche up, and this is a simple way to tell it.
>> >
>> > CON:
>> >
>> > * All of option 1's, except for the compatibility break
>> >
>> > * Uses query-qemu-features to expose a property of the build.  I
>> >   consider that a mistake.
>> >
>> > PRO
>> >
>> > * Libvirt can use either query-qmp-schema or query-qemu-features to find
>> >   out whether it can can rely on file-posix.c's drivers providing
>> >   dynamic read-only semantics.  To make query-qemu-features usable, we
>> >   need to promise query-qemu-features never returns false for it.  To
>> >   make query-qemu-features usable, we need to promise the value will
>> >   remain valid for the remainder of the QEMU run (defeats caching) or
>> >   for any future run of the same QEMU binary (enables caching).
>> >
>> >
>> > Option 3: Add @dynamic-read-only to the drivers that provide dynamic
>> > read-only, default to value of auto-read-only, promise we'll never add
>> > it to drivers that don't.
>> >
>> > Rationale: give users explicit control over dynamic vs. fallback for all
>> > drivers that can provide dynamic.  This makes some sense as an
>> > interface, as long as you ignore the fact that no driver implements both
>> > dynamic and fallback.  I can't see why a driver couldn't implement both.
>> > It also makes dynamic support visible in the schema.
>> >
>> > Behavior (three bools -> eight cases):
>> >
>> > * read-only=on: hard read-only (no change)
>> >
>> >   Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=off
>> >
>> > * read-only=on,auto-read-only=on: hard read-only (no change)
>> >
>> >   Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=on
>> >
>> > * read-only=off: hard read-write (no change)
>> >
>> >   Shorthand for read-only=off,auto-read-only=off,dynamic-read-only=off
>> >
>> > * read-only=off,auto-read-only=on:
>> >
>> >   Shorthand for read-only=off,auto-read-only=on,dynamic-read-only=on
>> >
>> >   - file-posix.c's drivers: dynamic read-only (no change,
>> >     dynamic-read-only=on is the default)
>> >   - some other drivers: fallback read-only (no change)
>> >   - remaining drivers: hard read/write (no change)
>> >
>> > * read-only=off,auto-read-only=on,dynamic-read-only=off
>> >
>> >   - file-posix.c's drivers: error
>> >   - all other drivers: N/A
>> >
>> > * read-only=off,auto-read-only=off,dynamic-read-only=on
>> >
>> >   - file-posix.c's drivers: error
>> >   - all other drivers: N/A
>> >
>> > * read-only=on,dynamic-read-only=on
>> >
>> >   Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=on
>> >
>> >   - file-posix.c's drivers: error
>> >   - all other drivers: N/A
>> >
>> > * read-only=on,auto-read-only=on,dynamic-read-only=off
>> >
>> >   Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=off
>> >
>> >   - file-posix.c's drivers: error
>> >   - all other drivers: N/A
>> >
>> > CON:
>> >
>> > * Two bools (read-only and auto-read-only) to select from three choices
>> >   was already ugly.  Three bools (the same plus dynamic-read-only) to
>> >   select from four choices is even uglier.
>> >
>> > * The explicit control is just a facade so far: since only the default
>> >   setting is implemented, it doesn't actually control anything.
>> >
>> > PRO:
>> >
>> > * When libvirt sees a driver providing a dynamic-read-only parameter, it
>> >   knows it can rely on the driver providing dynamic read-only semantics.
>> >
>> > * Adding dynamic read-only capability to drivers creates no
>> >   introspection problems: we simply add dynamic-read-only to their
>> >   parameters.
>> >
>> > Code sketch appended
>> 
>> Option 4 [Kevin]: Add a dummy option to BlockdevOptionsFile.
>> Message-ID: <address@hidden>
>> 
>> A variation of option 3, code is almost identical.  Where option 3 tries
>> to evolve what we have into the least bad permanent interface, option 4
>> only wants to serve as a stop gap until we get a better solution, such
>> as option 5.
>
> I think code-wise, this is actually very different from option 3. We
> need to actually process the dynamic-read-only option with option 3, and
> I can see things go wrong easily, which is not something to do for -rc2.
>
> Option 4, in contrast, is purely a minimal schema change without
> touching any C source file.

While I reject your "is very different" claim, I accept your "option 4
is less risky than option 3" claim.

Common part modulo naming:

    diff --git a/qapi/block-core.json b/qapi/block-core.json
    index 7ccbfff9d0..0cf15477ce 100644
    --- a/qapi/block-core.json
    +++ b/qapi/block-core.json
    @@ -2827,6 +2827,7 @@
     # Driver specific block device options for the file backend.
     #
     # @filename:    path to the image file
    +# @dynamic-read-only: FIXME document (since 4.0)
     # @pr-manager:  the id for the object that will handle persistent 
reservations
     #               for this device (default: none, forward the commands via 
SG_IO;
     #               since 2.11)
    @@ -2847,6 +2848,8 @@
     ##
     { 'struct': 'BlockdevOptionsFile',
       'data': { 'filename': 'str',
    +            '*dynamic-read-only': {'type': 'bool',
    +                                   'if': 'defined(CONFIG_POSIX)'},
                 '*pr-manager': 'str',
                 '*locking': 'OnOffAuto',
                 '*aio': 'BlockdevAioOptions',

Option 3 only:

    diff --git a/block/file-posix.c b/block/file-posix.c
    index db4cccbe51..d62681df14 100644
    --- a/block/file-posix.c
    +++ b/block/file-posix.c
    @@ -420,6 +420,11 @@ static QemuOptsList raw_runtime_opts = {
                 .type = QEMU_OPT_STRING,
                 .help = "File name of the image",
             },
    +        {
    +            .name = "dynamic-read-only",
    +            .type = QEMU_OPT_BOOL,
    +            .help = "FIXME",
    +        },
             {
                 .name = "aio",
                 .type = QEMU_OPT_STRING,
    @@ -540,6 +545,13 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
         s->open_flags = open_flags;
         raw_parse_flags(bdrv_flags, &s->open_flags, false);

    +    if (qemu_opt_get_bool(opts, "dynamic-read-only",
    +                          bdrv_flags & BDRV_O_AUTO_RDONLY)
    +        == !(bdrv_flags & BDRV_O_AUTO_RDONLY)) {
    +        error_setg(errp, "FIXME");
    +        ret = - EINVAL;
    +    }
    +
         s->fd = -1;
         fd = qemu_open(filename, s->open_flags, 0644);
         ret = fd < 0 ? -errno : 0;

Yes, this is more code, and yes, things could conceivably go wrong here.

>> Option 5 [Kevin]: Add feature flags to the QAPI language, use one to say
>> "this block driver provides dynamic read-only".
>> Message-ID: <address@hidden>
>
> More complex than option 4, but the advantage here is that none of the
> new code is actually externally visible, so if the implementation turns
> out buggy, we can fix it without having to maintain compatibility with
> the broken state.
>
> The actually visible change, the query-qmp-schema output, is almost as
> minimal as option 4 and can manually be verified.
>
> A bit more invasive than option 4, but I'd consider it still doable for
> -rc2 if we decide quickly.

It's a good option given the mess we made with auto-read-only in 3.1.
It's just three weeks late.

>> Option 6 [Peter]: Trigger reopen explicitly via QMP.
>> Message-ID: <address@hidden>
>> [Me] This leaves auto-read-only without a use case; deprecate?
>
> We have made some progress there with the new x-blockdev-reopen command.
> There are a few things that we know must be addressed before we can
> declare it stable, and probably there are more such things that we don't
> know of.
>
> x-blockdev-reopen takes the same options as blockdev-add and as such has
> a similar complexity. The experience with blockdev-add makes me doubt
> that we can mark it stable even for 4.1. So this might be a long-term
> solution, but we'd have to do something else for the meantime.

Understand.

>> Given that the libvirt code isn't ready, why do we need this done and
>> declared stable in 4.0?  We're going to tag -rc2 tomorrow...
>
> The next libvirt release will be long before the next QEMU release. If

Libvirt releases roughly monthly.

QEMU releases roughly April, August, December.

> we don't have it in 4.0, enabling -blockdev will have to wait until the
> first libvirt release after the QEMU 4.1 release (so maybe September)
> and we'll still be stuck with -drive and unable to start deprecating it
> until then.

Unless libvirt does yet another version check.

I talked this over with Peter.

libvirt 5.2 is expected today.  Peter is trying to get his work merged
in time for the next libvirt release (but he was trying that a month
ago, too).  So we're talking about 5.3 (May) or 5.4 (June).  Leaves a
gap of 2-3 month to 4.1 (August).

I offered option 4 "Add a dummy option to BlockdevOptionsFile".  He said
he'd advise against ugly hacks at this point, but going forward, QEMU
really needs a way to let libvirt detect code fixes, such as QAPI schema
feature flags.

> Also, we don't want to give Peter the excuse that the qemu code isn't
> ready. ;-)

Ah, you'd never do that to us, Peter, would you?


Bottom line:

* I can't bring myself to do QAPI schema language changes (option 5)
  this late in the game.  Sorry.

* I'm okay with option 4 if Kevin believes it could be useful.  Even if
  Peter won't actually use it, we're fairly unlikely to regret it.

* A more complete respin of the proposed QAPI schema language changes
  would be fair game for 4.1.

* QEMU and libvirt need to get better at co-developing features.  QEMU
  doesn't want to release stable interfaces without a user, and libvirt
  doesn't want to release a user without QEMU releasing first.  When we
  break this cycle by having QEMU releases stable interfaces completely
  unproven, we unsurprisingly get to regret it later.

Does this make sense?