Re: [PATCH v4 4/5] qapi: blockdev-backup: add discard-source parameter

2024-03-14 Thread Vladimir Sementsov-Ogievskiy

On 13.03.24 19:08, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


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
vv
[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. Still we can't take it
unconditionally, as it will break normal backup from RO source. So, we
have to add a parameter and pass it thorough bdrv_open flags.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Fiona Ebner 
Tested-by: Fiona Ebner 


[...]


diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1874f880a8..2ef52ae9a7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1610,6 +1610,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


"have been copied"?


Oh, right




+# to the target.  (Since 9.1)
+#
  # @x-perf: Performance options.  (Since 6.0)
  #
  # Features:
@@ -1631,6 +1634,7 @@
  '*on-target-error': 'BlockdevOnError',
  '*auto-finalize': 'bool', '*auto-dismiss': 'bool',
  '*filter-node-name': 'str',
+'*discard-source': 'bool',
  '*x-perf': { 'type': 'BackupPerf',
   'features': [ 'unstable' ] } } }


QAPI schema
Acked-by: Markus Armbruster 



Thanks!

--
Best regards,
Vladimir




Re: [PATCH v4 4/5] qapi: blockdev-backup: add discard-source parameter

2024-03-13 Thread Markus Armbruster
Vladimir Sementsov-Ogievskiy  writes:

> 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
>vv
> [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. Still we can't take it
> unconditionally, as it will break normal backup from RO source. So, we
> have to add a parameter and pass it thorough bdrv_open flags.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Reviewed-by: Fiona Ebner 
> Tested-by: Fiona Ebner 

[...]

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 1874f880a8..2ef52ae9a7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1610,6 +1610,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

"have been copied"?

> +# to the target.  (Since 9.1)
> +#
>  # @x-perf: Performance options.  (Since 6.0)
>  #
>  # Features:
> @@ -1631,6 +1634,7 @@
>  '*on-target-error': 'BlockdevOnError',
>  '*auto-finalize': 'bool', '*auto-dismiss': 'bool',
>  '*filter-node-name': 'str',
> +'*discard-source': 'bool',
>  '*x-perf': { 'type': 'BackupPerf',
>   'features': [ 'unstable' ] } } }

QAPI schema
Acked-by: Markus Armbruster 




[PATCH v4 4/5] qapi: blockdev-backup: add discard-source parameter

2024-03-13 Thread 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
   vv
[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. Still we can't take it
unconditionally, as it will break normal backup from RO source. So, we
have to add a parameter and pass it thorough bdrv_open flags.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Fiona Ebner 
Tested-by: Fiona Ebner 
---
 block/backup.c |  5 +++--
 block/block-copy.c |  9 +
 block/copy-before-write.c  | 15 +--
 block/copy-before-write.h  |  1 +
 block/replication.c|  4 ++--
 blockdev.c |  2 +-
 include/block/block-common.h   |  2 ++
 include/block/block-copy.h |  1 +
 include/block/block_int-global-state.h |  2 +-
 qapi/block-core.json   |  4 
 10 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index ec29d6b810..3dd2e229d2 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -356,7 +356,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,
@@ -457,7 +457,8 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 goto error;
 }
 
-cbw = bdrv_cbw_append(bs, target, filter_node_name, , errp);
+cbw = bdrv_cbw_append(bs, target, filter_node_name, discard_source,
+  , errp);
 if (!cbw) {
 goto error;
 }
diff --git a/block/block-copy.c b/block/block-copy.c
index 8fca2c3698..7e3b378528 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;
 /*
@@ -353,6 +354,7 @@ static int64_t 
block_copy_calculate_cluster_size(BlockDriverState *target,
 BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
  BlockDriverState *copy_bitmap_bs,
  const BdrvDirtyBitmap *bitmap,
+ bool discard_source,
  Error **errp)
 {
 ERRP_GUARD();
@@ -418,6 +420,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, 
BdrvChild *target,
 cluster_size),
 };
 
+s->discard_source = discard_source;
 block_copy_set_copy_opts(s, false, false);
 
 ratelimit_init(>rate_limit);
@@ -589,6 +592,12 @@ 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 ed2c228da7..cd65524e26 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 discard_source;
 
 /*
  * @lock: protects access to @access_bitmap, @done_bitmap and
@@ -357,6 +358,8 @@ cbw_child_perm(BlockDriverState *bs, BdrvChild *c, 
BdrvChildRole role,
uint64_t perm, uint64_t shared,
uint64_t *nperm, uint64_t *nshared)
 {
+BDRVCopyBeforeWriteState *s = bs->opaque;
+
 if (!(role & BDRV_CHILD_FILTERED)) {
 /*
  * Target child
@@ -381,6 +384,10 @@ cbw_child_perm(BlockDriverState *bs, BdrvChild *c, 
BdrvChildRole role,
  * start
  */
 *nperm = *nperm | BLK_PERM_CONSISTENT_READ;
+if (s->discard_source) {
+*nperm = *nperm |