fleecing-hook driver for backup
v3 cover fleecing-hook driver for backup
v2 was "[RFC v2] new, node-graph-based fleecing and backup"
Hi all!
These series introduce fleecing-hook driver. It's a filter-node, which do copy-before-write operation. Mirror uses filter-node for handling guest writes, let's move to filter-node (from write-notifiers) for backup too (patch 18)
Proposed filter driver is complete and separate: it can be used standalone, as fleecing provider (instead of backup(sync=none)). (old-style fleecing based on backup(sync=none) is supported too), look at patch 16.
There a lot of other ideas and improvements which can be achieved basing on these series which were discussed on v2 thread, for the beginning I want to concentrate on the following:
done in these series:
1. use filter node instead of write notifiers in backup
2. filter-based fleecing scheme, without a job
near to be done (a test is needed and may be tiny adjustment)
3. backup scheme for push backup with fleecing, to no disturb the guest with long handling of its writes (if target is far from remote NBD): just start fleecing to local qcow2 temp node and start a backup job (or mirror, why not?) from temp node to remote target. These series provide a possibility to share dirty bitmap between fleecing-hook and backup job to create efficient scenarios.
These series are based on
[PATCH v4 0/8] dirty-bitmap: rewrite bdrv_dirty_iter_next_area
and
[PATCH 0/2] replication: drop extra sync
Based-on: <[email protected]>
Based-on: <[email protected]>
Vladimir Sementsov-Ogievskiy (18):
block/dirty-bitmap: allow set/reset bits in disabled bitmaps block/io: allow BDRV_REQ_SERIALISING for read block/backup: simplify backup_incremental_init_copy_bitmap block/backup: move from HBitmap to BdrvDirtyBitmap util/id: add block-bitmap subsystem
block/backup: give a name to copy-bitmap block/backup: allow use existent copy-bitmap
block: allow serialized reads to intersect
block: improve should_update_child
iotests: handle -f argument correctly for qemu_io_silent
iotests: allow resume_drive by node name
iotests: prepare 055 to graph changes during backup job
block: introduce new filter driver: fleecing-hook block/fleecing-hook: internal api
qapi: add x-drop-fleecing qmp command
iotests: test new fleecing-hook driver in context of 222 iotest block/backup: tiny refactor backup_job_create block/backup: use fleecing-hook instead of write notifiers
qapi/block-core.json | 57 +++++- include/block/block.h | 9 + include/block/block_int.h | 2 +- include/qemu/id.h | 1 + block.c | 32 +++- block/backup.c | 320 ++++++++++++++----------------- block/dirty-bitmap.c | 2 - block/fleecing-hook.c | 349 ++++++++++++++++++++++++++++++++++ block/io.c | 16 +- block/replication.c | 2 +- blockdev.c | 49 ++++- util/id.c | 1 + block/Makefile.objs | 2 + tests/qemu-iotests/055 | 23 ++- tests/qemu-iotests/222 | 59 ++++-- tests/qemu-iotests/222.out | 66 +++++++ tests/qemu-iotests/iotests.py | 16 +- 17 files changed, 778 insertions(+), 228 deletions(-) create mode 100644 block/fleecing-hook.c
Signature · 1 line
2.18.0
PATCH v3 1/18 block/dirty-bitmap: allow set/reset bits in disabled bitmaps
It is needed for use the bitmaps in backup. "disabled" means that bitmap is not auto set by writes. Let's allow changing bitmap for other uses.
--- block/dirty-bitmap.c | 2 -- 1 file changed, 2 deletions(-)
block/dirty-bitmap.c +0 -2
@@ -532,7 +532,6 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)void bdrv_set_dirty_bitmap_locked (BdrvDirtyBitmap *bitmap,int64_t offset,int64_t bytes) { -assert (bdrv_dirty_bitmap_enabled (bitmap));assert (!bdrv_dirty_bitmap_readonly (bitmap));hbitmap_set (bitmap->bitmap, offset, bytes); }
@@ -549,7 +548,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,void bdrv_reset_dirty_bitmap_locked (BdrvDirtyBitmap *bitmap,int64_t offset,int64_t bytes) { -assert (bdrv_dirty_bitmap_enabled (bitmap));assert (!bdrv_dirty_bitmap_readonly (bitmap));hbitmap_reset (bitmap->bitmap, offset, bytes); }
Signature · 1 line
2.18.0
Re: [Qemu-devel] [PATCH v3 01/18] block/dirty-bitmap: allow set/reset bits in disabled bitmaps
Quoted 8 lines · Vladimir Sementsov-Ogievskiy
It is needed for use the bitmaps in backup. "disabled" means that bitmap is not auto set by writes. Let's allow changing bitmap for other uses.
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- block/dirty-bitmap.c | 2 -- 1 file changed, 2 deletions(-)
Looks like this duplicates John's series:
https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00406.html
Quoted 22 lines
block/dirty-bitmap.c +0
-2@@ -532,7 +532,6 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)void bdrv_set_dirty_bitmap_locked (BdrvDirtyBitmap *bitmap,int64_t offset,int64_t bytes) { -assert (bdrv_dirty_bitmap_enabled (bitmap));assert (!bdrv_dirty_bitmap_readonly (bitmap));hbitmap_set (bitmap->bitmap, offset, bytes); }@@ -549,7 +548,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,void bdrv_reset_dirty_bitmap_locked (BdrvDirtyBitmap *bitmap,int64_t offset,int64_t bytes) { -assert (bdrv_dirty_bitmap_enabled (bitmap));assert (!bdrv_dirty_bitmap_readonly (bitmap));hbitmap_reset (bitmap->bitmap, offset, bytes); }
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v3 01/18] block/dirty-bitmap: allow set/reset bits in disabled bitmaps
Quoted 5 lines · Eric Blake
Quoted 4 lines · Vladimir Sementsov-Ogievskiy
It is needed for use the bitmaps in backup. "disabled" means that bitmap is not auto set by writes. Let's allow changing bitmap for other uses.
Quoted 7 lines
Quoted 4 lines
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>---
block/dirty-bitmap.c| 2 --1 file changed, 2 deletions(-)Looks like this duplicates John's series:
https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00406.html
No. But it relates.
Quoted 25 lines
Quoted 24 lines
block/dirty-bitmap.c +0
-1@@ -532,7 +532,6 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter) void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap, int64_t offset, int64_t bytes) { - assert(bdrv_dirty_bitmap_enabled(bitmap)); assert(!bdrv_dirty_bitmap_readonly(bitmap)); hbitmap_set(bitmap->bitmap, offset, bytes);}
block/dirty-bitmap.c +0
-1@@ -549,7 +548,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap, void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap, int64_t offset, int64_t bytes) { - assert(bdrv_dirty_bitmap_enabled(bitmap)); assert(!bdrv_dirty_bitmap_readonly(bitmap)); hbitmap_reset(bitmap->bitmap, offset, bytes); }
Signature · 2 lines
Best regards, Vladimir
PATCH v3 2/18 block/io: allow BDRV_REQ_SERIALISING for read
In reply to Vladimir Sementsov-OgievskiyIt will be used in further commit, changing backup architecture.
--- block/io.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
block/io.c +6 -7
@@ -1318,10 +1318,11 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,* potential fallback support, if we ever implement any read flags * to pass through to drivers. For now, there aren't any * passthrough flags. */ -assert (!(flags & ~(BDRV_REQ_NO_SERIALISING |BDRV_REQ_COPY_ON_READ ))); +assert (!(flags & ~(BDRV_REQ_NO_SERIALISING |BDRV_REQ_COPY_ON_READ | +BDRV_REQ_SERIALISING )));/* Handle Copy on Read and associated serialisation */ -if (flags &BDRV_REQ_COPY_ON_READ ) { +if (flags & (BDRV_REQ_COPY_ON_READ |BDRV_REQ_SERIALISING )) {/* If we touch the same cluster it counts as an overlap. This * guarantees that allocating writes will be serialized and not race * with each other for the same cluster. For example, in copy-on-read
@@ -1330,9 +1331,6 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,mark_request_serialising (req,bdrv_get_cluster_size (bs)); } -/* BDRV_REQ_SERIALISING is only for write operation */ -assert (!(flags &BDRV_REQ_SERIALISING )); -if (!(flags &BDRV_REQ_NO_SERIALISING )) {wait_serialising_requests (req); }
@@ -3027,8 +3025,9 @@ static int coroutine_fn bdrv_co_copy_range_internal(tracked_request_begin (&req, src->bs, src_offset, bytes,BDRV_TRACKED_READ ); -/* BDRV_REQ_SERIALISING is only for write operation */ -assert (!(read_flags &BDRV_REQ_SERIALISING )); +if (read_flags &BDRV_REQ_SERIALISING ) { +mark_request_serialising (&req,bdrv_get_cluster_size (src->bs)); + }if (!(read_flags &BDRV_REQ_NO_SERIALISING )) {wait_serialising_requests (&req); }
Signature · 1 line
2.18.0
PATCH v3 3/18 block/backup: simplify backup_incremental_init_copy_bitmap
In reply to Vladimir Sementsov-OgievskiySimplify backup_incremental_init_copy_bitmap using the function bdrv_dirty_bitmap_next_dirty_area.
Note: move to job->len instead of bitmap size: it should not matter but less code.
--- block/backup.c | 40 ++++++++++++---------------------------- 1 file changed, 12 insertions(+), 28 deletions(-)
block/backup.c +12 -28
@@ -406,43 +406,27 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)/* init copy_bitmap from sync_bitmap */ static void backup_incremental_init_copy_bitmap (BackupBlockJob *job) { -BdrvDirtyBitmapIter *dbi; -int64_t offset; -int64_t end =DIV_ROUND_UP (bdrv_dirty_bitmap_size (job->sync_bitmap), - job->cluster_size); - - dbi =bdrv_dirty_iter_new (job->sync_bitmap); -while ((offset =bdrv_dirty_iter_next (dbi)) !=-1 ) { -int64_t cluster = offset / job->cluster_size; -int64_t next_cluster; - - offset +=bdrv_dirty_bitmap_granularity (job->sync_bitmap); -if (offset >=bdrv_dirty_bitmap_size (job->sync_bitmap)) { -hbitmap_set (job->copy_bitmap, cluster, end - cluster); -break ; - } +uint64_t offset =0 ; +uint64_t bytes = job->len; - offset =bdrv_dirty_bitmap_next_zero (job->sync_bitmap, offset, -UINT64_MAX ); -if (offset ==-1 ) { -hbitmap_set (job->copy_bitmap, cluster, end - cluster); -break ; - } +while (bdrv_dirty_bitmap_next_dirty_area (job->sync_bitmap, + &offset, &bytes)) + { +uint64_t cluster = offset / job->cluster_size; +uint64_t last_cluster = (offset + bytes) / job->cluster_size; - next_cluster =DIV_ROUND_UP (offset, job->cluster_size); -hbitmap_set (job->copy_bitmap, cluster, next_cluster - cluster); -if (next_cluster >= end) { +hbitmap_set (job->copy_bitmap, cluster, last_cluster - cluster +1 ); + + offset = (last_cluster +1 ) * job->cluster_size; +if (offset >= job->len) {break ; } - -bdrv_set_dirty_iter (dbi, next_cluster * job->cluster_size); + bytes = job->len - offset; }/* TODO job_progress_set_remaining() would make more sense */ job_progress_update (&job->common.job, job->len -hbitmap_count (job->copy_bitmap) * job->cluster_size); - -bdrv_dirty_iter_free (dbi); } static int coroutine_fn backup_run(Job *job, Error **errp)
Signature · 1 line
2.18.0
PATCH v3 4/18 block/backup: move from HBitmap to BdrvDirtyBitmap
In reply to Vladimir Sementsov-OgievskiyThis is a necessary step to share copy-bitmap between backup job and special filter driver in further commit.
--- block/backup.c | 73 ++++++++++++++++++++++++++++---------------------- 1 file changed, 41 insertions(+), 32 deletions(-)
block/backup.c +41 -32
@@ -51,7 +51,7 @@ typedef struct BackupBlockJob {NotifierWithReturn before_write; QLIST_HEAD(, CowRequest) inflight_reqs; -HBitmap *copy_bitmap; +BdrvDirtyBitmap *copy_bitmap;bool use_copy_range;int64_t copy_range_size;
@@ -114,7 +114,8 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,int read_flags = is_write_notifier ?BDRV_REQ_NO_SERIALISING :0 ;int write_flags = job->serialize_target_writes ?BDRV_REQ_SERIALISING :0 ; -hbitmap_reset (job->copy_bitmap, start / job->cluster_size,1 ); +assert (start % job->cluster_size ==0 ); +bdrv_reset_dirty_bitmap (job->copy_bitmap, start, job->cluster_size); nbytes =MIN (job->cluster_size, job->len - start);if (!*bounce_buffer) { *bounce_buffer =blk_blockalign (blk, job->cluster_size);
@@ -150,7 +151,7 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,return nbytes; fail: -hbitmap_set (job->copy_bitmap, start / job->cluster_size,1 ); +bdrv_set_dirty_bitmap (job->copy_bitmap, start, job->cluster_size);return ret; }
@@ -170,16 +171,17 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,int write_flags = job->serialize_target_writes ?BDRV_REQ_SERIALISING :0 ;assert (QEMU_IS_ALIGNED (job->copy_range_size, job->cluster_size)); +assert (QEMU_IS_ALIGNED (start, job->cluster_size)); nbytes =MIN (job->copy_range_size, end - start); nr_clusters =DIV_ROUND_UP (nbytes, job->cluster_size); -hbitmap_reset (job->copy_bitmap, start / job->cluster_size, - nr_clusters); +bdrv_reset_dirty_bitmap (job->copy_bitmap, start, + job->cluster_size * nr_clusters); ret =blk_co_copy_range (blk, start, job->target, start, nbytes, read_flags, write_flags);if (ret <0 ) {trace_backup_do_cow_copy_range_fail (job, start, ret); -hbitmap_set (job->copy_bitmap, start / job->cluster_size, - nr_clusters); +bdrv_set_dirty_bitmap (job->copy_bitmap, start, + job->cluster_size * nr_clusters);return ret; }
@@ -207,7 +209,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,cow_request_begin (&cow_request, job, start, end);while (start < end) { -if (!hbitmap_get (job->copy_bitmap, start / job->cluster_size)) { +if (!bdrv_get_dirty_locked (NULL , job->copy_bitmap, start)) {trace_backup_do_cow_skip (job, start); start += job->cluster_size;continue ;/* already copied */
@@ -303,6 +305,11 @@ static void backup_clean(Job *job)assert (s->target);blk_unref (s->target); s->target =NULL ; + +if (s->copy_bitmap) { +bdrv_release_dirty_bitmap (blk_bs (s->common.blk), s->copy_bitmap); + s->copy_bitmap =NULL ; + } }static void backup_attached_aio_context (BlockJob *job,AioContext *aio_context)
@@ -315,7 +322,6 @@ static void backup_attached_aio_context(BlockJob *job, AioContext *aio_context)void backup_do_checkpoint (BlockJob *job,Error **errp) {BackupBlockJob *backup_job =container_of (job, BackupBlockJob, common); -int64_t len;assert (block_job_driver (job) == &backup_job_driver);
@@ -325,8 +331,7 @@ void backup_do_checkpoint(BlockJob *job, Error **errp)return ; } - len =DIV_ROUND_UP (backup_job->len, backup_job->cluster_size); -hbitmap_set (backup_job->copy_bitmap,0 , len); +bdrv_set_dirty_bitmap (backup_job->copy_bitmap,0 , backup_job->len); }static void backup_drain (BlockJob *job)
@@ -379,28 +384,30 @@ static bool coroutine_fn yield_and_check(BackupBlockJob *job) static int coroutine_fn backup_run_incremental(BackupBlockJob *job) { -int ret; +int ret =0 ;bool error_is_read; -int64_t cluster; -HBitmapIter hbi; +int64_t offset; +BdrvDirtyBitmapIter *dbi =bdrv_dirty_iter_new (job->copy_bitmap); -hbitmap_iter_init (&hbi, job->copy_bitmap,0 ); -while ((cluster =hbitmap_iter_next (&hbi)) !=-1 ) { +while ((offset =bdrv_dirty_iter_next (dbi)) !=-1 ) {do {if (yield_and_check (job)) { -return 0 ; + goto out; } - ret =backup_do_cow (job, cluster * job->cluster_size, + ret =backup_do_cow (job, offset, job->cluster_size, &error_is_read, false);if (ret <0 &&backup_error_action (job, error_is_read, -ret) ==BLOCK_ERROR_ACTION_REPORT ) { -return ret; + goto out; } }while (ret <0 ); } -return 0 ; +out: +bdrv_dirty_iter_free (dbi); + +return ret; }/* init copy_bitmap from sync_bitmap */
@@ -412,12 +419,9 @@ static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)while (bdrv_dirty_bitmap_next_dirty_area (job->sync_bitmap, &offset, &bytes)) { -uint64_t cluster = offset / job->cluster_size; -uint64_t last_cluster = (offset + bytes) / job->cluster_size; +bdrv_set_dirty_bitmap (job->copy_bitmap, offset, bytes); -hbitmap_set (job->copy_bitmap, cluster, last_cluster - cluster +1 ); - - offset = (last_cluster +1 ) * job->cluster_size; + offset += bytes;if (offset >= job->len) {break ; }
@@ -426,30 +430,29 @@ static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)/* TODO job_progress_set_remaining() would make more sense */ job_progress_update (&job->common.job, - job->len -hbitmap_count (job->copy_bitmap) * job->cluster_size); + job->len -bdrv_get_dirty_count (job->copy_bitmap)); } static int coroutine_fn backup_run(Job *job, Error **errp) {BackupBlockJob *s =container_of (job, BackupBlockJob, common.job);BlockDriverState *bs =blk_bs (s->common.blk); -int64_t offset, nb_clusters; +int64_t offset;int ret =0 ;QLIST_INIT (&s->inflight_reqs);qemu_co_rwlock_init (&s->flush_rwlock); - nb_clusters =DIV_ROUND_UP (s->len, s->cluster_size);job_progress_set_remaining (job, s->len); - s->copy_bitmap =hbitmap_alloc (nb_clusters,0 ); +bdrv_disable_dirty_bitmap (s->copy_bitmap); +if (s->sync_mode ==MIRROR_SYNC_MODE_INCREMENTAL ) {backup_incremental_init_copy_bitmap (s); }else { -hbitmap_set (s->copy_bitmap,0 , nb_clusters); +bdrv_set_dirty_bitmap (s->copy_bitmap,0 , s->len); } - s->before_write.notify = backup_before_write_notify;bdrv_add_before_write_notifier (bs, &s->before_write);
@@ -530,7 +533,6 @@ static int coroutine_fn backup_run(Job *job, Error **errp)/* wait until pending backup_do_cow() calls have completed */ qemu_co_rwlock_wrlock (&s->flush_rwlock);qemu_co_rwlock_unlock (&s->flush_rwlock); -hbitmap_free (s->copy_bitmap);return ret; }
@@ -681,6 +683,13 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, }else { job->cluster_size =MAX (BACKUP_CLUSTER_SIZE_DEFAULT , bdi.cluster_size); } + + job->copy_bitmap =bdrv_create_dirty_bitmap (bs, job->cluster_size, +NULL , errp); +if (!job->copy_bitmap) { + goto error; + } + job->use_copy_range = true; job->copy_range_size =MIN_NON_ZERO (blk_get_max_transfer (job->common.blk),blk_get_max_transfer (job->target));
Signature · 1 line
2.18.0
PATCH v3 5/18 util/id: add block-bitmap subsystem
In reply to Vladimir Sementsov-OgievskiyAdd block-bitmap subsystem to generate bitmap names in the following commit.
--- include/qemu/id.h | 1 + util/id.c | 1 + 2 files changed, 2 insertions(+)
include/qemu/id.h +1 -0
@@ -4,6 +4,7 @@typedef enum IdSubSystems {ID_QDEV ,ID_BLOCK , +ID_BLOCK_BITMAP ,ID_MAX /* last element, used as array size */ }IdSubSystems ;
util/id.c +1 -0
@@ -34,6 +34,7 @@ bool id_wellformed(const char *id)static const char *const id_subsys_str[ID_MAX ] = { [ID_QDEV ] ="qdev" , [ID_BLOCK ] ="block" , + [ID_BLOCK_BITMAP ] ="block-bitmap" , };/*
Signature · 1 line
2.18.0
PATCH v3 6/18 block/backup: give a name to copy-bitmap
In reply to Vladimir Sementsov-OgievskiyThe bitmap should be named to be shared with special filter node in further patches. Make it possible to set this name in qmp command.
--- qapi/block-core.json | 18 ++++++++++++++++-- include/block/block_int.h | 2 +- block/backup.c | 10 ++++++++-- block/replication.c | 2 +- blockdev.c | 12 ++++++++++-- 5 files changed, 36 insertions(+), 8 deletions(-)
qapi/block-core.json +16 -2
@@ -1260,6 +1260,12 @@ # Must be present if sync is "incremental", must NOT be present # otherwise. (Since 2.4) # +# @x-copy-bitmap: the name for the copy-bitmap - the bitmap which drives the +# backup process. At backup start it corresponds to @bitmap (or +# just all bits set for full backup) and at the finish it +# should be zero. If the bitmap already exists it will be used +# as is. (Since 3.1) +# # @compress: true to compress data, if the target format supports it. # (default: false) (since 2.8) #
@@ -1297,7 +1303,8 @@ '*bitmap': 'str', '*compress': 'bool', '*on-source-error': 'BlockdevOnError', '*on-target-error': 'BlockdevOnError', - '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } } + '*auto-finalize': 'bool', '*auto-dismiss': 'bool', + '*x-copy-bitmap': 'str' } } ## # @BlockdevBackup:
@@ -1340,6 +1347,12 @@ # list without user intervention. # Defaults to true. (Since 2.12) # +# @x-copy-bitmap: the name for the copy-bitmap - the bitmap which drives the +# backup process. At backup start it corresponds to @bitmap (or +# just all bits set for full backup) and at the finish it +# should be zero. If the bitmap already exists it will be used +# as is. (Since 3.1) +# # Note: @on-source-error and @on-target-error only affect background # I/O. If an error occurs during a guest write request, the device's # rerror/werror actions will be used.
@@ -1351,7 +1364,8 @@ 'sync': 'MirrorSyncMode', '*speed': 'int', '*compress': 'bool', '*on-source-error': 'BlockdevOnError', '*on-target-error': 'BlockdevOnError', - '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } } + '*auto-finalize': 'bool', '*auto-dismiss': 'bool', + '*x-copy-bitmap': 'str' } } ## # @blockdev-snapshot-sync:
include/block/block_int.h +1 -1
@@ -1087,7 +1087,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,bool compress, BlockdevOnError on_source_error, BlockdevOnError on_target_error, - int creation_flags, + int creation_flags, const char *x_copy_bitmap, BlockCompletionFunc *cb, void *opaque, JobTxn *txn, Error **errp);
block/backup.c +8 -2
@@ -25,6 +25,7 @@#include "sysemu/block-backend.h" #include "qemu/bitmap.h" #include "qemu/error-report.h" +#include "qemu/id.h" #define BACKUP_CLUSTER_SIZE_DEFAULT (1 << 16)
@@ -559,7 +560,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,bool compress, BlockdevOnError on_source_error, BlockdevOnError on_target_error, - int creation_flags, + int creation_flags, const char *x_copy_bitmap, BlockCompletionFunc *cb, void *opaque, JobTxn *txn, Error **errp) {
@@ -567,6 +568,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,BlockDriverInfo bdi;BackupBlockJob *job =NULL ;int ret; +char *gen_bitmap_name =NULL ;assert (bs);assert (target);
@@ -684,8 +686,12 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, job->cluster_size =MAX (BACKUP_CLUSTER_SIZE_DEFAULT , bdi.cluster_size); } +if (!x_copy_bitmap) { + x_copy_bitmap = gen_bitmap_name =id_generate (ID_BLOCK_BITMAP ); + } job->copy_bitmap =bdrv_create_dirty_bitmap (bs, job->cluster_size, -NULL , errp); + x_copy_bitmap, errp); +g_free (gen_bitmap_name);if (!job->copy_bitmap) { goto error; }
block/replication.c +1 -1
@@ -548,7 +548,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode, job =backup_job_create (NULL , s->secondary_disk->bs, s->hidden_disk->bs,0 ,MIRROR_SYNC_MODE_NONE ,NULL , false,BLOCKDEV_ON_ERROR_REPORT , -BLOCKDEV_ON_ERROR_REPORT ,JOB_INTERNAL , +BLOCKDEV_ON_ERROR_REPORT ,JOB_INTERNAL ,NULL , backup_job_completed, bs,NULL , &local_err);if (local_err) {error_propagate (errp, local_err);
blockdev.c +10 -2
@@ -3407,6 +3407,9 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,if (!backup->has_compress) { backup->compress = false; } +if (!backup->has_x_copy_bitmap) { + backup->x_copy_bitmap =NULL ; + } bs =qmp_get_root_bs (backup->device, errp);if (!bs) {
@@ -3511,7 +3514,8 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, job =backup_job_create (backup->job_id, bs, target_bs, backup->speed, backup->sync, bmap, backup->compress, backup->on_source_error, backup->on_target_error, - job_flags,NULL ,NULL , txn, &local_err); + job_flags, backup->x_copy_bitmap, +NULL ,NULL , txn, &local_err);bdrv_unref (target_bs);if (local_err !=NULL ) {error_propagate (errp, local_err);
@@ -3569,6 +3573,9 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,if (!backup->has_compress) { backup->compress = false; } +if (!backup->has_x_copy_bitmap) { + backup->x_copy_bitmap =NULL ; + } bs =bdrv_lookup_bs (backup->device, backup->device, errp);if (!bs) {
@@ -3603,7 +3610,8 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn, job =backup_job_create (backup->job_id, bs, target_bs, backup->speed, backup->sync,NULL , backup->compress, backup->on_source_error, backup->on_target_error, - job_flags,NULL ,NULL , txn, &local_err); + job_flags, backup->x_copy_bitmap, +NULL ,NULL , txn, &local_err);if (local_err !=NULL ) {error_propagate (errp, local_err); }
Signature · 1 line
2.18.0
PATCH v3 7/18 block/backup: allow use existent copy-bitmap
In reply to Vladimir Sementsov-OgievskiyAllow use existent copy-bitmap to make it possible to synchronize with externally created fleecing-hook filter which will be introduced soon.
--- block/backup.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
block/backup.c +10 -4
@@ -53,6 +53,7 @@ typedef struct BackupBlockJob { QLIST_HEAD(, CowRequest) inflight_reqs;BdrvDirtyBitmap *copy_bitmap; +bool copy_bitmap_created;bool use_copy_range;int64_t copy_range_size;
@@ -307,7 +308,7 @@ static void backup_clean(Job *job)blk_unref (s->target); s->target =NULL ; -if (s->copy_bitmap) { +if (s->copy_bitmap_created) {bdrv_release_dirty_bitmap (blk_bs (s->common.blk), s->copy_bitmap); s->copy_bitmap =NULL ; }
@@ -686,11 +687,16 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, job->cluster_size =MAX (BACKUP_CLUSTER_SIZE_DEFAULT , bdi.cluster_size); } -if (!x_copy_bitmap) { +if (x_copy_bitmap) { + job->copy_bitmap =bdrv_find_dirty_bitmap (bs, x_copy_bitmap); + }else { x_copy_bitmap = gen_bitmap_name =id_generate (ID_BLOCK_BITMAP ); } - job->copy_bitmap =bdrv_create_dirty_bitmap (bs, job->cluster_size, - x_copy_bitmap, errp); +if (!job->copy_bitmap) { + job->copy_bitmap =bdrv_create_dirty_bitmap (bs, job->cluster_size, + x_copy_bitmap, errp); + job->copy_bitmap_created = !!job->copy_bitmap; + }g_free (gen_bitmap_name);if (!job->copy_bitmap) { goto error;
Signature · 1 line
2.18.0
PATCH v3 8/18 block: allow serialized reads to intersect
In reply to Vladimir Sementsov-OgievskiyOtherwise, if we have serialized read-part in copy_range from backing file to its parent if CoW take place, this CoW's sub-reads will intersect with firstly created serialized read request.
Anyway, reads should not influence on disk view, let's allow them to intersect.
--- block/io.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
block/io.c +2 -1
@@ -735,7 +735,8 @@ static bool coroutine_fn wait_serialising_requests(BdrvTrackedRequest *self) retry = false;qemu_co_mutex_lock (&bs->reqs_lock); QLIST_FOREACH(req, &bs->tracked_requests, list) { -if (req == self || (!req->serialising && !self->serialising)) { +if (req == self || (!req->serialising && !self->serialising) || + (self->type ==BDRV_TRACKED_READ && req->type == self->type)) {continue ; } if (tracked_request_overlaps(req, self->overlap_offset,
Signature · 1 line
2.18.0
PATCH v3 9/18 block: improve should_update_child
In reply to Vladimir Sementsov-OgievskiyAs it already said in the comment, we don't want to create loops in parent->child relations. So, when we try to append @to to @c, we should check that @c is not in @to children subtree, and we should check it recursively, not only the first level. The patch provides BFS-based search, to check the relations.
This is needed for further fleecing-hook filter usage: we need to append it to source, when the hook is already a parent of target, and source may be in a backing chain of target (fleecing-scheme). So, on appending, the hook should not became a child (direct or through children subtree) of the target.
--- block.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-)
block.c +27 -5
@@ -3432,7 +3432,7 @@ void bdrv_close_all(void)static bool should_update_child (BdrvChild *c,BlockDriverState *to) { -BdrvChild *to_c; +GList *queue =NULL , *pos;if (c->role->stay_at_node) {return false;
@@ -3468,13 +3468,35 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)* if A is a child of B, that means we cannot replace A by B there * because that would create a loop. Silently detaching A from B * is also not really an option. So overall just leaving A in -* place there is the most sensible choice. */ - QLIST_FOREACH(to_c, &to->children, next) { -if (to_c == c) { -return false; +* place there is the most sensible choice. +* +* upd: If the child @c belongs to the @to's children, or children of it's +* children and so on - this would create a loop to. To prevent it let's do +* a BFS search on @to children subtree. +*/ + + pos = queue =g_list_append (queue, to); +while (pos) { +BlockDriverState *v = pos->data; +BdrvChild *c2; + + QLIST_FOREACH(c2, &v->children, next) { +if (c2 == c) { +g_list_free (queue); +return false; + } + +if (g_list_find (queue, c2->bs)) { +continue ; + } + + queue =g_list_append (queue, c2->bs); } + + pos = pos->next; } +g_list_free (queue);return true; }
Signature · 1 line
2.18.0
PATCH v3 10/18 iotests: handle -f argument correctly for qemu_io_silent
In reply to Vladimir Sementsov-OgievskiyCorrectly rewrite default argument. After the patch, the function can be used for other (not only default test-chosen) image format.
--- tests/qemu-iotests/iotests.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
tests/qemu-iotests/iotests.py +6 -1
@@ -136,7 +136,12 @@ def qemu_io(*args): def qemu_io_silent(*args): '''Run qemu-io and return the exit code, suppressing stdout''' - args = qemu_io_args + list(args) + if '-f' in qemu_io_args and '-f' in args: + ind = qemu_io_args.index('-f') + args = qemu_io_args[:ind] + qemu_io_args[ind+2:] + list(args) + else: + args = qemu_io_args + list(args) + exitcode = subprocess.call(args, stdout=open('/dev/null', 'w')) if exitcode < 0: sys.stderr.write('qemu-io received signal %i: %s\n' %
Signature · 1 line
2.18.0
PATCH v3 11/18 iotests: allow resume_drive by node name
In reply to Vladimir Sementsov-OgievskiyAfter node graph changes, we may not be able to resume_drive by device name (backing files are not recursively searched). So, lets allow to resume by node-name. Set constant name for breakpoints, to avoid introducing extra parameters.
--- tests/qemu-iotests/iotests.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
tests/qemu-iotests/iotests.py +5 -4
@@ -404,11 +404,11 @@ class VM(qtest.QEMUQtestMachine): self.pause_drive(drive, "write_aio") return self.qmp('human-monitor-command', - command_line='qemu-io %s "break %s bp_%s"' % (drive, event, drive)) + command_line='qemu-io %s "break %s bp_0"' % (drive, event)) def resume_drive(self, drive): self.qmp('human-monitor-command', - command_line='qemu-io %s "remove_break bp_%s"' % (drive, drive)) + command_line='qemu-io %s "remove_break bp_0"' % (drive)) def hmp_qemu_io(self, drive, cmd): '''Write to a given drive using an HMP command'''
@@ -531,13 +531,14 @@ class QMPTestCase(unittest.TestCase): self.assertEqual(self.vm.flatten_qmp_object(json.loads(json_filename[5:])), self.vm.flatten_qmp_object(reference)) - def cancel_and_wait(self, drive='drive0', force=False, resume=False): + def cancel_and_wait(self, drive='drive0', force=False, resume=False, + resume_node=None): '''Cancel a block job and wait for it to finish, returning the event''' result = self.vm.qmp('block-job-cancel', device=drive, force=force) self.assert_qmp(result, 'return', {}) if resume: - self.vm.resume_drive(drive) + self.vm.resume_drive(resume_node or drive) cancelled = False result = None
Signature · 1 line
2.18.0
PATCH v3 12/18 iotests: prepare 055 to graph changes during backup job
In reply to Vladimir Sementsov-OgievskiyBackup will append fleecing-hook node above source node, so, we can't resume by device name (because resume don't search recursively through backing chain).
--- tests/qemu-iotests/055 | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)
tests/qemu-iotests/055 +13 -10
@@ -48,7 +48,8 @@ class TestSingleDrive(iotests.QMPTestCase): def setUp(self): qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(image_len)) - self.vm = iotests.VM().add_drive('blkdebug::' + test_img) + self.vm = iotests.VM().add_drive('blkdebug::' + test_img, + 'node-name=source') self.vm.add_drive(blockdev_target_img, interface="none") if iotests.qemu_default_machine == 'pc': self.vm.add_drive(None, 'media=cdrom', 'ide')
@@ -69,7 +70,7 @@ class TestSingleDrive(iotests.QMPTestCase): result = self.vm.qmp(cmd, device='drive0', target=target, sync='full') self.assert_qmp(result, 'return', {}) - event = self.cancel_and_wait(resume=True) + event = self.cancel_and_wait(resume=True, resume_node='source') self.assert_qmp(event, 'data/type', 'backup') def test_cancel_drive_backup(self):
@@ -87,7 +88,7 @@ class TestSingleDrive(iotests.QMPTestCase): self.assert_qmp(result, 'return', {}) self.pause_job('drive0', wait=False) - self.vm.resume_drive('drive0') + self.vm.resume_drive('source') self.pause_wait('drive0') result = self.vm.qmp('query-block-jobs')
@@ -165,7 +166,8 @@ class TestSetSpeed(iotests.QMPTestCase): def setUp(self): qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(image_len)) - self.vm = iotests.VM().add_drive('blkdebug::' + test_img) + self.vm = iotests.VM().add_drive('blkdebug::' + test_img, + 'node-name=source') self.vm.add_drive(blockdev_target_img, interface="none") self.vm.launch()
@@ -197,7 +199,7 @@ class TestSetSpeed(iotests.QMPTestCase): self.assert_qmp(result, 'return[0]/device', 'drive0') self.assert_qmp(result, 'return[0]/speed', 8 * 1024 * 1024) - event = self.cancel_and_wait(resume=True) + event = self.cancel_and_wait(resume=True, resume_node='source') self.assert_qmp(event, 'data/type', 'backup') # Check setting speed option works
@@ -210,7 +212,7 @@ class TestSetSpeed(iotests.QMPTestCase): self.assert_qmp(result, 'return[0]/device', 'drive0') self.assert_qmp(result, 'return[0]/speed', 4 * 1024 * 1024) - event = self.cancel_and_wait(resume=True) + event = self.cancel_and_wait(resume=True, resume_node='source') self.assert_qmp(event, 'data/type', 'backup') def test_set_speed_drive_backup(self):
@@ -236,7 +238,7 @@ class TestSetSpeed(iotests.QMPTestCase): result = self.vm.qmp('block-job-set-speed', device='drive0', speed=-1) self.assert_qmp(result, 'error/class', 'GenericError') - event = self.cancel_and_wait(resume=True) + event = self.cancel_and_wait(resume=True, resume_node='source') self.assert_qmp(event, 'data/type', 'backup') def test_set_speed_invalid_drive_backup(self):
@@ -464,7 +466,8 @@ class TestDriveCompression(iotests.QMPTestCase): pass def do_prepare_drives(self, fmt, args, attach_target): - self.vm = iotests.VM().add_drive('blkdebug::' + test_img) + self.vm = iotests.VM().add_drive('blkdebug::' + test_img, + 'node-name=source') qemu_img('create', '-f', fmt, blockdev_target_img, str(TestDriveCompression.image_len), *args)
@@ -507,7 +510,7 @@ class TestDriveCompression(iotests.QMPTestCase): result = self.vm.qmp(cmd, device='drive0', sync='full', compress=True, **args) self.assert_qmp(result, 'return', {}) - event = self.cancel_and_wait(resume=True) + event = self.cancel_and_wait(resume=True, resume_node='source') self.assert_qmp(event, 'data/type', 'backup') self.vm.shutdown()
@@ -532,7 +535,7 @@ class TestDriveCompression(iotests.QMPTestCase): self.assert_qmp(result, 'return', {}) self.pause_job('drive0', wait=False) - self.vm.resume_drive('drive0') + self.vm.resume_drive('source') self.pause_wait('drive0') result = self.vm.qmp('query-block-jobs')
Signature · 1 line
2.18.0
PATCH v3 13/18 block: introduce new filter driver: fleecing-hook
In reply to Vladimir Sementsov-OgievskiyFleecing-hook filter does copy-before-write operation. It should be inserted above active disk and has a target node for CBW, like the following:
+-------+
| Guest |
+---+---+
|r,w
v
+---+-----------+ target +---------------+
| Fleecing hook |---------->| target(qcow2) |
+---+-----------+ CBW +---+-----------+
| |
backing |r,w |
v |
+---+---------+ backing |
| Active disk |<----------------+
+-------------+ rTarget's backing may point to active disk (should be set up separately), which gives fleecing-scheme.
--- qapi/block-core.json | 22 +++- block/fleecing-hook.c | 298 ++++++++++++++++++++++++++++++++++++++++++ block/Makefile.objs | 2 + 3 files changed, 320 insertions(+), 2 deletions(-) create mode 100644 block/fleecing-hook.c
qapi/block-core.json +20 -2
@@ -2628,7 +2628,8 @@ 'host_cdrom', 'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs', 'null-aio', 'null-co', 'nvme', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'rbd', 'replication', 'sheepdog', - 'ssh', 'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] } + 'ssh', 'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs', + 'fleecing-hook'] } ## # @BlockdevOptionsFile:
@@ -2719,6 +2720,22 @@ { 'struct': 'BlockdevOptionsGenericFormat', 'data': { 'file': 'BlockdevRef' } } +## +# @BlockdevOptionsFleecingHook: +# +# Driver specific block device options for image format that have no option +# besides their data source. +# +# @append-to: reference to or definition of the data source block device +# @target: reference to or definition of the data source block device +# @copy-bitmap: name for the copy-bitmap of the process. May be shared TODO: normal description here +# +# Since: 2.9 +## + { 'struct': 'BlockdevOptionsFleecingHook', + 'data': { 'append-to': 'str', 'target': 'BlockdevRef', + '*copy-bitmap': 'str'} } + ## # @BlockdevOptionsLUKS: #
@@ -3718,7 +3735,8 @@ 'vmdk': 'BlockdevOptionsGenericCOWFormat', 'vpc': 'BlockdevOptionsGenericFormat', 'vvfat': 'BlockdevOptionsVVFAT', - 'vxhs': 'BlockdevOptionsVxHS' + 'vxhs': 'BlockdevOptionsVxHS', + 'fleecing-hook': 'BlockdevOptionsFleecingHook' } } ##
block/fleecing-hook.c new file +298 -0
298 lines not on this page · raw message
block/Makefile.objs +2 -0
@@ -31,6 +31,8 @@ block-obj-y += throttle.o copy-on-read.o block-obj-y += crypto.o +block-obj-y += fleecing-hook.o + common-obj-y += stream.o nfs.o-libs := $(LIBNFS_LIBS)
Signature · 1 line
2.18.0
Re: [Qemu-devel] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook
Am 01.10.2018 um 12:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
Quoted 19 lines
Fleecing-hook filter does copy-before-write operation. It should be inserted above active disk and has a target node for CBW, like the following:
+-------+ | Guest | +---+---+ |r,w v +---+-----------+ target +---------------+ | Fleecing hook |---------->| target(qcow2) | +---+-----------+ CBW +---+-----------+ | | backing |r,w | v | +---+---------+ backing | | Active disk |<----------------+ +-------------+ r
Quoted 4 lines
Target's backing may point to active disk (should be set up separately), which gives fleecing-scheme.
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>
This lacks an explanation why we need a specialised fleecing hook driver rather than just a generic bdrv_backup_top block driver in analogy to what commit and mirror are already doing.
In fact, if I'm reading the last patch of the series right, backup doesn't even restrict the use of the fleecing-hook driver to actual fleecing scenarios.
Maybe what doesn't feel right to me is just that it's a misnomer, and if you rename it into bdrv_backup_top (and make it internal to the block job), it is very close to what I actually have in mind?
Kevin
Re: [Qemu-devel] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook
Quoted 32 lines · Kevin Wolf
Am 01.10.2018 um 12:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
Quoted 19 lines
Fleecing-hook filter does copy-before-write operation. It should be inserted above active disk and has a target node for CBW, like the following:
+-------+ | Guest | +---+---+ |r,w v +---+-----------+ target +---------------+ | Fleecing hook |---------->| target(qcow2) | +---+-----------+ CBW +---+-----------+ | | backing |r,w | v | +---+---------+ backing | | Active disk |<----------------+ +-------------+ rQuoted 4 lines
Target's backing may point to active disk (should be set up separately), which gives fleecing-scheme.
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>This lacks an explanation why we need a specialised fleecing hook driver rather than just a generic
bdrv_backup_topblock driver in analogy to what commit and mirror are already doing.In fact, if I'm reading the last patch of the series right, backup doesn't even restrict the use of the fleecing-hook driver to actual fleecing scenarios.
Quoted 5 lines
Maybe what doesn't feel right to me is just that it's a misnomer, and if you rename it into
bdrv_backup_top(and make it internal to the block job), it is very close to what I actually have in mind?Kevin
Hm.
1. assume we move to internal bdrv_backup_top
2. backup(mode=none) becomes just a wrapper for append/drop of the bdrv_backup_top node
3. looks interesting to get rid of empty (doing nothing) job and use bdrv_backup_top directly.
I want to finally create different backup schemes, based on fleecing hook, for example:
+-------+ | Guest | +-------+ |r,w v +---+-----------+ target +---------------+ +--------+ | Fleecing hook +---------->+ fleecing-node +---------->+ target | +---+-----------+ CBW +---+-----------+ backup +--------+ | | (no hook) backing |r,w | v | +---+---------+ backing | | Active disk +<----------------+ +-------------+ r
This is needed for slow nbd target, if we don't need to slow down guest writes.
Here backup(no hook) is a backup job without hook / write notifiers, as it actually
do copy from static source.
Or, we can use mirror instead of backup, as mirror is asynchronous and is faster than backup. We can even use mirror with write-blocking mode (proposed by Max) and use something like null bds (but with backing) instead of qcow2 fleecing-node - this will imitate current backup approach, but with mirror instead of backup.
Of course, we can use old backup(sync=none) for all such schemes, I just think that architecture with filter node is more clean, than with backup job, which looks the same but with additional job:
+-------+ | Guest | +-------+ |r,w v +---------------+ target +---------------+ +--------+ |bdrv_backup_top+---------->+ fleecing-node +---------->+ target | +---------------+ CBW +---+----------++ backup +--------+ | | ^ (no hook) backing |r,w | | v | | +---+---------+ backing | | | Active disk +<----------------+ | +----------+--+ r | | | | backup(sync=none) | +-------------------------------+
Finally, the first picture looks nicer and has less entities (and I didn't draw target blk which backup creates and all the permissions). Hmm, it also may be more difficult to setup permissions in the second scheme, but I didn't dive into. We just agreed with Max that separate building brick which may be reused in different schemes is better than internal thing in backup, so, I went this way. However, if you are against, it isn't difficult to move it all into backup.
Signature · 2 lines
Best regards, Vladimir
Re: [Qemu-devel] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook
Am 04.10.2018 um 15:59 hat Vladimir Sementsov-Ogievskiy geschrieben:
Quoted 37 lines
Quoted 36 lines · Kevin Wolf
Am 01.10.2018 um 12:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
Quoted 19 lines
Fleecing-hook filter does copy-before-write operation. It should be inserted above active disk and has a target node for CBW, like the following:
+-------+ | Guest | +---+---+ |r,w v +---+-----------+ target +---------------+ | Fleecing hook |---------->| target(qcow2) | +---+-----------+ CBW +---+-----------+ | | backing |r,w | v | +---+---------+ backing | | Active disk |<----------------+ +-------------+ rQuoted 4 lines
Target's backing may point to active disk (should be set up separately), which gives fleecing-scheme.
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>This lacks an explanation why we need a specialised fleecing hook driver rather than just a generic
bdrv_backup_topblock driver in analogy to what commit and mirror are already doing.In fact, if I'm reading the last patch of the series right, backup doesn't even restrict the use of the fleecing-hook driver to actual fleecing scenarios.
Maybe what doesn't feel right to me is just that it's a misnomer, and if you rename it into
bdrv_backup_top(and make it internal to the block job), it is very close to what I actually have in mind?
Quoted 6 lines
Quoted 1 line
Kevin
Hm.
1. assume we move to internalbdrv_backup_top
2. backup(mode=none) becomes just a wrapper for append/drop of thebdrv_backup_topnode
I think you mean sync=none?
Yes, this is true. There is no actual background job taking place there, so the job infrastructure doesn't add much. As you say, it's just inserting the node at the start and dropping it again at the end.
Quoted 2 lines
3. looks interesting to get rid of empty (doing nothing) job and use
bdrv_backup_topdirectly.
We could directly make the filter node available for the user, like this series does. Should we do that? I'm not sure, but I'm not necessarily opposed either.
But looking at the big picture, I have some more thoughts on this:
1. Is backup with sync=none only useful for fleecing? My understanding was that "fleecing" specifically means a setup where the target of the backup node is an overlay of the active layer of the guest device.
I can imagine other use cases that would use sync=none (e.g. if you don't access arbitrary blocks like from the NBD server in the fleecing setup, but directly write to a backup file that can be commited back later to revert things).
So I think 'fleecing-hook' is too narrow as a name. Maybe just 'backup' would be better?
2. mirror has a sync=none mode, too. And like backup, it doesn't actually have any background job running then (at least in active mirror mode), but only changes the graph at the end of the job. Some consistency would be nice there, so is the goal to eventually let the user create filter nodes for all jobs that don't have a real background job?
3. We have been thinking about unifying backup, commit and mirror into a single copy block job because they are doing quite similar things. Of course, there are differences whether the old data or the new data should be copied on a write, and which graph changes to make at the end of the job, but many of the other differences are actually features that would make sense in all of them, but are only implemented in one job driver.
Maybe having a single 'copy' filter driver that provides options to select backup-like behaviour or mirror-like behaviour, and that can then internally be used by all three block jobs would be an interesting first step towards this?
We can start with supporting only what backup needs, but design everything with the idea that mirror and commit could use it, too.
I honestly feel that at first this wouldn't be very different from what you have, so with a few renames and cleanups we might be good. But it would give us a design in the grand scheme to work towards instead of doing one-off things for every special case like fleecing and ending up with even more similar things that are implemented separately even though they do mostly the same thing.
Quoted 19 lines
I want to finally create different backup schemes, based on fleecing hook, for example:
+-------+ | Guest | +-------+ |r,w v +---+-----------+ target +---------------+ +--------+ | Fleecing hook +---------->+ fleecing-node +---------->+ target | +---+-----------+ CBW +---+-----------+ backup +--------+ | | (no hook) backing |r,w | v | +---+---------+ backing | | Active disk +<----------------+ +-------------+ r
Quoted 3 lines
This is needed for slow nbd target, if we don't need to slow down guest writes. Here backup(no hook) is a backup job without hook / write notifiers, as it actually do copy from static source.
Right.
We don't actually have a backup without a hook yet (which would be the same as the equally missing mirror for read-only nodes), but we do have commit without a hook - it doesn't share the WRITE permission for the source. This is an example for a mode that a unified 'copy' driver would automatically support.
Quoted 5 lines
Or, we can use mirror instead of backup, as mirror is asynchronous and is faster than backup. We can even use mirror with write-blocking mode (proposed by Max) and use something like null bds (but with backing) instead of qcow2 fleecing-node - this will imitate current backup approach, but with mirror instead of backup.
To be honest, I don't understand the null BDS part. null throws away whatever data is written to it, so that's certainly not what you want?
Quoted 12 lines
Of course, we can use old backup(sync=none) for all such schemes, I just think that architecture with filter node is more clean, than with backup job, which looks the same but with additional job:
+-------+ | Guest | +-------+ |r,w v +---------------+ target +---------------+ +--------+ |bdrv_backup_top+---------->+ fleecing-node +---------->+ target | +---------------+ CBW +---+----------++ backup +--------+ | | ^ (no hook)
Quoted 8 lines
backing |r,w | | v | | +---+---------+ backing | | | Active disk +<----------------+ | +----------+--+ r | | | | backup(sync=none) | +-------------------------------+
This looks only more complex because you decided to draw the block job into the graph, as an edge connecting source and target. In reality, this is not an edge that would be existing because bdrv_backup_top already has both nodes as children. The job wouldn't have an additional reference, but just use the BdrvChild that is owned by bdrv_backup_top.
Maybe this is an interesting point for the decision between an integrated filter driver in the jobs and completely separate filter driver. The jobs probably need access to the internal data structure (bs->opaque) of the filter node at least, so that they can issue requests on the child nodes.
Of course, if it isn't an internal filter driver, but a proper standalone driver, letting jobs use those child nodes might be considered a bit ugly...
Quoted 7 lines
Finally, the first picture looks nicer and has less entities (and I didn't draw target blk which backup creates and all the permissions). Hmm, it also may be more difficult to setup permissions in the second scheme, but I didn't dive into. We just agreed with Max that separate building brick which may be reused in different schemes is better than internal thing in backup, so, I went this way. However, if you are against, it isn't difficult to move it all into backup.
The idea with bdrv_backup_top would obviously be to get rid of the additional BlockBackend and BdrvChild instances and only access source and target as children of the filter node.
Kevin
Re: [Qemu-devel] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook
Quoted 62 lines · Kevin Wolf
Am 04.10.2018 um 15:59 hat Vladimir Sementsov-Ogievskiy geschrieben:
Quoted 37 lines
Quoted 36 lines · Kevin Wolf
Am 01.10.2018 um 12:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
Quoted 19 lines
Fleecing-hook filter does copy-before-write operation. It should be inserted above active disk and has a target node for CBW, like the following:
+-------+ | Guest | +---+---+ |r,w v +---+-----------+ target +---------------+ | Fleecing hook |---------->| target(qcow2) | +---+-----------+ CBW +---+-----------+ | | backing |r,w | v | +---+---------+ backing | | Active disk |<----------------+ +-------------+ rQuoted 4 lines
Target's backing may point to active disk (should be set up separately), which gives fleecing-scheme.
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>This lacks an explanation why we need a specialised fleecing hook driver rather than just a generic
bdrv_backup_topblock driver in analogy to what commit and mirror are already doing.In fact, if I'm reading the last patch of the series right, backup doesn't even restrict the use of the fleecing-hook driver to actual fleecing scenarios.
Maybe what doesn't feel right to me is just that it's a misnomer, and if you rename it into
bdrv_backup_top(and make it internal to the block job), it is very close to what I actually have in mind?Quoted 5 lines
Quoted 1 line
Kevin
Hm.
1. assume we move to internalbdrv_backup_top
2. backup(mode=none) becomes just a wrapper for append/drop of thebdrv_backup_topnodeI think you mean sync=none?
Yes, this is true. There is no actual background job taking place there, so the job infrastructure doesn't add much. As you say, it's just inserting the node at the start and dropping it again at the end.
Quoted 2 lines
3. looks interesting to get rid of empty (doing nothing) job and use
bdrv_backup_topdirectly.We could directly make the filter node available for the user, like this series does. Should we do that? I'm not sure, but I'm not necessarily opposed either.
But looking at the big picture, I have some more thoughts on this:
1. Is backup with sync=none only useful for fleecing? My understanding was that "fleecing" specifically means a setup where the target of the backup node is an overlay of the active layer of the guest device.
Quoted 7 lines
I can imagine other use cases that would use sync=none (e.g. if you don't access arbitrary blocks like from the NBD server in the fleecing setup, but directly write to a backup file that can be commited back later to revert things).
So I think 'fleecing-hook' is too narrow as a name. Maybe just 'backup' would be better?
may be copy-before-write?
Quoted 16 lines
2. mirror has a sync=none mode, too. And like backup, it doesn't actually have any background job running then (at least in active mirror mode), but only changes the graph at the end of the job. Some consistency would be nice there, so is the goal to eventually let the user create filter nodes for all jobs that don't have a real background job?
3. We have been thinking about unifying backup, commit and mirror into a single copy block job because they are doing quite similar things. Of course, there are differences whether the old data or the new data should be copied on a write, and which graph changes to make at the end of the job, but many of the other differences are actually features that would make sense in all of them, but are only implemented in one job driver.
Quoted 4 lines
Maybe having a single 'copy' filter driver that provides options to select backup-like behaviour or mirror-like behaviour, and that can then internally be used by all three block jobs would be an interesting first step towards this?
Isn't it a question about having several simple things against one complicated?)
All these jobs are similar only in the fact that they are copying blocks from one point to another.. So, instead of creating one big job with a lot of options, we can separate copying code to some kind of internal copying api, to then share it between jobs (and qemi-img convert). Didn't you considered this way? Intuitively, I'm not a fan of idea to create one job, but I don't insist. Of course, we can create one job carefully split the code to different objects files, with (again) separate copying api, shared with qemu-img, so, difference between one-job vs several-jobs will be mostly in qapi/ , not in the real code...
Quoted 33 lines
We can start with supporting only what backup needs, but design everything with the idea that mirror and commit could use it, too.
I honestly feel that at first this wouldn't be very different from what you have, so with a few renames and cleanups we might be good. But it would give us a design in the grand scheme to work towards instead of doing one-off things for every special case like fleecing and ending up with even more similar things that are implemented separately even though they do mostly the same thing.
Quoted 19 lines
I want to finally create different backup schemes, based on fleecing hook, for example:
+-------+ | Guest | +-------+ |r,w v +---+-----------+ target +---------------+ +--------+ | Fleecing hook +---------->+ fleecing-node +---------->+ target | +---+-----------+ CBW +---+-----------+ backup +--------+ | | (no hook) backing |r,w | v | +---+---------+ backing | | Active disk +<----------------+ +-------------+ rQuoted 3 lines
This is needed for slow nbd target, if we don't need to slow down guest writes. Here backup(no hook) is a backup job without hook / write notifiers, as it actually do copy from static source.
Quoted 7 lines
Right.
We don't actually have a backup without a hook yet (which would be the same as the equally missing mirror for read-only nodes), but we do have commit without a hook - it doesn't share the WRITE permission for the source. This is an example for a mode that a unified 'copy' driver would automatically support.
I'm just afraid that copy driver will be even more complicated than mirror already is. Mirror needs several iterations through the whole disk, other jobs - don't..
Quoted 8 lines
Quoted 6 lines
Or, we can use mirror instead of backup, as mirror is asynchronous and is faster than backup. We can even use mirror with write-blocking mode (proposed by Max) and use something like null bds (but with backing) instead of qcow2 fleecing-node - this will imitate current backup approach, but with mirror instead of backup.
To be honest, I don't understand the null BDS part. null throws away whatever data is written to it, so that's certainly not what you want?
Exactly that. We don't need this data in fleecing node, as active sync mirror will copy it in-flight.
Quoted 33 lines
Quoted 13 lines
Of course, we can use old backup(sync=none) for all such schemes, I just think that architecture with filter node is more clean, than with backup job, which looks the same but with additional job:
+-------+ | Guest | +-------+ |r,w v +---------------+ target +---------------+ +--------+ |bdrv_backup_top+---------->+ fleecing-node +---------->+ target | +---------------+ CBW +---+----------++ backup +--------+ | | ^ (no hook)Quoted 8 lines
backing |r,w | | v | | +---+---------+ backing | | | Active disk +<----------------+ | +----------+--+ r | | | | backup(sync=none) | +-------------------------------+This looks only more complex because you decided to draw the block job into the graph, as an edge connecting source and target. In reality, this is not an edge that would be existing because
bdrv_backup_topalready has both nodes as children. The job wouldn't have an additional reference, but just use the BdrvChild that is owned bybdrv_backup_top.Maybe this is an interesting point for the decision between an integrated filter driver in the jobs and completely separate filter driver. The jobs probably need access to the internal data structure (
bs->opaque) of the filter node at least, so that they can issue requests on the child nodes.
Quoted 3 lines
Of course, if it isn't an internal filter driver, but a proper standalone driver, letting jobs use those child nodes might be considered a bit ugly...
Yes, in this case, there should be different children, sharing the same target BDS.. Hmm, sharing a BdrvChild is a point in favor of internal filter.
Quoted 8 lines
Quoted 8 lines
Finally, the first picture looks nicer and has less entities (and I didn't draw target blk which backup creates and all the permissions). Hmm, it also may be more difficult to setup permissions in the second scheme, but I didn't dive into. We just agreed with Max that separate building brick which may be reused in different schemes is better than internal thing in backup, so, I went this way. However, if you are against, it isn't difficult to move it all into backup.
Quoted 5 lines
The idea with
bdrv_backup_topwould obviously be to get rid of the additional BlockBackend and BdrvChild instances and only access source and target as children of the filter node.Kevin
Ok. Let's start from internal driver, anyway, it is a lot easier to turn from internal to external if it is needed than vice versa. I'll resend. Most of prerequisites will not change. Also, anyway, I'll need to share copy-bitmap between two backup jobs, to avoid extra copying in the picture above. And anyway, I want to try to get rid of backup intersecting requests, using serializing requests and copy-bitmap instead.
Re: [Qemu-devel] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook
05.10.2018 00:19, Vladimir Sementsov-Ogievskiy wrote:
On 10/04/2018 05:52 PM, Kevin Wolf wrote:
Am 04.10.2018 um 15:59 hat Vladimir Sementsov-Ogievskiy geschrieben:
04.10.2018 15:44, Kevin Wolf wrote:
Am 01.10.2018 um 12:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
Fleecing-hook filter does copy-before-write operation. It should be inserted above active disk and has a target node for CBW, like the following:
+-------+
| Guest |
+---+---+
|r,w
v
+---+-----------+ target +---------------+
| Fleecing hook |---------->| target(qcow2) |
+---+-----------+ CBW +---+-----------+
| |
backing |r,w |
v |
+---+---------+ backing |
| Active disk |<----------------+
+-------------+ rTarget's backing may point to active disk (should be set up separately), which gives fleecing-scheme.
This lacks an explanation why we need a specialised fleecing hook driver rather than just a generic bdrv_backup_top block driver in analogy to what commit and mirror are already doing.
In fact, if I'm reading the last patch of the series right, backup doesn't even restrict the use of the fleecing-hook driver to actual fleecing scenarios.
Maybe what doesn't feel right to me is just that it's a misnomer, and if you rename it into bdrv_backup_top (and make it internal to the block job), it is very close to what I actually have in mind?
Kevin
Hm.
1. assume we move to internal bdrv_backup_top
2. backup(mode=none) becomes just a wrapper for append/drop of the bdrv_backup_top node
I think you mean sync=none?
Yes, this is true. There is no actual background job taking place there, so the job infrastructure doesn't add much. As you say, it's just inserting the node at the start and dropping it again at the end.
3. looks interesting to get rid of empty (doing nothing) job and use bdrv_backup_top directly.
We could directly make the filter node available for the user, like this series does. Should we do that? I'm not sure, but I'm not necessarily opposed either.
But looking at the big picture, I have some more thoughts on this:
1. Is backup with sync=none only useful for fleecing? My understanding was that "fleecing" specifically means a setup where the target of the backup node is an overlay of the active layer of the guest device.
I can imagine other use cases that would use sync=none (e.g. if you don't access arbitrary blocks like from the NBD server in the fleecing setup, but directly write to a backup file that can be commited back later to revert things).
So I think 'fleecing-hook' is too narrow as a name. Maybe just 'backup' would be better?
may be copy-before-write?
2. mirror has a sync=none mode, too. And like backup, it doesn't actually have any background job running then (at least in active mirror mode), but only changes the graph at the end of the job. Some consistency would be nice there, so is the goal to eventually let the user create filter nodes for all jobs that don't have a real background job?
3. We have been thinking about unifying backup, commit and mirror into a single copy block job because they are doing quite similar things. Of course, there are differences whether the old data or the new data should be copied on a write, and which graph changes to make at the end of the job, but many of the other differences are actually features that would make sense in all of them, but are only implemented in one job driver.
Maybe having a single 'copy' filter driver that provides options to select backup-like behaviour or mirror-like behaviour, and that can then internally be used by all three block jobs would be an interesting first step towards this?
Isn't it a question about having several simple things against one complicated?)
All these jobs are similar only in the fact that they are copying blocks from one point to another.. So, instead of creating one big job with a lot of options, we can separate copying code to some kind of internal copying api, to then share it between jobs (and qemi-img convert). Didn't you considered this way? Intuitively, I'm not a fan of idea to create one job, but I don't insist. Of course, we can create one job carefully split the code to different objects files, with (again) separate copying api, shared with qemu-img, so, difference between one-job vs several-jobs will be mostly in qapi/ , not in the real code...
We can start with supporting only what backup needs, but design everything with the idea that mirror and commit could use it, too.
I honestly feel that at first this wouldn't be very different from what you have, so with a few renames and cleanups we might be good. But it would give us a design in the grand scheme to work towards instead of doing one-off things for every special case like fleecing and ending up with even more similar things that are implemented separately even though they do mostly the same thing.
I want to finally create different backup schemes, based on fleecing hook, for example:
+-------+
| Guest |
+-------+
|r,w
v
+---+-----------+ target +---------------+ +--------+
| Fleecing hook +---------->+ fleecing-node +---------->+ target |
+---+-----------+ CBW +---+-----------+ backup +--------+
| | (no hook)
backing |r,w |
v |
+---+---------+ backing |
| Active disk +<----------------+
+-------------+ rThis is needed for slow nbd target, if we don't need to slow down guest writes. Here backup(no hook) is a backup job without hook / write notifiers, as it actually do copy from static source. Right.
We don't actually have a backup without a hook yet (which would be the same as the equally missing mirror for read-only nodes), but we do have commit without a hook - it doesn't share the WRITE permission for the source. This is an example for a mode that a unified 'copy' driver would automatically support.
I'm just afraid that copy driver will be even more complicated than mirror already is. Mirror needs several iterations through the whole disk, other jobs - don't..
Or, we can use mirror instead of backup, as mirror is asynchronous and is faster than backup. We can even use mirror with write-blocking mode (proposed by Max) and use something like null bds (but with backing) instead of qcow2 fleecing-node - this will imitate current backup approach, but with mirror instead of backup.
To be honest, I don't understand the null BDS part. null throws away whatever data is written to it, so that's certainly not what you want?
Exactly that. We don't need this data in fleecing node, as active sync mirror will copy it in-flight.
Of course, we can use old backup(sync=none) for all such schemes, I just think that architecture with filter node is more clean, than with backup job, which looks the same but with additional job:
+-------+
| Guest |
+-------+
|r,w
v
+---------------+ target +---------------+ +--------+
|bdrv_backup_top+---------->+ fleecing-node +---------->+ target |
+---------------+ CBW +---+----------++ backup +--------+
| | ^ (no hook)
backing |r,w | |
v | |
+---+---------+ backing | |
| Active disk +<----------------+ |
+----------+--+ r |
| |
| backup(sync=none) |
+-------------------------------+This looks only more complex because you decided to draw the block job into the graph, as an edge connecting source and target. In reality, this is not an edge that would be existing because bdrv_backup_top already has both nodes as children. The job wouldn't have an additional reference, but just use the BdrvChild that is owned by bdrv_backup_top.
Maybe this is an interesting point for the decision between an integrated filter driver in the jobs and completely separate filter driver. The jobs probably need access to the internal data structure (bs->opaque) of the filter node at least, so that they can issue requests on the child nodes.
Of course, if it isn't an internal filter driver, but a proper standalone driver, letting jobs use those child nodes might be considered a bit ugly...
Yes, in this case, there should be different children, sharing the same target BDS.. Hmm, sharing a BdrvChild is a point in favor of internal filter.
Hmm, how to share children?
backup job has two source BdrvChild'ren - child_job and child_root of job blk and two target BdrvChild'ren - again, child_job and child_root.
backup_top has source child - child_backing and second - child_file (named "target")..
Which BdrvChild'ren you suggest to remove? They are all different.
I don't know, why job needs both unnamed blk's and child_job's, and I don't know is it necessary for backup to use blk's not BdrvChild'ren..
And with internal way in none-mode we'll have two unused blk's and four unused BdrvChild'ren.. Or we want to rewrite backup to use BdrvChild'ren for io operations and drop child_job BdrvChild'ren? So I'm lost. What did you mean?
Finally, the first picture looks nicer and has less entities (and I didn't draw target blk which backup creates and all the permissions). Hmm, it also may be more difficult to setup permissions in the second scheme, but I didn't dive into. We just agreed with Max that separate building brick which may be reused in different schemes is better than internal thing in backup, so, I went this way. However, if you are against, it isn't difficult to move it all into backup.
The idea with bdrv_backup_top would obviously be to get rid of the additional BlockBackend and BdrvChild instances and only access source and target as children of the filter node.
Kevin
Ok. Let's start from internal driver, anyway, it is a lot easier to turn from internal to external if it is needed than vice versa. I'll resend. Most of prerequisites will not change. Also, anyway, I'll need to share copy-bitmap between two backup jobs, to avoid extra copying in the picture above. And anyway, I want to try to get rid of backup intersecting requests, using serializing requests and copy-bitmap instead.
--
Best regards,
Vladimir
Re: [Qemu-devel] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook
Hi Vladimir,
can you please check your mailer settings? The plain text version of the emails is hardly legible because it mixes quotes text and replies. I had to manually open the HTML part to figure out what you really wrote.
Am 05.10.2018 um 17:00 hat Vladimir Sementsov-Ogievskiy geschrieben:
Quoted 8 lines
Hmm, how to share children?
backup job has two source BdrvChild'ren -
child_jobandchild_rootof job blk and two target BdrvChild'ren - again,child_jobandchild_root.
backup_tophas source child -child_backingand second -child_file(named "target")..
Right, these are six BdrvChild instances in total. I think we can ignore the child_job ones, they are internal to the block job infrastructure, so we have four of them left.
Quoted 1 line
Which BdrvChild'ren you suggest to remove? They are all different.
Now that you introduced backup_top, I think we don't need any BlockBackends any more. So I suggest to remove the child_root ones and to do all I/O through the child_backing and child_file ones of backup_top.
Quoted 2 lines
I don't know, why job needs both unnamed blk's and child_job's, and I don't know is it necessary for backup to use blk's not BdrvChild'ren..
I think we had a case recently where it turned out that it is strictly speaking even wrong for jobs to use BlockBackends in a function that intercepts a request on the BDS level (like the copy-before-write of backup).
So getting rid of the BlockBackends isn't only okay, but actually a good thing by itself.
Quoted 4 lines
And with internal way in none-mode we'll have two unused blk's and four unused BdrvChild'ren.. Or we want to rewrite backup to use BdrvChild'ren for io operations and drop
child_jobBdrvChild'ren? So I'm lost. What did you mean?
child_job isn't actually unused, even though you never use them to make requests. The child_job BdrvChild is important because of the BdrvChildRole callbacks it provides to the block job infrastructure.
Kevin
Re: [Qemu-devel] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook
Quoted 5 lines · Kevin Wolf
Hi Vladimir,
can you please check your mailer settings? The plain text version of the emails is hardly legible because it mixes quotes text and replies. I had to manually open the HTML part to figure out what you really wrote.
I've sent it from other thunderbird instance from home, I hope thunderbird at work (where I'm composing now) is ok..
Quoted 34 lines
Am 05.10.2018 um 17:00 hat Vladimir Sementsov-Ogievskiy geschrieben:
Quoted 8 lines
Hmm, how to share children?
backup job has two source BdrvChild'ren -
child_jobandchild_rootof job blk and two target BdrvChild'ren - again,child_jobandchild_root.
backup_tophas source child -child_backingand second -child_file(named "target")..Right, these are six BdrvChild instances in total. I think we can ignore the
child_jobones, they are internal to the block job infrastructure, so we have four of them left.Quoted 1 line
Which BdrvChild'ren you suggest to remove? They are all different.
Now that you introduced
backup_top, I think we don't need any BlockBackends any more. So I suggest to remove thechild_rootones and to do all I/O through thechild_backingandchild_fileones ofbackup_top.Quoted 2 lines
I don't know, why job needs both unnamed blk's and child_job's, and I don't know is it necessary for backup to use blk's not BdrvChild'ren..
I think we had a case recently where it turned out that it is strictly speaking even wrong for jobs to use BlockBackends in a function that intercepts a request on the BDS level (like the copy-before-write of backup).
So getting rid of the BlockBackends isn't only okay, but actually a good thing by itself.
Quoted 4 lines
And with internal way in none-mode we'll have two unused blk's and four unused BdrvChild'ren.. Or we want to rewrite backup to use BdrvChild'ren for io operations and drop
child_jobBdrvChild'ren? So I'm lost. What did you mean?
Quoted 5 lines
child_jobisn't actually unused, even though you never use them to make requests. Thechild_jobBdrvChild is important because of the BdrvChildRole callbacks it provides to the block job infrastructure.Kevin
Ok, understand, thank you for the explanation!
Signature · 2 lines
Best regards, Vladimir
Re: [Qemu-devel] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook
Quoted 9 lines · Vladimir Sementsov-Ogievskiy
Quoted 5 lines · Kevin Wolf
Hi Vladimir,
can you please check your mailer settings? The plain text version of the emails is hardly legible because it mixes quotes text and replies. I had to manually open the HTML part to figure out what you really wrote.
I've sent it from other thunderbird instance from home, I hope thunderbird at work (where I'm composing now) is ok..
Comparing the two:
Home:
Message-ID: <[email protected]>
...
Content-Type: multipart/alternative; boundary="_000_46e224bd8c1f4565944e52440e85e2f0virtuozzocom_"
Work:
Message-ID: <[email protected]>
...
Content-Type: text/plain; charset="utf-8"
Content-ID: <[email protected]>
Content-Transfer-Encoding: base64
So, the difference is that at home, you haven't told thunderbird to send plain-text only emails to specific recipients (setting up the list as one of those recipients that wants plain-text only), and something else in your local configurations then results in a multipart email where the html portion looks fine but the plain-text portion has horrendous quoting. But at work, you are configured for plain-text-only output, html is not even available, and the quoting is decent from the start.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook
Thank you, hope that's fixed now)
Quoted 34 lines · Eric Blake
Quoted 9 lines · Vladimir Sementsov-Ogievskiy
Quoted 5 lines · Kevin Wolf
Hi Vladimir,
can you please check your mailer settings? The plain text version of the emails is hardly legible because it mixes quotes text and replies. I had to manually open the HTML part to figure out what you really wrote.
I've sent it from other thunderbird instance from home, I hope thunderbird at work (where I'm composing now) is ok..
Comparing the two:
Home:
Message-ID: <[email protected]>
...
Content-Type: multipart/alternative;
boundary="_000_46e224bd8c1f4565944e52440e85e2f0virtuozzocom_"Work:
Message-ID: <[email protected]>
...
Content-Type: text/plain; charset="utf-8"
Content-ID: <[email protected]>
Content-Transfer-Encoding: base64So, the difference is that at home, you haven't told thunderbird to send plain-text only emails to specific recipients (setting up the list as one of those recipients that wants plain-text only), and something else in your local configurations then results in a multipart email where the html portion looks fine but the plain-text portion has horrendous quoting. But at work, you are configured for plain-text-only output, html is not even available, and the quoting is decent from the start.
PATCH v3 14/18 block/fleecing-hook: internal api
In reply to Vladimir Sementsov-OgievskiyAdd some functions to use fleecing-hook internally from backup job in further commit.
--- include/block/block.h | 9 ++++++++ block/fleecing-hook.c | 51 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+)
include/block/block.h +9 -0
@@ -710,4 +710,13 @@ int coroutine_fn bdrv_co_copy_range(BdrvChild *src, uint64_t src_offset, BdrvChild *dst, uint64_t dst_offset, uint64_t bytes, BdrvRequestFlags read_flags, BdrvRequestFlags write_flags); + + +BlockDriverState *bdrv_fleecing_hook_append (BlockDriverState *source, +BlockDriverState *target, +const char *copy_bitmap_name, +Error **errp); +void bdrv_fleecing_hook_drop (BlockDriverState *hook); +uint64_t bdrv_fleecing_hook_progress (BlockDriverState *hook); +#endif
block/fleecing-hook.c +51 -0
66 lines not on this page · raw message
Signature · 1 line
2.18.0
Re: [Qemu-devel] [PATCH v3 14/18] block/fleecing-hook: internal api
Am 01.10.2018 um 12:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
Quoted 15 lines
Add some functions to use fleecing-hook internally from backup job in further commit.
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- include/block/block.h | 9 ++++++++ block/fleecing-hook.c | 51 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+)include/block/block.h +0
-0@@ -710,4 +710,13 @@ int coroutine_fn bdrv_co_copy_range(BdrvChild *src, uint64_t src_offset, BdrvChild *dst, uint64_t dst_offset,
include/block/block.h int coroutine_fn bdrv_co_copy_range(BdrvChild *src, uint64_t src_offset, · line 720
Quoted 10 lines
include/block/block.h +8
-0uint64_t bytes, BdrvRequestFlags read_flags, BdrvRequestFlags write_flags); + + +BlockDriverState *bdrv_fleecing_hook_append (BlockDriverState *source, +BlockDriverState *target, +const char *copy_bitmap_name, +Error **errp); +void bdrv_fleecing_hook_drop (BlockDriverState *hook); +uint64_t bdrv_fleecing_hook_progress (BlockDriverState *hook);
Public block.h interfaces that are specific to a single driver are bad design. If at all possible, the generic block layer shouldn't know anything about specific drivers.
Luckily, if you make the driver more consistent with what mirror and commit are doing, you won't need public interfaces any more and the functions will just become part of the backup job driver. (The backup driver can still be split into multiple source files if necessary, and then use a separate internal header; I'm not sure if this is the case here, though.)
Kevin
PATCH v3 15/18 qapi: add x-drop-fleecing qmp command
In reply to Vladimir Sementsov-OgievskiyTo insert fleecing-hook, it should be just created (blockdev-add) with corresponding parameters. But it can't be properly removed by blockdev-del (we can't restore backing chain in fleecing-hook .close), So, let's add this expiremental api to drop the hook.
--- qapi/block-core.json | 17 +++++++++++++++++ blockdev.c | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+)
qapi/block-core.json +17 -0
@@ -3856,6 +3856,23 @@ ## { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } } +## +# @x-drop-fleecing: +# +# Deletes fleecing-hook filter from the top of the backing chain. +# +# @node-name: Name of the fleecing-hook node name. +# +# Since: 3.1 +# +# -> { "execute": "x-drop-fleecing", +# "arguments": { "node-name": "fleece0" } +# } +# <- { "return": {} } +# +## +{ 'command': 'x-drop-fleecing', 'data': { 'node-name': 'str' } } + ## # @BlockdevCreateOptionsFile: #
blockdev.c +37 -0
@@ -4250,6 +4250,43 @@ out:aio_context_release (aio_context); } +void qmp_x_drop_fleecing (const char *node_name,Error **errp) +{ +AioContext *aio_context; +BlockDriverState *bs; + + bs =bdrv_find_node (node_name); +if (!bs) { +error_setg (errp,"Cannot find node %s" , node_name); +return ; + } + +if (!bdrv_has_blk (bs)) { +error_setg (errp,"Node %s is not inserted" , node_name); +return ; + } + +if (!bs->backing) { +error_setg (errp,"'%s' has no backing" , node_name); +return ; + } + + aio_context =bdrv_get_aio_context (bs); +aio_context_acquire (aio_context); + +bdrv_drained_begin (bs); + +bdrv_child_try_set_perm (bs->backing,0 ,BLK_PERM_ALL , &error_abort); +bdrv_replace_node (bs,backing_bs (bs), &error_abort); +bdrv_set_backing_hd (bs,NULL , &error_abort); + +bdrv_drained_end (bs); + +qmp_blockdev_del (node_name, &error_abort); + +aio_context_release (aio_context); +} +static BdrvChild *bdrv_find_child (BlockDriverState *parent_bs,const char *child_name) {
Signature · 1 line
2.18.0
PATCH v3 16/18 iotests: test new fleecing-hook driver in context of 222 iotest
In reply to Vladimir Sementsov-Ogievskiy--- tests/qemu-iotests/222 | 59 +++++++++++++++++++++++++++------- tests/qemu-iotests/222.out | 66 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 11 deletions(-)
tests/qemu-iotests/222 +48 -11
@@ -4,7 +4,9 @@ # point-in-time snapshot of a node that can be queried over NBD. # # Copyright (C) 2018 Red Hat, Inc. +# Copyright (c) 2018 Virtuozzo International GmbH. All rights reserved. # John helped, too. +# And Vladimir, a bit. # # 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
@@ -46,11 +48,40 @@ remainder = [("0xd5", "0x108000", "32k"), # Right-end of partial-left [1] ("0xdc", "32M", "32k"), # Left-end of partial-right [2] ("0xcd", "0x3ff0000", "64k")] # patterns[3] -with iotests.FilePath('base.img') as base_img_path, \ - iotests.FilePath('fleece.img') as fleece_img_path, \ - iotests.FilePath('nbd.sock') as nbd_sock_path, \ - iotests.VM() as vm: +class Fleecing(): + def __init__(self, vm, src_node, tgt_node): + self.vm = vm + self.src_node = src_node + self.tgt_node = tgt_node +class OldBackupFleecing(Fleecing): + def start(self): + log(self.vm.qmp("blockdev-backup", + device=self.src_node, + target=self.tgt_node, + sync="none")) + + def stop(self): + log(self.vm.qmp('block-job-cancel', device=self.src_node)) + log(self.vm.event_wait('BLOCK_JOB_CANCELLED'), + filters=[iotests.filter_qmp_event]) + + +class FleecingHookFleecing(Fleecing): + def start(self): + log(self.vm.qmp("blockdev-add", **{ + "driver": "fleecing-hook", + "node-name": "hook", + "append-to": self.src_node, + "target": self.tgt_node, + })) + + def stop(self): + log(self.vm.qmp('x-drop-fleecing', node_name="hook")) + + +def do_test(base_img_path, fleece_img_path, nbd_sock_path, fleecing): + vm = iotests.VM() log('--- Setting up images ---') log('')
@@ -77,6 +108,7 @@ with iotests.FilePath('base.img') as base_img_path, \ src_node = "drive0" tgt_node = "fleeceNode" + fl = fleecing(vm, src_node, tgt_node) # create tgt_node backed by src_node log(vm.qmp("blockdev-add", **{
@@ -90,10 +122,7 @@ with iotests.FilePath('base.img') as base_img_path, \ })) # Establish COW from source to fleecing node - log(vm.qmp("blockdev-backup", - device=src_node, - target=tgt_node, - sync="none")) + fl.start() log('') log('--- Setting up NBD Export ---')
@@ -137,10 +166,8 @@ with iotests.FilePath('base.img') as base_img_path, \ log('--- Cleanup ---') log('') - log(vm.qmp('block-job-cancel', device=src_node)) - log(vm.event_wait('BLOCK_JOB_CANCELLED'), - filters=[iotests.filter_qmp_event]) log(vm.qmp('nbd-server-stop')) + fl.stop() log(vm.qmp('blockdev-del', node_name=tgt_node)) vm.shutdown()
@@ -155,3 +182,13 @@ with iotests.FilePath('base.img') as base_img_path, \ log('') log('Done') + +def test(fleecing): + with iotests.FilePath('base.img') as base_img_path, \ + iotests.FilePath('fleece.img') as fleece_img_path, \ + iotests.FilePath('nbd.sock') as nbd_sock_path: + + do_test(base_img_path, fleece_img_path, nbd_sock_path, fleecing) + +test(OldBackupFleecing) +test(FleecingHookFleecing)
tests/qemu-iotests/222.out +66 -0
75 lines not on this page · raw message
Signature · 1 line
2.18.0
PATCH v3 17/18 block/backup: tiny refactor backup_job_create
In reply to Vladimir Sementsov-OgievskiyMove copy-bitmap find/create code. It's needed for the following commit, as we'll need copy_bitmap before actual block job creation. Do it in a separate commit to simplify review.
--- block/backup.c | 95 +++++++++++++++++++++++++++----------------------- 1 file changed, 52 insertions(+), 43 deletions(-)
block/backup.c +52 -43
@@ -570,6 +570,9 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,BackupBlockJob *job =NULL ;int ret;char *gen_bitmap_name =NULL ; +int64_t cluster_size; +BdrvDirtyBitmap *copy_bitmap =NULL ; +bool copy_bitmap_created = false;assert (bs);assert (target);
@@ -624,6 +627,48 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,return NULL ; } +/* If there is no backing file on the target, we cannot rely on COW if our +* backup cluster size is smaller than the target cluster size. Even for +* targets with a backing file, try to avoid COW if possible. */ + ret =bdrv_get_info (target, &bdi); +if (ret == -ENOTSUP && !target->backing) { +/* Cluster size is not defined */ +warn_report ("The target block device doesn't provide " +"information about the block size and it doesn't have a " +"backing file. The default block size of %u bytes is " +"used. If the actual block size of the target exceeds " +"this default, the backup may be unusable" , +BACKUP_CLUSTER_SIZE_DEFAULT ); + cluster_size =BACKUP_CLUSTER_SIZE_DEFAULT ; + }else if (ret <0 && !target->backing) { +error_setg_errno (errp, -ret, +"Couldn't determine the cluster size of the target image, " +"which has no backing file" ); +error_append_hint (errp, +"Aborting, since this may create an unusable destination image\n" ); +return NULL ; + }else if (ret <0 && target->backing) { +/* Not fatal; just trudge on ahead. */ + cluster_size =BACKUP_CLUSTER_SIZE_DEFAULT ; + }else { + cluster_size =MAX (BACKUP_CLUSTER_SIZE_DEFAULT , bdi.cluster_size); + } + +if (x_copy_bitmap) { + copy_bitmap =bdrv_find_dirty_bitmap (bs, x_copy_bitmap); + }else { + x_copy_bitmap = gen_bitmap_name =id_generate (ID_BLOCK_BITMAP ); + } +if (!copy_bitmap) { + copy_bitmap =bdrv_create_dirty_bitmap (bs, cluster_size, + x_copy_bitmap, errp); + copy_bitmap_created = !!copy_bitmap; + } +g_free (gen_bitmap_name); +if (!copy_bitmap) { +return NULL ; + } + len =bdrv_getlength (bs);if (len <0 ) { error_setg_errno(errp, -len, "unable to get length for '%s'",
@@ -659,49 +704,9 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,/* Detect image-fleecing (and similar) schemes */ job->serialize_target_writes =bdrv_chain_contains (target, bs); - -/* If there is no backing file on the target, we cannot rely on COW if our -* backup cluster size is smaller than the target cluster size. Even for -* targets with a backing file, try to avoid COW if possible. */ - ret =bdrv_get_info (target, &bdi); -if (ret == -ENOTSUP && !target->backing) { -/* Cluster size is not defined */ -warn_report ("The target block device doesn't provide " -"information about the block size and it doesn't have a " -"backing file. The default block size of %u bytes is " -"used. If the actual block size of the target exceeds " -"this default, the backup may be unusable" , -BACKUP_CLUSTER_SIZE_DEFAULT ); - job->cluster_size =BACKUP_CLUSTER_SIZE_DEFAULT ; - }else if (ret <0 && !target->backing) { -error_setg_errno (errp, -ret, -"Couldn't determine the cluster size of the target image, " -"which has no backing file" ); -error_append_hint (errp, -"Aborting, since this may create an unusable destination image\n" ); - goto error; - }else if (ret <0 && target->backing) { -/* Not fatal; just trudge on ahead. */ - job->cluster_size =BACKUP_CLUSTER_SIZE_DEFAULT ; - }else { - job->cluster_size =MAX (BACKUP_CLUSTER_SIZE_DEFAULT , bdi.cluster_size); - } - -if (x_copy_bitmap) { - job->copy_bitmap =bdrv_find_dirty_bitmap (bs, x_copy_bitmap); - }else { - x_copy_bitmap = gen_bitmap_name =id_generate (ID_BLOCK_BITMAP ); - } -if (!job->copy_bitmap) { - job->copy_bitmap =bdrv_create_dirty_bitmap (bs, job->cluster_size, - x_copy_bitmap, errp); - job->copy_bitmap_created = !!job->copy_bitmap; - } -g_free (gen_bitmap_name); -if (!job->copy_bitmap) { - goto error; - } - + job->cluster_size = cluster_size; + job->copy_bitmap = copy_bitmap; + job->copy_bitmap_created = copy_bitmap_created; job->use_copy_range = true; job->copy_range_size =MIN_NON_ZERO (blk_get_max_transfer (job->common.blk),blk_get_max_transfer (job->target));
@@ -717,6 +722,10 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,return &job->common; error: +if (copy_bitmap_created) { +assert (!job || !job->copy_bitmap_created); +bdrv_release_dirty_bitmap (bs, copy_bitmap); + }if (sync_bitmap) {bdrv_reclaim_dirty_bitmap (bs, sync_bitmap,NULL ); }
Signature · 1 line
2.18.0
PATCH v3 18/18 block/backup: use fleecing-hook instead of write notifiers
In reply to Vladimir Sementsov-OgievskiyDrop write notifiers and use filter node instead. Changes:
1. copy-before-writes now handled by filter node, so, drop all is_write_notifier arguments.
2. we don't have intersecting requests, so their handling is dropped. Instead, synchronization works as follows:
when backup or fleecing-hook starts copying of some area it firstly clears copy-bitmap bits, and nobody touches areas, not marked with dirty bits in copy-bitmap, so there no intersection. Also, read requests are marked serializing, to not interfer with guest writes and not read changed data from source (before reading we clear corresponding bit in copy-bitmap, so, this area is not more handled by fleecing-hook).
3. To sync with in-flight requests we no just drain hook node, we don't need rw-lock.
--- block/backup.c | 142 ++++++++++++++++--------------------------------- 1 file changed, 45 insertions(+), 97 deletions(-)
block/backup.c +45 -97
@@ -29,13 +29,6 @@#define BACKUP_CLUSTER_SIZE_DEFAULT (1 << 16) -typedef struct CowRequest { -int64_t start_byte; -int64_t end_byte; -QLIST_ENTRY (CowRequest) list; -CoQueue wait_queue;/* coroutines blocked on this request */ -}CowRequest ; - typedef struct BackupBlockJob { BlockJob common; BlockBackend *target;
@@ -44,13 +37,10 @@ typedef struct BackupBlockJob {MirrorSyncMode sync_mode;BlockdevOnError on_source_error;BlockdevOnError on_target_error; -CoRwlock flush_rwlock;uint64_t len;uint64_t bytes_read;int64_t cluster_size;bool compress; -NotifierWithReturn before_write; - QLIST_HEAD(, CowRequest) inflight_reqs;BdrvDirtyBitmap *copy_bitmap;bool copy_bitmap_created;
@@ -58,53 +48,18 @@ typedef struct BackupBlockJob {int64_t copy_range_size;bool serialize_target_writes; + +BlockDriverState *hook; +uint64_t fleecing_hook_progress; } BackupBlockJob;static const BlockJobDriver backup_job_driver; -/* See if in-flight requests overlap and wait for them to complete */ -static void coroutine_fn wait_for_overlapping_requests(BackupBlockJob *job, -int64_t start, -int64_t end) -{ -CowRequest *req; -bool retry; - -do { - retry = false; - QLIST_FOREACH(req, &job->inflight_reqs, list) { -if (end > req->start_byte && start < req->end_byte) { -qemu_co_queue_wait (&req->wait_queue,NULL ); - retry = true; -break ; - } - } - }while (retry); -} - -/* Keep track of an in-flight request */ -static void cow_request_begin (CowRequest *req,BackupBlockJob *job, -int64_t start,int64_t end) -{ - req->start_byte = start; - req->end_byte = end; -qemu_co_queue_init (&req->wait_queue); -QLIST_INSERT_HEAD (&job->inflight_reqs, req, list); -} - -/* Forget about a completed request */ -static void cow_request_end (CowRequest *req) -{ -QLIST_REMOVE (req, list); -qemu_co_queue_restart_all (&req->wait_queue); -} -/* Copy range to target with a bounce buffer and return the bytes copied. If * error occurred, return a negative error number */ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,int64_t start,int64_t end, -bool is_write_notifier,bool *error_is_read,void **bounce_buffer) {
@@ -113,7 +68,7 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,QEMUIOVector qiov;BlockBackend *blk = job->common.blk;int nbytes; -int read_flags = is_write_notifier ?BDRV_REQ_NO_SERIALISING :0 ; +int read_flags =BDRV_REQ_SERIALISING ;int write_flags = job->serialize_target_writes ?BDRV_REQ_SERIALISING :0 ;assert (start % job->cluster_size ==0 );
@@ -161,15 +116,13 @@ fail:/* Copy range to target and return the bytes copied. If error occurred, return a * negative error number. */ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job, -int64_t start, -int64_t end, -bool is_write_notifier) +int64_t start,int64_t end) {int ret;int nr_clusters;BlockBackend *blk = job->common.blk;int nbytes; -int read_flags = is_write_notifier ?BDRV_REQ_NO_SERIALISING :0 ; +int read_flags =BDRV_REQ_SERIALISING ;int write_flags = job->serialize_target_writes ?BDRV_REQ_SERIALISING :0 ;assert (QEMU_IS_ALIGNED (job->copy_range_size, job->cluster_size));
@@ -192,24 +145,18 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job, static int coroutine_fn backup_do_cow(BackupBlockJob *job,int64_t offset,uint64_t bytes, -bool *error_is_read, -bool is_write_notifier) +bool *error_is_read) { -CowRequest cow_request;int ret =0 ;int64_t start, end;/* bytes */ void *bounce_buffer =NULL ; - -qemu_co_rwlock_rdlock (&job->flush_rwlock); +uint64_t fleecing_hook_progress; start =QEMU_ALIGN_DOWN (offset, job->cluster_size); end =QEMU_ALIGN_UP (bytes + offset, job->cluster_size);trace_backup_do_cow_enter (job, start, offset, bytes); -wait_for_overlapping_requests (job, start, end); -cow_request_begin (&cow_request, job, start, end); -while (start < end) {if (!bdrv_get_dirty_locked (NULL , job->copy_bitmap, start)) {trace_backup_do_cow_skip (job, start);
@@ -220,13 +167,13 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,trace_backup_do_cow_process (job, start);if (job->use_copy_range) { - ret =backup_cow_with_offload (job, start, end, is_write_notifier); + ret =backup_cow_with_offload (job, start, end);if (ret <0 ) { job->use_copy_range = false; } }if (!job->use_copy_range) { - ret =backup_cow_with_bounce_buffer (job, start, end, is_write_notifier, + ret =backup_cow_with_bounce_buffer (job, start, end, error_is_read, &bounce_buffer); }if (ret <0 ) {
@@ -238,7 +185,10 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,*/ start += ret; job->bytes_read += ret; -job_progress_update (&job->common.job, ret); + fleecing_hook_progress =bdrv_fleecing_hook_progress (job->hook); +job_progress_update (&job->common.job, ret + fleecing_hook_progress - + job->fleecing_hook_progress); + job->fleecing_hook_progress = fleecing_hook_progress; ret =0 ; }
@@ -246,29 +196,11 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,qemu_vfree (bounce_buffer); } -cow_request_end (&cow_request); -trace_backup_do_cow_return (job, offset, bytes, ret); -qemu_co_rwlock_unlock (&job->flush_rwlock); -return ret; } -static int coroutine_fn backup_before_write_notify( -NotifierWithReturn *notifier, -void *opaque) -{ -BackupBlockJob *job =container_of (notifier, BackupBlockJob, before_write); -BdrvTrackedRequest *req = opaque; - -assert (req->bs ==blk_bs (job->common.blk)); -assert (QEMU_IS_ALIGNED (req->offset,BDRV_SECTOR_SIZE )); -assert (QEMU_IS_ALIGNED (req->bytes,BDRV_SECTOR_SIZE )); - -return backup_do_cow (job, req->offset, req->bytes,NULL , true); -} -static void backup_cleanup_sync_bitmap (BackupBlockJob *job,int ret) {BdrvDirtyBitmap *bm;
@@ -312,6 +244,8 @@ static void backup_clean(Job *job)bdrv_release_dirty_bitmap (blk_bs (s->common.blk), s->copy_bitmap); s->copy_bitmap =NULL ; } + +bdrv_fleecing_hook_drop (s->hook); }static void backup_attached_aio_context (BlockJob *job,AioContext *aio_context)
@@ -396,8 +330,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)if (yield_and_check (job)) { goto out; } - ret =backup_do_cow (job, offset, - job->cluster_size, &error_is_read, false); + ret =backup_do_cow (job, offset, job->cluster_size, &error_is_read);if (ret <0 &&backup_error_action (job, error_is_read, -ret) ==BLOCK_ERROR_ACTION_REPORT ) {
@@ -441,9 +374,7 @@ static int coroutine_fn backup_run(Job *job, Error **errp)BlockDriverState *bs =blk_bs (s->common.blk);int64_t offset;int ret =0 ; - -QLIST_INIT (&s->inflight_reqs); -qemu_co_rwlock_init (&s->flush_rwlock); +uint64_t fleecing_hook_progress;job_progress_set_remaining (job, s->len);
@@ -455,15 +386,12 @@ static int coroutine_fn backup_run(Job *job, Error **errp)bdrv_set_dirty_bitmap (s->copy_bitmap,0 , s->len); } - s->before_write.notify = backup_before_write_notify; -bdrv_add_before_write_notifier (bs, &s->before_write); -if (s->sync_mode ==MIRROR_SYNC_MODE_NONE ) {/* All bits are set in copy_bitmap to allow any cluster to be copied. * This does not actually require them to be copied. */ while (!job_is_cancelled (job)) { -/* Yield until the job is cancelled. We just let our before_write -* notify callback service CoW requests. */ +/* Yield until the job is cancelled. We just let our fleecing-hook +* fileter driver service CbW requests. */ job_yield (job); } }else if (s->sync_mode ==MIRROR_SYNC_MODE_INCREMENTAL ) {
@@ -514,7 +442,7 @@ static int coroutine_fn backup_run(Job *job, Error **errp) ret = alloced; }else { ret =backup_do_cow (s, offset, s->cluster_size, - &error_is_read, false); + &error_is_read); }if (ret <0 ) {/* Depending on error action, fail now or retry cluster */
@@ -530,11 +458,13 @@ static int coroutine_fn backup_run(Job *job, Error **errp) } } -notifier_with_return_remove (&s->before_write); +/* wait pending CBW operations in fleecing hook */ +bdrv_drain (s->hook); -/* wait until pending backup_do_cow() calls have completed */ -qemu_co_rwlock_wrlock (&s->flush_rwlock); -qemu_co_rwlock_unlock (&s->flush_rwlock); + fleecing_hook_progress =bdrv_fleecing_hook_progress (s->hook); +job_progress_update (job, ret + fleecing_hook_progress - + s->fleecing_hook_progress); + s->fleecing_hook_progress = fleecing_hook_progress;return ret; }
@@ -573,6 +503,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,int64_t cluster_size;BdrvDirtyBitmap *copy_bitmap =NULL ;bool copy_bitmap_created = false; +BlockDriverState *hook;assert (bs);assert (target);
@@ -669,6 +600,19 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,return NULL ; } +/* bdrv_get_device_name will not help to find device name starting from +* @bs after fleecing hook append, so let's calculate job_id before. Do +* it in the same way like block_job_create +*/ +if (job_id ==NULL && !(creation_flags &JOB_INTERNAL )) { + job_id =bdrv_get_device_name (bs); + } + + hook =bdrv_fleecing_hook_append (bs, target, x_copy_bitmap, errp); +if (!hook) { +return NULL ; + } + len =bdrv_getlength (bs);if (len <0 ) { error_setg_errno(errp, -len, "unable to get length for '%s'",
@@ -718,6 +662,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,block_job_add_bdrv (&job->common,"target" , target,0 ,BLK_PERM_ALL , &error_abort); job->len = len; + job->hook = hook;return &job->common;
@@ -733,6 +678,9 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,backup_clean (&job->common.job);job_early_fail (&job->common.job); } +if (hook) { +bdrv_fleecing_hook_drop (hook); + }return NULL ; }
Signature · 1 line
2.18.0
Re: [Qemu-devel] [PATCH v3 18/18] block/backup: use fleecing-hook instead of write notifiers
Quoted 15 lines · Vladimir Sementsov-Ogievskiy
Drop write notifiers and use filter node instead. Changes:
1. copy-before-writes now handled by filter node, so, drop all
is_write_notifierarguments.2. we don't have intersecting requests, so their handling is dropped. Instead, synchronization works as follows:
when backup or fleecing-hook starts copying of some area it firstly clears copy-bitmap bits, and nobody touches areas, not marked with dirty bits in copy-bitmap, so there no intersection. Also, read requests are marked serializing, to not interfer with guest writes and not read changed data from source (before reading we clear corresponding bit in copy-bitmap, so, this area is not more handled by fleecing-hook).
Quoted 6 lines
3. To sync with in-flight requests we no just drain hook node, we don't need rw-lock.
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>---
There is a problem in this patch: to be sure, that if backup job started to read from source, guest will not write to the area, I use
1. dirty bitmap: if it is set this means there must not be any in flight write requests on source on this region
2. serialized read: we start it, so until it finishes nobody writes
But backup may restart copy operation if it failed. And of course, second try of read may appear after guest write.. I see the following way to fix:
don't retry the whole operation, but only read. Then all there reads will be serialized and no yield point between them, so, guest can't write until we finish..
in the same time, retrying of copy_range() operation should be OK as is, as it keeps read serialized requests during the whole operation.
so, a kind of refactoring needed here.
Or, is it better to create external interface for creating serialized requests? something like bdrv_co_pblockwrite, which will block writes on this region, by creating special serialized request, until paired bdrv_co_punblockwrite()?
Signature · 2 lines
Best regards, Vladimir
Quoted 9 lines · Vladimir Sementsov-Ogievskiy
v2 was "[RFC v2] new, node-graph-based fleecing and backup"
Hi all!
These series introduce fleecing-hook driver. It's a filter-node, which do copy-before-write operation. Mirror uses filter-node for handling guest writes, let's move to filter-node (from write-notifiers) for backup too (patch 18)
Quoted 4 lines
Proposed filter driver is complete and separate: it can be used standalone, as fleecing provider (instead of backup(sync=none)). (old-style fleecing based on backup(sync=none) is supported too), look at patch 16.
I haven't had time to look at this series in any sort of depth yet, but it reminds me of a question I just ran into with my libvirt code:
What happens if we want to have two parallel clients both reading off different backup/fleece nodes at once? Right now, 'nbd-server-start' is hard-coded to at most one NBD server, and 'nbd-server-add' is hardcoded to adding an export to the one-and-only NBD server. But it would be a lot nicer if you could pick different ports for different clients (or even mix TCP and Unix sockets), so that independent backup jobs can both operate in parallel via different NBD servers both under control of the same qemu process, instead of the second client having to wait for the first client to disconnect so that the first NBD server can stop. In the meantime, you can be somewhat careful by controlling which export names are exposed over NBD, but even with nbd-server-start using "tls-creds", all clients can see one another's exports via NBD_OPT_LIST, and you are relying on the clients being well-behaved, vs. the nicer ability to spawn multiple NBD servers, then control which exports are exposed over which servers, and where distinct servers could even have different tls-creds.
To get to that point, we'd need to enhance nbd-server-start to return a server id, and allow nbd-server-add and friends to take an optional parameter of a server id (for back-compat, if the server id is not provided, it operates on the first one).
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Quoted 35 lines · Eric Blake
Quoted 9 lines · Vladimir Sementsov-Ogievskiy
v2 was "[RFC v2] new, node-graph-based fleecing and backup"
Hi all!
These series introduce fleecing-hook driver. It's a filter-node, which do copy-before-write operation. Mirror uses filter-node for handling guest writes, let's move to filter-node (from write-notifiers) for backup too (patch 18)
Quoted 4 lines
Proposed filter driver is complete and separate: it can be used standalone, as fleecing provider (instead of backup(sync=none)). (old-style fleecing based on backup(sync=none) is supported too), look at patch 16.
I haven't had time to look at this series in any sort of depth yet, but it reminds me of a question I just ran into with my libvirt code:
What happens if we want to have two parallel clients both reading off different backup/fleece nodes at once? Right now, 'nbd-server-start' is hard-coded to at most one NBD server, and 'nbd-server-add' is hardcoded to adding an export to the one-and-only NBD server. But it would be a lot nicer if you could pick different ports for different clients (or even mix TCP and Unix sockets), so that independent backup jobs can both operate in parallel via different NBD servers both under control of the same qemu process, instead of the second client having to wait for the first client to disconnect so that the first NBD server can stop. In the meantime, you can be somewhat careful by controlling which export names are exposed over NBD, but even with nbd-server-start using "tls-creds", all clients can see one another's exports via
NBD_OPT_LIST, and you are relying on the clients being well-behaved, vs. the nicer ability to spawn multiple NBD servers, then control which exports are exposed over which servers, and where distinct servers could even have different tls-creds.
Quoted 5 lines
To get to that point, we'd need to enhance nbd-server-start to return a server id, and allow nbd-server-add and friends to take an optional parameter of a server id (for back-compat, if the server id is not provided, it operates on the first one).
Good thing.
Don't see any problems from block layer: if we want to export the same fleecing through several different servers, they all can share the same fleecing node.
However, with new approach, we even can setup several fleecing nodes for one active disk.. any benefits? For example we can start second external backup on the same disk, when the first one is still in progress.. Don't sure that's a real case.
Signature · 2 lines
Best regards, Vladimir
Quoted 35 lines · Eric Blake
Quoted 9 lines · Vladimir Sementsov-Ogievskiy
v2 was "[RFC v2] new, node-graph-based fleecing and backup"
Hi all!
These series introduce fleecing-hook driver. It's a filter-node, which do copy-before-write operation. Mirror uses filter-node for handling guest writes, let's move to filter-node (from write-notifiers) for backup too (patch 18)
Quoted 4 lines
Proposed filter driver is complete and separate: it can be used standalone, as fleecing provider (instead of backup(sync=none)). (old-style fleecing based on backup(sync=none) is supported too), look at patch 16.
I haven't had time to look at this series in any sort of depth yet, but it reminds me of a question I just ran into with my libvirt code:
What happens if we want to have two parallel clients both reading off different backup/fleece nodes at once? Right now, 'nbd-server-start' is hard-coded to at most one NBD server, and 'nbd-server-add' is hardcoded to adding an export to the one-and-only NBD server. But it would be a lot nicer if you could pick different ports for different clients (or even mix TCP and Unix sockets), so that independent backup jobs can both operate in parallel via different NBD servers both under control of the same qemu process, instead of the second client having to wait for the first client to disconnect so that the first NBD server can stop. In the meantime, you can be somewhat careful by controlling which export names are exposed over NBD, but even with nbd-server-start using "tls-creds", all clients can see one another's exports via
NBD_OPT_LIST, and you are relying on the clients being well-behaved, vs. the nicer ability to spawn multiple NBD servers, then control which exports are exposed over which servers, and where distinct servers could even have different tls-creds.
Quoted 5 lines
To get to that point, we'd need to enhance nbd-server-start to return a server id, and allow nbd-server-add and friends to take an optional parameter of a server id (for back-compat, if the server id is not provided, it operates on the first one).
hmm, about different ports, it's funny, but NBD-spec is directly against different ports for new style negotiation:
A client who wants to use the new style negotiation SHOULD connect on the IANA-reserved port for NBD, 10809. The server MAY listen on other ports as well, but it SHOULD use the old style handshake on those.
also, next sentence is strange too:
The server SHOULD refuse to allow oldstyle negotiations on the newstyle port.
refuse? Refuse to whom? It's a server, who choose negotiation type. Or this means, that it should refuse to start at all. so refuse to server admin, not to NBD client? sounds strange. Should it be "Server SHOULD NOT use the old style on port 10809"?
Signature · 2 lines
Best regards, Vladimir