* [PATCH] qemu-img map: Don't limit block status request size
@ 2020-07-07 14:46 Kevin Wolf
2020-07-07 14:54 ` Eric Blake
0 siblings, 1 reply; 4+ messages in thread
From: Kevin Wolf @ 2020-07-07 14:46 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] qemu-img map: Don't limit block status request size
2020-07-07 14:46 [PATCH] qemu-img map: Don't limit block status request size Kevin Wolf
@ 2020-07-07 14:54 ` Eric Blake
2020-07-07 15:21 ` Kevin Wolf
0 siblings, 1 reply; 4+ messages in thread
From: Eric Blake @ 2020-07-07 14:54 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: peter.maydell, qemu-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] qemu-img map: Don't limit block status request size
2020-07-07 14:54 ` Eric Blake
@ 2020-07-07 15:21 ` Kevin Wolf
2020-07-07 15:30 ` Eric Blake
0 siblings, 1 reply; 4+ messages in thread
From: Kevin Wolf @ 2020-07-07 15:21 UTC (permalink / raw)
To: Eric Blake; +Cc: peter.maydell, qemu-devel, qemu-block
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] qemu-img map: Don't limit block status request size
2020-07-07 15:21 ` Kevin Wolf
@ 2020-07-07 15:30 ` Eric Blake
0 siblings, 0 replies; 4+ messages in thread
From: Eric Blake @ 2020-07-07 15:30 UTC (permalink / raw)
To: Kevin Wolf; +Cc: peter.maydell, qemu-devel, qemu-block
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-07-07 15:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-07 14:46 [PATCH] qemu-img map: Don't limit block status request size Kevin Wolf
2020-07-07 14:54 ` Eric Blake
2020-07-07 15:21 ` Kevin Wolf
2020-07-07 15:30 ` Eric Blake
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.