11:43 a.m.
v3 was:
https://listman.redhat.com/archives/libguestfs/2022-June/029336.html
v4 makes almost all of the recommended changes from Laszlo's reviews.
I have dropped the third patch (the one which stablises
copy/copy-nbd-error.sh test). That patch is still necessary, but it's
not so urgent because the null: destination no longer advertises a 4M
preferred block size, so the test fails only very rarely as before.
Also I'd like to get Eric's input on that test when he's back.
Rich.
11:43 a.m.
New subject: [PATCH libnbd v4 1/2] copy: Store the preferred block size in the operations struct
This will be used in a subsequent commit. At the moment the preferred
block size for all sources / destinations is simply calculated and
stored.
---
copy/file-ops.c | 4 +++-
copy/main.c | 29 +++++++++++++++++++++++------
copy/nbd-ops.c | 10 ++++++++++
copy/nbdcopy.h | 4 +++-
copy/null-ops.c | 1 +
copy/pipe-ops.c | 1 +
6 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/copy/file-ops.c b/copy/file-ops.c
index ab3787545c..34f08e58c7 100644
--- a/copy/file-ops.c
+++ b/copy/file-ops.c
@@ -241,13 +241,15 @@ seek_hole_supported (int fd)
struct rw *
file_create (const char *name, int fd,
- off_t st_size, bool is_block, direction d)
+ off_t st_size, uint64_t preferred,
+ bool is_block, direction d)
{
struct rw_file *rwf = calloc (1, sizeof *rwf);
if (rwf == NULL) { perror ("calloc"); exit (EXIT_FAILURE); }
rwf->rw.ops = &file_ops;
rwf->rw.name = name;
+ rwf->rw.preferred = preferred;
rwf->fd = fd;
rwf->is_block = is_block;
diff --git a/copy/main.c b/copy/main.c
index cc379e98bc..19ec384d78 100644
--- a/copy/main.c
+++ b/copy/main.c
@@ -512,10 +512,26 @@ open_local (const char *filename, direction d)
fprintf (stderr, "%s: %s: %m\n", prog, filename);
exit (EXIT_FAILURE);
}
- if (S_ISBLK (stat.st_mode) || S_ISREG (stat.st_mode))
- return file_create (filename, fd, stat.st_size, S_ISBLK (stat.st_mode), d);
- else {
- /* Probably stdin/stdout, a pipe or a socket. */
+ if (S_ISREG (stat.st_mode)) /* Regular file. */
+ return file_create (filename, fd,
+ stat.st_size, (uint64_t) stat.st_blksize, false, d);
+ else if (S_ISBLK (stat.st_mode)) { /* Block device. */
+ unsigned int blkioopt;
+
+#ifdef BLKIOOPT
+ if (ioctl (fd, BLKIOOPT, &blkioopt) == -1) {
+ fprintf (stderr, "warning: cannot get optimal I/O size: %s: %m",
+ filename);
+ blkioopt = 4096;
+ }
+#else
+ blkioopt = 4096;
+#endif
+
+ return file_create (filename, fd,
+ stat.st_size, (uint64_t) blkioopt, true, d);
+ }
+ else { /* Probably stdin/stdout, a pipe or a socket. */
synchronous = true; /* Force synchronous mode for pipes. */
return pipe_create (filename, fd);
}
@@ -528,8 +544,9 @@ print_rw (struct rw *rw, const char *prefix, FILE *fp)
char buf[HUMAN_SIZE_LONGEST];
fprintf (fp, "%s: %s \"%s\"\n", prefix, rw->ops->ops_name,
rw->name);
- fprintf (fp, "%s: size=%" PRIi64 " (%s)\n",
- prefix, rw->size, human_size (buf, rw->size, NULL));
+ fprintf (fp, "%s: size=%" PRIi64 " (%s), preferred block size=%"
PRIu64 "\n",
+ prefix, rw->size, human_size (buf, rw->size, NULL),
+ rw->preferred);
}
/* Default implementation of rw->ops->get_extents for backends which
diff --git a/copy/nbd-ops.c b/copy/nbd-ops.c
index 3bc26ba613..098863453f 100644
--- a/copy/nbd-ops.c
+++ b/copy/nbd-ops.c
@@ -112,12 +112,22 @@ open_one_nbd_handle (struct rw_nbd *rwn)
* the same way.
*/
if (rwn->handles.len == 0) {
+ int64_t block_size;
+
rwn->can_zero = nbd_can_zero (nbd) > 0;
+
rwn->rw.size = nbd_get_size (nbd);
if (rwn->rw.size == -1) {
fprintf (stderr, "%s: %s: %s\n", prog, rwn->rw.name, nbd_get_error
());
exit (EXIT_FAILURE);
}
+
+ block_size = nbd_get_block_size (nbd, LIBNBD_SIZE_PREFERRED);
+ if (block_size == -1) {
+ fprintf (stderr, "%s: %s: %s\n", prog, rwn->rw.name, nbd_get_error
());
+ exit (EXIT_FAILURE);
+ }
+ rwn->rw.preferred = block_size == 0 ? 4096 : block_size;
}
if (handles_append (&rwn->handles, nbd) == -1) {
diff --git a/copy/nbdcopy.h b/copy/nbdcopy.h
index 19797dfd66..9438cce417 100644
--- a/copy/nbdcopy.h
+++ b/copy/nbdcopy.h
@@ -43,6 +43,7 @@ struct rw {
struct rw_ops *ops; /* Operations. */
const char *name; /* Printable name, for error messages etc. */
int64_t size; /* May be -1 for streams. */
+ uint64_t preferred; /* Preferred block size. */
/* Followed by private data for the particular subtype. */
};
@@ -53,7 +54,8 @@ typedef enum { READING, WRITING } direction;
/* Create subtypes. */
extern struct rw *file_create (const char *name, int fd,
- off_t st_size, bool is_block, direction d);
+ off_t st_size, uint64_t preferred,
+ bool is_block, direction d);
extern struct rw *nbd_rw_create_uri (const char *name,
const char *uri, direction d);
extern struct rw *nbd_rw_create_subprocess (const char **argv, size_t argc,
diff --git a/copy/null-ops.c b/copy/null-ops.c
index 1218a623e2..99cc9a73fe 100644
--- a/copy/null-ops.c
+++ b/copy/null-ops.c
@@ -45,6 +45,7 @@ null_create (const char *name)
rw->rw.ops = &null_ops;
rw->rw.name = name;
rw->rw.size = INT64_MAX;
+ rw->rw.preferred = 4096;
return &rw->rw;
}
diff --git a/copy/pipe-ops.c b/copy/pipe-ops.c
index 3c8b6c2b39..3815f824fc 100644
--- a/copy/pipe-ops.c
+++ b/copy/pipe-ops.c
@@ -43,6 +43,7 @@ pipe_create (const char *name, int fd)
rwp->rw.ops = &pipe_ops;
rwp->rw.name = name;
rwp->rw.size = -1;
+ rwp->rw.preferred = 4096;
rwp->fd = fd;
return &rwp->rw;
}
--
2.37.0.rc2
1:54 p.m.
New subject: [PATCH libnbd v4 1/2] copy: Store the preferred block size in the operations struct
On Thu, Jun 30, 2022 at 05:43:56PM +0100, Richard W.M. Jones wrote:
This will be used in a subsequent commit. At the moment the
preferred
block size for all sources / destinations is simply calculated and
stored.
---
copy/file-ops.c | 4 +++-
copy/main.c | 29 +++++++++++++++++++++++------
copy/nbd-ops.c | 10 ++++++++++
copy/nbdcopy.h | 4 +++-
copy/null-ops.c | 1 +
copy/pipe-ops.c | 1 +
6 files changed, 41 insertions(+), 8 deletions(-)
+++ b/copy/main.c
@@ -512,10 +512,26 @@ open_local (const char *filename, direction d)
fprintf (stderr, "%s: %s: %m\n", prog, filename);
exit (EXIT_FAILURE);
}
- if (S_ISBLK (stat.st_mode) || S_ISREG (stat.st_mode))
- return file_create (filename, fd, stat.st_size, S_ISBLK (stat.st_mode), d);
- else {
- /* Probably stdin/stdout, a pipe or a socket. */
+ if (S_ISREG (stat.st_mode)) /* Regular file. */
+ return file_create (filename, fd,
+ stat.st_size, (uint64_t) stat.st_blksize, false, d);
Is the cast to uint64_t actually needed?
+ else if (S_ISBLK (stat.st_mode)) { /* Block device. */
+ unsigned int blkioopt;
+
+#ifdef BLKIOOPT
+ if (ioctl (fd, BLKIOOPT, &blkioopt) == -1) {
+ fprintf (stderr, "warning: cannot get optimal I/O size: %s: %m",
+ filename);
+ blkioopt = 4096;
+ }
+#else
+ blkioopt = 4096;
+#endif
+
+ return file_create (filename, fd,
+ stat.st_size, (uint64_t) blkioopt, true, d);
Ditto.
+++ b/copy/nbd-ops.c
@@ -112,12 +112,22 @@ open_one_nbd_handle (struct rw_nbd *rwn)
* the same way.
*/
if (rwn->handles.len == 0) {
+ int64_t block_size;
+
rwn->can_zero = nbd_can_zero (nbd) > 0;
+
rwn->rw.size = nbd_get_size (nbd);
if (rwn->rw.size == -1) {
fprintf (stderr, "%s: %s: %s\n", prog, rwn->rw.name, nbd_get_error
());
exit (EXIT_FAILURE);
}
+
+ block_size = nbd_get_block_size (nbd, LIBNBD_SIZE_PREFERRED);
+ if (block_size == -1) {
+ fprintf (stderr, "%s: %s: %s\n", prog, rwn->rw.name, nbd_get_error
());
+ exit (EXIT_FAILURE);
+ }
+ rwn->rw.preferred = block_size == 0 ? 4096 : block_size;
Looks good for NBD.
}
if (handles_append (&rwn->handles, nbd) == -1) {
diff --git a/copy/nbdcopy.h b/copy/nbdcopy.h
index 19797dfd66..9438cce417 100644
--- a/copy/nbdcopy.h
+++ b/copy/nbdcopy.h
@@ -43,6 +43,7 @@ struct rw {
struct rw_ops *ops; /* Operations. */
const char *name; /* Printable name, for error messages etc. */
int64_t size; /* May be -1 for streams. */
+ uint64_t preferred; /* Preferred block size. */
Will we ever need to worry about preferred block sizes being larger
than signed int? Using uint64_t feels a bit oversized, but is not
technically wrong even if we never exceed 2^31-bit blocks in practice.
+++ b/copy/pipe-ops.c
@@ -43,6 +43,7 @@ pipe_create (const char *name, int fd)
rwp->rw.ops = &pipe_ops;
rwp->rw.name = name;
rwp->rw.size = -1;
+ rwp->rw.preferred = 4096;
Should we try to use PIPE_BUF instead of 4096? Or on Linux,
fcntl(F_GETPIPE_SZ)? Then again, 4096 is probably a safe default,
even if we aren't pushing the pipe to its full atomic capacity.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
2:43 p.m.
New subject: [PATCH libnbd v4 1/2] copy: Store the preferred block size in the operations struct
On Thu, Jul 07, 2022 at 01:54:55PM -0500, Eric Blake wrote:
> + if (S_ISREG (stat.st_mode)) /* Regular file. */
> + return file_create (filename, fd,
> + stat.st_size, (uint64_t) stat.st_blksize, false, d);
Is the cast to uint64_t actually needed?
No it's not, because the parameter is declared as uint64_t. I guess
it's my spidey sense about some imagined platform where blksize_t is a
strange type that can't be implicitly converted, but I don't know if
that's possible.
> diff --git a/copy/nbdcopy.h b/copy/nbdcopy.h
> index 19797dfd66..9438cce417 100644
> --- a/copy/nbdcopy.h
> +++ b/copy/nbdcopy.h
> @@ -43,6 +43,7 @@ struct rw {
> struct rw_ops *ops; /* Operations. */
> const char *name; /* Printable name, for error messages etc. */
> int64_t size; /* May be -1 for streams. */
> + uint64_t preferred; /* Preferred block size. */
Will we ever need to worry about preferred block sizes being larger
than signed int? Using uint64_t feels a bit oversized, but is not
technically wrong even if we never exceed 2^31-bit blocks in practice.
It seems very unlikely. The protocol limits these block sizes to
uint32_t (ie. unsigned int) right now, and preferred size to much
smaller. I'm not sure what you anticipate with your upcoming 64 bit
patches.
As this is preferred (not max) block size, it seems unlikely that we'd
ever want anything larger than 31 bits in future.
I think I used uint64_t here because it reduced the need to worry
about overflow.
> +++ b/copy/pipe-ops.c
> @@ -43,6 +43,7 @@ pipe_create (const char *name, int fd)
> rwp->rw.ops = &pipe_ops;
> rwp->rw.name = name;
> rwp->rw.size = -1;
> + rwp->rw.preferred = 4096;
Should we try to use PIPE_BUF instead of 4096? Or on Linux,
fcntl(F_GETPIPE_SZ)? Then again, 4096 is probably a safe default,
even if we aren't pushing the pipe to its full atomic capacity.
I'm going to say we shouldn't do that, for a few reasons:
- If the platform had a randomly large PIPE_BUF then it could cause
nbdcopy to automatically adjust request size upwards. We saw this
in an earlier iteration of this series where I set the null_ops
preferred size to 4M, which meant nbdcopy silently made all
requests huge, which had strange knock-on effects.
- As you say, atomic pipe size is not the same as preferred size.
It's hard to say what the preferred size for a kernel pipe is (it's
not a problem handling smallish writes).
- Despite the name, this file applies to sockets and anything else
that isn't S_ISREG / S_ISBLK.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
11:43 a.m.
New subject: [PATCH libnbd v4 2/2] copy: Use preferred block size for copying
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 | 149 +++++++++++++++++++-------
copy/nbdcopy.pod | 5 +-
8 files changed, 202 insertions(+), 50 deletions(-)
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
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 19ec384d78..0e27db8e17 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 at least as large as the preferred
+ * size of source & destination.
+ */
+ 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 > 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..369b8167a1 100644
--- a/copy/multi-thread-copying.c
+++ b/copy/multi-thread-copying.c
@@ -166,6 +166,62 @@ 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] intersects only with zero extents.
+ *
+ * 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,
+ uint64_t offset, unsigned len)
+{
+ size_t j;
+
+ /* Invariant. */
+ assert (*i < exts.len);
+ assert (exts.ptr[*i].offset <= offset);
+
+ /* Update the invariant. Search for the last possible extent in the
+ * 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 (*i < exts.len);
+ assert (exts.ptr[*i].offset <= offset);
+
+ /* If *i is not the last extent, then the next extent starts
+ * strictly beyond our current offset.
+ */
+ assert (*i == exts.len - 1 || exts.ptr[*i + 1].offset > offset);
+
+ /* Search forward, look for any non-zero extents overlapping the region. */
+ for (j = *i; j < exts.len; ++j) {
+ uint64_t start, end;
+
+ /* [start..end-1] is the current extent. */
+ start = exts.ptr[j].offset;
+ end = exts.ptr[j].offset + exts.ptr[j].length;
+
+ assert (end > offset);
+
+ if (start >= offset + len)
+ break;
+
+ /* 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 +233,10 @@ worker_thread (void *wp)
extent_list exts = empty_vector;
while (get_next_offset (&offset, &count)) {
- size_t i;
+ struct command *command;
+ size_t extent_index;
+ bool is_zeroing = false;
+ uint64_t zeroing_start = 0;
assert (0 < count && count <= THREAD_WORK_SIZE);
if (extents)
@@ -185,52 +244,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;
+ extent_index = 0; // index into extents array used to optimize only_zeroes
+ while (count) {
+ const size_t len = MIN (count, request_size);
- if (exts.ptr[i].zero) {
+ if (only_zeroes (exts, &extent_index, 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. */
diff --git a/copy/nbdcopy.pod b/copy/nbdcopy.pod
index 94c713f68f..501d6fb245 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>
--
2.37.0.rc2
2:35 p.m.
New subject: [PATCH libnbd v4 2/2] copy: Use preferred block size for copying
On Thu, Jun 30, 2022 at 05:43:57PM +0100, 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).
At this point, the patch is already in (I agree with your priority in
getting a libnbd release with the fix to allow --oo compress in v2v),
but I'm still happy to do a deep dive review to see if I spot any
followups.
qemu-nbd has (in the past) had bugs where it reports extents less
granular than sectors at the end of a regular file that was not a
multiple of 512 bytes. And I still have patches in my local working
tree for qemu bugs where use of the blkdebug device to change block
sizing can trigger bugs in both NBD_CMD_READ and NBD_CMD_BLOCK_STATUS
not obeying alignment rules according to what block sizes were
advertised, but they are corner-case enough fhat I agree that nbdcopy
won't usually trip up on these scenarios.
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).
Makes total sense for the approach.
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.
Yeah, we'll still need to get around to that, but since v2v wasn't
using it, and you wanted to get a libnbd release out the door for v2v,
I can understand why we have only the partial fix here.
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 | 149 +++++++++++++++++++-------
copy/nbdcopy.pod | 5 +-
8 files changed, 202 insertions(+), 50 deletions(-)
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.
+++ b/copy/copy-file-to-qcow2-compressed.sh
+
+# 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 ]
Long line, but not much you can do about it, short of adding in more
use of temporary shell variables. If you think it adds legibility,
this could be something like:
opts=driver=compress
opts+=,file.driver=qcow2
opts+=,file.file.driver=file
opts+=,file.file.filename=$file1
nbdcopy -- [ ... ] [ $QEMU_NBD --image-opts "$opts" ]
+
+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
Good test.
+++ 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 at least as large as the preferred
+ * size of source & destination.
+ */
+ 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 > 0 && sparse_size < dst->preferred)
+ sparse_size = dst->preferred;
+
Do we need to check anywhere that a command-line --request-size is a
power of 2, and does not exceed the size of how much extents map we
are willing to collect in one go?
/* 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..369b8167a1 100644
--- a/copy/multi-thread-copying.c
+++ b/copy/multi-thread-copying.c
@@ -166,6 +166,62 @@ 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] intersects only with zero extents.
+ *
+ * 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,
+ uint64_t offset, unsigned len)
+{
<snip>
A bit hairy to read, but well-commented and it looks like it correctly
iterates through the list of extents vs. the current region under
examination.
@@ -177,7 +233,10 @@ worker_thread (void *wp)
extent_list exts = empty_vector;
while (get_next_offset (&offset, &count)) {
- size_t i;
+ struct command *command;
+ size_t extent_index;
+ bool is_zeroing = false;
+ uint64_t zeroing_start = 0;
assert (0 < count && count <= THREAD_WORK_SIZE);
if (extents)
@@ -185,52 +244,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;
+ extent_index = 0; // index into extents array used to optimize only_zeroes
+ while (count) {
+ const size_t len = MIN (count, request_size);
- if (exts.ptr[i].zero) {
+ if (only_zeroes (exts, &extent_index, 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.
Here's one reason why a runtime check that command-line --request-size
doesn't exceed THREAD_WORK_SIZE might be worthwhile (that would be in
main, not here, though).
+ */
+ 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,
+ });
I'm not sure if asynch_read() can be made more efficient when the
source has a smaller granularity than --request-size or the
destination, so that we aren't reading zeroes for the subset of the
buffer. But that is performance, not correctness (it is always
correct to read zeroes); the question is whether it is wasted effort,
and since we know there is data at least somewhere in the window of
the read, this may be in the noise. I'm fine with the current
implementation; we may have few enough mixed-source clusters that we
could actually penalize ourselves with the overhead of trying to avoid
sub-region reads.
}
- 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. */
diff --git a/copy/nbdcopy.pod b/copy/nbdcopy.pod
index 94c713f68f..501d6fb245 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.
Overall looks like a good improvement.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
6:17 a.m.
New subject: [PATCH libnbd v4 2/2] copy: Use preferred block size for copying
On Thu, Jul 07, 2022 at 02:35:41PM -0500, Eric Blake wrote:
> +++ b/copy/copy-file-to-qcow2-compressed.sh
> +
> +# 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 ]
Long line, but not much you can do about it, short of adding in more
use of temporary shell variables. If you think it adds legibility,
this could be something like:
opts=driver=compress
opts+=,file.driver=qcow2
opts+=,file.file.driver=file
opts+=,file.file.filename=$file1
nbdcopy -- [ ... ] [ $QEMU_NBD --image-opts "$opts" ]
Thanks - updated as suggested in 05ef84e06357.
> +++ 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 at least as large as the preferred
> + * size of source & destination.
> + */
> + 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 > 0 && sparse_size < dst->preferred)
> + sparse_size = dst->preferred;
> +
Do we need to check anywhere that a command-line --request-size is a
power of 2, and does not exceed the size of how much extents map we
are willing to collect in one go?
We initially check request_size is a power of 2 when parsing it from
the command line, but it (was) possible that preferred block size is
not, in which case the above statements could change it so it's not a
power of 2.
I've added commit 06222abbd34f upstream which forces the preferred
values to always be powers of 2 and adds more checks. Hopefully now
both request_size and sparse_size should always be powers of 2.
> + /* 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.
Here's one reason why a runtime check that command-line --request-size
doesn't exceed THREAD_WORK_SIZE might be worthwhile (that would be in
main, not here, though).
Added in commit 4dd69b44291.
> + */
> + 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,
> + });
I'm not sure if asynch_read() can be made more efficient when the
source has a smaller granularity than --request-size or the
destination, so that we aren't reading zeroes for the subset of the
buffer. But that is performance, not correctness (it is always
correct to read zeroes); the question is whether it is wasted effort,
and since we know there is data at least somewhere in the window of
the read, this may be in the noise. I'm fine with the current
implementation; we may have few enough mixed-source clusters that we
could actually penalize ourselves with the overhead of trying to avoid
sub-region reads.
It is definitely wasted effort. I punted on an implementation which
tries to follow the extent map for the source when the source
granularity is smaller than the destination (just for sanity!)
All the internal tests pass, plus nbdkit tests pass when using
nbdcopy @4dd69b44291.
Thanks,
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
8:33 a.m.
On Thu, Jun 30, 2022 at 05:43:55PM +0100, Richard W.M. Jones wrote:
v3 was:
https://listman.redhat.com/archives/libguestfs/2022-June/029336.html
v4 makes almost all of the recommended changes from Laszlo's reviews.
I have dropped the third patch (the one which stablises
copy/copy-nbd-error.sh test). That patch is still necessary, but it's
not so urgent because the null: destination no longer advertises a 4M
preferred block size, so the test fails only very rarely as before.
Also I'd like to get Eric's input on that test when he's back.
I'm back now, but (as usual after a nice vacation from email) have a
large backlog in my inbox to prioritize and respond to. This series
looks like one that I will prioritize for today.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
682
days inactive
690
days old
7 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones