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

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

On 3/27/19 7:17 AM, Richard W.M. Jones wrote:
>>> 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.
> 
Indeed - if you round down, you have to iterate at least until you've
crossed back over the original offset. Tracking that takes more coding
effort than just blindly always giving the full 64k result (which is
guaranteed to overlap, because you never rounded down more than that).
Any more coding overhead should be accompanied by benchmarks showing
that they improve performance, or we just leave things as slightly
inefficient, because...
> 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
...of this ;)

> maybe we should consider removing that too,
> but it would then be really inefficient ...
Nah, the outer loop break is easy to justify without additional bookkeeping.


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

FYI I now have a VDDK extents function which now works and is tested
against a real server on real guests:

 
https://github.com/rwmjones/nbdkit/commit/b327a79ee3fa0af0fe27d5d18ac7b5f44a7c243e

There were quite a lot of changes:

 - The offset and length of the hole before each block was
   plain wrong: fixed.

 - I needed to add the implicit hole after the list of blocks
   returned by QueryAllocatedBlocks.

 - Don't consider req_one until we've got past the initial offset.

 - Centralise the add_extents code into one function and add
   a debug flag (-D) so we can debug it.

Unfortunately qemu's habit of setting req_one on every request hurts
performance quite a lot, because calling QueryAllocatedBlocks is very
expensive.  You end up having to call QAB before each pread and
constantly re-requesting data which we've already asked for.  We might
cache the QAB output but that's complex and hairy.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top