Re: [Libguestfs] [libnbd PATCH v3 12/22] copy: Update nbdcopy to use 64-bit block status

[Richard W.M. Jones]

On Thu, May 25, 2023 at 08:00:58AM -0500, Eric Blake wrote:
> Although our use of "base:allocation" doesn't require the use of the
> 64-bit API for flags, we might perform slightly faster for a server
> that does give us 64-bit extent lengths and honors larger nbd_zero
> lengths.

I think this change may turn out to be more significant than you have
describe here.

Consider copying from a remote, very large, mostly blank NBD source,
and also that our block status querying is synchronous.  The old
nbdcopy might end up doing lots of serialized round trips over 4GB
segments of the source to discover that it is sparse.  With this
change as I understand it, those would be coalesced into a single RTT.

(Similarly when zeroing on the output side, but to a lesser extent
because those requests can be issued in parallel.)

> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  copy/nbd-ops.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/copy/nbd-ops.c b/copy/nbd-ops.c
> index f3b3bed3..71ee0a3b 100644
> --- a/copy/nbd-ops.c
> +++ b/copy/nbd-ops.c
> @@ -428,7 +428,7 @@ nbd_ops_asynch_notify_write (struct rw *rw, size_t index)
>   * request for extents in a single round trip.
>   */
>  static int add_extent (void *vp, const char *metacontext,
> -                       uint64_t offset, uint32_t *entries, size_t nr_entries,
> +                       uint64_t offset, nbd_extent *entries, size_t 
> nr_entries,
>                         int *error);
> 
>  static void
> @@ -449,11 +449,11 @@ nbd_ops_get_extents (struct rw *rw, size_t index,
>      size_t i;
> 
>      exts.len = 0;
> -    if (nbd_block_status (nbd, count, offset,
> -                          (nbd_extent_callback) {
> -                            .user_data = &exts,
> -                            .callback = add_extent
> -                          }, 0) == -1) {
> +    if (nbd_block_status_64 (nbd, count, offset,
> +                             (nbd_extent64_callback) {
> +                               .user_data = &exts,
> +                               .callback = add_extent
> +                             }, 0) == -1) {
>        /* XXX We could call default_get_extents, but unclear if it's
>         * the right thing to do if the server is returning errors.
>         */
> @@ -496,7 +496,7 @@ nbd_ops_get_extents (struct rw *rw, size_t index,
> 
>  static int
>  add_extent (void *vp, const char *metacontext,
> -            uint64_t offset, uint32_t *entries, size_t nr_entries,
> +            uint64_t offset, nbd_extent *entries, size_t nr_entries,
>              int *error)
>  {
>    extent_list *ret = vp;
> @@ -505,25 +505,25 @@ add_extent (void *vp, const char *metacontext,
>    if (strcmp (metacontext, "base:allocation") != 0 || *error)
>      return 0;
> 
> -  for (i = 0; i < nr_entries; i += 2) {
> +  for (i = 0; i < nr_entries; i++) {
>      struct extent e;
> 
>      e.offset = offset;
> -    e.length = entries[i];
> +    e.length = entries[i].length;
> 
>      /* Note we deliberately don't care about the HOLE flag.  There is
>       * no need to read extent that reads as zeroes.  We will convert
>       * to it to a hole or allocated extents based on the command line
>       * arguments.
>       */
> -    e.zero = (entries[i+1] & LIBNBD_STATE_ZERO) != 0;
> +    e.zero = (entries[i].flags & LIBNBD_STATE_ZERO) != 0;
> 
>      if (extent_list_append (ret, e) == -1) {
>        perror ("realloc");
>        exit (EXIT_FAILURE);
>      }
> 
> -    offset += entries[i];
> +    offset += entries[i].length;
>    }
> 
>    return 0;
> -- 
> 2.40.1

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org