7:40 p.m.
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>
---
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