[Qemu-devel] [PULL 0/7] Block layer fixes for 3.0-rc1

[Qemu-devel] [PULL 0/7] Block layer fixes for 3.0-rc1

[Kevin Wolf]

The following changes since commit dc3c89d612252fc461a65f54885a1fe108e9ec05:

  ui/cocoa.m: replace scrollingDeltaY with deltaY (2018-07-12 16:03:50 +0100)

are available in the git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 8dcd3c9b91a300c86e315d7e5427dce1383f7387:

  qemu-img: align result of is_allocated_sectors (2018-07-12 18:24:08 +0200)

----------------------------------------------------------------
Block layer patches:

- file-posix: Check correct file type (regular file for 'file',
  character or block device for 'host_device'/'host_cdrom')
- scsi-disk: Block Device Characteristics emulation fix
- qemu-img: Consider required alignment for sparse area detection
- Documentation and test improvements

----------------------------------------------------------------
Daniel Henrique Barboza (1):
      scsi-disk: Block Device Characteristics emulation fix

Fam Zheng (3):
      iotests: 153: Fix dead code
      iotests: nbd: Stop qemu-nbd before remaking image
      qemu-img: Document copy offloading implications with -S and -c

John Snow (2):
      file-posix: specify expected filetypes
      iotests: add test 226 for file driver types

Peter Lieven (1):
      qemu-img: align result of is_allocated_sectors

 block/file-posix.c           | 39 ++++++++++++++++++++------
 hw/scsi/scsi-disk.c          |  7 +++--
 qemu-img.c                   | 44 +++++++++++++++++++++++++----
 qemu-doc.texi                |  6 ++++
 qemu-img.texi                |  6 ++--
 tests/qemu-iotests/122.out   | 18 ++++++------
 tests/qemu-iotests/153       |  2 ++
 tests/qemu-iotests/153.out   | 25 +++++++++++++++++
 tests/qemu-iotests/226       | 66 ++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/226.out   | 26 +++++++++++++++++
 tests/qemu-iotests/common.rc | 21 ++++++++++----
 tests/qemu-iotests/group     |  1 +
 12 files changed, 226 insertions(+), 35 deletions(-)
 create mode 100755 tests/qemu-iotests/226
 create mode 100644 tests/qemu-iotests/226.out

[Qemu-devel] [PULL 1/7] iotests: 153: Fix dead code

[Kevin Wolf]

From: Fam Zheng <famz@redhat.com>

This step was left behind my mistake. As suggested by the echoed text,
the intention was to test two devices with the same image, with
different options. The behavior should be the same as two QEMU
processes. Complete it.

Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/153     |  2 ++
 tests/qemu-iotests/153.out | 25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153
index 673813cd39..0daeb1b005 100755
--- a/tests/qemu-iotests/153
+++ b/tests/qemu-iotests/153
@@ -162,6 +162,7 @@ for opts1 in "" "read-only=on" "read-only=on,force-share=on"; do
     _cleanup_qemu
 done
 
+test_opts="read-only=off read-only=on read-only=on,force-share=on"
 for opt1 in $test_opts; do
     for opt2 in $test_opts; do
         echo
@@ -170,6 +171,7 @@ for opt1 in $test_opts; do
     done
 done
 
+echo
 echo "== Creating ${TEST_IMG}.[abc] ==" | _filter_testdir
 (
     $QEMU_IMG create -f qcow2 "${TEST_IMG}.a" -b "${TEST_IMG}"
diff --git a/tests/qemu-iotests/153.out b/tests/qemu-iotests/153.out
index 3492ba7a29..93eaf10486 100644
--- a/tests/qemu-iotests/153.out
+++ b/tests/qemu-iotests/153.out
@@ -369,6 +369,31 @@ _qemu_img_wrapper bench -U -w -c 1 TEST_DIR/t.qcow2
 qemu-img: Could not open 'TEST_DIR/t.qcow2': force-share=on can only be used with read-only images
 
 Round done
+
+== Two devices with the same image (read-only=off - read-only=off) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,read-only=off: Failed to get "write" lock
+Is another process using the image?
+
+== Two devices with the same image (read-only=off - read-only=on) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,read-only=on: Failed to get shared "write" lock
+Is another process using the image?
+
+== Two devices with the same image (read-only=off - read-only=on,force-share=on) ==
+
+== Two devices with the same image (read-only=on - read-only=off) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,read-only=off: Failed to get "write" lock
+Is another process using the image?
+
+== Two devices with the same image (read-only=on - read-only=on) ==
+
+== Two devices with the same image (read-only=on - read-only=on,force-share=on) ==
+
+== Two devices with the same image (read-only=on,force-share=on - read-only=off) ==
+
+== Two devices with the same image (read-only=on,force-share=on - read-only=on) ==
+
+== Two devices with the same image (read-only=on,force-share=on - read-only=on,force-share=on) ==
+
 == Creating TEST_DIR/t.qcow2.[abc] ==
 Formatting 'TEST_DIR/t.IMGFMT.a', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/t.IMGFMT
 Formatting 'TEST_DIR/t.IMGFMT.b', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/t.IMGFMT
-- 
2.13.6

[Qemu-devel] [PULL 2/7] iotests: nbd: Stop qemu-nbd before remaking image

[Kevin Wolf]

From: Fam Zheng <famz@redhat.com>

197 is one example where _make_test_img is used twice without stopping
the NBD server in between. An error will occur like this:

    @@ -26,9 +26,13 @@

     === Partial final cluster ===

    +qemu-img: TEST_DIR/t.IMGFMT: Failed to get "resize" lock
    +Is another process using the image?
     Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1024
    +Failed to find an available port: Address already in use
     read 1024/1024 bytes at offset 0

Patch _make_test_img to stop the old qemu-nbd before starting a new one,
which fixes this problem, and similarly 215.

Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/common.rc | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index d054cb97fc..44bee16a5e 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -195,6 +195,16 @@ _use_sample_img()
     fi
 }
 
+_stop_nbd_server()
+{
+    if [ -f "${QEMU_TEST_DIR}/qemu-nbd.pid" ]; then
+        local QEMU_NBD_PID
+        read QEMU_NBD_PID < "${QEMU_TEST_DIR}/qemu-nbd.pid"
+        kill ${QEMU_NBD_PID}
+        rm -f "${QEMU_TEST_DIR}/qemu-nbd.pid"
+    fi
+}
+
 _make_test_img()
 {
     # extra qemu-img options can be added by tests
@@ -234,6 +244,10 @@ _make_test_img()
         extra_img_options="-o $optstr $extra_img_options"
     fi
 
+    if [ $IMGPROTO = "nbd" ]; then
+        _stop_nbd_server
+    fi
+
     # XXX(hch): have global image options?
     (
      if [ $use_backing = 1 ]; then
@@ -274,12 +288,7 @@ _cleanup_test_img()
     case "$IMGPROTO" in
 
         nbd)
-            if [ -f "${QEMU_TEST_DIR}/qemu-nbd.pid" ]; then
-                local QEMU_NBD_PID
-                read QEMU_NBD_PID < "${QEMU_TEST_DIR}/qemu-nbd.pid"
-                kill ${QEMU_NBD_PID}
-                rm -f "${QEMU_TEST_DIR}/qemu-nbd.pid"
-            fi
+            _stop_nbd_server
             rm -f "$TEST_IMG_FILE"
             ;;
         vxhs)
-- 
2.13.6

[Qemu-devel] [PULL 3/7] qemu-img: Document copy offloading implications with -S and -c

[Kevin Wolf]

From: Fam Zheng <famz@redhat.com>

Explicitly enabling zero detection or compression suppresses copy
offloading during convert. Document it.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.texi | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/qemu-img.texi b/qemu-img.texi
index aeb1b9e66c..5853cd18d1 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -96,7 +96,8 @@ will enumerate information about backing files in a disk image chain. Refer
 below for further description.
 
 @item -c
-indicates that target image must be compressed (qcow format only)
+indicates that target image must be compressed (qcow format only). If this
+option is used, copy offloading will not be attempted.
 
 @item -h
 with or without a command shows help and lists the supported formats
@@ -115,7 +116,8 @@ in case both @var{-q} and @var{-p} options are used.
 indicates the consecutive number of bytes that must contain only zeros
 for qemu-img to create a sparse image during conversion. This value is rounded
 down to the nearest 512 bytes. You may use the common size suffixes like
-@code{k} for kilobytes.
+@code{k} for kilobytes. If this option is used, copy offloading will not be
+attempted.
 
 @item -t @var{cache}
 specifies the cache mode that should be used with the (destination) file. See
-- 
2.13.6

[Qemu-devel] [PULL 4/7] file-posix: specify expected filetypes

[Kevin Wolf]

From: John Snow <jsnow@redhat.com>

Adjust each caller of raw_open_common to specify if they are expecting
host and character devices or not. Tighten expectations of file types upon
open in the common code and refuse types that are not expected.

This has two effects:

(1) Character and block devices are now considered deprecated for the
    'file' driver, which expects only S_IFREG, and
(2) no file-posix driver (file, host_cdrom, or host_device) can open
    directories now.

I don't think there's a legitimate reason to open directories as if
they were files. This prevents QEMU from opening and attempting to probe
a directory inode, which can break in exciting ways. One of those ways
is lseek on ext4/xfs, which will return 0x7fffffffffffffff as the file
size instead of EISDIR. This can coax QEMU into responding with a
confusing "file too big" instead of "Hey, that's not a file".

See: https://bugs.launchpad.net/qemu/+bug/1739304/
Signed-off-by: John Snow <jsnow@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 39 +++++++++++++++++++++++++++++++--------
 qemu-doc.texi      |  6 ++++++
 2 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 28824aae65..60af4b3d51 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -438,7 +438,8 @@ static QemuOptsList raw_runtime_opts = {
 };
 
 static int raw_open_common(BlockDriverState *bs, QDict *options,
-                           int bdrv_flags, int open_flags, Error **errp)
+                           int bdrv_flags, int open_flags,
+                           bool device, Error **errp)
 {
     BDRVRawState *s = bs->opaque;
     QemuOpts *opts;
@@ -585,10 +586,32 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
         error_setg_errno(errp, errno, "Could not stat file");
         goto fail;
     }
-    if (S_ISREG(st.st_mode)) {
-        s->discard_zeroes = true;
-        s->has_fallocate = true;
+
+    if (!device) {
+        if (S_ISBLK(st.st_mode)) {
+            warn_report("Opening a block device as a file using the '%s' "
+                        "driver is deprecated", bs->drv->format_name);
+        } else if (S_ISCHR(st.st_mode)) {
+            warn_report("Opening a character device as a file using the '%s' "
+                        "driver is deprecated", bs->drv->format_name);
+        } else if (!S_ISREG(st.st_mode)) {
+            error_setg(errp, "A regular file was expected by the '%s' driver, "
+                       "but something else was given", bs->drv->format_name);
+            ret = -EINVAL;
+            goto fail;
+        } else {
+            s->discard_zeroes = true;
+            s->has_fallocate = true;
+        }
+    } else {
+        if (!(S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode))) {
+            error_setg(errp, "'%s' driver expects either "
+                       "a character or block device", bs->drv->format_name);
+            ret = -EINVAL;
+            goto fail;
+        }
     }
+
     if (S_ISBLK(st.st_mode)) {
 #ifdef BLKDISCARDZEROES
         unsigned int arg;
@@ -641,7 +664,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVRawState *s = bs->opaque;
 
     s->type = FTYPE_FILE;
-    return raw_open_common(bs, options, flags, 0, errp);
+    return raw_open_common(bs, options, flags, 0, false, errp);
 }
 
 typedef enum {
@@ -2939,7 +2962,7 @@ hdev_open_Mac_error:
 
     s->type = FTYPE_FILE;
 
-    ret = raw_open_common(bs, options, flags, 0, &local_err);
+    ret = raw_open_common(bs, options, flags, 0, true, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
 #if defined(__APPLE__) && defined(__MACH__)
@@ -3170,7 +3193,7 @@ static int cdrom_open(BlockDriverState *bs, QDict *options, int flags,
     s->type = FTYPE_CD;
 
     /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
-    return raw_open_common(bs, options, flags, O_NONBLOCK, errp);
+    return raw_open_common(bs, options, flags, O_NONBLOCK, true, errp);
 }
 
 static int cdrom_probe_device(const char *filename)
@@ -3284,7 +3307,7 @@ static int cdrom_open(BlockDriverState *bs, QDict *options, int flags,
 
     s->type = FTYPE_CD;
 
-    ret = raw_open_common(bs, options, flags, 0, &local_err);
+    ret = raw_open_common(bs, options, flags, 0, true, &local_err);
     if (ret) {
         error_propagate(errp, local_err);
         return ret;
diff --git a/qemu-doc.texi b/qemu-doc.texi
index 0fbf8b108b..1047c407e7 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2969,6 +2969,12 @@ replacement since it is not needed anymore.
 The @option{-enable-hax} option has been replaced by @option{-accel hax}.
 Both options have been introduced in QEMU version 2.9.0.
 
+@subsection -drive file=json:@{...@{'driver':'file'@}@} (since 3.0)
+
+The 'file' driver for drives is no longer appropriate for character or host
+devices and will only accept regular files (S_IFREG). The correct driver
+for these file types is 'host_cdrom' or 'host_device' as appropriate.
+
 @section QEMU Machine Protocol (QMP) commands
 
 @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
-- 
2.13.6

[Qemu-devel] [PULL 5/7] iotests: add test 226 for file driver types

[Kevin Wolf]

From: John Snow <jsnow@redhat.com>

Test that we're rejecting what we ought to for file,
host_driver and host_cdrom drivers. Test that we're
seeing the deprecated message for block and chardevs
on the file driver.

Signed-off-by: John Snow <jsnow@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/226     | 66 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/226.out | 26 ++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 93 insertions(+)
 create mode 100755 tests/qemu-iotests/226
 create mode 100644 tests/qemu-iotests/226.out

diff --git a/tests/qemu-iotests/226 b/tests/qemu-iotests/226
new file mode 100755
index 0000000000..460aea2fc9
--- /dev/null
+++ b/tests/qemu-iotests/226
@@ -0,0 +1,66 @@
+#!/bin/bash
+#
+# This test covers expected filetypes for the file, host_cdrom and
+# host_device drivers.
+#
+# Copyright (C) 2018 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/>.
+#
+
+# creator
+owner=jsnow@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1    # failure is the default!
+
+_cleanup()
+{
+    rmdir "$TEST_IMG"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.pattern
+
+# Generic format, but tests file-protocol specific error handling
+_supported_fmt generic
+_supported_proto file
+_supported_os Linux
+
+# Create something decidedly not a file, blockdev or chardev...
+mkdir "$TEST_IMG"
+
+for PROTO in "file" "host_device" "host_cdrom"; do
+    echo
+    echo "=== Testing with driver:$PROTO ==="
+    echo
+    echo "== Testing RO =="
+    $QEMU_IO -c "open -r -o driver=$PROTO,filename=$TEST_IMG" 2>&1 | _filter_imgfmt | _filter_testdir
+    $QEMU_IO -c "open -r -o driver=$PROTO,filename=/dev/null" 2>&1 | _filter_imgfmt
+    echo "== Testing RW =="
+    $QEMU_IO -c "open -o driver=$PROTO,filename=$TEST_IMG" 2>&1 | _filter_imgfmt | _filter_testdir
+    $QEMU_IO -c "open -o driver=$PROTO,filename=/dev/null" 2>&1 | _filter_imgfmt
+done
+
+# success, all done
+echo
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/226.out b/tests/qemu-iotests/226.out
new file mode 100644
index 0000000000..8c0d060ffc
--- /dev/null
+++ b/tests/qemu-iotests/226.out
@@ -0,0 +1,26 @@
+QA output created by 226
+
+=== Testing with driver:file ===
+
+== Testing RO ==
+can't open: A regular file was expected by the 'file' driver, but something else was given
+warning: Opening a character device as a file using the 'file' driver is deprecated
+== Testing RW ==
+can't open: Could not open 'TEST_DIR/t.IMGFMT': Is a directory
+warning: Opening a character device as a file using the 'file' driver is deprecated
+
+=== Testing with driver:host_device ===
+
+== Testing RO ==
+can't open: 'host_device' driver expects either a character or block device
+== Testing RW ==
+can't open: Could not open 'TEST_DIR/t.IMGFMT': Is a directory
+
+=== Testing with driver:host_cdrom ===
+
+== Testing RO ==
+can't open: 'host_cdrom' driver expects either a character or block device
+== Testing RW ==
+can't open: Could not open 'TEST_DIR/t.IMGFMT': Is a directory
+
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 1c9f679821..68f66259b2 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -223,3 +223,4 @@
 222 rw auto quick
 223 rw auto quick
 225 rw auto quick
+226 auto quick
-- 
2.13.6

[Qemu-devel] [PULL 6/7] scsi-disk: Block Device Characteristics emulation fix

[Kevin Wolf]

From: Daniel Henrique Barboza <danielhb413@gmail.com>

The current BDC VPD page (page 0xb1) is too short. This can be
seen running sg_utils:

$ sg_vpd --page=bdc /dev/sda
Block device characteristics VPD page (SBC):
Block device characteristics VPD page length too short=8

By the SCSI spec, the expected size of the SBC page is 0x40.
There is no telling how the guest will behave with a shorter
message - it can ignore it, or worse, make (wrong)
assumptions.

This patch fixes the emulation by setting the size to 0x40.
This is the output of the previous sg_vpd command after
applying it:

$ sg_vpd --page=bdc /dev/sda -v
    inquiry cdb: 12 01 b1 00 fc 00
Block device characteristics VPD page (SBC):
   [PQual=0  Peripheral device type: disk]
  Medium rotation rate is not reported
  Product type: Not specified
  WABEREQ=0
  WACEREQ=0
  Nominal form factor not reported
  FUAB=0
  VBULS=0

To improve readability, this patch also adds the VBULS value
explictly and add comments on the existing fields we're
setting.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/scsi/scsi-disk.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index d7df357029..5bb390773b 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -775,11 +775,12 @@ int scsi_disk_emulate_vpd_page(SCSIRequest *req, uint8_t *outbuf)
     }
     case 0xb1: /* block device characteristics */
     {
-        buflen = 8;
+        buflen = 0x40;
         outbuf[4] = (s->rotation_rate >> 8) & 0xff;
         outbuf[5] = s->rotation_rate & 0xff;
-        outbuf[6] = 0;
-        outbuf[7] = 0;
+        outbuf[6] = 0; /* PRODUCT TYPE */
+        outbuf[7] = 0; /* WABEREQ | WACEREQ | NOMINAL FORM FACTOR */
+        outbuf[8] = 0; /* VBULS */
         break;
     }
     case 0xb2: /* thin provisioning */
-- 
2.13.6

[Qemu-devel] [PULL 7/7] qemu-img: align result of is_allocated_sectors

[Kevin Wolf]

From: Peter Lieven <pl@kamp.de>

We currently don't enforce that the sparse segments we detect during convert are
aligned. This leads to unnecessary and costly read-modify-write cycles either
internally in Qemu or in the background on the storage device as nearly all
modern filesystems or hardware have a 4k alignment internally.

This patch modifies is_allocated_sectors so that its *pnum result will always
end at an alignment boundary. This way all requests will end at an alignment
boundary. The start of all requests will also be aligned as long as the results
of get_block_status do not lead to an unaligned offset.

The number of RMW cycles when converting an example image [1] to a raw device that
has 4k sector size is about 4600 4k read requests to perform a total of about 15000
write requests. With this path the additional 4600 read requests are eliminated while
the number of total write requests stays constant.

[1] https://cloud-images.ubuntu.com/releases/16.04/release/ubuntu-16.04-server-cloudimg-amd64-disk1.vmdk

Signed-off-by: Peter Lieven <pl@kamp.de>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c                 | 44 ++++++++++++++++++++++++++++++++++++++------
 tests/qemu-iotests/122.out | 18 ++++++++----------
 2 files changed, 46 insertions(+), 16 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index f4074ebf75..4a7ce43dc9 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1105,11 +1105,15 @@ static int64_t find_nonzero(const uint8_t *buf, int64_t n)
  *
  * 'pnum' is set to the number of sectors (including and immediately following
  * the first one) that are known to be in the same allocated/unallocated state.
+ * The function will try to align the end offset to alignment boundaries so
+ * that the request will at least end aligned and consequtive requests will
+ * also start at an aligned offset.
  */
-static int is_allocated_sectors(const uint8_t *buf, int n, int *pnum)
+static int is_allocated_sectors(const uint8_t *buf, int n, int *pnum,
+                                int64_t sector_num, int alignment)
 {
     bool is_zero;
-    int i;
+    int i, tail;
 
     if (n <= 0) {
         *pnum = 0;
@@ -1122,6 +1126,23 @@ static int is_allocated_sectors(const uint8_t *buf, int n, int *pnum)
             break;
         }
     }
+
+    tail = (sector_num + i) & (alignment - 1);
+    if (tail) {
+        if (is_zero && i <= tail) {
+            /* treat unallocated areas which only consist
+             * of a small tail as allocated. */
+            is_zero = false;
+        }
+        if (!is_zero) {
+            /* align up end offset of allocated areas. */
+            i += alignment - tail;
+            i = MIN(i, n);
+        } else {
+            /* align down end offset of zero areas. */
+            i -= tail;
+        }
+    }
     *pnum = i;
     return !is_zero;
 }
@@ -1132,7 +1153,7 @@ static int is_allocated_sectors(const uint8_t *buf, int n, int *pnum)
  * breaking up write requests for only small sparse areas.
  */
 static int is_allocated_sectors_min(const uint8_t *buf, int n, int *pnum,
-    int min)
+    int min, int64_t sector_num, int alignment)
 {
     int ret;
     int num_checked, num_used;
@@ -1141,7 +1162,7 @@ static int is_allocated_sectors_min(const uint8_t *buf, int n, int *pnum,
         min = n;
     }
 
-    ret = is_allocated_sectors(buf, n, pnum);
+    ret = is_allocated_sectors(buf, n, pnum, sector_num, alignment);
     if (!ret) {
         return ret;
     }
@@ -1149,13 +1170,15 @@ static int is_allocated_sectors_min(const uint8_t *buf, int n, int *pnum,
     num_used = *pnum;
     buf += BDRV_SECTOR_SIZE * *pnum;
     n -= *pnum;
+    sector_num += *pnum;
     num_checked = num_used;
 
     while (n > 0) {
-        ret = is_allocated_sectors(buf, n, pnum);
+        ret = is_allocated_sectors(buf, n, pnum, sector_num, alignment);
 
         buf += BDRV_SECTOR_SIZE * *pnum;
         n -= *pnum;
+        sector_num += *pnum;
         num_checked += *pnum;
         if (ret) {
             num_used = num_checked;
@@ -1560,6 +1583,7 @@ typedef struct ImgConvertState {
     bool wr_in_order;
     bool copy_range;
     int min_sparse;
+    int alignment;
     size_t cluster_sectors;
     size_t buf_sectors;
     long num_coroutines;
@@ -1724,7 +1748,8 @@ static int coroutine_fn convert_co_write(ImgConvertState *s, int64_t sector_num,
              * zeroed. */
             if (!s->min_sparse ||
                 (!s->compressed &&
-                 is_allocated_sectors_min(buf, n, &n, s->min_sparse)) ||
+                 is_allocated_sectors_min(buf, n, &n, s->min_sparse,
+                                          sector_num, s->alignment)) ||
                 (s->compressed &&
                  !buffer_is_zero(buf, n * BDRV_SECTOR_SIZE)))
             {
@@ -2368,6 +2393,13 @@ static int img_convert(int argc, char **argv)
                                 out_bs->bl.pdiscard_alignment >>
                                 BDRV_SECTOR_BITS)));
 
+    /* try to align the write requests to the destination to avoid unnecessary
+     * RMW cycles. */
+    s.alignment = MAX(pow2floor(s.min_sparse),
+                      DIV_ROUND_UP(out_bs->bl.request_alignment,
+                                   BDRV_SECTOR_SIZE));
+    assert(is_power_of_2(s.alignment));
+
     if (skip_create) {
         int64_t output_sectors = blk_nb_sectors(s.target);
         if (output_sectors < 0) {
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index 6c7ee1da6c..c576705284 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -194,12 +194,12 @@ wrote 1024/1024 bytes at offset 17408
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 convert -S 4k
-[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
-{ "start": 1024, "length": 7168, "depth": 0, "zero": true, "data": false},
-{ "start": 8192, "length": 1024, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
-{ "start": 9216, "length": 8192, "depth": 0, "zero": true, "data": false},
-{ "start": 17408, "length": 1024, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
-{ "start": 18432, "length": 67090432, "depth": 0, "zero": true, "data": false}]
+[{ "start": 0, "length": 4096, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 4096, "length": 4096, "depth": 0, "zero": true, "data": false},
+{ "start": 8192, "length": 4096, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 12288, "length": 4096, "depth": 0, "zero": true, "data": false},
+{ "start": 16384, "length": 4096, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 20480, "length": 67088384, "depth": 0, "zero": true, "data": false}]
 
 convert -c -S 4k
 [{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true},
@@ -210,10 +210,8 @@ convert -c -S 4k
 { "start": 18432, "length": 67090432, "depth": 0, "zero": true, "data": false}]
 
 convert -S 8k
-[{ "start": 0, "length": 9216, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
-{ "start": 9216, "length": 8192, "depth": 0, "zero": true, "data": false},
-{ "start": 17408, "length": 1024, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
-{ "start": 18432, "length": 67090432, "depth": 0, "zero": true, "data": false}]
+[{ "start": 0, "length": 24576, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 24576, "length": 67084288, "depth": 0, "zero": true, "data": false}]
 
 convert -c -S 8k
 [{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true},
-- 
2.13.6

Re: [Qemu-devel] [PULL 0/7] Block layer fixes for 3.0-rc1

[Peter Maydell]

On 12 July 2018 at 17:31, Kevin Wolf <kwolf@redhat.com> wrote:
> The following changes since commit dc3c89d612252fc461a65f54885a1fe108e9ec05:
>
>   ui/cocoa.m: replace scrollingDeltaY with deltaY (2018-07-12 16:03:50 +0100)
>
> are available in the git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 8dcd3c9b91a300c86e315d7e5427dce1383f7387:
>
>   qemu-img: align result of is_allocated_sectors (2018-07-12 18:24:08 +0200)
>
> ----------------------------------------------------------------
> Block layer patches:
>
> - file-posix: Check correct file type (regular file for 'file',
>   character or block device for 'host_device'/'host_cdrom')
> - scsi-disk: Block Device Characteristics emulation fix
> - qemu-img: Consider required alignment for sparse area detection
> - Documentation and test improvements
>
> ----------------------------------------------------------------


Applied, thanks.

-- PMM