Re: [Qemu-block] [PATCH v3 3/5] qcow2: Introduce an option for sufficient L2 cache for the entire image

[Kevin Wolf]

Am 26.07.2018 um 23:08 hat Leonid Bloch geschrieben:
> On July 26, 2018 11:19:03 PM EEST, Kevin Wolf <address@hidden> wrote:
> >Am 26.07.2018 um 21:43 hat Leonid Bloch geschrieben:
> >> On 07/26/2018 05:50 PM, Leonid Bloch wrote:
> >> > On 07/26/2018 05:42 PM, Kevin Wolf wrote:
> >> > > Am 26.07.2018 um 14:24 hat Leonid Bloch geschrieben:
> >> > > > > > You mean with QDict? I'll look into that now. But
> >> > > > > > already sent v5 before
> >> > > > > > reading this email.
> >> > > > > 
> >> > > > > Yes, with reading it from the QDict. (Or whatever the
> >simplest way is
> >> > > > > that results in the right external interface, but I suppose
> >this is the
> >> > > > > one.)
> >> > > > 
> >> > > > Well, there is a problem with that: I can easily isolate
> >> > > > l2-cache-size from QDict, check if it is "full", and if it is -
> >> > > > do whatever
> >> > > > is needed, and delete this option before parsing. But what if
> >it
> >> > > > is "foo"?
> >> > > > It will not get deleted, and the regular QEMU_OPT_SIZE parsing
> >> > > > error will
> >> > > > appear, stating that l2-cache-size "expects a non-negative
> >> > > > number..." - no
> >> > > > word about that it can expect "full" as well. Now, one can try
> >to modify
> >> > > > local_err->msg for this particular option, but this will
> >require
> >> > > > substantial
> >> > > > additional logic. I think considering this, it would be easier
> >> > > > to stick with
> >> > > > a dedicated option, l2-cache-full.
> >> > > > 
> >> > > > Do you think there is a smarter way to parse the l2-cache-size
> >> > > > option, so it
> >> > > > would accept both size and "full", while handling errors
> >> > > > correctly? It seems
> >> > > > more elegant to have a single option, but the internal handling
> >> > > > will be more
> >> > > > elegant and simpler with two mutually exclusive options.
> >> > > 
> >> > > I think we can live with the suboptimal error message for a
> >while. Once
> >> > > qcow2 is QAPIfied, it should become easy to improve it. Let's not
> >choose
> >> > > a worse design (that stays forever) for a temporarily better
> >error
> >> > > message.
> >> > 
> >> > OK. I'll add a TODO then.
> >> 
> >> Another problem without a dedicated option, is that if l2-cache-size
> >is
> >> processed and deleted, it can not be read again when needed for
> >resizing.
> >> Without some *extremely* dirty tricks, that is.
> >
> >Are you sure? The way that the .bdrv_open() callbacks work, _all_
> >options that are processed are removed from the QDict. The difference
> >is
> >just whether qemu_opts_absorb_qdict() removes them or you do that
> >manually. In the end, if options are left in the QDict, the block layer
> >returns an error because that means that an unknown option was
> >specified.
> >
> >I suppose you use bs->options while resizing. This is a copy of the
> >QDict which is not affected by the removal of processed options.
> 
> This probably explains why it works on the first resize, but fails on
> the second onward.

That makes sense. You probably need to clone the QDict before passing
it to qcow2_update_options(), with qdict_clone_shallow().

> >> Can QAPI be the solution? I've seen some examples, and it looks like
> >the
> >> qcow2 driver already uses QAPI to some extent - I mean all the qdict
> >stuff
> >> is from QAPI, no? Can you please point me to an example of how QAPI
> >can
> >> solve the issue of an option that can accept both a size and a
> >string?
> >
> >By using QAPI I mean using types like BlockdevOptionsQcow2. These types
> >are directly generated from the JSON schema, where it's possible to
> >specify an alternate of a string and an integer.
> >
> >One example for an alternate in the JSON schema is BlockdevRef, where
> >you can give a string (to reference a node-name) or an inline
> >declaration of a block device. In JSON, it looks like this:
> >
> >    { 'alternate': 'BlockdevRef',
> >      'data': { 'definition': 'BlockdevOptions',
> >                'reference': 'str' } }
> >
> >And the generated C type for it is:
> >
> >    struct BlockdevRef {
> >        QType type;
> >        union { /* union tag is @type */
> >            BlockdevOptions definition;
> >            char *reference;
> >        } u;
> >    };
> >
> >In our case, we would get a union of int64_t and char* instead, and
> >still the QType type as a discriminator.
> 
> Thanks for the explanation!
> But qcow2 already has the options in the JSON file, they're just not
> used for some reason?

The block layer configuration is a bit of a mess because of backwards
compatibility. The JSON schema is in fact used for -blockdev and for the
blockdev-add QMP command, but not for the old option -drive and
drive_add.

In the case of -blockdev/blockdev-add, after validating the input
against the schema, the QAPI objects are converted to QDicts so they can
use the same code path as the old code. Eventually, we want to use QAPI
objects all the way, but that requires either expressing all -drive
options in terms of QAPI object or getting rid of -driver altogether.
This is still going to take some time.

Kevin