[Qemu-devel] [PATCH 0/2] block/file-posix: Fix xfs_write_zeroes()

[Qemu-devel] [PATCH 0/2] block/file-posix: Fix xfs_write_zeroes()

[Max Reitz]

Lukàš ran over a nasty regression in our xfs_write_zeroes() function
(sorry, my fault) made apparent by a recent patch from Anton that makes
qcow2 images heavily exercise the offending code path.

This series fixes the bug and adds a test to prevent it from
reoccurring.


Max Reitz (2):
  block/file-posix: Fix xfs_write_zeroes()
  iotests: Test reverse sub-cluster qcow2 writes

 block/file-posix.c         | 16 ++++++---
 tests/qemu-iotests/265     | 67 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/265.out |  6 ++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 85 insertions(+), 5 deletions(-)
 create mode 100755 tests/qemu-iotests/265
 create mode 100644 tests/qemu-iotests/265.out

-- 
2.21.0

[Qemu-devel] [PATCH 1/2] block/file-posix: Fix xfs_write_zeroes()

[Max Reitz]

Calling ftruncate() in xfs_write_zeroes() is dangerous because it may
yield and then discard data that parallel write requests have written
past the old EOF in the meantime.  We must not use it here.

Instead, return -ENOTSUP and let the more generic fallocate code handle
writing zeroes past the EOF.

Reported-by: Lukáš Doktor <ldoktor@redhat.com>
Fixes: 50ba5b2d994853b38fed10e0841b119da0f8b8e5
Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/file-posix.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index fbeb0068db..b49e0784a4 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1472,10 +1472,13 @@ static int xfs_write_zeroes(BDRVRawState *s, int64_t offset, uint64_t bytes)
     }
 
     if (offset + bytes > len) {
-        /* XFS_IOC_ZERO_RANGE does not increase the file length */
-        if (ftruncate(s->fd, offset + bytes) < 0) {
-            return -errno;
-        }
+        /*
+         * XFS_IOC_ZERO_RANGE does not increase the file length, but
+         * the caller probably wants us to.
+         * Calling ftruncate() would not be safe, so let the generic
+         * implementation handle this case.
+         */
+        return -ENOTSUP;
     }
 
     memset(&fl, 0, sizeof(fl));
@@ -1580,7 +1583,10 @@ static int handle_aiocb_write_zeroes(void *opaque)
 
 #ifdef CONFIG_XFS
     if (s->is_xfs) {
-        return xfs_write_zeroes(s, aiocb->aio_offset, aiocb->aio_nbytes);
+        int ret = xfs_write_zeroes(s, aiocb->aio_offset, aiocb->aio_nbytes);
+        if (ret != -ENOTSUP) {
+            return ret;
+        }
     }
 #endif
 
-- 
2.21.0

[Qemu-devel] [PATCH 2/2] iotests: Test reverse sub-cluster qcow2 writes

[Max Reitz]

This exercises the regression introduced in commit
50ba5b2d994853b38fed10e0841b119da0f8b8e5.  On my machine, it has close
to a 50 % false-negative rate, but that should still be sufficient to
test the fix.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/265     | 67 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/265.out |  6 ++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 74 insertions(+)
 create mode 100755 tests/qemu-iotests/265
 create mode 100644 tests/qemu-iotests/265.out

diff --git a/tests/qemu-iotests/265 b/tests/qemu-iotests/265
new file mode 100755
index 0000000000..dce6f77be3
--- /dev/null
+++ b/tests/qemu-iotests/265
@@ -0,0 +1,67 @@
+#!/usr/bin/env bash
+#
+# Test reverse-ordered qcow2 writes on a sub-cluster level
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+seq=$(basename $0)
+echo "QA output created by $seq"
+
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# qcow2-specific test
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+echo '--- Writing to the image ---'
+
+# Reduce cluster size so we get more and quicker I/O
+IMGOPTS='cluster_size=4096' _make_test_img 1M
+(for ((kb = 1024 - 4; kb >= 0; kb -= 4)); do \
+     echo "aio_write -P 42 $((kb + 1))k 2k"; \
+ done) \
+ | $QEMU_IO "$TEST_IMG" > /dev/null
+
+echo '--- Verifying its content ---'
+
+(for ((kb = 0; kb < 1024; kb += 4)); do \
+    echo "read -P 0 ${kb}k 1k"; \
+    echo "read -P 42 $((kb + 1))k 2k"; \
+    echo "read -P 0 $((kb + 3))k 1k"; \
+ done) \
+ | $QEMU_IO "$TEST_IMG" | _filter_qemu_io | grep 'verification'
+
+# Status of qemu-io
+if [ ${PIPESTATUS[1]} = 0 ]; then
+    echo 'Content verified.'
+fi
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/265.out b/tests/qemu-iotests/265.out
new file mode 100644
index 0000000000..6eac620f25
--- /dev/null
+++ b/tests/qemu-iotests/265.out
@@ -0,0 +1,6 @@
+QA output created by 265
+--- Writing to the image ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+--- Verifying its content ---
+Content verified.
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index d95d556414..0c129c1644 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -274,3 +274,4 @@
 257 rw
 258 rw quick
 262 rw quick migration
+265 rw auto quick
-- 
2.21.0

Re: [Qemu-devel] [PATCH 0/2] block/file-posix: Fix xfs_write_zeroes()

[Paolo Bonzini]

On 22/08/19 18:26, Max Reitz wrote:
> Lukàš ran over a nasty regression in our xfs_write_zeroes() function
> (sorry, my fault) made apparent by a recent patch from Anton that makes
> qcow2 images heavily exercise the offending code path.
> 
> This series fixes the bug and adds a test to prevent it from
> reoccurring.
> 
> 
> Max Reitz (2):
>   block/file-posix: Fix xfs_write_zeroes()
>   iotests: Test reverse sub-cluster qcow2 writes
> 
>  block/file-posix.c         | 16 ++++++---
>  tests/qemu-iotests/265     | 67 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/265.out |  6 ++++
>  tests/qemu-iotests/group   |  1 +
>  4 files changed, 85 insertions(+), 5 deletions(-)
>  create mode 100755 tests/qemu-iotests/265
>  create mode 100644 tests/qemu-iotests/265.out
> 

What about just killing libxfs support and only use fallocate?
FALLOC_FL_ZERO_RANGE was added in Linux 3.15 (2014) and the only
platform we probably support with such an old kernel is of course
RHEL/CentOS 7 which has had it backported.

Paolo

Re: [Qemu-devel] [PATCH 0/2] block/file-posix: Fix xfs_write_zeroes()

[Max Reitz]

On 22.08.19 18:53, Paolo Bonzini wrote:
> On 22/08/19 18:26, Max Reitz wrote:
>> Lukàš ran over a nasty regression in our xfs_write_zeroes() function
>> (sorry, my fault) made apparent by a recent patch from Anton that makes
>> qcow2 images heavily exercise the offending code path.
>>
>> This series fixes the bug and adds a test to prevent it from
>> reoccurring.
>>
>>
>> Max Reitz (2):
>>   block/file-posix: Fix xfs_write_zeroes()
>>   iotests: Test reverse sub-cluster qcow2 writes
>>
>>  block/file-posix.c         | 16 ++++++---
>>  tests/qemu-iotests/265     | 67 ++++++++++++++++++++++++++++++++++++++
>>  tests/qemu-iotests/265.out |  6 ++++
>>  tests/qemu-iotests/group   |  1 +
>>  4 files changed, 85 insertions(+), 5 deletions(-)
>>  create mode 100755 tests/qemu-iotests/265
>>  create mode 100644 tests/qemu-iotests/265.out
>>
> 
> What about just killing libxfs support and only use fallocate?
> FALLOC_FL_ZERO_RANGE was added in Linux 3.15 (2014) and the only
> platform we probably support with such an old kernel is of course
> RHEL/CentOS 7 which has had it backported.

Works just as well for me. :-)

Max

Re: [Qemu-devel] [PATCH 0/2] block/file-posix: Fix xfs_write_zeroes()

[Lukáš Doktor]

[-- Attachment #1.1: Type: text/plain, Size: 1454 bytes --]

Dne 22. 08. 19 v 19:09 Max Reitz napsal(a):
> On 22.08.19 18:53, Paolo Bonzini wrote:
>> On 22/08/19 18:26, Max Reitz wrote:
>>> Lukàš ran over a nasty regression in our xfs_write_zeroes() function
>>> (sorry, my fault) made apparent by a recent patch from Anton that makes
>>> qcow2 images heavily exercise the offending code path.
>>>
>>> This series fixes the bug and adds a test to prevent it from
>>> reoccurring.
>>>
>>>
>>> Max Reitz (2):
>>>   block/file-posix: Fix xfs_write_zeroes()
>>>   iotests: Test reverse sub-cluster qcow2 writes
>>>
>>>  block/file-posix.c         | 16 ++++++---
>>>  tests/qemu-iotests/265     | 67 ++++++++++++++++++++++++++++++++++++++
>>>  tests/qemu-iotests/265.out |  6 ++++
>>>  tests/qemu-iotests/group   |  1 +
>>>  4 files changed, 85 insertions(+), 5 deletions(-)
>>>  create mode 100755 tests/qemu-iotests/265
>>>  create mode 100644 tests/qemu-iotests/265.out
>>>
>>
>> What about just killing libxfs support and only use fallocate?
>> FALLOC_FL_ZERO_RANGE was added in Linux 3.15 (2014) and the only
>> platform we probably support with such an old kernel is of course
>> RHEL/CentOS 7 which has had it backported.
> 
> Works just as well for me. :-)
> 
> Max
> 

Whatever you decide, anyway this patch fixes my arm installation issue and the attached qemu iotest is also passing on my system.

Thank you,
Lukáš

Tested-by: Lukáš Doktor <ldoktor@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 0/2] block/file-posix: Fix xfs_write_zeroes()

[Anton Nefedov]

On 22/8/2019 7:26 PM, Max Reitz wrote:
> Lukàš ran over a nasty regression in our xfs_write_zeroes() function
> (sorry, my fault) made apparent by a recent patch from Anton that makes
> qcow2 images heavily exercise the offending code path.
> 
> This series fixes the bug and adds a test to prevent it from
> reoccurring.
> 
> 
> Max Reitz (2):
>    block/file-posix: Fix xfs_write_zeroes()
>    iotests: Test reverse sub-cluster qcow2 writes
> 
>   block/file-posix.c         | 16 ++++++---
>   tests/qemu-iotests/265     | 67 ++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/265.out |  6 ++++
>   tests/qemu-iotests/group   |  1 +
>   4 files changed, 85 insertions(+), 5 deletions(-)
>   create mode 100755 tests/qemu-iotests/265
>   create mode 100644 tests/qemu-iotests/265.out
> 

Nice! thanks Max

Reviewed-by: Anton Nefedov <anton.nefedov@virtuozzo.com>

[Qemu-devel] [PATCH 0/2] block/file-posix: Reduce xfsctl() use

[Max Reitz]

Hi,

As suggested by Paolo, this series drops xfsctl() calls where we have
working fallocate() alternatives.  (And thus replaces “block/file-posix:
Fix xfs_write_zeroes()”.)

Unfortunately, we also use xfsctl() to inquire the request alignment for
O_DIRECT, and this is the only way we currently have to obtain it
without trying.  Therefore, I didn’t quite like removing that call, too,
so this series doesn’t get rid of xfsctl() completely.

(If we did, we could delete 146 lines instead of these measly 76 here.)


Anyway, dropping xfs_write_zeroes() will also fix the guest corruptions
Lukáš has reported (for qcow2, but I think it should be possible to see
similar corruptions with raw, although I haven’t investigated that too
far).


Max Reitz (2):
  block/file-posix: Reduce xfsctl() use
  iotests: Test reverse sub-cluster qcow2 writes

 block/file-posix.c         | 77 +-------------------------------------
 tests/qemu-iotests/265     | 67 +++++++++++++++++++++++++++++++++
 tests/qemu-iotests/265.out |  6 +++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 75 insertions(+), 76 deletions(-)
 create mode 100755 tests/qemu-iotests/265
 create mode 100644 tests/qemu-iotests/265.out

-- 
2.21.0

[Qemu-devel] [PATCH 2/2] iotests: Test reverse sub-cluster qcow2 writes

[Max Reitz]

This exercises the regression introduced in commit
50ba5b2d994853b38fed10e0841b119da0f8b8e5.  On my machine, it has close
to a 50 % false-negative rate, but that should still be sufficient to
test the fix.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/265     | 67 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/265.out |  6 ++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 74 insertions(+)
 create mode 100755 tests/qemu-iotests/265
 create mode 100644 tests/qemu-iotests/265.out

diff --git a/tests/qemu-iotests/265 b/tests/qemu-iotests/265
new file mode 100755
index 0000000000..dce6f77be3
--- /dev/null
+++ b/tests/qemu-iotests/265
@@ -0,0 +1,67 @@
+#!/usr/bin/env bash
+#
+# Test reverse-ordered qcow2 writes on a sub-cluster level
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+seq=$(basename $0)
+echo "QA output created by $seq"
+
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# qcow2-specific test
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+echo '--- Writing to the image ---'
+
+# Reduce cluster size so we get more and quicker I/O
+IMGOPTS='cluster_size=4096' _make_test_img 1M
+(for ((kb = 1024 - 4; kb >= 0; kb -= 4)); do \
+     echo "aio_write -P 42 $((kb + 1))k 2k"; \
+ done) \
+ | $QEMU_IO "$TEST_IMG" > /dev/null
+
+echo '--- Verifying its content ---'
+
+(for ((kb = 0; kb < 1024; kb += 4)); do \
+    echo "read -P 0 ${kb}k 1k"; \
+    echo "read -P 42 $((kb + 1))k 2k"; \
+    echo "read -P 0 $((kb + 3))k 1k"; \
+ done) \
+ | $QEMU_IO "$TEST_IMG" | _filter_qemu_io | grep 'verification'
+
+# Status of qemu-io
+if [ ${PIPESTATUS[1]} = 0 ]; then
+    echo 'Content verified.'
+fi
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/265.out b/tests/qemu-iotests/265.out
new file mode 100644
index 0000000000..6eac620f25
--- /dev/null
+++ b/tests/qemu-iotests/265.out
@@ -0,0 +1,6 @@
+QA output created by 265
+--- Writing to the image ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+--- Verifying its content ---
+Content verified.
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index d95d556414..0c129c1644 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -274,3 +274,4 @@
 257 rw
 258 rw quick
 262 rw quick migration
+265 rw auto quick
-- 
2.21.0