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

[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 1/2] block/file-posix: Reduce xfsctl() use

[Max Reitz]

This patch removes xfs_write_zeroes() and xfs_discard().  Both functions
have been added just before the same feature was present through
fallocate():

- fallocate() has supported PUNCH_HOLE for XFS since Linux 2.6.38 (March
  2011); xfs_discard() was added in December 2010.

- fallocate() has supported ZERO_RANGE for XFS since Linux 3.15 (June
  2014); xfs_write_zeroes() was added in November 2013.

Nowadays, all systems that qemu runs on should support both fallocate()
features (RHEL 7's kernel does).

xfsctl() is still useful for getting the request alignment for O_DIRECT,
so this patch does not remove our dependency on it completely.

Note that xfs_write_zeroes() had a bug: It calls ftruncate() when the
file is shorter than the specified range (because ZERO_RANGE does not
increase the file length).  ftruncate() may yield and then discard data
that parallel write requests have written past the EOF in the meantime.
Dropping the function altogether fixes the bug.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Fixes: 50ba5b2d994853b38fed10e0841b119da0f8b8e5
Reported-by: Lukáš Doktor <ldoktor@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/file-posix.c | 77 +---------------------------------------------
 1 file changed, 1 insertion(+), 76 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index fbeb0068db..4519065573 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1459,59 +1459,6 @@ out:
     }
 }
 
-#ifdef CONFIG_XFS
-static int xfs_write_zeroes(BDRVRawState *s, int64_t offset, uint64_t bytes)
-{
-    int64_t len;
-    struct xfs_flock64 fl;
-    int err;
-
-    len = lseek(s->fd, 0, SEEK_END);
-    if (len < 0) {
-        return -errno;
-    }
-
-    if (offset + bytes > len) {
-        /* XFS_IOC_ZERO_RANGE does not increase the file length */
-        if (ftruncate(s->fd, offset + bytes) < 0) {
-            return -errno;
-        }
-    }
-
-    memset(&fl, 0, sizeof(fl));
-    fl.l_whence = SEEK_SET;
-    fl.l_start = offset;
-    fl.l_len = bytes;
-
-    if (xfsctl(NULL, s->fd, XFS_IOC_ZERO_RANGE, &fl) < 0) {
-        err = errno;
-        trace_file_xfs_write_zeroes(strerror(errno));
-        return -err;
-    }
-
-    return 0;
-}
-
-static int xfs_discard(BDRVRawState *s, int64_t offset, uint64_t bytes)
-{
-    struct xfs_flock64 fl;
-    int err;
-
-    memset(&fl, 0, sizeof(fl));
-    fl.l_whence = SEEK_SET;
-    fl.l_start = offset;
-    fl.l_len = bytes;
-
-    if (xfsctl(NULL, s->fd, XFS_IOC_UNRESVSP64, &fl) < 0) {
-        err = errno;
-        trace_file_xfs_discard(strerror(errno));
-        return -err;
-    }
-
-    return 0;
-}
-#endif
-
 static int translate_err(int err)
 {
     if (err == -ENODEV || err == -ENOSYS || err == -EOPNOTSUPP ||
@@ -1567,10 +1514,8 @@ static ssize_t handle_aiocb_write_zeroes_block(RawPosixAIOData *aiocb)
 static int handle_aiocb_write_zeroes(void *opaque)
 {
     RawPosixAIOData *aiocb = opaque;
-#if defined(CONFIG_FALLOCATE) || defined(CONFIG_XFS)
-    BDRVRawState *s = aiocb->bs->opaque;
-#endif
 #ifdef CONFIG_FALLOCATE
+    BDRVRawState *s = aiocb->bs->opaque;
     int64_t len;
 #endif
 
@@ -1578,12 +1523,6 @@ static int handle_aiocb_write_zeroes(void *opaque)
         return handle_aiocb_write_zeroes_block(aiocb);
     }
 
-#ifdef CONFIG_XFS
-    if (s->is_xfs) {
-        return xfs_write_zeroes(s, aiocb->aio_offset, aiocb->aio_nbytes);
-    }
-#endif
-
 #ifdef CONFIG_FALLOCATE_ZERO_RANGE
     if (s->has_write_zeroes) {
         int ret = do_fallocate(s->fd, FALLOC_FL_ZERO_RANGE,
@@ -1646,14 +1585,6 @@ static int handle_aiocb_write_zeroes_unmap(void *opaque)
     }
 #endif
 
-#ifdef CONFIG_XFS
-    if (s->is_xfs) {
-        /* xfs_discard() guarantees that the discarded area reads as all-zero
-         * afterwards, so we can use it here. */
-        return xfs_discard(s, aiocb->aio_offset, aiocb->aio_nbytes);
-    }
-#endif
-
     /* If we couldn't manage to unmap while guaranteed that the area reads as
      * all-zero afterwards, just write zeroes without unmapping */
     ret = handle_aiocb_write_zeroes(aiocb);
@@ -1730,12 +1661,6 @@ static int handle_aiocb_discard(void *opaque)
         ret = -errno;
 #endif
     } else {
-#ifdef CONFIG_XFS
-        if (s->is_xfs) {
-            return xfs_discard(s, aiocb->aio_offset, aiocb->aio_nbytes);
-        }
-#endif
-
 #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
         ret = do_fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
                            aiocb->aio_offset, aiocb->aio_nbytes);
-- 
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] [Qemu-block] [PATCH 0/2] block/file-posix: Reduce xfsctl() use

[Stefano Garzarella]

On Fri, Aug 23, 2019 at 03:03:39PM +0200, Max Reitz wrote:
> 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

The patch and the test LGTM.

I tried to run the 265 test without the
"block/file-posix: Reduce xfsctl() use" patch and the failure rate is ~30% on
my system.

With the patch applied the failure rate is 0% :-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Tested-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks,
Stefano

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

[John Snow]

On 8/23/19 9:03 AM, Max Reitz wrote:
> This patch removes xfs_write_zeroes() and xfs_discard().  Both functions
> have been added just before the same feature was present through
> fallocate():
> 
> - fallocate() has supported PUNCH_HOLE for XFS since Linux 2.6.38 (March
>   2011); xfs_discard() was added in December 2010.
> 
> - fallocate() has supported ZERO_RANGE for XFS since Linux 3.15 (June
>   2014); xfs_write_zeroes() was added in November 2013.
> 
> Nowadays, all systems that qemu runs on should support both fallocate()
> features (RHEL 7's kernel does).
> 
> xfsctl() is still useful for getting the request alignment for O_DIRECT,
> so this patch does not remove our dependency on it completely.
> 
> Note that xfs_write_zeroes() had a bug: It calls ftruncate() when the
> file is shorter than the specified range (because ZERO_RANGE does not
> increase the file length).  ftruncate() may yield and then discard data
> that parallel write requests have written past the EOF in the meantime.
> Dropping the function altogether fixes the bug.
> 

And I assume getting rid of discard is just then simply convenient, so
why not.

> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Fixes: 50ba5b2d994853b38fed10e0841b119da0f8b8e5
> Reported-by: Lukáš Doktor <ldoktor@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Tested-by: John Snow <jsnow@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>

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

[John Snow]

On 8/23/19 9:03 AM, Max Reitz wrote:
> 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
> 

Tested-by: John Snow <jsnow@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>

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

[Kevin Wolf]

Am 23.08.2019 um 15:03 hat Max Reitz geschrieben:
> 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).

Thanks, applied to the block branch.

Kevin