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
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
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
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
© 2016 - 2024 Red Hat, Inc.