[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libguestfs] [PATCH v4 07/12] v2v: nbdkit: Add the readahead filter unconditionally if it is available.

On Fri, Sep 20, 2019 at 09:33:06AM -0500, Eric Blake wrote:
On 9/20/19 9:04 AM, Martin Kletzander wrote:
On Fri, Sep 20, 2019 at 10:28:18AM +0100, Richard W.M. Jones wrote:
The readahead filter is a self-configuring filter that makes
sequential reads faster when the plugin is slow (and all of the
plugins we use here are always slow).

@@ -133,9 +142,17 @@ let common_create plugin_name plugin_args
plugin_env =
  if have_selinux then (        (* label the socket so qemu can open
it *)
    add_arg "--selinux-label"; add_arg
+  (* Adding the readahead filter is always a win for our access
+   * patterns.  However if it doesn't exist don't worry.
+   *)
+  if Sys.file_exists (filterdir // "nbdkit-readahead-filter.so") then (
+    add_arg "--filter"; add_arg "readahead"

Just out of curiosity, wouldn't it be cleaner, at least in the future,
to add an
option in nbdkit to report filters it can load?  Of course it would require
newer nbdkit in the long run.

Checking for file existence for filters is somewhat less fragile than
for plugins, because all filters are built in-tree (we've specifically
documented that we don't provide ABI guarantees for filters, so the only
sane way to use a filter is to compile it at the same time/version as
the nbdkit binary that will load it).  But you do have a point that
checking for files is still more fragile than just asking nbdkit whether
a given filter exists:

$ nbdkit --dump-plugin --filter=nosuch null

Oh, this is way better than my:

 nbdkit --filter=nosuch memory size=1M --run true

nbdkit: error: cannot open filter 'nosuch':
/usr/lib64/nbdkit/filters/nbdkit-nosuch-filter.so: cannot open shared
object file: No such file or directory
$ echo $?
$ nbdkit --dump-plugin --filter=cache null
$ echo $?

Similarly for --help.  But notice:

$ diff -u <(nbdkit --dump-plugin --filter=cache null) \
  <(nbdkit --dump-plugin null)
$ diff -u <(nbdkit --help --filter=cache null) <(nbdkit --help null)
--- /dev/fd/63	2019-09-20 09:31:43.184428823 -0500
+++ /dev/fd/62	2019-09-20 09:31:43.185428824 -0500
@@ -23,15 +23,6 @@

Please read the nbdkit(1) manual page for full usage.

-filter: cache (nbdkit caching filter)
-cache=MODE                Set cache MODE, one of writeback (default),
-                          writethrough, or unsafe.
-cache-on-read=BOOL        Set to true to cache on reads (default false).
-cache-max-size=SIZE       Set maximum space used by cache.
-cache-high-threshold=PCT  Percentage of max size where reclaim begins.
-cache-low-threshold=PCT   Percentage of max size where reclaim ends.

But this is still better because it also allows checking for the filter
parameters had they been changed.

In this case I feel like there is no need to add anything else, I did not know
that this already exists.  I don't even know if this is really needed, I was
just curious if it would be nicer.

plugin: null
size=<SIZE>             Size of the backing disk

So we could probably improve --dump-plugin to ALSO show details about
the filter (such as a second path=), the way --help does, or even add a
'nbdkit --dump-filter filtername' to work in isolation.  But while you'd
have to wait for a future nbdkit release to get more details from
--dump-plugin, and/or a way to list all available filters (think search
permissions on a directory), you already have a way with current nbdkit
to check existence of a filter (think read permissions on a directory).

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Attachment: signature.asc
Description: PGP signature

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]