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

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

On Tue, Apr 23, 2019 at 07:59:52PM -0500, Eric Blake
wrote:
> 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].
We can't use qemu-img map to print the extents information that the
plugin returns, but we can use the log filter:

$ ./nbdkit -fv --filter=log \
           data data="1" size=512 logfile=/dev/stdout \
           --run 'qemu-img convert -n $nbd null-co://'  |& grep
Extents

2019-04-24 11:12:25.664206 connection=1 Extents id=2 offset=0x0 count=0x200
req_one=1 ...
2019-04-24 11:12:25.664302 connection=1 ...Extents id=2 extents=[{ offset=0x0,
length=0x200, hole=0, zero=0 }] return=0

Therefore I'll declare that in this case the problem above of overlong
extents information is fixed.  I believe that was fixed when we
rewrote nbdkit_extents_new to take an ‘end’ parameter in the final
version of commit 4ca66f70a5.
> > 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]
$ ./nbdkit -fv --filter=log --filter=truncate \
           data data="1" size=512 truncate=64K logfile=/dev/stdout \
           --run 'qemu-img convert -n $nbd null-co://'  |& grep
Extents
2019-04-24 11:17:31.618246 connection=1 Extents id=2 offset=0x0 count=0x10000
req_one=1 ...
2019-04-24 11:17:31.618371 connection=1 ...Extents id=2 extents=[{ offset=0x0,
length=0x200, hole=0, zero=0 }] return=0
2019-04-24 11:17:31.659589 connection=1 Extents id=3 offset=0x200 count=0xfe00
req_one=1 ...
2019-04-24 11:17:31.659674 connection=1 ...Extents id=3 extents=[{ offset=0x200,
length=0xfe00, hole=1, zero=1 }] return=0

It is now returning:

  [length=0x200, type=data]
  [offset=0x200, length=0xfe00, type=hole|zero]

which looks correct to me.
> 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 moved the clamp as originally suggested in your email here:

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

The output from the commands above is the same and the tests pass.
Patch is below.

Rich.
>From a2b7ce7538742fa703d06d78b4159f0705775256 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones@redhat.com>
Date: Wed, 24 Apr 2019 12:24:07 +0100
Subject: [PATCH] common/sparse: Return the maximum amount of information about
 sparseness.

By moving the clamp we can return more information about sparseness
that we have but which was not requested.  This is permitted and can
improve the efficiency of clients.

See: https://www.redhat.com/archives/libguestfs/2019-April/msg00192.html

Thanks: Eric Blake.
---
 common/sparse/sparse.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/common/sparse/sparse.c b/common/sparse/sparse.c
index b46bd31..cb44743 100644
--- a/common/sparse/sparse.c
+++ b/common/sparse/sparse.c
@@ -370,8 +370,6 @@ sparse_array_extents (struct sparse_array *sa,
 
   while (count > 0) {
     p = lookup (sa, offset, false, &n, NULL);
-    if (n > count)
-      n = count;
 
     /* Work out the type of this extent. */
     if (p == NULL)
@@ -388,6 +386,9 @@ sparse_array_extents (struct sparse_array *sa,
     if (nbdkit_add_extent (extents, offset, n, type) == -1)
       return -1;
 
+    if (n > count)
+      n = count;
+
     count -= n;
     offset += n;
   }
-- 
2.20.1




-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On 4/24/19 6:26 AM, Richard W.M. Jones wrote:
>> 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 moved the clamp as originally suggested in your email here:
> 
> https://www.redhat.com/archives/libguestfs/2019-March/msg00126.html
> 
> The output from the commands above is the same and the tests pass.
> Patch is below.
> 
> Rich.
> 
>>From a2b7ce7538742fa703d06d78b4159f0705775256 Mon Sep 17 00:00:00 2001
> From: "Richard W.M. Jones" <rjones@redhat.com>
> Date: Wed, 24 Apr 2019 12:24:07 +0100
> Subject: [PATCH] common/sparse: Return the maximum amount of information
about
>  sparseness.
> 
> By moving the clamp we can return more information about sparseness
> that we have but which was not requested.  This is permitted and can
> improve the efficiency of clients.
> 
> See: https://www.redhat.com/archives/libguestfs/2019-April/msg00192.html
LGTM.

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