* [PATCH 1/3] iotests: Fix 125 for growth_mode = metadata
2019-09-25 18:32 [PATCH 0/3] iotests: Fix 125 Max Reitz
@ 2019-09-25 18:32 ` Max Reitz
2019-09-25 21:29 ` Eric Blake
2019-09-25 18:32 ` [PATCH 2/3] iotests: Disable 125 on broken XFS versions Max Reitz
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Max Reitz @ 2019-09-25 18:32 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
If we use growth_mode = metadata, it is very much possible that the file
uses more disk space after we have written something to the added area.
We did indeed want to test for this case, but unfortunately we evidently
just copied the code from the "Test creation preallocation" section and
forgot to replace "$create_mode" by "$growth_mode".
We never noticed because we only read the first number from qemu-img
info's "disk size" output -- and that is effectively useless, because
qemu-img prints a human-readable value (which generally includes a
decimal point). That will be fixed in the patch after the next one.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/125 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/125 b/tests/qemu-iotests/125
index dc4b8f5fb9..df328a63a6 100755
--- a/tests/qemu-iotests/125
+++ b/tests/qemu-iotests/125
@@ -111,7 +111,7 @@ for GROWTH_SIZE in 16 48 80; do
if [ $file_length_2 -gt $file_length_1 ]; then
echo "ERROR (grow): Image length has grown from $file_length_1 to $file_length_2"
fi
- if [ $create_mode != metadata ]; then
+ if [ $growth_mode != metadata ]; then
# The host size should not have grown either
if [ $host_size_2 -gt $host_size_1 ]; then
echo "ERROR (grow): Host size has grown from $host_size_1 to $host_size_2"
--
2.21.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] iotests: Fix 125 for growth_mode = metadata
2019-09-25 18:32 ` [PATCH 1/3] iotests: Fix 125 for growth_mode = metadata Max Reitz
@ 2019-09-25 21:29 ` Eric Blake
0 siblings, 0 replies; 9+ messages in thread
From: Eric Blake @ 2019-09-25 21:29 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On 9/25/19 1:32 PM, Max Reitz wrote:
> If we use growth_mode = metadata, it is very much possible that the file
> uses more disk space after we have written something to the added area.
> We did indeed want to test for this case, but unfortunately we evidently
> just copied the code from the "Test creation preallocation" section and
> forgot to replace "$create_mode" by "$growth_mode".
>
> We never noticed because we only read the first number from qemu-img
> info's "disk size" output -- and that is effectively useless, because
> qemu-img prints a human-readable value (which generally includes a
> decimal point). That will be fixed in the patch after the next one.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/125 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
> diff --git a/tests/qemu-iotests/125 b/tests/qemu-iotests/125
> index dc4b8f5fb9..df328a63a6 100755
> --- a/tests/qemu-iotests/125
> +++ b/tests/qemu-iotests/125
> @@ -111,7 +111,7 @@ for GROWTH_SIZE in 16 48 80; do
> if [ $file_length_2 -gt $file_length_1 ]; then
> echo "ERROR (grow): Image length has grown from $file_length_1 to $file_length_2"
> fi
> - if [ $create_mode != metadata ]; then
> + if [ $growth_mode != metadata ]; then
> # The host size should not have grown either
> if [ $host_size_2 -gt $host_size_1 ]; then
> echo "ERROR (grow): Host size has grown from $host_size_1 to $host_size_2"
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] iotests: Disable 125 on broken XFS versions
2019-09-25 18:32 [PATCH 0/3] iotests: Fix 125 Max Reitz
2019-09-25 18:32 ` [PATCH 1/3] iotests: Fix 125 for growth_mode = metadata Max Reitz
@ 2019-09-25 18:32 ` Max Reitz
2019-09-25 21:28 ` Eric Blake
2019-09-25 18:32 ` [PATCH 3/3] iotests: Use stat -c %b in 125 Max Reitz
2019-09-27 9:29 ` [PATCH 0/3] iotests: Fix 125 Max Reitz
3 siblings, 1 reply; 9+ messages in thread
From: Max Reitz @ 2019-09-25 18:32 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
And by that I mean all XFS versions, as far as I can tell. All details
are in the comment below.
We never noticed this problem because we only read the first number from
qemu-img info's "disk size" output -- and that is effectively useless,
because qemu-img prints a human-readable value (which generally includes
a decimal point). That will be fixed in the next patch.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/125 | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/tests/qemu-iotests/125 b/tests/qemu-iotests/125
index df328a63a6..0ef51f1e21 100755
--- a/tests/qemu-iotests/125
+++ b/tests/qemu-iotests/125
@@ -49,6 +49,46 @@ if [ -z "$TEST_IMG_FILE" ]; then
TEST_IMG_FILE=$TEST_IMG
fi
+# Test whether we are running on a broken XFS version. There is this
+# bug:
+
+# $ rm -f foo
+# $ touch foo
+# $ block_size=4096 # Your FS's block size
+# $ fallocate -o $((block_size / 2)) -l $block_size foo
+# $ LANG=C xfs_bmap foo | grep hole
+# 1: [8..15]: hole
+#
+# The problem is that the XFS driver rounds down the offset and
+# rounds up the length to the block size, but independently. As
+# such, it only allocates the first block in the example above,
+# even though it should allocate the first two blocks (because our
+# request is to fallocate something that touches both the first
+# two blocks).
+#
+# This means that when you then write to the beginning of the
+# second block, the disk usage of the first two blocks grows.
+#
+# That is precisely what fallocate() promises, though: That when you
+# write to an area that you have fallocated, no new blocks will have
+# to be allocated.
+
+touch "$TEST_IMG_FILE"
+# Assuming there is no FS with a block size greater than 64k
+fallocate -o 65535 -l 2 "$TEST_IMG_FILE"
+len0=$(get_image_size_on_host)
+
+# Write to something that in theory we have just fallocated
+# (Thus, the on-disk size should not increase)
+poke_file "$TEST_IMG_FILE" 65536 42
+len1=$(get_image_size_on_host)
+
+if [ $len1 -gt $len0 ]; then
+ _notrun "the test filesystem's fallocate() is broken"
+fi
+
+rm -f "$TEST_IMG_FILE"
+
# Generally, we create some image with or without existing preallocation and
# then resize it. Then we write some data into the image and verify that its
# size does not change if we have used preallocation.
--
2.21.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 2/3] iotests: Disable 125 on broken XFS versions
2019-09-25 18:32 ` [PATCH 2/3] iotests: Disable 125 on broken XFS versions Max Reitz
@ 2019-09-25 21:28 ` Eric Blake
2019-09-26 10:58 ` Max Reitz
0 siblings, 1 reply; 9+ messages in thread
From: Eric Blake @ 2019-09-25 21:28 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On 9/25/19 1:32 PM, Max Reitz wrote:
> And by that I mean all XFS versions, as far as I can tell. All details
> are in the comment below.
>
> We never noticed this problem because we only read the first number from
> qemu-img info's "disk size" output -- and that is effectively useless,
> because qemu-img prints a human-readable value (which generally includes
> a decimal point). That will be fixed in the next patch.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/125 | 40 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
>
> diff --git a/tests/qemu-iotests/125 b/tests/qemu-iotests/125
> index df328a63a6..0ef51f1e21 100755
> --- a/tests/qemu-iotests/125
> +++ b/tests/qemu-iotests/125
> @@ -49,6 +49,46 @@ if [ -z "$TEST_IMG_FILE" ]; then
> TEST_IMG_FILE=$TEST_IMG
> fi
>
> +# Test whether we are running on a broken XFS version. There is this
> +# bug:
> +
> +# $ rm -f foo
> +# $ touch foo
> +# $ block_size=4096 # Your FS's block size
> +# $ fallocate -o $((block_size / 2)) -l $block_size foo
> +# $ LANG=C xfs_bmap foo | grep hole
> +# 1: [8..15]: hole
> +#
> +# The problem is that the XFS driver rounds down the offset and
> +# rounds up the length to the block size, but independently.
Eww. I concur you uncovered a bug. Have you reported this to xfs folks?
> +
> +touch "$TEST_IMG_FILE"
> +# Assuming there is no FS with a block size greater than 64k
> +fallocate -o 65535 -l 2 "$TEST_IMG_FILE"
> +len0=$(get_image_size_on_host)
> +
> +# Write to something that in theory we have just fallocated
> +# (Thus, the on-disk size should not increase)
> +poke_file "$TEST_IMG_FILE" 65536 42
> +len1=$(get_image_size_on_host)
> +
> +if [ $len1 -gt $len0 ]; then
> + _notrun "the test filesystem's fallocate() is broken"
> +fi
> +
> +rm -f "$TEST_IMG_FILE"
Reviewed-by: Eric Blake <eblake@redhat.com>
> +
> # Generally, we create some image with or without existing preallocation and
> # then resize it. Then we write some data into the image and verify that its
> # size does not change if we have used preallocation.
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] iotests: Disable 125 on broken XFS versions
2019-09-25 21:28 ` Eric Blake
@ 2019-09-26 10:58 ` Max Reitz
0 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2019-09-26 10:58 UTC (permalink / raw)
To: Eric Blake, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1590 bytes --]
On 25.09.19 23:28, Eric Blake wrote:
> On 9/25/19 1:32 PM, Max Reitz wrote:
>> And by that I mean all XFS versions, as far as I can tell. All details
>> are in the comment below.
>>
>> We never noticed this problem because we only read the first number from
>> qemu-img info's "disk size" output -- and that is effectively useless,
>> because qemu-img prints a human-readable value (which generally includes
>> a decimal point). That will be fixed in the next patch.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> tests/qemu-iotests/125 | 40 ++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 40 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/125 b/tests/qemu-iotests/125
>> index df328a63a6..0ef51f1e21 100755
>> --- a/tests/qemu-iotests/125
>> +++ b/tests/qemu-iotests/125
>> @@ -49,6 +49,46 @@ if [ -z "$TEST_IMG_FILE" ]; then
>> TEST_IMG_FILE=$TEST_IMG
>> fi
>> +# Test whether we are running on a broken XFS version. There is this
>> +# bug:
>> +
>> +# $ rm -f foo
>> +# $ touch foo
>> +# $ block_size=4096 # Your FS's block size
>> +# $ fallocate -o $((block_size / 2)) -l $block_size foo
>> +# $ LANG=C xfs_bmap foo | grep hole
>> +# 1: [8..15]: hole
>> +#
>> +# The problem is that the XFS driver rounds down the offset and
>> +# rounds up the length to the block size, but independently.
>
> Eww. I concur you uncovered a bug. Have you reported this to xfs folks?
I have now. Took a bit of kernel compiling to see whether what I think
would fix it works.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] iotests: Use stat -c %b in 125
2019-09-25 18:32 [PATCH 0/3] iotests: Fix 125 Max Reitz
2019-09-25 18:32 ` [PATCH 1/3] iotests: Fix 125 for growth_mode = metadata Max Reitz
2019-09-25 18:32 ` [PATCH 2/3] iotests: Disable 125 on broken XFS versions Max Reitz
@ 2019-09-25 18:32 ` Max Reitz
2019-09-25 21:31 ` Eric Blake
2019-09-27 9:29 ` [PATCH 0/3] iotests: Fix 125 Max Reitz
3 siblings, 1 reply; 9+ messages in thread
From: Max Reitz @ 2019-09-25 18:32 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
125 should not use qemu-img to get the on-disk image size, because that
reports it in a human-readable format that is useless to us. Just use
stat instead (like we do to get the image file length).
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/125 | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/125 b/tests/qemu-iotests/125
index 0ef51f1e21..4e31aa4e5f 100755
--- a/tests/qemu-iotests/125
+++ b/tests/qemu-iotests/125
@@ -34,8 +34,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
get_image_size_on_host()
{
- $QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep "disk size" \
- | sed -e 's/^[^0-9]*\([0-9]\+\).*$/\1/'
+ echo $(($(stat -c '%b * %B' "$TEST_IMG_FILE")))
}
# get standard environment and filters
--
2.21.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 3/3] iotests: Use stat -c %b in 125
2019-09-25 18:32 ` [PATCH 3/3] iotests: Use stat -c %b in 125 Max Reitz
@ 2019-09-25 21:31 ` Eric Blake
0 siblings, 0 replies; 9+ messages in thread
From: Eric Blake @ 2019-09-25 21:31 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On 9/25/19 1:32 PM, Max Reitz wrote:
> 125 should not use qemu-img to get the on-disk image size, because that
> reports it in a human-readable format that is useless to us. Just use
> stat instead (like we do to get the image file length).
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/125 | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/tests/qemu-iotests/125 b/tests/qemu-iotests/125
> index 0ef51f1e21..4e31aa4e5f 100755
> --- a/tests/qemu-iotests/125
> +++ b/tests/qemu-iotests/125
> @@ -34,8 +34,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>
> get_image_size_on_host()
> {
> - $QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep "disk size" \
> - | sed -e 's/^[^0-9]*\([0-9]\+\).*$/\1/'
> + echo $(($(stat -c '%b * %B' "$TEST_IMG_FILE")))
Cute use of $(()) around $().
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] 9+ messages in thread
* Re: [PATCH 0/3] iotests: Fix 125
2019-09-25 18:32 [PATCH 0/3] iotests: Fix 125 Max Reitz
` (2 preceding siblings ...)
2019-09-25 18:32 ` [PATCH 3/3] iotests: Use stat -c %b in 125 Max Reitz
@ 2019-09-27 9:29 ` Max Reitz
3 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2019-09-27 9:29 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1570 bytes --]
On 25.09.19 20:32, Max Reitz wrote:
> Hi,
>
> iotest 125 is very broken. It uses qemu-img info’s “disk size” to
> determine an image’s on-disk size, but it does so in a wrong way: It
> just fetches the first number ([0-9]+), but that isn’t very useful
> because qemu-img info emits human-readable values that include units and
> decimal points.
>
> We should ust stat -c %b instead. That’s done in patch 3.
> Unfortunately, doing so exposed more problems.
>
> Patch 1 fixes a stupid bug in the test itself that we never noticed
> because of what patch 3 fixes. (Pull patch 3 before patch 1 and you’ll
> see.)
>
> The other thing is actually a bug in XFS. Its fallocate()
> implementation rounds up the length independently of the offset, so if
> you try to fallocate an unaligned range, chances are that it might not
> allocate the last block your range touches. Patch 2 detects that case
> and skips the test then. (Pull patch 3 before patch 2 and you’ll see
> the test fail on XFS.)
>
>
> Max Reitz (3):
> iotests: Fix 125 for growth_mode = metadata
> iotests: Disable 125 on broken XFS versions
> iotests: Use stat -c %b in 125
>
> tests/qemu-iotests/125 | 45 +++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 42 insertions(+), 3 deletions(-)
Thanks for the review, applied to my block branch:
https://git.xanclic.moe/XanClic/qemu/commits/branch/block
And for the record, I’ve sent a patch to the XFS driver:
https://www.spinics.net/lists/linux-xfs/msg32174.html
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread