Re: [PATCH 1/2] block/rbd: support selected key-value-pairs via QAPI

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, May 15, 2025 at 01:29:07PM +0200, Fiona Ebner wrote:
> Currently, most Ceph configuration options are not exposed via QAPI.
> While it is possible to specify a dedicated Ceph configuration file,
> specialized options are often only required for a selection of images
> on the RBD storage, not all of them. To avoid the need to generate a
> dedicated Ceph configuration file for each image (or for each required
> combination of options), support a selection of key-value pairs via
> QAPI.
> 
> Initially, this is just 'rbd_cache_policy'. For example, this is
> useful with small images used as a pflash for EFI variables. Setting
> the 'rbd_cache_policy' to 'writeback' yields a substantial improvement
> there [0].
> 
> The function qemu_rbd_extract_key_value_pairs() was copied/adapted
> from the existing qemu_rbd_extract_encryption_create_options().
> 
> [0]: https://bugzilla.proxmox.com/show_bug.cgi?id=3329#c9
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  block/rbd.c          | 73 ++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 37 ++++++++++++++++++++++
>  2 files changed, 110 insertions(+)


> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 91c70e24a7..4666765e66 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4301,6 +4301,39 @@
>    'data': { 'luks': 'RbdEncryptionCreateOptionsLUKS',
>              'luks2': 'RbdEncryptionCreateOptionsLUKS2' } }
>  
> +##
> +# @RbdCachePolicy:
> +#
> +# An enumeration of values for the 'rbd_cache_policy' Ceph
> +# configuration setting.  See the Ceph documentation for details.
> +#
> +# @writearound: cachable writes return immediately, reads are not
> +#     served from the cache.
> +#
> +# @writeback: cachable writes return immediately, reads are served
> +#     from the cache.
> +#
> +# @writethrough: writes return only when the data is on disk for all
> +#     replicas, reads are served from the cache.
> +#
> +# Since 10.1
> +##
> +{ 'enum' : 'RbdCachePolicy',
> +  'data' : [ 'writearound', 'writeback', 'writethrough' ] }
> +
> +
> +##
> +# @RbdKeyValuePairs:
> +#
> +# Key-value pairs for Ceph configuration.
> +#
> +# @rbd-cache-policy: Ceph configuration option 'rbd_cache_policy'.
> +#
> +# Since 10.1
> +##
> +{ 'struct': 'RbdKeyValuePairs',
> +  'data': { '*rbd-cache-policy': 'RbdCachePolicy' } }
> +
>  ##
>  # @BlockdevOptionsRbd:
>  #
> @@ -4327,6 +4360,9 @@
>  #     authentication.  This maps to Ceph configuration option "key".
>  #     (Since 3.0)
>  #
> +# @key-value-pairs: Key-value pairs for additional Ceph configuraton.
> +#     (Since 10.1)
> +#
>  # @server: Monitor host address and port.  This maps to the "mon_host"
>  #     Ceph option.
>  #
> @@ -4342,6 +4378,7 @@
>              '*user': 'str',
>              '*auth-client-required': ['RbdAuthMode'],
>              '*key-secret': 'str',
> +            '*key-value-pairs' : 'RbdKeyValuePairs',

I'm not seeing any point in this 'RbdKeyValuePairs' struct. Why isn't
the 'rbd-cache-policy' field just directly part of the BlockdevOptionsRbd
struct like all the other options are ?

Also, 'rbd-' as a prefix in the field name is redundant when this is
already in an RBD specific struct.

>              '*server': ['InetSocketAddressBase'] } }
>  

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|