[Libguestfs] [nbdkit PATCH] cache: Reduce use of bounce-buffer

[Richard W.M. Jones]

On Sat, May 11, 2019 at 03:30:04PM -0500, Eric Blake wrote:
> Although the time spent in memcpy/memset probably pales in comparison
> to time spent in socket I/O, it's still worth worth reducing the
> number of times we have to utilize a bounce buffer when we already
> have aligned data.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  filters/cache/cache.c | 60 ++++++++++++++++++++++++++++---------------
>  1 file changed, 39 insertions(+), 21 deletions(-)
> 
> diff --git a/filters/cache/cache.c b/filters/cache/cache.c
> index 19ce555..98786b5 100644
> --- a/filters/cache/cache.c
> +++ b/filters/cache/cache.c
> @@ -1,5 +1,5 @@
>  /* nbdkit
> - * Copyright (C) 2018 Red Hat Inc.
> + * Copyright (C) 2018-2019 Red Hat Inc.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -58,6 +58,7 @@
>  #include "cache.h"
>  #include "blk.h"
>  #include "reclaim.h"
> +#include "isaligned.h"
> 
>  #define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
> 
> @@ -233,11 +234,13 @@ cache_pread (struct nbdkit_next_ops *next_ops, void *nxdata,
>    CLEANUP_FREE uint8_t *block = NULL;
> 
>    assert (!flags);
> -  block = malloc (blksize);
> -  if (block == NULL) {
> -    *err = errno;
> -    nbdkit_error ("malloc: %m");
> -    return -1;
> +  if (!IS_ALIGNED (count | offset, blksize)) {
> +    block = malloc (blksize);
> +    if (block == NULL) {
> +      *err = errno;
> +      nbdkit_error ("malloc: %m");
> +      return -1;
> +    }
>    }
> 
>    /* XXX This breaks up large read requests into smaller ones, which
> @@ -258,12 +261,14 @@ cache_pread (struct nbdkit_next_ops *next_ops, void *nxdata,
> 
>      {
>        ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
> -      r = blk_read (next_ops, nxdata, blknum, block, err);
> +      r = blk_read (next_ops, nxdata, blknum,
> +                    blkoffs || n < blksize ? block : buf, err);
>      }
>      if (r == -1)
>        return -1;
> 
> -    memcpy (buf, &block[blkoffs], n);
> +    if (blkoffs || n < blksize)
> +      memcpy (buf, &block[blkoffs], n);

I've stared at this patch for a while and I don't really understand
it.

One suggestion though.  The patch might be better (or at least might
be more obviously correct) if the function defined a boolean at the
top which was used to tell if the bounce buffer was in use, something
like:

  const bool use_bounce_buffer = /* condition */;

  if (use_bounce_buffer) {
    block = malloc (blksize);
    ...
  }

  r = blk_read (..., use_bounce_buffer ? block : buf, err);

etc.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW