Re: [PATCH] Skip resizing image to the same size

Re: [PATCH] Skip resizing image to the same size

[Fahrzin Hemmati]

[-- Attachment #1: Type: text/plain, Size: 3782 bytes --]

My apologies, I saw the Signed-off-by requirement at first, but as I
followed the docs I got to "git publish" and didn't realize "git publish"
was actually able to send emails on my system (I don't remember setting up
any SMTP settings). By that time, I forgot and thought this patch was short
enough to not warrant much of a commit message.

The main practical advantage is for users that call "qemu-img resize" via
scripts or other systems (like Packer in my case) that don't check the
image size ahead of time. I can upstream this change into them (by using
"qemu-img info —output=json ...") but I figured it would be useful to more
users here.

This could trivially be added to block/io.c:bdrv_co_truncate(), as well as
blockdev.c:qmp_block_resize() with a little more work. I'm not familiar
with those workflows, but if needed I can do that as well.

Here's the new patch:

From 17f5c5f03d930c4816b92b97e0e54db0725d7b94 Mon Sep 17 00:00:00 2001
From: Fahrzin Hemmati <fahhem@fahhem.com>
Date: Mon, 20 Jan 2025 01:56:24 -0800
Subject: [PATCH] Skip resizing image to the same size

Higher-level software, such as Packer, blindly call "qemu-img resize"
even when the size is the same. This change can be pushed to those
users, but it's very cheap to check and can save many other users more
time here.

Signed-off-by: Fahrzin Hemmati <fahhem@fahhem.com>
---
qemu-img.c | 6 ++++++
tests/qemu-iotests/061 | 8 ++++++++
tests/qemu-iotests/061.out | 9 +++++++++
3 files changed, 23 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index 2f2fac90e8..3345c4e63f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4184,6 +4184,12 @@ static int img_resize(int argc, char **argv)
goto out;
}

+ if (total_size == current_size) {
+ qprintf(quiet, "Image already has the desired size.\n");
+ ret = 0;
+ goto out;
+ }
+
/*
* The user expects the image to have the desired size after
* resizing, so pass @exact=true. It is of no use to report
diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
index b71ac097d1..88aec4ebc6 100755
--- a/tests/qemu-iotests/061
+++ b/tests/qemu-iotests/061
@@ -150,6 +150,14 @@ _qcow2_dump_header | grep '^\(version\|size\|nb_snap\)'

_check_test_img

+echo
+echo "=== Testing resize to same size ==="
+echo
+_make_test_img -o "compat=0.10" 32M
+$QEMU_IMG resize "$TEST_IMG" 32M ||
+ echo "unexpected fail"
+_qcow2_dump_header | grep '^\(version\|size\|nb_snap\)'
+_check_test_img

echo
echo "=== Testing dirty lazy_refcounts=off ==="
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index 24c33add7c..d94eb9d513 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -299,6 +299,15 @@ size 33554432
nb_snapshots 1
No errors were found on the image.

+=== Testing resize to same size ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33554432
+Image already has the desired size.
+version 2
+size 33554432
+nb_snapshots 0
+No errors were found on the image.
+
=== Testing dirty lazy_refcounts=off ===

Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-- 
2.43.0

On Mon Jan 20, 2025, 12:23 PM GMT, Kevin Wolf <kwolf@redhat.com> wrote:

Am 20.01.2025 um 11:37 hat Fahrzin Hemmati geschrieben:

---
qemu-img.c | 6 ++++++
tests/qemu-iotests/061 | 8 ++++++++
tests/qemu-iotests/061.out | 9 +++++++++
3 files changed, 23 insertions(+)


This is lacking a commit message.

Please describe in it why this is change is made, i.e. which practical
advantages the change brings, and why having it in qemu-img, but not in
other places like the block_resize monitor command is desirable.

Also, without a Signed-off-by line, with which you sign the DCO, patches
can't be accepted into QEMU.

Kevin

[-- Attachment #2: Type: text/html, Size: 6153 bytes --]

Re: [PATCH] Skip resizing image to the same size

[Michael Tokarev]

20.01.2025 23:17, Fahrzin Hemmati wrote:
> My apologies, I saw the Signed-off-by requirement at first, but as I followed the docs I got to "git publish" and didn't realize "git publish" was 
> actually able to send emails on my system (I don't remember setting up any SMTP settings). By that time, I forgot and thought this patch was short 
> enough to not warrant much of a commit message.
> 
> The main practical advantage is for users that call "qemu-img resize" via scripts or other systems (like Packer in my case) that don't check the image 
> size ahead of time. I can upstream this change into them (by using "qemu-img info —output=json ...") but I figured it would be useful to more users here.
> 
> This could trivially be added to block/io.c:bdrv_co_truncate(), as well as blockdev.c:qmp_block_resize() with a little more work. I'm not familiar 
> with those workflows, but if needed I can do that as well.
> 
> Here's the new patch:
> 
>  From 17f5c5f03d930c4816b92b97e0e54db0725d7b94 Mon Sep 17 00:00:00 2001
> From: Fahrzin Hemmati <fahhem@fahhem.com <mailto:fahhem@fahhem.com>>
> Date: Mon, 20 Jan 2025 01:56:24 -0800
> Subject: [PATCH] Skip resizing image to the same size
> 
> Higher-level software, such as Packer, blindly call "qemu-img resize"
> even when the size is the same. This change can be pushed to those
> users, but it's very cheap to check and can save many other users more
> time here.
> 
> Signed-off-by: Fahrzin Hemmati <fahhem@fahhem.com <mailto:fahhem@fahhem.com>>
> ---
> qemu-img.c | 6 ++++++
> tests/qemu-iotests/061 | 8 ++++++++
> tests/qemu-iotests/061.out | 9 +++++++++
> 3 files changed, 23 insertions(+)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 2f2fac90e8..3345c4e63f 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -4184,6 +4184,12 @@ static int img_resize(int argc, char **argv)
> goto out;
> }
> 
> + if (total_size == current_size) {
> + qprintf(quiet, "Image already has the desired size.\n");
> + ret = 0;
> + goto out;
> + }


I don't think this is necessary: the actual operation is a no-op
anyway, there's no need to special-case it.

Please also refrain from changing qemu-img until my refresh
patchset is either accepted or rejected.  It was a large work
and it'd be sad to lose it.

Thanks,

/mjt

Re: [PATCH] Skip resizing image to the same size

[Fahrzin Hemmati]

[-- Attachment #1: Type: text/plain, Size: 2520 bytes --]

Happy to wait until your patchset is in.

Yes, this is a no-op, but it reads the entire disk image to perform that
no-op, so this is merely a time-saving improvement, not a behavior change.

On Mon Jan 20, 2025, 09:49 PM GMT, Michael Tokarev <mjt@tls.msk.ru> wrote:

20.01.2025 23:17, Fahrzin Hemmati wrote:

My apologies, I saw the Signed-off-by requirement at first, but as I
followed the docs I got to "git publish" and didn't realize "git publish"
was
actually able to send emails on my system (I don't remember setting up any
SMTP settings). By that time, I forgot and thought this patch was short
enough to not warrant much of a commit message.

The main practical advantage is for users that call "qemu-img resize" via
scripts or other systems (like Packer in my case) that don't check the
image
size ahead of time. I can upstream this change into them (by using
"qemu-img info —output=json ...") but I figured it would be useful to more
users here.

This could trivially be added to block/io.c:bdrv_co_truncate(), as well as
blockdev.c:qmp_block_resize() with a little more work. I'm not familiar
with those workflows, but if needed I can do that as well.

Here's the new patch:

From 17f5c5f03d930c4816b92b97e0e54db0725d7b94 Mon Sep 17 00:00:00 2001
From: Fahrzin Hemmati <fahhem@fahhem.com <mailto:fahhem@fahhem.com>>
Date: Mon, 20 Jan 2025 01:56:24 -0800
Subject: [PATCH] Skip resizing image to the same size

Higher-level software, such as Packer, blindly call "qemu-img resize"
even when the size is the same. This change can be pushed to those
users, but it's very cheap to check and can save many other users more
time here.

Signed-off-by: Fahrzin Hemmati <fahhem@fahhem.com <mailto:fahhem@fahhem.com
>>
---
qemu-img.c | 6 ++++++
tests/qemu-iotests/061 | 8 ++++++++
tests/qemu-iotests/061.out | 9 +++++++++
3 files changed, 23 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index 2f2fac90e8..3345c4e63f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4184,6 +4184,12 @@ static int img_resize(int argc, char **argv)
goto out;
}

+ if (total_size == current_size) {
+ qprintf(quiet, "Image already has the desired size.\n");
+ ret = 0;
+ goto out;
+ }



I don't think this is necessary: the actual operation is a no-op
anyway, there's no need to special-case it.

Please also refrain from changing qemu-img until my refresh
patchset is either accepted or rejected. It was a large work
and it'd be sad to lose it.

Thanks,

/mjt

[-- Attachment #2: Type: text/html, Size: 3220 bytes --]

Re: [PATCH] Skip resizing image to the same size

[Kevin Wolf]

Am 20.01.2025 um 23:21 hat Fahrzin Hemmati geschrieben:
> Happy to wait until your patchset is in.
> 
> Yes, this is a no-op, but it reads the entire disk image to perform that
> no-op, so this is merely a time-saving improvement, not a behavior change.

Can you give more context on what exactly you're doing that it reads the
entire disk image just to resize it? This sounds completely unexpected
and if it does, there may be a different problem to be solved.

In my test, for a raw image, I see a single ftruncate() call, which is
unnecessary, but shouldn't cause any measurable time difference for the
qemu-img run. qcow2 has a little more code in QEMU to figure out that
there is nothing to do, but it doesn't involve any syscall and certainly
not reading the whole image.

Kevin

> On Mon Jan 20, 2025, 09:49 PM GMT, Michael Tokarev <mjt@tls.msk.ru> wrote:
> 
> 20.01.2025 23:17, Fahrzin Hemmati wrote:
> 
> My apologies, I saw the Signed-off-by requirement at first, but as I
> followed the docs I got to "git publish" and didn't realize "git publish"
> was
> actually able to send emails on my system (I don't remember setting up any
> SMTP settings). By that time, I forgot and thought this patch was short
> enough to not warrant much of a commit message.
> 
> The main practical advantage is for users that call "qemu-img resize" via
> scripts or other systems (like Packer in my case) that don't check the
> image
> size ahead of time. I can upstream this change into them (by using
> "qemu-img info —output=json ...") but I figured it would be useful to more
> users here.
> 
> This could trivially be added to block/io.c:bdrv_co_truncate(), as well as
> blockdev.c:qmp_block_resize() with a little more work. I'm not familiar
> with those workflows, but if needed I can do that as well.
> 
> Here's the new patch:
> 
> From 17f5c5f03d930c4816b92b97e0e54db0725d7b94 Mon Sep 17 00:00:00 2001
> From: Fahrzin Hemmati <fahhem@fahhem.com <mailto:fahhem@fahhem.com>>
> Date: Mon, 20 Jan 2025 01:56:24 -0800
> Subject: [PATCH] Skip resizing image to the same size
> 
> Higher-level software, such as Packer, blindly call "qemu-img resize"
> even when the size is the same. This change can be pushed to those
> users, but it's very cheap to check and can save many other users more
> time here.
> 
> Signed-off-by: Fahrzin Hemmati <fahhem@fahhem.com <mailto:fahhem@fahhem.com
> >>
> ---
> qemu-img.c | 6 ++++++
> tests/qemu-iotests/061 | 8 ++++++++
> tests/qemu-iotests/061.out | 9 +++++++++
> 3 files changed, 23 insertions(+)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 2f2fac90e8..3345c4e63f 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -4184,6 +4184,12 @@ static int img_resize(int argc, char **argv)
> goto out;
> }
> 
> + if (total_size == current_size) {
> + qprintf(quiet, "Image already has the desired size.\n");
> + ret = 0;
> + goto out;
> + }
> 
> 
> 
> I don't think this is necessary: the actual operation is a no-op
> anyway, there's no need to special-case it.
> 
> Please also refrain from changing qemu-img until my refresh
> patchset is either accepted or rejected. It was a large work
> and it'd be sad to lose it.
> 
> Thanks,
> 
> /mjt

Re: [PATCH] Skip resizing image to the same size

[Fahrzin Hemmati]

[-- Attachment #1: Type: text/plain, Size: 3632 bytes --]

I'm running packer, and with a qcow2 source image sized at 75000MB (but
only 5GB on disk) when it runs "qemu-img resize ... 75000MB" it takes about
10 seconds on my system. I guess it's not reading the whole disk since me
nvme isn't that fast, but it's a non-trivial amount of work. It also runs a
qemu-img convert call (again, from qcow2 to qcow2) that seems more like a
cp, and that takes the same 10 seconds on my system.

On Tue Jan 21, 2025, 11:40 AM GMT, Kevin Wolf <kwolf@redhat.com> wrote:

Am 20.01.2025 um 23:21 hat Fahrzin Hemmati geschrieben:

Happy to wait until your patchset is in.

Yes, this is a no-op, but it reads the entire disk image to perform that
no-op, so this is merely a time-saving improvement, not a behavior change.


Can you give more context on what exactly you're doing that it reads the
entire disk image just to resize it? This sounds completely unexpected
and if it does, there may be a different problem to be solved.

In my test, for a raw image, I see a single ftruncate() call, which is
unnecessary, but shouldn't cause any measurable time difference for the
qemu-img run. qcow2 has a little more code in QEMU to figure out that
there is nothing to do, but it doesn't involve any syscall and certainly
not reading the whole image.

Kevin

On Mon Jan 20, 2025, 09:49 PM GMT, Michael Tokarev <mjt@tls.msk.ru> wrote:

20.01.2025 23:17, Fahrzin Hemmati wrote:

My apologies, I saw the Signed-off-by requirement at first, but as I
followed the docs I got to "git publish" and didn't realize "git publish"
was
actually able to send emails on my system (I don't remember setting up any
SMTP settings). By that time, I forgot and thought this patch was short
enough to not warrant much of a commit message.

The main practical advantage is for users that call "qemu-img resize" via
scripts or other systems (like Packer in my case) that don't check the
image
size ahead of time. I can upstream this change into them (by using
"qemu-img info —output=json ...") but I figured it would be useful to more
users here.

This could trivially be added to block/io.c:bdrv_co_truncate(), as well as
blockdev.c:qmp_block_resize() with a little more work. I'm not familiar
with those workflows, but if needed I can do that as well.

Here's the new patch:

From 17f5c5f03d930c4816b92b97e0e54db0725d7b94 Mon Sep 17 00:00:00 2001
From: Fahrzin Hemmati <fahhem@fahhem.com <mailto:fahhem@fahhem.com>>
Date: Mon, 20 Jan 2025 01:56:24 -0800
Subject: [PATCH] Skip resizing image to the same size

Higher-level software, such as Packer, blindly call "qemu-img resize"
even when the size is the same. This change can be pushed to those
users, but it's very cheap to check and can save many other users more
time here.

Signed-off-by: Fahrzin Hemmati <fahhem@fahhem.com <mailto:fahhem@fahhem.com
>>
---
qemu-img.c | 6 ++++++
tests/qemu-iotests/061 | 8 ++++++++
tests/qemu-iotests/061.out | 9 +++++++++
3 files changed, 23 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index 2f2fac90e8..3345c4e63f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4184,6 +4184,12 @@ static int img_resize(int argc, char **argv)
goto out;
}

+ if (total_size == current_size) {
+ qprintf(quiet, "Image already has the desired size.\n");
+ ret = 0;
+ goto out;
+ }



I don't think this is necessary: the actual operation is a no-op
anyway, there's no need to special-case it.

Please also refrain from changing qemu-img until my refresh
patchset is either accepted or rejected. It was a large work
and it'd be sad to lose it.

Thanks,

/mjt

[-- Attachment #2: Type: text/html, Size: 4472 bytes --]