From 21ed8e10c0c6bee017472788a521ef2baa2ea0cc Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Wed, 17 Jul 2019 08:53:41 +0800 Subject: [PATCH 01/23] migration: use migration_is_active to represent active state Wrap the check into a function to make it easy to read. Signed-off-by: Wei Yang Message-Id: <20190717005341.14140-1-richardw.yang@linux.intel.com> Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Dr. David Alan Gilbert --- ...gration_is_active-to-represent-activ.patch | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 migration-use-migration_is_active-to-represent-activ.patch diff --git a/migration-use-migration_is_active-to-represent-activ.patch b/migration-use-migration_is_active-to-represent-activ.patch new file mode 100644 index 0000000..c9e926a --- /dev/null +++ b/migration-use-migration_is_active-to-represent-activ.patch @@ -0,0 +1,68 @@ +From 9662d44633dd4582dc47d58f63ee63b2c8f60a4f Mon Sep 17 00:00:00 2001 +From: Wei Yang +Date: Wed, 17 Jul 2019 08:53:41 +0800 +Subject: [PATCH] migration: use migration_is_active to represent active state + +Wrap the check into a function to make it easy to read. + +Signed-off-by: Wei Yang +Message-Id: <20190717005341.14140-1-richardw.yang@linux.intel.com> +Reviewed-by: Dr. David Alan Gilbert +Signed-off-by: Dr. David Alan Gilbert +--- + include/migration/misc.h | 1 + + migration/migration.c | 12 ++++++++---- + 2 files changed, 9 insertions(+), 4 deletions(-) + +diff --git a/include/migration/misc.h b/include/migration/misc.h +index 5cdbabd094..42d6abc920 100644 +--- a/include/migration/misc.h ++++ b/include/migration/misc.h +@@ -61,6 +61,7 @@ void migration_object_init(void); + void migration_shutdown(void); + void qemu_start_incoming_migration(const char *uri, Error **errp); + bool migration_is_idle(void); ++bool migration_is_active(MigrationState *); + void add_migration_state_change_notifier(Notifier *notify); + void remove_migration_state_change_notifier(Notifier *notify); + bool migration_in_setup(MigrationState *); +diff --git a/migration/migration.c b/migration/migration.c +index 9b40380d7c..fd7d81d4b6 100644 +--- a/migration/migration.c ++++ b/migration/migration.c +@@ -1578,8 +1578,7 @@ static void migrate_fd_cleanup(MigrationState *s) + qemu_fclose(tmp); + } + +- assert((s->state != MIGRATION_STATUS_ACTIVE) && +- (s->state != MIGRATION_STATUS_POSTCOPY_ACTIVE)); ++ assert(!migration_is_active(s)); + + if (s->state == MIGRATION_STATUS_CANCELLING) { + migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING, +@@ -1741,6 +1740,12 @@ bool migration_is_idle(void) + return false; + } + ++bool migration_is_active(MigrationState *s) ++{ ++ return (s->state == MIGRATION_STATUS_ACTIVE || ++ s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE); ++} ++ + void migrate_init(MigrationState *s) + { + /* +@@ -3307,8 +3312,7 @@ static void *migration_thread(void *opaque) + + trace_migration_thread_setup_complete(); + +- while (s->state == MIGRATION_STATUS_ACTIVE || +- s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) { ++ while (migration_is_active(s)) { + int64_t current_time; + + if (urgent || !qemu_file_rate_limit(s->to_dst_file)) { +-- +2.27.0 + -- Gitee From 4706000a5eb92d485d4d732c1ed6342f9af9570a Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Tue, 17 Mar 2020 17:05:18 +0000 Subject: [PATCH 02/23] migration: Rate limit inside host pages RH-Author: Laurent Vivier Message-id: <20200317170518.9303-1-lvivier@redhat.com> Patchwork-id: 94374 O-Subject: [RHEL-AV-8.2.0 qemu-kvm PATCH] migration: Rate limit inside host pages Bugzilla: 1814336 RH-Acked-by: Peter Xu RH-Acked-by: Juan Quintela RH-Acked-by: Dr. David Alan Gilbert From: "Dr. David Alan Gilbert" When using hugepages, rate limiting is necessary within each huge page, since a 1G huge page can take a significant time to send, so you end up with bursty behaviour. Fixes: 4c011c37ecb3 ("postcopy: Send whole huge pages") Reported-by: Lin Ma Signed-off-by: Dr. David Alan Gilbert Reviewed-by: Juan Quintela Reviewed-by: Peter Xu Signed-off-by: Juan Quintela (cherry picked from commit 97e1e06780e70f6e98a0d2df881e0c0927d3aeb6) Signed-off-by: Laurent Vivier BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1814336 BRANCH: rhel-av-8.2.0 UPSTREAM: Merged BREW: https://brewweb.engineering.redhat.com/brew/taskinfo?taskID=27283241 TESTED: Tested that the migration abort doesn't trigger an error message in the kernel logs on P9 Signed-off-by: Danilo C. L. de Paula --- migration-Rate-limit-inside-host-pages.patch | 173 +++++++++++++++++++ 1 file changed, 173 insertions(+) create mode 100644 migration-Rate-limit-inside-host-pages.patch diff --git a/migration-Rate-limit-inside-host-pages.patch b/migration-Rate-limit-inside-host-pages.patch new file mode 100644 index 0000000..17eb46f --- /dev/null +++ b/migration-Rate-limit-inside-host-pages.patch @@ -0,0 +1,173 @@ +From 3e8a587b055f0e3cabf91921fca0777fe7e349f5 Mon Sep 17 00:00:00 2001 +From: Laurent Vivier +Date: Tue, 17 Mar 2020 17:05:18 +0000 +Subject: [PATCH] migration: Rate limit inside host pages + +RH-Author: Laurent Vivier +Message-id: <20200317170518.9303-1-lvivier@redhat.com> +Patchwork-id: 94374 +O-Subject: [RHEL-AV-8.2.0 qemu-kvm PATCH] migration: Rate limit inside host pages +Bugzilla: 1814336 +RH-Acked-by: Peter Xu +RH-Acked-by: Juan Quintela +RH-Acked-by: Dr. David Alan Gilbert + +From: "Dr. David Alan Gilbert" + +When using hugepages, rate limiting is necessary within each huge +page, since a 1G huge page can take a significant time to send, so +you end up with bursty behaviour. + +Fixes: 4c011c37ecb3 ("postcopy: Send whole huge pages") +Reported-by: Lin Ma +Signed-off-by: Dr. David Alan Gilbert +Reviewed-by: Juan Quintela +Reviewed-by: Peter Xu +Signed-off-by: Juan Quintela +(cherry picked from commit 97e1e06780e70f6e98a0d2df881e0c0927d3aeb6) +Signed-off-by: Laurent Vivier + +BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1814336 +BRANCH: rhel-av-8.2.0 +UPSTREAM: Merged +BREW: https://brewweb.engineering.redhat.com/brew/taskinfo?taskID=27283241 +TESTED: Tested that the migration abort doesn't trigger an error message in + the kernel logs on P9 + +Signed-off-by: Danilo C. L. de Paula +--- + migration/migration.c | 57 ++++++++++++++++++++++++------------------ + migration/migration.h | 1 + + migration/ram.c | 2 ++ + migration/trace-events | 4 +-- + 4 files changed, 37 insertions(+), 27 deletions(-) + +diff --git a/migration/migration.c b/migration/migration.c +index fd7d81d4b6..b0b9430822 100644 +--- a/migration/migration.c ++++ b/migration/migration.c +@@ -3260,6 +3260,37 @@ void migration_consume_urgent_request(void) + qemu_sem_wait(&migrate_get_current()->rate_limit_sem); + } + ++/* Returns true if the rate limiting was broken by an urgent request */ ++bool migration_rate_limit(void) ++{ ++ int64_t now = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); ++ MigrationState *s = migrate_get_current(); ++ ++ bool urgent = false; ++ migration_update_counters(s, now); ++ if (qemu_file_rate_limit(s->to_dst_file)) { ++ /* ++ * Wait for a delay to do rate limiting OR ++ * something urgent to post the semaphore. ++ */ ++ int ms = s->iteration_start_time + BUFFER_DELAY - now; ++ trace_migration_rate_limit_pre(ms); ++ if (qemu_sem_timedwait(&s->rate_limit_sem, ms) == 0) { ++ /* ++ * We were woken by one or more urgent things but ++ * the timedwait will have consumed one of them. ++ * The service routine for the urgent wake will dec ++ * the semaphore itself for each item it consumes, ++ * so add this one we just eat back. ++ */ ++ qemu_sem_post(&s->rate_limit_sem); ++ urgent = true; ++ } ++ trace_migration_rate_limit_post(urgent); ++ } ++ return urgent; ++} ++ + /* + * Master migration thread on the source VM. + * It drives the migration and pumps the data down the outgoing channel. +@@ -3313,8 +3344,6 @@ static void *migration_thread(void *opaque) + trace_migration_thread_setup_complete(); + + while (migration_is_active(s)) { +- int64_t current_time; +- + if (urgent || !qemu_file_rate_limit(s->to_dst_file)) { + MigIterateState iter_state = migration_iteration_run(s); + if (iter_state == MIG_ITERATE_SKIP) { +@@ -3341,29 +3370,7 @@ static void *migration_thread(void *opaque) + update_iteration_initial_status(s); + } + +- current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); +- +- migration_update_counters(s, current_time); +- +- urgent = false; +- if (qemu_file_rate_limit(s->to_dst_file)) { +- /* Wait for a delay to do rate limiting OR +- * something urgent to post the semaphore. +- */ +- int ms = s->iteration_start_time + BUFFER_DELAY - current_time; +- trace_migration_thread_ratelimit_pre(ms); +- if (qemu_sem_timedwait(&s->rate_limit_sem, ms) == 0) { +- /* We were worken by one or more urgent things but +- * the timedwait will have consumed one of them. +- * The service routine for the urgent wake will dec +- * the semaphore itself for each item it consumes, +- * so add this one we just eat back. +- */ +- qemu_sem_post(&s->rate_limit_sem); +- urgent = true; +- } +- trace_migration_thread_ratelimit_post(urgent); +- } ++ urgent = migration_rate_limit(); + } + + trace_migration_thread_after_loop(); +diff --git a/migration/migration.h b/migration/migration.h +index 4aa72297fc..ff8a0bf12d 100644 +--- a/migration/migration.h ++++ b/migration/migration.h +@@ -345,6 +345,7 @@ int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque); + + void migration_make_urgent_request(void); + void migration_consume_urgent_request(void); ++bool migration_rate_limit(void); + + int migration_send_initial_packet(QIOChannel *c, uint8_t id, Error **errp); + int migration_recv_initial_packet(QIOChannel *c, Error **errp); +diff --git a/migration/ram.c b/migration/ram.c +index 27585a4f3e..d6657a8093 100644 +--- a/migration/ram.c ++++ b/migration/ram.c +@@ -3076,6 +3076,8 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss, + } + + pss->page++; ++ /* Allow rate limiting to happen in the middle of huge pages */ ++ migration_rate_limit(); + } while ((pss->page & (pagesize_bits - 1)) && + offset_in_ramblock(pss->block, pss->page << TARGET_PAGE_BITS)); + +diff --git a/migration/trace-events b/migration/trace-events +index c0640cd424..b4d85229d9 100644 +--- a/migration/trace-events ++++ b/migration/trace-events +@@ -131,12 +131,12 @@ migrate_send_rp_recv_bitmap(char *name, int64_t size) "block '%s' size 0x%"PRIi6 + migration_completion_file_err(void) "" + migration_completion_postcopy_end(void) "" + migration_completion_postcopy_end_after_complete(void) "" ++migration_rate_limit_pre(int ms) "%d ms" ++migration_rate_limit_post(int urgent) "urgent: %d" + migration_return_path_end_before(void) "" + migration_return_path_end_after(int rp_error) "%d" + migration_thread_after_loop(void) "" + migration_thread_file_err(void) "" +-migration_thread_ratelimit_pre(int ms) "%d ms" +-migration_thread_ratelimit_post(int urgent) "urgent: %d" + migration_thread_setup_complete(void) "" + open_return_path_on_source(void) "" + open_return_path_on_source_continue(void) "" +-- +2.27.0 + -- Gitee From 322d6152981b24523f7c1bb29feb85ebd2ff5563 Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Thu, 22 Jul 2021 21:28:32 +0800 Subject: [PATCH 03/23] spec: Update patch and changelog with !168 migration: Rate limit inside host pages !168 migration: use migration_is_active to represent active state migration: Rate limit inside host pages Signed-off-by: Chen Qun --- qemu.spec | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/qemu.spec b/qemu.spec index c02c609..f893e37 100644 --- a/qemu.spec +++ b/qemu.spec @@ -391,6 +391,8 @@ Patch0378: target-i386-enable-monitor-and-ucode-revision-with-c.patch Patch0379: target-i386-set-the-CPUID-level-to-0x14-on-old-machi.patch Patch0380: target-i386-kvm-initialize-feature-MSRs-very-early.patch Patch0381: target-i386-add-a-ucode-rev-property.patch +Patch0382: migration-use-migration_is_active-to-represent-activ.patch +Patch0383: migration-Rate-limit-inside-host-pages.patch BuildRequires: flex BuildRequires: gcc @@ -785,6 +787,10 @@ getent passwd qemu >/dev/null || \ %endif %changelog +* Thu Jul 22 2021 Chen Qun +- migration: use migration_is_active to represent active state +- migration: Rate limit inside host pages + * Thu Jul 22 2021 Chen Qun - virtio-net: delete also control queue when TX/RX deleted - target/i386: enable monitor and ucode revision with -cpu max -- Gitee From 54d6c83994bb436e9232796c582a3c9dee0ad404 Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Tue, 16 Jun 2020 12:25:36 -0400 Subject: [PATCH 04/23] hw/pci/pcie: Move hot plug capability check to pre_plug callback RH-Author: Julia Suvorova Message-id: <20200616122536.1027685-1-jusual@redhat.com> Patchwork-id: 97548 O-Subject: [RHEL-AV-8.2.1 qemu-kvm PATCH 1/1] hw/pci/pcie: Move hot plug capability check to pre_plug callback Bugzilla: 1820531 RH-Acked-by: Danilo de Paula RH-Acked-by: Auger Eric RH-Acked-by: Sergio Lopez Pascual BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1820531 BRANCH: rhel-av-8.2.1 UPSTREAM: merged BREW: 29422092 Check for hot plug capability earlier to avoid removing devices attached during the initialization process. Run qemu with an unattached drive: -drive file=$FILE,if=none,id=drive0 \ -device pcie-root-port,id=rp0,slot=3,bus=pcie.0,hotplug=off Hotplug a block device: device_add virtio-blk-pci,id=blk0,drive=drive0,bus=rp0 If hotplug fails on plug_cb, drive0 will be deleted. Fixes: 0501e1aa1d32a6 ("hw/pci/pcie: Forbid hot-plug if it's disabled on the slot") Acked-by: Igor Mammedov Signed-off-by: Julia Suvorova Message-Id: <20200604125947.881210-1-jusual@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin (cherry picked from commit 0dabc0f6544f2c0310546f6d6cf3b68979580a9c) Signed-off-by: Eduardo Lima (Etrunko) --- ...-hot-plug-capability-check-to-pre_pl.patch | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 hw-pci-pcie-Move-hot-plug-capability-check-to-pre_pl.patch diff --git a/hw-pci-pcie-Move-hot-plug-capability-check-to-pre_pl.patch b/hw-pci-pcie-Move-hot-plug-capability-check-to-pre_pl.patch new file mode 100644 index 0000000..e2f772c --- /dev/null +++ b/hw-pci-pcie-Move-hot-plug-capability-check-to-pre_pl.patch @@ -0,0 +1,68 @@ +From 86f70ed090478cc3b569b3606eb2723a0baadb52 Mon Sep 17 00:00:00 2001 +From: Julia Suvorova +Date: Tue, 16 Jun 2020 12:25:36 -0400 +Subject: [PATCH] hw/pci/pcie: Move hot plug capability check to pre_plug + callback + +RH-Author: Julia Suvorova +Message-id: <20200616122536.1027685-1-jusual@redhat.com> +Patchwork-id: 97548 +O-Subject: [RHEL-AV-8.2.1 qemu-kvm PATCH 1/1] hw/pci/pcie: Move hot plug capability check to pre_plug callback +Bugzilla: 1820531 +RH-Acked-by: Danilo de Paula +RH-Acked-by: Auger Eric +RH-Acked-by: Sergio Lopez Pascual + +BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1820531 +BRANCH: rhel-av-8.2.1 +UPSTREAM: merged +BREW: 29422092 + +Check for hot plug capability earlier to avoid removing devices attached +during the initialization process. + +Run qemu with an unattached drive: + -drive file=$FILE,if=none,id=drive0 \ + -device pcie-root-port,id=rp0,slot=3,bus=pcie.0,hotplug=off +Hotplug a block device: + device_add virtio-blk-pci,id=blk0,drive=drive0,bus=rp0 +If hotplug fails on plug_cb, drive0 will be deleted. + +Fixes: 0501e1aa1d32a6 ("hw/pci/pcie: Forbid hot-plug if it's disabled on the slot") + +Acked-by: Igor Mammedov +Signed-off-by: Julia Suvorova +Message-Id: <20200604125947.881210-1-jusual@redhat.com> +Reviewed-by: Michael S. Tsirkin +Signed-off-by: Michael S. Tsirkin +(cherry picked from commit 0dabc0f6544f2c0310546f6d6cf3b68979580a9c) +Signed-off-by: Eduardo Lima (Etrunko) +--- + hw/pci/pcie.c | 11 +++++++++++ + 1 file changed, 11 insertions(+) + +diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c +index 2b4eedd2bb..b5190a3a55 100644 +--- a/hw/pci/pcie.c ++++ b/hw/pci/pcie.c +@@ -419,6 +419,17 @@ static void pcie_cap_slot_plug_common(PCIDevice *hotplug_dev, DeviceState *dev, + void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, + Error **errp) + { ++ PCIDevice *hotplug_pdev = PCI_DEVICE(hotplug_dev); ++ uint8_t *exp_cap = hotplug_pdev->config + hotplug_pdev->exp.exp_cap; ++ uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP); ++ ++ /* Check if hot-plug is disabled on the slot */ ++ if (dev->hotplugged && (sltcap & PCI_EXP_SLTCAP_HPC) == 0) { ++ error_setg(errp, "Hot-plug failed: unsupported by the port device '%s'", ++ DEVICE(hotplug_pdev)->id); ++ return; ++ } ++ + pcie_cap_slot_plug_common(PCI_DEVICE(hotplug_dev), dev, errp); + } + +-- +2.27.0 + -- Gitee From 50771bbc53e4c607320e30d6b57d92c6d3e1c94d Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Thu, 22 Jul 2021 21:28:33 +0800 Subject: [PATCH 05/23] spec: Update patch and changelog with !169 bugfix: Move hot plug capability check to pre_plug callback !169 hw/pci/pcie: Move hot plug capability check to pre_plug callback Signed-off-by: Chen Qun --- qemu.spec | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/qemu.spec b/qemu.spec index f893e37..a393413 100644 --- a/qemu.spec +++ b/qemu.spec @@ -393,6 +393,7 @@ Patch0380: target-i386-kvm-initialize-feature-MSRs-very-early.patch Patch0381: target-i386-add-a-ucode-rev-property.patch Patch0382: migration-use-migration_is_active-to-represent-activ.patch Patch0383: migration-Rate-limit-inside-host-pages.patch +Patch0384: hw-pci-pcie-Move-hot-plug-capability-check-to-pre_pl.patch BuildRequires: flex BuildRequires: gcc @@ -787,6 +788,9 @@ getent passwd qemu >/dev/null || \ %endif %changelog +* Thu Jul 22 2021 Chen Qun +- hw/pci/pcie: Move hot plug capability check to pre_plug callback + * Thu Jul 22 2021 Chen Qun - migration: use migration_is_active to represent active state - migration: Rate limit inside host pages -- Gitee From 9d7ad585b25201313df24e4fea4391563844a975 Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Mon, 29 Jul 2019 16:35:52 -0400 Subject: [PATCH 06/23] qapi/block-core: Introduce BackupCommon drive-backup and blockdev-backup have an awful lot of things in common that are the same. Let's fix that. I don't deduplicate 'target', because the semantics actually did change between each structure. Leave that one alone so it can be documented separately. Where documentation was not identical, use the most up-to-date version. For "speed", use Blockdev-Backup's version. For "sync", use Drive-Backup's version. Signed-off-by: John Snow Reviewed-by: Max Reitz [Maintainer edit: modified commit message. --js] Reviewed-by: Markus Armbruster Message-id: 20190709232550.10724-2-jsnow@redhat.com Signed-off-by: John Snow --- qapi-block-core-Introduce-BackupCommon.patch | 171 +++++++++++++++++++ 1 file changed, 171 insertions(+) create mode 100644 qapi-block-core-Introduce-BackupCommon.patch diff --git a/qapi-block-core-Introduce-BackupCommon.patch b/qapi-block-core-Introduce-BackupCommon.patch new file mode 100644 index 0000000..2d16074 --- /dev/null +++ b/qapi-block-core-Introduce-BackupCommon.patch @@ -0,0 +1,171 @@ +From 2204b4839fb90658e13ddc608df7b35ed1ea9fd0 Mon Sep 17 00:00:00 2001 +From: John Snow +Date: Mon, 29 Jul 2019 16:35:52 -0400 +Subject: [PATCH] qapi/block-core: Introduce BackupCommon + +drive-backup and blockdev-backup have an awful lot of things in common +that are the same. Let's fix that. + +I don't deduplicate 'target', because the semantics actually did change +between each structure. Leave that one alone so it can be documented +separately. + +Where documentation was not identical, use the most up-to-date version. +For "speed", use Blockdev-Backup's version. For "sync", use +Drive-Backup's version. + +Signed-off-by: John Snow +Reviewed-by: Max Reitz +[Maintainer edit: modified commit message. --js] +Reviewed-by: Markus Armbruster +Message-id: 20190709232550.10724-2-jsnow@redhat.com +Signed-off-by: John Snow +--- + qapi/block-core.json | 95 ++++++++++++++------------------------------ + 1 file changed, 29 insertions(+), 66 deletions(-) + +diff --git a/qapi/block-core.json b/qapi/block-core.json +index db24f0dfe5..37aa1b7b9a 100644 +--- a/qapi/block-core.json ++++ b/qapi/block-core.json +@@ -1315,32 +1315,23 @@ + 'data': { 'node': 'str', 'overlay': 'str' } } + + ## +-# @DriveBackup: ++# @BackupCommon: + # + # @job-id: identifier for the newly-created block job. If + # omitted, the device name will be used. (Since 2.7) + # + # @device: the device name or node-name of a root node which should be copied. + # +-# @target: the target of the new image. If the file exists, or if it +-# is a device, the existing file/device will be used as the new +-# destination. If it does not exist, a new file will be created. +-# +-# @format: the format of the new destination, default is to +-# probe if @mode is 'existing', else the format of the source +-# + # @sync: what parts of the disk image should be copied to the destination + # (all the disk, only the sectors allocated in the topmost image, from a + # dirty bitmap, or only new I/O). + # +-# @mode: whether and how QEMU should create a new image, default is +-# 'absolute-paths'. +-# +-# @speed: the maximum speed, in bytes per second ++# @speed: the maximum speed, in bytes per second. The default is 0, ++# for unlimited. + # + # @bitmap: the name of dirty bitmap if sync is "incremental". + # Must be present if sync is "incremental", must NOT be present +-# otherwise. (Since 2.4) ++# otherwise. (Since 2.4 (drive-backup), 3.1 (blockdev-backup)) + # + # @compress: true to compress data, if the target format supports it. + # (default: false) (since 2.8) +@@ -1370,75 +1361,47 @@ + # I/O. If an error occurs during a guest write request, the device's + # rerror/werror actions will be used. + # +-# Since: 1.6 ++# Since: 4.2 + ## +-{ 'struct': 'DriveBackup', +- 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str', +- '*format': 'str', 'sync': 'MirrorSyncMode', +- '*mode': 'NewImageMode', '*speed': 'int', ++{ 'struct': 'BackupCommon', ++ 'data': { '*job-id': 'str', 'device': 'str', ++ 'sync': 'MirrorSyncMode', '*speed': 'int', + '*bitmap': 'str', '*compress': 'bool', + '*on-source-error': 'BlockdevOnError', + '*on-target-error': 'BlockdevOnError', + '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } } + + ## +-# @BlockdevBackup: +-# +-# @job-id: identifier for the newly-created block job. If +-# omitted, the device name will be used. (Since 2.7) +-# +-# @device: the device name or node-name of a root node which should be copied. +-# +-# @target: the device name or node-name of the backup target node. +-# +-# @sync: what parts of the disk image should be copied to the destination +-# (all the disk, only the sectors allocated in the topmost image, or +-# only new I/O). +-# +-# @speed: the maximum speed, in bytes per second. The default is 0, +-# for unlimited. +-# +-# @bitmap: the name of dirty bitmap if sync is "incremental". +-# Must be present if sync is "incremental", must NOT be present +-# otherwise. (Since 3.1) +-# +-# @compress: true to compress data, if the target format supports it. +-# (default: false) (since 2.8) ++# @DriveBackup: + # +-# @on-source-error: the action to take on an error on the source, +-# default 'report'. 'stop' and 'enospc' can only be used +-# if the block device supports io-status (see BlockInfo). ++# @target: the target of the new image. If the file exists, or if it ++# is a device, the existing file/device will be used as the new ++# destination. If it does not exist, a new file will be created. + # +-# @on-target-error: the action to take on an error on the target, +-# default 'report' (no limitations, since this applies to +-# a different block device than @device). ++# @format: the format of the new destination, default is to ++# probe if @mode is 'existing', else the format of the source + # +-# @auto-finalize: When false, this job will wait in a PENDING state after it has +-# finished its work, waiting for @block-job-finalize before +-# making any block graph changes. +-# When true, this job will automatically +-# perform its abort or commit actions. +-# Defaults to true. (Since 2.12) ++# @mode: whether and how QEMU should create a new image, default is ++# 'absolute-paths'. + # +-# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it +-# has completely ceased all work, and awaits @block-job-dismiss. +-# When true, this job will automatically disappear from the query +-# list without user intervention. +-# Defaults to true. (Since 2.12) ++# Since: 1.6 ++## ++{ 'struct': 'DriveBackup', ++ 'base': 'BackupCommon', ++ 'data': { 'target': 'str', ++ '*format': 'str', ++ '*mode': 'NewImageMode' } } ++ ++## ++# @BlockdevBackup: + # +-# 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. ++# @target: the device name or node-name of the backup target node. + # + # Since: 2.3 + ## + { 'struct': 'BlockdevBackup', +- 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str', +- 'sync': 'MirrorSyncMode', '*speed': 'int', +- '*bitmap': 'str', '*compress': 'bool', +- '*on-source-error': 'BlockdevOnError', +- '*on-target-error': 'BlockdevOnError', +- '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } } ++ 'base': 'BackupCommon', ++ 'data': { 'target': 'str' } } + + ## + # @blockdev-snapshot-sync: +-- +2.27.0 + -- Gitee From 458a9d4b6854142351b65e54ab6481c2a7ec27bd Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Mon, 29 Jul 2019 16:35:52 -0400 Subject: [PATCH 07/23] drive-backup: create do_backup_common Create a common core that comprises the actual meat of what the backup API boundary needs to do, and then switch drive-backup to use it. Signed-off-by: John Snow Reviewed-by: Max Reitz Message-id: 20190709232550.10724-3-jsnow@redhat.com Signed-off-by: John Snow --- drive-backup-create-do_backup_common.patch | 163 +++++++++++++++++++++ 1 file changed, 163 insertions(+) create mode 100644 drive-backup-create-do_backup_common.patch diff --git a/drive-backup-create-do_backup_common.patch b/drive-backup-create-do_backup_common.patch new file mode 100644 index 0000000..cccbc2e --- /dev/null +++ b/drive-backup-create-do_backup_common.patch @@ -0,0 +1,163 @@ +From 98dcfbd5ee53f3be705df7acf37e8706533f494f Mon Sep 17 00:00:00 2001 +From: John Snow +Date: Mon, 29 Jul 2019 16:35:52 -0400 +Subject: [PATCH] drive-backup: create do_backup_common + +Create a common core that comprises the actual meat of what the backup API +boundary needs to do, and then switch drive-backup to use it. + +Signed-off-by: John Snow +Reviewed-by: Max Reitz +Message-id: 20190709232550.10724-3-jsnow@redhat.com +Signed-off-by: John Snow +--- + blockdev.c | 102 ++++++++++++++++++++++++++++++----------------------- + 1 file changed, 57 insertions(+), 45 deletions(-) + +diff --git a/blockdev.c b/blockdev.c +index 99c92b96d2..a29838a1c8 100644 +--- a/blockdev.c ++++ b/blockdev.c +@@ -3469,20 +3469,16 @@ out: + aio_context_release(aio_context); + } + +-static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, +- Error **errp) ++/* Common QMP interface for drive-backup and blockdev-backup */ ++static BlockJob *do_backup_common(BackupCommon *backup, ++ BlockDriverState *bs, ++ BlockDriverState *target_bs, ++ AioContext *aio_context, ++ JobTxn *txn, Error **errp) + { +- BlockDriverState *bs; +- BlockDriverState *target_bs; +- BlockDriverState *source = NULL; + BlockJob *job = NULL; + BdrvDirtyBitmap *bmap = NULL; +- AioContext *aio_context; +- QDict *options = NULL; +- Error *local_err = NULL; +- int flags, job_flags = JOB_DEFAULT; +- int64_t size; +- bool set_backing_hd = false; ++ int job_flags = JOB_DEFAULT; + int ret; + + if (!backup->has_speed) { +@@ -3494,9 +3490,6 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, + if (!backup->has_on_target_error) { + backup->on_target_error = BLOCKDEV_ON_ERROR_REPORT; + } +- if (!backup->has_mode) { +- backup->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS; +- } + if (!backup->has_job_id) { + backup->job_id = NULL; + } +@@ -3510,6 +3503,54 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, + backup->compress = false; + } + ++ ret = bdrv_try_set_aio_context(target_bs, aio_context, errp); ++ if (ret < 0) { ++ return NULL; ++ } ++ ++ if (backup->has_bitmap) { ++ bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap); ++ if (!bmap) { ++ error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap); ++ return NULL; ++ } ++ if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_DEFAULT, errp)) { ++ return NULL; ++ } ++ } ++ ++ if (!backup->auto_finalize) { ++ job_flags |= JOB_MANUAL_FINALIZE; ++ } ++ if (!backup->auto_dismiss) { ++ job_flags |= JOB_MANUAL_DISMISS; ++ } ++ ++ 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, errp); ++ return job; ++} ++ ++static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, ++ Error **errp) ++{ ++ BlockDriverState *bs; ++ BlockDriverState *target_bs; ++ BlockDriverState *source = NULL; ++ BlockJob *job = NULL; ++ AioContext *aio_context; ++ QDict *options = NULL; ++ Error *local_err = NULL; ++ int flags; ++ int64_t size; ++ bool set_backing_hd = false; ++ ++ if (!backup->has_mode) { ++ backup->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS; ++ } ++ + bs = bdrv_lookup_bs(backup->device, backup->device, errp); + if (!bs) { + return NULL; +@@ -3585,12 +3626,6 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, + goto out; + } + +- ret = bdrv_try_set_aio_context(target_bs, aio_context, errp); +- if (ret < 0) { +- bdrv_unref(target_bs); +- goto out; +- } +- + if (set_backing_hd) { + bdrv_set_backing_hd(target_bs, source, &local_err); + if (local_err) { +@@ -3598,31 +3633,8 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, + } + } + +- if (backup->has_bitmap) { +- bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap); +- if (!bmap) { +- error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap); +- goto unref; +- } +- if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_DEFAULT, errp)) { +- goto unref; +- } +- } +- if (!backup->auto_finalize) { +- job_flags |= JOB_MANUAL_FINALIZE; +- } +- if (!backup->auto_dismiss) { +- job_flags |= JOB_MANUAL_DISMISS; +- } +- +- 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); +- if (local_err != NULL) { +- error_propagate(errp, local_err); +- goto unref; +- } ++ job = do_backup_common(qapi_DriveBackup_base(backup), ++ bs, target_bs, aio_context, txn, errp); + + unref: + bdrv_unref(target_bs); +-- +2.27.0 + -- Gitee From 9d74b7ea25e4508450596f9712d578a5fdb3cecd Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Mon, 29 Jul 2019 16:35:52 -0400 Subject: [PATCH 08/23] blockdev-backup: utilize do_backup_common Signed-off-by: John Snow Reviewed-by: Max Reitz Message-id: 20190709232550.10724-4-jsnow@redhat.com Signed-off-by: John Snow --- ...kdev-backup-utilize-do_backup_common.patch | 105 ++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 blockdev-backup-utilize-do_backup_common.patch diff --git a/blockdev-backup-utilize-do_backup_common.patch b/blockdev-backup-utilize-do_backup_common.patch new file mode 100644 index 0000000..6827b22 --- /dev/null +++ b/blockdev-backup-utilize-do_backup_common.patch @@ -0,0 +1,105 @@ +From e5456acf2332efd0ed6106eb13cf24e6bca1ee64 Mon Sep 17 00:00:00 2001 +From: John Snow +Date: Mon, 29 Jul 2019 16:35:52 -0400 +Subject: [PATCH] blockdev-backup: utilize do_backup_common + +Signed-off-by: John Snow +Reviewed-by: Max Reitz +Message-id: 20190709232550.10724-4-jsnow@redhat.com +Signed-off-by: John Snow +--- + blockdev.c | 65 +++++------------------------------------------------- + 1 file changed, 6 insertions(+), 59 deletions(-) + +diff --git a/blockdev.c b/blockdev.c +index a29838a1c8..aa15ed1f00 100644 +--- a/blockdev.c ++++ b/blockdev.c +@@ -3668,78 +3668,25 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn, + { + BlockDriverState *bs; + BlockDriverState *target_bs; +- Error *local_err = NULL; +- BdrvDirtyBitmap *bmap = NULL; + AioContext *aio_context; +- BlockJob *job = NULL; +- int job_flags = JOB_DEFAULT; +- int ret; +- +- if (!backup->has_speed) { +- backup->speed = 0; +- } +- if (!backup->has_on_source_error) { +- backup->on_source_error = BLOCKDEV_ON_ERROR_REPORT; +- } +- if (!backup->has_on_target_error) { +- backup->on_target_error = BLOCKDEV_ON_ERROR_REPORT; +- } +- if (!backup->has_job_id) { +- backup->job_id = NULL; +- } +- if (!backup->has_auto_finalize) { +- backup->auto_finalize = true; +- } +- if (!backup->has_auto_dismiss) { +- backup->auto_dismiss = true; +- } +- if (!backup->has_compress) { +- backup->compress = false; +- } ++ BlockJob *job; + + bs = bdrv_lookup_bs(backup->device, backup->device, errp); + if (!bs) { + return NULL; + } + +- aio_context = bdrv_get_aio_context(bs); +- aio_context_acquire(aio_context); +- + target_bs = bdrv_lookup_bs(backup->target, backup->target, errp); + if (!target_bs) { +- goto out; ++ return NULL; + } + +- ret = bdrv_try_set_aio_context(target_bs, aio_context, errp); +- if (ret < 0) { +- goto out; +- } ++ aio_context = bdrv_get_aio_context(bs); ++ aio_context_acquire(aio_context); + +- if (backup->has_bitmap) { +- bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap); +- if (!bmap) { +- error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap); +- goto out; +- } +- if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_DEFAULT, errp)) { +- goto out; +- } +- } ++ job = do_backup_common(qapi_BlockdevBackup_base(backup), ++ bs, target_bs, aio_context, txn, errp); + +- if (!backup->auto_finalize) { +- job_flags |= JOB_MANUAL_FINALIZE; +- } +- if (!backup->auto_dismiss) { +- job_flags |= JOB_MANUAL_DISMISS; +- } +- 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); +- if (local_err != NULL) { +- error_propagate(errp, local_err); +- } +-out: + aio_context_release(aio_context); + return job; + } +-- +2.27.0 + -- Gitee From dd816556ff3ddb98d11ff941fca2a1f8185cd3c0 Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Mon, 29 Jul 2019 16:35:52 -0400 Subject: [PATCH 09/23] qapi: add BitmapSyncMode enum Depending on what a user is trying to accomplish, there might be a few bitmap cleanup actions that occur when an operation is finished that could be useful. I am proposing three: - NEVER: The bitmap is never synchronized against what was copied. - ALWAYS: The bitmap is always synchronized, even on failures. - ON-SUCCESS: The bitmap is synchronized only on success. The existing incremental backup modes use 'on-success' semantics, so add just that one for right now. Signed-off-by: John Snow Reviewed-by: Max Reitz Reviewed-by: Markus Armbruster Message-id: 20190709232550.10724-5-jsnow@redhat.com Signed-off-by: John Snow --- qapi-add-BitmapSyncMode-enum.patch | 54 ++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 qapi-add-BitmapSyncMode-enum.patch diff --git a/qapi-add-BitmapSyncMode-enum.patch b/qapi-add-BitmapSyncMode-enum.patch new file mode 100644 index 0000000..778faee --- /dev/null +++ b/qapi-add-BitmapSyncMode-enum.patch @@ -0,0 +1,54 @@ +From bd1d5d79f4629520d0753676cea8129c60fc6bbc Mon Sep 17 00:00:00 2001 +From: John Snow +Date: Mon, 29 Jul 2019 16:35:52 -0400 +Subject: [PATCH] qapi: add BitmapSyncMode enum + +Depending on what a user is trying to accomplish, there might be a few +bitmap cleanup actions that occur when an operation is finished that +could be useful. + +I am proposing three: +- NEVER: The bitmap is never synchronized against what was copied. +- ALWAYS: The bitmap is always synchronized, even on failures. +- ON-SUCCESS: The bitmap is synchronized only on success. + +The existing incremental backup modes use 'on-success' semantics, +so add just that one for right now. + +Signed-off-by: John Snow +Reviewed-by: Max Reitz +Reviewed-by: Markus Armbruster +Message-id: 20190709232550.10724-5-jsnow@redhat.com +Signed-off-by: John Snow +--- + qapi/block-core.json | 14 ++++++++++++++ + 1 file changed, 14 insertions(+) + +diff --git a/qapi/block-core.json b/qapi/block-core.json +index 37aa1b7b9a..b8d12a4951 100644 +--- a/qapi/block-core.json ++++ b/qapi/block-core.json +@@ -1134,6 +1134,20 @@ + { 'enum': 'MirrorSyncMode', + 'data': ['top', 'full', 'none', 'incremental'] } + ++## ++# @BitmapSyncMode: ++# ++# An enumeration of possible behaviors for the synchronization of a bitmap ++# when used for data copy operations. ++# ++# @on-success: The bitmap is only synced when the operation is successful. ++# This is the behavior always used for 'INCREMENTAL' backups. ++# ++# Since: 4.2 ++## ++{ 'enum': 'BitmapSyncMode', ++ 'data': ['on-success'] } ++ + ## + # @MirrorCopyMode: + # +-- +2.27.0 + -- Gitee From a5b372dbc6b2796d7594edce64bb9338dba8f3d4 Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Mon, 29 Jul 2019 16:35:52 -0400 Subject: [PATCH 10/23] block/backup: Add mirror sync mode 'bitmap' We don't need or want a new sync mode for simple differences in semantics. Create a new mode simply named "BITMAP" that is designed to make use of the new Bitmap Sync Mode field. Because the only bitmap sync mode is 'on-success', this adds no new functionality to the backup job (yet). The old incremental backup mode is maintained as a syntactic sugar for sync=bitmap, mode=on-success. Add all of the plumbing necessary to support this new instruction. Signed-off-by: John Snow Reviewed-by: Max Reitz Message-id: 20190709232550.10724-6-jsnow@redhat.com Signed-off-by: John Snow --- ...k-backup-Add-mirror-sync-mode-bitmap.patch | 252 ++++++++++++++++++ 1 file changed, 252 insertions(+) create mode 100644 block-backup-Add-mirror-sync-mode-bitmap.patch diff --git a/block-backup-Add-mirror-sync-mode-bitmap.patch b/block-backup-Add-mirror-sync-mode-bitmap.patch new file mode 100644 index 0000000..fb11120 --- /dev/null +++ b/block-backup-Add-mirror-sync-mode-bitmap.patch @@ -0,0 +1,252 @@ +From e0a0150e671e8129f11aa3df907e444e91711f53 Mon Sep 17 00:00:00 2001 +From: John Snow +Date: Mon, 29 Jul 2019 16:35:52 -0400 +Subject: [PATCH] block/backup: Add mirror sync mode 'bitmap' + +We don't need or want a new sync mode for simple differences in +semantics. Create a new mode simply named "BITMAP" that is designed to +make use of the new Bitmap Sync Mode field. + +Because the only bitmap sync mode is 'on-success', this adds no new +functionality to the backup job (yet). The old incremental backup mode +is maintained as a syntactic sugar for sync=bitmap, mode=on-success. + +Add all of the plumbing necessary to support this new instruction. + +Signed-off-by: John Snow +Reviewed-by: Max Reitz +Message-id: 20190709232550.10724-6-jsnow@redhat.com +Signed-off-by: John Snow +--- + block/backup.c | 20 ++++++++++++-------- + block/mirror.c | 6 ++++-- + block/replication.c | 2 +- + blockdev.c | 25 +++++++++++++++++++++++-- + include/block/block_int.h | 4 +++- + qapi/block-core.json | 21 +++++++++++++++------ + 6 files changed, 58 insertions(+), 20 deletions(-) + +diff --git a/block/backup.c b/block/backup.c +index 88354dcb32..e37eda80cd 100644 +--- a/block/backup.c ++++ b/block/backup.c +@@ -38,9 +38,9 @@ typedef struct CowRequest { + typedef struct BackupBlockJob { + BlockJob common; + BlockBackend *target; +- /* bitmap for sync=incremental */ + BdrvDirtyBitmap *sync_bitmap; + MirrorSyncMode sync_mode; ++ BitmapSyncMode bitmap_mode; + BlockdevOnError on_source_error; + BlockdevOnError on_target_error; + CoRwlock flush_rwlock; +@@ -461,7 +461,7 @@ static int coroutine_fn backup_run(Job *job, Error **errp) + + job_progress_set_remaining(job, s->len); + +- if (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) { ++ if (s->sync_mode == MIRROR_SYNC_MODE_BITMAP) { + backup_incremental_init_copy_bitmap(s); + } else { + hbitmap_set(s->copy_bitmap, 0, s->len); +@@ -545,6 +545,7 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target, + BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, + BlockDriverState *target, int64_t speed, + MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap, ++ BitmapSyncMode bitmap_mode, + bool compress, + BlockdevOnError on_source_error, + BlockdevOnError on_target_error, +@@ -592,10 +593,13 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, + return NULL; + } + +- if (sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) { ++ /* QMP interface should have handled translating this to bitmap mode */ ++ assert(sync_mode != MIRROR_SYNC_MODE_INCREMENTAL); ++ ++ if (sync_mode == MIRROR_SYNC_MODE_BITMAP) { + if (!sync_bitmap) { + error_setg(errp, "must provide a valid bitmap name for " +- "\"incremental\" sync mode"); ++ "'%s' sync mode", MirrorSyncMode_str(sync_mode)); + return NULL; + } + +@@ -605,8 +609,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, + } + } else if (sync_bitmap) { + error_setg(errp, +- "a sync_bitmap was provided to backup_run, " +- "but received an incompatible sync_mode (%s)", ++ "a bitmap was given to backup_job_create, " ++ "but it received an incompatible sync_mode (%s)", + MirrorSyncMode_str(sync_mode)); + return NULL; + } +@@ -648,8 +652,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, + job->on_source_error = on_source_error; + job->on_target_error = on_target_error; + job->sync_mode = sync_mode; +- job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ? +- sync_bitmap : NULL; ++ job->sync_bitmap = sync_bitmap; ++ job->bitmap_mode = bitmap_mode; + job->compress = compress; + + /* Detect image-fleecing (and similar) schemes */ +diff --git a/block/mirror.c b/block/mirror.c +index abcf60a961..ccae49a28e 100644 +--- a/block/mirror.c ++++ b/block/mirror.c +@@ -1770,8 +1770,10 @@ void mirror_start(const char *job_id, BlockDriverState *bs, + bool is_none_mode; + BlockDriverState *base; + +- if (mode == MIRROR_SYNC_MODE_INCREMENTAL) { +- error_setg(errp, "Sync mode 'incremental' not supported"); ++ if ((mode == MIRROR_SYNC_MODE_INCREMENTAL) || ++ (mode == MIRROR_SYNC_MODE_BITMAP)) { ++ error_setg(errp, "Sync mode '%s' not supported", ++ MirrorSyncMode_str(mode)); + return; + } + is_none_mode = mode == MIRROR_SYNC_MODE_NONE; +diff --git a/block/replication.c b/block/replication.c +index 23b2993d74..936b2f8b5a 100644 +--- a/block/replication.c ++++ b/block/replication.c +@@ -543,7 +543,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode, + + s->backup_job = backup_job_create( + NULL, s->secondary_disk->bs, s->hidden_disk->bs, +- 0, MIRROR_SYNC_MODE_NONE, NULL, false, ++ 0, MIRROR_SYNC_MODE_NONE, NULL, 0, false, + BLOCKDEV_ON_ERROR_REPORT, + BLOCKDEV_ON_ERROR_REPORT, JOB_INTERNAL, + backup_job_completed, bs, NULL, &local_err); +diff --git a/blockdev.c b/blockdev.c +index aa15ed1f00..34c8b651e1 100644 +--- a/blockdev.c ++++ b/blockdev.c +@@ -3508,12 +3508,31 @@ static BlockJob *do_backup_common(BackupCommon *backup, + return NULL; + } + ++ if (backup->sync == MIRROR_SYNC_MODE_INCREMENTAL) { ++ if (backup->has_bitmap_mode && ++ backup->bitmap_mode != BITMAP_SYNC_MODE_ON_SUCCESS) { ++ error_setg(errp, "Bitmap sync mode must be '%s' " ++ "when using sync mode '%s'", ++ BitmapSyncMode_str(BITMAP_SYNC_MODE_ON_SUCCESS), ++ MirrorSyncMode_str(backup->sync)); ++ return NULL; ++ } ++ backup->has_bitmap_mode = true; ++ backup->sync = MIRROR_SYNC_MODE_BITMAP; ++ backup->bitmap_mode = BITMAP_SYNC_MODE_ON_SUCCESS; ++ } ++ + if (backup->has_bitmap) { + bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap); + if (!bmap) { + error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap); + return NULL; + } ++ if (!backup->has_bitmap_mode) { ++ error_setg(errp, "Bitmap sync mode must be given " ++ "when providing a bitmap"); ++ return NULL; ++ } + if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_DEFAULT, errp)) { + return NULL; + } +@@ -3527,8 +3546,10 @@ static BlockJob *do_backup_common(BackupCommon *backup, + } + + 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, ++ backup->sync, bmap, backup->bitmap_mode, ++ backup->compress, ++ backup->on_source_error, ++ backup->on_target_error, + job_flags, NULL, NULL, txn, errp); + return job; + } +diff --git a/include/block/block_int.h b/include/block/block_int.h +index 05ee6b4866..76117a761a 100644 +--- a/include/block/block_int.h ++++ b/include/block/block_int.h +@@ -1152,7 +1152,8 @@ void mirror_start(const char *job_id, BlockDriverState *bs, + * @target: Block device to write to. + * @speed: The maximum speed, in bytes per second, or 0 for unlimited. + * @sync_mode: What parts of the disk image should be copied to the destination. +- * @sync_bitmap: The dirty bitmap if sync_mode is MIRROR_SYNC_MODE_INCREMENTAL. ++ * @sync_bitmap: The dirty bitmap if sync_mode is 'bitmap' or 'incremental' ++ * @bitmap_mode: The bitmap synchronization policy to use. + * @on_source_error: The action to take upon error reading from the source. + * @on_target_error: The action to take upon error writing to the target. + * @creation_flags: Flags that control the behavior of the Job lifetime. +@@ -1168,6 +1169,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, + BlockDriverState *target, int64_t speed, + MirrorSyncMode sync_mode, + BdrvDirtyBitmap *sync_bitmap, ++ BitmapSyncMode bitmap_mode, + bool compress, + BlockdevOnError on_source_error, + BlockdevOnError on_target_error, +diff --git a/qapi/block-core.json b/qapi/block-core.json +index b8d12a4951..97baff3a8c 100644 +--- a/qapi/block-core.json ++++ b/qapi/block-core.json +@@ -1127,12 +1127,15 @@ + # + # @none: only copy data written from now on + # +-# @incremental: only copy data described by the dirty bitmap. Since: 2.4 ++# @incremental: only copy data described by the dirty bitmap. (since: 2.4) ++# ++# @bitmap: only copy data described by the dirty bitmap. (since: 4.2) ++# Behavior on completion is determined by the BitmapSyncMode. + # + # Since: 1.3 + ## + { 'enum': 'MirrorSyncMode', +- 'data': ['top', 'full', 'none', 'incremental'] } ++ 'data': ['top', 'full', 'none', 'incremental', 'bitmap'] } + + ## + # @BitmapSyncMode: +@@ -1343,9 +1346,14 @@ + # @speed: the maximum speed, in bytes per second. The default is 0, + # for unlimited. + # +-# @bitmap: the name of dirty bitmap if sync is "incremental". +-# Must be present if sync is "incremental", must NOT be present +-# otherwise. (Since 2.4 (drive-backup), 3.1 (blockdev-backup)) ++# @bitmap: the name of a dirty bitmap if sync is "bitmap" or "incremental". ++# Must be present if sync is "bitmap" or "incremental". ++# Must not be present otherwise. ++# (Since 2.4 (drive-backup), 3.1 (blockdev-backup)) ++# ++# @bitmap-mode: Specifies the type of data the bitmap should contain after ++# the operation concludes. Must be present if sync is "bitmap". ++# Must NOT be present otherwise. (Since 4.2) + # + # @compress: true to compress data, if the target format supports it. + # (default: false) (since 2.8) +@@ -1380,7 +1388,8 @@ + { 'struct': 'BackupCommon', + 'data': { '*job-id': 'str', 'device': 'str', + 'sync': 'MirrorSyncMode', '*speed': 'int', +- '*bitmap': 'str', '*compress': 'bool', ++ '*bitmap': 'str', '*bitmap-mode': 'BitmapSyncMode', ++ '*compress': 'bool', + '*on-source-error': 'BlockdevOnError', + '*on-target-error': 'BlockdevOnError', + '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } } +-- +2.27.0 + -- Gitee From b4ad4436e1e07e0890087502df79381653cad88f Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Mon, 29 Jul 2019 16:35:53 -0400 Subject: [PATCH 11/23] block/backup: add 'never' policy to bitmap sync mode This adds a "never" policy for bitmap synchronization. Regardless of if the job succeeds or fails, we never update the bitmap. This can be used to perform differential backups, or simply to avoid the job modifying a bitmap. Signed-off-by: John Snow Reviewed-by: Max Reitz Message-id: 20190709232550.10724-7-jsnow@redhat.com Signed-off-by: John Snow --- ...add-never-policy-to-bitmap-sync-mode.patch | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 block-backup-add-never-policy-to-bitmap-sync-mode.patch diff --git a/block-backup-add-never-policy-to-bitmap-sync-mode.patch b/block-backup-add-never-policy-to-bitmap-sync-mode.patch new file mode 100644 index 0000000..e7a3dc3 --- /dev/null +++ b/block-backup-add-never-policy-to-bitmap-sync-mode.patch @@ -0,0 +1,59 @@ +From 98ed0f915cf3335768ed84ee5dfa54f4e99aaf00 Mon Sep 17 00:00:00 2001 +From: John Snow +Date: Mon, 29 Jul 2019 16:35:53 -0400 +Subject: [PATCH] block/backup: add 'never' policy to bitmap sync mode + +This adds a "never" policy for bitmap synchronization. Regardless of if +the job succeeds or fails, we never update the bitmap. This can be used +to perform differential backups, or simply to avoid the job modifying a +bitmap. + +Signed-off-by: John Snow +Reviewed-by: Max Reitz +Message-id: 20190709232550.10724-7-jsnow@redhat.com +Signed-off-by: John Snow +--- + block/backup.c | 7 +++++-- + qapi/block-core.json | 5 ++++- + 2 files changed, 9 insertions(+), 3 deletions(-) + +diff --git a/block/backup.c b/block/backup.c +index e37eda80cd..84a56337ac 100644 +--- a/block/backup.c ++++ b/block/backup.c +@@ -274,8 +274,11 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret) + BdrvDirtyBitmap *bm; + BlockDriverState *bs = blk_bs(job->common.blk); + +- if (ret < 0) { +- /* Merge the successor back into the parent, delete nothing. */ ++ if (ret < 0 || job->bitmap_mode == BITMAP_SYNC_MODE_NEVER) { ++ /* ++ * Failure, or we don't want to synchronize the bitmap. ++ * Merge the successor back into the parent, delete nothing. ++ */ + bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL); + assert(bm); + } else { +diff --git a/qapi/block-core.json b/qapi/block-core.json +index 97baff3a8c..48a0bfab63 100644 +--- a/qapi/block-core.json ++++ b/qapi/block-core.json +@@ -1146,10 +1146,13 @@ + # @on-success: The bitmap is only synced when the operation is successful. + # This is the behavior always used for 'INCREMENTAL' backups. + # ++# @never: The bitmap is never synchronized with the operation, and is ++# treated solely as a read-only manifest of blocks to copy. ++# + # Since: 4.2 + ## + { 'enum': 'BitmapSyncMode', +- 'data': ['on-success'] } ++ 'data': ['on-success', 'never'] } + + ## + # @MirrorCopyMode: +-- +2.27.0 + -- Gitee From 13d5e14c13059455df03eff4492aa89420725160 Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Mon, 29 Jul 2019 16:35:54 -0400 Subject: [PATCH 12/23] block/backup: loosen restriction on readonly bitmaps With the "never" sync policy, we actually can utilize readonly bitmaps now. Loosen the check at the QMP level, and tighten it based on provided arguments down at the job creation level instead. Signed-off-by: John Snow Reviewed-by: Max Reitz Message-id: 20190709232550.10724-19-jsnow@redhat.com Signed-off-by: John Snow --- ...osen-restriction-on-readonly-bitmaps.patch | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 block-backup-loosen-restriction-on-readonly-bitmaps.patch diff --git a/block-backup-loosen-restriction-on-readonly-bitmaps.patch b/block-backup-loosen-restriction-on-readonly-bitmaps.patch new file mode 100644 index 0000000..ab0617c --- /dev/null +++ b/block-backup-loosen-restriction-on-readonly-bitmaps.patch @@ -0,0 +1,51 @@ +From 801e9452bc80a38ee26fe12ba42356851acd6a9e Mon Sep 17 00:00:00 2001 +From: John Snow +Date: Mon, 29 Jul 2019 16:35:54 -0400 +Subject: [PATCH] block/backup: loosen restriction on readonly bitmaps + +With the "never" sync policy, we actually can utilize readonly bitmaps +now. Loosen the check at the QMP level, and tighten it based on +provided arguments down at the job creation level instead. + +Signed-off-by: John Snow +Reviewed-by: Max Reitz +Message-id: 20190709232550.10724-19-jsnow@redhat.com +Signed-off-by: John Snow +--- + block/backup.c | 6 ++++++ + blockdev.c | 2 +- + 2 files changed, 7 insertions(+), 1 deletion(-) + +diff --git a/block/backup.c b/block/backup.c +index 84a56337ac..59ac2c0396 100644 +--- a/block/backup.c ++++ b/block/backup.c +@@ -606,6 +606,12 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, + return NULL; + } + ++ /* If we need to write to this bitmap, check that we can: */ ++ if (bitmap_mode != BITMAP_SYNC_MODE_NEVER && ++ bdrv_dirty_bitmap_check(sync_bitmap, BDRV_BITMAP_DEFAULT, errp)) { ++ return NULL; ++ } ++ + /* Create a new bitmap, and freeze/disable this one. */ + if (bdrv_dirty_bitmap_create_successor(bs, sync_bitmap, errp) < 0) { + return NULL; +diff --git a/blockdev.c b/blockdev.c +index 34c8b651e1..efb69d343a 100644 +--- a/blockdev.c ++++ b/blockdev.c +@@ -3533,7 +3533,7 @@ static BlockJob *do_backup_common(BackupCommon *backup, + "when providing a bitmap"); + return NULL; + } +- if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_DEFAULT, errp)) { ++ if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) { + return NULL; + } + } +-- +2.27.0 + -- Gitee From 19b6914ef5479fad54896354d2c93267ac2cc6de Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Mon, 29 Jul 2019 16:35:55 -0400 Subject: [PATCH 13/23] block/backup: hoist bitmap check into QMP interface This is nicer to do in the unified QMP interface that we have now, because it lets us use the right terminology back at the user. Signed-off-by: John Snow Reviewed-by: Max Reitz Message-id: 20190716000117.25219-5-jsnow@redhat.com Signed-off-by: John Snow --- ...oist-bitmap-check-into-QMP-interface.patch | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100644 block-backup-hoist-bitmap-check-into-QMP-interface.patch diff --git a/block-backup-hoist-bitmap-check-into-QMP-interface.patch b/block-backup-hoist-bitmap-check-into-QMP-interface.patch new file mode 100644 index 0000000..51dc67c --- /dev/null +++ b/block-backup-hoist-bitmap-check-into-QMP-interface.patch @@ -0,0 +1,73 @@ +From 9cc9e9657aad126502183fa4ceb9b962b55471cb Mon Sep 17 00:00:00 2001 +From: John Snow +Date: Mon, 29 Jul 2019 16:35:55 -0400 +Subject: [PATCH] block/backup: hoist bitmap check into QMP interface + +This is nicer to do in the unified QMP interface that we have now, +because it lets us use the right terminology back at the user. + +Signed-off-by: John Snow +Reviewed-by: Max Reitz +Message-id: 20190716000117.25219-5-jsnow@redhat.com +Signed-off-by: John Snow +--- + block/backup.c | 13 ++++--------- + blockdev.c | 10 ++++++++++ + 2 files changed, 14 insertions(+), 9 deletions(-) + +diff --git a/block/backup.c b/block/backup.c +index 59ac2c0396..cc19643b47 100644 +--- a/block/backup.c ++++ b/block/backup.c +@@ -565,6 +565,10 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, + assert(bs); + assert(target); + ++ /* QMP interface protects us from these cases */ ++ assert(sync_mode != MIRROR_SYNC_MODE_INCREMENTAL); ++ assert(sync_bitmap || sync_mode != MIRROR_SYNC_MODE_BITMAP); ++ + if (bs == target) { + error_setg(errp, "Source and target cannot be the same"); + return NULL; +@@ -596,16 +600,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, + return NULL; + } + +- /* QMP interface should have handled translating this to bitmap mode */ +- assert(sync_mode != MIRROR_SYNC_MODE_INCREMENTAL); +- + if (sync_mode == MIRROR_SYNC_MODE_BITMAP) { +- if (!sync_bitmap) { +- error_setg(errp, "must provide a valid bitmap name for " +- "'%s' sync mode", MirrorSyncMode_str(sync_mode)); +- return NULL; +- } +- + /* If we need to write to this bitmap, check that we can: */ + if (bitmap_mode != BITMAP_SYNC_MODE_NEVER && + bdrv_dirty_bitmap_check(sync_bitmap, BDRV_BITMAP_DEFAULT, errp)) { +diff --git a/blockdev.c b/blockdev.c +index efb69d343a..0a71a15fa2 100644 +--- a/blockdev.c ++++ b/blockdev.c +@@ -3508,6 +3508,16 @@ static BlockJob *do_backup_common(BackupCommon *backup, + return NULL; + } + ++ if ((backup->sync == MIRROR_SYNC_MODE_BITMAP) || ++ (backup->sync == MIRROR_SYNC_MODE_INCREMENTAL)) { ++ /* done before desugaring 'incremental' to print the right message */ ++ if (!backup->has_bitmap) { ++ error_setg(errp, "must provide a valid bitmap name for " ++ "'%s' sync mode", MirrorSyncMode_str(backup->sync)); ++ return NULL; ++ } ++ } ++ + if (backup->sync == MIRROR_SYNC_MODE_INCREMENTAL) { + if (backup->has_bitmap_mode && + backup->bitmap_mode != BITMAP_SYNC_MODE_ON_SUCCESS) { +-- +2.27.0 + -- Gitee From 6751482910a64eb65563903e72522705fc556bc2 Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Tue, 30 Jul 2019 19:32:49 +0300 Subject: [PATCH 14/23] block/backup: deal with zero detection We have detect_zeroes option, so at least for blockdev-backup user should define it if zero-detection is needed. For drive-backup leave detection enabled by default but do it through existing option instead of open-coding. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: John Snow Reviewed-by: Max Reitz Message-id: 20190730163251.755248-2-vsementsov@virtuozzo.com Signed-off-by: John Snow --- block-backup-deal-with-zero-detection.patch | 83 +++++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 block-backup-deal-with-zero-detection.patch diff --git a/block-backup-deal-with-zero-detection.patch b/block-backup-deal-with-zero-detection.patch new file mode 100644 index 0000000..9f111e5 --- /dev/null +++ b/block-backup-deal-with-zero-detection.patch @@ -0,0 +1,83 @@ +From 3cf14b9a7daf0a40eb2af7a86e67cb05f6d2bea6 Mon Sep 17 00:00:00 2001 +From: Vladimir Sementsov-Ogievskiy +Date: Tue, 30 Jul 2019 19:32:49 +0300 +Subject: [PATCH] block/backup: deal with zero detection + +We have detect_zeroes option, so at least for blockdev-backup user +should define it if zero-detection is needed. For drive-backup leave +detection enabled by default but do it through existing option instead +of open-coding. + +Signed-off-by: Vladimir Sementsov-Ogievskiy +Reviewed-by: John Snow +Reviewed-by: Max Reitz +Message-id: 20190730163251.755248-2-vsementsov@virtuozzo.com +Signed-off-by: John Snow +--- + block/backup.c | 15 ++++++--------- + blockdev.c | 8 ++++---- + 2 files changed, 10 insertions(+), 13 deletions(-) + +diff --git a/block/backup.c b/block/backup.c +index cc19643b47..6023573299 100644 +--- a/block/backup.c ++++ b/block/backup.c +@@ -110,7 +110,10 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job, + BlockBackend *blk = job->common.blk; + int nbytes; + int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0; +- int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0; ++ int write_flags = ++ (job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0) | ++ (job->compress ? BDRV_REQ_WRITE_COMPRESSED : 0); ++ + + assert(QEMU_IS_ALIGNED(start, job->cluster_size)); + hbitmap_reset(job->copy_bitmap, start, job->cluster_size); +@@ -128,14 +131,8 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job, + goto fail; + } + +- if (buffer_is_zero(*bounce_buffer, nbytes)) { +- ret = blk_co_pwrite_zeroes(job->target, start, +- nbytes, write_flags | BDRV_REQ_MAY_UNMAP); +- } else { +- ret = blk_co_pwrite(job->target, start, +- nbytes, *bounce_buffer, write_flags | +- (job->compress ? BDRV_REQ_WRITE_COMPRESSED : 0)); +- } ++ ret = blk_co_pwrite(job->target, start, nbytes, *bounce_buffer, ++ write_flags); + if (ret < 0) { + trace_backup_do_cow_write_fail(job, start, ret); + if (error_is_read) { +diff --git a/blockdev.c b/blockdev.c +index 0a71a15fa2..94e5aee30b 100644 +--- a/blockdev.c ++++ b/blockdev.c +@@ -3572,7 +3572,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, + BlockDriverState *source = NULL; + BlockJob *job = NULL; + AioContext *aio_context; +- QDict *options = NULL; ++ QDict *options; + Error *local_err = NULL; + int flags; + int64_t size; +@@ -3645,10 +3645,10 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, + goto out; + } + ++ options = qdict_new(); ++ qdict_put_str(options, "discard", "unmap"); ++ qdict_put_str(options, "detect-zeroes", "unmap"); + if (backup->format) { +- if (!options) { +- options = qdict_new(); +- } + qdict_put_str(options, "driver", backup->format); + } + +-- +2.27.0 + -- Gitee From 19b8d6bdf3898bfc856f8376cccd96966fce4d52 Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Wed, 24 Jul 2019 19:12:30 +0200 Subject: [PATCH 15/23] mirror: Fix bdrv_has_zero_init() use bdrv_has_zero_init() only has meaning for newly created images or image areas. If the mirror job itself did not create the image, it cannot rely on bdrv_has_zero_init()'s result to carry any meaning. This is the case for drive-mirror with mode=existing and always for blockdev-mirror. Note that we only have to zero-initialize the target with sync=full, because other modes actually do not promise that the target will contain the same data as the source after the job -- sync=top only promises to copy anything allocated in the top layer, and sync=none will only copy new I/O. (Which is how mirror has always handled it.) Signed-off-by: Max Reitz Message-id: 20190724171239.8764-3-mreitz@redhat.com Reviewed-by: Maxim Levitsky Signed-off-by: Max Reitz --- mirror-Fix-bdrv_has_zero_init-use.patch | 205 ++++++++++++++++++++++++ 1 file changed, 205 insertions(+) create mode 100644 mirror-Fix-bdrv_has_zero_init-use.patch diff --git a/mirror-Fix-bdrv_has_zero_init-use.patch b/mirror-Fix-bdrv_has_zero_init-use.patch new file mode 100644 index 0000000..54fde69 --- /dev/null +++ b/mirror-Fix-bdrv_has_zero_init-use.patch @@ -0,0 +1,205 @@ +From 7fcb1c1a956a8cad5c2e8585e53878edc4fd9eca Mon Sep 17 00:00:00 2001 +From: Max Reitz +Date: Wed, 24 Jul 2019 19:12:30 +0200 +Subject: [PATCH] mirror: Fix bdrv_has_zero_init() use + +bdrv_has_zero_init() only has meaning for newly created images or image +areas. If the mirror job itself did not create the image, it cannot +rely on bdrv_has_zero_init()'s result to carry any meaning. + +This is the case for drive-mirror with mode=existing and always for +blockdev-mirror. + +Note that we only have to zero-initialize the target with sync=full, +because other modes actually do not promise that the target will contain +the same data as the source after the job -- sync=top only promises to +copy anything allocated in the top layer, and sync=none will only copy +new I/O. (Which is how mirror has always handled it.) + +Signed-off-by: Max Reitz +Message-id: 20190724171239.8764-3-mreitz@redhat.com +Reviewed-by: Maxim Levitsky +Signed-off-by: Max Reitz +--- + block/mirror.c | 11 ++++++++--- + blockdev.c | 16 +++++++++++++--- + include/block/block_int.h | 2 ++ + tests/test-block-iothread.c | 2 +- + 4 files changed, 24 insertions(+), 7 deletions(-) + +diff --git a/block/mirror.c b/block/mirror.c +index ccae49a28e..89a053b265 100644 +--- a/block/mirror.c ++++ b/block/mirror.c +@@ -51,6 +51,8 @@ typedef struct MirrorBlockJob { + Error *replace_blocker; + bool is_none_mode; + BlockMirrorBackingMode backing_mode; ++ /* Whether the target image requires explicit zero-initialization */ ++ bool zero_target; + MirrorCopyMode copy_mode; + BlockdevOnError on_source_error, on_target_error; + bool synced; +@@ -779,7 +781,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s) + int ret; + int64_t count; + +- if (base == NULL && !bdrv_has_zero_init(target_bs)) { ++ if (s->zero_target) { + if (!bdrv_can_write_zeroes_with_unmap(target_bs)) { + bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, s->bdev_length); + return 0; +@@ -1531,6 +1533,7 @@ static BlockJob *mirror_start_job( + const char *replaces, int64_t speed, + uint32_t granularity, int64_t buf_size, + BlockMirrorBackingMode backing_mode, ++ bool zero_target, + BlockdevOnError on_source_error, + BlockdevOnError on_target_error, + bool unmap, +@@ -1658,6 +1661,7 @@ static BlockJob *mirror_start_job( + s->on_target_error = on_target_error; + s->is_none_mode = is_none_mode; + s->backing_mode = backing_mode; ++ s->zero_target = zero_target; + s->copy_mode = copy_mode; + s->base = base; + s->granularity = granularity; +@@ -1762,6 +1766,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs, + int creation_flags, int64_t speed, + uint32_t granularity, int64_t buf_size, + MirrorSyncMode mode, BlockMirrorBackingMode backing_mode, ++ bool zero_target, + BlockdevOnError on_source_error, + BlockdevOnError on_target_error, + bool unmap, const char *filter_node_name, +@@ -1779,7 +1784,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs, + is_none_mode = mode == MIRROR_SYNC_MODE_NONE; + base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL; + mirror_start_job(job_id, bs, creation_flags, target, replaces, +- speed, granularity, buf_size, backing_mode, ++ speed, granularity, buf_size, backing_mode, zero_target, + on_source_error, on_target_error, unmap, NULL, NULL, + &mirror_job_driver, is_none_mode, base, false, + filter_node_name, true, copy_mode, errp); +@@ -1806,7 +1811,7 @@ BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs, + + ret = mirror_start_job( + job_id, bs, creation_flags, base, NULL, speed, 0, 0, +- MIRROR_LEAVE_BACKING_CHAIN, ++ MIRROR_LEAVE_BACKING_CHAIN, false, + on_error, on_error, true, cb, opaque, + &commit_active_job_driver, false, base, auto_complete, + filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND, +diff --git a/blockdev.c b/blockdev.c +index 94e5aee30b..4435795b6d 100644 +--- a/blockdev.c ++++ b/blockdev.c +@@ -3739,6 +3739,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs, + bool has_replaces, const char *replaces, + enum MirrorSyncMode sync, + BlockMirrorBackingMode backing_mode, ++ bool zero_target, + bool has_speed, int64_t speed, + bool has_granularity, uint32_t granularity, + bool has_buf_size, int64_t buf_size, +@@ -3847,7 +3848,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs, + */ + mirror_start(job_id, bs, target, + has_replaces ? replaces : NULL, job_flags, +- speed, granularity, buf_size, sync, backing_mode, ++ speed, granularity, buf_size, sync, backing_mode, zero_target, + on_source_error, on_target_error, unmap, filter_node_name, + copy_mode, errp); + } +@@ -3863,6 +3864,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp) + int flags; + int64_t size; + const char *format = arg->format; ++ bool zero_target; + int ret; + + bs = qmp_get_root_bs(arg->device, errp); +@@ -3964,6 +3966,10 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp) + goto out; + } + ++ zero_target = (arg->sync == MIRROR_SYNC_MODE_FULL && ++ (arg->mode == NEW_IMAGE_MODE_EXISTING || ++ !bdrv_has_zero_init(target_bs))); ++ + ret = bdrv_try_set_aio_context(target_bs, aio_context, errp); + if (ret < 0) { + bdrv_unref(target_bs); +@@ -3972,7 +3978,8 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp) + + blockdev_mirror_common(arg->has_job_id ? arg->job_id : NULL, bs, target_bs, + arg->has_replaces, arg->replaces, arg->sync, +- backing_mode, arg->has_speed, arg->speed, ++ backing_mode, zero_target, ++ arg->has_speed, arg->speed, + arg->has_granularity, arg->granularity, + arg->has_buf_size, arg->buf_size, + arg->has_on_source_error, arg->on_source_error, +@@ -4012,6 +4019,7 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id, + AioContext *aio_context; + BlockMirrorBackingMode backing_mode = MIRROR_LEAVE_BACKING_CHAIN; + Error *local_err = NULL; ++ bool zero_target; + int ret; + + bs = qmp_get_root_bs(device, errp); +@@ -4024,6 +4032,8 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id, + return; + } + ++ zero_target = (sync == MIRROR_SYNC_MODE_FULL); ++ + aio_context = bdrv_get_aio_context(bs); + aio_context_acquire(aio_context); + +@@ -4034,7 +4044,7 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id, + + blockdev_mirror_common(has_job_id ? job_id : NULL, bs, target_bs, + has_replaces, replaces, sync, backing_mode, +- has_speed, speed, ++ zero_target, has_speed, speed, + has_granularity, granularity, + has_buf_size, buf_size, + has_on_source_error, on_source_error, +diff --git a/include/block/block_int.h b/include/block/block_int.h +index 76117a761a..154b9b5501 100644 +--- a/include/block/block_int.h ++++ b/include/block/block_int.h +@@ -1120,6 +1120,7 @@ BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs, + * @buf_size: The amount of data that can be in flight at one time. + * @mode: Whether to collapse all images in the chain to the target. + * @backing_mode: How to establish the target's backing chain after completion. ++ * @zero_target: Whether the target should be explicitly zero-initialized + * @on_source_error: The action to take upon error reading from the source. + * @on_target_error: The action to take upon error writing to the target. + * @unmap: Whether to unmap target where source sectors only contain zeroes. +@@ -1139,6 +1140,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs, + int creation_flags, int64_t speed, + uint32_t granularity, int64_t buf_size, + MirrorSyncMode mode, BlockMirrorBackingMode backing_mode, ++ bool zero_target, + BlockdevOnError on_source_error, + BlockdevOnError on_target_error, + bool unmap, const char *filter_node_name, +diff --git a/tests/test-block-iothread.c b/tests/test-block-iothread.c +index 1949d5e61a..debfb69bfb 100644 +--- a/tests/test-block-iothread.c ++++ b/tests/test-block-iothread.c +@@ -611,7 +611,7 @@ static void test_propagate_mirror(void) + + /* Start a mirror job */ + mirror_start("job0", src, target, NULL, JOB_DEFAULT, 0, 0, 0, +- MIRROR_SYNC_MODE_NONE, MIRROR_OPEN_BACKING_CHAIN, ++ MIRROR_SYNC_MODE_NONE, MIRROR_OPEN_BACKING_CHAIN, false, + BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT, + false, "filter_node", MIRROR_COPY_MODE_BACKGROUND, + &error_abort); +-- +2.27.0 + -- Gitee From c9ae03e3f37d934d3fabeb111bd5723b630c1eb2 Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Wed, 8 Jan 2020 15:31:31 +0100 Subject: [PATCH 16/23] blockdev: fix coding style issues in drive_backup_prepare Fix a couple of minor coding style issues in drive_backup_prepare. Signed-off-by: Sergio Lopez Reviewed-by: Max Reitz Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- ...ing-style-issues-in-drive_backup_pre.patch | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 blockdev-fix-coding-style-issues-in-drive_backup_pre.patch diff --git a/blockdev-fix-coding-style-issues-in-drive_backup_pre.patch b/blockdev-fix-coding-style-issues-in-drive_backup_pre.patch new file mode 100644 index 0000000..e915b05 --- /dev/null +++ b/blockdev-fix-coding-style-issues-in-drive_backup_pre.patch @@ -0,0 +1,44 @@ +From ffbf1e237d0311512c411e195278e69d710fb9cf Mon Sep 17 00:00:00 2001 +From: Sergio Lopez +Date: Wed, 8 Jan 2020 15:31:31 +0100 +Subject: [PATCH] blockdev: fix coding style issues in drive_backup_prepare + +Fix a couple of minor coding style issues in drive_backup_prepare. + +Signed-off-by: Sergio Lopez +Reviewed-by: Max Reitz +Reviewed-by: Kevin Wolf +Signed-off-by: Kevin Wolf +--- + blockdev.c | 8 +++++--- + 1 file changed, 5 insertions(+), 3 deletions(-) + +diff --git a/blockdev.c b/blockdev.c +index 4435795b6d..99b1cafb8f 100644 +--- a/blockdev.c ++++ b/blockdev.c +@@ -3597,7 +3597,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, + + if (!backup->has_format) { + backup->format = backup->mode == NEW_IMAGE_MODE_EXISTING ? +- NULL : (char*) bs->drv->format_name; ++ NULL : (char *) bs->drv->format_name; + } + + /* Early check to avoid creating target */ +@@ -3607,8 +3607,10 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, + + flags = bs->open_flags | BDRV_O_RDWR; + +- /* See if we have a backing HD we can use to create our new image +- * on top of. */ ++ /* ++ * See if we have a backing HD we can use to create our new image ++ * on top of. ++ */ + if (backup->sync == MIRROR_SYNC_MODE_TOP) { + source = backing_bs(bs); + if (!source) { +-- +2.27.0 + -- Gitee From 38929e0e68256a1f53258c70bf2d326672d71250 Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Wed, 8 Jan 2020 15:31:32 +0100 Subject: [PATCH 17/23] blockdev: unify qmp_drive_backup and drive-backup transaction paths Issuing a drive-backup from qmp_drive_backup takes a slightly different path than when it's issued from a transaction. In the code, this is manifested as some redundancy between do_drive_backup() and drive_backup_prepare(). This change unifies both paths, merging do_drive_backup() and drive_backup_prepare(), and changing qmp_drive_backup() to create a transaction instead of calling do_backup_common() direcly. As a side-effect, now qmp_drive_backup() is executed inside a drained section, as it happens when creating a drive-backup transaction. This change is visible from the user's perspective, as the job gets paused and immediately resumed before starting the actual work. Also fix tests 141, 185 and 219 to cope with the extra JOB_STATUS_CHANGE lines. Signed-off-by: Sergio Lopez Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- ...mp_drive_backup-and-drive-backup-tra.patch | 406 ++++++++++++++++++ 1 file changed, 406 insertions(+) create mode 100644 blockdev-unify-qmp_drive_backup-and-drive-backup-tra.patch diff --git a/blockdev-unify-qmp_drive_backup-and-drive-backup-tra.patch b/blockdev-unify-qmp_drive_backup-and-drive-backup-tra.patch new file mode 100644 index 0000000..aefa05e --- /dev/null +++ b/blockdev-unify-qmp_drive_backup-and-drive-backup-tra.patch @@ -0,0 +1,406 @@ +From 952f7f53cdd4320d1a0328481fa578dd199eb1ce Mon Sep 17 00:00:00 2001 +From: Sergio Lopez +Date: Wed, 8 Jan 2020 15:31:32 +0100 +Subject: [PATCH] blockdev: unify qmp_drive_backup and drive-backup transaction + paths + +Issuing a drive-backup from qmp_drive_backup takes a slightly +different path than when it's issued from a transaction. In the code, +this is manifested as some redundancy between do_drive_backup() and +drive_backup_prepare(). + +This change unifies both paths, merging do_drive_backup() and +drive_backup_prepare(), and changing qmp_drive_backup() to create a +transaction instead of calling do_backup_common() direcly. + +As a side-effect, now qmp_drive_backup() is executed inside a drained +section, as it happens when creating a drive-backup transaction. This +change is visible from the user's perspective, as the job gets paused +and immediately resumed before starting the actual work. + +Also fix tests 141, 185 and 219 to cope with the extra +JOB_STATUS_CHANGE lines. + +Signed-off-by: Sergio Lopez +Reviewed-by: Kevin Wolf +Signed-off-by: Kevin Wolf +--- + blockdev.c | 224 +++++++++++++++++-------------------- + tests/qemu-iotests/141.out | 2 + + tests/qemu-iotests/185.out | 2 + + tests/qemu-iotests/219 | 7 +- + tests/qemu-iotests/219.out | 8 ++ + 5 files changed, 117 insertions(+), 126 deletions(-) + +diff --git a/blockdev.c b/blockdev.c +index 99b1cafb8f..7016054688 100644 +--- a/blockdev.c ++++ b/blockdev.c +@@ -1804,39 +1804,128 @@ typedef struct DriveBackupState { + BlockJob *job; + } DriveBackupState; + +-static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, +- Error **errp); ++static BlockJob *do_backup_common(BackupCommon *backup, ++ BlockDriverState *bs, ++ BlockDriverState *target_bs, ++ AioContext *aio_context, ++ JobTxn *txn, Error **errp); + + static void drive_backup_prepare(BlkActionState *common, Error **errp) + { + DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); +- BlockDriverState *bs; + DriveBackup *backup; ++ BlockDriverState *bs; ++ BlockDriverState *target_bs; ++ BlockDriverState *source = NULL; + AioContext *aio_context; ++ QDict *options; + Error *local_err = NULL; ++ int flags; ++ int64_t size; ++ bool set_backing_hd = false; + + assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP); + backup = common->action->u.drive_backup.data; + ++ if (!backup->has_mode) { ++ backup->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS; ++ } ++ + bs = bdrv_lookup_bs(backup->device, backup->device, errp); + if (!bs) { + return; + } + ++ if (!bs->drv) { ++ error_setg(errp, "Device has no medium"); ++ return; ++ } ++ + aio_context = bdrv_get_aio_context(bs); + aio_context_acquire(aio_context); + + /* Paired with .clean() */ + bdrv_drained_begin(bs); + +- state->bs = bs; ++ if (!backup->has_format) { ++ backup->format = backup->mode == NEW_IMAGE_MODE_EXISTING ? ++ NULL : (char *) bs->drv->format_name; ++ } ++ ++ /* Early check to avoid creating target */ ++ if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) { ++ goto out; ++ } ++ ++ flags = bs->open_flags | BDRV_O_RDWR; ++ ++ /* ++ * See if we have a backing HD we can use to create our new image ++ * on top of. ++ */ ++ if (backup->sync == MIRROR_SYNC_MODE_TOP) { ++ source = backing_bs(bs); ++ if (!source) { ++ backup->sync = MIRROR_SYNC_MODE_FULL; ++ } ++ } ++ if (backup->sync == MIRROR_SYNC_MODE_NONE) { ++ source = bs; ++ flags |= BDRV_O_NO_BACKING; ++ set_backing_hd = true; ++ } ++ ++ size = bdrv_getlength(bs); ++ if (size < 0) { ++ error_setg_errno(errp, -size, "bdrv_getlength failed"); ++ goto out; ++ } ++ ++ if (backup->mode != NEW_IMAGE_MODE_EXISTING) { ++ assert(backup->format); ++ if (source) { ++ bdrv_refresh_filename(source); ++ bdrv_img_create(backup->target, backup->format, source->filename, ++ source->drv->format_name, NULL, ++ size, flags, false, &local_err); ++ } else { ++ bdrv_img_create(backup->target, backup->format, NULL, NULL, NULL, ++ size, flags, false, &local_err); ++ } ++ } + +- state->job = do_drive_backup(backup, common->block_job_txn, &local_err); + if (local_err) { + error_propagate(errp, local_err); + goto out; + } + ++ options = qdict_new(); ++ qdict_put_str(options, "discard", "unmap"); ++ qdict_put_str(options, "detect-zeroes", "unmap"); ++ if (backup->format) { ++ qdict_put_str(options, "driver", backup->format); ++ } ++ ++ target_bs = bdrv_open(backup->target, NULL, options, flags, errp); ++ if (!target_bs) { ++ goto out; ++ } ++ ++ if (set_backing_hd) { ++ bdrv_set_backing_hd(target_bs, source, &local_err); ++ if (local_err) { ++ goto unref; ++ } ++ } ++ ++ state->bs = bs; ++ ++ state->job = do_backup_common(qapi_DriveBackup_base(backup), ++ bs, target_bs, aio_context, ++ common->block_job_txn, errp); ++ ++unref: ++ bdrv_unref(target_bs); + out: + aio_context_release(aio_context); + } +@@ -3564,126 +3653,13 @@ static BlockJob *do_backup_common(BackupCommon *backup, + return job; + } + +-static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, +- Error **errp) +-{ +- BlockDriverState *bs; +- BlockDriverState *target_bs; +- BlockDriverState *source = NULL; +- BlockJob *job = NULL; +- AioContext *aio_context; +- QDict *options; +- Error *local_err = NULL; +- int flags; +- int64_t size; +- bool set_backing_hd = false; +- +- if (!backup->has_mode) { +- backup->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS; +- } +- +- bs = bdrv_lookup_bs(backup->device, backup->device, errp); +- if (!bs) { +- return NULL; +- } +- +- if (!bs->drv) { +- error_setg(errp, "Device has no medium"); +- return NULL; +- } +- +- aio_context = bdrv_get_aio_context(bs); +- aio_context_acquire(aio_context); +- +- if (!backup->has_format) { +- backup->format = backup->mode == NEW_IMAGE_MODE_EXISTING ? +- NULL : (char *) bs->drv->format_name; +- } +- +- /* Early check to avoid creating target */ +- if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) { +- goto out; +- } +- +- flags = bs->open_flags | BDRV_O_RDWR; +- +- /* +- * See if we have a backing HD we can use to create our new image +- * on top of. +- */ +- if (backup->sync == MIRROR_SYNC_MODE_TOP) { +- source = backing_bs(bs); +- if (!source) { +- backup->sync = MIRROR_SYNC_MODE_FULL; +- } +- } +- if (backup->sync == MIRROR_SYNC_MODE_NONE) { +- source = bs; +- flags |= BDRV_O_NO_BACKING; +- set_backing_hd = true; +- } +- +- size = bdrv_getlength(bs); +- if (size < 0) { +- error_setg_errno(errp, -size, "bdrv_getlength failed"); +- goto out; +- } +- +- if (backup->mode != NEW_IMAGE_MODE_EXISTING) { +- assert(backup->format); +- if (source) { +- bdrv_refresh_filename(source); +- bdrv_img_create(backup->target, backup->format, source->filename, +- source->drv->format_name, NULL, +- size, flags, false, &local_err); +- } else { +- bdrv_img_create(backup->target, backup->format, NULL, NULL, NULL, +- size, flags, false, &local_err); +- } +- } +- +- if (local_err) { +- error_propagate(errp, local_err); +- goto out; +- } +- +- options = qdict_new(); +- qdict_put_str(options, "discard", "unmap"); +- qdict_put_str(options, "detect-zeroes", "unmap"); +- if (backup->format) { +- qdict_put_str(options, "driver", backup->format); +- } +- +- target_bs = bdrv_open(backup->target, NULL, options, flags, errp); +- if (!target_bs) { +- goto out; +- } +- +- if (set_backing_hd) { +- bdrv_set_backing_hd(target_bs, source, &local_err); +- if (local_err) { +- goto unref; +- } +- } +- +- job = do_backup_common(qapi_DriveBackup_base(backup), +- bs, target_bs, aio_context, txn, errp); +- +-unref: +- bdrv_unref(target_bs); +-out: +- aio_context_release(aio_context); +- return job; +-} +- +-void qmp_drive_backup(DriveBackup *arg, Error **errp) ++void qmp_drive_backup(DriveBackup *backup, Error **errp) + { +- +- BlockJob *job; +- job = do_drive_backup(arg, NULL, errp); +- if (job) { +- job_start(&job->job); +- } ++ TransactionAction action = { ++ .type = TRANSACTION_ACTION_KIND_DRIVE_BACKUP, ++ .u.drive_backup.data = backup, ++ }; ++ blockdev_do_action(&action, errp); + } + + BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp) +diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out +index 4d71d9dcae..07e0ec66d7 100644 +--- a/tests/qemu-iotests/141.out ++++ b/tests/qemu-iotests/141.out +@@ -10,6 +10,8 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/m. + Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT + {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}} + {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}} ++{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "paused", "id": "job0"}} ++{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}} + {"return": {}} + {"error": {"class": "GenericError", "desc": "Node drv0 is in use"}} + {"return": {}} +diff --git a/tests/qemu-iotests/185.out b/tests/qemu-iotests/185.out +index ddfbf3c765..a233be7f58 100644 +--- a/tests/qemu-iotests/185.out ++++ b/tests/qemu-iotests/185.out +@@ -51,6 +51,8 @@ Formatting 'TEST_DIR/t.qcow2.copy', fmt=qcow2 size=67108864 cluster_size=65536 l + Formatting 'TEST_DIR/t.qcow2.copy', fmt=qcow2 size=67108864 cluster_size=65536 lazy_refcounts=off refcount_bits=16 + {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "disk"}} + {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "disk"}} ++{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "paused", "id": "disk"}} ++{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "disk"}} + {"return": {}} + {"return": {}} + {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} +diff --git a/tests/qemu-iotests/219 b/tests/qemu-iotests/219 +index e0c51662c0..655f54d881 100755 +--- a/tests/qemu-iotests/219 ++++ b/tests/qemu-iotests/219 +@@ -63,7 +63,7 @@ def test_pause_resume(vm): + # logged immediately + iotests.log(vm.qmp('query-jobs')) + +-def test_job_lifecycle(vm, job, job_args, has_ready=False): ++def test_job_lifecycle(vm, job, job_args, has_ready=False, is_mirror=False): + global img_size + + iotests.log('') +@@ -135,6 +135,9 @@ def test_job_lifecycle(vm, job, job_args, has_ready=False): + iotests.log('Waiting for PENDING state...') + iotests.log(iotests.filter_qmp_event(vm.event_wait('JOB_STATUS_CHANGE'))) + iotests.log(iotests.filter_qmp_event(vm.event_wait('JOB_STATUS_CHANGE'))) ++ if is_mirror: ++ iotests.log(iotests.filter_qmp_event(vm.event_wait('JOB_STATUS_CHANGE'))) ++ iotests.log(iotests.filter_qmp_event(vm.event_wait('JOB_STATUS_CHANGE'))) + + if not job_args.get('auto-finalize', True): + # PENDING state: +@@ -218,7 +221,7 @@ with iotests.FilePath('disk.img') as disk_path, \ + + for auto_finalize in [True, False]: + for auto_dismiss in [True, False]: +- test_job_lifecycle(vm, 'drive-backup', job_args={ ++ test_job_lifecycle(vm, 'drive-backup', is_mirror=True, job_args={ + 'device': 'drive0-node', + 'target': copy_path, + 'sync': 'full', +diff --git a/tests/qemu-iotests/219.out b/tests/qemu-iotests/219.out +index 8ebd3fee60..0ea5d0b9d5 100644 +--- a/tests/qemu-iotests/219.out ++++ b/tests/qemu-iotests/219.out +@@ -135,6 +135,8 @@ Pause/resume in RUNNING + {"return": {}} + + Waiting for PENDING state... ++{"data": {"id": "job0", "status": "paused"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} ++{"data": {"id": "job0", "status": "running"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} + {"data": {"id": "job0", "status": "waiting"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} + {"data": {"id": "job0", "status": "pending"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} + {"data": {"id": "job0", "status": "concluded"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} +@@ -186,6 +188,8 @@ Pause/resume in RUNNING + {"return": {}} + + Waiting for PENDING state... ++{"data": {"id": "job0", "status": "paused"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} ++{"data": {"id": "job0", "status": "running"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} + {"data": {"id": "job0", "status": "waiting"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} + {"data": {"id": "job0", "status": "pending"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} + {"data": {"id": "job0", "status": "concluded"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} +@@ -245,6 +249,8 @@ Pause/resume in RUNNING + {"return": {}} + + Waiting for PENDING state... ++{"data": {"id": "job0", "status": "paused"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} ++{"data": {"id": "job0", "status": "running"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} + {"data": {"id": "job0", "status": "waiting"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} + {"data": {"id": "job0", "status": "pending"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} + {"return": [{"current-progress": 4194304, "id": "job0", "status": "pending", "total-progress": 4194304, "type": "backup"}]} +@@ -304,6 +310,8 @@ Pause/resume in RUNNING + {"return": {}} + + Waiting for PENDING state... ++{"data": {"id": "job0", "status": "paused"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} ++{"data": {"id": "job0", "status": "running"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} + {"data": {"id": "job0", "status": "waiting"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} + {"data": {"id": "job0", "status": "pending"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} + {"return": [{"current-progress": 4194304, "id": "job0", "status": "pending", "total-progress": 4194304, "type": "backup"}]} +-- +2.27.0 + -- Gitee From 26ad20e4a1fb43345de79a20566f6385cb45b937 Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Wed, 8 Jan 2020 15:31:33 +0100 Subject: [PATCH 18/23] blockdev: unify qmp_blockdev_backup and blockdev-backup transaction paths Issuing a blockdev-backup from qmp_blockdev_backup takes a slightly different path than when it's issued from a transaction. In the code, this is manifested as some redundancy between do_blockdev_backup() and blockdev_backup_prepare(). This change unifies both paths, merging do_blockdev_backup() and blockdev_backup_prepare(), and changing qmp_blockdev_backup() to create a transaction instead of calling do_backup_common() direcly. As a side-effect, now qmp_blockdev_backup() is executed inside a drained section, as it happens when creating a blockdev-backup transaction. This change is visible from the user's perspective, as the job gets paused and immediately resumed before starting the actual work. Signed-off-by: Sergio Lopez Reviewed-by: Max Reitz Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- ...mp_blockdev_backup-and-blockdev-back.patch | 131 ++++++++++++++++++ 1 file changed, 131 insertions(+) create mode 100644 blockdev-unify-qmp_blockdev_backup-and-blockdev-back.patch diff --git a/blockdev-unify-qmp_blockdev_backup-and-blockdev-back.patch b/blockdev-unify-qmp_blockdev_backup-and-blockdev-back.patch new file mode 100644 index 0000000..84e29ff --- /dev/null +++ b/blockdev-unify-qmp_blockdev_backup-and-blockdev-back.patch @@ -0,0 +1,131 @@ +From 6d89e4923e9c341975dbfdd2bae153ba367a1b79 Mon Sep 17 00:00:00 2001 +From: Sergio Lopez +Date: Wed, 8 Jan 2020 15:31:33 +0100 +Subject: [PATCH] blockdev: unify qmp_blockdev_backup and blockdev-backup + transaction paths + +Issuing a blockdev-backup from qmp_blockdev_backup takes a slightly +different path than when it's issued from a transaction. In the code, +this is manifested as some redundancy between do_blockdev_backup() and +blockdev_backup_prepare(). + +This change unifies both paths, merging do_blockdev_backup() and +blockdev_backup_prepare(), and changing qmp_blockdev_backup() to +create a transaction instead of calling do_backup_common() direcly. + +As a side-effect, now qmp_blockdev_backup() is executed inside a +drained section, as it happens when creating a blockdev-backup +transaction. This change is visible from the user's perspective, as +the job gets paused and immediately resumed before starting the actual +work. + +Signed-off-by: Sergio Lopez +Reviewed-by: Max Reitz +Reviewed-by: Kevin Wolf +Signed-off-by: Kevin Wolf +--- + blockdev.c | 60 ++++++++++++------------------------------------------ + 1 file changed, 13 insertions(+), 47 deletions(-) + +diff --git a/blockdev.c b/blockdev.c +index 7016054688..d3309c205a 100644 +--- a/blockdev.c ++++ b/blockdev.c +@@ -1983,16 +1983,13 @@ typedef struct BlockdevBackupState { + BlockJob *job; + } BlockdevBackupState; + +-static BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn, +- Error **errp); +- + static void blockdev_backup_prepare(BlkActionState *common, Error **errp) + { + BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); + BlockdevBackup *backup; +- BlockDriverState *bs, *target; ++ BlockDriverState *bs; ++ BlockDriverState *target_bs; + AioContext *aio_context; +- Error *local_err = NULL; + + assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP); + backup = common->action->u.blockdev_backup.data; +@@ -2002,8 +1999,8 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp) + return; + } + +- target = bdrv_lookup_bs(backup->target, backup->target, errp); +- if (!target) { ++ target_bs = bdrv_lookup_bs(backup->target, backup->target, errp); ++ if (!target_bs) { + return; + } + +@@ -2014,13 +2011,10 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp) + /* Paired with .clean() */ + bdrv_drained_begin(state->bs); + +- state->job = do_blockdev_backup(backup, common->block_job_txn, &local_err); +- if (local_err) { +- error_propagate(errp, local_err); +- goto out; +- } ++ state->job = do_backup_common(qapi_BlockdevBackup_base(backup), ++ bs, target_bs, aio_context, ++ common->block_job_txn, errp); + +-out: + aio_context_release(aio_context); + } + +@@ -3672,41 +3666,13 @@ XDbgBlockGraph *qmp_x_debug_query_block_graph(Error **errp) + return bdrv_get_xdbg_block_graph(errp); + } + +-BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn, +- Error **errp) ++void qmp_blockdev_backup(BlockdevBackup *backup, Error **errp) + { +- BlockDriverState *bs; +- BlockDriverState *target_bs; +- AioContext *aio_context; +- BlockJob *job; +- +- bs = bdrv_lookup_bs(backup->device, backup->device, errp); +- if (!bs) { +- return NULL; +- } +- +- target_bs = bdrv_lookup_bs(backup->target, backup->target, errp); +- if (!target_bs) { +- return NULL; +- } +- +- aio_context = bdrv_get_aio_context(bs); +- aio_context_acquire(aio_context); +- +- job = do_backup_common(qapi_BlockdevBackup_base(backup), +- bs, target_bs, aio_context, txn, errp); +- +- aio_context_release(aio_context); +- return job; +-} +- +-void qmp_blockdev_backup(BlockdevBackup *arg, Error **errp) +-{ +- BlockJob *job; +- job = do_blockdev_backup(arg, NULL, errp); +- if (job) { +- job_start(&job->job); +- } ++ TransactionAction action = { ++ .type = TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP, ++ .u.blockdev_backup.data = backup, ++ }; ++ blockdev_do_action(&action, errp); + } + + /* Parameter check and block job starting for drive mirroring. +-- +2.27.0 + -- Gitee From 32605c9dc362a4fa9e6483528a3fc34be9907903 Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Wed, 8 Jan 2020 15:31:34 +0100 Subject: [PATCH 19/23] blockdev: honor bdrv_try_set_aio_context() context requirements bdrv_try_set_aio_context() requires that the old context is held, and the new context is not held. Fix all the occurrences where it's not done this way. Suggested-by: Max Reitz Signed-off-by: Sergio Lopez Signed-off-by: Kevin Wolf --- ...drv_try_set_aio_context-context-requ.patch | 191 ++++++++++++++++++ 1 file changed, 191 insertions(+) create mode 100644 blockdev-honor-bdrv_try_set_aio_context-context-requ.patch diff --git a/blockdev-honor-bdrv_try_set_aio_context-context-requ.patch b/blockdev-honor-bdrv_try_set_aio_context-context-requ.patch new file mode 100644 index 0000000..9700571 --- /dev/null +++ b/blockdev-honor-bdrv_try_set_aio_context-context-requ.patch @@ -0,0 +1,191 @@ +From 64c6b3b911f65c19f3a235c8394f5db894c1ee6a Mon Sep 17 00:00:00 2001 +From: Sergio Lopez +Date: Wed, 8 Jan 2020 15:31:34 +0100 +Subject: [PATCH] blockdev: honor bdrv_try_set_aio_context() context + requirements + +bdrv_try_set_aio_context() requires that the old context is held, and +the new context is not held. Fix all the occurrences where it's not +done this way. + +Suggested-by: Max Reitz +Signed-off-by: Sergio Lopez +Signed-off-by: Kevin Wolf +--- + blockdev.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++------- + 1 file changed, 60 insertions(+), 8 deletions(-) + +diff --git a/blockdev.c b/blockdev.c +index d3309c205a..5088541591 100644 +--- a/blockdev.c ++++ b/blockdev.c +@@ -1578,6 +1578,7 @@ static void external_snapshot_prepare(BlkActionState *common, + DO_UPCAST(ExternalSnapshotState, common, common); + TransactionAction *action = common->action; + AioContext *aio_context; ++ AioContext *old_context; + int ret; + + /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar +@@ -1718,7 +1719,16 @@ static void external_snapshot_prepare(BlkActionState *common, + goto out; + } + ++ /* Honor bdrv_try_set_aio_context() context acquisition requirements. */ ++ old_context = bdrv_get_aio_context(state->new_bs); ++ aio_context_release(aio_context); ++ aio_context_acquire(old_context); ++ + ret = bdrv_try_set_aio_context(state->new_bs, aio_context, errp); ++ ++ aio_context_release(old_context); ++ aio_context_acquire(aio_context); ++ + if (ret < 0) { + goto out; + } +@@ -1818,11 +1828,13 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) + BlockDriverState *target_bs; + BlockDriverState *source = NULL; + AioContext *aio_context; ++ AioContext *old_context; + QDict *options; + Error *local_err = NULL; + int flags; + int64_t size; + bool set_backing_hd = false; ++ int ret; + + assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP); + backup = common->action->u.drive_backup.data; +@@ -1911,6 +1923,21 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) + goto out; + } + ++ /* Honor bdrv_try_set_aio_context() context acquisition requirements. */ ++ old_context = bdrv_get_aio_context(target_bs); ++ aio_context_release(aio_context); ++ aio_context_acquire(old_context); ++ ++ ret = bdrv_try_set_aio_context(target_bs, aio_context, errp); ++ if (ret < 0) { ++ bdrv_unref(target_bs); ++ aio_context_release(old_context); ++ return; ++ } ++ ++ aio_context_release(old_context); ++ aio_context_acquire(aio_context); ++ + if (set_backing_hd) { + bdrv_set_backing_hd(target_bs, source, &local_err); + if (local_err) { +@@ -1990,6 +2017,8 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp) + BlockDriverState *bs; + BlockDriverState *target_bs; + AioContext *aio_context; ++ AioContext *old_context; ++ int ret; + + assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP); + backup = common->action->u.blockdev_backup.data; +@@ -2004,7 +2033,18 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp) + return; + } + ++ /* Honor bdrv_try_set_aio_context() context acquisition requirements. */ + aio_context = bdrv_get_aio_context(bs); ++ old_context = bdrv_get_aio_context(target_bs); ++ aio_context_acquire(old_context); ++ ++ ret = bdrv_try_set_aio_context(target_bs, aio_context, errp); ++ if (ret < 0) { ++ aio_context_release(old_context); ++ return; ++ } ++ ++ aio_context_release(old_context); + aio_context_acquire(aio_context); + state->bs = bs; + +@@ -3562,7 +3602,6 @@ static BlockJob *do_backup_common(BackupCommon *backup, + BlockJob *job = NULL; + BdrvDirtyBitmap *bmap = NULL; + int job_flags = JOB_DEFAULT; +- int ret; + + if (!backup->has_speed) { + backup->speed = 0; +@@ -3586,11 +3625,6 @@ static BlockJob *do_backup_common(BackupCommon *backup, + backup->compress = false; + } + +- ret = bdrv_try_set_aio_context(target_bs, aio_context, errp); +- if (ret < 0) { +- return NULL; +- } +- + if ((backup->sync == MIRROR_SYNC_MODE_BITMAP) || + (backup->sync == MIRROR_SYNC_MODE_INCREMENTAL)) { + /* done before desugaring 'incremental' to print the right message */ +@@ -3802,6 +3836,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp) + BlockDriverState *bs; + BlockDriverState *source, *target_bs; + AioContext *aio_context; ++ AioContext *old_context; + BlockMirrorBackingMode backing_mode; + Error *local_err = NULL; + QDict *options = NULL; +@@ -3914,12 +3949,22 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp) + (arg->mode == NEW_IMAGE_MODE_EXISTING || + !bdrv_has_zero_init(target_bs))); + ++ ++ /* Honor bdrv_try_set_aio_context() context acquisition requirements. */ ++ old_context = bdrv_get_aio_context(target_bs); ++ aio_context_release(aio_context); ++ aio_context_acquire(old_context); ++ + ret = bdrv_try_set_aio_context(target_bs, aio_context, errp); + if (ret < 0) { + bdrv_unref(target_bs); +- goto out; ++ aio_context_release(old_context); ++ return; + } + ++ aio_context_release(old_context); ++ aio_context_acquire(aio_context); ++ + blockdev_mirror_common(arg->has_job_id ? arg->job_id : NULL, bs, target_bs, + arg->has_replaces, arg->replaces, arg->sync, + backing_mode, zero_target, +@@ -3961,6 +4006,7 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id, + BlockDriverState *bs; + BlockDriverState *target_bs; + AioContext *aio_context; ++ AioContext *old_context; + BlockMirrorBackingMode backing_mode = MIRROR_LEAVE_BACKING_CHAIN; + Error *local_err = NULL; + bool zero_target; +@@ -3978,10 +4024,16 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id, + + zero_target = (sync == MIRROR_SYNC_MODE_FULL); + ++ /* Honor bdrv_try_set_aio_context() context acquisition requirements. */ ++ old_context = bdrv_get_aio_context(target_bs); + aio_context = bdrv_get_aio_context(bs); +- aio_context_acquire(aio_context); ++ aio_context_acquire(old_context); + + ret = bdrv_try_set_aio_context(target_bs, aio_context, errp); ++ ++ aio_context_release(old_context); ++ aio_context_acquire(aio_context); ++ + if (ret < 0) { + goto out; + } +-- +2.27.0 + -- Gitee From 4f28b80853ce202ed54444ffb322706e69be9c48 Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Wed, 8 Jan 2020 15:31:37 +0100 Subject: [PATCH 20/23] blockdev: Return bs to the proper context on snapshot abort external_snapshot_abort() calls to bdrv_set_backing_hd(), which returns state->old_bs to the main AioContext, as it's intended to be used then the BDS is going to be released. As that's not the case when aborting an external snapshot, return it to the AioContext it was before the call. This issue can be triggered by issuing a transaction with two actions, a proper blockdev-snapshot-sync and a bogus one, so the second will trigger a transaction abort. This results in a crash with an stack trace like this one: #0 0x00007fa1048b28df in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 #1 0x00007fa10489ccf5 in __GI_abort () at abort.c:79 #2 0x00007fa10489cbc9 in __assert_fail_base (fmt=0x7fa104a03300 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x5572240b44d8 "bdrv_get_aio_context(old_bs) == bdrv_get_aio_context(new_bs)", file=0x557224014d30 "block.c", line=2240, function=) at assert.c:92 #3 0x00007fa1048aae96 in __GI___assert_fail (assertion=assertion@entry=0x5572240b44d8 "bdrv_get_aio_context(old_bs) == bdrv_get_aio_context(new_bs)", file=file@entry=0x557224014d30 "block.c", line=line@entry=2240, function=function@entry=0x5572240b5d60 <__PRETTY_FUNCTION__.31620> "bdrv_replace_child_noperm") at assert.c:101 #4 0x0000557223e631f8 in bdrv_replace_child_noperm (child=0x557225b9c980, new_bs=new_bs@entry=0x557225c42e40) at block.c:2240 #5 0x0000557223e68be7 in bdrv_replace_node (from=0x557226951a60, to=0x557225c42e40, errp=0x5572247d6138 ) at block.c:4196 #6 0x0000557223d069c4 in external_snapshot_abort (common=0x557225d7e170) at blockdev.c:1731 #7 0x0000557223d069c4 in external_snapshot_abort (common=0x557225d7e170) at blockdev.c:1717 #8 0x0000557223d09013 in qmp_transaction (dev_list=, has_props=, props=0x557225cc7d70, errp=errp@entry=0x7ffe704c0c98) at blockdev.c:2360 #9 0x0000557223e32085 in qmp_marshal_transaction (args=, ret=, errp=0x7ffe704c0d08) at qapi/qapi-commands-transaction.c:44 #10 0x0000557223ee798c in do_qmp_dispatch (errp=0x7ffe704c0d00, allow_oob=, request=, cmds=0x5572247d3cc0 ) at qapi/qmp-dispatch.c:132 #11 0x0000557223ee798c in qmp_dispatch (cmds=0x5572247d3cc0 , request=, allow_oob=) at qapi/qmp-dispatch.c:175 #12 0x0000557223e06141 in monitor_qmp_dispatch (mon=0x557225c69ff0, req=) at monitor/qmp.c:120 #13 0x0000557223e0678a in monitor_qmp_bh_dispatcher (data=) at monitor/qmp.c:209 #14 0x0000557223f2f366 in aio_bh_call (bh=0x557225b9dc60) at util/async.c:117 #15 0x0000557223f2f366 in aio_bh_poll (ctx=ctx@entry=0x557225b9c840) at util/async.c:117 #16 0x0000557223f32754 in aio_dispatch (ctx=0x557225b9c840) at util/aio-posix.c:459 #17 0x0000557223f2f242 in aio_ctx_dispatch (source=, callback=, user_data=) at util/async.c:260 #18 0x00007fa10913467d in g_main_dispatch (context=0x557225c28e80) at gmain.c:3176 #19 0x00007fa10913467d in g_main_context_dispatch (context=context@entry=0x557225c28e80) at gmain.c:3829 #20 0x0000557223f31808 in glib_pollfds_poll () at util/main-loop.c:219 #21 0x0000557223f31808 in os_host_main_loop_wait (timeout=) at util/main-loop.c:242 #22 0x0000557223f31808 in main_loop_wait (nonblocking=) at util/main-loop.c:518 #23 0x0000557223d13201 in main_loop () at vl.c:1828 #24 0x0000557223bbfb82 in main (argc=, argv=, envp=) at vl.c:4504 RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1779036 Signed-off-by: Sergio Lopez Signed-off-by: Kevin Wolf --- ...bs-to-the-proper-context-on-snapshot.patch | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 blockdev-Return-bs-to-the-proper-context-on-snapshot.patch diff --git a/blockdev-Return-bs-to-the-proper-context-on-snapshot.patch b/blockdev-Return-bs-to-the-proper-context-on-snapshot.patch new file mode 100644 index 0000000..a232c74 --- /dev/null +++ b/blockdev-Return-bs-to-the-proper-context-on-snapshot.patch @@ -0,0 +1,93 @@ +From dc6b61f12750b3ab5a3965af2ec758750389233d Mon Sep 17 00:00:00 2001 +From: Sergio Lopez +Date: Wed, 8 Jan 2020 15:31:37 +0100 +Subject: [PATCH] blockdev: Return bs to the proper context on snapshot abort + +external_snapshot_abort() calls to bdrv_set_backing_hd(), which +returns state->old_bs to the main AioContext, as it's intended to be +used then the BDS is going to be released. As that's not the case when +aborting an external snapshot, return it to the AioContext it was +before the call. + +This issue can be triggered by issuing a transaction with two actions, +a proper blockdev-snapshot-sync and a bogus one, so the second will +trigger a transaction abort. This results in a crash with an stack +trace like this one: + + #0 0x00007fa1048b28df in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 + #1 0x00007fa10489ccf5 in __GI_abort () at abort.c:79 + #2 0x00007fa10489cbc9 in __assert_fail_base + (fmt=0x7fa104a03300 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x5572240b44d8 "bdrv_get_aio_context(old_bs) == bdrv_get_aio_context(new_bs)", file=0x557224014d30 "block.c", line=2240, function=) at assert.c:92 + #3 0x00007fa1048aae96 in __GI___assert_fail + (assertion=assertion@entry=0x5572240b44d8 "bdrv_get_aio_context(old_bs) == bdrv_get_aio_context(new_bs)", file=file@entry=0x557224014d30 "block.c", line=line@entry=2240, function=function@entry=0x5572240b5d60 <__PRETTY_FUNCTION__.31620> "bdrv_replace_child_noperm") at assert.c:101 + #4 0x0000557223e631f8 in bdrv_replace_child_noperm (child=0x557225b9c980, new_bs=new_bs@entry=0x557225c42e40) at block.c:2240 + #5 0x0000557223e68be7 in bdrv_replace_node (from=0x557226951a60, to=0x557225c42e40, errp=0x5572247d6138 ) at block.c:4196 + #6 0x0000557223d069c4 in external_snapshot_abort (common=0x557225d7e170) at blockdev.c:1731 + #7 0x0000557223d069c4 in external_snapshot_abort (common=0x557225d7e170) at blockdev.c:1717 + #8 0x0000557223d09013 in qmp_transaction (dev_list=, has_props=, props=0x557225cc7d70, errp=errp@entry=0x7ffe704c0c98) at blockdev.c:2360 + #9 0x0000557223e32085 in qmp_marshal_transaction (args=, ret=, errp=0x7ffe704c0d08) at qapi/qapi-commands-transaction.c:44 + #10 0x0000557223ee798c in do_qmp_dispatch (errp=0x7ffe704c0d00, allow_oob=, request=, cmds=0x5572247d3cc0 ) at qapi/qmp-dispatch.c:132 + #11 0x0000557223ee798c in qmp_dispatch (cmds=0x5572247d3cc0 , request=, allow_oob=) at qapi/qmp-dispatch.c:175 + #12 0x0000557223e06141 in monitor_qmp_dispatch (mon=0x557225c69ff0, req=) at monitor/qmp.c:120 + #13 0x0000557223e0678a in monitor_qmp_bh_dispatcher (data=) at monitor/qmp.c:209 + #14 0x0000557223f2f366 in aio_bh_call (bh=0x557225b9dc60) at util/async.c:117 + #15 0x0000557223f2f366 in aio_bh_poll (ctx=ctx@entry=0x557225b9c840) at util/async.c:117 + #16 0x0000557223f32754 in aio_dispatch (ctx=0x557225b9c840) at util/aio-posix.c:459 + #17 0x0000557223f2f242 in aio_ctx_dispatch (source=, callback=, user_data=) at util/async.c:260 + #18 0x00007fa10913467d in g_main_dispatch (context=0x557225c28e80) at gmain.c:3176 + #19 0x00007fa10913467d in g_main_context_dispatch (context=context@entry=0x557225c28e80) at gmain.c:3829 + #20 0x0000557223f31808 in glib_pollfds_poll () at util/main-loop.c:219 + #21 0x0000557223f31808 in os_host_main_loop_wait (timeout=) at util/main-loop.c:242 + #22 0x0000557223f31808 in main_loop_wait (nonblocking=) at util/main-loop.c:518 + #23 0x0000557223d13201 in main_loop () at vl.c:1828 + #24 0x0000557223bbfb82 in main (argc=, argv=, envp=) at vl.c:4504 + +RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1779036 +Signed-off-by: Sergio Lopez +Signed-off-by: Kevin Wolf +--- + blockdev.c | 21 +++++++++++++++++++++ + 1 file changed, 21 insertions(+) + +diff --git a/blockdev.c b/blockdev.c +index 5088541591..79112be2e6 100644 +--- a/blockdev.c ++++ b/blockdev.c +@@ -1774,6 +1774,8 @@ static void external_snapshot_abort(BlkActionState *common) + if (state->new_bs) { + if (state->overlay_appended) { + AioContext *aio_context; ++ AioContext *tmp_context; ++ int ret; + + aio_context = bdrv_get_aio_context(state->old_bs); + aio_context_acquire(aio_context); +@@ -1781,6 +1783,25 @@ static void external_snapshot_abort(BlkActionState *common) + bdrv_ref(state->old_bs); /* we can't let bdrv_set_backind_hd() + close state->old_bs; we need it */ + bdrv_set_backing_hd(state->new_bs, NULL, &error_abort); ++ ++ /* ++ * The call to bdrv_set_backing_hd() above returns state->old_bs to ++ * the main AioContext. As we're still going to be using it, return ++ * it to the AioContext it was before. ++ */ ++ tmp_context = bdrv_get_aio_context(state->old_bs); ++ if (aio_context != tmp_context) { ++ aio_context_release(aio_context); ++ aio_context_acquire(tmp_context); ++ ++ ret = bdrv_try_set_aio_context(state->old_bs, ++ aio_context, NULL); ++ assert(ret == 0); ++ ++ aio_context_release(tmp_context); ++ aio_context_acquire(aio_context); ++ } ++ + bdrv_replace_node(state->new_bs, state->old_bs, &error_abort); + bdrv_unref(state->old_bs); /* bdrv_replace_node() ref'ed old_bs */ + +-- +2.27.0 + -- Gitee From bdfa280bbefbd6f255cf80025b584dfad3301ebf Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Tue, 10 Mar 2020 12:38:29 +0100 Subject: [PATCH 21/23] block: Fix cross-AioContext blockdev-snapshot external_snapshot_prepare() tries to move the overlay to the AioContext of the backing file (the snapshotted node). However, it's possible that this doesn't work, but the backing file can instead be moved to the overlay's AioContext (e.g. opening the backing chain for a mirror target). bdrv_append() already indirectly uses bdrv_attach_node(), which takes care to move nodes to make sure they use the same AioContext and which tries both directions. So the problem has a simple fix: Just delete the unnecessary extra bdrv_try_set_aio_context() call in external_snapshot_prepare() and instead assert in bdrv_append() that both nodes were indeed moved to the same AioContext. Signed-off-by: Kevin Wolf Message-Id: <20200310113831.27293-6-kwolf@redhat.com> Tested-by: Peter Krempa Signed-off-by: Kevin Wolf --- ...x-cross-AioContext-blockdev-snapshot.patch | 78 +++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 block-Fix-cross-AioContext-blockdev-snapshot.patch diff --git a/block-Fix-cross-AioContext-blockdev-snapshot.patch b/block-Fix-cross-AioContext-blockdev-snapshot.patch new file mode 100644 index 0000000..a4a4d9d --- /dev/null +++ b/block-Fix-cross-AioContext-blockdev-snapshot.patch @@ -0,0 +1,78 @@ +From ec96b9f64c239736003413d70dc3999ad0b8271c Mon Sep 17 00:00:00 2001 +From: Kevin Wolf +Date: Tue, 10 Mar 2020 12:38:29 +0100 +Subject: [PATCH] block: Fix cross-AioContext blockdev-snapshot + +external_snapshot_prepare() tries to move the overlay to the AioContext +of the backing file (the snapshotted node). However, it's possible that +this doesn't work, but the backing file can instead be moved to the +overlay's AioContext (e.g. opening the backing chain for a mirror +target). + +bdrv_append() already indirectly uses bdrv_attach_node(), which takes +care to move nodes to make sure they use the same AioContext and which +tries both directions. + +So the problem has a simple fix: Just delete the unnecessary extra +bdrv_try_set_aio_context() call in external_snapshot_prepare() and +instead assert in bdrv_append() that both nodes were indeed moved to the +same AioContext. + +Signed-off-by: Kevin Wolf +Message-Id: <20200310113831.27293-6-kwolf@redhat.com> +Tested-by: Peter Krempa +Signed-off-by: Kevin Wolf +--- + block.c | 1 + + blockdev.c | 16 ---------------- + 2 files changed, 1 insertion(+), 16 deletions(-) + +diff --git a/block.c b/block.c +index ba36b53a00..824025f781 100644 +--- a/block.c ++++ b/block.c +@@ -4165,6 +4165,7 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, + bdrv_ref(from); + + assert(qemu_get_current_aio_context() == qemu_get_aio_context()); ++ assert(bdrv_get_aio_context(from) == bdrv_get_aio_context(to)); + bdrv_drained_begin(from); + + /* Put all parents into @list and calculate their cumulative permissions */ +diff --git a/blockdev.c b/blockdev.c +index 79112be2e6..d1a3b6a630 100644 +--- a/blockdev.c ++++ b/blockdev.c +@@ -1578,8 +1578,6 @@ static void external_snapshot_prepare(BlkActionState *common, + DO_UPCAST(ExternalSnapshotState, common, common); + TransactionAction *action = common->action; + AioContext *aio_context; +- AioContext *old_context; +- int ret; + + /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar + * purpose but a different set of parameters */ +@@ -1719,20 +1717,6 @@ static void external_snapshot_prepare(BlkActionState *common, + goto out; + } + +- /* Honor bdrv_try_set_aio_context() context acquisition requirements. */ +- old_context = bdrv_get_aio_context(state->new_bs); +- aio_context_release(aio_context); +- aio_context_acquire(old_context); +- +- ret = bdrv_try_set_aio_context(state->new_bs, aio_context, errp); +- +- aio_context_release(old_context); +- aio_context_acquire(aio_context); +- +- if (ret < 0) { +- goto out; +- } +- + /* This removes our old bs and adds the new bs. This is an operation that + * can fail, so we need to do it in .prepare; undoing it for abort is + * always possible. */ +-- +2.27.0 + -- Gitee From 81f1d7c169f14a6fe52bf7aceda08cf8d08e7847 Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Thu, 22 Jul 2021 21:28:35 +0800 Subject: [PATCH 22/23] spec: Update patch and changelog with !170 block: backport qemu-4.1 bugfix !170 qapi/block-core: Introduce BackupCommon drive-backup: create do_backup_common blockdev-backup: utilize do_backup_common qapi: add BitmapSyncMode enum block/backup: Add mirror sync mode 'bitmap' block/backup: add 'never' policy to bitmap sync mode block/backup: loosen restriction on readonly bitmaps block/backup: hoist bitmap check into QMP interface block/backup: deal with zero detection mirror: Fix bdrv_has_zero_init() use blockdev: fix coding style issues in drive_backup_prepare blockdev: unify qmp_drive_backup and drive-backup transaction paths blockdev: unify qmp_blockdev_backup and blockdev-backup transaction paths blockdev: honor bdrv_try_set_aio_context() context requirements blockdev: Return bs to the proper context on snapshot abort block: Fix cross-AioContext blockdev-snapshot Signed-off-by: Chen Qun --- qemu.spec | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/qemu.spec b/qemu.spec index a393413..449e333 100644 --- a/qemu.spec +++ b/qemu.spec @@ -394,6 +394,22 @@ Patch0381: target-i386-add-a-ucode-rev-property.patch Patch0382: migration-use-migration_is_active-to-represent-activ.patch Patch0383: migration-Rate-limit-inside-host-pages.patch Patch0384: hw-pci-pcie-Move-hot-plug-capability-check-to-pre_pl.patch +Patch0385: qapi-block-core-Introduce-BackupCommon.patch +Patch0386: drive-backup-create-do_backup_common.patch +Patch0387: blockdev-backup-utilize-do_backup_common.patch +Patch0388: qapi-add-BitmapSyncMode-enum.patch +Patch0389: block-backup-Add-mirror-sync-mode-bitmap.patch +Patch0390: block-backup-add-never-policy-to-bitmap-sync-mode.patch +Patch0391: block-backup-loosen-restriction-on-readonly-bitmaps.patch +Patch0392: block-backup-hoist-bitmap-check-into-QMP-interface.patch +Patch0393: block-backup-deal-with-zero-detection.patch +Patch0394: mirror-Fix-bdrv_has_zero_init-use.patch +Patch0395: blockdev-fix-coding-style-issues-in-drive_backup_pre.patch +Patch0396: blockdev-unify-qmp_drive_backup-and-drive-backup-tra.patch +Patch0397: blockdev-unify-qmp_blockdev_backup-and-blockdev-back.patch +Patch0398: blockdev-honor-bdrv_try_set_aio_context-context-requ.patch +Patch0399: blockdev-Return-bs-to-the-proper-context-on-snapshot.patch +Patch0400: block-Fix-cross-AioContext-blockdev-snapshot.patch BuildRequires: flex BuildRequires: gcc @@ -788,6 +804,24 @@ getent passwd qemu >/dev/null || \ %endif %changelog +* Thu Jul 22 2021 Chen Qun +- qapi/block-core: Introduce BackupCommon +- drive-backup: create do_backup_common +- blockdev-backup: utilize do_backup_common +- qapi: add BitmapSyncMode enum +- block/backup: Add mirror sync mode 'bitmap' +- block/backup: add 'never' policy to bitmap sync mode +- block/backup: loosen restriction on readonly bitmaps +- block/backup: hoist bitmap check into QMP interface +- block/backup: deal with zero detection +- mirror: Fix bdrv_has_zero_init() use +- blockdev: fix coding style issues in drive_backup_prepare +- blockdev: unify qmp_drive_backup and drive-backup transaction paths +- blockdev: unify qmp_blockdev_backup and blockdev-backup transaction paths +- blockdev: honor bdrv_try_set_aio_context() context requirements +- blockdev: Return bs to the proper context on snapshot abort +- block: Fix cross-AioContext blockdev-snapshot + * Thu Jul 22 2021 Chen Qun - hw/pci/pcie: Move hot plug capability check to pre_plug callback -- Gitee From 02c35518f2fc8ff8eb09f9d2f545b59b2eecf203 Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Thu, 22 Jul 2021 21:28:35 +0800 Subject: [PATCH 23/23] spec: Update release version with !168 !169 !170 increase release verison by one Signed-off-by: Chen Qun --- qemu.spec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qemu.spec b/qemu.spec index 449e333..c809b46 100644 --- a/qemu.spec +++ b/qemu.spec @@ -1,6 +1,6 @@ Name: qemu Version: 4.1.0 -Release: 68 +Release: 69 Epoch: 2 Summary: QEMU is a generic and open source machine emulator and virtualizer License: GPLv2 and BSD and MIT and CC-BY-SA-4.0 -- Gitee