From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id E9F5E90E2B for ; Thu, 25 Jan 2024 15:42:01 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 126951BC5C for ; Thu, 25 Jan 2024 15:42:00 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 25 Jan 2024 15:41:55 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 91F934929A for ; Thu, 25 Jan 2024 15:41:55 +0100 (CET) From: Fiona Ebner To: pve-devel@lists.proxmox.com Date: Thu, 25 Jan 2024 15:41:41 +0100 Message-Id: <20240125144149.216064-6-f.ebner@proxmox.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20240125144149.216064-1-f.ebner@proxmox.com> References: <20240125144149.216064-1-f.ebner@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.074 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: [pve-devel] [RFC qemu 05/13] qapi: blockdev-backup: add discard-source parameter X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Jan 2024 14:42:02 -0000 From: Vladimir Sementsov-Ogievskiy Add a parameter that enables discard-after-copy. That is mostly useful in "push backup with fleecing" scheme, when source is snapshot-access format driver node, based on copy-before-write filter snapshot-access API: [guest] [snapshot-access] ~~ blockdev-backup ~~> [backup target] | | | root | file v v [copy-before-write] | | | file | target v v [active disk] [temp.img] In this case discard-after-copy does two things: - discard data in temp.img to save disk space - avoid further copy-before-write operation in discarded area Note that we have to declare WRITE permission on source in copy-before-write filter, for discard to work. This is done via a new source-write-perm open option for copy-before-write, because it needs to happen conditionally only when discard_source is set. It cannot be set for usual backup, because there, the associated virtual hardware device already requires exclusive write. Signed-off-by: Vladimir Sementsov-Ogievskiy [FE: adapt for Proxmox downstream 8.1 fix discard at end of image when not aligned to cluster size add an option option for setting the write permission on the cbw source child conditionally during open and adapt commit message] Signed-off-by: Fiona Ebner --- It'd be better if the source-write-perm for copy-before-write would not be exposed via QAPI, but only internal. block/backup.c | 7 ++++--- block/block-copy.c | 11 +++++++++-- block/copy-before-write.c | 14 ++++++++++++++ block/copy-before-write.h | 1 + block/replication.c | 4 ++-- blockdev.c | 2 +- include/block/block-copy.h | 2 +- include/block/block_int-global-state.h | 2 +- pve-backup.c | 2 +- qapi/block-core.json | 10 +++++++++- 10 files changed, 43 insertions(+), 12 deletions(-) diff --git a/block/backup.c b/block/backup.c index af87fa6aa9..51f9d28115 100644 --- a/block/backup.c +++ b/block/backup.c @@ -332,7 +332,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, BlockDriverState *target, int64_t speed, MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap, BitmapSyncMode bitmap_mode, - bool compress, + bool compress, bool discard_source, const char *filter_node_name, BackupPerf *perf, BlockdevOnError on_source_error, @@ -429,7 +429,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, goto error; } - cbw = bdrv_cbw_append(bs, target, filter_node_name, &bcs, errp); + cbw = bdrv_cbw_append(bs, target, filter_node_name, discard_source, &bcs, errp); if (!cbw) { goto error; } @@ -471,7 +471,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, job->len = len; job->perf = *perf; - block_copy_set_copy_opts(bcs, perf->use_copy_range, compress); + block_copy_set_copy_opts(bcs, perf->use_copy_range, compress, + discard_source); block_copy_set_progress_meter(bcs, &job->common.job.progress); block_copy_set_speed(bcs, speed); diff --git a/block/block-copy.c b/block/block-copy.c index b61685f1a2..05cfccfda8 100644 --- a/block/block-copy.c +++ b/block/block-copy.c @@ -137,6 +137,7 @@ typedef struct BlockCopyState { CoMutex lock; int64_t in_flight_bytes; BlockCopyMethod method; + bool discard_source; BlockReqList reqs; QLIST_HEAD(, BlockCopyCallState) calls; /* @@ -282,11 +283,12 @@ static uint32_t block_copy_max_transfer(BdrvChild *source, BdrvChild *target) } void block_copy_set_copy_opts(BlockCopyState *s, bool use_copy_range, - bool compress) + bool compress, bool discard_source) { /* Keep BDRV_REQ_SERIALISING set (or not set) in block_copy_state_new() */ s->write_flags = (s->write_flags & BDRV_REQ_SERIALISING) | (compress ? BDRV_REQ_WRITE_COMPRESSED : 0); + s->discard_source = discard_source; if (s->max_transfer < s->cluster_size) { /* @@ -409,7 +411,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target, cluster_size), }; - block_copy_set_copy_opts(s, false, false); + block_copy_set_copy_opts(s, false, false, false); ratelimit_init(&s->rate_limit); qemu_co_mutex_init(&s->lock); @@ -580,6 +582,11 @@ static coroutine_fn int block_copy_task_entry(AioTask *task) co_put_to_shres(s->mem, t->req.bytes); block_copy_task_end(t, ret); + if (s->discard_source && ret == 0) { + int64_t nbytes = MIN(t->req.offset + t->req.bytes, s->len) - t->req.offset; + bdrv_co_pdiscard(s->source, t->req.offset, nbytes); + } + return ret; } diff --git a/block/copy-before-write.c b/block/copy-before-write.c index 41cb0955c9..87f047ad5f 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -44,6 +44,7 @@ typedef struct BDRVCopyBeforeWriteState { BdrvChild *target; OnCbwError on_cbw_error; uint32_t cbw_timeout_ns; + bool source_write_perm; /* * @lock: protects access to @access_bitmap, @done_bitmap and @@ -363,6 +364,11 @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c, bdrv_default_perms(bs, c, role, reopen_queue, perm, shared, nperm, nshared); + BDRVCopyBeforeWriteState *s = bs->opaque; + if (s->source_write_perm) { + *nperm = *nperm | BLK_PERM_WRITE; + } + if (!QLIST_EMPTY(&bs->parents)) { if (perm & BLK_PERM_WRITE) { *nperm = *nperm | BLK_PERM_CONSISTENT_READ; @@ -422,6 +428,12 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags, assert(full_opts->driver == BLOCKDEV_DRIVER_COPY_BEFORE_WRITE); opts = &full_opts->u.copy_before_write; + if (opts->source_write_perm) { + s->source_write_perm = true; + } else { + s->source_write_perm = false; + } + ret = bdrv_open_file_child(NULL, options, "file", bs, errp); if (ret < 0) { return ret; @@ -530,6 +542,7 @@ BlockDriver bdrv_cbw_filter = { BlockDriverState *bdrv_cbw_append(BlockDriverState *source, BlockDriverState *target, const char *filter_node_name, + bool source_write_perm, BlockCopyState **bcs, Error **errp) { @@ -547,6 +560,7 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, } qdict_put_str(opts, "file", bdrv_get_node_name(source)); qdict_put_str(opts, "target", bdrv_get_node_name(target)); + qdict_put_bool(opts, "source-write-perm", source_write_perm); top = bdrv_insert_node(source, opts, BDRV_O_RDWR, errp); if (!top) { diff --git a/block/copy-before-write.h b/block/copy-before-write.h index 6e72bb25e9..03ee708d10 100644 --- a/block/copy-before-write.h +++ b/block/copy-before-write.h @@ -39,6 +39,7 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, BlockDriverState *target, const char *filter_node_name, + bool source_write_perm, BlockCopyState **bcs, Error **errp); void bdrv_cbw_drop(BlockDriverState *bs); diff --git a/block/replication.c b/block/replication.c index ea4bf1aa80..39ad78cf98 100644 --- a/block/replication.c +++ b/block/replication.c @@ -579,8 +579,8 @@ 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, NULL, - &perf, + 0, MIRROR_SYNC_MODE_NONE, NULL, 0, false, false, + NULL, &perf, 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 7793143d76..ce3fef924c 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2802,7 +2802,7 @@ static BlockJob *do_backup_common(BackupCommon *backup, job = backup_job_create(backup->job_id, bs, target_bs, backup->speed, backup->sync, bmap, backup->bitmap_mode, - backup->compress, + backup->compress, backup->discard_source, backup->filter_node_name, &perf, backup->on_source_error, diff --git a/include/block/block-copy.h b/include/block/block-copy.h index 8b41643bfa..9555de2562 100644 --- a/include/block/block-copy.h +++ b/include/block/block-copy.h @@ -31,7 +31,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target, /* Function should be called prior any actual copy request */ void block_copy_set_copy_opts(BlockCopyState *s, bool use_copy_range, - bool compress); + bool compress, bool discard_source); void block_copy_set_progress_meter(BlockCopyState *s, ProgressMeter *pm); void block_copy_state_free(BlockCopyState *s); diff --git a/include/block/block_int-global-state.h b/include/block/block_int-global-state.h index 32f0f9858a..546f2b5532 100644 --- a/include/block/block_int-global-state.h +++ b/include/block/block_int-global-state.h @@ -189,7 +189,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap, BitmapSyncMode bitmap_mode, - bool compress, + bool compress, bool discard_source, const char *filter_node_name, BackupPerf *perf, BlockdevOnError on_source_error, diff --git a/pve-backup.c b/pve-backup.c index 4b0dcca246..4bf641ce5a 100644 --- a/pve-backup.c +++ b/pve-backup.c @@ -518,7 +518,7 @@ static void create_backup_jobs_bh(void *opaque) { BlockJob *job = backup_job_create( NULL, di->bs, di->target, backup_state.speed, sync_mode, di->bitmap, - bitmap_mode, false, NULL, &backup_state.perf, BLOCKDEV_ON_ERROR_REPORT, + bitmap_mode, false, false, NULL, &backup_state.perf, BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT, JOB_DEFAULT, pvebackup_complete_cb, di, backup_state.txn, &local_err); diff --git a/qapi/block-core.json b/qapi/block-core.json index 48eb47c6ea..51ce786bc5 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1816,6 +1816,9 @@ # node specified by @drive. If this option is not given, a node # name is autogenerated. (Since: 4.2) # +# @discard-source: Discard blocks on source which are already copied to the +# target. (Since 8.1) +# # @x-perf: Performance options. (Since 6.0) # # Features: @@ -1837,6 +1840,7 @@ '*on-target-error': 'BlockdevOnError', '*auto-finalize': 'bool', '*auto-dismiss': 'bool', '*filter-node-name': 'str', + '*discard-source': 'bool', '*x-perf': { 'type': 'BackupPerf', 'features': [ 'unstable' ] } } } @@ -4817,12 +4821,16 @@ # @on-cbw-error parameter will decide how this failure is handled. # Default 0. (Since 7.1) # +# @source-write-perm: For internal use only. Set write permission +# for the source child. (Since 8.1) +# # Since: 6.2 ## { 'struct': 'BlockdevOptionsCbw', 'base': 'BlockdevOptionsGenericFormat', 'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap', - '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32' } } + '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32', + '*source-write-perm': 'bool' } } ## # @BlockdevOptions: -- 2.39.2