Libguestfs - Mar 2019 - Re: [PATCH nbdkit 6/8] data, memory: Implement extents.

On 3/20/19 5:11 PM, Richard W.M. Jones wrote:
> These plugins are both based on the same sparse array structure which
> supports a simple implementation of extents.
> ---
> +int
> +sparse_array_extents (struct sparse_array *sa,
> +                      uint32_t count, uint64_t offset,
> +                      struct nbdkit_extents *extents)
> +{
> +  uint32_t n, type;
> +  void *p;
> +
> +  while (count > 0) {
> +    p = lookup (sa, offset, false, &n, NULL);
> +    if (n > count)
> +      n = count;
Why are we clamping the information we return? I'd move this clamp...
> +
> +    if (p == NULL)
> +      type = NBDKIT_EXTENT_HOLE | NBDKIT_EXTENT_ZERO;
> +    else
> +      type = 0; /* allocated data */
> +    if (nbdkit_add_extent (extents, offset, n, type) == -1)
> +      return -1;
...here, after we've recorded the maximum amount of information possible
into the extents array. (We need the clamp to terminate the loop, but
that doesn't mean we have to truncate our answer)
> +
> +    count -= n;
> +    offset += n;
> +  }
> +
> +  return 0;
> +}
Otherwise, looks good.

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

On Sat, Mar 23, 2019 at 12:05:51PM -0500, Eric Blake
wrote:
> On 3/20/19 5:11 PM, Richard W.M. Jones wrote:
> > These plugins are both based on the same sparse array structure which
> > supports a simple implementation of extents.
> > ---
> 
> > +int
> > +sparse_array_extents (struct sparse_array *sa,
> > +                      uint32_t count, uint64_t offset,
> > +                      struct nbdkit_extents *extents)
> > +{
> > +  uint32_t n, type;
> > +  void *p;
> > +
> > +  while (count > 0) {
> > +    p = lookup (sa, offset, false, &n, NULL);
> > +    if (n > count)
> > +      n = count;
> 
> Why are we clamping the information we return? I'd move this clamp...
It's a copy and paste from one of the previous sparse_array_*
functions in the same file.  I agree that we should return the most
possible information and moving the clamp as you suggest is the way to
do that.

Rich.
> > +
> > +    if (p == NULL)
> > +      type = NBDKIT_EXTENT_HOLE | NBDKIT_EXTENT_ZERO;
> > +    else
> > +      type = 0; /* allocated data */
> > +    if (nbdkit_add_extent (extents, offset, n, type) == -1)
> > +      return -1;
> 
> ...here, after we've recorded the maximum amount of information
possible
> into the extents array. (We need the clamp to terminate the loop, but
> that doesn't mean we have to truncate our answer)
> 
> > +
> > +    count -= n;
> > +    offset += n;
> > +  }
> > +
> > +  return 0;
> > +}
> 
> Otherwise, looks good.
> 
> -- 
> 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
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Mon, Mar 25, 2019 at 05:30:32PM +0000, Richard W.M. Jones
wrote:
> On Sat, Mar 23, 2019 at 12:05:51PM -0500, Eric Blake wrote:
> > On 3/20/19 5:11 PM, Richard W.M. Jones wrote:
> > > These plugins are both based on the same sparse array structure
which
> > > supports a simple implementation of extents.
> > > ---
> > 
> > > +int
> > > +sparse_array_extents (struct sparse_array *sa,
> > > +                      uint32_t count, uint64_t offset,
> > > +                      struct nbdkit_extents *extents)
> > > +{
> > > +  uint32_t n, type;
> > > +  void *p;
> > > +
> > > +  while (count > 0) {
> > > +    p = lookup (sa, offset, false, &n, NULL);
> > > +    if (n > count)
> > > +      n = count;
> > 
> > Why are we clamping the information we return? I'd move this
clamp...
> 
> It's a copy and paste from one of the previous sparse_array_*
> functions in the same file.  I agree that we should return the most
> possible information and moving the clamp as you suggest is the way to
> do that.
So I tried that, but it introduces a subtle error.

Imagine an allocated RAM disk with a size smaller than the 32K page
size of the sparse array:

  nbdkit data data="1" size=512

This will return extents information: [length=32768, type=data].

This doesn't matter for qemu which appears to ignore the extra
information, but it causes a bug if we place the truncate filter on
top:

  nbdkit --filter=truncate data data="1" size=512 truncate=64K

This now returns the following extents information which is wrong:

  [length=32768, type=data]
  [length=32768, type=hole|zero]

If we leave the test where it is then we get the expected extents:

  [length=512, type=data]
  [length=65536-512, type=hole|zero]

Now it's fair to say this is also caused by the sparse array not
knowing the real size of the plugin.  We might pass that information
into the sparse array, but I feel as it's very low cost to query the
sparse array we might as well do the simpler thing.

Oh yes and I found a bug in qemu, will discuss that in another email :-)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v