From 854fda48ebc3437e823a9a370aad857d0f9a72d4 Mon Sep 17 00:00:00 2001 From: "Richard W.M. Jones" Date: Tue, 15 Apr 2025 11:34:13 +0100 Subject: [PATCH] Further optimizations and fixes for nbdcopy --blkhash resolves: RHEL-85513 --- ...e-block_type-outside-of-block-struct.patch | 66 ++++++++++++++++ 0008-copy-Shrink-struct-block.patch | 78 +++++++++++++++++++ ...o-optimization-for-allocated-extents.patch | 65 ++++++++++++++++ ...ix-corrupted-hash-on-incomplete-read.patch | 39 ++++++++++ libnbd.spec | 8 +- 5 files changed, 254 insertions(+), 2 deletions(-) create mode 100644 0007-copy-Define-block_type-outside-of-block-struct.patch create mode 100644 0008-copy-Shrink-struct-block.patch create mode 100644 0009-copy-Enable-zero-optimization-for-allocated-extents.patch create mode 100644 0010-copy-Fix-corrupted-hash-on-incomplete-read.patch diff --git a/0007-copy-Define-block_type-outside-of-block-struct.patch b/0007-copy-Define-block_type-outside-of-block-struct.patch new file mode 100644 index 0000000..0b776aa --- /dev/null +++ b/0007-copy-Define-block_type-outside-of-block-struct.patch @@ -0,0 +1,66 @@ +From acc2ec1028879327dde9874b2969c20cd5f50842 Mon Sep 17 00:00:00 2001 +From: Nir Soffer +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 + diff --git a/0008-copy-Shrink-struct-block.patch b/0008-copy-Shrink-struct-block.patch new file mode 100644 index 0000000..aa71650 --- /dev/null +++ b/0008-copy-Shrink-struct-block.patch @@ -0,0 +1,78 @@ +From d2a40df593c1f3ebd9d61277c1c4d336e27f1596 Mon Sep 17 00:00:00 2001 +From: Nir Soffer +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 + diff --git a/0009-copy-Enable-zero-optimization-for-allocated-extents.patch b/0009-copy-Enable-zero-optimization-for-allocated-extents.patch new file mode 100644 index 0000000..14a6d94 --- /dev/null +++ b/0009-copy-Enable-zero-optimization-for-allocated-extents.patch @@ -0,0 +1,65 @@ +From c92dab9825cc4234777583d56dbd11e27b61637f Mon Sep 17 00:00:00 2001 +From: Nir Soffer +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 + diff --git a/0010-copy-Fix-corrupted-hash-on-incomplete-read.patch b/0010-copy-Fix-corrupted-hash-on-incomplete-read.patch new file mode 100644 index 0000000..2b0ad28 --- /dev/null +++ b/0010-copy-Fix-corrupted-hash-on-incomplete-read.patch @@ -0,0 +1,39 @@ +From 692d28fd6dc3a74855b25173b39d5e2ad6ddc30f Mon Sep 17 00:00:00 2001 +From: Nir Soffer +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 + diff --git a/libnbd.spec b/libnbd.spec index be98b28..01afab5 100644 --- a/libnbd.spec +++ b/libnbd.spec @@ -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 - 1.22.1-3 +* Tue Apr 15 2025 Richard W.M. Jones - 1.22.1-4 - Rebase to libnbd 1.22.1 - Synch spec file with Fedora Rawhide. resolves: RHEL-78831