rpms/nbdkit
6
0
Fork 0
Code Issues Pull Requests Releases Activity

Add extra system call checking and debugging to nbdkit-file-plugin

Browse Source 
resolves: RHEL-85510
This commit is contained in:
Richard W.M. Jones 2025-03-31 14:02:41 +01:00
parent 8243333e16
commit ebdd780c47
15 changed files with 397 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
0001-server-log-Move-preserve-errno-to-log_verror-functio.patch
View File

@ -147,5 +147,5 @@ index 464e4f9a..9c1f667a 100644
/* Note: preserves the previous value of errno. */
--
2.43.0
2.47.1

2
0002-server-Rename-threadlocal_-set-get-_error-to-._errno.patch
View File

@ -173,5 +173,5 @@ index 088fe55a..9bb656bc 100644
int err = errno;
struct threadlocal *threadlocal = pthread_getspecific (threadlocal_key);
--
2.43.0
2.47.1

2
0003-server-Introduce-threadlocal_-set-get-_last_error.patch
View File

@ -91,5 +91,5 @@ index 9bb656bc..74a3c4e5 100644
* size is increased to ‘size’ bytes if required.
*
--
2.43.0
2.47.1

2
0004-server-Take-a-thread-local-copy-of-the-last-call-to-.patch
View File

@ -91,5 +91,5 @@ index 677da05c..d428bfc8 100644
case NBD_CMD_READ:
if (backend_pread (c, buf, count, offset, 0, &err) == -1)
--
2.43.0
2.47.1

2
0005-server-Send-the-last-error-to-the-NBD-client.patch
View File

@ -173,5 +173,5 @@ index 00000000..fc720606
+
+grep "Go Away" $out
--
2.43.0
2.47.1

2
0006-vddk-Include-stdbool.h.patch
View File

@ -24,5 +24,5 @@ index 467d00ca..5982fcea 100644
#include <inttypes.h>
--
2.43.0
2.47.1

2
0007-vddk-Cache-the-disk-size-in-the-handle.patch
View File

@ -55,5 +55,5 @@ index fb0c79a8..1d1069cc 100644
/* reexec.c */
--
2.43.0
2.47.1

2
0008-vddk-do_extents-Mark-some-local-variables-const.patch
View File

@ -30,5 +30,5 @@ index 5982fcea..bc015d16 100644
uint64_t position, end, start_sector;
--
2.43.0
2.47.1

2
0009-vddk-do_extents-Exit-the-function-if-we-hit-req_one-.patch
View File

@ -27,5 +27,5 @@ index bc015d16..112111e3 100644
return 0;
--
2.43.0
2.47.1

2
0010-vddk-do_extents-Avoid-reading-partial-chunk-beyond-t.patch
View File

@ -198,5 +198,5 @@ index 00000000..28fccd6c
+ 196608 65536 3 hole,zero
+ 262144 1536 0 data"
--
2.43.0
2.47.1

41
0011-file-Add-debugging-if-sync_file_range-posix_fadvise-.patch Normal file
View File

@ -0,0 +1,41 @@
From 881113e29b45975742ca11a4d0539ed2eb40b717 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones@redhat.com>
Date: Fri, 28 Mar 2025 20:32:46 +0000
Subject: [PATCH] file: Add debugging if sync_file_range / posix_fadvise fails
These are advisory hints, but add some debugging so we know if these
calls are failing. The errors are ignored so there is no behaviour
change.
Thanks: Eric Sandeen
(cherry picked from commit feb22cfc85b0750a4e4bfbc5fe56636883599feb)
---
plugins/file/file.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/plugins/file/file.c b/plugins/file/file.c
index 2bad6480..6d0e77a1 100644
--- a/plugins/file/file.c
+++ b/plugins/file/file.c
@@ -123,11 +123,13 @@ evict_writes (int fd, uint64_t offset, size_t len)
/* Evict the oldest window from the page cache. */
if (window[0].len > 0) {
- sync_file_range (window[0].fd, window[0].offset, window[0].len,
- SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE|
- SYNC_FILE_RANGE_WAIT_AFTER);
- posix_fadvise (window[0].fd, window[0].offset, window[0].len,
- POSIX_FADV_DONTNEED);
+ if (sync_file_range (window[0].fd, window[0].offset, window[0].len,
+ SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE|
+ SYNC_FILE_RANGE_WAIT_AFTER) == -1)
+ nbdkit_debug ("sync_file_range: wait: (ignored): %m");
+ if (posix_fadvise (window[0].fd, window[0].offset, window[0].len,
+ POSIX_FADV_DONTNEED) == -1)
+ nbdkit_debug ("posix_fadvise: POSIX_FADV_DONTNEED: (ignored): %m");
}
/* Move the Nth window to N-1. */
--
2.47.1

82
0012-file-If-sync_file_range-fails-to-start-don-t-add-win.patch Normal file
View File

@ -0,0 +1,82 @@
From 205fb5f8ebd72a37fb624e7d551ead313da2437b Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones@redhat.com>
Date: Fri, 28 Mar 2025 20:35:54 +0000
Subject: [PATCH] file: If sync_file_range fails to start, don't add window to
list
When starting the process of writing out the newest range, we
previously didn't check if sync_file_range succeeded or failed. But
if it failed there's no reason to add it to the list of windows. Also
debug any failures.
This shouldn't change any semantics as far as we're aware.
Thanks: Eric Sandeen
(cherry picked from commit d35f0636373e6a58c8f3fcfcf505af248e27c574)
---
plugins/file/file.c | 48 ++++++++++++++++++++++++++-------------------
1 file changed, 28 insertions(+), 20 deletions(-)
diff --git a/plugins/file/file.c b/plugins/file/file.c
index 6d0e77a1..638c960d 100644
--- a/plugins/file/file.c
+++ b/plugins/file/file.c
@@ -121,27 +121,35 @@ evict_writes (int fd, uint64_t offset, size_t len)
{
ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&window_lock);
- /* Evict the oldest window from the page cache. */
- if (window[0].len > 0) {
- if (sync_file_range (window[0].fd, window[0].offset, window[0].len,
- SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE|
- SYNC_FILE_RANGE_WAIT_AFTER) == -1)
- nbdkit_debug ("sync_file_range: wait: (ignored): %m");
- if (posix_fadvise (window[0].fd, window[0].offset, window[0].len,
- POSIX_FADV_DONTNEED) == -1)
- nbdkit_debug ("posix_fadvise: POSIX_FADV_DONTNEED: (ignored): %m");
- }
-
- /* Move the Nth window to N-1. */
- memmove (&window[0], &window[1], sizeof window[0] * (NR_WINDOWS-1));
-
- /* Set up the current window and tell Linux to start writing it out
- * to disk (asynchronously).
+ /* Tell Linux to start writing the current range out to disk
+ * (asynchronously).
*/
- sync_file_range (fd, offset, len, SYNC_FILE_RANGE_WRITE);
- window[NR_WINDOWS-1].fd = fd;
- window[NR_WINDOWS-1].offset = offset;
- window[NR_WINDOWS-1].len = len;
+ if (sync_file_range (fd, offset, len, SYNC_FILE_RANGE_WRITE) == -1) {
+ /* sync_file_range to start the write failed */
+ nbdkit_debug ("sync_file_range: write: (ignored): %m");
+ }
+ else {
+ /* sync_file_range to start the write succeeded, so
+ * evict the oldest window from the page cache.
+ */
+ if (window[0].len > 0) {
+ if (sync_file_range (window[0].fd, window[0].offset, window[0].len,
+ SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE|
+ SYNC_FILE_RANGE_WAIT_AFTER) == -1)
+ nbdkit_debug ("sync_file_range: wait: (ignored): %m");
+ if (posix_fadvise (window[0].fd, window[0].offset, window[0].len,
+ POSIX_FADV_DONTNEED) == -1)
+ nbdkit_debug ("posix_fadvise: POSIX_FADV_DONTNEED: (ignored): %m");
+ }
+
+ /* Move the Nth window to N-1. */
+ memmove (&window[0], &window[1], sizeof window[0] * (NR_WINDOWS-1));
+
+ /* Add the range to the newest end of the list of windows. */
+ window[NR_WINDOWS-1].fd = fd;
+ window[NR_WINDOWS-1].offset = offset;
+ window[NR_WINDOWS-1].len = len;
+ }
}
/* When we close the handle we must remove any windows which are still
--
2.47.1

254
0013-tests-Add-tests-of-file-plugin-cache-none.patch Normal file
View File

@ -0,0 +1,254 @@
From e9c972bc03d42d6fe7f11cd076a5500d39976a61 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones@redhat.com>
Date: Sat, 29 Mar 2025 10:01:05 +0000
Subject: [PATCH] tests: Add tests of file plugin cache=none
These are based on the tests which were done in the commit message
when the feature was first added in commit aa5a2183a6, but cleaned up
and modernised a little.
It is likely that these tests would fail under memory pressure (since
they test that the page cache is used vs not used). Making them more
robust left as an exercise for the future.
(cherry picked from commit 6017ba21aeeb3d7ad85925e78dba85a005194dee)
---
README.md | 1 +
tests/Makefile.am | 4 ++
tests/test-file-cache-none-read.sh | 90 ++++++++++++++++++++++++++++
tests/test-file-cache-none-write.sh | 92 +++++++++++++++++++++++++++++
4 files changed, 187 insertions(+)
create mode 100755 tests/test-file-cache-none-read.sh
create mode 100755 tests/test-file-cache-none-write.sh
diff --git a/README.md b/README.md
index fbe403da..86ac6c57 100644
--- a/README.md
+++ b/README.md
@@ -174,6 +174,7 @@ To test for memory leaks (`make check-valgrind`):
For non-essential enhancements to the test suite:
+* cachedel, cachestats (from https://github.com/Feh/nocache)
* expect
* fdisk, sfdisk (from util-linux)
* flake8
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 0aa36846..4340cc38 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -846,6 +846,8 @@ TESTS += \
test-file-extents.sh \
test-file-dir.sh \
test-file-dirfd.sh \
+ test-file-cache-none-read.sh \
+ test-file-cache-none-write.sh \
$(NULL)
EXTRA_DIST += \
test-file.sh \
@@ -855,6 +857,8 @@ EXTRA_DIST += \
test-file-extents.sh \
test-file-dir.sh \
test-file-dirfd.sh \
+ test-file-cache-none-read.sh \
+ test-file-cache-none-write.sh \
$(NULL)
LIBGUESTFS_TESTS += test-file-block
diff --git a/tests/test-file-cache-none-read.sh b/tests/test-file-cache-none-read.sh
new file mode 100755
index 00000000..c9831b43
--- /dev/null
+++ b/tests/test-file-cache-none-read.sh
@@ -0,0 +1,90 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright Red Hat
+#
+# 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.
+
+# Test the file plugin, reading with cache=none.
+
+source ./functions.sh
+set -e
+set -x
+
+# Makes no sense to run this test under valgrind.
+skip_if_valgrind
+
+requires_plugin file
+requires_run
+requires_nbdcopy
+requires $TRUNCATE --version
+requires test -r /dev/urandom
+requires dd --version
+requires $SED --version
+
+# Requires the cachestats tool from https://github.com/Feh/nocache
+# It doesn't support --version or --help, so use 'type' instead.
+requires type cachestats
+requires type cachedel
+
+inp=file-cache-none-read.in
+stats1=file-cache-none-read.s1
+stats2=file-cache-none-read.s2
+rm -f $inp $stats1 $stats2
+cleanup_fn rm -f $inp $stats1 $stats2
+
+# Create a large random file as input.
+dd if=/dev/urandom of=$inp bs=1024k count=1024
+
+# Drop the input file from the page cache and read it out with nbdkit.
+# We expect to see the input file mostly or completely in cache after.
+cachedel $inp
+nbdkit file $inp --run 'nbdcopy "$uri" null:'
+cachestats $inp > $stats1
+cat $stats1
+
+# The same, with cache=none.
+# We expect to see the input file not cached after.
+cachedel $inp
+nbdkit file $inp --run 'nbdcopy "$uri" null:' cache=none
+cachestats $inp > $stats2
+cat $stats2
+
+# The output of cachestats looks like this:
+# pages in cache: 262144/262144 (100.0%) [filesize=1048576.0K, pagesize=4K]
+# We want to check that % pages in cache using cache=none is much
+# lower than the default case.
+pic1="$($SED 's,pages in cache: [0-9/]* (\([0-9]*\)\.[0-9]*%).*,\1,' \
+ < $stats1)"
+pic2="$($SED 's,pages in cache: [0-9/]* (\([0-9]*\)\.[0-9]*%).*,\1,' \
+ < $stats2)"
+
+# Test before is > 10%
+test "$pic1" -gt 10
+# Test after is < 10%
+test "$pic2" -lt 10
diff --git a/tests/test-file-cache-none-write.sh b/tests/test-file-cache-none-write.sh
new file mode 100755
index 00000000..2041a5cd
--- /dev/null
+++ b/tests/test-file-cache-none-write.sh
@@ -0,0 +1,92 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright Red Hat
+#
+# 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.
+
+# Test the file plugin, writing with cache=none.
+
+source ./functions.sh
+set -e
+set -x
+
+# Makes no sense to run this test under valgrind.
+skip_if_valgrind
+
+requires_plugin file
+requires_run
+requires_nbdcopy
+requires $TRUNCATE --version
+requires test -r /dev/urandom
+requires dd --version
+requires $SED --version
+
+# Requires the cachestats tool from https://github.com/Feh/nocache
+# It doesn't support --version or --help, so use 'type' instead.
+requires type cachestats
+
+inp=file-cache-none-write.in
+out=file-cache-none-write.out
+stats1=file-cache-none-write.s1
+stats2=file-cache-none-write.s2
+rm -f $inp $out $stats1 $stats2
+cleanup_fn rm -f $inp $out $stats1 $stats2
+
+# Create a large random file as input.
+dd if=/dev/urandom of=$inp bs=1024k count=1024
+
+# Copy to output using cache=default and collect the stats.
+# We expect to see the output file mostly or completely in cache after.
+rm -f $out; truncate -r $inp $out
+export inp
+nbdkit file $out --run 'nbdcopy $inp "$uri"' cache=default
+cachestats $out > $stats1
+cat $stats1
+
+# The same, with cache=none.
+# We expect to see the output file not cached after.
+rm -f $out; truncate -r $inp $out
+export inp
+nbdkit file $out --run 'nbdcopy $inp "$uri"' cache=none
+cachestats $out > $stats2
+cat $stats2
+
+# The output of cachestats looks like this:
+# pages in cache: 262144/262144 (100.0%) [filesize=1048576.0K, pagesize=4K]
+# We want to check that % pages in cache using cache=none is much
+# lower than the default case.
+pic1="$($SED 's,pages in cache: [0-9/]* (\([0-9]*\)\.[0-9]*%).*,\1,' \
+ < $stats1)"
+pic2="$($SED 's,pages in cache: [0-9/]* (\([0-9]*\)\.[0-9]*%).*,\1,' \
+ < $stats2)"
+
+# Test before is > 10%
+test "$pic1" -gt 10
+# Test after is < 10%
+test "$pic2" -lt 10
--
2.47.1

2
copy-patches.sh
View File

@ -6,7 +6,7 @@ set -e
# directory. Use it like this:
# ./copy-patches.sh
rhel_version=9.6
rhel_version=9.7
# Check we're in the right directory.
if [ ! -f nbdkit.spec ]; then

11
nbdkit.spec
View File

@ -56,7 +56,7 @@
Name: nbdkit
Version: 1.38.5
Release: 2%{?dist}
Release: 3%{?dist}
Summary: NBD server
License: BSD-3-Clause
@ -78,7 +78,7 @@ Source2: libguestfs.keyring
Source3: copy-patches.sh
# Patches come from the upstream repository:
# https://gitlab.com/nbdkit/nbdkit/-/commits/rhel-9.6/
# https://gitlab.com/nbdkit/nbdkit/-/commits/rhel-9.7/
# Patches.
Patch0001: 0001-server-log-Move-preserve-errno-to-log_verror-functio.patch
@ -91,6 +91,9 @@ Patch0007: 0007-vddk-Cache-the-disk-size-in-the-handle.patch
Patch0008: 0008-vddk-do_extents-Mark-some-local-variables-const.patch
Patch0009: 0009-vddk-do_extents-Exit-the-function-if-we-hit-req_one-.patch
Patch0010: 0010-vddk-do_extents-Avoid-reading-partial-chunk-beyond-t.patch
Patch0011: 0011-file-Add-debugging-if-sync_file_range-posix_fadvise-.patch
Patch0012: 0012-file-If-sync_file_range-fails-to-start-don-t-add-win.patch
Patch0013: 0013-tests-Add-tests-of-file-plugin-cache-none.patch
# For automatic RPM Provides generation.
# See: https://rpm-software-management.github.io/rpm/manual/dependency_generators.html
@ -1509,6 +1512,10 @@ fi
%changelog
* Mon Mar 31 2025 Richard W.M. Jones <rjones@redhat.com> - 1.38.5-3
- Add extra system call checking and debugging to nbdkit-file-plugin
resolves: RHEL-85510
* Mon Jan 06 2025 Richard W.M. Jones <rjones@redhat.com> - 1.38.5-2
- vddk: Avoid reading partial chunk beyond the end of the disk
resolves: RHEL-71694