Martin Kletzander
2021-Aug-04 08:47 UTC
[Libguestfs] [PATCH nbdkit 6/7] cache: Add cache-min-block-size parameter
On Mon, Jul 26, 2021 at 06:28:59PM +0100, Richard W.M. Jones wrote:>This allows you to choose a larger block size. I found experimentally >that this improves performance because of locality in access patterns. >The idea came from qcow2 which implicitly does the same thing because >of the relatively large cluster size (32K). > >nbdkit + cache-filter with 4K block size + copy-on-read + curl >(to a very slow remote site): >=> virt-inspector took 22 mins > >same with 64K block size: >=> virt-inspector took 19 mins > >However compared to a qcow2 file using copy-on-read, backed with >nbdkit + curl (ie. implicit 32K block size) we are still a lot slower, >possibly because having the cache inside virt-inspector greatly >reduces round trips: >=> virt-inspector took 13 mins >--- > filters/cache/nbdkit-cache-filter.pod | 9 ++++ > tests/Makefile.am | 2 + > filters/cache/cache.h | 3 ++ > filters/cache/blk.c | 2 +- > filters/cache/cache.c | 36 ++++++++++---- > tests/test-cache-block-size.sh | 70 +++++++++++++++++++++++++++ > 6 files changed, 112 insertions(+), 10 deletions(-) > >diff --git a/filters/cache/nbdkit-cache-filter.pod b/filters/cache/nbdkit-cache-filter.pod >index 2ac307e0..9511e91b 100644 >--- a/filters/cache/nbdkit-cache-filter.pod >+++ b/filters/cache/nbdkit-cache-filter.pod >@@ -5,6 +5,7 @@ nbdkit-cache-filter - nbdkit caching filter > =head1 SYNOPSIS > > nbdkit --filter=cache plugin [cache=writeback|writethrough|unsafe] >+ [cache-min-block-size=SIZE] > [cache-max-size=SIZE] > [cache-high-threshold=N] > [cache-low-threshold=N] >@@ -59,6 +60,14 @@ This is dangerous and can cause data loss, but this may be acceptable > if you only use it for testing or with data that you don't care about > or can cheaply reconstruct. > >+=item B<cache-min-block-size=>SIZE >+ >+Set the minimum block size used by the cache. This must be a power of >+2 and E<ge> 4096. >+ >+The default is 4096, or the block size of the filesystem which >+contains the temporary file storing the cache (whichever is larger). >+ > =item B<cache-max-size=>SIZE > > =item B<cache-high-threshold=>N >diff --git a/tests/Makefile.am b/tests/Makefile.am >index 8e0304d4..7146a2d1 100644 >--- a/tests/Makefile.am >+++ b/tests/Makefile.am >@@ -1373,12 +1373,14 @@ EXTRA_DIST += test-blocksize.sh test-blocksize-extents.sh > # cache filter test. > TESTS += \ > test-cache.sh \ >+ test-cache-block-size.sh \ > test-cache-on-read.sh \ > test-cache-max-size.sh \ > test-cache-unaligned.sh \ > $(NULL) > EXTRA_DIST += \ > test-cache.sh \ >+ test-cache-block-size.sh \ > test-cache-on-read.sh \ > test-cache-max-size.sh \ > test-cache-unaligned.sh \ >diff --git a/filters/cache/cache.h b/filters/cache/cache.h >index a559adef..5c32c37c 100644 >--- a/filters/cache/cache.h >+++ b/filters/cache/cache.h >@@ -45,6 +45,9 @@ extern enum cache_mode { > /* Size of a block in the cache. */ > extern unsigned blksize; > >+/* Minimum block size (cache-min-block-size parameter). */ >+extern unsigned min_block_size; >+ > /* Maximum size of the cache and high/low thresholds. */ > extern int64_t max_size; > extern unsigned hi_thresh, lo_thresh; >diff --git a/filters/cache/blk.c b/filters/cache/blk.c >index 19f79605..6276985f 100644 >--- a/filters/cache/blk.c >+++ b/filters/cache/blk.c >@@ -149,7 +149,7 @@ blk_init (void) > nbdkit_error ("fstatvfs: %s: %m", tmpdir); > return -1; > } >- blksize = MAX (4096, statvfs.f_bsize); >+ blksize = MAX (min_block_size, statvfs.f_bsize); > nbdkit_debug ("cache: block size: %u", blksize); > > bitmap_init (&bm, blksize, 2 /* bits per block */); >diff --git a/filters/cache/cache.c b/filters/cache/cache.c >index 8af52106..48a20c3b 100644 >--- a/filters/cache/cache.c >+++ b/filters/cache/cache.c >@@ -40,6 +40,7 @@ > #include <inttypes.h> > #include <unistd.h> > #include <fcntl.h> >+#include <limits.h> > #include <errno.h> > #include <assert.h> > #include <sys/types.h> >@@ -62,6 +63,7 @@ > #include "blk.h" > #include "reclaim.h" > #include "isaligned.h" >+#include "ispowerof2.h" > #include "minmax.h" > #include "rounding.h" > >@@ -70,7 +72,8 @@ > */ > static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER; > >-unsigned blksize; >+unsigned blksize; /* actual block size (picked by blk.c) */ >+unsigned min_block_size = 4096; > enum cache_mode cache_mode = CACHE_MODE_WRITEBACK; > int64_t max_size = -1; > unsigned hi_thresh = 95, lo_thresh = 80; >@@ -80,13 +83,6 @@ const char *cor_path; > static int cache_flush (nbdkit_next *next, void *handle, uint32_t flags, > int *err); > >-static void >-cache_load (void) >-{ >- if (blk_init () == -1) >- exit (EXIT_FAILURE); >-} >- > static void > cache_unload (void) > { >@@ -116,6 +112,19 @@ cache_config (nbdkit_next_config *next, nbdkit_backend *nxdata, > return -1; > } > } >+ else if (strcmp (key, "cache-min-block-size") == 0) { >+ int64_t r; >+ >+ r = nbdkit_parse_size (value); >+ if (r == -1) >+ return -1; >+ if (r < 4096 || !is_power_of_2 (r) || r > UINT_MAX) { >+ nbdkit_error ("cache-min-block-size is not a power of 2, or is too small or too large"); >+ return -1; >+ } >+ min_block_size = r; >+ return 0; >+ } > #ifdef HAVE_CACHE_RECLAIM > else if (strcmp (key, "cache-max-size") == 0) { > int64_t r; >@@ -220,6 +229,15 @@ cache_config_complete (nbdkit_next_config_complete *next, > return next (nxdata); > } > >+static int >+cache_get_ready (int thread_model) >+{ >+ if (blk_init () == -1) >+ return -1; >+ >+ return 0; >+} >+ > /* Get the file size, set the cache size. */ > static int64_t > cache_get_size (nbdkit_next *next, >@@ -691,11 +709,11 @@ cache_cache (nbdkit_next *next, > static struct nbdkit_filter filter = { > .name = "cache", > .longname = "nbdkit caching filter", >- .load = cache_load, > .unload = cache_unload, > .config = cache_config, > .config_complete = cache_config_complete, > .config_help = cache_config_help, >+ .get_ready = cache_get_ready, > .prepare = cache_prepare, > .get_size = cache_get_size, > .can_cache = cache_can_cache, >diff --git a/tests/test-cache-block-size.sh b/tests/test-cache-block-size.sh >new file mode 100755 >index 00000000..a2a27407 >--- /dev/null >+++ b/tests/test-cache-block-size.sh >@@ -0,0 +1,70 @@ >+#!/usr/bin/env bash >+# nbdkit >+# Copyright (C) 2018-2021 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. >+ >+source ./functions.sh >+set -e >+set -x >+ >+requires_filter cache >+requires_nbdsh_uri >+ >+sock=$(mktemp -u /tmp/nbdkit-test-sock.XXXXXX) >+files="cache-block-size.img $sock cache-block-size.pid" >+rm -f $files >+cleanup_fn rm -f $files >+ >+# Create an empty base image. >+truncate -s 128K cache-block-size.img >+ >+# Run nbdkit with the caching filter. >+start_nbdkit -P cache-block-size.pid -U $sock --filter=cache \ >+ file cache-block-size.img cache-min-block-size=64K >+ >+nbdsh --connect "nbd+unix://?socket=$sock" \ >+ -c ' >+# Write some pattern data to the overlay and check it reads back OK. >+buf = b"abcd" * 16384 >+h.pwrite(buf, 32768) >+zero = h.pread(32768, 0) >+assert zero == bytearray(32768) >+buf2 = h.pread(65536, 32768) >+assert buf == buf2 >+ >+# Flushing should write through to the underlying file. >+h.flush() >+ >+with open("cache-block-size.img", "rb") as file: >+ zero = file.read(32768) >+ assert zero == bytearray(32768) >+ buf2 = file.read(65536) >+ assert buf == buf2 >+'I do not see how this checks that the min block size is respected. I would expect something along the lines of: 1) read 32k 2) write to the underlying file past those 32k 3) read another 32k (from offset == 32k) 4) check that the last read returned original data and not those written in step 2) -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 833 bytes Desc: not available URL: <http://listman.redhat.com/archives/libguestfs/attachments/20210804/6890de35/attachment.sig>
Richard W.M. Jones
2021-Aug-04 10:07 UTC
[Libguestfs] [PATCH nbdkit 6/7] cache: Add cache-min-block-size parameter
On Wed, Aug 04, 2021 at 10:47:34AM +0200, Martin Kletzander wrote:> >+# Run nbdkit with the caching filter. > >+start_nbdkit -P cache-block-size.pid -U $sock --filter=cache \ > >+ file cache-block-size.img cache-min-block-size=64K > >+ > >+nbdsh --connect "nbd+unix://?socket=$sock" \ > >+ -c ' > >+# Write some pattern data to the overlay and check it reads back OK. > >+buf = b"abcd" * 16384 > >+h.pwrite(buf, 32768) > >+zero = h.pread(32768, 0) > >+assert zero == bytearray(32768) > >+buf2 = h.pread(65536, 32768) > >+assert buf == buf2 > >+ > >+# Flushing should write through to the underlying file. > >+h.flush() > >+ > >+with open("cache-block-size.img", "rb") as file: > >+ zero = file.read(32768) > >+ assert zero == bytearray(32768) > >+ buf2 = file.read(65536) > >+ assert buf == buf2 > >+' > > > I do not see how this checks that the min block size is respected. I > would expect something along the lines of: > > 1) read 32k > 2) write to the underlying file past those 32k > 3) read another 32k (from offset == 32k) > 4) check that the last read returned original data and not those written > in step 2)Yes this test is not great. I think it was basically copied from this one: https://gitlab.com/nbdkit/nbdkit/-/blob/master/tests/test-cache.sh I'll see if I can rethink this once I've got the v2v stuff out of the way unless someone else comes up with a fix. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top