[PATCH] qemu-img map: Don't limit block status request size

[PATCH] qemu-img map: Don't limit block status request size

[Kevin Wolf]

Limiting each loop iteration of qemu-img map to 1 GB was arbitrary from
the beginning, though it only cut the maximum in half then because the
interface a signed 32 bit byte count. These days, bdrv_block_status()
supports a 64 bit byte count, so the arbitrary limit is even worse.

On file-posix, bdrv_block_status() eventually maps to SEEK_HOLE and
SEEK_DATA, which don't support a limit, but always do all of the work
necessary to find the start of the next hole/data. Much of this work may
be repeated if we don't use this information fully, but query with an
only slightly larger offset in the next loop iteration. Therefore, if
bdrv_block_status() is called in a loop, it should always pass the
full number of bytes that the whole loop is interested in.

This removes the arbitrary limit and speeds up 'qemu-img map'
significantly on heavily fragmented images.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index bdb9f6aa46..74946f81ca 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3217,12 +3217,9 @@ static int img_map(int argc, char **argv)
     curr.start = start_offset;
     while (curr.start + curr.length < length) {
         int64_t offset = curr.start + curr.length;
-        int64_t n;
+        int64_t n = length - offset;
 
-        /* Probe up to 1 GiB at a time.  */
-        n = MIN(1 * GiB, length - offset);
         ret = get_block_status(bs, offset, n, &next);
-
         if (ret < 0) {
             error_report("Could not read file metadata: %s", strerror(-ret));
             goto out;
-- 
2.25.4

Re: [PATCH] qemu-img map: Don't limit block status request size

[Eric Blake]

On 7/7/20 9:46 AM, Kevin Wolf wrote:
> Limiting each loop iteration of qemu-img map to 1 GB was arbitrary from
> the beginning, though it only cut the maximum in half then because the
> interface a signed 32 bit byte count. These days, bdrv_block_status()
> supports a 64 bit byte count, so the arbitrary limit is even worse.
> 
> On file-posix, bdrv_block_status() eventually maps to SEEK_HOLE and
> SEEK_DATA, which don't support a limit, but always do all of the work
> necessary to find the start of the next hole/data. Much of this work may
> be repeated if we don't use this information fully, but query with an
> only slightly larger offset in the next loop iteration. Therefore, if
> bdrv_block_status() is called in a loop, it should always pass the
> full number of bytes that the whole loop is interested in.
> 
> This removes the arbitrary limit and speeds up 'qemu-img map'
> significantly on heavily fragmented images.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   qemu-img.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)

Do you want this in 5.1?  It seems like a nice optimization.

> 
> diff --git a/qemu-img.c b/qemu-img.c
> index bdb9f6aa46..74946f81ca 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3217,12 +3217,9 @@ static int img_map(int argc, char **argv)
>       curr.start = start_offset;
>       while (curr.start + curr.length < length) {
>           int64_t offset = curr.start + curr.length;
> -        int64_t n;
> +        int64_t n = length - offset;
>   
> -        /* Probe up to 1 GiB at a time.  */
> -        n = MIN(1 * GiB, length - offset);
>           ret = get_block_status(bs, offset, n, &next);
> -
>           if (ret < 0) {
>               error_report("Could not read file metadata: %s", strerror(-ret));
>               goto out;
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH] qemu-img map: Don't limit block status request size

[Kevin Wolf]

Am 07.07.2020 um 16:54 hat Eric Blake geschrieben:
> On 7/7/20 9:46 AM, Kevin Wolf wrote:
> > Limiting each loop iteration of qemu-img map to 1 GB was arbitrary from
> > the beginning, though it only cut the maximum in half then because the
> > interface a signed 32 bit byte count. These days, bdrv_block_status()
> > supports a 64 bit byte count, so the arbitrary limit is even worse.
> > 
> > On file-posix, bdrv_block_status() eventually maps to SEEK_HOLE and
> > SEEK_DATA, which don't support a limit, but always do all of the work
> > necessary to find the start of the next hole/data. Much of this work may
> > be repeated if we don't use this information fully, but query with an
> > only slightly larger offset in the next loop iteration. Therefore, if
> > bdrv_block_status() is called in a loop, it should always pass the
> > full number of bytes that the whole loop is interested in.
> > 
> > This removes the arbitrary limit and speeds up 'qemu-img map'
> > significantly on heavily fragmented images.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   qemu-img.c | 5 +----
> >   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> Do you want this in 5.1?  It seems like a nice optimization.

I guess now that I have your R-b, I can sneak both patches in for soft
freeze. :-)

Kevin

Re: [PATCH] qemu-img map: Don't limit block status request size

[Eric Blake]

On 7/7/20 10:21 AM, Kevin Wolf wrote:
> Am 07.07.2020 um 16:54 hat Eric Blake geschrieben:
>> On 7/7/20 9:46 AM, Kevin Wolf wrote:
>>> Limiting each loop iteration of qemu-img map to 1 GB was arbitrary from
>>> the beginning, though it only cut the maximum in half then because the
>>> interface a signed 32 bit byte count. These days, bdrv_block_status()

interface was a

>>> supports a 64 bit byte count, so the arbitrary limit is even worse.
>>>
>>> On file-posix, bdrv_block_status() eventually maps to SEEK_HOLE and
>>> SEEK_DATA, which don't support a limit, but always do all of the work
>>> necessary to find the start of the next hole/data. Much of this work may
>>> be repeated if we don't use this information fully, but query with an
>>> only slightly larger offset in the next loop iteration. Therefore, if
>>> bdrv_block_status() is called in a loop, it should always pass the
>>> full number of bytes that the whole loop is interested in.
>>>
>>> This removes the arbitrary limit and speeds up 'qemu-img map'
>>> significantly on heavily fragmented images.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>    qemu-img.c | 5 +----
>>>    1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> Do you want this in 5.1?  It seems like a nice optimization.
> 
> I guess now that I have your R-b, I can sneak both patches in for soft
> freeze. :-)

Can we treat optimizations for speed problems as bug fixes if they land 
after soft freeze but before -rc1?

At any rate, this post reminds me that Vladimir's series to support 
64-bit actions elsewhere has probably slipped into 5.2 territory, but I 
still need to fix the fact that nbd is sending uint32_t trim/zero values 
into signed int block layer functions (Vladimir's work is nicer but 
harder to review, so I'll probably end up writing one-off patches for 
5.1 with minimal churn to just block/nbd.c rather than the whole block 
layer...)

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