all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH v2 qemu 1/2] fix #5409: backup: fix copy-before-write timeout
@ 2024-04-29 14:27 Fiona Ebner
  2024-04-29 14:27 ` [pve-devel] [PATCH v2 qemu 2/2] backup: improve error when copy-before-write fails for fleecing Fiona Ebner
  2024-04-29 15:05 ` [pve-devel] applied: [PATCH v2 qemu 1/2] fix #5409: backup: fix copy-before-write timeout Thomas Lamprecht
  0 siblings, 2 replies; 3+ messages in thread
From: Fiona Ebner @ 2024-04-29 14:27 UTC (permalink / raw)
  To: pve-devel

The type for the copy-before-write timeout in nanoseconds was wrong.
By being just uint32_t, a maximum of slightly over 4 seconds was
possible. Larger values would overflow and thus the 45 seconds set by
Proxmox's backup with fleecing, resulted in effectively 2 seconds
timeout for copy-before-write operations.

Reported-by: Friedrich Weber <f.weber@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

New in v2.

Upstream submission of this patch:
https://lore.kernel.org/qemu-devel/20240429141934.442154-1-f.ebner@proxmox.com/T/#u

 ...e-write-use-uint64_t-for-timeout-in-.patch | 35 +++++++++++++++++++
 ...ock-copy-before-write-fix-permission.patch |  2 +-
 ...e-write-support-unligned-snapshot-di.patch |  2 +-
 ...e-write-create-block_copy-bitmap-in-.patch |  2 +-
 ...-backup-add-discard-source-parameter.patch |  4 +--
 ...e-allow-specifying-minimum-cluster-s.patch |  2 +-
 ...um-cluster-size-to-performance-optio.patch |  2 +-
 debian/patches/series                         |  1 +
 8 files changed, 43 insertions(+), 7 deletions(-)
 create mode 100644 debian/patches/extra/0014-block-copy-before-write-use-uint64_t-for-timeout-in-.patch

diff --git a/debian/patches/extra/0014-block-copy-before-write-use-uint64_t-for-timeout-in-.patch b/debian/patches/extra/0014-block-copy-before-write-use-uint64_t-for-timeout-in-.patch
new file mode 100644
index 0000000..b350067
--- /dev/null
+++ b/debian/patches/extra/0014-block-copy-before-write-use-uint64_t-for-timeout-in-.patch
@@ -0,0 +1,35 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Fiona Ebner <f.ebner@proxmox.com>
+Date: Mon, 29 Apr 2024 15:41:11 +0200
+Subject: [PATCH] block/copy-before-write: use uint64_t for timeout in
+ nanoseconds
+
+rather than the uint32_t for which the maximum is slightly more than 4
+seconds and larger values would overflow. The QAPI interface allows
+specifying the number of seconds, so only values 0 to 4 are safe right
+now, other values lead to a much lower timeout than a user expects.
+
+The block_copy() call where this is used already takes a uint64_t for
+the timeout, so no change required there.
+
+Fixes: 6db7fd1ca9 ("block/copy-before-write: implement cbw-timeout option")
+Reported-by: Friedrich Weber <f.weber@proxmox.com>
+Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
+Tested-by: Friedrich Weber <f.weber@proxmox.com>
+---
+ block/copy-before-write.c | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/block/copy-before-write.c b/block/copy-before-write.c
+index b866e42271..3ee95c0e7a 100644
+--- a/block/copy-before-write.c
++++ b/block/copy-before-write.c
+@@ -43,7 +43,7 @@ typedef struct BDRVCopyBeforeWriteState {
+     BlockCopyState *bcs;
+     BdrvChild *target;
+     OnCbwError on_cbw_error;
+-    uint32_t cbw_timeout_ns;
++    uint64_t cbw_timeout_ns;
+ 
+     /*
+      * @lock: protects access to @access_bitmap, @done_bitmap and
diff --git a/debian/patches/pve/0046-block-copy-before-write-fix-permission.patch b/debian/patches/pve/0046-block-copy-before-write-fix-permission.patch
index 1c2e5bd..5703311 100644
--- a/debian/patches/pve/0046-block-copy-before-write-fix-permission.patch
+++ b/debian/patches/pve/0046-block-copy-before-write-fix-permission.patch
@@ -33,7 +33,7 @@ Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
  1 file changed, 7 insertions(+), 3 deletions(-)
 
 diff --git a/block/copy-before-write.c b/block/copy-before-write.c
-index b866e42271..a2dddf6f57 100644
+index 3ee95c0e7a..641236dbf8 100644
 --- a/block/copy-before-write.c
 +++ b/block/copy-before-write.c
 @@ -364,9 +364,13 @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c,
diff --git a/debian/patches/pve/0047-block-copy-before-write-support-unligned-snapshot-di.patch b/debian/patches/pve/0047-block-copy-before-write-support-unligned-snapshot-di.patch
index 4656750..b02025a 100644
--- a/debian/patches/pve/0047-block-copy-before-write-support-unligned-snapshot-di.patch
+++ b/debian/patches/pve/0047-block-copy-before-write-support-unligned-snapshot-di.patch
@@ -15,7 +15,7 @@ Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
  1 file changed, 13 insertions(+), 3 deletions(-)
 
 diff --git a/block/copy-before-write.c b/block/copy-before-write.c
-index a2dddf6f57..0a219c2b75 100644
+index 641236dbf8..71cf50343f 100644
 --- a/block/copy-before-write.c
 +++ b/block/copy-before-write.c
 @@ -325,14 +325,24 @@ static int coroutine_fn GRAPH_RDLOCK
diff --git a/debian/patches/pve/0048-block-copy-before-write-create-block_copy-bitmap-in-.patch b/debian/patches/pve/0048-block-copy-before-write-create-block_copy-bitmap-in-.patch
index ab7c0bf..5148257 100644
--- a/debian/patches/pve/0048-block-copy-before-write-create-block_copy-bitmap-in-.patch
+++ b/debian/patches/pve/0048-block-copy-before-write-create-block_copy-bitmap-in-.patch
@@ -49,7 +49,7 @@ index e13d7bc6b6..b61685f1a2 100644
      if (!copy_bitmap) {
          return NULL;
 diff --git a/block/copy-before-write.c b/block/copy-before-write.c
-index 0a219c2b75..d3b95bd600 100644
+index 71cf50343f..ba827b0aba 100644
 --- a/block/copy-before-write.c
 +++ b/block/copy-before-write.c
 @@ -470,7 +470,7 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
diff --git a/debian/patches/pve/0049-qapi-blockdev-backup-add-discard-source-parameter.patch b/debian/patches/pve/0049-qapi-blockdev-backup-add-discard-source-parameter.patch
index b6eee3e..2152c49 100644
--- a/debian/patches/pve/0049-qapi-blockdev-backup-add-discard-source-parameter.patch
+++ b/debian/patches/pve/0049-qapi-blockdev-backup-add-discard-source-parameter.patch
@@ -109,13 +109,13 @@ index b61685f1a2..3c61e52bae 100644
  }
  
 diff --git a/block/copy-before-write.c b/block/copy-before-write.c
-index d3b95bd600..3503702d71 100644
+index ba827b0aba..2d92ae4043 100644
 --- a/block/copy-before-write.c
 +++ b/block/copy-before-write.c
 @@ -44,6 +44,7 @@ typedef struct BDRVCopyBeforeWriteState {
      BdrvChild *target;
      OnCbwError on_cbw_error;
-     uint32_t cbw_timeout_ns;
+     uint64_t cbw_timeout_ns;
 +    bool discard_source;
  
      /*
diff --git a/debian/patches/pve/0050-copy-before-write-allow-specifying-minimum-cluster-s.patch b/debian/patches/pve/0050-copy-before-write-allow-specifying-minimum-cluster-s.patch
index 17949f4..557cd3a 100644
--- a/debian/patches/pve/0050-copy-before-write-allow-specifying-minimum-cluster-s.patch
+++ b/debian/patches/pve/0050-copy-before-write-allow-specifying-minimum-cluster-s.patch
@@ -82,7 +82,7 @@ index 3c61e52bae..c9a722a5a6 100644
          return NULL;
      }
 diff --git a/block/copy-before-write.c b/block/copy-before-write.c
-index 3503702d71..4a8c5bdb62 100644
+index 2d92ae4043..8a8f7685fa 100644
 --- a/block/copy-before-write.c
 +++ b/block/copy-before-write.c
 @@ -479,7 +479,8 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
diff --git a/debian/patches/pve/0051-backup-add-minimum-cluster-size-to-performance-optio.patch b/debian/patches/pve/0051-backup-add-minimum-cluster-size-to-performance-optio.patch
index d760e45..3ac4b68 100644
--- a/debian/patches/pve/0051-backup-add-minimum-cluster-size-to-performance-optio.patch
+++ b/debian/patches/pve/0051-backup-add-minimum-cluster-size-to-performance-optio.patch
@@ -36,7 +36,7 @@ index 3dc955f625..ac5bd81338 100644
          goto error;
      }
 diff --git a/block/copy-before-write.c b/block/copy-before-write.c
-index 4a8c5bdb62..9ca5ec5e5c 100644
+index 8a8f7685fa..7cc5e4f9b9 100644
 --- a/block/copy-before-write.c
 +++ b/block/copy-before-write.c
 @@ -555,6 +555,7 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
diff --git a/debian/patches/series b/debian/patches/series
index 1680bca..44086f1 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -11,6 +11,7 @@ extra/0010-virtio-scsi-Attach-event-vq-notifier-with-no_poll.patch
 extra/0011-virtio-Re-enable-notifications-after-drain.patch
 extra/0012-qemu_init-increase-NOFILE-soft-limit-on-POSIX.patch
 extra/0013-virtio-blk-avoid-using-ioeventfd-state-in-irqfd-cond.patch
+extra/0014-block-copy-before-write-use-uint64_t-for-timeout-in-.patch
 bitmap-mirror/0001-drive-mirror-add-support-for-sync-bitmap-mode-never.patch
 bitmap-mirror/0002-drive-mirror-add-support-for-conditional-and-always-.patch
 bitmap-mirror/0003-mirror-add-check-for-bitmap-mode-without-bitmap.patch
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2024-04-29 15:05 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-29 14:27 [pve-devel] [PATCH v2 qemu 1/2] fix #5409: backup: fix copy-before-write timeout Fiona Ebner
2024-04-29 14:27 ` [pve-devel] [PATCH v2 qemu 2/2] backup: improve error when copy-before-write fails for fleecing Fiona Ebner
2024-04-29 15:05 ` [pve-devel] applied: [PATCH v2 qemu 1/2] fix #5409: backup: fix copy-before-write timeout Thomas Lamprecht

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal