Re: [Qemu-block] bdrv_reopen() and backing images

[Kevin Wolf]

Am 09.09.2016 um 18:38 hat Alberto Garcia geschrieben:
> On Wed 17 Aug 2016 05:19:06 PM CEST, Kevin Wolf <address@hidden> wrote:
> 
> >> But I would like to fix bdrv_reopen() properly. From what I can see
> >> the situation is that bdrv_backing_options() forces all backing BDSs
> >> to be opened without the BDRV_O_RDWR flag, which makes sense when you
> >> open an image for the first time, but not so much when you reopen it.
> >
> > The wanted behaviour is, like with all other options, that the option
> > stays as it is if it was set explicitly on the node, and to be updated
> > if it was inherited from the parent node.
> >
> > I know that we do get this behaviour e.g. for cache modes. It might be
> > that the (hopefully only) reason why we can't have it for read-only
> > yet is that the read-only option is still passed in flags rather than
> > the QDict.
> >
> > Converting it to be a "normal" option using the QDict should fix the
> > problem then.
> 
> Status update: I've been taking a look at this during the past days.
> 
> Although I managed to make it work, at least for the specific problem
> that I was having, I'm not done yet.
> 
> First I looked into removing the BDRV_O_RDWR flag completely. The
> problem is that that flag is used in a lot of places all over the QEMU
> code. The way to deal with its removal varies depending on the case:
> 
>   1. If we're setting the flag, it needs to be converted into a QBool
>      and stored in a (possibly new) QDict in order to be passed to
>      e.g. bdrv_open(). A bit more work, some APIs need to be adjusted,
>      but else it's relatively straightforward.
> 
>   2. It can be replaced with bs->read_only. This is the simplest one.
> 
>   3. It can be replaced with qemu_opt_get_bool(). This is also simple,
>      but it may involve creating the QemuOpts object earlier if it
>      doesn't exist yet. It shouldn't be a big deal.
> 
>   4. Sometimes we cannot create a QemuOpts object and the only way to
>      get that value is from a QDict, which can contain a QBool or a
>      QString with "on" / "off". So we'd need additional code to parse
>      it. This is rather tedious and I don't like it.
> 
> Even if we can get rid of all instances of BDRV_O_RDWR, the problem
> with all this is that we'd be replacing very simple operations to
> check/set flags with code that is significantly less efficient and
> more complicated. There are some functions that nicely transform QEMU
> flags into standard POSIX flags that would become more complicated for
> no additional benefit (qemu_gluster_parse_flags(), raw_parse_flags()),
> and in general I think flags are a better solution for this kind of
> boolean properties.

Yes, flags generally work okay for boolean options. One problem, as you
know is that with reopen it becomes actually tristate (one/off/leave it
alone), which would at least require two flags (keep/set, on/off).

> So the other alternative is to add "read-only" as an option and keep
> it in sync with the BDRV_O_RDWR flag. This is similar to what commit
> 91a097e7478940483 did with the cache options.
> 
> There's a problem here, mostly related to the bdrv_open_inherit()
> code. The goal of this change is to inherit the "read-only" option only
> if it hasn't been explicitly set, e.g. with qdict_set_default_str().
> However that means that we don't know the new value unless we parse the
> QDict (scenario 4 above), and if we don't do it the flags and options
> will be (temporarily) out of sync. I think we can deal with that, and
> and that's what I'm trying at the moment. It's significantly less code,
> although I think it requires moving things from bdrv_open_inherit() to
> bdrv_open_common() (so we can use the QemuOpts object).

Hm, is it that much code that must be moved, actually?

Very early in bdrv_open_inherit() we call bdrv_fill_options(), which
basically converts everything into the QDict form. Specifically, for
flag we call update_options_from_flags(), so if you add read-only there,
the flags are repesented in the QDict (but not the other way around).

At the moment, we call update_flags_from_options() for the other
direction only in bdrv_open_common(), but if we move that earlier (or
maybe just do it a second time instead of moving, not sure if we make
later changes to the QDict that require another call where we currently
do it), would that break anything? We could perhaps stick it right below
the update_options_from_flags() call in bdrv_fill_options().

With that one line moved/copied, everything else should be able to stay
as it is for opening.

> I'm also wondering: if the only reason why we need to do all this is
> to distinguish the flags that were set explicitly from the ones that
> were inherited, isn't it simpler and more efficient to use a mask
> instead?
> 
>     struct BlockDriverState {
>         /* ... */
>         int open_flags;
>         int open_flags_mask;
>         /* ... */
>     };
> 
> I haven't explored this possibility much yet, but intuitively it
> doesn't look bad. It would even allow us to tell bdrv_reopen() which
> flags we want to alter and which ones we don't want to touch.

There are actually multiple (even though related) reasons.

The first is consistency, and having the QDict as the single
authoritative source for options just makes the interfaces simpler.
We're not there yet and block drivers still get flags passed, but I'd
hope to remove that sooner or later.

The second one is that the QDict is an exact mapping of the blockdev-add
QAPI structure. When the request comes from QMP, you don't get a legacy
filename string or flag, but just the options. This is the primary
interface to support, and other (internal and external) interfaces
should map to it if possible.

The third one is that in the QDict, we support setting the option for
child nodes (e.g. backing.file.read-only=on). This can't be done with
flags.

So it's pretty clear to me that getting the option into the QDict is the
way to go. Whether or not we keep it in the flags additionally is
secondary, so if it makes our lives easier, we can do that. But the flag
would then mainly be for checking the state (and as a shortcut for
internal callers who don't want to create a QDict) rather than for
the actual configuration code.

Kevin