Libguestfs - Mar 2019 - Re: [PATCH nbdkit 7/8] vddk: Implement extents.

On 3/20/19 5:11 PM, Richard W.M. Jones wrote:
> This uses a new API VixDiskLib_QueryAllocatedBlocks provided in
> VDDK >= 6.7.
> 
> Thanks: Martin Kletzander.
> ---
>  plugins/vddk/vddk-structs.h |  15 +++-
>  plugins/vddk/vddk.c         | 138 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 152 insertions(+), 1 deletion(-)
> 
> +static int
> +vddk_extents (void *handle, uint32_t count, uint64_t offset, uint32_t
flags,
> +              struct nbdkit_extents *extents)
> +{
> +  struct vddk_handle *h = handle;
> +  bool req_one = flags & NBDKIT_FLAG_REQ_ONE;
> +  uint64_t position, end, start_sector;
> +
> +  position = offset;
> +
...
> +
> +    for (i = 0; i < block_list->numBlocks; ++i) {
> +      uint64_t offset, length;
> +
> +      offset = block_list->blocks[i].offset * VIXDISKLIB_SECTOR_SIZE;
> +      length = block_list->blocks[i].length * VIXDISKLIB_SECTOR_SIZE;
> +
> +      /* The query returns blocks.  We must insert holes between the
> +       * blocks as necessary.
> +       */
> +      if (position < offset) {
> +        if (nbdkit_add_extent (extents,
> +                               offset, length,
> +                               NBDKIT_EXTENT_HOLE | NBDKIT_EXTENT_ZERO) ==
-1)
> +          goto error_in_add;
> +      }
> +
> +      if (nbdkit_add_extent (extents,
> +                             offset, length, 0 /* allocated data */) ==
-1) {
> +      error_in_add:
> +        DEBUG_CALL ("VixDiskLib_FreeBlockList",
"block_list");
> +        VixDiskLib_FreeBlockList (block_list);
> +        return -1;
> +      }
> +
> +      position = offset + length;
This inner loop might be long; you could add this:

if (req_one)
  break;
> +    }
> +    DEBUG_CALL ("VixDiskLib_FreeBlockList",
"block_list");
> +    VixDiskLib_FreeBlockList (block_list);
> +
> +    start_sector += nr_sectors;
> +
> +    if (req_one)
> +      break;
just as you break the outer loop here, to make REQ_ONE slightly faster.

Otherwise looks sane. m not in a position to test it, but if 'qemu-img
map --output=json nbd://...' pointing to your server matches what you
expect for vddk, that's a good sign.

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

On Sat, Mar 23, 2019 at 02:29:54PM -0500, Eric Blake
wrote:
> On 3/20/19 5:11 PM, Richard W.M. Jones wrote:
...
> > +    for (i = 0; i < block_list->numBlocks; ++i) {
> > +      uint64_t offset, length;
> > +
> > +      offset = block_list->blocks[i].offset *
VIXDISKLIB_SECTOR_SIZE;
> > +      length = block_list->blocks[i].length *
VIXDISKLIB_SECTOR_SIZE;
> > +
> > +      /* The query returns blocks.  We must insert holes between the
> > +       * blocks as necessary.
> > +       */
> > +      if (position < offset) {
> > +        if (nbdkit_add_extent (extents,
> > +                               offset, length,
> > +                               NBDKIT_EXTENT_HOLE |
NBDKIT_EXTENT_ZERO) == -1)
> > +          goto error_in_add;
> > +      }
> > +
> > +      if (nbdkit_add_extent (extents,
> > +                             offset, length, 0 /* allocated data */)
== -1) {
> > +      error_in_add:
> > +        DEBUG_CALL ("VixDiskLib_FreeBlockList",
"block_list");
> > +        VixDiskLib_FreeBlockList (block_list);
> > +        return -1;
> > +      }
> > +
> > +      position = offset + length;
> 
> This inner loop might be long; you could add this:
> 
> if (req_one)
>   break;
The issue in practice is that VixDiskLib_QueryAllocatedBlocks is very
expensive (because it's a remote procedure call), but we expect
calling nbdkit_add_extent should be very cheap.

However I'll experiment with breaking earlier from the inner loop.  I
need to do a lot more testing on this patch anyway.

Rich.
> > +    }
> > +    DEBUG_CALL ("VixDiskLib_FreeBlockList",
"block_list");
> > +    VixDiskLib_FreeBlockList (block_list);
> > +
> > +    start_sector += nr_sectors;
> > +
> > +    if (req_one)
> > +      break;
> 
> just as you break the outer loop here, to make REQ_ONE slightly faster.
> 
> Otherwise looks sane. m not in a position to test it, but if 'qemu-img
> map --output=json nbd://...' pointing to your server matches what you
> expect for vddk, that's a good sign.
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 



-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Mon, Mar 25, 2019 at 05:34:25PM +0000, Richard W.M. Jones
wrote:
> On Sat, Mar 23, 2019 at 02:29:54PM -0500, Eric Blake wrote:
> > On 3/20/19 5:11 PM, Richard W.M. Jones wrote:
> ...
> > > +    for (i = 0; i < block_list->numBlocks; ++i) {
> > > +      uint64_t offset, length;
> > > +
> > > +      offset = block_list->blocks[i].offset *
VIXDISKLIB_SECTOR_SIZE;
> > > +      length = block_list->blocks[i].length *
VIXDISKLIB_SECTOR_SIZE;
> > > +
> > > +      /* The query returns blocks.  We must insert holes between
the
> > > +       * blocks as necessary.
> > > +       */
> > > +      if (position < offset) {
> > > +        if (nbdkit_add_extent (extents,
> > > +                               offset, length,
> > > +                               NBDKIT_EXTENT_HOLE |
NBDKIT_EXTENT_ZERO) == -1)
> > > +          goto error_in_add;
> > > +      }
> > > +
> > > +      if (nbdkit_add_extent (extents,
> > > +                             offset, length, 0 /* allocated data
*/) == -1) {
> > > +      error_in_add:
> > > +        DEBUG_CALL ("VixDiskLib_FreeBlockList",
"block_list");
> > > +        VixDiskLib_FreeBlockList (block_list);
> > > +        return -1;
> > > +      }
> > > +
> > > +      position = offset + length;
> > 
> > This inner loop might be long; you could add this:
> > 
> > if (req_one)
> >   break;
So interestingly _no_ you can't add that here, and the reason does
reveal why the REQ_ONE flag is still hard to get right even with the
simpler implementation of extents.

Because VDDK's QueryAllocatedBlocks API only works for some very large
aligned block size (64K IIRC), we round down the requested offset
before querying VDDK.  So in the inner loop the first extent we get
back might lie before the offset (before extents->start).  IOW
nbdkit_add_extent has succeeded but we haven't added any extents.  We
could call nbdkit_extents_count here, but technically we've said
that's a filter-only API.

I believe the test for req_one in the outer loop is fine, although it
relies on QueryAllocatedBlocks not being crazy.  As it's VDDK and
therefore crazy by nature maybe we should consider removing that too,
but it would then be really inefficient ...

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