On 06/29/22 15:36, Richard W.M. Jones wrote:

You're not supposed to read or write NBD servers at a granularity less than the advertised minimum block size. nbdcopy has ignored this requirement, and this is usually fine because the NBD servers we care about support 512-byte sector granularity, and never advertise sizes / extents less granular than sectors (even if it's a bit suboptimal in a few cases).

However there is one new case where we do care: When writing to a compressed qcow2 file, qemu advertises a minimum and preferred block size of 64K, and it really means it. You cannot write blocks smaller than this because of the way qcow2 compression is implemented.

This commit attempts to do the least work possible to fix this.

The previous multi-thread-copying loop was driven by the extent map received from the source. I have modified the loop so that it iterates over request_size blocks. request_size is set from the command line (--request-size) but will be adjusted upwards if either the source or destination preferred block size is larger. So this will always copy blocks which are at least the preferred block size (except for the very last block of the disk).

While copying these blocks we consult the source extent map. If it contains only zero regions covering the whole block (only_zeroes function) then we can skip straight to zeroing the target (fill_dst_range_with_zeroes), else we do read + write as before.

I only modified the multi-thread-copying loop, not the synchronous loop. That should be updated in the same way later.

One side effect of this change is it always makes larger requests, even for regions we know are sparse. This is clear in the copy-sparse.sh and copy-sparse-allocated.sh tests which were previously driven by the 32K sparse map granularity of the source. Without changing these tests, they would make make 256K reads & writes (and also read from areas of the disk even though we know they are sparse). I adjusted these tests to use --request-size=32768 to force the existing behaviour.

Note this doesn't attempt to limit the maximum block size when reading or writing. That is for future work.

This is a partial fix for https://bugzilla.redhat.com/2047660. Further changes will be required in virt-v2v.

Link: https://lists.gnu.org/archive/html/qemu-block/2022-01/threads.html#00729 Link: https://bugzilla.redhat.com/show_bug.cgi?id=2047660 --- TODO | 4 +- copy/Makefile.am | 6 +- copy/copy-file-to-qcow2-compressed.sh | 64 ++++++++++++ copy/copy-sparse-allocated.sh | 4 +- copy/copy-sparse.sh | 7 +- copy/main.c | 13 +++ copy/multi-thread-copying.c | 143 +++++++++++++++++++------- copy/nbdcopy.pod | 5 +- 8 files changed, 197 insertions(+), 49 deletions(-)

I'm going to perform a very rudimentary review of this patch, as a good chunk of it is still over my head. Please bear with me...

diff --git a/TODO b/TODO index 7c9c15e245..bc38d7045e 100644 --- a/TODO +++ b/TODO @@ -28,7 +28,9 @@ Performance: Chart it over various buffer sizes and threads, as that Examine other fuzzers: https://gitlab.com/akihe/radamsa

nbdcopy: - - Minimum/preferred/maximum block size. + - Enforce maximum block size. + - Synchronous loop should be adjusted to take into account + the NBD preferred block size, as was done for multi-thread loop. - Benchmark. - Better page cache usage, see nbdkit-file-plugin options fadvise=sequential cache=none. diff --git a/copy/Makefile.am b/copy/Makefile.am index e729f86a2b..25f75c5706 100644 --- a/copy/Makefile.am +++ b/copy/Makefile.am @@ -23,6 +23,7 @@ EXTRA_DIST = \ copy-file-to-nbd.sh \ copy-file-to-null.sh \ copy-file-to-qcow2.sh \ + copy-file-to-qcow2-compressed.sh \ copy-nbd-to-block.sh \ copy-nbd-to-file.sh \ copy-nbd-to-hexdump.sh \ @@ -142,7 +143,10 @@ TESTS += \ $(NULL)

if HAVE_QEMU_NBD -TESTS += copy-file-to-qcow2.sh +TESTS += \ + copy-file-to-qcow2.sh \ + copy-file-to-qcow2-compressed.sh \ + $(NULL) endif

if HAVE_GNUTLS diff --git a/copy/copy-file-to-qcow2-compressed.sh b/copy/copy-file-to-qcow2-compressed.sh new file mode 100755 index 0000000000..dfe4fa583c --- /dev/null +++ b/copy/copy-file-to-qcow2-compressed.sh @@ -0,0 +1,64 @@ +#!/usr/bin/env bash +# nbd client library in userspace +# Copyright (C) 2020-2022 Red Hat Inc. +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + +. ../tests/functions.sh + +set -e +set -x + +requires $QEMU_NBD --version +requires nbdkit --exit-with-parent --version +requires nbdkit sparse-random --dump-plugin +requires qemu-img --version +requires stat --version + +file1=copy-file-to-qcow2-compressed.file1 +file2=copy-file-to-qcow2-compressed.file2 +rm -f $file1 $file2 +cleanup_fn rm -f $file1 $file2 + +size=1G +seed=$RANDOM + +# Create a compressed qcow2 file1. +# +# sparse-random files should compress easily because by default each +# block uses repeated bytes. +qemu-img create -f qcow2 $file1 $size +nbdcopy -- [ nbdkit --exit-with-parent sparse-random $size seed=$seed ] \ + [ $QEMU_NBD --image-opts driver=compress,file.driver=qcow2,file.file.driver=file,file.file.filename=$file1 ] + +ls -l $file1 + +# Create an uncompressed qcow2 file2 with the same data. +qemu-img create -f qcow2 $file2 $size +nbdcopy -- [ nbdkit --exit-with-parent sparse-random $size seed=$seed ] \ + [ $QEMU_NBD --image-opts driver=qcow2,file.driver=file,file.filename=$file2 ] + +ls -l $file2 + +# file1 < file2 (shows the compression is having some effect). +size1="$( stat -c %s $file1 )" +size2="$( stat -c %s $file2 )" +if [ $size1 -ge $size2 ]; then + echo "$0: qcow2 compression did not make the file smaller" + exit 1 +fi + +# Logical content of the files should be identical. +qemu-img compare -f qcow2 $file1 -F qcow2 $file2

(1) I suggest splitting the new test to a separate patch; to minimize the amount one has to review in one go.

diff --git a/copy/copy-sparse-allocated.sh b/copy/copy-sparse-allocated.sh index 203c3b9850..465e347067 100755 --- a/copy/copy-sparse-allocated.sh +++ b/copy/copy-sparse-allocated.sh @@ -17,8 +17,6 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA

# Adapted from copy-sparse.sh. -# -# This test depends on the nbdkit default sparse block size (32K).

. ../tests/functions.sh

@@ -33,7 +31,7 @@ requires nbdkit eval --version out=copy-sparse-allocated.out cleanup_fn rm -f $out

-$VG nbdcopy --allocated -- \ +$VG nbdcopy --allocated --request-size=32768 -- \ [ nbdkit --exit-with-parent data data=' 1 @1073741823 1 diff --git a/copy/copy-sparse.sh b/copy/copy-sparse.sh index 1a6da868e9..7912a2120a 100755 --- a/copy/copy-sparse.sh +++ b/copy/copy-sparse.sh @@ -16,8 +16,6 @@ # License along with this library; if not, write to the Free Software # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA

-# This test depends on the nbdkit default sparse block size (32K). - . ../tests/functions.sh

set -e @@ -34,8 +32,9 @@ cleanup_fn rm -f $out # Copy from a sparse data disk to an nbdkit-eval-plugin instance which # is logging everything. This allows us to see exactly what nbdcopy # is writing, to ensure it is writing and zeroing the target as -# expected. -$VG nbdcopy -S 0 -- \ +# expected. Force request size to match nbdkit default sparse +# allocator block size (32K). +$VG nbdcopy -S 0 --request-size=32768 -- \ [ nbdkit --exit-with-parent data data=' 1 @1073741823 1 diff --git a/copy/main.c b/copy/main.c index 9b551de664..32e8f370bd 100644 --- a/copy/main.c +++ b/copy/main.c @@ -40,6 +40,7 @@

#include "ispowerof2.h" #include "human-size.h" +#include "minmax.h" #include "version.h" #include "nbdcopy.h"

@@ -379,10 +380,22 @@ main (int argc, char *argv[]) if (threads < connections) connections = threads;

+ /* request_size must always be larger than preferred size of + * source or destination. + */

(2) "larger than" is incorrect here, "at least as large as" is precise.

+ request_size = MAX (request_size, src->preferred); + request_size = MAX (request_size, dst->preferred); + /* Adapt queue to size to request size if needed. */ if (request_size > queue_size) queue_size = request_size;

+ /* Sparse size (if using) must not be smaller than the destination + * preferred size, otherwise we end up creating too small requests. + */ + if (sparse_size) + sparse_size = MAX (sparse_size, dst->preferred); +

(3) Not wrong, but I think this is better expressed as if (sparse_size > 0 && sparse_size < dst->preferred) sparse_size = dst->preferred;

/* Truncate the destination to the same size as the source. Only * has an effect on regular files. */ diff --git a/copy/multi-thread-copying.c b/copy/multi-thread-copying.c index 06cdb8eef6..7194f4fafd 100644 --- a/copy/multi-thread-copying.c +++ b/copy/multi-thread-copying.c @@ -166,6 +166,58 @@ decrease_queue_size (struct worker *worker, size_t len) worker->queue_size -= len; }

+/* Using the extents map 'exts', check if the region + * [offset..offset+len-1] contains only zero extents.

(4) I suggest replacing "contains" with "overlaps" or "intersects with".

+ * + * The invariant for '*i' is always an extent which starts before or + * equal to the current offset. + */ +static bool +only_zeroes (const extent_list exts, size_t *i,

(5) I think it's a bit wasteful (or at least strange) to pass in the entire vector structure; we certainly don't need the "cap" field. Other functions in libnbd seem to take pointers to vectors (which is what I'd expect).

+ uint64_t offset, unsigned len) +{ + size_t j; + + /* Invariant. */ + assert (0 <= *i && *i < exts.len);

(6) I suggest dropping the non-negativity check; size_t is an unsigned type.

+ assert (exts.ptr[*i].offset <= offset); + + /* Update the variant. Search for the last possible extent in the

(7) Typo: s/variant/invariant/

+ * list which is <= offset. + */ + for (j = *i + 1; j < exts.len; ++j) { + if (exts.ptr[j].offset <= offset) + *i = j; + else + break; + } + + /* Check invariant again. */ + assert (0 <= *i && *i < exts.len);

(8) Same comment as (6).

+ assert (exts.ptr[*i].offset <= offset);

(9) We can append another assertion: assert (*i == exts.len - 1 || exts.ptr[*i + 1].offset > offset); I.e., if *i is not the last extent, then the next extent starts strictly beyond "offset"

+ + /* Search forward, look for any non-zero extents covering the region. */

(10) I propose s/covering/overlapping/

+ for (j = *i; j < exts.len; ++j) { + uint64_t start, end; + + /* [start..end] of the current extent. */

(11) The notation is wrong, "end" is exclusive, not inclusive, therefore we should write [start..end) (note the closing paren).

+ start = exts.ptr[j].offset; + end = exts.ptr[j].offset + exts.ptr[j].length; + + if (end <= offset) + continue;

(12) This condition always evaluates to false. Previously, we looked up the "rightmost" extent that is not a successor of "offset", therefore the (exclusive) end of that extent will certainly be greater than "offset". Furthermore, whenever we move to the next extent, the (exclusive) end of that extent will be even greater. The *purpose* of the condition is valid however (see below), so I suggest adding assert (end > offset);

+ + if (start >= offset + len) + break; +

(13) I think we can insert a micro-optimization here: *i = j; Namely: - at this point we have: offset < end && start < offset + len which is precisely the definition of a non-empty intersection. That is, we're sure that the extent and the subject region overlap. - the next entry to this function will be with a subject region that starts (inclusively) where the current subject region ends (exclusively) -- next_request_offset := offset + len. That means the start of the next subject region will *either* fall again into this same extent, *or* into some extent *beyond* the current extent (j). But never into an extent with a smaller subscript than "j". Basically, consider the second subcondition above: start < offset + len If you substitute the above definition for next_request_offset, and expand "start", you get: exts.ptr[j].offset < next_request_offset and that means we need not perform the initial loop for extents that are "to the left" of extent "j". The loop at the start of the function is correct, but here we can save some work for its next execution (in the next invocation of this function).

+ /* Non-zero extent covering this region => test failed. */ + if (!exts.ptr[j].zero) + return false; + } + + return true; +} + /* There are 'threads' worker threads, each copying work ranges from * src to dst until there are no more work ranges. */ @@ -177,7 +229,10 @@ worker_thread (void *wp) extent_list exts = empty_vector;

while (get_next_offset (&offset, &count)) { + struct command *command; size_t i; + bool is_zeroing = false; + uint64_t zeroing_start = 0;

(14) I think we should not initialize "zeroing_start". It suggests that the initializer value is meaningful, but it is not.

assert (0 < count && count <= THREAD_WORK_SIZE); if (extents) @@ -185,52 +240,64 @@ worker_thread (void *wp) else default_get_extents (src, w->index, offset, count, &exts);

- for (i = 0; i < exts.len; ++i) { - struct command *command; - size_t len; + i = 0; // index into extents array

(15) We might as well rename "i" to something more telling, like "extent_index" :)

+ while (count) { + const size_t len = MIN (count, request_size);

- if (exts.ptr[i].zero) { + if (only_zeroes (exts, &i, offset, len)) { /* The source is zero so we can proceed directly to skipping, - * fast zeroing, or writing zeroes at the destination. + * fast zeroing, or writing zeroes at the destination. Defer + * zeroing so we can send it as a single large command. */ - command = create_command (exts.ptr[i].offset, exts.ptr[i].length, - true, w); - fill_dst_range_with_zeroes (command); + if (!is_zeroing) { + is_zeroing = true; + zeroing_start = offset; + } } - else /* data */ { - /* As the extent might be larger than permitted for a single - * command, we may have to split this into multiple read - * requests. - */ - while (exts.ptr[i].length > 0) { - len = exts.ptr[i].length; - if (len > request_size) - len = request_size; - - command = create_command (exts.ptr[i].offset, len, - false, w); - - wait_for_request_slots (w); - - /* NOTE: Must increase the queue size after waiting. */ - increase_queue_size (w, len); - - /* Begin the asynch read operation. */ - src->ops->asynch_read (src, command, - (nbd_completion_callback) { - .callback = finished_read, - .user_data = command, - }); - - exts.ptr[i].offset += len; - exts.ptr[i].length -= len; + /* If we were in the middle of deferred zeroing, do it now. */ + if (is_zeroing) { + /* Note that offset-zeroing_start can never exceed + * THREAD_WORK_SIZE, so there is no danger of overflowing + * size_t. + */ + command = create_command (zeroing_start, offset-zeroing_start, + true, w); + fill_dst_range_with_zeroes (command); + is_zeroing = false; } + + /* Issue the asynchronous read command. */ + command = create_command (offset, len, false, w); + + wait_for_request_slots (w); + + /* NOTE: Must increase the queue size after waiting. */ + increase_queue_size (w, len); + + /* Begin the asynch read operation. */ + src->ops->asynch_read (src, command, + (nbd_completion_callback) { + .callback = finished_read, + .user_data = command, + }); }

- offset += count; - count = 0; - } /* for extents */ + offset += len; + count -= len; + } /* while (count) */ + + /* If we were in the middle of deferred zeroing, do it now. */ + if (is_zeroing) { + /* Note that offset-zeroing_start can never exceed + * THREAD_WORK_SIZE, so there is no danger of overflowing + * size_t. + */ + command = create_command (zeroing_start, offset - zeroing_start, + true, w); + fill_dst_range_with_zeroes (command); + is_zeroing = false; + } }

/* Wait for in flight NBD requests to finish. */

This looks OK to me. Thanks Laszlo

diff --git a/copy/nbdcopy.pod b/copy/nbdcopy.pod index 54e43915f3..7a4b57f1b5 100644 --- a/copy/nbdcopy.pod +++ b/copy/nbdcopy.pod @@ -182,8 +182,9 @@ Set the maximum number of requests in flight per NBD connection. =item B<--sparse=>N

Detect all zero blocks of size N (bytes) and make them sparse on the -output. You can also turn off sparse detection using S<I<-S 0>>. -The default is 4096 bytes. +output. You can also turn off sparse detection using S<I<-S 0>>. The +default is 4096 bytes, or the destination preferred block size, +whichever is larger.

=item B<--synchronous>

On 06/30/22 17:47, Richard W.M. Jones wrote:

On Thu, Jun 30, 2022 at 05:38:14PM +0200, Laszlo Ersek wrote:

...

On 06/29/22 15:36, Richard W.M. Jones wrote:

...

+ * The invariant for '*i' is always an extent which starts before or + * equal to the current offset. + */ +static bool +only_zeroes (const extent_list exts, size_t *i,

(5) I think it's a bit wasteful (or at least strange) to pass in the entire vector structure; we certainly don't need the "cap" field. Other functions in libnbd seem to take pointers to vectors (which is what I'd expect).

Note it's only passing in 3 fields (24 bytes), not the entire array (and I guess it should inline the whole function). Does that change things?

It's not wrong by any means, just unusual :) Whole-structure passing and returning is really rare in my understanding, div() is one standard function that does it. I don't insist :)

...

...

@@ -177,7 +229,10 @@ worker_thread (void *wp) extent_list exts = empty_vector;

while (get_next_offset (&offset, &count)) { + struct command *command; size_t i; + bool is_zeroing = false; + uint64_t zeroing_start = 0;

(14) I think we should not initialize "zeroing_start". It suggests that the initializer value is meaningful, but it is not.

GCC warns if you don't initialize it! It doesn't understand that is_zeroing is related to the validity of zeroing_start.

We use "#pragma GCC diagnostic" elsewhere in libnbd (in particular in "copy/progress.c"), can we use it here too? (Just for this one variable?) If not, I'd suggest appending a small comment: uint64_t zeroing_start = 0; /* shut up invalid GCC warning */

Thanks for the other suggestions, I'll rework things.

Rich.

Thanks! Laszlo

On 06/29/22 15:36, Richard W.M. Jones wrote:

Because the test previously used error rates of 50%, it could sometimes "fail to fail". This is noticable if you run the test repeatedly:

$ while make -C copy check TESTS=copy-nbd-error.sh >& /tmp/log ; do echo -n . ; done

This now happens more often because of the larger requests made by the new multi-threaded loop, resulting in fewer calls to the error filter, so a greater chance that a series of 50% coin tosses will come up all heads in the test.

Fix this by making the test non-stocastic.

Fixes: commit 8d444b41d09a700c7ee6f9182a649f3f2d325abb --- copy/copy-nbd-error.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/copy/copy-nbd-error.sh b/copy/copy-nbd-error.sh index 0088807f54..01524a890c 100755 --- a/copy/copy-nbd-error.sh +++ b/copy/copy-nbd-error.sh @@ -40,7 +40,7 @@ $VG nbdcopy -- [ nbdkit --exit-with-parent -v --filter=error pattern 5M \ # Failure to read should be fatal echo "Testing read failures on non-sparse source" $VG nbdcopy -- [ nbdkit --exit-with-parent -v --filter=error pattern 5M \ - error-pread-rate=0.5 ] null: && fail=1 + error-pread-rate=1 ] null: && fail=1

# However, reliable block status on a sparse image can avoid the need to read echo "Testing read failures on sparse source" @@ -51,7 +51,7 @@ $VG nbdcopy -- [ nbdkit --exit-with-parent -v --filter=error null 5M \ echo "Testing write data failures on arbitrary destination" $VG nbdcopy -- [ nbdkit --exit-with-parent -v pattern 5M ] \ [ nbdkit --exit-with-parent -v --filter=error --filter=noextents \ - memory 5M error-pwrite-rate=0.5 ] && fail=1 + memory 5M error-pwrite-rate=1 ] && fail=1

# However, writing zeroes can bypass the need for normal writes echo "Testing write data failures from sparse source"

Wasn't the original intent of the 50% error rate that the first error manifest usually at a different offset every time? If we change the error rate to 1, the tests will fail upon the first access, which kind of breaks the original intent. I wonder if we could determine a random offset in advance, and make sure that the read or write access fails 100%, but only if the request covers that offset. ... The probability that n subsequent accesses *don't* fail is (1-error_rate)^n. (The probability that at least one access fails is 1-(1-error_rate)^n.) And n is given by (I think?) image_size/request_size. So, if we change the request_size, we can recalculate "n", for the test not to fail with the same probability as before. (1-err1)^(imgsz/rsz1) = (1-err2)^(imgsz/rsz2) draw the imgsz'th root of both sides (1-err1)^(1/rsz1) = (1-err2)^(1/rsz2) raise both sides to the rsz2'nd power (1-err1)^(rsz2/rsz1) = 1-err2 err2 = 1 - (1-err1)^(rsz2/rsz1) I know that err1=0.5, but don't know rsz2 and rsz1 (the request sizes after, and before, the last patch in the series). Assuming (just guessing!) we increased the request size 8-fold, we'd have to go from error rate 0.5 to: err2 = 1 - (1-0.5)^8 = 1 - (1/2)^8 = 1 - (1 / 256) = 255/256 = 0.99609375 We basically group every eight coin tosses into one super-toss, and want the latter to show "failure" with the same probability as *at least one* of the original 8 tosses failing. Laszlo