[Libguestfs] [PATCH nbdkit 7/8] vddk: Implement extents.

[Richard W.M. Jones]

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