[Libguestfs] [PATCH v2v] output: Don't use nbdkit-file-plugin cache=none when writing

[Laszlo Ersek]

On 12/07/21 14:09, Richard W.M. Jones wrote:
> nbdkit-file-plugin flag cache=none is meant to ensure that the file
> plugin does not pollute the page cache.  However it doesn't yet work
> very well, especially for writing.  As you can see here:
> 
> https://gitlab.com/nbdkit/nbdkit/-/blob/048d5b9818c88355e596824355269773e5c4f6ad/plugins/file/file.c#L594
> 
> it is currently implemented by flushing the whole file (including
> parallel requests), and then evicting the data from the page cache.
> This implementation is naive.  (See nbdcopy copy/file-ops.c for a much

(1) s/nbdcopy/libnbd/

> better implementation.)
> 
> This causes quite a considerable slow down when writing to a local
> file (eg -os local).  The penalty varies between machines and possibly
> kernels.  On my Fedora Rawhide AMD server with 12 cores and SSDs, the
> slow down is under 5%.  But on my RHEL 9 Intel NUC server with 2 cores
> and a hard disk, it makes a really huge difference, nearly doubling
> the total time taken.
> 
> I left a note in the code that we should fix this in nbdkit and re-add
> this option or similar when it is working.
> ---
>  output/output.ml | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/output/output.ml b/output/output.ml
> index 7e9765085d..c3bc4eeac7 100644
> --- a/output/output.ml
> +++ b/output/output.ml
> @@ -99,7 +99,13 @@ let output_to_local_file ?(changeuid = fun f -> f ())
>       let cmd =
>         if Nbdkit.version nbdkit_config >= (1, 22, 0) then (
>           let cmd = Nbdkit.add_arg cmd "fadvise" "sequential" in
> -         let cmd = Nbdkit.add_arg cmd "cache" "none" in
> +         (* nbdkit 1.28 has a very naive implementation of
> +          * page cache eviction.  We need to copy the one from
> +          * nbdcopy copy/file-ops.c.  Until then do not use

(2) s/nbdcopy/libnbd/

> +          * this flag because it causes a large slow down on
> +          * some machines. XXX
> +          *)
> +         (*let cmd = Nbdkit.add_arg cmd "cache" "none" in*)
>           cmd
>         )
>         else cmd in
> 

I agree this is OK given that the original purpose was to spare the page
cache some thrashing. (If the original purpose had been to ensure the
integrity of the data written as soon as possible, then this would be
very wrong -- but it's not the case. NBDKIT_FLAG_FUA is just a
happenstance wrt. the original purpose.
posix_fadvise(POSIX_FADV_DONTNEED) will be the right solution.)

Acked-by: Laszlo Ersek <lersek at redhat.com>

Thanks
Laszlo