From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 4DC9820EC8A for ; Mon, 29 Apr 2024 16:27:42 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id ED14912F32; Mon, 29 Apr 2024 16:27:44 +0200 (CEST) From: Fiona Ebner To: pve-devel@lists.proxmox.com Date: Mon, 29 Apr 2024 16:27:29 +0200 Message-Id: <20240429142730.445084-1-f.ebner@proxmox.com> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.066 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pve-devel] [PATCH v2 qemu 1/2] fix #5409: backup: fix copy-before-write timeout X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "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 Signed-off-by: Fiona Ebner --- 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 +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 +Signed-off-by: Fiona Ebner +Tested-by: Friedrich Weber +--- + 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 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 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