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

[Kevin Wolf <kwolf@redhat.com>]

Am 24.07.2018 um 22:03 hat Leonid Bloch geschrieben:
> An option "l2-cache-full" is introduced to automatically set the qcow2
> L2 cache to a sufficient value for covering the entire image. The memory
> overhead when using this option is not big (1 MB for each 8 GB of
> virtual image size with the default cluster size) and it can noticeably
> improve performance when using large images with frequent I/O.
> Previously, for this functionality the correct L2 cache size needed to
> be calculated manually or with a script, and then this size needed to be
> passed to the "l2-cache-size" option. Now it is sufficient to just pass
> the boolean "l2-cache-full" option.
> 
> Signed-off-by: Leonid Bloch <lbloch@janustech.com>

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index d40d5ecc3b..c584059e23 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2812,7 +2812,12 @@
>  #                         refcount block caches in bytes (since 2.2)
>  #
>  # @l2-cache-size:         the maximum size of the L2 table cache in
> -#                         bytes (since 2.2)
> +#                         bytes (mutually exclusive with l2-cache-full)
> +#                         (since 2.2)
> +#
> +# @l2-cache-full:         make the L2 table cache large enough to cover the
> +#                         entire image (mutually exclusive with l2-cache-size)
> +#                         (since 3.1)
>  #
>  # @l2-cache-entry-size:   the size of each entry in the L2 cache in
>  #                         bytes. It must be a power of two between 512
> @@ -2840,6 +2845,7 @@
>              '*overlap-check': 'Qcow2OverlapChecks',
>              '*cache-size': 'int',
>              '*l2-cache-size': 'int',
> +            '*l2-cache-full': 'bool',
>              '*l2-cache-entry-size': 'int',
>              '*refcount-cache-size': 'int',
>              '*cache-clean-interval': 'int',

Only looking at the external interface for now, I wonder whether it
would be nicer not to have two mutually exclusive options, but to make
l2-cache-size an alternate that can take either an int like before
(meaning the number of bytes) or a string/enum (with the only accepted
value "full" for now).

Another interesting question is whether 'full' shouldn't keep meaning
full throughout the lifetime of the BlockDriverState, i.e. should it
keep adapting to the new size when the image size changes?

Kevin