public inbox for pve-devel@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal