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

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

[Fiona Ebner]

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.

Fiona Ebner (2):
  block/rbd: support selected key-value-pairs via QAPI
  block/rbd: support keyring option via QAPI

 block/rbd.c          | 81 ++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 40 ++++++++++++++++++++++
 2 files changed, 121 insertions(+)

-- 
2.39.5

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

[Fiona Ebner]

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/block/rbd.c b/block/rbd.c
index 7446e66659..2924f23093 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -298,6 +298,27 @@ static int qemu_rbd_set_auth(rados_t cluster, BlockdevOptionsRbd *opts,
     return 0;
 }
 
+static int qemu_rbd_set_key_value_pairs(rados_t cluster,
+                                        RbdKeyValuePairs *key_value_pairs,
+                                        Error **errp)
+{
+    if (!key_value_pairs) {
+        return 0;
+    }
+
+    if (key_value_pairs->has_rbd_cache_policy) {
+        RbdCachePolicy value = key_value_pairs->rbd_cache_policy;
+        int r = rados_conf_set(cluster, "rbd_cache_policy",
+                               RbdCachePolicy_str(value));
+        if (r < 0) {
+            error_setg_errno(errp, -r, "could not set 'rbd_cache_policy'");
+            return -EINVAL;
+        }
+    }
+
+    return 0;
+}
+
 static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json,
                                  Error **errp)
 {
@@ -791,6 +812,44 @@ exit:
     return ret;
 }
 
+static int qemu_rbd_extract_key_value_pairs(
+        QemuOpts *opts,
+        RbdKeyValuePairs **key_value_pairs,
+        Error **errp)
+{
+    QDict *opts_qdict;
+    QDict *key_value_pairs_qdict;
+    Visitor *v;
+    int ret = 0;
+
+    opts_qdict = qemu_opts_to_qdict(opts, NULL);
+    qdict_extract_subqdict(opts_qdict, &key_value_pairs_qdict,
+                           "key-value-pairs.");
+    qobject_unref(opts_qdict);
+    if (!qdict_size(key_value_pairs_qdict)) {
+        *key_value_pairs = NULL;
+        goto exit;
+    }
+
+    /* Convert options into a QAPI object */
+    v = qobject_input_visitor_new_flat_confused(key_value_pairs_qdict, errp);
+    if (!v) {
+        ret = -EINVAL;
+        goto exit;
+    }
+
+    visit_type_RbdKeyValuePairs(v, NULL, key_value_pairs, errp);
+    visit_free(v);
+    if (!*key_value_pairs) {
+        ret = -EINVAL;
+        goto exit;
+    }
+
+exit:
+    qobject_unref(key_value_pairs_qdict);
+    return ret;
+}
+
 static int coroutine_fn qemu_rbd_co_create_opts(BlockDriver *drv,
                                                 const char *filename,
                                                 QemuOpts *opts,
@@ -800,6 +859,7 @@ static int coroutine_fn qemu_rbd_co_create_opts(BlockDriver *drv,
     BlockdevCreateOptionsRbd *rbd_opts;
     BlockdevOptionsRbd *loc;
     RbdEncryptionCreateOptions *encrypt = NULL;
+    RbdKeyValuePairs *key_value_pairs = NULL;
     Error *local_err = NULL;
     const char *keypairs, *password_secret;
     QDict *options = NULL;
@@ -848,6 +908,13 @@ static int coroutine_fn qemu_rbd_co_create_opts(BlockDriver *drv,
     loc->image       = g_strdup(qdict_get_try_str(options, "image"));
     keypairs         = qdict_get_try_str(options, "=keyvalue-pairs");
 
+    /* These are the key-value pairs coming in via the QAPI. */
+    ret = qemu_rbd_extract_key_value_pairs(opts, &key_value_pairs, errp);
+    if (ret < 0) {
+        goto exit;
+    }
+    loc->key_value_pairs = key_value_pairs;
+
     ret = qemu_rbd_do_create(create_options, keypairs, password_secret, errp);
     if (ret < 0) {
         goto exit;
@@ -937,6 +1004,12 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
         goto failed_shutdown;
     }
 
+    /* For the key-value pairs coming via QAPI. */
+    r = qemu_rbd_set_key_value_pairs(*cluster, opts->key_value_pairs, errp);
+    if (r < 0) {
+        goto failed_shutdown;
+    }
+
     if (mon_host) {
         r = rados_conf_set(*cluster, "mon_host", mon_host);
         if (r < 0) {
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',
             '*server': ['InetSocketAddressBase'] } }
 
 ##
-- 
2.39.5

[PATCH 2/2] block/rbd: support keyring option via QAPI

[Fiona Ebner]

In Proxmox VE, it is not always required to have a dedicated Ceph
configuration file, and using the 'key-secret' QAPI option would
require obtaining a key from the keyring first. The keyring location
is readily available however, so having support for the 'keyring'
configuration option is most convenient.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 block/rbd.c          | 8 ++++++++
 qapi/block-core.json | 5 ++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/block/rbd.c b/block/rbd.c
index 2924f23093..660224c6c8 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -306,6 +306,14 @@ static int qemu_rbd_set_key_value_pairs(rados_t cluster,
         return 0;
     }
 
+    if (key_value_pairs->keyring) {
+        int r = rados_conf_set(cluster, "keyring", key_value_pairs->keyring);
+        if (r < 0) {
+            error_setg_errno(errp, -r, "could not set 'keyring'");
+            return -EINVAL;
+        }
+    }
+
     if (key_value_pairs->has_rbd_cache_policy) {
         RbdCachePolicy value = key_value_pairs->rbd_cache_policy;
         int r = rados_conf_set(cluster, "rbd_cache_policy",
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4666765e66..3253c6e6e9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4327,12 +4327,15 @@
 #
 # Key-value pairs for Ceph configuration.
 #
+# @keyring: Ceph configuration option 'keyring'.
+#
 # @rbd-cache-policy: Ceph configuration option 'rbd_cache_policy'.
 #
 # Since 10.1
 ##
 { 'struct': 'RbdKeyValuePairs',
-  'data': { '*rbd-cache-policy': 'RbdCachePolicy' } }
+  'data': { '*keyring': 'str',
+            '*rbd-cache-policy': 'RbdCachePolicy' } }
 
 ##
 # @BlockdevOptionsRbd:
-- 
2.39.5

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

[Fiona Ebner]

Am 15.05.25 um 13:29 schrieb Fiona Ebner:
> 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.
> 
> Fiona Ebner (2):
>   block/rbd: support selected key-value-pairs via QAPI
>   block/rbd: support keyring option via QAPI
> 
>  block/rbd.c          | 81 ++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 40 ++++++++++++++++++++++
>  2 files changed, 121 insertions(+)

Ping

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

[Ilya Dryomov]

On Thu, May 15, 2025 at 1:29 PM Fiona Ebner <f.ebner@proxmox.com> 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/block/rbd.c b/block/rbd.c
> index 7446e66659..2924f23093 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -298,6 +298,27 @@ static int qemu_rbd_set_auth(rados_t cluster, BlockdevOptionsRbd *opts,
>      return 0;
>  }
>
> +static int qemu_rbd_set_key_value_pairs(rados_t cluster,
> +                                        RbdKeyValuePairs *key_value_pairs,
> +                                        Error **errp)
> +{
> +    if (!key_value_pairs) {
> +        return 0;
> +    }
> +
> +    if (key_value_pairs->has_rbd_cache_policy) {
> +        RbdCachePolicy value = key_value_pairs->rbd_cache_policy;
> +        int r = rados_conf_set(cluster, "rbd_cache_policy",
> +                               RbdCachePolicy_str(value));
> +        if (r < 0) {
> +            error_setg_errno(errp, -r, "could not set 'rbd_cache_policy'");
> +            return -EINVAL;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json,
>                                   Error **errp)
>  {
> @@ -791,6 +812,44 @@ exit:
>      return ret;
>  }
>
> +static int qemu_rbd_extract_key_value_pairs(
> +        QemuOpts *opts,
> +        RbdKeyValuePairs **key_value_pairs,
> +        Error **errp)
> +{
> +    QDict *opts_qdict;
> +    QDict *key_value_pairs_qdict;
> +    Visitor *v;
> +    int ret = 0;
> +
> +    opts_qdict = qemu_opts_to_qdict(opts, NULL);
> +    qdict_extract_subqdict(opts_qdict, &key_value_pairs_qdict,
> +                           "key-value-pairs.");
> +    qobject_unref(opts_qdict);
> +    if (!qdict_size(key_value_pairs_qdict)) {
> +        *key_value_pairs = NULL;
> +        goto exit;
> +    }
> +
> +    /* Convert options into a QAPI object */
> +    v = qobject_input_visitor_new_flat_confused(key_value_pairs_qdict, errp);
> +    if (!v) {
> +        ret = -EINVAL;
> +        goto exit;
> +    }
> +
> +    visit_type_RbdKeyValuePairs(v, NULL, key_value_pairs, errp);
> +    visit_free(v);
> +    if (!*key_value_pairs) {
> +        ret = -EINVAL;
> +        goto exit;
> +    }
> +
> +exit:
> +    qobject_unref(key_value_pairs_qdict);
> +    return ret;
> +}
> +
>  static int coroutine_fn qemu_rbd_co_create_opts(BlockDriver *drv,
>                                                  const char *filename,
>                                                  QemuOpts *opts,
> @@ -800,6 +859,7 @@ static int coroutine_fn qemu_rbd_co_create_opts(BlockDriver *drv,
>      BlockdevCreateOptionsRbd *rbd_opts;
>      BlockdevOptionsRbd *loc;
>      RbdEncryptionCreateOptions *encrypt = NULL;
> +    RbdKeyValuePairs *key_value_pairs = NULL;
>      Error *local_err = NULL;
>      const char *keypairs, *password_secret;
>      QDict *options = NULL;
> @@ -848,6 +908,13 @@ static int coroutine_fn qemu_rbd_co_create_opts(BlockDriver *drv,
>      loc->image       = g_strdup(qdict_get_try_str(options, "image"));
>      keypairs         = qdict_get_try_str(options, "=keyvalue-pairs");
>
> +    /* These are the key-value pairs coming in via the QAPI. */
> +    ret = qemu_rbd_extract_key_value_pairs(opts, &key_value_pairs, errp);
> +    if (ret < 0) {
> +        goto exit;
> +    }
> +    loc->key_value_pairs = key_value_pairs;
> +
>      ret = qemu_rbd_do_create(create_options, keypairs, password_secret, errp);
>      if (ret < 0) {
>          goto exit;
> @@ -937,6 +1004,12 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
>          goto failed_shutdown;
>      }
>
> +    /* For the key-value pairs coming via QAPI. */
> +    r = qemu_rbd_set_key_value_pairs(*cluster, opts->key_value_pairs, errp);
> +    if (r < 0) {
> +        goto failed_shutdown;
> +    }
> +
>      if (mon_host) {
>          r = rados_conf_set(*cluster, "mon_host", mon_host);
>          if (r < 0) {
> 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' } }

Hi Fiona,

I'm not following the rationale for introducing RbdKeyValuePairs
struct.  If there is a desire to expose rbd_cache_policy option this
way, couldn't it just be added to BlockdevOptionsRbd struct?  The
existing auth_client_required option has a very similar pattern.

If exposing rbd_cache_policy option, rbd_cache option (enabled/disabled
bool) should likely be exposed as well.  It doesn't make much sense to
provide a built-in way to adjust the cache policy without also providing
a built-in way to disable the cache entirely.  Then the question of what
would be better from the QAPI perspective arises: a bool option to map
to Ceph as close as possible or perhaps an additional 'disabled' value
in RbdCachePolicy enum?  And regardless of that, the need to remember
to update QEMU if a new cache policy is ever added because even though
these are just strings, QEMU is going to be validating them...

> +
>  ##
>  # @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',

To side-step all of the above, have you considered implementing
a straightforward passthrough to Ceph instead?  Something like

  '*key-value-pairs': ['RbdKeyValuePair']

where RbdKeyValuePair is just a pair arbitrary strings (and
key-value-pairs is thus an optional list of those).  rados_conf_set()
would be called just the same but the user would be able to override
any Ceph option they wish, not just a few that we thought of here.

Thanks,

                Ilya

Re: [PATCH 2/2] block/rbd: support keyring option via QAPI

[Ilya Dryomov]

On Thu, May 15, 2025 at 1:29 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>
> In Proxmox VE, it is not always required to have a dedicated Ceph
> configuration file, and using the 'key-secret' QAPI option would
> require obtaining a key from the keyring first. The keyring location
> is readily available however, so having support for the 'keyring'
> configuration option is most convenient.

Would such a setup have a ceph.conf file that is shared between
multiple users (or no ceph.conf file at all if the monitors are
specified via QAPI option) but individual keyring files for each
user?

Thanks,

                Ilya

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

[Daniel P. Berrangé]

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 :|

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

[Daniel P. Berrangé]

On Mon, Jun 16, 2025 at 11:25:54AM +0200, Ilya Dryomov wrote:
> On Thu, May 15, 2025 at 1:29 PM Fiona Ebner <f.ebner@proxmox.com> 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>

snip

> >  ##
> >  # @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',
> 
> To side-step all of the above, have you considered implementing
> a straightforward passthrough to Ceph instead?  Something like
> 
>   '*key-value-pairs': ['RbdKeyValuePair']
> 
> where RbdKeyValuePair is just a pair arbitrary strings (and
> key-value-pairs is thus an optional list of those).  rados_conf_set()
> would be called just the same but the user would be able to override
> any Ceph option they wish, not just a few that we thought of here.

Passing through arbitrary key/value pairs as strings is essentially
abdicating our design responsibility in QAPI. enums would no longer
be introspectable. Integers / booleans would require abnormal formatting
by clients. API stability / deprecation promises can no longer be made.
and more besides.

Given that limitation, if we did go the string pairs route, I would
expect it to be marked as "unstable" in the QAPI schema, so apps have
a suitable warning NOT to rely on this.

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 :|

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

[Ilya Dryomov]

On Mon, Jun 16, 2025 at 11:52 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Mon, Jun 16, 2025 at 11:25:54AM +0200, Ilya Dryomov wrote:
> > On Thu, May 15, 2025 at 1:29 PM Fiona Ebner <f.ebner@proxmox.com> 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>
>
> snip
>
> > >  ##
> > >  # @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',
> >
> > To side-step all of the above, have you considered implementing
> > a straightforward passthrough to Ceph instead?  Something like
> >
> >   '*key-value-pairs': ['RbdKeyValuePair']
> >
> > where RbdKeyValuePair is just a pair arbitrary strings (and
> > key-value-pairs is thus an optional list of those).  rados_conf_set()
> > would be called just the same but the user would be able to override
> > any Ceph option they wish, not just a few that we thought of here.
>
> Passing through arbitrary key/value pairs as strings is essentially
> abdicating our design responsibility in QAPI. enums would no longer
> be introspectable. Integers / booleans would require abnormal formatting
> by clients. API stability / deprecation promises can no longer be made.
> and more besides.
>
> Given that limitation, if we did go the string pairs route, I would
> expect it to be marked as "unstable" in the QAPI schema, so apps have
> a suitable warning NOT to rely on this.

This sounds sensible to me.  We can continue exposing the most common
Ceph options through a proper QAPI schema but add key-value-pairs as an
alternative low-level route for those who want to avoid dealing with
physical configuration files.

Thanks,

                Ilya

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

[Fiona Ebner]

Am 16.06.25 um 11:41 schrieb Daniel P. Berrangé:
> On Thu, May 15, 2025 at 01:29:07PM +0200, Fiona Ebner wrote:
>> @@ -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.

Am 16.06.25 um 11:25 schrieb Ilya Dryomov:
> Hi Fiona,
> 
> I'm not following the rationale for introducing RbdKeyValuePairs
> struct.  If there is a desire to expose rbd_cache_policy option this
> way, couldn't it just be added to BlockdevOptionsRbd struct?  The
> existing auth_client_required option has a very similar pattern.

Hi Ilya and Daniel,

my intention was to not "pollute" the top-level struct with rather
uncommon options, but collect them separately and also make it explicit
that those are the key-value pairs passed along to Ceph.

> If exposing rbd_cache_policy option, rbd_cache option (enabled/disabled
> bool) should likely be exposed as well.  It doesn't make much sense to
> provide a built-in way to adjust the cache policy without also providing
> a built-in way to disable the cache entirely.  Then the question of what
> would be better from the QAPI perspective arises: a bool option to map
> to Ceph as close as possible or perhaps an additional 'disabled' value
> in RbdCachePolicy enum?  And regardless of that, the need to remember
> to update QEMU if a new cache policy is ever added because even though
> these are just strings, QEMU is going to be validating them...

Good point! If going with the key-value-pairs approach, it would be
nicer to go with a direct mapping IMHO. If adding it to the top-level,
I'd be in favor of the 'disabled' value.

Best Regards,
Fiona

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

[Daniel P. Berrangé]

On Mon, Jun 16, 2025 at 02:29:56PM +0200, Fiona Ebner wrote:
> Am 16.06.25 um 11:41 schrieb Daniel P. Berrangé:
> > On Thu, May 15, 2025 at 01:29:07PM +0200, Fiona Ebner wrote:
> >> @@ -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.
> 
> Am 16.06.25 um 11:25 schrieb Ilya Dryomov:
> > Hi Fiona,
> > 
> > I'm not following the rationale for introducing RbdKeyValuePairs
> > struct.  If there is a desire to expose rbd_cache_policy option this
> > way, couldn't it just be added to BlockdevOptionsRbd struct?  The
> > existing auth_client_required option has a very similar pattern.
> 
> Hi Ilya and Daniel,
> 
> my intention was to not "pollute" the top-level struct with rather
> uncommon options, but collect them separately and also make it explicit
> that those are the key-value pairs passed along to Ceph.

If these are useful things that users of ceph need to be able to
control for their QEMU VMs, then this is not "pollution".

> > If exposing rbd_cache_policy option, rbd_cache option (enabled/disabled
> > bool) should likely be exposed as well.  It doesn't make much sense to
> > provide a built-in way to adjust the cache policy without also providing
> > a built-in way to disable the cache entirely.  Then the question of what
> > would be better from the QAPI perspective arises: a bool option to map
> > to Ceph as close as possible or perhaps an additional 'disabled' value
> > in RbdCachePolicy enum?  And regardless of that, the need to remember
> > to update QEMU if a new cache policy is ever added because even though
> > these are just strings, QEMU is going to be validating them...
> 
> Good point! If going with the key-value-pairs approach, it would be
> nicer to go with a direct mapping IMHO. If adding it to the top-level,
> I'd be in favor of the 'disabled' value.

If this is stuff that users expect/need to be able to configure for
their VMs, that would bias towards formal modelling in QAPI, not plain
passthrough.

NB, any features intended to be configurable via libvirt would need
to be formalling modelled, as libvirt won't introduce a dependency
on things that QEMU declares "unstable" in QAPI.

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 :|

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

[Fiona Ebner]

Am 16.06.25 um 12:28 schrieb Ilya Dryomov:
> On Mon, Jun 16, 2025 at 11:52 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>> On Mon, Jun 16, 2025 at 11:25:54AM +0200, Ilya Dryomov wrote:
>>> On Thu, May 15, 2025 at 1:29 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>>>>  ##
>>>>  # @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',
>>>
>>> To side-step all of the above, have you considered implementing
>>> a straightforward passthrough to Ceph instead?  Something like
>>>
>>>   '*key-value-pairs': ['RbdKeyValuePair']
>>>
>>> where RbdKeyValuePair is just a pair arbitrary strings (and
>>> key-value-pairs is thus an optional list of those).  rados_conf_set()
>>> would be called just the same but the user would be able to override
>>> any Ceph option they wish, not just a few that we thought of here.
>>
>> Passing through arbitrary key/value pairs as strings is essentially
>> abdicating our design responsibility in QAPI. enums would no longer
>> be introspectable. Integers / booleans would require abnormal formatting
>> by clients. API stability / deprecation promises can no longer be made.
>> and more besides.

Yes, and I also was under the impression that there is no desire to
re-introduce arbitrary key-value pairs with QMP/blockdev options.

>> Given that limitation, if we did go the string pairs route, I would
>> expect it to be marked as "unstable" in the QAPI schema, so apps have
>> a suitable warning NOT to rely on this.
> 
> This sounds sensible to me.  We can continue exposing the most common
> Ceph options through a proper QAPI schema but add key-value-pairs as an
> alternative low-level route for those who want to avoid dealing with
> physical configuration files.

As written in the commit message, the cache option should not apply to
all volumes, so using configuration files is rather impractical there.

I'd prefer defining the cache option(s) explicitly, and have people add
additional key-value pairs they require explicitly going forward. But if
you really don't want me to, I can still go with the unstable, arbitrary
strings approach instead.

Best Regards,
Fiona

Re: [PATCH 2/2] block/rbd: support keyring option via QAPI

[Fiona Ebner]

Am 16.06.25 um 11:34 schrieb Ilya Dryomov:
> On Thu, May 15, 2025 at 1:29 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>>
>> In Proxmox VE, it is not always required to have a dedicated Ceph
>> configuration file, and using the 'key-secret' QAPI option would
>> require obtaining a key from the keyring first. The keyring location
>> is readily available however, so having support for the 'keyring'
>> configuration option is most convenient.
> 
> Would such a setup have a ceph.conf file that is shared between
> multiple users (or no ceph.conf file at all if the monitors are
> specified via QAPI option) but individual keyring files for each
> user?

There is only a single Ceph user and we could create a ceph.conf file
with the 'keyring' option set. It was just not required in the past,
because we specified 'keyring' via '-drive' directly, so having this
option would be more convenient for us.

In short: we can still make it work on our side if there is no interest
in adding this option in the QAPI.

Best Regards,
Fiona

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

[Ilya Dryomov]

On Mon, Jun 16, 2025 at 2:38 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>
> Am 16.06.25 um 12:28 schrieb Ilya Dryomov:
> > On Mon, Jun 16, 2025 at 11:52 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >> On Mon, Jun 16, 2025 at 11:25:54AM +0200, Ilya Dryomov wrote:
> >>> On Thu, May 15, 2025 at 1:29 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
> >>>>  ##
> >>>>  # @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',
> >>>
> >>> To side-step all of the above, have you considered implementing
> >>> a straightforward passthrough to Ceph instead?  Something like
> >>>
> >>>   '*key-value-pairs': ['RbdKeyValuePair']
> >>>
> >>> where RbdKeyValuePair is just a pair arbitrary strings (and
> >>> key-value-pairs is thus an optional list of those).  rados_conf_set()
> >>> would be called just the same but the user would be able to override
> >>> any Ceph option they wish, not just a few that we thought of here.
> >>
> >> Passing through arbitrary key/value pairs as strings is essentially
> >> abdicating our design responsibility in QAPI. enums would no longer
> >> be introspectable. Integers / booleans would require abnormal formatting
> >> by clients. API stability / deprecation promises can no longer be made.
> >> and more besides.
>
> Yes, and I also was under the impression that there is no desire to
> re-introduce arbitrary key-value pairs with QMP/blockdev options.

Hi Fiona,

What do you mean by re-introduce?

>
> >> Given that limitation, if we did go the string pairs route, I would
> >> expect it to be marked as "unstable" in the QAPI schema, so apps have
> >> a suitable warning NOT to rely on this.
> >
> > This sounds sensible to me.  We can continue exposing the most common
> > Ceph options through a proper QAPI schema but add key-value-pairs as an
> > alternative low-level route for those who want to avoid dealing with
> > physical configuration files.
>
> As written in the commit message, the cache option should not apply to
> all volumes, so using configuration files is rather impractical there.
>
> I'd prefer defining the cache option(s) explicitly, and have people add
> additional key-value pairs they require explicitly going forward. But if
> you really don't want me to, I can still go with the unstable, arbitrary
> strings approach instead.

The RBD cache policy option would definitely count as one of the most
common, so I don't have an objection to it being added in an explicit
form.  I'm also fine with the "disabled" enum value that you expressed
a preference for in another email.

Thanks,

                Ilya

Re: [PATCH 2/2] block/rbd: support keyring option via QAPI

[Ilya Dryomov]

On Mon, Jun 16, 2025 at 2:51 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>
> Am 16.06.25 um 11:34 schrieb Ilya Dryomov:
> > On Thu, May 15, 2025 at 1:29 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
> >>
> >> In Proxmox VE, it is not always required to have a dedicated Ceph
> >> configuration file, and using the 'key-secret' QAPI option would
> >> require obtaining a key from the keyring first. The keyring location
> >> is readily available however, so having support for the 'keyring'
> >> configuration option is most convenient.
> >
> > Would such a setup have a ceph.conf file that is shared between
> > multiple users (or no ceph.conf file at all if the monitors are
> > specified via QAPI option) but individual keyring files for each
> > user?
>
> There is only a single Ceph user and we could create a ceph.conf file
> with the 'keyring' option set. It was just not required in the past,
> because we specified 'keyring' via '-drive' directly, so having this
> option would be more convenient for us.
>
> In short: we can still make it work on our side if there is no interest
> in adding this option in the QAPI.

I don't have a strong opinion, but it feels a bit like circumventing
the QAPI secret infrastructure.  It's already possible to circumvent it
indirectly through the keyring option in ceph.conf file but that is
something that falls out naturally and has always been there.  Adding
a more direct way to do it has me split...

Thanks,

                Ilya

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

[Ilya Dryomov]

On Thu, Jun 19, 2025 at 8:38 PM Ilya Dryomov <idryomov@gmail.com> wrote:
>
> On Mon, Jun 16, 2025 at 2:38 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
> >
> > Am 16.06.25 um 12:28 schrieb Ilya Dryomov:
> > > On Mon, Jun 16, 2025 at 11:52 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > >> On Mon, Jun 16, 2025 at 11:25:54AM +0200, Ilya Dryomov wrote:
> > >>> On Thu, May 15, 2025 at 1:29 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
> > >>>>  ##
> > >>>>  # @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',
> > >>>
> > >>> To side-step all of the above, have you considered implementing
> > >>> a straightforward passthrough to Ceph instead?  Something like
> > >>>
> > >>>   '*key-value-pairs': ['RbdKeyValuePair']
> > >>>
> > >>> where RbdKeyValuePair is just a pair arbitrary strings (and
> > >>> key-value-pairs is thus an optional list of those).  rados_conf_set()
> > >>> would be called just the same but the user would be able to override
> > >>> any Ceph option they wish, not just a few that we thought of here.
> > >>
> > >> Passing through arbitrary key/value pairs as strings is essentially
> > >> abdicating our design responsibility in QAPI. enums would no longer
> > >> be introspectable. Integers / booleans would require abnormal formatting
> > >> by clients. API stability / deprecation promises can no longer be made.
> > >> and more besides.
> >
> > Yes, and I also was under the impression that there is no desire to
> > re-introduce arbitrary key-value pairs with QMP/blockdev options.
>
> Hi Fiona,
>
> What do you mean by re-introduce?
>
> >
> > >> Given that limitation, if we did go the string pairs route, I would
> > >> expect it to be marked as "unstable" in the QAPI schema, so apps have
> > >> a suitable warning NOT to rely on this.
> > >
> > > This sounds sensible to me.  We can continue exposing the most common
> > > Ceph options through a proper QAPI schema but add key-value-pairs as an
> > > alternative low-level route for those who want to avoid dealing with
> > > physical configuration files.
> >
> > As written in the commit message, the cache option should not apply to
> > all volumes, so using configuration files is rather impractical there.
> >
> > I'd prefer defining the cache option(s) explicitly, and have people add
> > additional key-value pairs they require explicitly going forward. But if
> > you really don't want me to, I can still go with the unstable, arbitrary
> > strings approach instead.
>
> The RBD cache policy option would definitely count as one of the most
> common, so I don't have an objection to it being added in an explicit
> form.  I'm also fine with the "disabled" enum value that you expressed
> a preference for in another email.

The QEMU block layer-wide "cache" option is kind of in the way though:
if it's set to "off"/"none" or the more specific "cache.direct" option
is set to "on", we disable the RBD cache.  So there is an existing way
to control that, but it's at another level and QEMU cache modes aren't
distinguished (i.e. there is no mapping to RBD which means that one can
have "cache=writeback" set in QEMU but still get e.g. "writethrough"
RBD cache policy come from the ceph.conf file).  An extra enum value
for disabling the RBD cache might muddy the waters further.

Another thing that comes to mind is that if you need to control the
cache policy (or any other RBD option) on a per-image basis as opposed
to per-user basis, you can employ image-level configuration overrides
on the RBD side -- see "rbd config image get/set/ls/rm" commands.

Thanks,

                Ilya

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

[Fiona Ebner]

Am 19.06.25 um 23:20 schrieb Ilya Dryomov:
> On Thu, Jun 19, 2025 at 8:38 PM Ilya Dryomov <idryomov@gmail.com> wrote:
>>
>> On Mon, Jun 16, 2025 at 2:38 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>>>
>>> Am 16.06.25 um 12:28 schrieb Ilya Dryomov:
>>>> On Mon, Jun 16, 2025 at 11:52 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>>> On Mon, Jun 16, 2025 at 11:25:54AM +0200, Ilya Dryomov wrote:
>>>>>> On Thu, May 15, 2025 at 1:29 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>>>>>>>  ##
>>>>>>>  # @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',
>>>>>>
>>>>>> To side-step all of the above, have you considered implementing
>>>>>> a straightforward passthrough to Ceph instead?  Something like
>>>>>>
>>>>>>   '*key-value-pairs': ['RbdKeyValuePair']
>>>>>>
>>>>>> where RbdKeyValuePair is just a pair arbitrary strings (and
>>>>>> key-value-pairs is thus an optional list of those).  rados_conf_set()
>>>>>> would be called just the same but the user would be able to override
>>>>>> any Ceph option they wish, not just a few that we thought of here.
>>>>>
>>>>> Passing through arbitrary key/value pairs as strings is essentially
>>>>> abdicating our design responsibility in QAPI. enums would no longer
>>>>> be introspectable. Integers / booleans would require abnormal formatting
>>>>> by clients. API stability / deprecation promises can no longer be made.
>>>>> and more besides.
>>>
>>> Yes, and I also was under the impression that there is no desire to
>>> re-introduce arbitrary key-value pairs with QMP/blockdev options.
>>
>> Hi Fiona,
>>
>> What do you mean by re-introduce?

Sorry, should've stated this better. It is possible to specify arbitrary
key-value pairs with '-drive', and by "re-introduce" I mean allowing the
same for '-blockdev'.

>>>>> Given that limitation, if we did go the string pairs route, I would
>>>>> expect it to be marked as "unstable" in the QAPI schema, so apps have
>>>>> a suitable warning NOT to rely on this.
>>>>
>>>> This sounds sensible to me.  We can continue exposing the most common
>>>> Ceph options through a proper QAPI schema but add key-value-pairs as an
>>>> alternative low-level route for those who want to avoid dealing with
>>>> physical configuration files.
>>>
>>> As written in the commit message, the cache option should not apply to
>>> all volumes, so using configuration files is rather impractical there.
>>>
>>> I'd prefer defining the cache option(s) explicitly, and have people add
>>> additional key-value pairs they require explicitly going forward. But if
>>> you really don't want me to, I can still go with the unstable, arbitrary
>>> strings approach instead.
>>
>> The RBD cache policy option would definitely count as one of the most
>> common, so I don't have an objection to it being added in an explicit
>> form.  I'm also fine with the "disabled" enum value that you expressed
>> a preference for in another email.
> 
> The QEMU block layer-wide "cache" option is kind of in the way though:
> if it's set to "off"/"none" or the more specific "cache.direct" option
> is set to "on", we disable the RBD cache.  So there is an existing way
> to control that, but it's at another level and QEMU cache modes aren't
> distinguished (i.e. there is no mapping to RBD which means that one can
> have "cache=writeback" set in QEMU but still get e.g. "writethrough"
> RBD cache policy come from the ceph.conf file).  An extra enum value
> for disabling the RBD cache might muddy the waters further.

Even with "cache=writeback" in QEMU, the RBD cache policy will default
to "writearound". And we do need "writeback" on the RBD side too:
https://git.proxmox.com/?p=qemu-server.git;a=commitdiff;h=738dc81cbae03ff03592c30a82c7e4d0d39a54ba;hp=e43b19109e57e6e7654c4fa6e2ed14a39b4a6fe2

> Another thing that comes to mind is that if you need to control the
> cache policy (or any other RBD option) on a per-image basis as opposed
> to per-user basis, you can employ image-level configuration overrides
> on the RBD side -- see "rbd config image get/set/ls/rm" commands.

Oh, that sounds like a good alternative :)

Best Regards,
Fiona