6:26 a.m.
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(a)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/