rpms/libnbd
7
0
Fork 0
Code Issues Pull Requests Releases Activity

Further optimizations and fixes for nbdcopy --blkhash

Browse Source 
resolves: RHEL-85513
This commit is contained in:
Richard W.M. Jones 2025-04-15 11:34:13 +01:00
parent 6342bfcb83
commit 854fda48eb
5 changed files with 254 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

66
0007-copy-Define-block_type-outside-of-block-struct.patch Normal file
View File

@ -0,0 +1,66 @@
From acc2ec1028879327dde9874b2969c20cd5f50842 Mon Sep 17 00:00:00 2001
From: Nir Soffer <nsoffer@redhat.com>
Date: Sun, 13 Apr 2025 14:51:09 +0000
Subject: [PATCH] copy: Define block_type outside of block struct
This make the code easier to follow and maintain.
(cherry picked from commit dc5f0e6c79e7aa03ba634b71d4780f6d7d039cdd)
---
copy/blkhash.c | 38 ++++++++++++++++++++------------------
1 file changed, 20 insertions(+), 18 deletions(-)
diff --git a/copy/blkhash.c b/copy/blkhash.c
index 622d8a39..526db4d2 100644
--- a/copy/blkhash.c
+++ b/copy/blkhash.c
@@ -43,26 +43,28 @@
#ifdef HAVE_GNUTLS
+/* unknown => We haven't seen this block yet. 'ptr' is NULL.
+ *
+ * zero => The block is all zeroes. 'ptr' is NULL.
+ *
+ * data => The block is all data, and we have seen the whole block,
+ * and the hash has been computed. 'ptr' points to the computed
+ * hash. 'n' is unused.
+ *
+ * incomplete => Part of the block was seen. 'ptr' points to the
+ * data block, waiting to be completed. 'n' is the number of bytes
+ * seen so far. We will compute the hash and turn this into a
+ * 'data' or 'zero' block, either when we have seen all bytes of
+ * this block, or at the end.
+ *
+ * Note that this code assumes that we are called exactly once for a
+ * range in the disk image.
+ */
+enum block_type { block_unknown = 0, block_zero, block_data, block_incomplete };
+
/* We will have one of these structs per blkhash block. */
struct block {
- /* unknown => We haven't seen this block yet. 'ptr' is NULL.
- *
- * zero => The block is all zeroes. 'ptr' is NULL.
- *
- * data => The block is all data, and we have seen the whole block,
- * and the hash has been computed. 'ptr' points to the computed
- * hash. 'n' is unused.
- *
- * incomplete => Part of the block was seen. 'ptr' points to the
- * data block, waiting to be completed. 'n' is the number of bytes
- * seen so far. We will compute the hash and turn this into a
- * 'data' or 'zero' block, either when we have seen all bytes of
- * this block, or at the end.
- *
- * Note that this code assumes that we are called exactly once for a
- * range in the disk image.
- */
- enum { block_unknown = 0, block_zero, block_data, block_incomplete } type;
+ enum block_type type;
void *ptr;
size_t n;
};
--
2.47.1

78
0008-copy-Shrink-struct-block.patch Normal file
View File

@ -0,0 +1,78 @@
From d2a40df593c1f3ebd9d61277c1c4d336e27f1596 Mon Sep 17 00:00:00 2001
From: Nir Soffer <nsoffer@redhat.com>
Date: Sun, 13 Apr 2025 14:54:31 +0000
Subject: [PATCH] copy: Shrink struct block
Change n to uint32_t since block size bigger than 4g does not make
sense. Move the type field to the end to shrink struct size from 24
bytes to 16.
This minimizes memory usage and improves locality. For example we can
have 4 blocks in a single cache line instead of 2.5.
Testing shows up to 8% improvement in time and 33% in maximum resident
set size with 1000g empty image. With images full of zeros or images
full of non-zero bytes we see lower memory usage but no difference in
time.
| size | content | tool | source | version | memory | time |
|--------|---------|------------|--------|---------|----------|----------|
| 1000g | hole | nbdcopy | file | before | 644716k | 3.33s |
| 1000g | hole | nbdcopy | file | after | 516716k | 3.10s |
| 1000g | hole | nbdcopy | nbd | before | 388844k | 1.13s |
| 1000g | hole | nbdcopy | nbd | after | 260716k | 1.04s |
| 1000g | hole | blksum | nbd | - | 10792k | 0.29s |
| 1000g | hole | sha256sum | file | - | *2796k | *445.00s |
|--------|---------|------------|--------|---------|----------|----------|
| 10g | zero | nbdcopy | file | before | 20236k | 1.33s |
| 10g | zero | nbdcopy | file | after | 18796k | 1.32s |
| 10g | zero | nbdcopy | nbd | before | 32648k | 8.21s |
| 10g | zero | nbdcopy | nbd | after | 31416k | 8.23s |
| 10g | zero | nbdcopy | pipe | before | 19052k | 4.56s |
| 10g | zero | nbdcopy | pipe | after | 17772k | 4.56s |
| 10g | zero | blksum | nbd | - | 13948k | 3.90s |
| 10g | zero | blksum | pipe | - | 10340k | 0.55s |
| 10g | zero | sha256sum | file | - | 2796k | 4.45s |
|--------|---------|------------|--------|---------|----------|----------|
| 10g | data | nbdcopy | file | before | 20224k | 1.28s |
| 10g | data | nbdcopy | file | after | 19036k | 1.26s |
| 10g | data | nbdcopy | nbd | before | 32792k | 8.02s |
| 10g | data | nbdcopy | nbd | after | 31512k | 8.02s |
| 10g | data | nbdcopy | pipe | before | 19052k | 4.56s |
| 10g | data | nbdcopy | pipe | after | 17772k | 4.57s |
| 10g | data | blksum | nbd | - | 13888k | 3.88s |
| 10g | data | blksum | pipe | - | 12512k | 1.10s |
| 10g | data | sha256sum | file | - | 2788k | 4.49s |
* estimated based on 10g image
Measured using:
/usr/bin/time -f "memory=%Mk time=%es" ./nbdcopy --blkhash ...
Tested on Fedora 41 VM on MacBook Pro M2 Max.
(cherry picked from commit f3e1b5fe8423558b49a2b829c0fe13f601b475f2)
---
copy/blkhash.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/copy/blkhash.c b/copy/blkhash.c
index 526db4d2..41253ec8 100644
--- a/copy/blkhash.c
+++ b/copy/blkhash.c
@@ -64,9 +64,9 @@ enum block_type { block_unknown = 0, block_zero, block_data, block_incomplete };
/* We will have one of these structs per blkhash block. */
struct block {
- enum block_type type;
void *ptr;
- size_t n;
+ uint32_t n;
+ enum block_type type;
};
DEFINE_VECTOR_TYPE(blocks, struct block);
--
2.47.1

65
0009-copy-Enable-zero-optimization-for-allocated-extents.patch Normal file
View File

@ -0,0 +1,65 @@
From c92dab9825cc4234777583d56dbd11e27b61637f Mon Sep 17 00:00:00 2001
From: Nir Soffer <nsoffer@redhat.com>
Date: Sun, 13 Apr 2025 23:39:15 +0000
Subject: [PATCH] copy: Enable zero optimization for allocated extents
We optimized zero extents but computed the hash for all data blocks,
including data blocks full of zeros. Detecting a zero block is 20-100
times faster than computing a hash, depending on the machine and the
hash algorithm.
When adding a completed block, detect zero blocks and mark the block as
zero block, saving the computation of the hash and the allocation of the
digest buffer.
This optimization is already implemented for incomplete blocks.
Testing shows that computing a hash for image full of zeros is up to 7.4
times faster, and memory usage is up to 40% lower.
| size | content | tool | source | version | memory | time |
|--------|---------|------------|--------|---------|----------|----------|
| 10g | zero | nbdcopy | file | before | 20236k | 1.33s |
| 10g | zero | nbdcopy | file | after | 13212k | 0.33s |
| 10g | zero | nbdcopy | nbd | before | 32648k | 8.21s |
| 10g | zero | nbdcopy | nbd | after | 24996k | 3.32s |
| 10g | zero | nbdcopy | pipe | before | 19052k | 4.56s |
| 10g | zero | nbdcopy | pipe | after | 11244k | 0.61s |
| 10g | zero | blksum | nbd | - | 13948k | 3.90s |
| 10g | zero | blksum | pipe | - | 10340k | 0.55s |
| 10g | zero | sha256sum | file | - | 2796k | 4.45s |
|--------|---------|------------|--------|---------|----------|----------|
| 10g | data | nbdcopy | file | before | 20224k | 1.28s |
| 10g | data | nbdcopy | file | after | 20400k | 1.28s |
| 10g | data | nbdcopy | nbd | before | 32792k | 8.02s |
| 10g | data | nbdcopy | nbd | after | 32536k | 8.01s |
| 10g | data | nbdcopy | pipe | before | 19052k | 4.56s |
| 10g | data | nbdcopy | pipe | after | 19048k | 4.55s |
| 10g | data | blksum | nbd | - | 13888k | 3.88s |
| 10g | data | blksum | pipe | - | 12512k | 1.10s |
| 10g | data | sha256sum | file | - | 2788k | 4.49s |
(cherry picked from commit efbe283f9fcfc8b4e57370f71356b1bfe7ffd0a4)
---
copy/blkhash.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/copy/blkhash.c b/copy/blkhash.c
index 41253ec8..92ffafbd 100644
--- a/copy/blkhash.c
+++ b/copy/blkhash.c
@@ -213,7 +213,10 @@ set_complete_block (uint64_t blknum, const char *buf)
/* Assert that we haven't seen this block before. */
assert (b.type == block_unknown);
- if (buf) {
+ /* Detecting a zero block is 20-100 times faster than computing a hash
+ * depending on the machine and the algorithm.
+ */
+ if (buf && !is_zero (buf, blkhash_size)) {
b.type = block_data;
/* Compute the hash of the whole block now. */
--
2.47.1

39
0010-copy-Fix-corrupted-hash-on-incomplete-read.patch Normal file
View File

@ -0,0 +1,39 @@
From 692d28fd6dc3a74855b25173b39d5e2ad6ddc30f Mon Sep 17 00:00:00 2001
From: Nir Soffer <nsoffer@redhat.com>
Date: Mon, 14 Apr 2025 21:40:16 +0000
Subject: [PATCH] copy: Fix corrupted hash on incomplete read
When using synchronous read with unknown file size, if the read was
shorter than request size, we updated the hash with the complete buffer,
inserting leftover bytes from the previous read into the hash.
I'm not sure if there is validation for source size and number of blocks
in the blocks vector, so this can generate a corrupted hash silently.
We probably need to validate later that the image size matches the size
of the hashed data.
I could not reproduce a corrupted hash, the issue discovered by reading
the code.
(cherry picked from commit 49cd9fbc0022c0ae5bc5d0b9dd48219dfb92b2f7)
---
copy/synch-copying.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/copy/synch-copying.c b/copy/synch-copying.c
index 4c65c86d..b030a85a 100644
--- a/copy/synch-copying.c
+++ b/copy/synch-copying.c
@@ -49,7 +49,7 @@ synch_copying (void)
size_t r;
while ((r = src->ops->synch_read (src, buf, request_size, offset)) > 0) {
- update_blkhash ((const char *) buf, offset, request_size);
+ update_blkhash ((const char *) buf, offset, r);
dst->ops->synch_write (dst, buf, r, offset);
offset += r;
progress_bar (offset, src->size);
--
2.47.1

8
libnbd.spec
View File

@ -21,7 +21,7 @@
Name: libnbd
Version: 1.22.1
Release: 3%{?dist}
Release: 4%{?dist}
Summary: NBD client library in userspace
License: LGPL-2.0-or-later AND BSD-3-Clause
@ -47,6 +47,10 @@ Patch0003: 0003-copy-Set-the-total-size-in-bytes-copied.patch
Patch0004: 0004-info-info-uri-nbds.sh-Fix-test-if-compiled-without-G.patch
Patch0005: 0005-copy-Add-blkhash-option.patch
Patch0006: 0006-copy-Fix-crash-when-blkhash-size-is-not-a-power-of-2.patch
Patch0007: 0007-copy-Define-block_type-outside-of-block-struct.patch
Patch0008: 0008-copy-Shrink-struct-block.patch
Patch0009: 0009-copy-Enable-zero-optimization-for-allocated-extents.patch
Patch0010: 0010-copy-Fix-corrupted-hash-on-incomplete-read.patch
%if 0%{verify_tarball_signature}
BuildRequires: gnupg2
@ -387,7 +391,7 @@ make %{?_smp_mflags} check || {
%changelog
* Mon Apr 07 2025 Richard W.M. Jones <rjones@redhat.com> - 1.22.1-3
* Tue Apr 15 2025 Richard W.M. Jones <rjones@redhat.com> - 1.22.1-4
- Rebase to libnbd 1.22.1
- Synch spec file with Fedora Rawhide.
resolves: RHEL-78831