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

[Martin Kletzander]

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
>>> "system_u:object_r:svirt_socket_t:s0"
>>>   );
>>> +
>>> +  (* 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 $?
>1
>$ nbdkit --dump-plugin --filter=cache null
>path=/usr/lib64/nbdkit/plugins/nbdkit-null-plugin.so
>name=null
>...
>$ echo $?
>0
>
>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)
>-(/usr/lib64/nbdkit/filters/nbdkit-cache-filter.so)
>-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
> (/usr/lib64/nbdkit/plugins/nbdkit-null-plugin.so)
> 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
>



-------------- 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/20190920/21c4dbe6/attachment.sig>