Re: [pve-devel] [RFC qemu 3/7] block/backup: allow passing additional options for copy-before-write upon job creation

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

Quoting Fiona Ebner (2024-06-10 14:59:38)
> In particular, useful for setting the 'on-cbw-error' and 'cbw-timeout'
> options (see BlockdevOptionsCbw in QAPI).
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  block/backup.c                         | 10 +++++++---
>  block/replication.c                    |  2 +-
>  blockdev.c                             |  2 +-
>  include/block/block_int-global-state.h |  2 ++
>  pve-backup.c                           |  2 +-
>  5 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index e0acfe6758..82fedf1680 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -336,6 +336,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>                    bool compress, bool discard_source,
>                    const char *filter_node_name,
>                    BackupPerf *perf,
> +                  QDict *cbw_opts,
>                    BlockdevOnError on_source_error,
>                    BlockdevOnError on_target_error,
>                    int creation_flags,
> @@ -347,7 +348,6 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>      int64_t cluster_size;
>      BlockDriverState *cbw = NULL;
>      BlockCopyState *bcs = NULL;
> -    QDict *cbw_opts = NULL;
>  
>      assert(bs);
>      assert(target);
> @@ -436,8 +436,12 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>      }
>  
>      if (perf->has_min_cluster_size) {
> -        cbw_opts = qdict_new();
> -        qdict_put_int(cbw_opts, "min-cluster-size", perf->min_cluster_size);
> +        if (!cbw_opts) {
> +            cbw_opts = qdict_new();
> +        }
> +        if (!qdict_haskey(cbw_opts, "min-cluster-size")) {
> +            qdict_put_int(cbw_opts, "min-cluster-size", perf->min_cluster_size);

so here cbw_opts "wins" without any indication that this happens

> +        }
>      }
>  
>      cbw = bdrv_cbw_append(bs, target, filter_node_name, discard_source,
> diff --git a/block/replication.c b/block/replication.c
> index 0415a5e8b7..c5a27f593e 100644
> --- a/block/replication.c
> +++ b/block/replication.c
> @@ -583,7 +583,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
>          s->backup_job = backup_job_create(
>                                  NULL, s->secondary_disk->bs, s->hidden_disk->bs,
>                                  0, MIRROR_SYNC_MODE_NONE, NULL, 0, false, false,
> -                                NULL, &perf,
> +                                NULL, &perf, NULL,
>                                  BLOCKDEV_ON_ERROR_REPORT,
>                                  BLOCKDEV_ON_ERROR_REPORT, JOB_INTERNAL,
>                                  backup_job_completed, bs, NULL, &local_err);
> diff --git a/blockdev.c b/blockdev.c
> index cbe224387b..55ca967430 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2732,7 +2732,7 @@ static BlockJob *do_backup_common(BackupCommon *backup,
>                              backup->sync, bmap, backup->bitmap_mode,
>                              backup->compress, backup->discard_source,
>                              backup->filter_node_name,
> -                            &perf,
> +                            &perf, NULL,
>                              backup->on_source_error,
>                              backup->on_target_error,
>                              job_flags, NULL, NULL, txn, errp);
> diff --git a/include/block/block_int-global-state.h b/include/block/block_int-global-state.h
> index f0c642b194..7332680087 100644
> --- a/include/block/block_int-global-state.h
> +++ b/include/block/block_int-global-state.h
> @@ -179,6 +179,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
>   * @bitmap_mode: The bitmap synchronization policy to use.
>   * @perf: Performance options. All actual fields assumed to be present,
>   *        all ".has_*" fields are ignored.
> + * @cbw_opts: Additional options to configure cbw filter with.

from this, I'd have guessed the other way round ;) should the precedence be
made explicit somewhere? or, for this particular option, should the higher
value win? but such logic might quickly get complicated once more parameters
need to be handled..

>   * @on_source_error: The action to take upon error reading from the source.
>   * @on_target_error: The action to take upon error writing to the target.
>   * @creation_flags: Flags that control the behavior of the Job lifetime.
> @@ -198,6 +199,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>                              bool compress, bool discard_source,
>                              const char *filter_node_name,
>                              BackupPerf *perf,
> +                            QDict *cbw_opts,
>                              BlockdevOnError on_source_error,
>                              BlockdevOnError on_target_error,
>                              int creation_flags,
> diff --git a/pve-backup.c b/pve-backup.c
> index 4e80a9f283..108e185a20 100644
> --- a/pve-backup.c
> +++ b/pve-backup.c
> @@ -635,7 +635,7 @@ static void create_backup_jobs_bh(void *opaque) {
>  
>          BlockJob *job = backup_job_create(
>              job_id, source_bs, di->target, backup_state.speed, sync_mode, di->bitmap,
> -            bitmap_mode, false, discard_source, NULL, &perf, BLOCKDEV_ON_ERROR_REPORT,
> +            bitmap_mode, false, discard_source, NULL, &perf, NULL, BLOCKDEV_ON_ERROR_REPORT,
>              BLOCKDEV_ON_ERROR_REPORT, JOB_DEFAULT, pvebackup_complete_cb, di, backup_state.txn,
>              &local_err);
>  
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel