[PATCH 0/3] iotests: Fix 125

[PATCH 0/3] iotests: Fix 125

[Max Reitz]

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(-)

-- 
2.21.0

[PATCH 1/3] iotests: Fix 125 for growth_mode = metadata

[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

[PATCH 2/3] iotests: Disable 125 on broken XFS versions

[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

[PATCH 3/3] iotests: Use stat -c %b in 125

[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

Re: [PATCH 2/3] iotests: Disable 125 on broken XFS versions

[Eric Blake]

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

Re: [PATCH 1/3] iotests: Fix 125 for growth_mode = metadata

[Eric Blake]

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

Re: [PATCH 3/3] iotests: Use stat -c %b in 125

[Eric Blake]

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

Re: [PATCH 2/3] iotests: Disable 125 on broken XFS versions

[Max Reitz]

[-- 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 --]

Re: [PATCH 0/3] iotests: Fix 125

[Max Reitz]

[-- 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 --]