It is easy to demonstrate that our blocksize filter has a bug: if the minblock= setting is a higher granularity than the underlying plugin actually supports, and the client is trying to collect block status for the entire disk, a mid-block transition in the plugin can result in the filter rounding a request so small that it no longer makes progress, causing the client to see: nbd.Error: nbd_block_status: block-status: command failed: Invalid argument (EINVAL) Better is to round mid-block transitions up to the block size; even though the client can make read/write requests smaller than the block alignment, they should not see transitions in status at that granularity. Fixes: 2515532316 Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- I'm pushing this already since the tests prove it makes a difference, but it was tricky enough that I thought it worth documenting on-list. filters/blocksize/Makefile.am | 6 +- tests/Makefile.am | 4 +- filters/blocksize/blocksize.c | 40 ++++++++++--- tests/test-blocksize-extents.sh | 102 ++++++++++++++++++++++++++++++++ 4 files changed, 140 insertions(+), 12 deletions(-) create mode 100755 tests/test-blocksize-extents.sh diff --git a/filters/blocksize/Makefile.am b/filters/blocksize/Makefile.am index 54a09bdb..0cb6b1f8 100644 --- a/filters/blocksize/Makefile.am +++ b/filters/blocksize/Makefile.am @@ -1,5 +1,5 @@ # nbdkit -# Copyright (C) 2018 Red Hat Inc. +# Copyright (C) 2018-2020 Red Hat Inc. # # Redistribution and use in source and binary forms, with or without # modification, are permitted provided that the following conditions are @@ -43,12 +43,16 @@ nbdkit_blocksize_filter_la_SOURCES = \ nbdkit_blocksize_filter_la_CPPFLAGS = \ -I$(top_srcdir)/include \ -I$(top_srcdir)/common/include \ + -I$(top_srcdir)/common/utils \ $(NULL) nbdkit_blocksize_filter_la_CFLAGS = $(WARNINGS_CFLAGS) nbdkit_blocksize_filter_la_LDFLAGS = \ -module -avoid-version -shared $(SHARED_LDFLAGS) \ -Wl,--version-script=$(top_srcdir)/filters/filters.syms \ $(NULL) +nbdkit_blocksize_filter_la_LIBADD = \ + $(top_builddir)/common/utils/libutils.la \ + $(NULL) if HAVE_POD diff --git a/tests/Makefile.am b/tests/Makefile.am index 481291e6..c0c7e981 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1199,8 +1199,8 @@ test_layers_filter3_la_LDFLAGS = \ $(NULL) # blocksize filter test. -TESTS += test-blocksize.sh -EXTRA_DIST += test-blocksize.sh +TESTS += test-blocksize.sh test-blocksize-extents.sh +EXTRA_DIST += test-blocksize.sh test-blocksize-extents.sh # cache filter test. TESTS += \ diff --git a/filters/blocksize/blocksize.c b/filters/blocksize/blocksize.c index 8504f985..c3a2d60d 100644 --- a/filters/blocksize/blocksize.c +++ b/filters/blocksize/blocksize.c @@ -43,6 +43,7 @@ #include <nbdkit-filter.h> +#include "cleanup.h" #include "minmax.h" #include "rounding.h" @@ -372,16 +373,37 @@ blocksize_extents (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle, uint32_t count, uint64_t offset, uint32_t flags, struct nbdkit_extents *extents, int *err) { - /* Ask the plugin for blocksize-aligned data. Since the extents - * list start is set to the real offset, everything before the - * offset is ignored automatically. Also we only need to ask for - * maxlen of data, because it's fine to return less than the full - * count as long as we're making progress. + /* Ask the plugin for blocksize-aligned data. Copying that into the + * callers' extents will then take care of truncating unaligned + * ends. Also we only need to ask for maxlen of data, because it's + * fine to return less than the full count as long as we're making + * progress. */ - return next_ops->extents (nxdata, - MIN (count, maxlen), - ROUND_DOWN (offset, minblock), - flags, extents, err); + CLEANUP_EXTENTS_FREE struct nbdkit_extents *extents2 = NULL; + size_t i; + struct nbdkit_extent e; + + extents2 = nbdkit_extents_new (ROUND_DOWN (offset, minblock), + ROUND_UP (offset + count, minblock)); + if (extents2 == NULL) { + *err = errno; + return -1; + } + + if (nbdkit_extents_aligned (next_ops, nxdata, + MIN (ROUND_UP (count, minblock), maxlen), + ROUND_DOWN (offset, minblock), + flags, minblock, extents2, err) == -1) + return -1; + + for (i = 0; i < nbdkit_extents_count (extents2); ++i) { + e = nbdkit_get_extent (extents2, i); + if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1) { + *err = errno; + return -1; + } + } + return 0; } static int diff --git a/tests/test-blocksize-extents.sh b/tests/test-blocksize-extents.sh new file mode 100755 index 00000000..156777ad --- /dev/null +++ b/tests/test-blocksize-extents.sh @@ -0,0 +1,102 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2018-2020 Red Hat Inc. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +# Demonstrate a fix for a bug where blocksize used to cause extents failures + +source ./functions.sh +set -e +set -x + +requires nbdsh --base-allocation -c 'exit(not h.supports_uri())' + +# Script a server that requires 512-byte aligned requests, reports only one +# extent at a time, and with a hole placed unaligned to 4k bounds +exts=' +if test $3 -gt $(( 32 * 1024 )); then + echo "EINVAL request too large" 2>&1 + exit 1 +fi +if test $(( ($3|$4) & 511 )) != 0; then + echo "EINVAL request unaligned" 2>&1 + exit 1 +fi +type= +if test $(( $4 >= 512 && $4 < 8 * 1024 )) = 1; then + type=hole,zero +fi +echo $4 512 $type +' + +# We also need an nbdsh script to parse all extents, coalescing adjacent +# types for simplicity, as well as testing some unaligned probes. +export script=' +size = h.get_size() +offs = 0 +entries = [] +def f (metacontext, offset, e, err): + global entries + global offs + assert offs == offset + for length, flags in zip(*[iter(e)] * 2): + if entries and flags == entries[-1][1]: + entries[-1] = (entries[-1][0] + length, flags) + else: + entries.append((length, flags)) + offs = offs + length + +# Test a loop over the entire device +while offs < size: + h.block_status (size - offs, offs, f) +assert entries == [(4096, 0), (4096, 3), (57344, 0)] + +# Unaligned status queries must also work +offs = 1 +entries = [] +h.block_status (1, offs, f, nbd.CMD_FLAG_REQ_ONE) +assert entries == [(1, 0)] + +offs = 512 +entries = [] +h.block_status (512, offs, f) +assert entries == [(3584, 0)] + +offs = 4095 +entries=[] +while offs < 4097: + h.block_status (4097 - offs, offs, f, nbd.CMD_FLAG_REQ_ONE) +assert entries == [(1, 0), (1, 3)] +' + +# Now run everything +nbdkit -U - --filter=blocksize eval minblock=4k maxlen=32k \ + get_size='echo 64k' pread='exit 1' extents="$exts" \ + --run 'nbdsh --base-allocation -u $uri -c "$script"' -- 2.27.0