Libguestfs - Apr 2019 - Re: [PATCH nbdkit v5 FINAL 14/19] data, memory: Implement extents.

On 3/28/19 11:18 AM, Richard W.M. Jones wrote:
> These plugins are both based on the same sparse array structure which
> supports a simple implementation of extents.
> ---
>  common/sparse/sparse.h     |   7 +-
>  common/sparse/sparse.c     |  37 ++++++++++-
>  plugins/data/data.c        |  16 ++++-
>  plugins/memory/memory.c    |  16 ++++-
>  README                     |   2 +
>  tests/Makefile.am          |   2 +
>  tests/test-data-extents.sh | 131 +++++++++++++++++++++++++++++++++++++
>  7 files changed, 207 insertions(+), 4 deletions(-)
> 
> +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;
I know in earlier review I asked whether we can move the clamping...
> +
> +    /* Work out the type of this extent. */
> +    if (p == NULL)
> +      /* No backing page, so it's a hole. */
> +      type = NBDKIT_EXTENT_HOLE | NBDKIT_EXTENT_ZERO;
> +    else {
> +      if (is_zero (p, n))
> +        /* A backing page and it's all zero, it's a zero extent.
*/
> +        type = NBDKIT_EXTENT_ZERO;
> +      else
> +        /* Normal allocated data. */
> +        type = 0;
> +    }
> +    if (nbdkit_add_extent (extents, offset, n, type) == -1)
> +      return -1;
> +
...to here, after the final nbdkit_add_extent, so that we can return a
larger extent than the client's request. I remember when I originally
asked, you declined due to odd interactions with REQ_ONE semantics, but
since then, we changed how add_extent() works. Does it work now to defer
the clamping?

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

On Tue, Apr 23, 2019 at 02:46:10PM -0500, Eric Blake
wrote:
> On 3/28/19 11:18 AM, Richard W.M. Jones wrote:
> > These plugins are both based on the same sparse array structure which
> > supports a simple implementation of extents.
> > ---
> >  common/sparse/sparse.h     |   7 +-
> >  common/sparse/sparse.c     |  37 ++++++++++-
> >  plugins/data/data.c        |  16 ++++-
> >  plugins/memory/memory.c    |  16 ++++-
> >  README                     |   2 +
> >  tests/Makefile.am          |   2 +
> >  tests/test-data-extents.sh | 131
+++++++++++++++++++++++++++++++++++++
> >  7 files changed, 207 insertions(+), 4 deletions(-)
> > 
> > +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;
> 
> I know in earlier review I asked whether we can move the clamping...
> 
> > +
> > +    /* Work out the type of this extent. */
> > +    if (p == NULL)
> > +      /* No backing page, so it's a hole. */
> > +      type = NBDKIT_EXTENT_HOLE | NBDKIT_EXTENT_ZERO;
> > +    else {
> > +      if (is_zero (p, n))
> > +        /* A backing page and it's all zero, it's a zero
extent. */
> > +        type = NBDKIT_EXTENT_ZERO;
> > +      else
> > +        /* Normal allocated data. */
> > +        type = 0;
> > +    }
> > +    if (nbdkit_add_extent (extents, offset, n, type) == -1)
> > +      return -1;
> > +
> 
> ...to here, after the final nbdkit_add_extent, so that we can return a
> larger extent than the client's request. I remember when I originally
> asked, you declined due to odd interactions with REQ_ONE semantics, but
> since then, we changed how add_extent() works. Does it work now to defer
> the clamping?
It's a bit late at night for me to think clearly about extents, but
here is a description of the original problem with moving the clamp:

https://www.redhat.com/archives/libguestfs/2019-March/msg00202.html

I'll see if this still applies tomorrow morning ...

Rich.

-- 
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 4/23/19 4:49 PM, Richard W.M. Jones wrote:
>>
>> ...to here, after the final nbdkit_add_extent, so that we can return a
>> larger extent than the client's request. I remember when I
originally
>> asked, you declined due to odd interactions with REQ_ONE semantics, but
>> since then, we changed how add_extent() works. Does it work now to
defer
>> the clamping?
> 
> It's a bit late at night for me to think clearly about extents, but
> here is a description of the original problem with moving the clamp:
> 
> https://www.redhat.com/archives/libguestfs/2019-March/msg00202.html
Thanks for finding that link:
> 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]
But since then, we've fixed the truncate filter to allocate a second
extents bounded by the truncation size to hand to next_ops->extents, so
even if the plugin calls nbdkit_add_extent with information beyond the
length that the truncation filter cares about, the result is still
bounded to the truncation's choice of end offset. So I think we have
indeed fixed the initial problem.
> I'll see if this still applies tomorrow morning ...
No rush :)

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