* [PATCH v3 1/5] block/copy-before-write: fix permission
2024-02-28 14:14 [PATCH v3 0/5] backup: discard-source parameter Vladimir Sementsov-Ogievskiy
@ 2024-02-28 14:14 ` Vladimir Sementsov-Ogievskiy
2024-02-28 14:14 ` [PATCH v3 2/5] block/copy-before-write: support unligned snapshot-discard Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-02-28 14:14 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, eblake, xiechanglong.d, wencongyang2, hreitz,
kwolf, vsementsov, jsnow, f.ebner, den, alexander.ivanov
In case when source node does not have any parents, the condition still
works as required: backup job do create the parent by
block_job_create -> block_job_add_bdrv -> bdrv_root_attach_child
Still, in this case checking @perm variable doesn't work, as backup job
creates the root blk with empty permissions (as it rely on CBW filter
to require correct permissions and don't want to create extra
conflicts).
So, we should not check @perm.
The hack may be dropped entirely when transactional insertion of
filter (when we don't try to recalculate permissions in intermediate
state, when filter does conflict with original parent of the source
node) merged (old big series
"[PATCH v5 00/45] Transactional block-graph modifying API"[1] and it's
current in-flight part is "[PATCH v8 0/7] blockdev-replace"[2])
[1] https://patchew.org/QEMU/20220330212902.590099-1-vsementsov@openvz.org/
[2] https://patchew.org/QEMU/20231017184444.932733-1-vsementsov@yandex-team.ru/
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
block/copy-before-write.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 0842a1a6df..3919d495d7 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -364,9 +364,13 @@ cbw_child_perm(BlockDriverState *bs, BdrvChild *c, BdrvChildRole role,
perm, shared, nperm, nshared);
if (!QLIST_EMPTY(&bs->parents)) {
- if (perm & BLK_PERM_WRITE) {
- *nperm = *nperm | BLK_PERM_CONSISTENT_READ;
- }
+ /*
+ * Note, that source child may be shared with backup job. Backup job
+ * does create own blk parent on copy-before-write node, so this
+ * works even if source node does not have any parents before backup
+ * start
+ */
+ *nperm = *nperm | BLK_PERM_CONSISTENT_READ;
*nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v3 2/5] block/copy-before-write: support unligned snapshot-discard
2024-02-28 14:14 [PATCH v3 0/5] backup: discard-source parameter Vladimir Sementsov-Ogievskiy
2024-02-28 14:14 ` [PATCH v3 1/5] block/copy-before-write: fix permission Vladimir Sementsov-Ogievskiy
@ 2024-02-28 14:14 ` Vladimir Sementsov-Ogievskiy
2024-03-08 15:49 ` Fiona Ebner
2024-02-28 14:14 ` [PATCH v3 3/5] block/copy-before-write: create block_copy bitmap in filter node Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-02-28 14:14 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, eblake, xiechanglong.d, wencongyang2, hreitz,
kwolf, vsementsov, jsnow, f.ebner, den, alexander.ivanov
First thing that crashes on unligned access here is
bdrv_reset_dirty_bitmap(). Correct way is to align-down the
snapshot-discard request.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
block/copy-before-write.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 3919d495d7..6547eda707 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -325,14 +325,24 @@ static int coroutine_fn GRAPH_RDLOCK
cbw_co_pdiscard_snapshot(BlockDriverState *bs, int64_t offset, int64_t bytes)
{
BDRVCopyBeforeWriteState *s = bs->opaque;
+ uint32_t cluster_size = block_copy_cluster_size(s->bcs);
+ int64_t aligned_offset = QEMU_ALIGN_UP(offset, cluster_size);
+ int64_t aligned_end = QEMU_ALIGN_DOWN(offset + bytes, cluster_size);
+ int64_t aligned_bytes;
+
+ if (aligned_end <= aligned_offset) {
+ return 0;
+ }
+ aligned_bytes = aligned_end - aligned_offset;
WITH_QEMU_LOCK_GUARD(&s->lock) {
- bdrv_reset_dirty_bitmap(s->access_bitmap, offset, bytes);
+ bdrv_reset_dirty_bitmap(s->access_bitmap, aligned_offset,
+ aligned_bytes);
}
- block_copy_reset(s->bcs, offset, bytes);
+ block_copy_reset(s->bcs, aligned_offset, aligned_bytes);
- return bdrv_co_pdiscard(s->target, offset, bytes);
+ return bdrv_co_pdiscard(s->target, aligned_offset, aligned_bytes);
}
static void GRAPH_RDLOCK cbw_refresh_filename(BlockDriverState *bs)
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3 2/5] block/copy-before-write: support unligned snapshot-discard
2024-02-28 14:14 ` [PATCH v3 2/5] block/copy-before-write: support unligned snapshot-discard Vladimir Sementsov-Ogievskiy
@ 2024-03-08 15:49 ` Fiona Ebner
0 siblings, 0 replies; 11+ messages in thread
From: Fiona Ebner @ 2024-03-08 15:49 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: qemu-devel, armbru, eblake, xiechanglong.d, wencongyang2, hreitz,
kwolf, jsnow, den, alexander.ivanov
Am 28.02.24 um 15:14 schrieb Vladimir Sementsov-Ogievskiy:
> First thing that crashes on unligned access here is
> bdrv_reset_dirty_bitmap(). Correct way is to align-down the
> snapshot-discard request.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 3/5] block/copy-before-write: create block_copy bitmap in filter node
2024-02-28 14:14 [PATCH v3 0/5] backup: discard-source parameter Vladimir Sementsov-Ogievskiy
2024-02-28 14:14 ` [PATCH v3 1/5] block/copy-before-write: fix permission Vladimir Sementsov-Ogievskiy
2024-02-28 14:14 ` [PATCH v3 2/5] block/copy-before-write: support unligned snapshot-discard Vladimir Sementsov-Ogievskiy
@ 2024-02-28 14:14 ` Vladimir Sementsov-Ogievskiy
2024-03-08 15:49 ` Fiona Ebner
2024-02-28 14:15 ` [PATCH v3 4/5] qapi: blockdev-backup: add discard-source parameter Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-02-28 14:14 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, eblake, xiechanglong.d, wencongyang2, hreitz,
kwolf, vsementsov, jsnow, f.ebner, den, alexander.ivanov
Currently block_copy creates copy_bitmap in source node. But that is in
bad relation with .independent_close=true of copy-before-write filter:
source node may be detached and removed before .bdrv_close() handler
called, which should call block_copy_state_free(), which in turn should
remove copy_bitmap.
That's all not ideal: it would be better if internal bitmap of
block-copy object is not attached to any node. But that is not possible
now.
The simplest solution is just create copy_bitmap in filter node, where
anyway two other bitmaps are created.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
block/block-copy.c | 3 +-
block/copy-before-write.c | 2 +-
include/block/block-copy.h | 1 +
tests/qemu-iotests/257.out | 112 ++++++++++++++++++-------------------
4 files changed, 60 insertions(+), 58 deletions(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index 9ee3dd7ef5..8fca2c3698 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -351,6 +351,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,
Error **errp)
{
@@ -367,7 +368,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
return NULL;
}
- copy_bitmap = bdrv_create_dirty_bitmap(source->bs, cluster_size, NULL,
+ copy_bitmap = bdrv_create_dirty_bitmap(copy_bitmap_bs, cluster_size, NULL,
errp);
if (!copy_bitmap) {
return NULL;
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 6547eda707..44a9385aa7 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -467,7 +467,7 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
bs->file->bs->supported_zero_flags);
- s->bcs = block_copy_state_new(bs->file, s->target, bitmap, errp);
+ s->bcs = block_copy_state_new(bs->file, s->target, bs, bitmap, errp);
if (!s->bcs) {
error_prepend(errp, "Cannot create block-copy-state: ");
return -EINVAL;
diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index 0700953ab8..8b41643bfa 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -25,6 +25,7 @@ typedef struct BlockCopyState BlockCopyState;
typedef struct BlockCopyCallState BlockCopyCallState;
BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
+ BlockDriverState *copy_bitmap_bs,
const BdrvDirtyBitmap *bitmap,
Error **errp);
diff --git a/tests/qemu-iotests/257.out b/tests/qemu-iotests/257.out
index aa76131ca9..c33dd7f3a9 100644
--- a/tests/qemu-iotests/257.out
+++ b/tests/qemu-iotests/257.out
@@ -120,16 +120,16 @@ write -P0x67 0x3fe0000 0x20000
"granularity": 65536,
"persistent": false,
"recording": false
- }
- ],
- "drive0": [
+ },
{
"busy": false,
"count": 0,
"granularity": 65536,
"persistent": false,
"recording": false
- },
+ }
+ ],
+ "drive0": [
{
"busy": false,
"count": 458752,
@@ -596,16 +596,16 @@ write -P0x67 0x3fe0000 0x20000
"granularity": 65536,
"persistent": false,
"recording": false
- }
- ],
- "drive0": [
+ },
{
"busy": false,
"count": 0,
"granularity": 65536,
"persistent": false,
"recording": false
- },
+ }
+ ],
+ "drive0": [
{
"busy": false,
"count": 458752,
@@ -865,16 +865,16 @@ write -P0x67 0x3fe0000 0x20000
"granularity": 65536,
"persistent": false,
"recording": false
- }
- ],
- "drive0": [
+ },
{
"busy": false,
"count": 0,
"granularity": 65536,
"persistent": false,
"recording": false
- },
+ }
+ ],
+ "drive0": [
{
"busy": false,
"count": 458752,
@@ -1341,16 +1341,16 @@ write -P0x67 0x3fe0000 0x20000
"granularity": 65536,
"persistent": false,
"recording": false
- }
- ],
- "drive0": [
+ },
{
"busy": false,
"count": 0,
"granularity": 65536,
"persistent": false,
"recording": false
- },
+ }
+ ],
+ "drive0": [
{
"busy": false,
"count": 458752,
@@ -1610,16 +1610,16 @@ write -P0x67 0x3fe0000 0x20000
"granularity": 65536,
"persistent": false,
"recording": false
- }
- ],
- "drive0": [
+ },
{
"busy": false,
"count": 0,
"granularity": 65536,
"persistent": false,
"recording": false
- },
+ }
+ ],
+ "drive0": [
{
"busy": false,
"count": 458752,
@@ -2086,16 +2086,16 @@ write -P0x67 0x3fe0000 0x20000
"granularity": 65536,
"persistent": false,
"recording": false
- }
- ],
- "drive0": [
+ },
{
"busy": false,
"count": 0,
"granularity": 65536,
"persistent": false,
"recording": false
- },
+ }
+ ],
+ "drive0": [
{
"busy": false,
"count": 458752,
@@ -2355,16 +2355,16 @@ write -P0x67 0x3fe0000 0x20000
"granularity": 65536,
"persistent": false,
"recording": false
- }
- ],
- "drive0": [
+ },
{
"busy": false,
"count": 0,
"granularity": 65536,
"persistent": false,
"recording": false
- },
+ }
+ ],
+ "drive0": [
{
"busy": false,
"count": 458752,
@@ -2831,16 +2831,16 @@ write -P0x67 0x3fe0000 0x20000
"granularity": 65536,
"persistent": false,
"recording": false
- }
- ],
- "drive0": [
+ },
{
"busy": false,
"count": 0,
"granularity": 65536,
"persistent": false,
"recording": false
- },
+ }
+ ],
+ "drive0": [
{
"busy": false,
"count": 458752,
@@ -3100,16 +3100,16 @@ write -P0x67 0x3fe0000 0x20000
"granularity": 65536,
"persistent": false,
"recording": false
- }
- ],
- "drive0": [
+ },
{
"busy": false,
"count": 0,
"granularity": 65536,
"persistent": false,
"recording": false
- },
+ }
+ ],
+ "drive0": [
{
"busy": false,
"count": 458752,
@@ -3576,16 +3576,16 @@ write -P0x67 0x3fe0000 0x20000
"granularity": 65536,
"persistent": false,
"recording": false
- }
- ],
- "drive0": [
+ },
{
"busy": false,
"count": 0,
"granularity": 65536,
"persistent": false,
"recording": false
- },
+ }
+ ],
+ "drive0": [
{
"busy": false,
"count": 458752,
@@ -3845,16 +3845,16 @@ write -P0x67 0x3fe0000 0x20000
"granularity": 65536,
"persistent": false,
"recording": false
- }
- ],
- "drive0": [
+ },
{
"busy": false,
"count": 0,
"granularity": 65536,
"persistent": false,
"recording": false
- },
+ }
+ ],
+ "drive0": [
{
"busy": false,
"count": 458752,
@@ -4321,16 +4321,16 @@ write -P0x67 0x3fe0000 0x20000
"granularity": 65536,
"persistent": false,
"recording": false
- }
- ],
- "drive0": [
+ },
{
"busy": false,
"count": 0,
"granularity": 65536,
"persistent": false,
"recording": false
- },
+ }
+ ],
+ "drive0": [
{
"busy": false,
"count": 458752,
@@ -4590,16 +4590,16 @@ write -P0x67 0x3fe0000 0x20000
"granularity": 65536,
"persistent": false,
"recording": false
- }
- ],
- "drive0": [
+ },
{
"busy": false,
"count": 0,
"granularity": 65536,
"persistent": false,
"recording": false
- },
+ }
+ ],
+ "drive0": [
{
"busy": false,
"count": 458752,
@@ -5066,16 +5066,16 @@ write -P0x67 0x3fe0000 0x20000
"granularity": 65536,
"persistent": false,
"recording": false
- }
- ],
- "drive0": [
+ },
{
"busy": false,
"count": 0,
"granularity": 65536,
"persistent": false,
"recording": false
- },
+ }
+ ],
+ "drive0": [
{
"busy": false,
"count": 458752,
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3 3/5] block/copy-before-write: create block_copy bitmap in filter node
2024-02-28 14:14 ` [PATCH v3 3/5] block/copy-before-write: create block_copy bitmap in filter node Vladimir Sementsov-Ogievskiy
@ 2024-03-08 15:49 ` Fiona Ebner
0 siblings, 0 replies; 11+ messages in thread
From: Fiona Ebner @ 2024-03-08 15:49 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: qemu-devel, armbru, eblake, xiechanglong.d, wencongyang2, hreitz,
kwolf, jsnow, den, alexander.ivanov
Am 28.02.24 um 15:14 schrieb Vladimir Sementsov-Ogievskiy:
> Currently block_copy creates copy_bitmap in source node. But that is in
> bad relation with .independent_close=true of copy-before-write filter:
> source node may be detached and removed before .bdrv_close() handler
> called, which should call block_copy_state_free(), which in turn should
> remove copy_bitmap.
>
> That's all not ideal: it would be better if internal bitmap of
> block-copy object is not attached to any node. But that is not possible
> now.
>
> The simplest solution is just create copy_bitmap in filter node, where
> anyway two other bitmaps are created.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 4/5] qapi: blockdev-backup: add discard-source parameter
2024-02-28 14:14 [PATCH v3 0/5] backup: discard-source parameter Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2024-02-28 14:14 ` [PATCH v3 3/5] block/copy-before-write: create block_copy bitmap in filter node Vladimir Sementsov-Ogievskiy
@ 2024-02-28 14:15 ` Vladimir Sementsov-Ogievskiy
2024-03-08 15:49 ` Fiona Ebner
2024-02-28 14:15 ` [PATCH v3 5/5] iotests: add backup-discard-source Vladimir Sementsov-Ogievskiy
2024-03-08 15:49 ` [PATCH v3 0/5] backup: discard-source parameter Fiona Ebner
5 siblings, 1 reply; 11+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-02-28 14:15 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, eblake, xiechanglong.d, wencongyang2, hreitz,
kwolf, vsementsov, jsnow, f.ebner, den, alexander.ivanov
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. 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 <vsementsov@yandex-team.ru>
---
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, &bcs, errp);
+ cbw = bdrv_cbw_append(bs, target, filter_node_name, discard_source,
+ &bcs, 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(&s->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 44a9385aa7..dac57481c5 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 | BLK_PERM_WRITE;
+ }
+
*nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
}
}
@@ -467,7 +474,9 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
bs->file->bs->supported_zero_flags);
- s->bcs = block_copy_state_new(bs->file, s->target, bs, bitmap, errp);
+ s->discard_source = flags & BDRV_O_CBW_DISCARD_SOURCE;
+ s->bcs = block_copy_state_new(bs->file, s->target, bs, bitmap,
+ flags & BDRV_O_CBW_DISCARD_SOURCE, errp);
if (!s->bcs) {
error_prepend(errp, "Cannot create block-copy-state: ");
return -EINVAL;
@@ -534,12 +543,14 @@ static BlockDriver bdrv_cbw_filter = {
BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
BlockDriverState *target,
const char *filter_node_name,
+ bool discard_source,
BlockCopyState **bcs,
Error **errp)
{
BDRVCopyBeforeWriteState *state;
BlockDriverState *top;
QDict *opts;
+ int flags = BDRV_O_RDWR | (discard_source ? BDRV_O_CBW_DISCARD_SOURCE : 0);
assert(source->total_sectors == target->total_sectors);
GLOBAL_STATE_CODE();
@@ -552,7 +563,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));
- top = bdrv_insert_node(source, opts, BDRV_O_RDWR, errp);
+ top = bdrv_insert_node(source, opts, flags, errp);
if (!top) {
return NULL;
}
diff --git a/block/copy-before-write.h b/block/copy-before-write.h
index 6e72bb25e9..01af0cd3c4 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 discard_source,
BlockCopyState **bcs,
Error **errp);
void bdrv_cbw_drop(BlockDriverState *bs);
diff --git a/block/replication.c b/block/replication.c
index ca6bd0a720..0415a5e8b7 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -582,8 +582,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 f8bb0932f8..daceb50460 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2726,7 +2726,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-common.h b/include/block/block-common.h
index a846023a09..338fe5ff7a 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -243,6 +243,8 @@ typedef enum {
read-write fails */
#define BDRV_O_IO_URING 0x40000 /* use io_uring instead of the thread pool */
+#define BDRV_O_CBW_DISCARD_SOURCE 0x80000 /* for copy-before-write filter */
+
#define BDRV_O_CACHE_MASK (BDRV_O_NOCACHE | BDRV_O_NO_FLUSH)
diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index 8b41643bfa..bdc703bacd 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -27,6 +27,7 @@ typedef struct BlockCopyCallState BlockCopyCallState;
BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
BlockDriverState *copy_bitmap_bs,
const BdrvDirtyBitmap *bitmap,
+ bool discard_source,
Error **errp);
/* Function should be called prior any actual copy request */
diff --git a/include/block/block_int-global-state.h b/include/block/block_int-global-state.h
index d2201e27f4..eb2d92a226 100644
--- a/include/block/block_int-global-state.h
+++ b/include/block/block_int-global-state.h
@@ -193,7 +193,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/qapi/block-core.json b/qapi/block-core.json
index 22b8634422..9cbe3988b4 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1611,6 +1611,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 9.0)
+#
# @x-perf: Performance options. (Since 6.0)
#
# Features:
@@ -1632,6 +1635,7 @@
'*on-target-error': 'BlockdevOnError',
'*auto-finalize': 'bool', '*auto-dismiss': 'bool',
'*filter-node-name': 'str',
+ '*discard-source': 'bool',
'*x-perf': { 'type': 'BackupPerf',
'features': [ 'unstable' ] } } }
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3 4/5] qapi: blockdev-backup: add discard-source parameter
2024-02-28 14:15 ` [PATCH v3 4/5] qapi: blockdev-backup: add discard-source parameter Vladimir Sementsov-Ogievskiy
@ 2024-03-08 15:49 ` Fiona Ebner
0 siblings, 0 replies; 11+ messages in thread
From: Fiona Ebner @ 2024-03-08 15:49 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: qemu-devel, armbru, eblake, xiechanglong.d, wencongyang2, hreitz,
kwolf, jsnow, den, alexander.ivanov
Am 28.02.24 um 15:15 schrieb 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. 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 <vsementsov@yandex-team.ru>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 5/5] iotests: add backup-discard-source
2024-02-28 14:14 [PATCH v3 0/5] backup: discard-source parameter Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2024-02-28 14:15 ` [PATCH v3 4/5] qapi: blockdev-backup: add discard-source parameter Vladimir Sementsov-Ogievskiy
@ 2024-02-28 14:15 ` Vladimir Sementsov-Ogievskiy
2024-03-08 15:49 ` Fiona Ebner
2024-03-08 15:49 ` [PATCH v3 0/5] backup: discard-source parameter Fiona Ebner
5 siblings, 1 reply; 11+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-02-28 14:15 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, eblake, xiechanglong.d, wencongyang2, hreitz,
kwolf, vsementsov, jsnow, f.ebner, den, alexander.ivanov
Add test for a new backup option: discard-source.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
.../qemu-iotests/tests/backup-discard-source | 151 ++++++++++++++++++
.../tests/backup-discard-source.out | 5 +
2 files changed, 156 insertions(+)
create mode 100755 tests/qemu-iotests/tests/backup-discard-source
create mode 100644 tests/qemu-iotests/tests/backup-discard-source.out
diff --git a/tests/qemu-iotests/tests/backup-discard-source b/tests/qemu-iotests/tests/backup-discard-source
new file mode 100755
index 0000000000..8a88b0f6c4
--- /dev/null
+++ b/tests/qemu-iotests/tests/backup-discard-source
@@ -0,0 +1,151 @@
+#!/usr/bin/env python3
+#
+# Test removing persistent bitmap from backing
+#
+# Copyright (c) 2022 Virtuozzo International GmbH.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+
+import iotests
+from iotests import qemu_img_create, qemu_img_map, qemu_io
+
+
+temp_img = os.path.join(iotests.test_dir, 'temp')
+source_img = os.path.join(iotests.test_dir, 'source')
+target_img = os.path.join(iotests.test_dir, 'target')
+size = '1M'
+
+
+def get_actual_size(vm, node_name):
+ nodes = vm.cmd('query-named-block-nodes', flat=True)
+ node = next(n for n in nodes if n['node-name'] == node_name)
+ return node['image']['actual-size']
+
+
+class TestBackup(iotests.QMPTestCase):
+ def setUp(self):
+ qemu_img_create('-f', iotests.imgfmt, source_img, size)
+ qemu_img_create('-f', iotests.imgfmt, temp_img, size)
+ qemu_img_create('-f', iotests.imgfmt, target_img, size)
+ qemu_io('-c', 'write 0 1M', source_img)
+
+ self.vm = iotests.VM()
+ self.vm.launch()
+
+ self.vm.cmd('blockdev-add', {
+ 'node-name': 'cbw',
+ 'driver': 'copy-before-write',
+ 'file': {
+ 'driver': iotests.imgfmt,
+ 'file': {
+ 'driver': 'file',
+ 'filename': source_img,
+ }
+ },
+ 'target': {
+ 'driver': iotests.imgfmt,
+ 'discard': 'unmap',
+ 'node-name': 'temp',
+ 'file': {
+ 'driver': 'file',
+ 'filename': temp_img
+ }
+ }
+ })
+
+ self.vm.cmd('blockdev-add', {
+ 'node-name': 'access',
+ 'discard': 'unmap',
+ 'driver': 'snapshot-access',
+ 'file': 'cbw'
+ })
+
+ self.vm.cmd('blockdev-add', {
+ 'driver': iotests.imgfmt,
+ 'node-name': 'target',
+ 'file': {
+ 'driver': 'file',
+ 'filename': target_img
+ }
+ })
+
+ self.assertLess(get_actual_size(self.vm, 'temp'), 512 * 1024)
+
+ def tearDown(self):
+ # That should fail, because region is discarded
+ self.vm.hmp_qemu_io('access', 'read 0 1M')
+
+ self.vm.shutdown()
+
+ self.assertTrue('read failed: Permission denied' in self.vm.get_log())
+
+ # Final check that temp image is empty
+ mapping = qemu_img_map(temp_img)
+ self.assertEqual(len(mapping), 1)
+ self.assertEqual(mapping[0]['start'], 0)
+ self.assertEqual(mapping[0]['length'], 1024 * 1024)
+ self.assertEqual(mapping[0]['data'], False)
+
+ os.remove(temp_img)
+ os.remove(source_img)
+ os.remove(target_img)
+
+ def do_backup(self):
+ self.vm.cmd('blockdev-backup', device='access',
+ sync='full', target='target',
+ job_id='backup0',
+ discard_source=True)
+
+ self.vm.event_wait(name='BLOCK_JOB_COMPLETED')
+
+ def test_discard_written(self):
+ """
+ 1. Guest writes
+ 2. copy-before-write operation, data is stored to temp
+ 3. start backup(discard_source=True), check that data is
+ removed from temp
+ """
+ # Trigger copy-before-write operation
+ result = self.vm.hmp_qemu_io('cbw', 'write 0 1M')
+ self.assert_qmp(result, 'return', '')
+
+ # Check that data is written to temporary image
+ self.assertGreater(get_actual_size(self.vm, 'temp'), 1024 * 1024)
+
+ self.do_backup()
+
+ def test_discard_cbw(self):
+ """
+ 1. do backup(discard_source=True), which should inform
+ copy-before-write that data is not needed anymore
+ 2. Guest writes
+ 3. Check that copy-before-write operation is not done
+ """
+ self.do_backup()
+
+ # Try trigger copy-before-write operation
+ result = self.vm.hmp_qemu_io('cbw', 'write 0 1M')
+ self.assert_qmp(result, 'return', '')
+
+ # Check that data is not written to temporary image, as region
+ # is discarded from copy-before-write process
+ self.assertLess(get_actual_size(self.vm, 'temp'), 512 * 1024)
+
+
+if __name__ == '__main__':
+ iotests.main(supported_fmts=['qcow2'],
+ supported_protocols=['file'])
diff --git a/tests/qemu-iotests/tests/backup-discard-source.out b/tests/qemu-iotests/tests/backup-discard-source.out
new file mode 100644
index 0000000000..fbc63e62f8
--- /dev/null
+++ b/tests/qemu-iotests/tests/backup-discard-source.out
@@ -0,0 +1,5 @@
+..
+----------------------------------------------------------------------
+Ran 2 tests
+
+OK
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3 5/5] iotests: add backup-discard-source
2024-02-28 14:15 ` [PATCH v3 5/5] iotests: add backup-discard-source Vladimir Sementsov-Ogievskiy
@ 2024-03-08 15:49 ` Fiona Ebner
0 siblings, 0 replies; 11+ messages in thread
From: Fiona Ebner @ 2024-03-08 15:49 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: qemu-devel, armbru, eblake, xiechanglong.d, wencongyang2, hreitz,
kwolf, jsnow, den, alexander.ivanov
Am 28.02.24 um 15:15 schrieb Vladimir Sementsov-Ogievskiy:
> Add test for a new backup option: discard-source.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> .../qemu-iotests/tests/backup-discard-source | 151 ++++++++++++++++++
> .../tests/backup-discard-source.out | 5 +
> 2 files changed, 156 insertions(+)
> create mode 100755 tests/qemu-iotests/tests/backup-discard-source
> create mode 100644 tests/qemu-iotests/tests/backup-discard-source.out
>
> diff --git a/tests/qemu-iotests/tests/backup-discard-source b/tests/qemu-iotests/tests/backup-discard-source
> new file mode 100755
> index 0000000000..8a88b0f6c4
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/backup-discard-source
> @@ -0,0 +1,151 @@
> +#!/usr/bin/env python3
> +#
> +# Test removing persistent bitmap from backing
> +#
> +# Copyright (c) 2022 Virtuozzo International GmbH.
> +#
Title and copyright year are wrong.
Apart from that:
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 0/5] backup: discard-source parameter
2024-02-28 14:14 [PATCH v3 0/5] backup: discard-source parameter Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2024-02-28 14:15 ` [PATCH v3 5/5] iotests: add backup-discard-source Vladimir Sementsov-Ogievskiy
@ 2024-03-08 15:49 ` Fiona Ebner
5 siblings, 0 replies; 11+ messages in thread
From: Fiona Ebner @ 2024-03-08 15:49 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: qemu-devel, armbru, eblake, xiechanglong.d, wencongyang2, hreitz,
kwolf, jsnow, den, alexander.ivanov
Am 28.02.24 um 15:14 schrieb Vladimir Sementsov-Ogievskiy:
> Hi all! The main patch is 04, please look at it for description and
> diagram.
>
> v3:
> 02: new patch
> 04: take WRITE permission only when discard_source is required
>
> Vladimir Sementsov-Ogievskiy (5):
> block/copy-before-write: fix permission
> block/copy-before-write: support unligned snapshot-discard
> block/copy-before-write: create block_copy bitmap in filter node
> qapi: blockdev-backup: add discard-source parameter
> iotests: add backup-discard-source
>
> block/backup.c | 5 +-
> block/block-copy.c | 12 +-
> block/copy-before-write.c | 39 ++++-
> block/copy-before-write.h | 1 +
> block/replication.c | 4 +-
> blockdev.c | 2 +-
> include/block/block-common.h | 2 +
> include/block/block-copy.h | 2 +
> include/block/block_int-global-state.h | 2 +-
> qapi/block-core.json | 4 +
> tests/qemu-iotests/257.out | 112 ++++++-------
> .../qemu-iotests/tests/backup-discard-source | 151 ++++++++++++++++++
> .../tests/backup-discard-source.out | 5 +
> 13 files changed, 271 insertions(+), 70 deletions(-)
> create mode 100755 tests/qemu-iotests/tests/backup-discard-source
> create mode 100644 tests/qemu-iotests/tests/backup-discard-source.out
>
Tested-by: Fiona Ebner <f.ebner@proxmox.com>
^ permalink raw reply [flat|nested] 11+ messages in thread