Re: [Qemu-block] [Qemu-devel] [PATCH] sheepdog: Set error when connection fails

[Markus Armbruster]

Kevin Wolf  writes:

> Am 20.04.2017 um 17:30 hat Daniel P. Berrange geschrieben:
>> On Thu, Apr 20, 2017 at 12:00:03PM +0800, Fam Zheng wrote:
>> > Signed-off-by: Fam Zheng 
>> > ---
>> >  block/sheepdog.c | 1 +
>> >  1 file changed, 1 insertion(+)
>> > 
>> > diff --git a/block/sheepdog.c b/block/sheepdog.c
>> > index fb9203e..7e889ee 100644
>> > --- a/block/sheepdog.c
>> > +++ b/block/sheepdog.c
>> > @@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error 
>> > **errp)
>> >  qemu_set_nonblock(fd);
>> >  } else {
>> >  fd = -EIO;
>> > +error_setg(errp, "Failed to connect to sheepdog server");
>> >  }
>> 
>> This doesn't make much sense to me. The lines just above the
>> diff context have this:
>> 
>> fd = socket_connect(s->addr, errp, NULL, NULL);
>> 
>> socket_connect should have already reported an error on "errp"
>> in the scenario that 'fd == -1'.
>
> By the way, am I the only one who thinks that having errp anywhere else
> than as the last argument is bad style? I can easily see myself missing
> that this functions sets it because the last argument is NULL.

Yes, it's bad style because it's suprising.  Worth fixing.

Re: [Qemu-block] [PATCH] qemu-img: use blk_co_pwrite_zeroes for zero sectors when compressed

[858585 jemmy]

On Fri, Apr 21, 2017 at 10:58 AM, 858585 jemmy  wrote:
> On Thu, Apr 20, 2017 at 6:00 PM, Kevin Wolf  wrote:
>> Am 20.04.2017 um 10:38 hat jemmy858...@gmail.com geschrieben:
>>> From: Lidong Chen 
>>>
>>> when the buffer is zero, blk_co_pwrite_zeroes is more effectively than
>>> blk_co_pwritev with BDRV_REQ_WRITE_COMPRESSED. this patch can reduces
>>> the time when converts the qcow2 image with lots of zero.
>>>
>>> Signed-off-by: Lidong Chen 
>>
>> Good catch, using blk_co_pwrite_zeroes() makes sense even for compressed
>> images.
>>
>>> diff --git a/qemu-img.c b/qemu-img.c
>>> index b220cf7..0256539 100644
>>> --- a/qemu-img.c
>>> +++ b/qemu-img.c
>>> @@ -1675,13 +1675,20 @@ static int coroutine_fn 
>>> convert_co_write(ImgConvertState *s, int64_t sector_num,
>>>   * write if the buffer is completely zeroed and we're allowed 
>>> to
>>>   * keep the target sparse. */
>>>  if (s->compressed) {
>>> -if (s->has_zero_init && s->min_sparse &&
>>> -buffer_is_zero(buf, n * BDRV_SECTOR_SIZE))
>>> -{
>>> -assert(!s->target_has_backing);
>>> -break;
>>> +if (buffer_is_zero(buf, n * BDRV_SECTOR_SIZE)) {
>>> +if (s->has_zero_init && s->min_sparse) {
>>> +assert(!s->target_has_backing);
>>> +break;
>>> +} else {
>>> +ret = blk_co_pwrite_zeroes(s->target,
>>> +   sector_num << BDRV_SECTOR_BITS,
>>> +   n << BDRV_SECTOR_BITS, 0);
>>> +if (ret < 0) {
>>> +return ret;
>>> +}
>>> +break;
>>> +}
>>>  }
>>
>> If s->min_sparse == 0, we may neither skip the write not use
>> blk_co_pwrite_zeroes(), because this requires actual full allocation
>> with explicit zero sectors.
>>
>> Of course, if you fix this, what you end up with here is a duplicate of
>> the code path for non-compressed images. The remaining difference seems
>> to be the BDRV_REQ_WRITE_COMPRESSED flag and buffer_is_zero() vs.
>> is_allocated_sectors_min() (because uncompressed clusters can be written
>> partially, but compressed clusters can't).
>
> I have a try to unify the code.
>
> I don't understand why use  is_allocated_sectors_min when don't compressed.
> the s->min_sparse is 8 default, which is smaller than cluster_sectors.
>
> if a cluster which data is  8 sector zero and 8 sector non-zero
> repeated, it will call
> blk_co_pwritev and blk_co_pwrite_zeroes many times for a cluster.
>
> why not compare the zero by cluster_sectors size?

I write this code, run in guest os.

#include 
#include 
#include 

int main()
{
char *zero;
char *nonzero;
FILE* fp = fopen("./test.dat", "ab");

zero = malloc(sizeof(char)*512*8);
nonzero = malloc(sizeof(char)*512*8);

memset(zero, 0, sizeof(char)*512*8);
memset(nonzero, 1, sizeof(char)*512*8);

while (1) {
fwrite(zero, sizeof(char)*512*8, 1, fp);
fwrite(nonzero, sizeof(char)*512*8, 1, fp);
}
fclose(fp);
}

qemu-img info /mnt/img2016111016860868.qcow2
image: /mnt/img2016111016860868.qcow2
file format: qcow2
virtual size: 20G (21474836480 bytes)
disk size: 19G (20061552640 bytes)
cluster_size: 65536
backing file: /baseimage/img2016042213665396/img2016042213665396.qcow2

use -S 65536 option.

time /root/kvm/bin/qemu-img convert -p -B
/baseimage/img2016042213665396/img2016042213665396.qcow2 -O qcow2
/mnt/img2016111016860868.qcow2 /mnt/img2017041611162809_zip_new.qcow2
-S 65536
(100.00/100%)

real0m32.203s
user0m5.165s
sys 0m27.887s

time /root/kvm/bin/qemu-img convert -p -B
/baseimage/img2016042213665396/img2016042213665396.qcow2 -O qcow2
/mnt/img2016111016860868.qcow2 /mnt/img2017041611162809_zip_new.qcow2
(100.00/100%)

real1m38.665s
user0m45.418s
sys 1m7.518s

should we set cluster_sectors as the default value of s->min_sparse?

>
>>
>> So I suppose that instead of just fixing the above bug, we could actually
>> mostly unify the two code paths, if you want to have a try at it.
>>
>> Kevin

[Qemu-block] [PATCH v14 20/20] qemu-iotests: Add test case 153 for image locking

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/153 | 219 
 tests/qemu-iotests/153.out | 627 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 847 insertions(+)
 create mode 100755 tests/qemu-iotests/153
 create mode 100644 tests/qemu-iotests/153.out

diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153
new file mode 100755
index 000..ff22615
--- /dev/null
+++ b/tests/qemu-iotests/153
@@ -0,0 +1,219 @@
+#!/bin/bash
+#
+# Test image locking
+#
+# Copyright 2016, 2017 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 .
+#
+
+# creator
+owner=f...@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1   # failure is the default!
+
+_cleanup()
+{
+_cleanup_test_img
+rm -f "${TEST_IMG}.base"
+rm -f "${TEST_IMG}.convert"
+rm -f "${TEST_IMG}.a"
+rm -f "${TEST_IMG}.b"
+rm -f "${TEST_IMG}.lnk"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+size=32M
+
+_run_cmd()
+{
+echo
+(echo "$@"; "$@" 2>&1 1>/dev/null) | _filter_testdir
+}
+
+function _do_run_qemu()
+{
+(
+if ! test -t 0; then
+while read cmd; do
+echo $cmd
+done
+fi
+echo quit
+) | $QEMU -nographic -monitor stdio -serial none "$@" 1>/dev/null
+}
+
+function _run_qemu_with_images()
+{
+_do_run_qemu \
+$(for i in $@; do echo "-drive if=none,file=$i"; done) 2>&1 \
+| _filter_testdir | _filter_qemu
+}
+
+test_opts="$(echo readonly={on,off},force-shared-write={on,off})"
+
+for opts1 in "" $test_opts; do
+echo
+echo "== Creating base image =="
+TEST_IMG="${TEST_IMG}.base" _make_test_img $size
+
+echo
+echo "== Creating test image =="
+$QEMU_IMG create -f $IMGFMT "${TEST_IMG}" -b ${TEST_IMG}.base | 
_filter_img_create
+
+echo
+echo "== Launching QEMU, opts: '$opts1' =="
+_launch_qemu -drive file="${TEST_IMG}",if=none,$opts1
+h=$QEMU_HANDLE
+
+for opts2 in "" $test_opts; do
+echo
+echo "== Launching another QEMU, opts: '$opts2' =="
+echo "quit" | \
+$QEMU -nographic -monitor stdio \
+-drive file="${TEST_IMG}",if=none,$opts2 2>&1 1>/dev/null | \
+_filter_testdir | _filter_qemu
+done
+
+for L in "" "-U"; do
+
+echo
+echo "== Running utility commands $L =="
+_run_cmd $QEMU_IO $L -c "read 0 512" "${TEST_IMG}"
+_run_cmd $QEMU_IO $L -r -c "read 0 512" "${TEST_IMG}"
+_run_cmd $QEMU_IO -c "open $L ${TEST_IMG}" -c "read 0 512"
+_run_cmd $QEMU_IO -c "open -r $L ${TEST_IMG}" -c "read 0 512"
+_run_cmd $QEMU_IMG info$L "${TEST_IMG}"
+_run_cmd $QEMU_IMG check   $L "${TEST_IMG}"
+_run_cmd $QEMU_IMG compare $L "${TEST_IMG}" "${TEST_IMG}"
+_run_cmd $QEMU_IMG map $L "${TEST_IMG}"
+_run_cmd $QEMU_IMG amend -o "" $L "${TEST_IMG}"
+_run_cmd $QEMU_IMG commit  $L "${TEST_IMG}"
+_run_cmd $QEMU_IMG resize  $L "${TEST_IMG}" $size
+_run_cmd $QEMU_IMG rebase  $L "${TEST_IMG}" -b "${TEST_IMG}.base"
+_run_cmd $QEMU_IMG snapshot -l $L "${TEST_IMG}"
+_run_cmd $QEMU_IMG convert $L "${TEST_IMG}" "${TEST_IMG}.convert"
+_run_cmd $QEMU_IMG dd  $L if="${TEST_IMG}" 
of="${TEST_IMG}.convert" bs=512 count=1
+_run_cmd $QEMU_IMG bench   $L -c 1 "${TEST_IMG}"
+_run_cmd $QEMU_IMG bench   $L -w -c 1 "${TEST_IMG}"
+done
+_send_qemu_cmd $h "{ 'execute': 'quit', }" ""
+echo
+echo "Round done"
+_cleanup_qemu
+done
+
+for opt1 in $test_opts; do
+for opt2 in $test_opts; do
+echo
+echo "== Two devices with the same image ($opt1 - $opt2) =="
+_run_qemu_with_images "${TEST_IMG},$opt1" "${TEST_IMG},$opt2"
+done
+done
+
+echo "== Creating ${TEST_IMG}.[abc] =="
+(
+$QEMU_IMG create -f qcow2 "${TEST_IMG}.a" -b "${TEST_IMG}"
+$QEMU_IMG create -f qcow2 "${TEST_IMG}.b" -b "${TEST_IMG}"
+$QEMU_IMG create -f qcow2 "${TEST_IMG}.c" -b "${TEST_IMG}.b"
+) | _filter_img_create
+
+echo
+echo "== Two devices sharing 

[Qemu-block] [PATCH v14 19/20] file-posix: Add image locking in perm operations

[Fam Zheng]

virtlockd in libvirt locks the first byte, so we start looking at the
file bytes from 0x10.

The complication is in the transactional interface.  To make the reopen
logic managable, and allow better reuse, the code is internally
organized with a table from old mode to the new one.

Signed-off-by: Fam Zheng 
---
 block/file-posix.c | 739 -
 1 file changed, 736 insertions(+), 3 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 2114720..c307493 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -129,8 +129,54 @@ do { \
 
 #define MAX_BLOCKSIZE  4096
 
+/* Posix file locking bytes. Libvirt takes byte 0, we start from byte 0x10,
+ * leaving a few more bytes for its future use. */
+#define RAW_LOCK_BYTE_MIN 0x10
+#define RAW_LOCK_BYTE_NO_OTHER_WRITER 0x10
+#define RAW_LOCK_BYTE_WRITE   0x11
+#ifdef F_OFD_SETLK
+#define RAW_LOCK_SUPPORTED 1
+#else
+#define RAW_LOCK_SUPPORTED 0
+#endif
+
+/*
+ ** reader that can tolerate writers: Don't do anything
+ *
+ ** reader that can't tolerate writers: Take shared lock on byte 0x10. Test
+ *  byte 0x11 is unlocked.
+ *
+ ** shared writer: Take shared lock on byte 0x11. Test byte 0x10 is unlocked.
+ *
+ ** exclusive writer: Take exclusive locks on both bytes.
+ */
+
+typedef enum {
+/* Read only and accept other writers. */
+RAW_L_READ_SHARE_RW,
+/* Read only and try to forbid other writers. */
+RAW_L_READ,
+/* Read/write and accept other writers. */
+RAW_L_WRITE_SHARE_RW,
+/* Read/write and try to forbid other writers. */
+RAW_L_WRITE,
+} BDRVRawLockMode;
+
+typedef struct BDRVRawLockUpdateState {
+/* A dup of @fd used for acquiring lock. */
+int image_fd;
+int lock_fd;
+int open_flags;
+BDRVRawLockMode new_lock;
+bool use_lock;
+} BDRVRawLockUpdateState;
+
 typedef struct BDRVRawState {
 int fd;
+/* A dup of @fd to make manipulating lock easier, especially during reopen,
+ * where this will accept BDRVRawReopenState.lock_fd. */
+int lock_fd;
+bool use_lock;
 int type;
 int open_flags;
 size_t buf_align;
@@ -145,6 +191,11 @@ typedef struct BDRVRawState {
 bool page_cache_inconsistent:1;
 bool has_fallocate;
 bool needs_alignment;
+/* The current lock mode we are in. Note that in incoming migration this is
+ * the "desired" mode to be applied at bdrv_invalidate_cache. */
+BDRVRawLockMode cur_lock_mode;
+/* Used by raw_check_perm/raw_set_perm. */
+BDRVRawLockUpdateState *lock_update;
 } BDRVRawState;
 
 typedef struct BDRVRawReopenState {
@@ -367,6 +418,59 @@ static void raw_parse_flags(int bdrv_flags, int 
*open_flags)
 }
 }
 
+static int raw_lock_fd(int fd, BDRVRawLockMode mode, Error **errp)
+{
+int ret;
+assert(fd >= 0);
+assert(RAW_LOCK_SUPPORTED);
+switch (mode) {
+case RAW_L_READ_SHARE_RW:
+ret = qemu_unlock_fd(fd, RAW_LOCK_BYTE_MIN, 2);
+if (ret) {
+error_setg_errno(errp, -ret, "Failed to unlock fd");
+goto fail;
+}
+break;
+case RAW_L_READ:
+ret = qemu_lock_fd(fd, RAW_LOCK_BYTE_NO_OTHER_WRITER, 1, false);
+if (ret) {
+error_setg_errno(errp, -ret, "Failed to lock share byte");
+goto fail;
+}
+ret = qemu_lock_fd_test(fd, RAW_LOCK_BYTE_WRITE, 1, true);
+if (ret) {
+error_setg_errno(errp, -ret, "Write byte lock is taken");
+goto fail;
+}
+break;
+case RAW_L_WRITE_SHARE_RW:
+ret = qemu_lock_fd(fd, RAW_LOCK_BYTE_WRITE, 1, false);
+if (ret) {
+error_setg_errno(errp, -ret, "Failed to lock write byte");
+goto fail;
+}
+ret = qemu_lock_fd_test(fd, RAW_LOCK_BYTE_NO_OTHER_WRITER, 1, true);
+if (ret) {
+error_setg_errno(errp, -ret, "Share byte lock is taken");
+goto fail;
+}
+break;
+case RAW_L_WRITE:
+ret = qemu_lock_fd(fd, RAW_LOCK_BYTE_MIN, 2, true);
+if (ret) {
+error_setg_errno(errp, -ret, "Failed to lock image");
+goto fail;
+}
+break;
+default:
+abort();
+}
+return 0;
+fail:
+qemu_unlock_fd(fd, RAW_LOCK_BYTE_MIN, 2);
+return ret;
+}
+
 static void raw_parse_filename(const char *filename, QDict *options,
Error **errp)
 {
@@ -401,6 +505,23 @@ static QemuOptsList raw_runtime_opts = {
 },
 };
 
+static BDRVRawLockMode raw_get_lock_mode(bool write, bool shared)
+{
+if (write) {
+if (shared) {
+return RAW_L_WRITE_SHARE_RW;
+} else {
+return RAW_L_WRITE;
+}
+} else {
+if (shared) {
+return RAW_L_READ_SHARE_RW;
+} else {
+return RAW_L_READ;
+}
+}
+}
+
 static int raw_open_common(BlockDriverState *bs, QDict *options,
   

[Qemu-block] [PATCH v14 15/20] file-posix: Add 'locking' option

[Fam Zheng]

Making this option available even before implementing it will let
converting tests easier: in coming patches they can specify the option
already when necessary, before we actually write code to lock the
images.

Signed-off-by: Fam Zheng 
---
 block/file-posix.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index ade71db..2114720 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -392,6 +392,11 @@ static QemuOptsList raw_runtime_opts = {
 .type = QEMU_OPT_STRING,
 .help = "host AIO implementation (threads, native)",
 },
+{
+.name = "locking",
+.type = QEMU_OPT_BOOL,
+.help = "lock the file",
+},
 { /* end of list */ }
 },
 };
-- 
2.9.3

[Qemu-block] [PATCH v14 18/20] osdep: Add qemu_lock_fd and qemu_unlock_fd

[Fam Zheng]

They are wrappers of POSIX fcntl "file private locking", with a
convenient "try lock" wrapper implemented with F_OFD_GETLK.

Signed-off-by: Fam Zheng 
Reviewed-by: Max Reitz 
---
 include/qemu/osdep.h |  3 +++
 util/osdep.c | 48 
 2 files changed, 51 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 122ff06..1c9f5e2 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -341,6 +341,9 @@ int qemu_close(int fd);
 #ifndef _WIN32
 int qemu_dup(int fd);
 #endif
+int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
+int qemu_unlock_fd(int fd, int64_t start, int64_t len);
+int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive);
 
 #if defined(__HAIKU__) && defined(__i386__)
 #define FMT_pid "%ld"
diff --git a/util/osdep.c b/util/osdep.c
index 06fb1cf..3de4a18 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -140,6 +140,54 @@ static int qemu_parse_fdset(const char *param)
 {
 return qemu_parse_fd(param);
 }
+
+static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
+{
+#ifdef F_OFD_SETLK
+int ret;
+struct flock fl = {
+.l_whence = SEEK_SET,
+.l_start  = start,
+.l_len= len,
+.l_type   = fl_type,
+};
+ret = fcntl(fd, F_OFD_SETLK, );
+return ret == -1 ? -errno : 0;
+#else
+return -ENOTSUP;
+#endif
+}
+
+int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive)
+{
+return qemu_lock_fcntl(fd, start, len, exclusive ? F_WRLCK : F_RDLCK);
+}
+
+int qemu_unlock_fd(int fd, int64_t start, int64_t len)
+{
+return qemu_lock_fcntl(fd, start, len, F_UNLCK);
+}
+
+int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
+{
+#ifdef F_OFD_SETLK
+int ret;
+struct flock fl = {
+.l_whence = SEEK_SET,
+.l_start  = start,
+.l_len= len,
+.l_type   = exclusive ? F_WRLCK : F_RDLCK,
+};
+ret = fcntl(fd, F_OFD_GETLK, );
+if (ret == -1) {
+return -errno;
+} else {
+return fl.l_type == F_UNLCK ? 0 : -EAGAIN;
+}
+#else
+return -ENOTSUP;
+#endif
+}
 #endif
 
 /*
-- 
2.9.3

[Qemu-block] [PATCH v14 17/20] block: Reuse bs as backing hd for drive-backup sync=none

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 blockdev.c | 15 ++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 4927914..4e04dec 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3174,6 +3174,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
BlockJobTxn *txn,
 Error *local_err = NULL;
 int flags;
 int64_t size;
+bool set_backing_hd = false;
 
 if (!backup->has_speed) {
 backup->speed = 0;
@@ -3224,6 +3225,8 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
BlockJobTxn *txn,
 }
 if (backup->sync == MIRROR_SYNC_MODE_NONE) {
 source = bs;
+flags |= BDRV_O_NO_BACKING;
+set_backing_hd = true;
 }
 
 size = bdrv_getlength(bs);
@@ -3250,7 +3253,9 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
BlockJobTxn *txn,
 }
 
 if (backup->format) {
-options = qdict_new();
+if (!options) {
+options = qdict_new();
+}
 qdict_put(options, "driver", qstring_from_str(backup->format));
 }
 
@@ -3261,6 +3266,14 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
BlockJobTxn *txn,
 
 bdrv_set_aio_context(target_bs, aio_context);
 
+if (set_backing_hd) {
+bdrv_set_backing_hd(target_bs, source, _err);
+if (local_err) {
+bdrv_unref(target_bs);
+goto out;
+}
+}
+
 if (backup->has_bitmap) {
 bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap);
 if (!bmap) {
-- 
2.9.3

[Qemu-block] [PATCH v14 12/20] iotests: 091: Quit QEMU before checking image

[Fam Zheng]

Signed-off-by: Fam Zheng 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/091 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qemu-iotests/091 b/tests/qemu-iotests/091
index 32bbd56..10ac4a8 100755
--- a/tests/qemu-iotests/091
+++ b/tests/qemu-iotests/091
@@ -95,7 +95,9 @@ echo "vm2: qemu process running successfully"
 echo "vm2: flush io, and quit"
 _send_qemu_cmd $h2 'qemu-io disk flush' "(qemu)"
 _send_qemu_cmd $h2 'quit' ""
+_send_qemu_cmd $h1 'quit' ""
 
+wait
 echo "Check image pattern"
 ${QEMU_IO} -c "read -P 0x22 0 4M" "${TEST_IMG}" | _filter_testdir | 
_filter_qemu_io
 
-- 
2.9.3

[Qemu-block] [PATCH v14 13/20] iotests: 172: Use separate images for multiple devices

[Fam Zheng]

To avoid image lock failures.

Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/172 | 55 +-
 tests/qemu-iotests/172.out | 50 +
 2 files changed, 56 insertions(+), 49 deletions(-)

diff --git a/tests/qemu-iotests/172 b/tests/qemu-iotests/172
index 1b7d3a1..826d6fe 100755
--- a/tests/qemu-iotests/172
+++ b/tests/qemu-iotests/172
@@ -30,6 +30,8 @@ status=1  # failure is the default!
 _cleanup()
 {
_cleanup_test_img
+rm -f "$TEST_IMG.2"
+rm -f "$TEST_IMG.3"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -86,6 +88,9 @@ size=720k
 
 _make_test_img $size
 
+TEST_IMG="$TEST_IMG.2" _make_test_img $size
+TEST_IMG="$TEST_IMG.3" _make_test_img $size
+
 # Default drive semantics:
 #
 # By default you get a single empty floppy drive. You can override it with
@@ -105,7 +110,7 @@ echo === Using -fda/-fdb options ===
 
 check_floppy_qtree -fda "$TEST_IMG"
 check_floppy_qtree -fdb "$TEST_IMG"
-check_floppy_qtree -fda "$TEST_IMG" -fdb "$TEST_IMG"
+check_floppy_qtree -fda "$TEST_IMG" -fdb "$TEST_IMG.2"
 
 
 echo
@@ -114,7 +119,7 @@ echo === Using -drive options ===
 
 check_floppy_qtree -drive if=floppy,file="$TEST_IMG"
 check_floppy_qtree -drive if=floppy,file="$TEST_IMG",index=1
-check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive 
if=floppy,file="$TEST_IMG",index=1
+check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive 
if=floppy,file="$TEST_IMG.2",index=1
 
 echo
 echo
@@ -122,7 +127,7 @@ echo === Using -drive if=none and -global ===
 
 check_floppy_qtree -drive if=none,file="$TEST_IMG" -global isa-fdc.driveA=none0
 check_floppy_qtree -drive if=none,file="$TEST_IMG" -global isa-fdc.driveB=none0
-check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG" \
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG.2" \
-global isa-fdc.driveA=none0 -global isa-fdc.driveB=none1
 
 echo
@@ -131,7 +136,7 @@ echo === Using -drive if=none and -device ===
 
 check_floppy_qtree -drive if=none,file="$TEST_IMG" -device floppy,drive=none0
 check_floppy_qtree -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0,unit=1
-check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG" \
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG.2" \
-device floppy,drive=none0 -device floppy,drive=none1,unit=1
 
 echo
@@ -139,58 +144,58 @@ echo
 echo === Mixing -fdX and -global ===
 
 # Working
-check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveB=none0
-check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveA=none0
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -global 
isa-fdc.driveB=none0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -global 
isa-fdc.driveA=none0
 
 # Conflicting (-fdX wins)
-check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveA=none0
-check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveB=none0
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -global 
isa-fdc.driveA=none0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -global 
isa-fdc.driveB=none0
 
 echo
 echo
 echo === Mixing -fdX and -device ===
 
 # Working
-check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0
-check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0,unit=1
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device 
floppy,drive=none0
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device 
floppy,drive=none0,unit=1
 
-check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0
-check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0,unit=0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device 
floppy,drive=none0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device 
floppy,drive=none0,unit=0
 
 # Conflicting
-check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0,unit=0
-check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0,unit=1
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device 
floppy,drive=none0,unit=0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device 
floppy,drive=none0,unit=1
 
 echo
 echo
 echo === Mixing -drive and -device ===
 
 # Working
-check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG" -device floppy,drive=none0
-check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive 

[Qemu-block] [PATCH v14 11/20] iotests: 087: Don't attach test image twice

[Fam Zheng]

The test scenario doesn't require the same image, instead it focuses on
the duplicated node-name, so use null-co to avoid locking conflict.

Reviewed-by: Max Reitz 
Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/087 | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index 9de57dd..6d52f7d 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -82,8 +82,7 @@ run_qemu -drive 
driver=$IMGFMT,id=disk,node-name=test-node,file="$TEST_IMG" <

[Qemu-block] [PATCH v14 08/20] iotests: 046: Prepare for image locking

[Fam Zheng]

The qemu-img info command is executed while VM is running, add -U option
to avoid the image locking error.

Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/046 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/046 b/tests/qemu-iotests/046
index e528b67..f2ebecf 100755
--- a/tests/qemu-iotests/046
+++ b/tests/qemu-iotests/046
@@ -192,7 +192,7 @@ echo "== Verify image content =="
 
 function verify_io()
 {
-if ($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep "compat: 0.10" > 
/dev/null); then
+if ($QEMU_IMG info -U -f "$IMGFMT" "$TEST_IMG" | grep "compat: 0.10" > 
/dev/null); then
 # For v2 images, discarded clusters are read from the backing file
 # Keep the variable empty so that the backing file value can be used as
 # the default below
-- 
2.9.3

[Qemu-block] [PATCH v14 10/20] iotests: 085: Avoid image locking conflict

[Fam Zheng]

In the case where we test the expected error when a blockdev-snapshot
target already has a backing image, the backing chain is opened multiple
times. This will be a problem when we use image locking, so use a
different backing file that is not already open.

Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/085 | 34 --
 tests/qemu-iotests/085.out |  3 ++-
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index c53e97f..cc6efd8 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -45,7 +45,7 @@ _cleanup()
 rm -f "${TEST_DIR}/${i}-${snapshot_virt0}"
 rm -f "${TEST_DIR}/${i}-${snapshot_virt1}"
 done
-rm -f "${TEST_IMG}.1" "${TEST_IMG}.2"
+rm -f "${TEST_IMG}" "${TEST_IMG}.1" "${TEST_IMG}.2" "${TEST_IMG}.base"
 
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
@@ -87,24 +87,26 @@ function create_group_snapshot()
 }
 
 # ${1}: unique identifier for the snapshot filename
-# ${2}: true: open backing images; false: don't open them (default)
+# ${2}: extra_params to the blockdev-add command
+# ${3}: filename
+function do_blockdev_add()
+{
+cmd="{ 'execute': 'blockdev-add', 'arguments':
+   { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${2}
+ 'file':
+ { 'driver': 'file', 'filename': '${3}',
+   'node-name': 'file_${1}' } } }"
+_send_qemu_cmd $h "${cmd}" "return"
+}
+
+# ${1}: unique identifier for the snapshot filename
 function add_snapshot_image()
 {
-if [ "${2}" = "true" ]; then
-extra_params=""
-else
-extra_params="'backing': '', "
-fi
 base_image="${TEST_DIR}/$((${1}-1))-${snapshot_virt0}"
 snapshot_file="${TEST_DIR}/${1}-${snapshot_virt0}"
 _make_test_img -b "${base_image}" "$size"
 mv "${TEST_IMG}" "${snapshot_file}"
-cmd="{ 'execute': 'blockdev-add', 'arguments':
-   { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${extra_params}
- 'file':
- { 'driver': 'file', 'filename': '${snapshot_file}',
-   'node-name': 'file_${1}' } } }"
-_send_qemu_cmd $h "${cmd}" "return"
+do_blockdev_add "$1" "'backing': '', " "${snapshot_file}"
 }
 
 # ${1}: unique identifier for the snapshot filename
@@ -222,7 +224,11 @@ echo === Invalid command - snapshot node has a backing 
image ===
 echo
 
 SNAPSHOTS=$((${SNAPSHOTS}+1))
-add_snapshot_image ${SNAPSHOTS} true
+
+_make_test_img "$size"
+mv "${TEST_IMG}" "${TEST_IMG}.base"
+_make_test_img -b "${TEST_IMG}.base" "$size"
+do_blockdev_add ${SNAPSHOTS} "" "${TEST_IMG}"
 blockdev_snapshot ${SNAPSHOTS} error
 
 echo
diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out
index 182acb4..65b0f68 100644
--- a/tests/qemu-iotests/085.out
+++ b/tests/qemu-iotests/085.out
@@ -78,7 +78,8 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
backing_file=TEST_DIR/
 
 === Invalid command - snapshot node has a backing image ===
 
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
backing_file=TEST_DIR/12-snapshot-v0.IMGFMT
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
backing_file=TEST_DIR/t.IMGFMT.base
 {"return": {}}
 {"error": {"class": "GenericError", "desc": "The snapshot already has a 
backing image"}}
 
-- 
2.9.3

[Qemu-block] [PATCH v14 09/20] iotests: 055: Don't attach the target image already for drive-backup

[Fam Zheng]

Double attach is not a valid usage of the target image, drive-backup
will open the blockdev itself so skip the add_drive call in this case.

Signed-off-by: Fam Zheng 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/055 | 32 ++--
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index aafcd24..ba4da65 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -458,17 +458,18 @@ class TestDriveCompression(iotests.QMPTestCase):
 except OSError:
 pass
 
-def do_prepare_drives(self, fmt, args):
+def do_prepare_drives(self, fmt, args, attach_target):
 self.vm = iotests.VM().add_drive(test_img)
 
 qemu_img('create', '-f', fmt, blockdev_target_img,
  str(TestDriveCompression.image_len), *args)
-self.vm.add_drive(blockdev_target_img, format=fmt, interface="none")
+if attach_target:
+self.vm.add_drive(blockdev_target_img, format=fmt, 
interface="none")
 
 self.vm.launch()
 
-def do_test_compress_complete(self, cmd, format, **args):
-self.do_prepare_drives(format['type'], format['args'])
+def do_test_compress_complete(self, cmd, format, attach_target, **args):
+self.do_prepare_drives(format['type'], format['args'], attach_target)
 
 self.assert_no_active_block_jobs()
 
@@ -484,15 +485,16 @@ class TestDriveCompression(iotests.QMPTestCase):
 
 def test_complete_compress_drive_backup(self):
 for format in TestDriveCompression.fmt_supports_compression:
-self.do_test_compress_complete('drive-backup', format,
+self.do_test_compress_complete('drive-backup', format, False,
target=blockdev_target_img, 
mode='existing')
 
 def test_complete_compress_blockdev_backup(self):
 for format in TestDriveCompression.fmt_supports_compression:
-self.do_test_compress_complete('blockdev-backup', format, 
target='drive1')
+self.do_test_compress_complete('blockdev-backup', format, True,
+   target='drive1')
 
-def do_test_compress_cancel(self, cmd, format, **args):
-self.do_prepare_drives(format['type'], format['args'])
+def do_test_compress_cancel(self, cmd, format, attach_target, **args):
+self.do_prepare_drives(format['type'], format['args'], attach_target)
 
 self.assert_no_active_block_jobs()
 
@@ -506,15 +508,16 @@ class TestDriveCompression(iotests.QMPTestCase):
 
 def test_compress_cancel_drive_backup(self):
 for format in TestDriveCompression.fmt_supports_compression:
-self.do_test_compress_cancel('drive-backup', format,
+self.do_test_compress_cancel('drive-backup', format, False,
  target=blockdev_target_img, 
mode='existing')
 
 def test_compress_cancel_blockdev_backup(self):
for format in TestDriveCompression.fmt_supports_compression:
-self.do_test_compress_cancel('blockdev-backup', format, 
target='drive1')
+self.do_test_compress_cancel('blockdev-backup', format, True,
+ target='drive1')
 
-def do_test_compress_pause(self, cmd, format, **args):
-self.do_prepare_drives(format['type'], format['args'])
+def do_test_compress_pause(self, cmd, format, attach_target, **args):
+self.do_prepare_drives(format['type'], format['args'], attach_target)
 
 self.assert_no_active_block_jobs()
 
@@ -546,12 +549,13 @@ class TestDriveCompression(iotests.QMPTestCase):
 
 def test_compress_pause_drive_backup(self):
 for format in TestDriveCompression.fmt_supports_compression:
-self.do_test_compress_pause('drive-backup', format,
+self.do_test_compress_pause('drive-backup', format, False,
 target=blockdev_target_img, 
mode='existing')
 
 def test_compress_pause_blockdev_backup(self):
 for format in TestDriveCompression.fmt_supports_compression:
-self.do_test_compress_pause('blockdev-backup', format, 
target='drive1')
+self.do_test_compress_pause('blockdev-backup', format, True,
+target='drive1')
 
 if __name__ == '__main__':
 iotests.main(supported_fmts=['raw', 'qcow2'])
-- 
2.9.3

[Qemu-block] [PATCH v14 16/20] tests: Disable image lock in test-replication

[Fam Zheng]

The COLO block replication architecture requires one disk to be shared
between primary and secondary, in the test both processes use posix file
protocol (instead of over NBD) so it is affected by image locking.
Disable the lock.

Signed-off-by: Fam Zheng 
---
 tests/test-replication.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tests/test-replication.c b/tests/test-replication.c
index fac2da3..e1c4235 100644
--- a/tests/test-replication.c
+++ b/tests/test-replication.c
@@ -179,7 +179,8 @@ static BlockBackend *start_primary(void)
 char *cmdline;
 
 cmdline = g_strdup_printf("driver=replication,mode=primary,node-name=xxx,"
-  "file.driver=qcow2,file.file.filename=%s"
+  "file.driver=qcow2,file.file.filename=%s,"
+  "file.file.locking=off"
   , p_local_disk);
 opts = qemu_opts_parse_noisily(_drive_opts, cmdline, false);
 g_free(cmdline);
@@ -310,7 +311,9 @@ static BlockBackend *start_secondary(void)
 Error *local_err = NULL;
 
 /* add s_local_disk and forge S_LOCAL_DISK_ID */
-cmdline = g_strdup_printf("file.filename=%s,driver=qcow2", s_local_disk);
+cmdline = g_strdup_printf("file.filename=%s,driver=qcow2,"
+  "file.locking=off",
+  s_local_disk);
 opts = qemu_opts_parse_noisily(_drive_opts, cmdline, false);
 g_free(cmdline);
 
@@ -331,8 +334,10 @@ static BlockBackend *start_secondary(void)
 /* add S_(ACTIVE/HIDDEN)_DISK and forge S_ID */
 cmdline = g_strdup_printf("driver=replication,mode=secondary,top-id=%s,"
   "file.driver=qcow2,file.file.filename=%s,"
+  "file.file.locking=off,"
   "file.backing.driver=qcow2,"
   "file.backing.file.filename=%s,"
+  "file.backing.file.locking=off,"
   "file.backing.backing=%s"
   , S_ID, s_active_disk, s_hidden_disk
   , S_LOCAL_DISK_ID);
-- 
2.9.3

[Qemu-block] [PATCH v14 14/20] tests: Use null-co:// instead of /dev/null as the dummy image

[Fam Zheng]

Signed-off-by: Fam Zheng 
Reviewed-by: Max Reitz 
---
 tests/drive_del-test.c| 2 +-
 tests/nvme-test.c | 2 +-
 tests/usb-hcd-uhci-test.c | 2 +-
 tests/usb-hcd-xhci-test.c | 2 +-
 tests/virtio-blk-test.c   | 2 +-
 tests/virtio-scsi-test.c  | 4 ++--
 6 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c
index 121b9c9..2175139 100644
--- a/tests/drive_del-test.c
+++ b/tests/drive_del-test.c
@@ -92,7 +92,7 @@ static void test_after_failed_device_add(void)
 static void test_drive_del_device_del(void)
 {
 /* Start with a drive used by a device that unplugs instantaneously */
-qtest_start("-drive if=none,id=drive0,file=/dev/null,format=raw"
+qtest_start("-drive if=none,id=drive0,file=null-co://,format=raw"
 " -device virtio-scsi-pci"
 " -device scsi-hd,drive=drive0,id=dev0");
 
diff --git a/tests/nvme-test.c b/tests/nvme-test.c
index c8bece4..7674a44 100644
--- a/tests/nvme-test.c
+++ b/tests/nvme-test.c
@@ -22,7 +22,7 @@ int main(int argc, char **argv)
 g_test_init(, , NULL);
 qtest_add_func("/nvme/nop", nop);
 
-qtest_start("-drive id=drv0,if=none,file=/dev/null,format=raw "
+qtest_start("-drive id=drv0,if=none,file=null-co://,format=raw "
 "-device nvme,drive=drv0,serial=foo");
 ret = g_test_run();
 
diff --git a/tests/usb-hcd-uhci-test.c b/tests/usb-hcd-uhci-test.c
index f25bae5..5b500fe 100644
--- a/tests/usb-hcd-uhci-test.c
+++ b/tests/usb-hcd-uhci-test.c
@@ -79,7 +79,7 @@ int main(int argc, char **argv)
 {
 const char *arch = qtest_get_arch();
 const char *cmd = "-device piix3-usb-uhci,id=uhci,addr=1d.0"
-  " -drive id=drive0,if=none,file=/dev/null,format=raw"
+  " -drive id=drive0,if=none,file=null-co://,format=raw"
   " -device usb-tablet,bus=uhci.0,port=1";
 int ret;
 
diff --git a/tests/usb-hcd-xhci-test.c b/tests/usb-hcd-xhci-test.c
index 22513e9..031764d 100644
--- a/tests/usb-hcd-xhci-test.c
+++ b/tests/usb-hcd-xhci-test.c
@@ -89,7 +89,7 @@ int main(int argc, char **argv)
 qtest_add_func("/xhci/pci/hotplug/usb-uas", test_usb_uas_hotplug);
 
 qtest_start("-device nec-usb-xhci,id=xhci"
-" -drive id=drive0,if=none,file=/dev/null,format=raw");
+" -drive id=drive0,if=none,file=null-co://,format=raw");
 ret = g_test_run();
 qtest_end();
 
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 1eee95d..fd2078c 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -63,7 +63,7 @@ static QOSState *pci_test_start(void)
 const char *arch = qtest_get_arch();
 char *tmp_path;
 const char *cmd = "-drive if=none,id=drive0,file=%s,format=raw "
-  "-drive if=none,id=drive1,file=/dev/null,format=raw "
+  "-drive if=none,id=drive1,file=null-co://,format=raw "
   "-device virtio-blk-pci,id=drv0,drive=drive0,"
   "addr=%x.%x";
 
diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
index 0eabd56..5e2ff14 100644
--- a/tests/virtio-scsi-test.c
+++ b/tests/virtio-scsi-test.c
@@ -35,7 +35,7 @@ typedef struct {
 static QOSState *qvirtio_scsi_start(const char *extra_opts)
 {
 const char *arch = qtest_get_arch();
-const char *cmd = "-drive id=drv0,if=none,file=/dev/null,format=raw "
+const char *cmd = "-drive id=drv0,if=none,file=null-co://,format=raw "
   "-device virtio-scsi-pci,id=vs0 "
   "-device scsi-hd,bus=vs0.0,drive=drv0 %s";
 
@@ -195,7 +195,7 @@ static void hotplug(void)
 QDict *response;
 QOSState *qs;
 
-qs = qvirtio_scsi_start("-drive 
id=drv1,if=none,file=/dev/null,format=raw");
+qs = qvirtio_scsi_start("-drive 
id=drv1,if=none,file=null-co://,format=raw");
 response = qmp("{\"execute\": \"device_add\","
" \"arguments\": {"
"   \"driver\": \"scsi-hd\","
-- 
2.9.3

[Qemu-block] [PATCH v14 07/20] iotests: 030: Prepare for image locking

[Fam Zheng]

qemu-img and qemu-io commands when guest is running need "-U" option,
add it.

Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/030 | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 0d472d5..5f1dce8 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -63,8 +63,8 @@ class TestSingleDrive(iotests.QMPTestCase):
 def test_stream_intermediate(self):
 self.assert_no_active_block_jobs()
 
-self.assertNotEqual(qemu_io('-f', 'raw', '-c', 'map', backing_img),
-qemu_io('-f', iotests.imgfmt, '-c', 'map', 
mid_img),
+self.assertNotEqual(qemu_io('-f', 'raw', '-rU', '-c', 'map', 
backing_img),
+qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', 
mid_img),
 'image file map matches backing file before 
streaming')
 
 result = self.vm.qmp('block-stream', device='mid', job_id='stream-mid')
@@ -114,7 +114,7 @@ class TestSingleDrive(iotests.QMPTestCase):
 self.assert_no_active_block_jobs()
 
 # The image map is empty before the operation
-empty_map = qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img)
+empty_map = qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', test_img)
 
 # This is a no-op: no data should ever be copied from the base image
 result = self.vm.qmp('block-stream', device='drive0', base=mid_img)
@@ -125,7 +125,7 @@ class TestSingleDrive(iotests.QMPTestCase):
 self.assert_no_active_block_jobs()
 self.vm.shutdown()
 
-self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img),
+self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', '-r', 
test_img),
  empty_map, 'image file map changed after a no-op')
 
 def test_stream_partial(self):
@@ -197,8 +197,8 @@ class TestParallelOps(iotests.QMPTestCase):
 
 # Check that the maps don't match before the streaming operations
 for i in range(2, self.num_imgs, 2):
-self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', 
self.imgs[i]),
-qemu_io('-f', iotests.imgfmt, '-c', 'map', 
self.imgs[i-1]),
+self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-U', '-c', 
'map', self.imgs[i]),
+qemu_io('-f', iotests.imgfmt, '-U', '-c', 
'map', self.imgs[i-1]),
 'image file map matches backing file before 
streaming')
 
 # Create all streaming jobs
@@ -351,8 +351,8 @@ class TestParallelOps(iotests.QMPTestCase):
 def test_stream_base_node_name(self):
 self.assert_no_active_block_jobs()
 
-self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', 
self.imgs[4]),
-qemu_io('-f', iotests.imgfmt, '-c', 'map', 
self.imgs[3]),
+self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-U', '-c', 'map', 
self.imgs[4]),
+qemu_io('-f', iotests.imgfmt, '-U', '-c', 'map', 
self.imgs[3]),
 'image file map matches backing file before 
streaming')
 
 # Error: the base node does not exist
@@ -422,8 +422,8 @@ class TestQuorum(iotests.QMPTestCase):
 if not iotests.supports_quorum():
 return
 
-self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', 
self.children[0]),
-qemu_io('-f', iotests.imgfmt, '-c', 'map', 
self.backing[0]),
+self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-U', '-c', 'map', 
self.children[0]),
+qemu_io('-f', iotests.imgfmt, '-U', '-c', 'map', 
self.backing[0]),
 'image file map matches backing file before 
streaming')
 
 self.assert_no_active_block_jobs()
@@ -436,8 +436,8 @@ class TestQuorum(iotests.QMPTestCase):
 self.assert_no_active_block_jobs()
 self.vm.shutdown()
 
-self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', 
self.children[0]),
- qemu_io('-f', iotests.imgfmt, '-c', 'map', 
self.backing[0]),
+self.assertEqual(qemu_io('-f', iotests.imgfmt, '-U', '-c', 'map', 
self.children[0]),
+ qemu_io('-f', iotests.imgfmt, '-U', '-c', 'map', 
self.backing[0]),
  'image file map does not match backing file after 
streaming')
 
 class TestSmallerBackingFile(iotests.QMPTestCase):
-- 
2.9.3

[Qemu-block] [PATCH v14 05/20] qemu-img: Update documentation for --share-rw

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 qemu-img-cmds.hx | 48 
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 8ac7822..1b00bb8 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -10,15 +10,15 @@ STEXI
 ETEXI
 
 DEF("bench", img_bench,
-"bench [-c count] [-d depth] [-f fmt] [--flush-interval=flush_interval] 
[-n] [--no-drain] [-o offset] [--pattern=pattern] [-q] [-s buffer_size] [-S 
step_size] [-t cache] [-w] filename")
+"bench [-c count] [-d depth] [-f fmt] [--flush-interval=flush_interval] 
[-n] [--no-drain] [-o offset] [--pattern=pattern] [-q] [-s buffer_size] [-S 
step_size] [-t cache] [-w] [--share-rw] filename")
 STEXI
-@item bench [-c @var{count}] [-d @var{depth}] [-f @var{fmt}] 
[--flush-interval=@var{flush_interval}] [-n] [--no-drain] [-o @var{offset}] 
[--pattern=@var{pattern}] [-q] [-s @var{buffer_size}] [-S @var{step_size}] [-t 
@var{cache}] [-w] @var{filename}
+@item bench [-c @var{count}] [-d @var{depth}] [-f @var{fmt}] 
[--flush-interval=@var{flush_interval}] [-n] [--no-drain] [-o @var{offset}] 
[--pattern=@var{pattern}] [-q] [-s @var{buffer_size}] [-S @var{step_size}] [-t 
@var{cache}] [-w] [--share-rw] @var{filename}
 ETEXI
 
 DEF("check", img_check,
-"check [-q] [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] 
[-r [leaks | all]] [-T src_cache] filename")
+"check [-q] [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] 
[-r [leaks | all]] [-T src_cache] [--share-rw] filename")
 STEXI
-@item check [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] 
[--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] @var{filename}
+@item check [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] 
[--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] [--share-rw] 
@var{filename}
 ETEXI
 
 DEF("create", img_create,
@@ -28,62 +28,62 @@ STEXI
 ETEXI
 
 DEF("commit", img_commit,
-"commit [-q] [--object objectdef] [--image-opts] [-f fmt] [-t cache] [-b 
base] [-d] [-p] filename")
+"commit [-q] [--object objectdef] [--image-opts] [-f fmt] [-t cache] [-b 
base] [-d] [-p] [--share-rw] filename")
 STEXI
-@item commit [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] [-t 
@var{cache}] [-b @var{base}] [-d] [-p] @var{filename}
+@item commit [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] [-t 
@var{cache}] [-b @var{base}] [-d] [-p] [--share-rw] @var{filename}
 ETEXI
 
 DEF("compare", img_compare,
-"compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T 
src_cache] [-p] [-q] [-s] filename1 filename2")
+"compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T 
src_cache] [-p] [-q] [-s] [--share-rw] filename1 filename2")
 STEXI
-@item compare [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [-F 
@var{fmt}] [-T @var{src_cache}] [-p] [-q] [-s] @var{filename1} @var{filename2}
+@item compare [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [-F 
@var{fmt}] [-T @var{src_cache}] [-p] [-q] [-s] [--share-rw] @var{filename1} 
@var{filename2}
 ETEXI
 
 DEF("convert", img_convert,
-"convert [--object objectdef] [--image-opts] [-c] [-p] [-q] [-n] [-f fmt] 
[-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] 
[-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] filename 
[filename2 [...]] output_filename")
+"convert [--object objectdef] [--image-opts] [-c] [-p] [-q] [-n] [-f fmt] 
[-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] 
[-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] [--share-rw] 
filename [filename2 [...]] output_filename")
 STEXI
-@item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-n] 
[-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o 
@var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S 
@var{sparse_size}] [-m @var{num_coroutines}] [-W] @var{filename} 
[@var{filename2} [...]] @var{output_filename}
+@item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-n] 
[-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o 
@var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S 
@var{sparse_size}] [-m @var{num_coroutines}] [-W] [--share-rw] @var{filename} 
[@var{filename2} [...]] @var{output_filename}
 ETEXI
 
 DEF("dd", img_dd,
-"dd [--image-opts] [-f fmt] [-O output_fmt] [bs=block_size] [count=blocks] 
[skip=blocks] if=input of=output")
+"dd [--image-opts] [--share-rw] [-f fmt] [-O output_fmt] [bs=block_size] 
[count=blocks] [skip=blocks] if=input of=output")
 STEXI
-@item dd [--image-opts] [-f @var{fmt}] [-O @var{output_fmt}] 
[bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} 
of=@var{output}
+@item dd [--image-opts] [--share-rw] [-f @var{fmt}] [-O @var{output_fmt}] 
[bs=@var{block_size}] 

[Qemu-block] [PATCH v14 06/20] qemu-io: Add --share-rw option

[Fam Zheng]

Add --share-rw/-U to program options and -U to open subcommand.

Signed-off-by: Fam Zheng 
---
 qemu-io.c | 35 +++
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/qemu-io.c b/qemu-io.c
index 427cbae..c4d824f 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -20,6 +20,7 @@
 #include "qemu/readline.h"
 #include "qemu/log.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/qmp/qbool.h"
 #include "qom/object_interfaces.h"
 #include "sysemu/block-backend.h"
 #include "block/block_int.h"
@@ -53,7 +54,8 @@ static const cmdinfo_t close_cmd = {
 .oneline= "close the current open file",
 };
 
-static int openfile(char *name, int flags, bool writethrough, QDict *opts)
+static int openfile(char *name, int flags, bool writethrough, bool share_rw,
+QDict *opts)
 {
 Error *local_err = NULL;
 BlockDriverState *bs;
@@ -64,6 +66,12 @@ static int openfile(char *name, int flags, bool 
writethrough, QDict *opts)
 return 1;
 }
 
+if (share_rw) {
+if (!opts) {
+opts = qdict_new();
+}
+qdict_put(opts, BDRV_OPT_FORCE_SHARED_WRITE, qbool_from_bool(true));
+}
 qemuio_blk = blk_new_open(name, NULL, opts, flags, _err);
 if (!qemuio_blk) {
 error_reportf_err(local_err, "can't open%s%s: ",
@@ -108,6 +116,7 @@ static void open_help(void)
 " -r, -- open file read-only\n"
 " -s, -- use snapshot file\n"
 " -n, -- disable host cache, short for -t none\n"
+" -U, -- allow shared write, don't lock image\n"
 " -k, -- use kernel AIO implementation (on Linux only)\n"
 " -t, -- use the given cache mode for the image\n"
 " -d, -- use the given discard mode for the image\n"
@@ -124,7 +133,7 @@ static const cmdinfo_t open_cmd = {
 .argmin = 1,
 .argmax = -1,
 .flags  = CMD_NOFILE_OK,
-.args   = "[-rsnk] [-t cache] [-d discard] [-o options] [path]",
+.args   = "[-rsnkU] [-t cache] [-d discard] [-o options] [path]",
 .oneline= "open the file specified by path",
 .help   = open_help,
 };
@@ -147,8 +156,9 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
 int c;
 QemuOpts *qopts;
 QDict *opts;
+bool share_rw = false;
 
-while ((c = getopt(argc, argv, "snro:kt:d:")) != -1) {
+while ((c = getopt(argc, argv, "snro:kt:d:U")) != -1) {
 switch (c) {
 case 's':
 flags |= BDRV_O_SNAPSHOT;
@@ -188,6 +198,9 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
 return 0;
 }
 break;
+case 'U':
+share_rw = true;
+break;
 default:
 qemu_opts_reset(_opts);
 return qemuio_command_usage(_cmd);
@@ -211,9 +224,9 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
 qemu_opts_reset(_opts);
 
 if (optind == argc - 1) {
-return openfile(argv[optind], flags, writethrough, opts);
+return openfile(argv[optind], flags, writethrough, share_rw, opts);
 } else if (optind == argc) {
-return openfile(NULL, flags, writethrough, opts);
+return openfile(NULL, flags, writethrough, share_rw, opts);
 } else {
 QDECREF(opts);
 return qemuio_command_usage(_cmd);
@@ -257,6 +270,7 @@ static void usage(const char *name)
 "  -T, --trace [[enable=]][,events=][,file=]\n"
 "   specify tracing options\n"
 "   see qemu-img(1) man page for full description\n"
+"  -U, --share-rw   allow shared write\n"
 "  -h, --help   display this help and exit\n"
 "  -V, --versionoutput version information and exit\n"
 "\n"
@@ -436,7 +450,7 @@ static QemuOptsList file_opts = {
 int main(int argc, char **argv)
 {
 int readonly = 0;
-const char *sopt = "hVc:d:f:rsnmkt:T:";
+const char *sopt = "hVc:d:f:rsnmkt:T:U";
 const struct option lopt[] = {
 { "help", no_argument, NULL, 'h' },
 { "version", no_argument, NULL, 'V' },
@@ -452,6 +466,7 @@ int main(int argc, char **argv)
 { "trace", required_argument, NULL, 'T' },
 { "object", required_argument, NULL, OPTION_OBJECT },
 { "image-opts", no_argument, NULL, OPTION_IMAGE_OPTS },
+{ "share-rw", no_argument, 0, 'U'},
 { NULL, 0, NULL, 0 }
 };
 int c;
@@ -462,6 +477,7 @@ int main(int argc, char **argv)
 QDict *opts = NULL;
 const char *format = NULL;
 char *trace_file = NULL;
+bool share_rw = false;
 
 #ifdef CONFIG_POSIX
 signal(SIGPIPE, SIG_IGN);
@@ -524,6 +540,9 @@ int main(int argc, char **argv)
 case 'h':
 usage(progname);
 exit(0);
+case 'U':
+share_rw = true;
+break;
 case OPTION_OBJECT: {
 QemuOpts *qopts;
 qopts = qemu_opts_parse_noisily(_object_opts,
@@ -595,7 +614,7 @@ int main(int argc, char **argv)
 exit(1);
 }

[Qemu-block] [PATCH v14 04/20] qemu-img: Add --share-rw option to subcommands

[Fam Zheng]

Similar to share-rw qdev property, this will force the opened images to
allow shared write permission of other programs.

Signed-off-by: Fam Zheng 
---
 qemu-img.c | 155 +++--
 1 file changed, 119 insertions(+), 36 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index ed24371..df88a79 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -28,6 +28,7 @@
 #include "qapi/qobject-output-visitor.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qbool.h"
 #include "qemu/cutils.h"
 #include "qemu/config-file.h"
 #include "qemu/option.h"
@@ -283,12 +284,15 @@ static int img_open_password(BlockBackend *blk, const 
char *filename,
 
 static BlockBackend *img_open_opts(const char *optstr,
QemuOpts *opts, int flags, bool 
writethrough,
-   bool quiet)
+   bool quiet, bool share_rw)
 {
 QDict *options;
 Error *local_err = NULL;
 BlockBackend *blk;
 options = qemu_opts_to_qdict(opts, NULL);
+if (share_rw) {
+qdict_put(options, BDRV_OPT_FORCE_SHARED_WRITE, qbool_from_bool(true));
+}
 blk = blk_new_open(NULL, NULL, options, flags, _err);
 if (!blk) {
 error_reportf_err(local_err, "Could not open '%s': ", optstr);
@@ -305,17 +309,19 @@ static BlockBackend *img_open_opts(const char *optstr,
 
 static BlockBackend *img_open_file(const char *filename,
const char *fmt, int flags,
-   bool writethrough, bool quiet)
+   bool writethrough, bool quiet, bool 
share_rw)
 {
 BlockBackend *blk;
 Error *local_err = NULL;
-QDict *options = NULL;
+QDict *options = qdict_new();
 
 if (fmt) {
-options = qdict_new();
 qdict_put(options, "driver", qstring_from_str(fmt));
 }
 
+if (share_rw) {
+qdict_put(options, BDRV_OPT_FORCE_SHARED_WRITE, qbool_from_bool(true));
+}
 blk = blk_new_open(filename, NULL, options, flags, _err);
 if (!blk) {
 error_reportf_err(local_err, "Could not open '%s': ", filename);
@@ -334,7 +340,7 @@ static BlockBackend *img_open_file(const char *filename,
 static BlockBackend *img_open(bool image_opts,
   const char *filename,
   const char *fmt, int flags, bool writethrough,
-  bool quiet)
+  bool quiet, bool share_rw)
 {
 BlockBackend *blk;
 if (image_opts) {
@@ -348,9 +354,9 @@ static BlockBackend *img_open(bool image_opts,
 if (!opts) {
 return NULL;
 }
-blk = img_open_opts(filename, opts, flags, writethrough, quiet);
+blk = img_open_opts(filename, opts, flags, writethrough, quiet, 
share_rw);
 } else {
-blk = img_open_file(filename, fmt, flags, writethrough, quiet);
+blk = img_open_file(filename, fmt, flags, writethrough, quiet, 
share_rw);
 }
 return blk;
 }
@@ -650,6 +656,7 @@ static int img_check(int argc, char **argv)
 ImageCheck *check;
 bool quiet = false;
 bool image_opts = false;
+bool share_rw = false;
 
 fmt = NULL;
 output = NULL;
@@ -664,9 +671,10 @@ static int img_check(int argc, char **argv)
 {"output", required_argument, 0, OPTION_OUTPUT},
 {"object", required_argument, 0, OPTION_OBJECT},
 {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+{"share-rw", no_argument, 0, 'U'},
 {0, 0, 0, 0}
 };
-c = getopt_long(argc, argv, ":hf:r:T:q",
+c = getopt_long(argc, argv, ":hf:r:T:qU",
 long_options, _index);
 if (c == -1) {
 break;
@@ -705,6 +713,9 @@ static int img_check(int argc, char **argv)
 case 'q':
 quiet = true;
 break;
+case 'U':
+share_rw = true;
+break;
 case OPTION_OBJECT: {
 QemuOpts *opts;
 opts = qemu_opts_parse_noisily(_object_opts,
@@ -744,7 +755,8 @@ static int img_check(int argc, char **argv)
 return 1;
 }
 
-blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet);
+blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet,
+   share_rw);
 if (!blk) {
 return 1;
 }
@@ -864,6 +876,7 @@ static int img_commit(int argc, char **argv)
 CommonBlockJobCBInfo cbi;
 bool image_opts = false;
 AioContext *aio_context;
+bool share_rw = false;
 
 fmt = NULL;
 cache = BDRV_DEFAULT_CACHE;
@@ -873,9 +886,10 @@ static int img_commit(int argc, char **argv)
 {"help", no_argument, 0, 'h'},
 {"object", required_argument, 0, OPTION_OBJECT},
 {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+{"share-rw", no_argument, 0, 

[Qemu-block] [PATCH v14 03/20] block: Respect "force-shared-write" in perm propagating

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 block.c | 31 +++
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index f5c4e97..6acf618 100644
--- a/block.c
+++ b/block.c
@@ -1422,6 +1422,21 @@ static int bdrv_child_check_perm(BdrvChild *c, uint64_t 
perm, uint64_t shared,
 static void bdrv_child_abort_perm_update(BdrvChild *c);
 static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
 
+static void bdrv_child_perm(BlockDriverState *bs, BdrvChild *c,
+const BdrvChildRole *role,
+uint64_t parent_perm, uint64_t parent_shared,
+uint64_t *nperm, uint64_t *nshared)
+{
+if (bs->drv && bs->drv->bdrv_child_perm) {
+bs->drv->bdrv_child_perm(bs, c, role,
+ parent_perm, parent_shared,
+ nperm, nshared);
+}
+if (bs->force_shared_write) {
+*nshared |= BLK_PERM_WRITE;
+}
+}
+
 /*
  * Check whether permissions on this node can be changed in a way that
  * @cumulative_perms and @cumulative_shared_perms are the new cumulative
@@ -1466,9 +1481,9 @@ static int bdrv_check_perm(BlockDriverState *bs, uint64_t 
cumulative_perms,
 /* Check all children */
 QLIST_FOREACH(c, >children, next) {
 uint64_t cur_perm, cur_shared;
-drv->bdrv_child_perm(bs, c, c->role,
- cumulative_perms, cumulative_shared_perms,
- _perm, _shared);
+bdrv_child_perm(bs, c, c->role,
+cumulative_perms, cumulative_shared_perms,
+_perm, _shared);
 ret = bdrv_child_check_perm(c, cur_perm, cur_shared, ignore_children,
 errp);
 if (ret < 0) {
@@ -1528,9 +1543,9 @@ static void bdrv_set_perm(BlockDriverState *bs, uint64_t 
cumulative_perms,
 /* Update all children */
 QLIST_FOREACH(c, >children, next) {
 uint64_t cur_perm, cur_shared;
-drv->bdrv_child_perm(bs, c, c->role,
- cumulative_perms, cumulative_shared_perms,
- _perm, _shared);
+bdrv_child_perm(bs, c, c->role,
+cumulative_perms, cumulative_shared_perms,
+_perm, _shared);
 bdrv_child_set_perm(c, cur_perm, cur_shared);
 }
 }
@@ -1865,8 +1880,8 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
 
 assert(parent_bs->drv);
 assert(bdrv_get_aio_context(parent_bs) == bdrv_get_aio_context(child_bs));
-parent_bs->drv->bdrv_child_perm(parent_bs, NULL, child_role,
-perm, shared_perm, , _perm);
+bdrv_child_perm(parent_bs, NULL, child_role,
+perm, shared_perm, , _perm);
 
 child = bdrv_root_attach_child(child_bs, child_name, child_role,
perm, shared_perm, parent_bs, errp);
-- 
2.9.3

[Qemu-block] [PATCH v14 02/20] qapi: Add 'force-shared-write' to blockdev-add arguments

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 qapi/block-core.json | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 033457c..b9b8002 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2880,6 +2880,8 @@
 # (default: false)
 # @detect-zeroes: detect and optimize zero writes (Since 2.1)
 # (default: off)
+# @force-shared-write: enforce shared write permission on added nodes
+#  (Since 2.10)
 #
 # Remaining options are determined by the block driver.
 #
@@ -2891,6 +2893,7 @@
 '*discard': 'BlockdevDiscardOptions',
 '*cache': 'BlockdevCacheOptions',
 '*read-only': 'bool',
+'*force-shared-write': 'bool',
 '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
   'discriminator': 'driver',
   'data': {
-- 
2.9.3

[Qemu-block] [PATCH v14 01/20] block: Add, parse and store "force-shared-write" option

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 block.c   | 9 +
 include/block/block.h | 2 ++
 include/block/block_int.h | 1 +
 3 files changed, 12 insertions(+)

diff --git a/block.c b/block.c
index 1e668fb..f5c4e97 100644
--- a/block.c
+++ b/block.c
@@ -763,6 +763,7 @@ static void bdrv_inherited_options(int *child_flags, QDict 
*child_options,
  * the parent. */
 qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_DIRECT);
 qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_NO_FLUSH);
+qdict_copy_default(child_options, parent_options, 
BDRV_OPT_FORCE_SHARED_WRITE);
 
 /* Inherit the read-only option from the parent if it's not set */
 qdict_copy_default(child_options, parent_options, BDRV_OPT_READ_ONLY);
@@ -871,6 +872,7 @@ static void bdrv_backing_options(int *child_flags, QDict 
*child_options,
  * which is only applied on the top level (BlockBackend) */
 qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_DIRECT);
 qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_NO_FLUSH);
+qdict_copy_default(child_options, parent_options, 
BDRV_OPT_FORCE_SHARED_WRITE);
 
 /* backing files always opened read-only */
 qdict_set_default_str(child_options, BDRV_OPT_READ_ONLY, "on");
@@ -1115,6 +1117,11 @@ QemuOptsList bdrv_runtime_opts = {
 .type = QEMU_OPT_STRING,
 .help = "discard operation (ignore/off, unmap/on)",
 },
+{
+.name = BDRV_OPT_FORCE_SHARED_WRITE,
+.type = QEMU_OPT_BOOL,
+.help = "always accept other writers (default: off)",
+},
 { /* end of list */ }
 },
 };
@@ -1154,6 +1161,8 @@ static int bdrv_open_common(BlockDriverState *bs, 
BlockBackend *file,
 drv = bdrv_find_format(driver_name);
 assert(drv != NULL);
 
+bs->force_shared_write = qemu_opt_get_bool(opts, 
BDRV_OPT_FORCE_SHARED_WRITE, false);
+
 if (file != NULL) {
 filename = blk_bs(file)->filename;
 } else {
diff --git a/include/block/block.h b/include/block/block.h
index 5ddc0cf..d3afb75 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -109,6 +109,8 @@ typedef struct HDGeometry {
 #define BDRV_OPT_CACHE_NO_FLUSH "cache.no-flush"
 #define BDRV_OPT_READ_ONLY  "read-only"
 #define BDRV_OPT_DISCARD"discard"
+#define BDRV_OPT_FORCE_SHARED_WRITE \
+"force-shared-write"
 
 
 #define BDRV_SECTOR_BITS   9
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 59400bd..fb81692 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -518,6 +518,7 @@ struct BlockDriverState {
 bool valid_key; /* if true, a valid encryption key has been set */
 bool sg;/* if true, the device is a /dev/sg* */
 bool probed;/* if true, format was probed rather than specified */
+bool force_shared_write; /* if true, always allow shared write perm. */
 
 BlockDriver *drv; /* NULL means no media */
 void *opaque;
-- 
2.9.3

[Qemu-block] [PATCH v14 00/20] block: Image locking series

[Fam Zheng]

v14: - Replace BDRV_ flag with the "force-shared-write" block option. [Kevin]
 - Add bs->force_shared_write.
 - Update test case accordingly, back to bash again. :)
 - A few fixes in the locking code spotted by patchew and the new test,
   though the long line error is still there for readability.
 - Replace the workaround to drive-backup with a real fix in patch 17.

v13: - Address Max's comments.
 - Add reviewed-by from Max and Eric.
 - Rebase for 2.10:
   * Use op blocker API
   * Add --unsafe-read for qemu-img and qemu-io

Fam Zheng (20):
  block: Add, parse and store "force-shared-write" option
  qapi: Add 'force-shared-write' to blockdev-add arguments
  block: Respect "force-shared-write" in perm propagating
  qemu-img: Add --share-rw option to subcommands
  qemu-img: Update documentation for --share-rw
  qemu-io: Add --share-rw option
  iotests: 030: Prepare for image locking
  iotests: 046: Prepare for image locking
  iotests: 055: Don't attach the target image already for drive-backup
  iotests: 085: Avoid image locking conflict
  iotests: 087: Don't attach test image twice
  iotests: 091: Quit QEMU before checking image
  iotests: 172: Use separate images for multiple devices
  tests: Use null-co:// instead of /dev/null as the dummy image
  file-posix: Add 'locking' option
  tests: Disable image lock in test-replication
  block: Reuse bs as backing hd for drive-backup sync=none
  osdep: Add qemu_lock_fd and qemu_unlock_fd
  file-posix: Add image locking in perm operations
  qemu-iotests: Add test case 153 for image locking

 block.c|  40 ++-
 block/file-posix.c | 744 -
 blockdev.c |  15 +-
 include/block/block.h  |   2 +
 include/block/block_int.h  |   1 +
 include/qemu/osdep.h   |   3 +
 qapi/block-core.json   |   3 +
 qemu-img-cmds.hx   |  48 +--
 qemu-img.c | 155 +++---
 qemu-io.c  |  35 ++-
 tests/drive_del-test.c |   2 +-
 tests/nvme-test.c  |   2 +-
 tests/qemu-iotests/030 |  24 +-
 tests/qemu-iotests/046 |   2 +-
 tests/qemu-iotests/055 |  32 +-
 tests/qemu-iotests/085 |  34 ++-
 tests/qemu-iotests/085.out |   3 +-
 tests/qemu-iotests/087 |   6 +-
 tests/qemu-iotests/091 |   2 +
 tests/qemu-iotests/153 | 219 +
 tests/qemu-iotests/153.out | 627 ++
 tests/qemu-iotests/172 |  55 ++--
 tests/qemu-iotests/172.out |  50 +--
 tests/qemu-iotests/group   |   1 +
 tests/test-replication.c   |   9 +-
 tests/usb-hcd-uhci-test.c  |   2 +-
 tests/usb-hcd-xhci-test.c  |   2 +-
 tests/virtio-blk-test.c|   2 +-
 tests/virtio-scsi-test.c   |   4 +-
 util/osdep.c   |  48 +++
 30 files changed, 1988 insertions(+), 184 deletions(-)
 create mode 100755 tests/qemu-iotests/153
 create mode 100644 tests/qemu-iotests/153.out

-- 
2.9.3

Re: [Qemu-block] [PATCH] qemu-img: use blk_co_pwrite_zeroes for zero sectors when compressed

[858585 jemmy]

On Thu, Apr 20, 2017 at 6:00 PM, Kevin Wolf  wrote:
> Am 20.04.2017 um 10:38 hat jemmy858...@gmail.com geschrieben:
>> From: Lidong Chen 
>>
>> when the buffer is zero, blk_co_pwrite_zeroes is more effectively than
>> blk_co_pwritev with BDRV_REQ_WRITE_COMPRESSED. this patch can reduces
>> the time when converts the qcow2 image with lots of zero.
>>
>> Signed-off-by: Lidong Chen 
>
> Good catch, using blk_co_pwrite_zeroes() makes sense even for compressed
> images.
>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index b220cf7..0256539 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1675,13 +1675,20 @@ static int coroutine_fn 
>> convert_co_write(ImgConvertState *s, int64_t sector_num,
>>   * write if the buffer is completely zeroed and we're allowed to
>>   * keep the target sparse. */
>>  if (s->compressed) {
>> -if (s->has_zero_init && s->min_sparse &&
>> -buffer_is_zero(buf, n * BDRV_SECTOR_SIZE))
>> -{
>> -assert(!s->target_has_backing);
>> -break;
>> +if (buffer_is_zero(buf, n * BDRV_SECTOR_SIZE)) {
>> +if (s->has_zero_init && s->min_sparse) {
>> +assert(!s->target_has_backing);
>> +break;
>> +} else {
>> +ret = blk_co_pwrite_zeroes(s->target,
>> +   sector_num << BDRV_SECTOR_BITS,
>> +   n << BDRV_SECTOR_BITS, 0);
>> +if (ret < 0) {
>> +return ret;
>> +}
>> +break;
>> +}
>>  }
>
> If s->min_sparse == 0, we may neither skip the write not use
> blk_co_pwrite_zeroes(), because this requires actual full allocation
> with explicit zero sectors.
>
> Of course, if you fix this, what you end up with here is a duplicate of
> the code path for non-compressed images. The remaining difference seems
> to be the BDRV_REQ_WRITE_COMPRESSED flag and buffer_is_zero() vs.
> is_allocated_sectors_min() (because uncompressed clusters can be written
> partially, but compressed clusters can't).

I have a try to unify the code.

I don't understand why use  is_allocated_sectors_min when don't compressed.
the s->min_sparse is 8 default, which is smaller than cluster_sectors.

if a cluster which data is  8 sector zero and 8 sector non-zero
repeated, it will call
blk_co_pwritev and blk_co_pwrite_zeroes many times for a cluster.

why not compare the zero by cluster_sectors size?

>
> So I suppose that instead of just fixing the above bug, we could actually
> mostly unify the two code paths, if you want to have a try at it.
>
> Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-iotests: Remove PERL_PROG and BC_PROG

[Fam Zheng]

On Thu, 04/20 22:15, Kevin Wolf wrote:
> We test for the presence of perl and bc and save their path in the
> variables PERL_PROG and BC_PROG, but never actually make use of them.
> Remove the checks and assignments so qemu-iotests can run even when
> bc isn't installed.
> 
> Reported-by: Yash Mankad 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/common.config | 6 --
>  1 file changed, 6 deletions(-)
> 
> diff --git a/tests/qemu-iotests/common.config 
> b/tests/qemu-iotests/common.config
> index 55527aa..1222e43 100644
> --- a/tests/qemu-iotests/common.config
> +++ b/tests/qemu-iotests/common.config
> @@ -75,18 +75,12 @@ _fatal()
>  exit 1
>  }
>  
> -export PERL_PROG="`set_prog_path perl`"
> -[ "$PERL_PROG" = "" ] && _fatal "perl not found"
> -

Then we should drop _readlink which is unused and uses perl too. Whether in this
patch together or in a separate patch doesn't matter much, so:

>  export AWK_PROG="`set_prog_path awk`"
>  [ "$AWK_PROG" = "" ] && _fatal "awk not found"
>  
>  export SED_PROG="`set_prog_path sed`"
>  [ "$SED_PROG" = "" ] && _fatal "sed not found"
>  
> -export BC_PROG="`set_prog_path bc`"
> -[ "$BC_PROG" = "" ] && _fatal "bc not found"
> -

The bc part is nice!

>  export PS_ALL_FLAGS="-ef"
>  
>  if [ -z "$QEMU_PROG" ]; then
> -- 
> 1.8.3.1
> 
> 

Reviewed-by: Fam Zheng 

Re: [Qemu-block] [Qemu-devel] [PATCH] sheepdog: Set error when connection fails

[Fam Zheng]

On Thu, 04/20 22:32, Kevin Wolf wrote:
> Am 20.04.2017 um 17:30 hat Daniel P. Berrange geschrieben:
> > On Thu, Apr 20, 2017 at 12:00:03PM +0800, Fam Zheng wrote:
> > > Signed-off-by: Fam Zheng 
> > > ---
> > >  block/sheepdog.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > > index fb9203e..7e889ee 100644
> > > --- a/block/sheepdog.c
> > > +++ b/block/sheepdog.c
> > > @@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, 
> > > Error **errp)
> > >  qemu_set_nonblock(fd);
> > >  } else {
> > >  fd = -EIO;
> > > +error_setg(errp, "Failed to connect to sheepdog server");
> > >  }
> > 
> > This doesn't make much sense to me. The lines just above the
> > diff context have this:
> > 
> > fd = socket_connect(s->addr, errp, NULL, NULL);

Oops! :(

> > 
> > socket_connect should have already reported an error on "errp"
> > in the scenario that 'fd == -1'.
> 
> By the way, am I the only one who thinks that having errp anywhere else
> than as the last argument is bad style? I can easily see myself missing
> that this functions sets it because the last argument is NULL.

Hmm, exactly.. Socket code does this here and there, and it's hard to read.

Fam

Re: [Qemu-block] [Qemu-devel] [PATCH] sheepdog: Set error when connection fails

[Kevin Wolf]

Am 20.04.2017 um 17:30 hat Daniel P. Berrange geschrieben:
> On Thu, Apr 20, 2017 at 12:00:03PM +0800, Fam Zheng wrote:
> > Signed-off-by: Fam Zheng 
> > ---
> >  block/sheepdog.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > index fb9203e..7e889ee 100644
> > --- a/block/sheepdog.c
> > +++ b/block/sheepdog.c
> > @@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error 
> > **errp)
> >  qemu_set_nonblock(fd);
> >  } else {
> >  fd = -EIO;
> > +error_setg(errp, "Failed to connect to sheepdog server");
> >  }
> 
> This doesn't make much sense to me. The lines just above the
> diff context have this:
> 
> fd = socket_connect(s->addr, errp, NULL, NULL);
> 
> socket_connect should have already reported an error on "errp"
> in the scenario that 'fd == -1'.

By the way, am I the only one who thinks that having errp anywhere else
than as the last argument is bad style? I can easily see myself missing
that this functions sets it because the last argument is NULL.

Kevin

Re: [Qemu-block] [PATCH] qemu-iotests: Remove PERL_PROG and BC_PROG

[Eric Blake]

On 04/20/2017 03:15 PM, Kevin Wolf wrote:
> We test for the presence of perl and bc and save their path in the
> variables PERL_PROG and BC_PROG, but never actually make use of them.
> Remove the checks and assignments so qemu-iotests can run even when
> bc isn't installed.
> 
> Reported-by: Yash Mankad 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/common.config | 6 --
>  1 file changed, 6 deletions(-)

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH] qemu-iotests: Remove PERL_PROG and BC_PROG

[Kevin Wolf]

We test for the presence of perl and bc and save their path in the
variables PERL_PROG and BC_PROG, but never actually make use of them.
Remove the checks and assignments so qemu-iotests can run even when
bc isn't installed.

Reported-by: Yash Mankad 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/common.config | 6 --
 1 file changed, 6 deletions(-)

diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index 55527aa..1222e43 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -75,18 +75,12 @@ _fatal()
 exit 1
 }
 
-export PERL_PROG="`set_prog_path perl`"
-[ "$PERL_PROG" = "" ] && _fatal "perl not found"
-
 export AWK_PROG="`set_prog_path awk`"
 [ "$AWK_PROG" = "" ] && _fatal "awk not found"
 
 export SED_PROG="`set_prog_path sed`"
 [ "$SED_PROG" = "" ] && _fatal "sed not found"
 
-export BC_PROG="`set_prog_path bc`"
-[ "$BC_PROG" = "" ] && _fatal "bc not found"
-
 export PS_ALL_FLAGS="-ef"
 
 if [ -z "$QEMU_PROG" ]; then
-- 
1.8.3.1

Re: [Qemu-block] [PATCH] qemu-img: simplify img_convert

[Kevin Wolf]

Am 20.04.2017 um 16:08 hat Peter Lieven geschrieben:
> Am 20.04.2017 um 16:05 schrieb Fam Zheng:
> >On Tue, 02/28 14:35, Peter Lieven wrote:
> >>img_convert has been around before there was an ImgConvertState or
> >>a block backend, but it has never been modified to directly use
> >>these structs. Change this by parsing parameters directly into
> >>the ImgConvertState and directly use BlockBackend where possible.
> >>Futhermore variable initalization has been reworked and sorted.
> >>
> >>Signed-off-by: Peter Lieven 
> >I see an iotest failure with this patch, in Kevin's block-next tree:
> >
> >019 1s ... - output mismatch (see 019.out.bad)
> >--- /stor/work/qemu/tests/qemu-iotests/019.out   2017-04-17 
> >16:19:56.523968474 +0800
> >+++ 019.out.bad  2017-04-20 22:03:29.868216955 +0800
> >@@ -1086,8 +1086,8 @@
> >  Checking if backing clusters are allocated when they shouldn't
> >-0/128 sectors allocated at offset 1 MiB
> >-0/128 sectors allocated at offset 4.001 GiB
> >+128/128 sectors allocated at offset 1 MiB
> >+128/128 sectors allocated at offset 4.001 GiB
> >  Reading
> >  === IO: pattern 42
> >Failures: 019
> >Failed 1 of 1 tests
> 
> Thanks for notifying, Fam.
> 
> @Kevin: Can you drop the patch again? I will send a V2 addressing this
> issue and the other comments I have got.

I could, but if it doesn't take too long, I'd prefer to only do so when
v2 is there. There are already a few conflicting patches and I asked
their authors to rebase onto yours, so it wouldn't be nice if they
wouldn't apply again because I removed your patch...

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH] sheepdog: Set error when connection fails

[Jeff Cody]

On Thu, Apr 20, 2017 at 04:30:16PM +0100, Daniel P. Berrange wrote:
> On Thu, Apr 20, 2017 at 12:00:03PM +0800, Fam Zheng wrote:
> > Signed-off-by: Fam Zheng 
> > ---
> >  block/sheepdog.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > index fb9203e..7e889ee 100644
> > --- a/block/sheepdog.c
> > +++ b/block/sheepdog.c
> > @@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error 
> > **errp)
> >  qemu_set_nonblock(fd);
> >  } else {
> >  fd = -EIO;
> > +error_setg(errp, "Failed to connect to sheepdog server");
> >  }
> 
> This doesn't make much sense to me. The lines just above the
> diff context have this:
> 
> fd = socket_connect(s->addr, errp, NULL, NULL);
> 
> socket_connect should have already reported an error on "errp"
> in the scenario that 'fd == -1'. So AFAICT the new error_setg is
> just throwing away the real detailed error message in favour of
> a generic message.
> 
> So I'm puzzelled why we need to change anything - error reporting
> should already be working fine.
> 

Indeed, you are right. (Dequeuing patch)

It would also make more sense to check fd after the socket_connect() call
and return error then, rather than keep checking fd throughout the rest of
the function.

-Jeff

Re: [Qemu-block] [Qemu-devel] [PATCH] sheepdog: Set error when connection fails

[Daniel P. Berrange]

On Thu, Apr 20, 2017 at 12:00:03PM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng 
> ---
>  block/sheepdog.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index fb9203e..7e889ee 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error 
> **errp)
>  qemu_set_nonblock(fd);
>  } else {
>  fd = -EIO;
> +error_setg(errp, "Failed to connect to sheepdog server");
>  }

This doesn't make much sense to me. The lines just above the
diff context have this:

fd = socket_connect(s->addr, errp, NULL, NULL);

socket_connect should have already reported an error on "errp"
in the scenario that 'fd == -1'. So AFAICT the new error_setg is
just throwing away the real detailed error message in favour of
a generic message.

So I'm puzzelled why we need to change anything - error reporting
should already be working fine.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-block] [PATCH] sheepdog: Set error when connection fails

[Jeff Cody]

On Thu, Apr 20, 2017 at 12:00:03PM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng 
> ---
>  block/sheepdog.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index fb9203e..7e889ee 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error 
> **errp)
>  qemu_set_nonblock(fd);
>  } else {
>  fd = -EIO;
> +error_setg(errp, "Failed to connect to sheepdog server");
>  }

A bit odd that we don't just return right away in this function after a
failed called to socket_connect(), but that has nothing to do with your
patch.

Reviewed-by: Jeff Cody 


>  
>  return fd;
> -- 
> 2.9.3
> 

Re: [Qemu-block] [PATCH v3 for-2.10] qemu-iotests: _cleanup_qemu must be called on exit

[Jeff Cody]

On Tue, Apr 18, 2017 at 03:42:41PM -0400, Jeff Cody wrote:
> For the tests that use the common.qemu functions for running a QEMU
> process, _cleanup_qemu must be called in the exit function.
> 
> If it is not, if the qemu process aborts, then not all of the droppings
> are cleaned up (e.g. pidfile, fifos).
> 
> This updates those tests that did not have a cleanup in qemu-iotests.
> 
> (I swapped spaces for tabs in test 102 as well)
> 
> Reported-by: Eric Blake 
> Reviewed-by: Eric Blake 
> Signed-off-by: Jeff Cody 
> ---
> 
> v3 added test 156
> v2 fixed typo in test 094, s/cleanup/_cleanup/ for the trap function. (Eric)
> 
>  tests/qemu-iotests/028 |  1 +
>  tests/qemu-iotests/094 | 11 ---
>  tests/qemu-iotests/102 |  5 +++--
>  tests/qemu-iotests/109 |  1 +
>  tests/qemu-iotests/117 |  1 +
>  tests/qemu-iotests/130 |  1 +
>  tests/qemu-iotests/140 |  1 +
>  tests/qemu-iotests/141 |  1 +
>  tests/qemu-iotests/143 |  1 +
>  tests/qemu-iotests/156 |  1 +
>  10 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/qemu-iotests/028 b/tests/qemu-iotests/028
> index 7783e57..97a8869 100755
> --- a/tests/qemu-iotests/028
> +++ b/tests/qemu-iotests/028
> @@ -32,6 +32,7 @@ status=1# failure is the default!
>  
>  _cleanup()
>  {
> +_cleanup_qemu
>  rm -f "${TEST_IMG}.copy"
>  _cleanup_test_img
>  }
> diff --git a/tests/qemu-iotests/094 b/tests/qemu-iotests/094
> index 0ba0b0c..9aa01e3 100755
> --- a/tests/qemu-iotests/094
> +++ b/tests/qemu-iotests/094
> @@ -27,7 +27,14 @@ echo "QA output created by $seq"
>  here="$PWD"
>  status=1 # failure is the default!
>  
> -trap "exit \$status" 0 1 2 3 15
> +_cleanup()
> +{
> +_cleanup_qemu
> +_cleanup_test_img
> +rm -f "$TEST_DIR/source.$IMGFMT"
> +}
> +
> +trap "_cleanup; exit \$status" 0 1 2 3 15
>  
>  # get standard environment, filters and checks
>  . ./common.rc
> @@ -73,8 +80,6 @@ _send_qemu_cmd $QEMU_HANDLE \
>  
>  wait=1 _cleanup_qemu
>  
> -_cleanup_test_img
> -rm -f "$TEST_DIR/source.$IMGFMT"
>  
>  # success, all done
>  echo '*** done'
> diff --git a/tests/qemu-iotests/102 b/tests/qemu-iotests/102
> index 64b4af9..87db1bb 100755
> --- a/tests/qemu-iotests/102
> +++ b/tests/qemu-iotests/102
> @@ -25,11 +25,12 @@ seq=$(basename $0)
>  echo "QA output created by $seq"
>  
>  here=$PWD
> -status=1 # failure is the default!
> +status=1# failure is the default!
>  
>  _cleanup()
>  {
> - _cleanup_test_img
> +_cleanup_qemu
> +_cleanup_test_img
>  }
>  trap "_cleanup; exit \$status" 0 1 2 3 15
>  
> diff --git a/tests/qemu-iotests/109 b/tests/qemu-iotests/109
> index 927151a..6161633 100755
> --- a/tests/qemu-iotests/109
> +++ b/tests/qemu-iotests/109
> @@ -29,6 +29,7 @@ status=1# failure is the default!
>  
>  _cleanup()
>  {
> +_cleanup_qemu
>  rm -f $TEST_IMG.src
>   _cleanup_test_img
>  }
> diff --git a/tests/qemu-iotests/117 b/tests/qemu-iotests/117
> index e955d52..6c83461 100755
> --- a/tests/qemu-iotests/117
> +++ b/tests/qemu-iotests/117
> @@ -29,6 +29,7 @@ status=1# failure is the default!
>  
>  _cleanup()
>  {
> +_cleanup_qemu
>   _cleanup_test_img
>  }
>  trap "_cleanup; exit \$status" 0 1 2 3 15
> diff --git a/tests/qemu-iotests/130 b/tests/qemu-iotests/130
> index ecc8a5b..d4d148b 100755
> --- a/tests/qemu-iotests/130
> +++ b/tests/qemu-iotests/130
> @@ -31,6 +31,7 @@ status=1# failure is the default!
>  
>  _cleanup()
>  {
> +_cleanup_qemu
>  _cleanup_test_img
>  }
>  trap "_cleanup; exit \$status" 0 1 2 3 15
> diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140
> index 49f9df4..8c80a5a 100755
> --- a/tests/qemu-iotests/140
> +++ b/tests/qemu-iotests/140
> @@ -33,6 +33,7 @@ status=1# failure is the default!
>  
>  _cleanup()
>  {
> +_cleanup_qemu
>  _cleanup_test_img
>  rm -f "$TEST_DIR/nbd"
>  }
> diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141
> index 27fb1cc..40a3405 100755
> --- a/tests/qemu-iotests/141
> +++ b/tests/qemu-iotests/141
> @@ -29,6 +29,7 @@ status=1# failure is the default!
>  
>  _cleanup()
>  {
> +_cleanup_qemu
>  _cleanup_test_img
>  rm -f "$TEST_DIR/{b,m,o}.$IMGFMT"
>  }
> diff --git a/tests/qemu-iotests/143 b/tests/qemu-iotests/143
> index ec4ef22..5ff1944 100755
> --- a/tests/qemu-iotests/143
> +++ b/tests/qemu-iotests/143
> @@ -29,6 +29,7 @@ status=1# failure is the default!
>  
>  _cleanup()
>  {
> +_cleanup_qemu
>  rm -f "$TEST_DIR/nbd"
>  }
>  trap "_cleanup; exit \$status" 0 1 2 3 15
> diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156
> index cc95ff1..2c9d23b 100755
> --- a/tests/qemu-iotests/156
> +++ b/tests/qemu-iotests/156
> @@ -37,6 +37,7 @@ status=1# failure is the default!
>  
>  _cleanup()
>  {
> +_cleanup_qemu
>  rm -f "$TEST_IMG{,.target}{,.backing,.overlay}"
>  }
>  trap "_cleanup; exit \$status" 0 1 2 3 15
> -- 
> 2.9.3
> 

Thanks,

Applied to my block 

Re: [Qemu-block] [PATCH v2 for-2.10 0/8] RBD reopen, read_only cleanup

[Jeff Cody]

On Fri, Apr 07, 2017 at 04:55:24PM -0400, Jeff Cody wrote:
> 
> Changes from v1:
> 
> Patch 2: Has v1 patch 8 (do not blindly xset bs->read_only) squashed into it
>  (thanks Stefan)
>  COW -> "copy-on-read" (Thanks John)
>  Drop unneeded call in vvfat, and bypass enable_write_target (Stefan)
> 
> Patch 5: Rename bdrv_try_... to bdrv_can_set_read_only() (Thanks John, Stefan)
> 
> Patch 6: Use "reopen_state->flags" not "reopen_state->bs->open_flags"
>  (Thanks John)
> 
> 
> 
> This series does two things:
> 
> 1.) Cleans up some of the logic behind setting the read_only flag
> for a BDS in the block layer, so that it is done consistently
> (and rules are applied consistently), and
> 
> 2.) Adds .bdrv_reopen_prepare() implementation for RBD, so that block
> jobs can be run on backing chains that have rbd protocol nodes.
> 
> Jeff Cody (8):
>   block: add bdrv_set_read_only() helper function
>   block: do not set BDS read_only if copy_on_read enabled
>   block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only
>   block: code movement
>   block: introduce bdrv_can_set_read_only()
>   block: use bdrv_can_set_read_only() during reopen
>   block/rbd - update variable names to more apt names
>   block/rbd: Add support for reopen()
> 
>  block.c   | 56 +++-
>  block/bochs.c |  5 +++-
>  block/cloop.c |  5 +++-
>  block/dmg.c   |  6 -
>  block/rbd.c   | 65 
> +--
>  block/vvfat.c | 19 +++
>  include/block/block.h |  2 ++
>  7 files changed, 123 insertions(+), 35 deletions(-)
> 
> -- 
> 2.9.3
> 

Thanks,

Applied to my block branch:

git://github.com/codyprime/qemu-kvm-jtc block

-Jeff

Re: [Qemu-block] [Qemu-devel] [PATCH 16/17] block: protect modification of dirty bitmaps with a mutex

[Eric Blake]

On 04/20/2017 07:00 AM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini 
> ---
>  block/dirty-bitmap.c | 74 
> +---
>  block/mirror.c   | 11 +--
>  include/block/block_int.h|  4 +--
>  include/block/dirty-bitmap.h | 23 +++---
>  4 files changed, 97 insertions(+), 15 deletions(-)

Fun conflicts with my pending patches to switch dirty-bitmap to be
byte-based instead of sector-based. I doubt the computer will be able to
resolve nicely, but I think rebasing by hand should be pretty doable, no
matter which of our patches lands first.

https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg02163.html

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-img: simplify img_convert

[Peter Lieven]

Am 20.04.2017 um 16:18 schrieb Eric Blake:

On 04/20/2017 09:11 AM, Eric Blake wrote:

On 04/20/2017 09:05 AM, Fam Zheng wrote:

On Tue, 02/28 14:35, Peter Lieven wrote:

img_convert has been around before there was an ImgConvertState or
a block backend, but it has never been modified to directly use
these structs. Change this by parsing parameters directly into
the ImgConvertState and directly use BlockBackend where possible.
Futhermore variable initalization has been reworked and sorted.

Signed-off-by: Peter Lieven 

I see an iotest failure with this patch, in Kevin's block-next tree:

019 1s ... - output mismatch (see 019.out.bad)
--- /stor/work/qemu/tests/qemu-iotests/019.out  2017-04-17 16:19:56.523968474 
+0800
+++ 019.out.bad 2017-04-20 22:03:29.868216955 +0800
@@ -1086,8 +1086,8 @@
  
  Checking if backing clusters are allocated when they shouldn't
  
-0/128 sectors allocated at offset 1 MiB

-0/128 sectors allocated at offset 4.001 GiB
+128/128 sectors allocated at offset 1 MiB
+128/128 sectors allocated at offset 4.001 GiB

Hmm - I wonder if my patch to make 'zero with unmap' on an image with no
backing file prefer pure unallocated clusters over a reads-as-zero would
make a difference.
https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg01728.html

Testing now...

Nope, did not make a difference, although it's certainly related; per my
comment in that thread that:

 Note that technically, we _could_ write a cluster as unallocated
 rather than zero if a backing file exists but the backing file
 also reads as zero, but that's more expensive to determine, so
 this optimization is limited to qcow2 without a backing file.




I will look into this tomorrow.


Peter

--

Mit freundlichen Grüßen

Peter Lieven

...

  KAMP Netzwerkdienste GmbH
  Vestische Str. 89-91 | 46117 Oberhausen
  Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40
  p...@kamp.de | http://www.kamp.de

  Geschäftsführer: Heiner Lante | Michael Lante
  Amtsgericht Duisburg | HRB Nr. 12154
  USt-Id-Nr.: DE 120607556

...

Re: [Qemu-block] [Qemu-devel] [PATCH v2 3/3] qemu-iotests: Test postcopy migration

[Daniel P. Berrange]

On Thu, Apr 20, 2017 at 10:18:27PM +0800, Fam Zheng wrote:
> On Wed, 04/19 17:16, Kevin Wolf wrote:
> > +_supported_fmt generic
> 
> This doesn't work for formats that install migration blockers, so maybe
> s/generic/raw qcow2/ instead?

It would be nice to use a more feature driven check and centralize
knowledge of which formats are migratable. eg define a
"_migratable_fmt" function in common.rc that checks a MIGRATABLE_FMT
env variable, and in 'common' where we process the command line args,
set MIGRATABLE_FMT for each format or protocol as needed.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-img: simplify img_convert

[Eric Blake]

On 04/20/2017 09:11 AM, Eric Blake wrote:
> On 04/20/2017 09:05 AM, Fam Zheng wrote:
>> On Tue, 02/28 14:35, Peter Lieven wrote:
>>> img_convert has been around before there was an ImgConvertState or
>>> a block backend, but it has never been modified to directly use
>>> these structs. Change this by parsing parameters directly into
>>> the ImgConvertState and directly use BlockBackend where possible.
>>> Futhermore variable initalization has been reworked and sorted.
>>>
>>> Signed-off-by: Peter Lieven 
>>
>> I see an iotest failure with this patch, in Kevin's block-next tree:
>>
>> 019 1s ... - output mismatch (see 019.out.bad)
>> --- /stor/work/qemu/tests/qemu-iotests/019.out   2017-04-17 
>> 16:19:56.523968474 +0800
>> +++ 019.out.bad  2017-04-20 22:03:29.868216955 +0800
>> @@ -1086,8 +1086,8 @@
>>  
>>  Checking if backing clusters are allocated when they shouldn't
>>  
>> -0/128 sectors allocated at offset 1 MiB
>> -0/128 sectors allocated at offset 4.001 GiB
>> +128/128 sectors allocated at offset 1 MiB
>> +128/128 sectors allocated at offset 4.001 GiB
> 
> Hmm - I wonder if my patch to make 'zero with unmap' on an image with no
> backing file prefer pure unallocated clusters over a reads-as-zero would
> make a difference.
> https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg01728.html
> 
> Testing now...

Nope, did not make a difference, although it's certainly related; per my
comment in that thread that:

Note that technically, we _could_ write a cluster as unallocated
rather than zero if a backing file exists but the backing file
also reads as zero, but that's more expensive to determine, so
this optimization is limited to qcow2 without a backing file.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v2 3/3] qemu-iotests: Test postcopy migration

[Fam Zheng]

On Wed, 04/19 17:16, Kevin Wolf wrote:
> +_supported_fmt generic

This doesn't work for formats that install migration blockers, so maybe
s/generic/raw qcow2/ instead?

Fam

Re: [Qemu-block] [PATCH] qemu-img: simplify img_convert

[Eric Blake]

On 04/20/2017 09:05 AM, Fam Zheng wrote:
> On Tue, 02/28 14:35, Peter Lieven wrote:
>> img_convert has been around before there was an ImgConvertState or
>> a block backend, but it has never been modified to directly use
>> these structs. Change this by parsing parameters directly into
>> the ImgConvertState and directly use BlockBackend where possible.
>> Futhermore variable initalization has been reworked and sorted.
>>
>> Signed-off-by: Peter Lieven 
> 
> I see an iotest failure with this patch, in Kevin's block-next tree:
> 
> 019 1s ... - output mismatch (see 019.out.bad)
> --- /stor/work/qemu/tests/qemu-iotests/019.out2017-04-17 
> 16:19:56.523968474 +0800
> +++ 019.out.bad   2017-04-20 22:03:29.868216955 +0800
> @@ -1086,8 +1086,8 @@
>  
>  Checking if backing clusters are allocated when they shouldn't
>  
> -0/128 sectors allocated at offset 1 MiB
> -0/128 sectors allocated at offset 4.001 GiB
> +128/128 sectors allocated at offset 1 MiB
> +128/128 sectors allocated at offset 4.001 GiB

Hmm - I wonder if my patch to make 'zero with unmap' on an image with no
backing file prefer pure unallocated clusters over a reads-as-zero would
make a difference.
https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg01728.html

Testing now...

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] qemu-img: simplify img_convert

[Peter Lieven]

Am 20.04.2017 um 16:05 schrieb Fam Zheng:

On Tue, 02/28 14:35, Peter Lieven wrote:

img_convert has been around before there was an ImgConvertState or
a block backend, but it has never been modified to directly use
these structs. Change this by parsing parameters directly into
the ImgConvertState and directly use BlockBackend where possible.
Futhermore variable initalization has been reworked and sorted.

Signed-off-by: Peter Lieven 

I see an iotest failure with this patch, in Kevin's block-next tree:

019 1s ... - output mismatch (see 019.out.bad)
--- /stor/work/qemu/tests/qemu-iotests/019.out  2017-04-17 16:19:56.523968474 
+0800
+++ 019.out.bad 2017-04-20 22:03:29.868216955 +0800
@@ -1086,8 +1086,8 @@
  
  Checking if backing clusters are allocated when they shouldn't
  
-0/128 sectors allocated at offset 1 MiB

-0/128 sectors allocated at offset 4.001 GiB
+128/128 sectors allocated at offset 1 MiB
+128/128 sectors allocated at offset 4.001 GiB
  Reading
  
  === IO: pattern 42

Failures: 019
Failed 1 of 1 tests


Thanks for notifying, Fam.

@Kevin: Can you drop the patch again? I will send a V2 addressing this issue 
and the other comments

I have got.


Peter

Re: [Qemu-block] [PATCH] qemu-img: simplify img_convert

[Fam Zheng]

On Tue, 02/28 14:35, Peter Lieven wrote:
> img_convert has been around before there was an ImgConvertState or
> a block backend, but it has never been modified to directly use
> these structs. Change this by parsing parameters directly into
> the ImgConvertState and directly use BlockBackend where possible.
> Futhermore variable initalization has been reworked and sorted.
> 
> Signed-off-by: Peter Lieven 

I see an iotest failure with this patch, in Kevin's block-next tree:

019 1s ... - output mismatch (see 019.out.bad)
--- /stor/work/qemu/tests/qemu-iotests/019.out  2017-04-17 16:19:56.523968474 
+0800
+++ 019.out.bad 2017-04-20 22:03:29.868216955 +0800
@@ -1086,8 +1086,8 @@
 
 Checking if backing clusters are allocated when they shouldn't
 
-0/128 sectors allocated at offset 1 MiB
-0/128 sectors allocated at offset 4.001 GiB
+128/128 sectors allocated at offset 1 MiB
+128/128 sectors allocated at offset 4.001 GiB
 Reading
 
 === IO: pattern 42
Failures: 019
Failed 1 of 1 tests

Re: [Qemu-block] [PATCH v3] iotests: 109: Filter out "len" of failed jobs

[Eric Blake]

On 04/19/2017 07:54 PM, Fam Zheng wrote:
> Mirror calculates job len from current I/O progress:
> 
> s->common.len = s->common.offset +
> (cnt + s->sectors_in_flight) * BDRV_SECTOR_SIZE;
> 
> The final "len" of a failed mirror job in iotests 109 depends on the
> subtle timing of the completion of read and write issued in the first
> mirror iteration.  The second iteration may or may not have run when the
> I/O error happens, resulting in non-deterministic output of the
> BLOCK_JOB_COMPLETED event text.
> 
> Similar to what was done in a752e4786, filter out the field to make the
> test robust.
> 
> Signed-off-by: Fam Zheng 
> 
> ---
> 
> v3: Cover more cases. [Kevin, Paolo]

That my testing missed those extra cases is just proof that the behavior
is non-deterministic.  Munging the 'len' data in more places is the
right solution.

> 
> v2: Add Eric's r-b.

Once again:

Reviewed-by: Eric Blake 
Tested-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH for 2.10 00/17] Block layer thread safety, part 1

[no-reply]

Hi,

This series failed automatic build test. Please find the testing commands and
their output below. If you have docker installed, you can probably reproduce it
locally.

Type: series
Message-id: 20170420120058.28404-1-pbonz...@redhat.com
Subject: [Qemu-devel] [PATCH for 2.10 00/17] Block layer thread safety, part 1

=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=8
make docker-test-quick@centos6
make docker-test-mingw@fedora
make docker-test-build@min-glib
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/20170420120058.28404-1-pbonz...@redhat.com -> 
patchew/20170420120058.28404-1-pbonz...@redhat.com
Switched to a new branch 'test'
c716264 block: make accounting thread-safe
7d0704f block: protect modification of dirty bitmaps with a mutex
2259950 block: introduce dirty_bitmap_mutex
0532009 block: optimize access to reqs_lock
862fd98 coroutine-lock: introduce qemu_co_mutex_lock_unlock
5d9d699 block: protect tracked_requests and flush_queue with reqs_lock
a65d4b0 block: access write_gen with atomics
01ac75b block: use Stat64 for wr_highest_offset
628d1ba util: add stats64 module
ea6bb59 throttle-groups: protect throttled requests with a CoMutex
895a11d throttle-groups: do not use qemu_co_enter_next
42ce4b3 block: access io_plugged with atomic ops
bc2deb1 block: access wakeup with atomic ops
18d2d96 block: access serialising_in_flight with atomic ops
a11151d block: access io_limits_disabled with atomic ops
c4d7323 block: access quiesce_counter with atomic ops
9c192bd block: access copy_on_read with atomic ops

=== OUTPUT BEGIN ===
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into '/var/tmp/patchew-tester-tmp-f1fvdzwe/src/dtc'...
Submodule path 'dtc': checked out '558cd81bdd432769b59bff01240c44f82cfb1a9d'
  BUILD   centos6
make[1]: Entering directory '/var/tmp/patchew-tester-tmp-f1fvdzwe/src'
  ARCHIVE qemu.tgz
  ARCHIVE dtc.tgz
  COPYRUNNER
RUN test-quick in qemu:centos6 
Packages installed:
SDL-devel-1.2.14-7.el6_7.1.x86_64
ccache-3.1.6-2.el6.x86_64
epel-release-6-8.noarch
gcc-4.4.7-17.el6.x86_64
git-1.7.1-4.el6_7.1.x86_64
glib2-devel-2.28.8-5.el6.x86_64
libfdt-devel-1.4.0-1.el6.x86_64
make-3.81-23.el6.x86_64
package g++ is not installed
pixman-devel-0.32.8-1.el6.x86_64
tar-1.23-15.el6_8.x86_64
zlib-devel-1.2.3-29.el6.x86_64

Environment variables:
PACKAGES=libfdt-devel ccache tar git make gcc g++ zlib-devel 
glib2-devel SDL-devel pixman-devel epel-release
HOSTNAME=6ae6c396f9d6
TERM=xterm
MAKEFLAGS= -j8
HISTSIZE=1000
J=8
USER=root
CCACHE_DIR=/var/tmp/ccache
EXTRA_CONFIGURE_OPTS=
V=
SHOW_ENV=1
MAIL=/var/spool/mail/root
PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
PWD=/
LANG=en_US.UTF-8
TARGET_LIST=
HISTCONTROL=ignoredups
SHLVL=1
HOME=/root
TEST_DIR=/tmp/qemu-test
LOGNAME=root
LESSOPEN=||/usr/bin/lesspipe.sh %s
FEATURES= dtc
DEBUG=
G_BROKEN_FILENAMES=1
CCACHE_HASHDIR=
_=/usr/bin/env

Configure options:
--enable-werror --target-list=x86_64-softmmu,aarch64-softmmu 
--prefix=/var/tmp/qemu-build/install
No C++ compiler available; disabling C++ specific optional code
Install prefix/var/tmp/qemu-build/install
BIOS directory/var/tmp/qemu-build/install/share/qemu
binary directory  /var/tmp/qemu-build/install/bin
library directory /var/tmp/qemu-build/install/lib
module directory  /var/tmp/qemu-build/install/lib/qemu
libexec directory /var/tmp/qemu-build/install/libexec
include directory /var/tmp/qemu-build/install/include
config directory  /var/tmp/qemu-build/install/etc
local state directory   /var/tmp/qemu-build/install/var
Manual directory  /var/tmp/qemu-build/install/share/man
ELF interp prefix /usr/gnemul/qemu-%M
Source path   /tmp/qemu-test/src
C compilercc
Host C compiler   cc
C++ compiler  
Objective-C compiler cc
ARFLAGS   rv
CFLAGS-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g 
QEMU_CFLAGS   -I/usr/include/pixman-1   -I$(SRC_PATH)/dtc/libfdt -pthread 
-I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include   -fPIE -DPIE -m64 -mcx16 
-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv  -Wendif-labels 
-Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security 
-Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration 
-Wold-style-definition -Wtype-limits -fstack-protector-all
LDFLAGS   -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -pie -m64 -g 
make  make
install   install
pythonpython -B
smbd  /usr/sbin/smbd
module supportno
host CPU  x86_64
host big endian   no
target list   x86_64-softmmu aarch64-softmmu
tcg debug enabled no

Re: [Qemu-block] [Qemu-devel] [PATCH for 2.10 00/17] Block layer thread safety, part 1

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20170420120058.28404-1-pbonz...@redhat.com
Subject: [Qemu-devel] [PATCH for 2.10 00/17] Block layer thread safety, part 1

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/20170420121639.32685-1-berra...@redhat.com -> 
patchew/20170420121639.32685-1-berra...@redhat.com
Switched to a new branch 'test'
c716264 block: make accounting thread-safe
7d0704f block: protect modification of dirty bitmaps with a mutex
2259950 block: introduce dirty_bitmap_mutex
0532009 block: optimize access to reqs_lock
862fd98 coroutine-lock: introduce qemu_co_mutex_lock_unlock
5d9d699 block: protect tracked_requests and flush_queue with reqs_lock
a65d4b0 block: access write_gen with atomics
01ac75b block: use Stat64 for wr_highest_offset
628d1ba util: add stats64 module
ea6bb59 throttle-groups: protect throttled requests with a CoMutex
895a11d throttle-groups: do not use qemu_co_enter_next
42ce4b3 block: access io_plugged with atomic ops
bc2deb1 block: access wakeup with atomic ops
18d2d96 block: access serialising_in_flight with atomic ops
a11151d block: access io_limits_disabled with atomic ops
c4d7323 block: access quiesce_counter with atomic ops
9c192bd block: access copy_on_read with atomic ops

=== OUTPUT BEGIN ===
Checking PATCH 1/17: block: access copy_on_read with atomic ops...
Checking PATCH 2/17: block: access quiesce_counter with atomic ops...
Checking PATCH 3/17: block: access io_limits_disabled with atomic ops...
Checking PATCH 4/17: block: access serialising_in_flight with atomic ops...
Checking PATCH 5/17: block: access wakeup with atomic ops...
Checking PATCH 6/17: block: access io_plugged with atomic ops...
Checking PATCH 7/17: throttle-groups: do not use qemu_co_enter_next...
Checking PATCH 8/17: throttle-groups: protect throttled requests with a 
CoMutex...
Checking PATCH 9/17: util: add stats64 module...
WARNING: architecture specific defines should be avoided
#35: FILE: include/qemu/stats64.h:18:
+#if __SIZEOF_LONG__ < 8

ERROR: memory barrier without comment
#343: FILE: util/stats64.c:100:
+smp_wmb();

ERROR: memory barrier without comment
#372: FILE: util/stats64.c:129:
+smp_wmb();

total: 2 errors, 1 warnings, 350 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 10/17: block: use Stat64 for wr_highest_offset...
Checking PATCH 11/17: block: access write_gen with atomics...
Checking PATCH 12/17: block: protect tracked_requests and flush_queue with 
reqs_lock...
Checking PATCH 13/17: coroutine-lock: introduce qemu_co_mutex_lock_unlock...
Checking PATCH 14/17: block: optimize access to reqs_lock...
Checking PATCH 15/17: block: introduce dirty_bitmap_mutex...
Checking PATCH 16/17: block: protect modification of dirty bitmaps with a 
mutex...
Checking PATCH 17/17: block: make accounting thread-safe...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

[Qemu-block] [PATCH 04/17] block: access serialising_in_flight with atomic ops

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 block/io.c|  6 +++---
 include/block/block_int.h | 10 ++
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/block/io.c b/block/io.c
index 3a27fd5..869322a 100644
--- a/block/io.c
+++ b/block/io.c
@@ -375,7 +375,7 @@ void bdrv_drain_all(void)
 static void tracked_request_end(BdrvTrackedRequest *req)
 {
 if (req->serialising) {
-req->bs->serialising_in_flight--;
+atomic_dec(>bs->serialising_in_flight);
 }
 
 QLIST_REMOVE(req, list);
@@ -414,7 +414,7 @@ static void mark_request_serialising(BdrvTrackedRequest 
*req, uint64_t align)
- overlap_offset;
 
 if (!req->serialising) {
-req->bs->serialising_in_flight++;
+atomic_inc(>bs->serialising_in_flight);
 req->serialising = true;
 }
 
@@ -519,7 +519,7 @@ static bool coroutine_fn 
wait_serialising_requests(BdrvTrackedRequest *self)
 bool retry;
 bool waited = false;
 
-if (!bs->serialising_in_flight) {
+if (!atomic_read(>serialising_in_flight)) {
 return false;
 }
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index a43fe78..7317db6 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -590,10 +590,6 @@ struct BlockDriverState {
 /* Callback before write request is processed */
 NotifierWithReturnList before_write_notifiers;
 
-/* number of in-flight requests; overall and serialising */
-unsigned int in_flight;
-unsigned int serialising_in_flight;
-
 bool wakeup;
 
 /* Offset after the highest byte written to */
@@ -620,6 +616,12 @@ struct BlockDriverState {
  */
 int copy_on_read;
 
+/* number of in-flight requests; overall and serialising.
+ * Accessed with atomic ops.
+ */
+unsigned int in_flight;
+unsigned int serialising_in_flight;
+
 /* do we need to tell the quest if we have a volatile write cache? */
 int enable_write_cache;
 
-- 
2.9.3

[Qemu-block] [PATCH 11/17] block: access write_gen with atomics

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 block.c   | 2 +-
 block/io.c| 6 +++---
 include/block/block_int.h | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index a48fdd8..f1aec36 100644
--- a/block.c
+++ b/block.c
@@ -3294,7 +3294,7 @@ int bdrv_truncate(BdrvChild *child, int64_t offset)
 ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
 bdrv_dirty_bitmap_truncate(bs);
 bdrv_parent_cb_resize(bs);
-++bs->write_gen;
+atomic_inc(>write_gen);
 }
 return ret;
 }
diff --git a/block/io.c b/block/io.c
index fa8abff..d17564b 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1410,7 +1410,7 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild 
*child,
 }
 bdrv_debug_event(bs, BLKDBG_PWRITEV_DONE);
 
-++bs->write_gen;
+atomic_inc(>write_gen);
 bdrv_set_dirty(bs, start_sector, end_sector - start_sector);
 
 stat64_max(>wr_highest_offset, offset + bytes);
@@ -2299,7 +2299,7 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
 goto early_exit;
 }
 
-current_gen = bs->write_gen;
+current_gen = atomic_read(>write_gen);
 
 /* Wait until any previous flushes are completed */
 while (bs->active_flush_req) {
@@ -2524,7 +2524,7 @@ int coroutine_fn bdrv_co_pdiscard(BlockDriverState *bs, 
int64_t offset,
 }
 ret = 0;
 out:
-++bs->write_gen;
+atomic_inc(>write_gen);
 bdrv_set_dirty(bs, req.offset >> BDRV_SECTOR_BITS,
req.bytes >> BDRV_SECTOR_BITS);
 tracked_request_end();
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 505c271..552680c 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -598,7 +598,6 @@ struct BlockDriverState {
 QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
 CoQueue flush_queue;  /* Serializing flush queue */
 bool active_flush_req;/* Flush request in flight? */
-unsigned int write_gen;   /* Current data generation */
 unsigned int flushed_gen; /* Flushed write generation */
 
 QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
@@ -630,6 +629,7 @@ struct BlockDriverState {
 
 /* Accessed with atomic ops.  */
 int quiesce_counter;
+unsigned int write_gen;   /* Current data generation */
 };
 
 struct BlockBackendRootState {
-- 
2.9.3

[Qemu-block] [PATCH 13/17] coroutine-lock: introduce qemu_co_mutex_lock_unlock

[Paolo Bonzini]

This primitive lets you lock/unlock a CoMutex, guaranteeing neither
blocking nor cacheline bouncing if there is no qemu_co_mutex_lock
critical section.

Signed-off-by: Paolo Bonzini 
---
 include/qemu/coroutine.h   |  6 ++
 util/qemu-coroutine-lock.c | 36 
 2 files changed, 42 insertions(+)

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index a4509bd..8d4416c 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -157,6 +157,12 @@ void qemu_co_mutex_init(CoMutex *mutex);
 void coroutine_fn qemu_co_mutex_lock(CoMutex *mutex);
 
 /**
+ * Locks the mutex and immediately unlock it.  This is faster than back-to-back
+ * lock/unlock if the mutex is not taken by anyone.
+ */
+void coroutine_fn qemu_co_mutex_lock_unlock(CoMutex *mutex);
+
+/**
  * Unlocks the mutex and schedules the next coroutine that was waiting for this
  * lock to be run.
  */
diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
index 6328eed..86f56cd 100644
--- a/util/qemu-coroutine-lock.c
+++ b/util/qemu-coroutine-lock.c
@@ -287,6 +287,42 @@ retry_fast_path:
 self->locks_held++;
 }
 
+void coroutine_fn qemu_co_mutex_lock_unlock(CoMutex *mutex)
+{
+AioContext *ctx = qemu_get_current_aio_context();
+int waiters, i;
+
+retry_fast_path:
+waiters = atomic_read(>locked);
+if (waiters == 0) {
+/* Provide same memory ordering semantics as mutex lock/unlock.  */
+smp_mb_acquire();
+smp_mb_release();
+return;
+}
+
+i = 0;
+while (waiters == 1 && ++i < 1000) {
+if (atomic_read(>ctx) == ctx) {
+break;
+}
+waiters = atomic_read(>locked);
+if (waiters == 0) {
+smp_mb_acquire();
+smp_mb_release();
+return;
+}
+cpu_relax();
+}
+
+if (atomic_cmpxchg(>locked, waiters, waiters + 1) != waiters) {
+goto retry_fast_path;
+}
+
+qemu_co_mutex_lock_slowpath(ctx, mutex);
+qemu_co_mutex_unlock(mutex);
+}
+
 void coroutine_fn qemu_co_mutex_unlock(CoMutex *mutex)
 {
 Coroutine *self = qemu_coroutine_self();
-- 
2.9.3

[Qemu-block] [PATCH 16/17] block: protect modification of dirty bitmaps with a mutex

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 block/dirty-bitmap.c | 74 +---
 block/mirror.c   | 11 +--
 include/block/block_int.h|  4 +--
 include/block/dirty-bitmap.h | 23 +++---
 4 files changed, 97 insertions(+), 15 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index e13718e..b854077 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -37,6 +37,7 @@
  * or enabled. A frozen bitmap can only abdicate() or reclaim().
  */
 struct BdrvDirtyBitmap {
+QemuMutex *mutex;
 HBitmap *bitmap;/* Dirty sector bitmap implementation */
 HBitmap *meta;  /* Meta dirty bitmap */
 BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
@@ -69,6 +70,16 @@ static inline void 
bdrv_dirty_bitmaps_unlock(BlockDriverState *bs)
 qemu_mutex_unlock(_bitmap_mutex);
 }
 
+void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap)
+{
+qemu_mutex_lock(bitmap->mutex);
+}
+
+void bdrv_dirty_bitmap_unlock(BdrvDirtyBitmap *bitmap)
+{
+qemu_mutex_unlock(bitmap->mutex);
+}
+
 /* Called with BQL or dirty_bitmap lock taken.  */
 BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
 {
@@ -116,6 +127,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState 
*bs,
 return NULL;
 }
 bitmap = g_new0(BdrvDirtyBitmap, 1);
+bitmap->mutex = >dirty_bitmap_mutex;
 bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(sector_granularity));
 bitmap->size = bitmap_size;
 bitmap->name = g_strdup(name);
@@ -141,18 +153,22 @@ void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap 
*bitmap,
int chunk_size)
 {
 assert(!bitmap->meta);
+qemu_mutex_lock(bitmap->mutex);
 bitmap->meta = hbitmap_create_meta(bitmap->bitmap,
chunk_size * BITS_PER_BYTE);
+qemu_mutex_unlock(bitmap->mutex);
 }
 
 void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
 assert(bitmap->meta);
+qemu_mutex_lock(bitmap->mutex);
 hbitmap_free_meta(bitmap->bitmap);
 bitmap->meta = NULL;
+qemu_mutex_unlock(bitmap->mutex);
 }
 
-int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
+int bdrv_dirty_bitmap_get_meta_locked(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap, int64_t sector,
int nb_sectors)
 {
@@ -169,11 +185,26 @@ int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
 return false;
 }
 
+int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
+   BdrvDirtyBitmap *bitmap, int64_t sector,
+   int nb_sectors)
+{
+bool dirty;
+
+qemu_mutex_lock(bitmap->mutex);
+dirty = bdrv_dirty_bitmap_get_meta_locked(bs, bitmap, sector, nb_sectors);
+qemu_mutex_unlock(bitmap->mutex);
+
+return dirty;
+}
+
 void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs,
   BdrvDirtyBitmap *bitmap, int64_t sector,
   int nb_sectors)
 {
+qemu_mutex_lock(bitmap->mutex);
 hbitmap_reset(bitmap->meta, sector, nb_sectors);
+qemu_mutex_unlock(bitmap->mutex);
 }
 
 int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap)
@@ -400,7 +431,8 @@ BlockDirtyInfoList 
*bdrv_query_dirty_bitmaps(BlockDriverState *bs)
 return list;
 }
 
-int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+/* Called within bdrv_dirty_bitmap_lock..unlock */
+int bdrv_get_dirty_locked(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t sector)
 {
 if (bitmap) {
@@ -410,6 +442,18 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap 
*bitmap,
 }
 }
 
+int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+   int64_t sector)
+{
+bool dirty;
+
+bdrv_dirty_bitmap_lock(bitmap);
+dirty = bdrv_get_dirty_locked(bs, bitmap, sector);
+bdrv_dirty_bitmap_unlock(bitmap);
+
+return dirty;
+}
+
 /**
  * Chooses a default granularity based on the existing cluster size,
  * but clamped between [4K, 64K]. Defaults to 64K in the case that there
@@ -474,23 +518,42 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
 return hbitmap_iter_next(>hbi);
 }
 
+/* Called within bdrv_dirty_bitmap_lock..unlock */
+void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
+  int64_t cur_sector, int64_t nr_sectors)
+{
+assert(bdrv_dirty_bitmap_enabled(bitmap));
+hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
+}
+
 void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int64_t nr_sectors)
 {
+bdrv_dirty_bitmap_lock(bitmap);
+bdrv_set_dirty_bitmap_locked(bitmap, cur_sector, nr_sectors);
+bdrv_dirty_bitmap_unlock(bitmap);
+}
+
+/* Called within bdrv_dirty_bitmap_lock..unlock */
+void 

[Qemu-block] [PATCH 10/17] block: use Stat64 for wr_highest_offset

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 block/io.c| 4 +---
 block/qapi.c  | 2 +-
 include/block/block_int.h | 7 ---
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/block/io.c b/block/io.c
index 3ab9476..fa8abff 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1413,9 +1413,7 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild 
*child,
 ++bs->write_gen;
 bdrv_set_dirty(bs, start_sector, end_sector - start_sector);
 
-if (bs->wr_highest_offset < offset + bytes) {
-bs->wr_highest_offset = offset + bytes;
-}
+stat64_max(>wr_highest_offset, offset + bytes);
 
 if (ret >= 0) {
 bs->total_sectors = MAX(bs->total_sectors, end_sector);
diff --git a/block/qapi.c b/block/qapi.c
index a40922e..14b60ae 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -441,7 +441,7 @@ static BlockStats *bdrv_query_bds_stats(const 
BlockDriverState *bs,
 s->node_name = g_strdup(bdrv_get_node_name(bs));
 }
 
-s->stats->wr_highest_offset = bs->wr_highest_offset;
+s->stats->wr_highest_offset = stat64_get(>wr_highest_offset);
 
 if (bs->file) {
 s->has_parent = true;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 283d079..505c271 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -29,6 +29,7 @@
 #include "qemu/option.h"
 #include "qemu/queue.h"
 #include "qemu/coroutine.h"
+#include "qemu/stats64.h"
 #include "qemu/timer.h"
 #include "qapi-types.h"
 #include "qemu/hbitmap.h"
@@ -590,9 +591,6 @@ struct BlockDriverState {
 /* Callback before write request is processed */
 NotifierWithReturnList before_write_notifiers;
 
-/* Offset after the highest byte written to */
-uint64_t wr_highest_offset;
-
 /* threshold limit for writes, in bytes. "High water mark". */
 uint64_t write_threshold_offset;
 NotifierWithReturn write_threshold_notifier;
@@ -605,6 +603,9 @@ struct BlockDriverState {
 
 QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
 
+/* Offset after the highest byte written to */
+Stat64 wr_highest_offset;
+
 /* If true, copy read backing sectors into image.  Can be >1 if more
  * than one client has requested copy-on-read.  Accessed with atomic
  * ops.
-- 
2.9.3

[Qemu-block] [PATCH 12/17] block: protect tracked_requests and flush_queue with reqs_lock

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 block.c   |  1 +
 block/io.c| 20 +---
 include/block/block_int.h | 12 +++-
 3 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index f1aec36..3b2ed29 100644
--- a/block.c
+++ b/block.c
@@ -234,6 +234,7 @@ BlockDriverState *bdrv_new(void)
 QLIST_INIT(>op_blockers[i]);
 }
 notifier_with_return_list_init(>before_write_notifiers);
+qemu_co_mutex_init(>reqs_lock);
 bs->refcnt = 1;
 bs->aio_context = qemu_get_aio_context();
 
diff --git a/block/io.c b/block/io.c
index d17564b..7af9d47 100644
--- a/block/io.c
+++ b/block/io.c
@@ -378,8 +378,10 @@ static void tracked_request_end(BdrvTrackedRequest *req)
 atomic_dec(>bs->serialising_in_flight);
 }
 
+qemu_co_mutex_lock(>bs->reqs_lock);
 QLIST_REMOVE(req, list);
 qemu_co_queue_restart_all(>wait_queue);
+qemu_co_mutex_unlock(>bs->reqs_lock);
 }
 
 /**
@@ -404,7 +406,9 @@ static void tracked_request_begin(BdrvTrackedRequest *req,
 
 qemu_co_queue_init(>wait_queue);
 
+qemu_co_mutex_lock(>reqs_lock);
 QLIST_INSERT_HEAD(>tracked_requests, req, list);
+qemu_co_mutex_unlock(>reqs_lock);
 }
 
 static void mark_request_serialising(BdrvTrackedRequest *req, uint64_t align)
@@ -526,6 +530,7 @@ static bool coroutine_fn 
wait_serialising_requests(BdrvTrackedRequest *self)
 
 do {
 retry = false;
+qemu_co_mutex_lock(>reqs_lock);
 QLIST_FOREACH(req, >tracked_requests, list) {
 if (req == self || (!req->serialising && !self->serialising)) {
 continue;
@@ -544,7 +549,7 @@ static bool coroutine_fn 
wait_serialising_requests(BdrvTrackedRequest *self)
  * (instead of producing a deadlock in the former case). */
 if (!req->waiting_for) {
 self->waiting_for = req;
-qemu_co_queue_wait(>wait_queue, NULL);
+qemu_co_queue_wait(>wait_queue, >reqs_lock);
 self->waiting_for = NULL;
 retry = true;
 waited = true;
@@ -552,6 +557,7 @@ static bool coroutine_fn 
wait_serialising_requests(BdrvTrackedRequest *self)
 }
 }
 }
+qemu_co_mutex_unlock(>reqs_lock);
 } while (retry);
 
 return waited;
@@ -2302,11 +2308,13 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
 current_gen = atomic_read(>write_gen);
 
 /* Wait until any previous flushes are completed */
+qemu_co_mutex_lock(>reqs_lock);
 while (bs->active_flush_req) {
-qemu_co_queue_wait(>flush_queue, NULL);
+qemu_co_queue_wait(>flush_queue, >reqs_lock);
 }
 
 bs->active_flush_req = true;
+qemu_co_mutex_unlock(>reqs_lock);
 
 /* Write back all layers by calling one driver function */
 if (bs->drv->bdrv_co_flush) {
@@ -2328,10 +2336,14 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
 goto flush_parent;
 }
 
-/* Check if we really need to flush anything */
+/* Check if we really need to flush anything
+ * TODO: use int and atomic access */
+qemu_co_mutex_lock(>reqs_lock);
 if (bs->flushed_gen == current_gen) {
+qemu_co_mutex_unlock(>reqs_lock);
 goto flush_parent;
 }
+qemu_co_mutex_unlock(>reqs_lock);
 
 BLKDBG_EVENT(bs->file, BLKDBG_FLUSH_TO_DISK);
 if (bs->drv->bdrv_co_flush_to_disk) {
@@ -2375,12 +2387,14 @@ flush_parent:
 ret = bs->file ? bdrv_co_flush(bs->file->bs) : 0;
 out:
 /* Notify any pending flushes that we have completed */
+qemu_co_mutex_lock(>reqs_lock);
 if (ret == 0) {
 bs->flushed_gen = current_gen;
 }
 bs->active_flush_req = false;
 /* Return value is ignored - it's ok if wait queue is empty */
 qemu_co_queue_next(>flush_queue);
+qemu_co_mutex_unlock(>reqs_lock);
 
 early_exit:
 bdrv_dec_in_flight(bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 552680c..42b49f5 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -595,11 +595,6 @@ struct BlockDriverState {
 uint64_t write_threshold_offset;
 NotifierWithReturn write_threshold_notifier;
 
-QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
-CoQueue flush_queue;  /* Serializing flush queue */
-bool active_flush_req;/* Flush request in flight? */
-unsigned int flushed_gen; /* Flushed write generation */
-
 QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
 
 /* Offset after the highest byte written to */
@@ -630,6 +625,13 @@ struct BlockDriverState {
 /* Accessed with atomic ops.  */
 int quiesce_counter;
 unsigned int write_gen;   /* Current data generation */
+
+/* Protected by reqs_lock.  */
+QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
+CoQueue flush_queue;  /* 

[Qemu-block] [PATCH 02/17] block: access quiesce_counter with atomic ops

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 block/io.c| 4 ++--
 include/block/block_int.h | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c
index 08de488..3a27fd5 100644
--- a/block/io.c
+++ b/block/io.c
@@ -241,7 +241,7 @@ void bdrv_drained_begin(BlockDriverState *bs)
 return;
 }
 
-if (!bs->quiesce_counter++) {
+if (atomic_fetch_inc(>quiesce_counter) == 0) {
 aio_disable_external(bdrv_get_aio_context(bs));
 bdrv_parent_drained_begin(bs);
 }
@@ -252,7 +252,7 @@ void bdrv_drained_begin(BlockDriverState *bs)
 void bdrv_drained_end(BlockDriverState *bs)
 {
 assert(bs->quiesce_counter > 0);
-if (--bs->quiesce_counter > 0) {
+if (atomic_fetch_dec(>quiesce_counter) > 1) {
 return;
 }
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index d0b88ff..a43fe78 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -623,6 +623,7 @@ struct BlockDriverState {
 /* do we need to tell the quest if we have a volatile write cache? */
 int enable_write_cache;
 
+/* Accessed with atomic ops.  */
 int quiesce_counter;
 };
 
-- 
2.9.3

[Qemu-block] [PATCH 06/17] block: access io_plugged with atomic ops

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 block/io.c| 4 ++--
 include/block/block_int.h | 8 +---
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/io.c b/block/io.c
index 3b2ede9..3ab9476 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2653,7 +2653,7 @@ void bdrv_io_plug(BlockDriverState *bs)
 bdrv_io_plug(child->bs);
 }
 
-if (bs->io_plugged++ == 0) {
+if (atomic_fetch_inc(>io_plugged) == 0) {
 BlockDriver *drv = bs->drv;
 if (drv && drv->bdrv_io_plug) {
 drv->bdrv_io_plug(bs);
@@ -2666,7 +2666,7 @@ void bdrv_io_unplug(BlockDriverState *bs)
 BdrvChild *child;
 
 assert(bs->io_plugged);
-if (--bs->io_plugged == 0) {
+if (atomic_fetch_dec(>io_plugged) == 1) {
 BlockDriver *drv = bs->drv;
 if (drv && drv->bdrv_io_unplug) {
 drv->bdrv_io_unplug(bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ca34c99..283d079 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -597,9 +597,6 @@ struct BlockDriverState {
 uint64_t write_threshold_offset;
 NotifierWithReturn write_threshold_notifier;
 
-/* counter for nested bdrv_io_plug */
-unsigned io_plugged;
-
 QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
 CoQueue flush_queue;  /* Serializing flush queue */
 bool active_flush_req;/* Flush request in flight? */
@@ -622,6 +619,11 @@ struct BlockDriverState {
 
 bool wakeup;
 
+/* counter for nested bdrv_io_plug.
+ * Accessed with atomic ops.
+*/
+unsigned io_plugged;
+
 /* do we need to tell the quest if we have a volatile write cache? */
 int enable_write_cache;
 
-- 
2.9.3

[Qemu-block] [PATCH 03/17] block: access io_limits_disabled with atomic ops

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 block/block-backend.c  | 4 ++--
 block/throttle-groups.c| 2 +-
 include/sysemu/block-backend.h | 3 ++-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 7405024..915ccc5 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1927,7 +1927,7 @@ static void blk_root_drained_begin(BdrvChild *child)
 /* Note that blk->root may not be accessible here yet if we are just
  * attaching to a BlockDriverState that is drained. Use child instead. */
 
-if (blk->public.io_limits_disabled++ == 0) {
+if (atomic_fetch_inc(>public.io_limits_disabled) == 0) {
 throttle_group_restart_blk(blk);
 }
 }
@@ -1938,7 +1938,7 @@ static void blk_root_drained_end(BdrvChild *child)
 assert(blk->quiesce_counter);
 
 assert(blk->public.io_limits_disabled);
---blk->public.io_limits_disabled;
+atomic_dec(>public.io_limits_disabled);
 
 if (--blk->quiesce_counter == 0) {
 if (blk->dev_ops && blk->dev_ops->drained_end) {
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index b73e7a8..69bfbd4 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -240,7 +240,7 @@ static bool throttle_group_schedule_timer(BlockBackend 
*blk, bool is_write)
 ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
 bool must_wait;
 
-if (blkp->io_limits_disabled) {
+if (atomic_read(>io_limits_disabled)) {
 return false;
 }
 
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 7462228..87a43b0 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -80,7 +80,8 @@ typedef struct BlockBackendPublic {
 CoQueue  throttled_reqs[2];
 
 /* Nonzero if the I/O limits are currently being ignored; generally
- * it is zero.  */
+ * it is zero.  Accessed with atomic operations.
+ */
 unsigned int io_limits_disabled;
 
 /* The following fields are protected by the ThrottleGroup lock.
-- 
2.9.3

[Qemu-block] [PATCH 17/17] block: make accounting thread-safe

[Paolo Bonzini]

I'm not trying too hard yet.  Later, with multiqueue support,
this may cause cacheline bouncing.

Signed-off-by: Paolo Bonzini 
---
 block/accounting.c | 15 +++
 include/block/accounting.h |  8 ++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/block/accounting.c b/block/accounting.c
index 3f457c4..dc10855 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -35,6 +35,7 @@ static const int qtest_latency_ns = NANOSECONDS_PER_SECOND / 
1000;
 void block_acct_init(BlockAcctStats *stats, bool account_invalid,
  bool account_failed)
 {
+qemu_spin_init(>spin);
 stats->account_invalid = account_invalid;
 stats->account_failed = account_failed;
 
@@ -58,12 +59,15 @@ void block_acct_add_interval(BlockAcctStats *stats, 
unsigned interval_length)
 
 s = g_new0(BlockAcctTimedStats, 1);
 s->interval_length = interval_length;
+s->stats = stats;
+qemu_spin_lock(>spin);
 QSLIST_INSERT_HEAD(>intervals, s, entries);
 
 for (i = 0; i < BLOCK_MAX_IOTYPE; i++) {
 timed_average_init(>latency[i], clock_type,
(uint64_t) interval_length * 
NANOSECONDS_PER_SECOND);
 }
+qemu_spin_unlock(>spin);
 }
 
 BlockAcctTimedStats *block_acct_interval_next(BlockAcctStats *stats,
@@ -98,6 +102,7 @@ void block_acct_done(BlockAcctStats *stats, BlockAcctCookie 
*cookie)
 
 assert(cookie->type < BLOCK_MAX_IOTYPE);
 
+qemu_spin_lock(>spin);
 stats->nr_bytes[cookie->type] += cookie->bytes;
 stats->nr_ops[cookie->type]++;
 stats->total_time_ns[cookie->type] += latency_ns;
@@ -106,12 +111,14 @@ void block_acct_done(BlockAcctStats *stats, 
BlockAcctCookie *cookie)
 QSLIST_FOREACH(s, >intervals, entries) {
 timed_average_account(>latency[cookie->type], latency_ns);
 }
+qemu_spin_unlock(>spin);
 }
 
 void block_acct_failed(BlockAcctStats *stats, BlockAcctCookie *cookie)
 {
 assert(cookie->type < BLOCK_MAX_IOTYPE);
 
+qemu_spin_lock(>spin);
 stats->failed_ops[cookie->type]++;
 
 if (stats->account_failed) {
@@ -130,6 +137,7 @@ void block_acct_failed(BlockAcctStats *stats, 
BlockAcctCookie *cookie)
 timed_average_account(>latency[cookie->type], latency_ns);
 }
 }
+qemu_spin_unlock(>spin);
 }
 
 void block_acct_invalid(BlockAcctStats *stats, enum BlockAcctType type)
@@ -141,18 +149,23 @@ void block_acct_invalid(BlockAcctStats *stats, enum 
BlockAcctType type)
  * invalid requests are accounted during their submission,
  * therefore there's no actual I/O involved. */
 
+qemu_spin_lock(>spin);
 stats->invalid_ops[type]++;
 
 if (stats->account_invalid) {
 stats->last_access_time_ns = qemu_clock_get_ns(clock_type);
 }
+qemu_spin_unlock(>spin);
 }
 
 void block_acct_merge_done(BlockAcctStats *stats, enum BlockAcctType type,
   int num_requests)
 {
 assert(type < BLOCK_MAX_IOTYPE);
+
+qemu_spin_lock(>spin);
 stats->merged[type] += num_requests;
+qemu_spin_unlock(>spin);
 }
 
 int64_t block_acct_idle_time_ns(BlockAcctStats *stats)
@@ -167,7 +180,9 @@ double block_acct_queue_depth(BlockAcctTimedStats *stats,
 
 assert(type < BLOCK_MAX_IOTYPE);
 
+qemu_spin_lock(>stats->spin);
 sum = timed_average_sum(>latency[type], );
+qemu_spin_unlock(>stats->spin);
 
 return (double) sum / elapsed;
 }
diff --git a/include/block/accounting.h b/include/block/accounting.h
index 2089163..90b7a1d 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -26,8 +26,10 @@
 #define BLOCK_ACCOUNTING_H
 
 #include "qemu/timed-average.h"
+#include "qemu/thread.h"
 
 typedef struct BlockAcctTimedStats BlockAcctTimedStats;
+typedef struct BlockAcctStats BlockAcctStats;
 
 enum BlockAcctType {
 BLOCK_ACCT_READ,
@@ -37,12 +39,14 @@ enum BlockAcctType {
 };
 
 struct BlockAcctTimedStats {
+BlockAcctStats *stats;
 TimedAverage latency[BLOCK_MAX_IOTYPE];
 unsigned interval_length; /* in seconds */
 QSLIST_ENTRY(BlockAcctTimedStats) entries;
 };
 
-typedef struct BlockAcctStats {
+struct BlockAcctStats {
+QemuSpin spin;
 uint64_t nr_bytes[BLOCK_MAX_IOTYPE];
 uint64_t nr_ops[BLOCK_MAX_IOTYPE];
 uint64_t invalid_ops[BLOCK_MAX_IOTYPE];
@@ -53,7 +57,7 @@ typedef struct BlockAcctStats {
 QSLIST_HEAD(, BlockAcctTimedStats) intervals;
 bool account_invalid;
 bool account_failed;
-} BlockAcctStats;
+};
 
 typedef struct BlockAcctCookie {
 int64_t bytes;
-- 
2.9.3

[Qemu-block] [PATCH 08/17] throttle-groups: protect throttled requests with a CoMutex

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 block/block-backend.c  |  1 +
 block/throttle-groups.c| 11 +--
 include/sysemu/block-backend.h |  7 ++-
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 915ccc5..a37d74d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -163,6 +163,7 @@ BlockBackend *blk_new(uint64_t perm, uint64_t shared_perm)
 blk->shared_perm = shared_perm;
 blk_set_enable_write_cache(blk, true);
 
+qemu_co_mutex_init(>public.reqs_lock);
 qemu_co_queue_init(>public.throttled_reqs[0]);
 qemu_co_queue_init(>public.throttled_reqs[1]);
 
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index d66bf62..695d28d 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -268,8 +268,13 @@ static bool throttle_group_schedule_timer(BlockBackend 
*blk, bool is_write)
 static bool throttle_group_co_restart_queue(BlockBackend *blk, bool is_write)
 {
 BlockBackendPublic *blkp = blk_get_public(blk);
+bool ret;
 
-return qemu_co_queue_next(>throttled_reqs[is_write]);
+qemu_co_mutex_lock(>reqs_lock);
+ret = qemu_co_queue_next(>throttled_reqs[is_write]);
+qemu_co_mutex_unlock(>reqs_lock);
+
+return ret;
 }
 
 /* Look for the next pending I/O request and schedule it.
@@ -338,7 +343,9 @@ void coroutine_fn 
throttle_group_co_io_limits_intercept(BlockBackend *blk,
 if (must_wait || blkp->pending_reqs[is_write]) {
 blkp->pending_reqs[is_write]++;
 qemu_mutex_unlock(>lock);
-qemu_co_queue_wait(>throttled_reqs[is_write], NULL);
+qemu_co_mutex_lock(>reqs_lock);
+qemu_co_queue_wait(>throttled_reqs[is_write], >reqs_lock);
+qemu_co_mutex_unlock(>reqs_lock);
 qemu_mutex_lock(>lock);
 blkp->pending_reqs[is_write]--;
 }
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 87a43b0..e9529fb 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -72,11 +72,8 @@ typedef struct BlockDevOps {
  * fields that must be public. This is in particular for QLIST_ENTRY() and
  * friends so that BlockBackends can be kept in lists outside block-backend.c 
*/
 typedef struct BlockBackendPublic {
-/* I/O throttling has its own locking, but also some fields are
- * protected by the AioContext lock.
- */
-
-/* Protected by AioContext lock.  */
+/* reqs_lock protects the CoQueues for throttled requests.  */
+CoMutex  reqs_lock;
 CoQueue  throttled_reqs[2];
 
 /* Nonzero if the I/O limits are currently being ignored; generally
-- 
2.9.3

[Qemu-block] [PATCH 05/17] block: access wakeup with atomic ops

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 block/io.c| 3 ++-
 block/nfs.c   | 4 +++-
 block/sheepdog.c  | 3 ++-
 include/block/block.h | 5 +++--
 include/block/block_int.h | 4 ++--
 5 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/block/io.c b/block/io.c
index 869322a..3b2ede9 100644
--- a/block/io.c
+++ b/block/io.c
@@ -501,7 +501,8 @@ static void dummy_bh_cb(void *opaque)
 
 void bdrv_wakeup(BlockDriverState *bs)
 {
-if (bs->wakeup) {
+/* The barrier (or an atomic op) is in the caller.  */
+if (atomic_read(>wakeup)) {
 aio_bh_schedule_oneshot(qemu_get_aio_context(), dummy_bh_cb, NULL);
 }
 }
diff --git a/block/nfs.c b/block/nfs.c
index 0816678..fd2508a 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -736,7 +736,9 @@ nfs_get_allocated_file_size_cb(int ret, struct nfs_context 
*nfs, void *data,
 if (task->ret < 0) {
 error_report("NFS Error: %s", nfs_get_error(nfs));
 }
-task->complete = 1;
+
+/* Set task->complete before reading bs->wakeup.  */
+atomic_mb_set(>complete, 1);
 bdrv_wakeup(task->bs);
 }
 
diff --git a/block/sheepdog.c b/block/sheepdog.c
index fb9203e..7e90a24 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -704,7 +704,8 @@ out:
 
 srco->co = NULL;
 srco->ret = ret;
-srco->finished = true;
+/* Set srco->finished before reading bs->wakeup.  */
+atomic_mb_set(>finished, true);
 if (srco->bs) {
 bdrv_wakeup(srco->bs);
 }
diff --git a/include/block/block.h b/include/block/block.h
index 5ddc0cf..486b6ed 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -398,7 +398,8 @@ void bdrv_drain_all(void);
  * block_job_defer_to_main_loop for how to do it). \
  */\
 assert(!bs_->wakeup);  \
-bs_->wakeup = true;\
+/* Set bs->wakeup before evaluating cond.  */  \
+atomic_mb_set(_->wakeup, true); \
 while (busy_) {\
 if ((cond)) {  \
 waited_ = busy_ = true;\
@@ -410,7 +411,7 @@ void bdrv_drain_all(void);
 waited_ |= busy_;  \
 }  \
 }  \
-bs_->wakeup = false;   \
+atomic_set(_->wakeup, false);   \
 }  \
 waited_; })
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 7317db6..ca34c99 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -590,8 +590,6 @@ struct BlockDriverState {
 /* Callback before write request is processed */
 NotifierWithReturnList before_write_notifiers;
 
-bool wakeup;
-
 /* Offset after the highest byte written to */
 uint64_t wr_highest_offset;
 
@@ -622,6 +620,8 @@ struct BlockDriverState {
 unsigned int in_flight;
 unsigned int serialising_in_flight;
 
+bool wakeup;
+
 /* do we need to tell the quest if we have a volatile write cache? */
 int enable_write_cache;
 
-- 
2.9.3

[Qemu-block] [PATCH 01/17] block: access copy_on_read with atomic ops

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 block.c   |  6 --
 block/io.c|  8 
 blockdev.c|  2 +-
 include/block/block_int.h | 11 ++-
 4 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/block.c b/block.c
index 1fbbb8d..a48fdd8 100644
--- a/block.c
+++ b/block.c
@@ -1189,7 +1189,9 @@ static int bdrv_open_common(BlockDriverState *bs, 
BlockBackend *file,
 goto fail_opts;
 }
 
-assert(bs->copy_on_read == 0); /* bdrv_new() and bdrv_close() make it so */
+/* bdrv_new() and bdrv_close() make it so */
+assert(atomic_read(>copy_on_read) == 0);
+
 if (bs->open_flags & BDRV_O_COPY_ON_READ) {
 if (!bs->read_only) {
 bdrv_enable_copy_on_read(bs);
@@ -2937,7 +2939,7 @@ static void bdrv_close(BlockDriverState *bs)
 
 g_free(bs->opaque);
 bs->opaque = NULL;
-bs->copy_on_read = 0;
+atomic_set(>copy_on_read, 0);
 bs->backing_file[0] = '\0';
 bs->backing_format[0] = '\0';
 bs->total_sectors = 0;
diff --git a/block/io.c b/block/io.c
index a54e5c8..08de488 100644
--- a/block/io.c
+++ b/block/io.c
@@ -130,13 +130,13 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error 
**errp)
  */
 void bdrv_enable_copy_on_read(BlockDriverState *bs)
 {
-bs->copy_on_read++;
+atomic_inc(>copy_on_read);
 }
 
 void bdrv_disable_copy_on_read(BlockDriverState *bs)
 {
-assert(bs->copy_on_read > 0);
-bs->copy_on_read--;
+assert(atomic_read(>copy_on_read) > 0);
+atomic_dec(>copy_on_read);
 }
 
 /* Check if any requests are in-flight (including throttled requests) */
@@ -1144,7 +1144,7 @@ int coroutine_fn bdrv_co_preadv(BdrvChild *child,
 bdrv_inc_in_flight(bs);
 
 /* Don't do copy-on-read if we read data before write operation */
-if (bs->copy_on_read && !(flags & BDRV_REQ_NO_SERIALISING)) {
+if (atomic_read(>copy_on_read) && !(flags & BDRV_REQ_NO_SERIALISING)) {
 flags |= BDRV_REQ_COPY_ON_READ;
 }
 
diff --git a/blockdev.c b/blockdev.c
index 9098233..e9b5717 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1794,7 +1794,7 @@ static void external_snapshot_commit(BlkActionState 
*common)
 /* We don't need (or want) to use the transactional
  * bdrv_reopen_multiple() across all the entries at once, because we
  * don't want to abort all of them if one of them fails the reopen */
-if (!state->old_bs->copy_on_read) {
+if (!atomic_read(>old_bs->copy_on_read)) {
 bdrv_reopen(state->old_bs, state->old_bs->open_flags & ~BDRV_O_RDWR,
 NULL);
 }
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 59400bd..d0b88ff 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -581,11 +581,6 @@ struct BlockDriverState {
 
 /* Protected by AioContext lock */
 
-/* If true, copy read backing sectors into image.  Can be >1 if more
- * than one client has requested copy-on-read.
- */
-int copy_on_read;
-
 /* If we are reading a disk image, give its size in sectors.
  * Generally read-only; it is written to by load_vmstate and save_vmstate,
  * but the block layer is quiescent during those.
@@ -619,6 +614,12 @@ struct BlockDriverState {
 
 QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
 
+/* If true, copy read backing sectors into image.  Can be >1 if more
+ * than one client has requested copy-on-read.  Accessed with atomic
+ * ops.
+ */
+int copy_on_read;
+
 /* do we need to tell the quest if we have a volatile write cache? */
 int enable_write_cache;
 
-- 
2.9.3

[Qemu-block] [PATCH for 2.10 00/17] Block layer thread safety, part 1

[Paolo Bonzini]

This series uses mutexes or atomic operations around core block layer
operations.  The remaining parts include:

- drivers, though most of them are already thread safe (part 2, 8 patches,
  depends on Kevin's conversion of QED to coroutines)

- block jobs, before-write notifiers, the write threshold mechanism,
  snapshots, replication, key management (part 3, 16 patches)

- devices (virtio-blk/virtio-scsi, part 4, 5 patches)

Once these four parts are done the AioContext lock can be removed
together with all temporary workarounds that have accumulated.

Paolo

Paolo Bonzini (17):
  block: access copy_on_read with atomic ops
  block: access quiesce_counter with atomic ops
  block: access io_limits_disabled with atomic ops
  block: access serialising_in_flight with atomic ops
  block: access wakeup with atomic ops
  block: access io_plugged with atomic ops
  throttle-groups: do not use qemu_co_enter_next
  throttle-groups: protect throttled requests with a CoMutex
  util: add stats64 module
  block: use Stat64 for wr_highest_offset
  block: access write_gen with atomics
  block: protect tracked_requests and flush_queue with reqs_lock
  coroutine-lock: introduce qemu_co_mutex_lock_unlock
  block: optimize access to reqs_lock
  block: introduce dirty_bitmap_mutex
  block: protect modification of dirty bitmaps with a mutex
  block: make accounting thread-safe

 block.c|  10 +-
 block/accounting.c |  15 +++
 block/block-backend.c  |   5 +-
 block/dirty-bitmap.c   | 125 ++--
 block/io.c |  70 +-
 block/mirror.c |  14 ++-
 block/nfs.c|   4 +-
 block/qapi.c   |   2 +-
 block/sheepdog.c   |   3 +-
 block/throttle-groups.c|  76 +--
 block/write-threshold.c|  37 
 blockdev.c |  46 ++---
 include/block/accounting.h |   8 +-
 include/block/block.h  |   5 +-
 include/block/block_int.h  |  63 -
 include/block/dirty-bitmap.h   |  23 -
 include/qemu/coroutine.h   |   6 ++
 include/qemu/stats64.h | 210 +
 include/sysemu/block-backend.h |  10 +-
 migration/block.c  |   6 --
 util/Makefile.objs |   1 +
 util/qemu-coroutine-lock.c |  36 +++
 util/stats64.c | 135 ++
 23 files changed, 763 insertions(+), 147 deletions(-)
 create mode 100644 include/qemu/stats64.h
 create mode 100644 util/stats64.c

-- 
2.9.3

[Qemu-block] [PATCH 07/17] throttle-groups: do not use qemu_co_enter_next

[Paolo Bonzini]

Prepare for removing this function; always restart throttled requests
from coroutine context.  This will matter when restarting throttled
requests will have to acquire a CoMutex.

Signed-off-by: Paolo Bonzini 
---
 block/throttle-groups.c | 65 +++--
 1 file changed, 58 insertions(+), 7 deletions(-)

diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 69bfbd4..d66bf62 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -260,6 +260,18 @@ static bool throttle_group_schedule_timer(BlockBackend 
*blk, bool is_write)
 return must_wait;
 }
 
+/* Start the next pending I/O request for a BlockBackend.
+ *
+ * @blk:   the current BlockBackend
+ * @is_write:  the type of operation (read/write)
+ */
+static bool throttle_group_co_restart_queue(BlockBackend *blk, bool is_write)
+{
+BlockBackendPublic *blkp = blk_get_public(blk);
+
+return qemu_co_queue_next(>throttled_reqs[is_write]);
+}
+
 /* Look for the next pending I/O request and schedule it.
  *
  * This assumes that tg->lock is held.
@@ -287,7 +299,7 @@ static void schedule_next_request(BlockBackend *blk, bool 
is_write)
 if (!must_wait) {
 /* Give preference to requests from the current blk */
 if (qemu_in_coroutine() &&
-qemu_co_queue_next(>throttled_reqs[is_write])) {
+throttle_group_co_restart_queue(blk, is_write)) {
 token = blk;
 } else {
 ThrottleTimers *tt = _get_public(token)->throttle_timers;
@@ -340,18 +352,57 @@ void coroutine_fn 
throttle_group_co_io_limits_intercept(BlockBackend *blk,
 qemu_mutex_unlock(>lock);
 }
 
-void throttle_group_restart_blk(BlockBackend *blk)
+typedef struct {
+BlockBackend *blk;
+bool is_write;
+int ret;
+} RestartData;
+
+static void throttle_group_restart_queue_entry(void *opaque)
 {
-BlockBackendPublic *blkp = blk_get_public(blk);
+RestartData *data = opaque;
+
+data->ret = throttle_group_co_restart_queue(data->blk, data->is_write);
+}
+
+static int throttle_group_restart_queue(BlockBackend *blk, bool is_write)
+{
+Coroutine *co;
+RestartData rd = {
+.blk = blk,
+.is_write = is_write
+};
+
+aio_context_acquire(blk_get_aio_context(blk));
+co = qemu_coroutine_create(throttle_group_restart_queue_entry, );
+/* The request doesn't start until after throttle_group_restart_queue_entry
+ * returns, so the coroutine cannot yield.
+ */
+qemu_coroutine_enter(co);
+aio_context_release(blk_get_aio_context(blk));
+return rd.ret;
+}
+
+static void throttle_group_restart_blk_entry(void *opaque)
+{
+BlockBackend *blk = opaque;
 int i;
 
 for (i = 0; i < 2; i++) {
-while (qemu_co_enter_next(>throttled_reqs[i])) {
+while (throttle_group_co_restart_queue(blk, i)) {
 ;
 }
 }
 }
 
+void throttle_group_restart_blk(BlockBackend *blk)
+{
+Coroutine *co;
+
+co = qemu_coroutine_create(throttle_group_restart_blk_entry, blk);
+qemu_coroutine_enter(co);
+}
+
 /* Update the throttle configuration for a particular group. Similar
  * to throttle_config(), but guarantees atomicity within the
  * throttling group.
@@ -376,8 +427,8 @@ void throttle_group_config(BlockBackend *blk, 
ThrottleConfig *cfg)
 throttle_config(ts, tt, cfg);
 qemu_mutex_unlock(>lock);
 
-qemu_co_enter_next(>throttled_reqs[0]);
-qemu_co_enter_next(>throttled_reqs[1]);
+throttle_group_restart_queue(blk, 0);
+throttle_group_restart_queue(blk, 1);
 }
 
 /* Get the throttle configuration from a particular group. Similar to
@@ -417,7 +468,7 @@ static void timer_cb(BlockBackend *blk, bool is_write)
 
 /* Run the request that was waiting for this timer */
 aio_context_acquire(blk_get_aio_context(blk));
-empty_queue = !qemu_co_enter_next(>throttled_reqs[is_write]);
+empty_queue = !throttle_group_restart_queue(blk, is_write);
 aio_context_release(blk_get_aio_context(blk));
 
 /* If the request queue was empty then we have to take care of
-- 
2.9.3

[Qemu-block] [PATCH 09/17] util: add stats64 module

[Paolo Bonzini]

This module provides fast paths for 64-bit atomic operations on machines
that only have 32-bit atomic access.

Signed-off-by: Paolo Bonzini 
---
 include/qemu/stats64.h | 210 +
 util/Makefile.objs |   1 +
 util/stats64.c | 135 
 3 files changed, 346 insertions(+)
 create mode 100644 include/qemu/stats64.h
 create mode 100644 util/stats64.c

diff --git a/include/qemu/stats64.h b/include/qemu/stats64.h
new file mode 100644
index 000..70963f4
--- /dev/null
+++ b/include/qemu/stats64.h
@@ -0,0 +1,210 @@
+/*
+ * Atomic operations on 64-bit quantities.
+ *
+ * Copyright (C) 2017 Red Hat, Inc.
+ *
+ * Author: Paolo Bonzini 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_STATS64_H
+#define QEMU_STATS64_H 1
+
+#include "qemu/atomic.h"
+
+/* FIXME: i386 doesn't need the spinlock.  Are there any others? */
+#if __SIZEOF_LONG__ < 8
+#define STAT64_NEED_SPINLOCK 1
+#endif
+
+/* This provides atomic operations on 64-bit type, using a reader-writer
+ * spinlock on architectures that do not have 64-bit accesses.  However
+ * it tries hard not to take the lock.
+ */
+
+typedef struct Stat64 {
+#ifdef STAT64_NEED_SPINLOCK
+uint32_t low, high;
+uint32_t lock;
+#else
+uint64_t value;
+#endif
+} Stat64;
+
+#ifndef STAT64_NEED_SPINLOCK
+static inline void stat64_init(Stat64 *s, uint64_t value)
+{
+/* This is not guaranteed to be atomic! */
+*s = (Stat64) { value };
+}
+
+static inline uint64_t stat64_get(const Stat64 *s)
+{
+return atomic_read(>value);
+}
+
+static inline void stat64_add(Stat64 *s, uint64_t value)
+{
+atomic_add(>value, value);
+}
+
+static inline void stat64_min(Stat64 *s, uint32_t value)
+{
+for (;;) {
+uint64_t orig = atomic_read(>value);
+if (orig <= value) {
+break;
+}
+orig = atomic_cmpxchg(>value, orig, value);
+if (orig <= value) {
+break;
+}
+}
+}
+
+static inline void stat64_max(Stat64 *s, uint32_t value)
+{
+for (;;) {
+uint64_t orig = atomic_read(>value);
+if (orig >= value) {
+break;
+}
+orig = atomic_cmpxchg(>value, orig, value);
+if (orig >= value) {
+break;
+}
+}
+}
+#else
+uint64_t stat64_get(const Stat64 *s);
+bool stat64_min_slow(Stat64 *s, uint64_t value);
+bool stat64_max_slow(Stat64 *s, uint64_t value);
+bool stat64_add32_carry(Stat64 *s, uint32_t low, uint32_t high);
+
+static inline void stat64_init(Stat64 *s, uint64_t value)
+{
+/* This is not guaranteed to be atomic! */
+*s = (Stat64) { .low = value, .high = value >> 32, .lock = 0 };
+}
+
+static inline void stat64_add(Stat64 *s, uint64_t value)
+{
+uint32_t low, high;
+high = value >> 32;
+low = (uint32_t) value;
+if (!low) {
+if (high) {
+atomic_add(>high, high);
+}
+return;
+}
+
+for (;;) {
+uint32_t orig = s->low;
+uint32_t result = orig + low;
+uint32_t old;
+
+if (result < low || high) {
+/* If the high part is affected, take the lock.  */
+if (stat64_add32_carry(s, low, high)) {
+return;
+}
+continue;
+}
+
+/* No carry, try with a 32-bit cmpxchg.  The result is independent of
+ * the high 32 bits, so it can race just fine with stat64_add32_carry
+ * and even stat64_get!
+ */
+old = atomic_cmpxchg(>low, orig, result);
+if (orig == old) {
+return;
+}
+}
+}
+
+static inline void stat64_min(Stat64 *s, uint64_t value)
+{
+uint32_t low, high;
+uint32_t orig_low, orig_high;
+
+high = value >> 32;
+low = (uint32_t) value;
+do {
+orig_high = atomic_read(>high);
+if (orig_high < high) {
+return;
+}
+
+if (orig_high == high) {
+/* High 32 bits are equal.  Read low after high, otherwise we
+ * can get a false positive (e.g. 0x1235,0x changes to
+ * 0x1234,0x8000 and we read it as 0x1234,0x). Pairs with
+ * the write barrier in stat64_min_slow.
+ */
+smp_rmb();
+orig_low = atomic_read(>low);
+if (orig_low <= low) {
+return;
+}
+
+/* See if we were lucky and a writer raced against us.  The
+ * barrier is theoretically unnecessary, but if we remove it
+ * we may miss being lucky.
+ */
+smp_rmb();
+orig_high = atomic_read(>high);
+if (orig_high < high) {
+return;
+}
+}
+
+/* If the value changes in any way, we have to take the lock.  */
+} while 

[Qemu-block] [PATCH 14/17] block: optimize access to reqs_lock

[Paolo Bonzini]

Hot path reqs_lock critical sections are very small; the only large critical
sections happen when a request waits for serialising requests, and these
should never happen in usual circumstances.

We do not want these small critical sections to yield in any case,
which calls for using a spinlock while writing the list.  The reqs_lock
is still used to protect the individual requests' CoQueue.  For this
purpose, serializing removals against concurrent walks of the request
list can use lock_unlock for efficiency and determinism.

The reqs_lock is also used to protect the flush generation counts, but
that's unrelated.

Signed-off-by: Paolo Bonzini 
---
 block.c   |  1 +
 block/io.c| 25 -
 include/block/block_int.h | 11 ---
 3 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 3b2ed29..7ba6afe 100644
--- a/block.c
+++ b/block.c
@@ -234,6 +234,7 @@ BlockDriverState *bdrv_new(void)
 QLIST_INIT(>op_blockers[i]);
 }
 notifier_with_return_list_init(>before_write_notifiers);
+qemu_spin_init(>reqs_list_write_lock);
 qemu_co_mutex_init(>reqs_lock);
 bs->refcnt = 1;
 bs->aio_context = qemu_get_aio_context();
diff --git a/block/io.c b/block/io.c
index 7af9d47..476807d 100644
--- a/block/io.c
+++ b/block/io.c
@@ -374,14 +374,29 @@ void bdrv_drain_all(void)
  */
 static void tracked_request_end(BdrvTrackedRequest *req)
 {
+BlockDriverState *bs = req->bs;
+
 if (req->serialising) {
-atomic_dec(>bs->serialising_in_flight);
+atomic_dec(>serialising_in_flight);
 }
 
-qemu_co_mutex_lock(>bs->reqs_lock);
+/* Note that there can be a concurrent visit while we remove the list,
+ * so we need to...
+ */
+qemu_spin_lock(>reqs_list_write_lock);
 QLIST_REMOVE(req, list);
+qemu_spin_unlock(>reqs_list_write_lock);
+
+/* ... wait for it to end before we leave.  qemu_co_mutex_lock_unlock
+ * avoids cacheline bouncing in the common case of no concurrent
+ * reader.
+ */
+qemu_co_mutex_lock_unlock(>reqs_lock);
+
+/* Now no coroutine can add itself to the wait queue, so it is
+ * safe to call qemu_co_queue_restart_all outside the reqs_lock.
+ */
 qemu_co_queue_restart_all(>wait_queue);
-qemu_co_mutex_unlock(>bs->reqs_lock);
 }
 
 /**
@@ -406,9 +421,9 @@ static void tracked_request_begin(BdrvTrackedRequest *req,
 
 qemu_co_queue_init(>wait_queue);
 
-qemu_co_mutex_lock(>reqs_lock);
+qemu_spin_lock(>reqs_list_write_lock);
 QLIST_INSERT_HEAD(>tracked_requests, req, list);
-qemu_co_mutex_unlock(>reqs_lock);
+qemu_spin_unlock(>reqs_list_write_lock);
 }
 
 static void mark_request_serialising(BdrvTrackedRequest *req, uint64_t align)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 42b49f5..b298de8 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -78,9 +78,10 @@ typedef struct BdrvTrackedRequest {
 
 QLIST_ENTRY(BdrvTrackedRequest) list;
 Coroutine *co; /* owner, used for deadlock detection */
-CoQueue wait_queue; /* coroutines blocked on this request */
-
 struct BdrvTrackedRequest *waiting_for;
+
+/* Protected by BlockDriverState's reqs_lock.  */
+CoQueue wait_queue; /* coroutines blocked on this request */
 } BdrvTrackedRequest;
 
 struct BlockDriver {
@@ -626,11 +627,15 @@ struct BlockDriverState {
 int quiesce_counter;
 unsigned int write_gen;   /* Current data generation */
 
-/* Protected by reqs_lock.  */
+/* Writes are protected by reqs_list_write_lock.  Reads take
+ * reqs_lock so that removals can easily synchronize with walks.
+ */
 QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
 CoQueue flush_queue;  /* Serializing flush queue */
 bool active_flush_req;/* Flush request in flight? */
 unsigned int flushed_gen; /* Flushed write generation */
+
+QemuSpin reqs_list_write_lock;
 CoMutex reqs_lock;
 };
 
-- 
2.9.3

Re: [Qemu-block] [PATCH v13 19/20] file-posix: Add image locking in perm operations

[Kevin Wolf]

Am 20.04.2017 um 09:52 hat Fam Zheng geschrieben:
> virtlockd in libvirt locks the first byte, so we start looking at the
> file bytes from 0x10.
> 
> The complication is in the transactional interface.  To make the reopen
> logic managable, and allow better reuse, the code is internally
> organized with a table from old mode to the new one.
> 
> Signed-off-by: Fam Zheng 

Looking at the very early patches in this series, I think it quickly
becomes obvious that we need to discuss one thing first:

> +static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t 
> shared,
> +  Error **errp)
> +{
> +bool is_shared;
> +BDRVRawState *s = bs->opaque;
> +
> +if (!RAW_LOCK_SUPPORTED) {
> +return 0;
> +}
> +if (s->lock_update) {
> +/* Override the previously stashed update. */
> +g_free(s->lock_update);
> +s->lock_update = NULL;
> +}
> +is_shared = !(perm & BLK_PERM_CONSISTENT_READ) && (shared & 
> BLK_PERM_WRITE);

Why do you check BLK_PERM_CONSISTENT_READ? The locks that we said we
would take on the image file represent BLK_PERM_WRITE, both in perm and
in shared, so this is what they should be checked against. Opening the
image in another process is fine if BLK_PERM_WRITE is set in shared,
even if BLK_PERM_CONSISTENT_READ is also set in perm.

BLK_PERM_CONSISTENT_READ is for cases where the contents of an image
is inherently invalid, not just because of a concurrent writer that we
might not be aware of, but because the image just doesn't makes sense on
it own. It may make sense as part of larger backing chain, though (the
only place where we clear the flag is for intermediate nodes in the
commit job). This semantics is more or less separate from what we want
to achieve here.

Of course, if we wanted, I guess we could individually map all 64
bits of each perm and shared to bytes to be locked in the file, so that
all permissions would be shared between qemu instances. That's probably
not worth the effort though.

And even if we did that, most likely you still wouldn't need any special
exceptions for BLK_PERM_CONSISTENT_READ, because it is always shared
except when one process wants to run a commit job - and for a commit
job, making sure that nobody else uses the image would probably be
right.

So if we really treat the file system level locks just as a mapping of
BLK_PERM_WRITE, things should become a bit easier in the early patches
of this series.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v13 02/20] block: Drop consistent read perm if opened unsafe

[Fam Zheng]

On Thu, 04/20 12:58, Kevin Wolf wrote:
> Am 20.04.2017 um 09:52 hat Fam Zheng geschrieben:
> > Signed-off-by: Fam Zheng 
> > ---
> >  block.c | 12 +---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 1fbbb8d..f5182d8 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1722,9 +1722,15 @@ void bdrv_format_default_perms(BlockDriverState *bs, 
> > BdrvChild *c,
> >  }
> >  
> >  /* bs->file always needs to be consistent because of the metadata. 
> > We
> > - * can never allow other users to resize or write to it. */
> > -perm |= BLK_PERM_CONSISTENT_READ;
> > -shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> > + * cannot allow other users to resize or write to it unless the 
> > caller
> > + * explicitly expects unsafe readings. */
> > +if (!(bdrv_get_flags(bs) & BDRV_O_UNSAFE_READ)) {
> 
> We have already spent considerable time to get rid of flags and instead
> convert them into options passed in the QDict, so that they become
> configurable with things like blockdev-add. Adding new flags potentially
> moves in the opposite direction, so we have to be careful there.
> 
> I would be okay with patch 1, because in this case it's basically just a
> shortcut for callers of blk_new_open(), which is fine. As soon as we
> start querying the flag later and even rely on it being inherited, like
> in this patch, I think it becomes a problem.
> 
> So if we need the flag in all nodes, can we make it an option that is
> parsed in bdrv_open_common() into a bool bs->unsafe_read and inherited
> explicitly in bdrv_inherited_options() and bdrv_backing_options()?

OK, I knew new flags are bad, but this is perhaps what I was missing, as an
alternative.

> 
> > +perm |= BLK_PERM_CONSISTENT_READ;
> > +shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> > +} else {
> > +perm &= ~BLK_PERM_CONSISTENT_READ;
> > +shared |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
> > +}
> 
> I'm not completely sure why we would be interested in CONSISTENT_READ
> anyway, isn't allowing shared writes what we really need? (Which you
> already do here in addition to dropping CONSISTENT_READ, without it
> being mentioned in the commit message.)

I think taking external programs into account, CONSISTENT_READ and shared write
are related: if another writer can modify file, our view is not consistent.
That's why I handle them together.

> 
> Also, another thought: Being only at the start of the series, I'm not
> sure what this will be used for, but can we make sure that unsafe_read
> is only set if the image is opened read-only? If this is for the
> libguestfs use case, this restriction should be fine.

I guess you are right. I will give a try to your QDict idea, and only apply it
if read-only.

Fam

Re: [Qemu-block] [PATCH v13 02/20] block: Drop consistent read perm if opened unsafe

[Kevin Wolf]

Am 20.04.2017 um 09:52 hat Fam Zheng geschrieben:
> Signed-off-by: Fam Zheng 
> ---
>  block.c | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 1fbbb8d..f5182d8 100644
> --- a/block.c
> +++ b/block.c
> @@ -1722,9 +1722,15 @@ void bdrv_format_default_perms(BlockDriverState *bs, 
> BdrvChild *c,
>  }
>  
>  /* bs->file always needs to be consistent because of the metadata. We
> - * can never allow other users to resize or write to it. */
> -perm |= BLK_PERM_CONSISTENT_READ;
> -shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> + * cannot allow other users to resize or write to it unless the 
> caller
> + * explicitly expects unsafe readings. */
> +if (!(bdrv_get_flags(bs) & BDRV_O_UNSAFE_READ)) {

We have already spent considerable time to get rid of flags and instead
convert them into options passed in the QDict, so that they become
configurable with things like blockdev-add. Adding new flags potentially
moves in the opposite direction, so we have to be careful there.

I would be okay with patch 1, because in this case it's basically just a
shortcut for callers of blk_new_open(), which is fine. As soon as we
start querying the flag later and even rely on it being inherited, like
in this patch, I think it becomes a problem.

So if we need the flag in all nodes, can we make it an option that is
parsed in bdrv_open_common() into a bool bs->unsafe_read and inherited
explicitly in bdrv_inherited_options() and bdrv_backing_options()?

> +perm |= BLK_PERM_CONSISTENT_READ;
> +shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> +} else {
> +perm &= ~BLK_PERM_CONSISTENT_READ;
> +shared |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
> +}

I'm not completely sure why we would be interested in CONSISTENT_READ
anyway, isn't allowing shared writes what we really need? (Which you
already do here in addition to dropping CONSISTENT_READ, without it
being mentioned in the commit message.)

Also, another thought: Being only at the start of the series, I'm not
sure what this will be used for, but can we make sure that unsafe_read
is only set if the image is opened read-only? If this is for the
libguestfs use case, this restriction should be fine.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v13 04/20] qemu-img: Add --unsafe-read option to subcommands

[Fam Zheng]

On Thu, 04/20 12:20, Kevin Wolf wrote:
> Am 20.04.2017 um 09:52 hat Fam Zheng geschrieben:
> > Signed-off-by: Fam Zheng 
> 
> This one conflicts with my block-next branch, probably Peter's
> simplification of img_convert().

Probably, thanks for pointing out. I've tried to resolve it and it's
straightforward. I'll send v14 basing off your block-next, once you've done
reviewing.

For now, the rebased branch, including the two small fixes, is pushed to my
github branch:

git://github.com/famz/qemu.git image-lock

Fam

Re: [Qemu-block] [PATCH v13 04/20] qemu-img: Add --unsafe-read option to subcommands

[Kevin Wolf]

Am 20.04.2017 um 09:52 hat Fam Zheng geschrieben:
> Signed-off-by: Fam Zheng 

This one conflicts with my block-next branch, probably Peter's
simplification of img_convert().

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v13 00/20] block: Image locking series

[Fam Zheng]

On Thu, 04/20 12:03, Kevin Wolf wrote:
> Am 20.04.2017 um 10:32 hat Fam Zheng geschrieben:
> > > /var/tmp/patchew-tester-tmp-7vbaovju/src/block/file-posix.c: In function 
> > > ‘raw_handle_lock_update’:
> > > /var/tmp/patchew-tester-tmp-7vbaovju/src/block/file-posix.c:1117:17: 
> > > error: ‘lock_fd’ may be used uninitialized in this function 
> > > [-Werror=maybe-uninitialized]
> > >  qemu_close(lock_fd);
> > >  ^~~
> > > cc1: all warnings being treated as errors
> > > /var/tmp/patchew-tester-tmp-7vbaovju/src/rules.mak:69: recipe for target 
> > > 'block/file-posix.o' failed
> > 
> > False positive. lock_fd is initialized in a "if (op == RAW_LT_PREPARE)" 
> > branch
> > above, and we are in a "case RAW_LT_PREPARE" switch branch too.
> 
> Even if it's a false positive, obviously we still need to work around
> the build failure.

Sure, easy to do. I'll initialize the variable.

Fam

Re: [Qemu-block] [Qemu-devel] [PATCH v13 00/20] block: Image locking series

[Kevin Wolf]

Am 20.04.2017 um 10:32 hat Fam Zheng geschrieben:
> > /var/tmp/patchew-tester-tmp-7vbaovju/src/block/file-posix.c: In function 
> > ‘raw_handle_lock_update’:
> > /var/tmp/patchew-tester-tmp-7vbaovju/src/block/file-posix.c:1117:17: error: 
> > ‘lock_fd’ may be used uninitialized in this function 
> > [-Werror=maybe-uninitialized]
> >  qemu_close(lock_fd);
> >  ^~~
> > cc1: all warnings being treated as errors
> > /var/tmp/patchew-tester-tmp-7vbaovju/src/rules.mak:69: recipe for target 
> > 'block/file-posix.o' failed
> 
> False positive. lock_fd is initialized in a "if (op == RAW_LT_PREPARE)" branch
> above, and we are in a "case RAW_LT_PREPARE" switch branch too.

Even if it's a false positive, obviously we still need to work around
the build failure.

Kevin

Re: [Qemu-block] [PATCH] qemu-img: use blk_co_pwrite_zeroes for zero sectors when compressed

[Kevin Wolf]

Am 20.04.2017 um 10:38 hat jemmy858...@gmail.com geschrieben:
> From: Lidong Chen 
> 
> when the buffer is zero, blk_co_pwrite_zeroes is more effectively than
> blk_co_pwritev with BDRV_REQ_WRITE_COMPRESSED. this patch can reduces
> the time when converts the qcow2 image with lots of zero.
> 
> Signed-off-by: Lidong Chen 

Good catch, using blk_co_pwrite_zeroes() makes sense even for compressed
images.

> diff --git a/qemu-img.c b/qemu-img.c
> index b220cf7..0256539 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1675,13 +1675,20 @@ static int coroutine_fn 
> convert_co_write(ImgConvertState *s, int64_t sector_num,
>   * write if the buffer is completely zeroed and we're allowed to
>   * keep the target sparse. */
>  if (s->compressed) {
> -if (s->has_zero_init && s->min_sparse &&
> -buffer_is_zero(buf, n * BDRV_SECTOR_SIZE))
> -{
> -assert(!s->target_has_backing);
> -break;
> +if (buffer_is_zero(buf, n * BDRV_SECTOR_SIZE)) {
> +if (s->has_zero_init && s->min_sparse) {
> +assert(!s->target_has_backing);
> +break;
> +} else {
> +ret = blk_co_pwrite_zeroes(s->target,
> +   sector_num << BDRV_SECTOR_BITS,
> +   n << BDRV_SECTOR_BITS, 0);
> +if (ret < 0) {
> +return ret;
> +}
> +break;
> +}
>  }

If s->min_sparse == 0, we may neither skip the write not use
blk_co_pwrite_zeroes(), because this requires actual full allocation
with explicit zero sectors.

Of course, if you fix this, what you end up with here is a duplicate of
the code path for non-compressed images. The remaining difference seems
to be the BDRV_REQ_WRITE_COMPRESSED flag and buffer_is_zero() vs.
is_allocated_sectors_min() (because uncompressed clusters can be written
partially, but compressed clusters can't).

So I suppose that instead of just fixing the above bug, we could actually
mostly unify the two code paths, if you want to have a try at it.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v13 19/20] file-posix: Add image locking in perm operations

[Fam Zheng]

On Thu, 04/20 15:52, Fam Zheng wrote:
> virtlockd in libvirt locks the first byte, so we start looking at the
> file bytes from 0x10.
> 
> The complication is in the transactional interface.  To make the reopen
> logic managable, and allow better reuse, the code is internally
> organized with a table from old mode to the new one.
> 
> Signed-off-by: Fam Zheng 
> ---
>  block/file-posix.c | 744 
> -
>  1 file changed, 741 insertions(+), 3 deletions(-)

Need to squash this in to fix the patchew make check error on centos 6:

diff --git a/block/file-posix.c b/block/file-posix.c
index b85ac9c..f1563ae 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1069,7 +1069,7 @@ static int raw_handle_lock_update(BlockDriverState *bs,
 int lock_fd;
 
 if (!RAW_LOCK_SUPPORTED) {
-return 0;
+goto cleanup;
 }
 
 if (bdrv_get_flags(bs) & BDRV_O_INACTIVE) {

Re: [Qemu-block] [PATCH v2] qemu-img: check bs_n when use old style option

[858585 jemmy]

On Thu, Apr 20, 2017 at 5:33 PM, Kevin Wolf  wrote:
> Am 20.04.2017 um 11:19 hat jemmy858...@gmail.com geschrieben:
>> From: Lidong Chen 
>>
>> When use old style option like -o backing_file, img_convert
>> continue run when bs_n > 1, this patch fix this bug.
>>
>> Signed-off-by: Lidong Chen 
>
> I think this is a duplicate of Max' "[PATCH for-2.10 3/3]
> qemu-img/convert: Move bs_n > 1 && -B check down".
ok, thanks.

>
> Also, -B is the old-style option, not -o backing_file.
>
> Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v13 00/20] block: Image locking series

[Fam Zheng]

On Thu, 04/20 01:39, no-re...@patchew.org wrote:
> test-replication: /tmp/qemu-test/src/block/file-posix.c:1168: 
> raw_init_lock_update: Assertion `!s->lock_update' failed.
> GTester: last random seed: R02Scba16429ff5a192bda9c88444de57b96
> test-replication: /tmp/qemu-test/src/block/file-posix.c:1168: 
> raw_init_lock_update: Assertion `!s->lock_update' failed.
> GTester: last random seed: R02S71ef34cf124dcfa2d2816a5a05a83ee9
> test-replication: /tmp/qemu-test/src/block/file-posix.c:1168: 
> raw_init_lock_update: Assertion `!s->lock_update' failed.
> GTester: last random seed: R02S6296241250c2052edfd7157c5417b65f
> test-replication: /tmp/qemu-test/src/block/file-posix.c:1168: 
> raw_init_lock_update: Assertion `!s->lock_update' failed.
> GTester: last random seed: R02Sd7e01803d3bd42deab2a0a3015bcc52f

Good catch. Centos 6 doesn't have the OFD lock, and the code path missed a
cleanup. Will fix this.

Fam

Re: [Qemu-block] [PATCH v2] qemu-img: check bs_n when use old style option

[Kevin Wolf]

Am 20.04.2017 um 11:19 hat jemmy858...@gmail.com geschrieben:
> From: Lidong Chen 
> 
> When use old style option like -o backing_file, img_convert
> continue run when bs_n > 1, this patch fix this bug.
> 
> Signed-off-by: Lidong Chen 

I think this is a duplicate of Max' "[PATCH for-2.10 3/3]
qemu-img/convert: Move bs_n > 1 && -B check down".

Also, -B is the old-style option, not -o backing_file.

Kevin

Re: [Qemu-block] [PATCH v2] qemu-img: check bs_n when use old style option

[858585 jemmy]

test result for this patch:
qemu-img convert -c -p -o
backing_file=/baseimage/img2015122818606660/img2015122818606660.qcow2
-O qcow2 /data/img2017041611162809.qcow2
/data/img2017041611162809.qcow2 /mnt/img2017041611162809_zip_new.qcow2
qemu-img: Specifying backing image makes no sense when concatenating
multiple input images

qemu-img convert -c -p -B
baseimage/img2015122818606660/img2015122818606660.qcow2 -O qcow2
/data/img2017041611162809.qcow2 /data/img2017041611162809.qcow2
/mnt/img2017041611162809_zip_new.qcow2
qemu-img: Specifying backing image makes no sense when concatenating
multiple input images


On Thu, Apr 20, 2017 at 5:19 PM,   wrote:
> From: Lidong Chen 
>
> When use old style option like -o backing_file, img_convert
> continue run when bs_n > 1, this patch fix this bug.
>
> Signed-off-by: Lidong Chen 
> ---
> v2 changelog:
> avoid duplicating code.
> ---
>  qemu-img.c | 15 +++
>  1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index b220cf7..b4d9255 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2108,14 +2108,6 @@ static int img_convert(int argc, char **argv)
>  error_exit("Must specify image file name");
>  }
>
> -
> -if (bs_n > 1 && out_baseimg) {
> -error_report("-B makes no sense when concatenating multiple input "
> - "images");
> -ret = -1;
> -goto out;
> -}
> -
>  src_flags = 0;
>  ret = bdrv_parse_cache_mode(src_cache, _flags, _writethrough);
>  if (ret < 0) {
> @@ -2225,6 +2217,13 @@ static int img_convert(int argc, char **argv)
>  out_baseimg = out_baseimg_param;
>  }
>
> +if (bs_n > 1 && out_baseimg) {
> +error_report("Specifying backing image makes no sense when "
> + "concatenating multiple input images");
> +ret = -1;
> +goto out;
> +}
> +
>  /* Check if compression is supported */
>  if (compress) {
>  bool encryption =
> --
> 1.8.3.1
>

[Qemu-block] [PATCH v2] qemu-img: check bs_n when use old style option

[jemmy858585]

From: Lidong Chen 

When use old style option like -o backing_file, img_convert
continue run when bs_n > 1, this patch fix this bug.

Signed-off-by: Lidong Chen 
---
v2 changelog:
avoid duplicating code.
---
 qemu-img.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index b220cf7..b4d9255 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2108,14 +2108,6 @@ static int img_convert(int argc, char **argv)
 error_exit("Must specify image file name");
 }
 
-
-if (bs_n > 1 && out_baseimg) {
-error_report("-B makes no sense when concatenating multiple input "
- "images");
-ret = -1;
-goto out;
-}
-
 src_flags = 0;
 ret = bdrv_parse_cache_mode(src_cache, _flags, _writethrough);
 if (ret < 0) {
@@ -2225,6 +2217,13 @@ static int img_convert(int argc, char **argv)
 out_baseimg = out_baseimg_param;
 }
 
+if (bs_n > 1 && out_baseimg) {
+error_report("Specifying backing image makes no sense when "
+ "concatenating multiple input images");
+ret = -1;
+goto out;
+}
+
 /* Check if compression is supported */
 if (compress) {
 bool encryption =
-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] [PATCH] sheepdog: Set error when connection fails

[Philippe Mathieu-Daudé]

On 04/20/2017 01:00 AM, Fam Zheng wrote:

Signed-off-by: Fam Zheng 


Reviewed-by: Philippe Mathieu-Daudé 


---
 block/sheepdog.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index fb9203e..7e889ee 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error 
**errp)
 qemu_set_nonblock(fd);
 } else {
 fd = -EIO;
+error_setg(errp, "Failed to connect to sheepdog server");
 }

 return fd;

Re: [Qemu-block] [PATCH] qemu-img: use blk_co_pwrite_zeroes for zero sectors when compressed

[858585 jemmy]

On Thu, Apr 20, 2017 at 4:38 PM,   wrote:
> From: Lidong Chen 
>
> when the buffer is zero, blk_co_pwrite_zeroes is more effectively than
> blk_co_pwritev with BDRV_REQ_WRITE_COMPRESSED. this patch can reduces
> the time when converts the qcow2 image with lots of zero.
>

the original qcow2 file which have lots of cluster unallocated:
[root]# qemu-img info /mnt/img2016111016860868_old.qcow2
image: /mnt/img2016111016860868_old.qcow2
file format: qcow2
virtual size: 20G (21474836480 bytes)
disk size: 214M (224460800 bytes)
cluster_size: 65536
backing file: /baseimage/img2015122818606660/img2015122818606660.qcow2

the time used for qemu-img convert:
[root ~]# time /root/kvm/bin/qemu-img convert -c -p -o
backing_file=/baseimage/img2015122818606660/img2015122818606660.qcow2
-O
qcow2 /mnt/img2016111016860868.qcow2 /mnt/img2016111016860868_old.qcow2
(100.00/100%)
real0m29.456s
user0m29.345s
sys 0m0.481s

run dd if=/dev/zero of=./test bs=65536 in guest os
then convert again.

before apply this patch:
[root~]# time /root/kvm/bin/qemu-img convert -c -p -o
backing_file=/baseimage/img2015122818606660/img2015122818606660.qcow2
-O qcow2 /mnt/img2016111016860868.qcow2 /mnt/img2016111016860868_new.qcow2
(100.00/100%)

real5m35.617s
user5m33.417s
sys 0m10.699s

after apply this patch:
[root~]# time /root/kvm/bin/qemu-img convert -c -p -o
backing_file=/baseimage/img2015122818606660/img2015122818606660.qcow2
-O
qcow2 /mnt/img2016111016860868.qcow2 /mnt/img2016111016860868_new1.qcow2
(100.00/100%)

real0m51.189s
user0m35.239s
sys 0m14.251s

the time reduce from 5m35.617s to 0m51.189s.

[root ]# ll /mnt/img2016111016860868* -h
-rw-r--r-- 1 root root 254M Apr 20 14:50 /mnt/img2016111016860868_new.qcow2
-rw-r--r-- 1 root root 232M Apr 20 15:27 /mnt/img2016111016860868_new1.qcow2

the size reduce from 254M to 232M.

> Signed-off-by: Lidong Chen 
> ---
>  qemu-img.c | 19 +--
>  1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index b220cf7..0256539 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1675,13 +1675,20 @@ static int coroutine_fn 
> convert_co_write(ImgConvertState *s, int64_t sector_num,
>   * write if the buffer is completely zeroed and we're allowed to
>   * keep the target sparse. */
>  if (s->compressed) {
> -if (s->has_zero_init && s->min_sparse &&
> -buffer_is_zero(buf, n * BDRV_SECTOR_SIZE))
> -{
> -assert(!s->target_has_backing);
> -break;
> +if (buffer_is_zero(buf, n * BDRV_SECTOR_SIZE)) {
> +if (s->has_zero_init && s->min_sparse) {
> +assert(!s->target_has_backing);
> +break;
> +} else {
> +ret = blk_co_pwrite_zeroes(s->target,
> +   sector_num << BDRV_SECTOR_BITS,
> +   n << BDRV_SECTOR_BITS, 0);
> +if (ret < 0) {
> +return ret;
> +}
> +break;
> +}
>  }
> -
>  iov.iov_base = buf;
>  iov.iov_len = n << BDRV_SECTOR_BITS;
>  qemu_iovec_init_external(, , 1);
> --
> 1.8.3.1
>

Re: [Qemu-block] [Qemu-devel] [PATCH v13 00/20] block: Image locking series

[Fam Zheng]

On Thu, 04/20 01:40, no-re...@patchew.org wrote:
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:

All long lines (>90 cols). Intended. Wrapping will make it less readable.

Fam

Re: [Qemu-block] [Qemu-devel] [PATCH v13 00/20] block: Image locking series

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20170420075237.18219-1-f...@redhat.com
Subject: [Qemu-devel] [PATCH v13 00/20] block: Image locking series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
2d5d050 tests: Add test-image-lock
a0d4c3c file-posix: Add image locking in perm operations
17c490b osdep: Add qemu_lock_fd and qemu_unlock_fd
2383a28 block: Workaround drive-backup sync=none for image locking
63541e0 tests: Disable image lock in test-replication
354af7e file-posix: Add 'locking' option
9cf4312 tests: Use null-co:// instead of /dev/null as the dummy image
4f5bbc4 iotests: 172: Use separate images for multiple devices
76ff84d iotests: 091: Quit QEMU before checking image
82cf7cc iotests: 087: Don't attach test image twice
b9329f3 iotests: 085: Avoid image locking conflict
8a52859 iotests: 055: Don't attach the target image already for drive-backup
4159096 iotests: 046: Prepare for image locking
a8af152 iotests: 030: Prepare for image locking
1137b91 qemu-io: Add --unsafe-read option
d66cf70 qemu-img: Update documentation for --unsafe-read
7f30dc1 qemu-img: Add --unsafe-read option to subcommands
d392aa4 block: Don't require BLK_PERM_CONSISTENT_READ when unsafe open
03a43f3 block: Drop consistent read perm if opened unsafe
a45035b block: Introduce BDRV_O_UNSAFE_READ

=== OUTPUT BEGIN ===
Checking PATCH 1/20: block: Introduce BDRV_O_UNSAFE_READ...
Checking PATCH 2/20: block: Drop consistent read perm if opened unsafe...
Checking PATCH 3/20: block: Don't require BLK_PERM_CONSISTENT_READ when unsafe 
open...
Checking PATCH 4/20: qemu-img: Add --unsafe-read option to subcommands...
Checking PATCH 5/20: qemu-img: Update documentation for --unsafe-read...
Checking PATCH 6/20: qemu-io: Add --unsafe-read option...
WARNING: line over 80 characters
#140: FILE: qemu-io.c:621:
+if (openfile(argv[optind], flags, writethrough, unsafe_read, 
opts)) {

total: 0 errors, 1 warnings, 117 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 7/20: iotests: 030: Prepare for image locking...
Checking PATCH 8/20: iotests: 046: Prepare for image locking...
Checking PATCH 9/20: iotests: 055: Don't attach the target image already for 
drive-backup...
Checking PATCH 10/20: iotests: 085: Avoid image locking conflict...
Checking PATCH 11/20: iotests: 087: Don't attach test image twice...
Checking PATCH 12/20: iotests: 091: Quit QEMU before checking image...
Checking PATCH 13/20: iotests: 172: Use separate images for multiple devices...
Checking PATCH 14/20: tests: Use null-co:// instead of /dev/null as the dummy 
image...
WARNING: line over 80 characters
#93: FILE: tests/virtio-scsi-test.c:198:
+qs = qvirtio_scsi_start("-drive 
id=drv1,if=none,file=null-co://,format=raw");

total: 0 errors, 1 warnings, 56 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 15/20: file-posix: Add 'locking' option...
Checking PATCH 16/20: tests: Disable image lock in test-replication...
Checking PATCH 17/20: block: Workaround drive-backup sync=none for image 
locking...
Checking PATCH 18/20: osdep: Add qemu_lock_fd and qemu_unlock_fd...
Checking PATCH 19/20: file-posix: Add image locking in perm operations...
WARNING: line over 80 characters
#119: FILE: block/file-posix.c:451:
+error_setg_errno(errp, -ret, "Failed to lock write byte 
exclusively");

WARNING: line over 80 characters
#261: FILE: block/file-posix.c:743:
+ret = qemu_lock_fd(new_lock_fd, RAW_LOCK_BYTE_NO_OTHER_WRITER, 1, 
false);

WARNING: line over 80 characters
#268: FILE: block/file-posix.c:750:
+error_setg_errno(errp, -ret, "Failed to unlock old fd (share 
byte)");

WARNING: line over 80 characters
#273: FILE: block/file-posix.c:755:
+error_setg_errno(errp, -ret, "Failed to upgrade new fd (share 
byte)");

WARNING: line over 80 characters
#279: FILE: block/file-posix.c:761:
+error_setg_errno(errp, -ret, "Failed to unlock new fd (share 
byte)");

WARNING: line over 80 characters
#284: FILE: block/file-posix.c:766:
+error_setg_errno(errp, -ret, "Failed to downgrade new fd (write 
byte)");

WARNING: line over 80 

Re: [Qemu-block] [Qemu-devel] [PATCH v13 00/20] block: Image locking series

[no-reply]

Hi,

This series failed automatic build test. Please find the testing commands and
their output below. If you have docker installed, you can probably reproduce it
locally.

Type: series
Message-id: 20170420075237.18219-1-f...@redhat.com
Subject: [Qemu-devel] [PATCH v13 00/20] block: Image locking series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=8
make docker-test-quick@centos6
make docker-test-mingw@fedora
make docker-test-build@min-glib
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
2d5d050 tests: Add test-image-lock
a0d4c3c file-posix: Add image locking in perm operations
17c490b osdep: Add qemu_lock_fd and qemu_unlock_fd
2383a28 block: Workaround drive-backup sync=none for image locking
63541e0 tests: Disable image lock in test-replication
354af7e file-posix: Add 'locking' option
9cf4312 tests: Use null-co:// instead of /dev/null as the dummy image
4f5bbc4 iotests: 172: Use separate images for multiple devices
76ff84d iotests: 091: Quit QEMU before checking image
82cf7cc iotests: 087: Don't attach test image twice
b9329f3 iotests: 085: Avoid image locking conflict
8a52859 iotests: 055: Don't attach the target image already for drive-backup
4159096 iotests: 046: Prepare for image locking
a8af152 iotests: 030: Prepare for image locking
1137b91 qemu-io: Add --unsafe-read option
d66cf70 qemu-img: Update documentation for --unsafe-read
7f30dc1 qemu-img: Add --unsafe-read option to subcommands
d392aa4 block: Don't require BLK_PERM_CONSISTENT_READ when unsafe open
03a43f3 block: Drop consistent read perm if opened unsafe
a45035b block: Introduce BDRV_O_UNSAFE_READ

=== OUTPUT BEGIN ===
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into '/var/tmp/patchew-tester-tmp-s4d5iont/src/dtc'...
Submodule path 'dtc': checked out '558cd81bdd432769b59bff01240c44f82cfb1a9d'
  BUILD   centos6
make[1]: Entering directory '/var/tmp/patchew-tester-tmp-s4d5iont/src'
  ARCHIVE qemu.tgz
  ARCHIVE dtc.tgz
  COPYRUNNER
RUN test-quick in qemu:centos6 
Packages installed:
SDL-devel-1.2.14-7.el6_7.1.x86_64
ccache-3.1.6-2.el6.x86_64
epel-release-6-8.noarch
gcc-4.4.7-17.el6.x86_64
git-1.7.1-4.el6_7.1.x86_64
glib2-devel-2.28.8-5.el6.x86_64
libfdt-devel-1.4.0-1.el6.x86_64
make-3.81-23.el6.x86_64
package g++ is not installed
pixman-devel-0.32.8-1.el6.x86_64
tar-1.23-15.el6_8.x86_64
zlib-devel-1.2.3-29.el6.x86_64

Environment variables:
PACKAGES=libfdt-devel ccache tar git make gcc g++ zlib-devel 
glib2-devel SDL-devel pixman-devel epel-release
HOSTNAME=aa92988b7e2c
TERM=xterm
MAKEFLAGS= -j8
HISTSIZE=1000
J=8
USER=root
CCACHE_DIR=/var/tmp/ccache
EXTRA_CONFIGURE_OPTS=
V=
SHOW_ENV=1
MAIL=/var/spool/mail/root
PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
PWD=/
LANG=en_US.UTF-8
TARGET_LIST=
HISTCONTROL=ignoredups
SHLVL=1
HOME=/root
TEST_DIR=/tmp/qemu-test
LOGNAME=root
LESSOPEN=||/usr/bin/lesspipe.sh %s
FEATURES= dtc
DEBUG=
G_BROKEN_FILENAMES=1
CCACHE_HASHDIR=
_=/usr/bin/env

Configure options:
--enable-werror --target-list=x86_64-softmmu,aarch64-softmmu 
--prefix=/var/tmp/qemu-build/install
No C++ compiler available; disabling C++ specific optional code
Install prefix/var/tmp/qemu-build/install
BIOS directory/var/tmp/qemu-build/install/share/qemu
binary directory  /var/tmp/qemu-build/install/bin
library directory /var/tmp/qemu-build/install/lib
module directory  /var/tmp/qemu-build/install/lib/qemu
libexec directory /var/tmp/qemu-build/install/libexec
include directory /var/tmp/qemu-build/install/include
config directory  /var/tmp/qemu-build/install/etc
local state directory   /var/tmp/qemu-build/install/var
Manual directory  /var/tmp/qemu-build/install/share/man
ELF interp prefix /usr/gnemul/qemu-%M
Source path   /tmp/qemu-test/src
C compilercc
Host C compiler   cc
C++ compiler  
Objective-C compiler cc
ARFLAGS   rv
CFLAGS-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g 
QEMU_CFLAGS   -I/usr/include/pixman-1   -I$(SRC_PATH)/dtc/libfdt -pthread 
-I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include   -fPIE -DPIE -m64 -mcx16 
-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv  -Wendif-labels 
-Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security 
-Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration 
-Wold-style-definition -Wtype-limits -fstack-protector-all
LDFLAGS   -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -pie -m64 -g 
make  make
install   install
pythonpython -B
smbd  /usr/sbin/smbd
module supportno
host CPU  x86_64
host big endian   no
target list   x86_64-softmmu aarch64-softmmu
tcg 

[Qemu-block] [PATCH] qemu-img: use blk_co_pwrite_zeroes for zero sectors when compressed

[jemmy858585]

From: Lidong Chen 

when the buffer is zero, blk_co_pwrite_zeroes is more effectively than
blk_co_pwritev with BDRV_REQ_WRITE_COMPRESSED. this patch can reduces
the time when converts the qcow2 image with lots of zero.

Signed-off-by: Lidong Chen 
---
 qemu-img.c | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index b220cf7..0256539 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1675,13 +1675,20 @@ static int coroutine_fn 
convert_co_write(ImgConvertState *s, int64_t sector_num,
  * write if the buffer is completely zeroed and we're allowed to
  * keep the target sparse. */
 if (s->compressed) {
-if (s->has_zero_init && s->min_sparse &&
-buffer_is_zero(buf, n * BDRV_SECTOR_SIZE))
-{
-assert(!s->target_has_backing);
-break;
+if (buffer_is_zero(buf, n * BDRV_SECTOR_SIZE)) {
+if (s->has_zero_init && s->min_sparse) {
+assert(!s->target_has_backing);
+break;
+} else {
+ret = blk_co_pwrite_zeroes(s->target,
+   sector_num << BDRV_SECTOR_BITS,
+   n << BDRV_SECTOR_BITS, 0);
+if (ret < 0) {
+return ret;
+}
+break;
+}
 }
-
 iov.iov_base = buf;
 iov.iov_len = n << BDRV_SECTOR_BITS;
 qemu_iovec_init_external(, , 1);
-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] [PATCH v13 00/20] block: Image locking series

[Fam Zheng]

> /var/tmp/patchew-tester-tmp-7vbaovju/src/block/file-posix.c: In function 
> ‘raw_handle_lock_update’:
> /var/tmp/patchew-tester-tmp-7vbaovju/src/block/file-posix.c:1117:17: error: 
> ‘lock_fd’ may be used uninitialized in this function 
> [-Werror=maybe-uninitialized]
>  qemu_close(lock_fd);
>  ^~~
> cc1: all warnings being treated as errors
> /var/tmp/patchew-tester-tmp-7vbaovju/src/rules.mak:69: recipe for target 
> 'block/file-posix.o' failed

False positive. lock_fd is initialized in a "if (op == RAW_LT_PREPARE)" branch
above, and we are in a "case RAW_LT_PREPARE" switch branch too.

Fam

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-img: check bs_n when use old style option

[858585 jemmy]

On Thu, Apr 20, 2017 at 4:11 PM, Fam Zheng  wrote:
> On Thu, 04/20 15:59, 858585 jemmy wrote:
>> On Thu, Apr 20, 2017 at 3:51 PM, Fam Zheng  wrote:
>> > On Thu, 04/20 12:04, jemmy858...@gmail.com wrote:
>> >> From: Lidong Chen 
>> >>
>> >> When use old style option like -o backing_file, img_convert
>> >> continue run when bs_n > 1, this patch fix this bug.
>> >>
>> >> Signed-off-by: Lidong Chen 
>> >> ---
>> >>  qemu-img.c | 7 +++
>> >>  1 file changed, 7 insertions(+)
>> >>
>> >> diff --git a/qemu-img.c b/qemu-img.c
>> >> index b220cf7..c673aef 100644
>> >> --- a/qemu-img.c
>> >> +++ b/qemu-img.c
>> >> @@ -2225,6 +2225,13 @@ static int img_convert(int argc, char **argv)
>> >>  out_baseimg = out_baseimg_param;
>> >>  }
>> >>
>> >> +if (bs_n > 1 && out_baseimg) {
>> >> +error_report("-B makes no sense when concatenating multiple 
>> >> input "
>> >> + "images");
>> >> +ret = -1;
>> >> +goto out;
>> >> +}
>> >> +
>> >>  /* Check if compression is supported */
>> >>  if (compress) {
>> >>  bool encryption =
>> >> --
>> >> 1.8.3.1
>> >>
>> >>
>> >
>> > Is this essentially the same as the check a few lines above:
>> >
>> > ...
>> > if (bs_n < 1) {
>> > error_exit("Must specify image file name");
>> > }
>> >
>> >
>> > if (bs_n > 1 && out_baseimg) {
>> > error_report("-B makes no sense when concatenating multiple input "
>> >  "images");
>> > ret = -1;
>> > goto out;
>> > }
>> >
>> > src_flags = 0;
>> > ret = bdrv_parse_cache_mode(src_cache, _flags, _writethrough);
>> > if (ret < 0) {
>> > error_report("Invalid source cache option: %s", src_cache);
>> > goto out;
>> > }
>> > ...
>> >
>> > How about moving that down?
>> moving that down is ok.
>> but will exit later if use -B option.
>> which way do you think better?
>
> Exiting later is not a problem, I assume? And it's better to avoid duplicating
> code if possible.
>
> BTW if you do that way, it's better to "s/-B/Specifying backing image/" in the
> error message (to be compatible with -o backing_file syntax).
Thanks. i will submit this patch again.


>
> Fam

Re: [Qemu-block] [Qemu-devel] [PATCH v13 00/20] block: Image locking series

[no-reply]

Hi,

This series failed build test on s390x host. Please find the details below.

Message-id: 20170420075237.18219-1-f...@redhat.com
Subject: [Qemu-devel] [PATCH v13 00/20] block: Image locking series
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e
echo "=== ENV ==="
env
echo "=== PACKAGES ==="
rpm -qa
echo "=== TEST BEGIN ==="
CC=$HOME/bin/cc
INSTALL=$PWD/install
BUILD=$PWD/build
echo -n "Using CC: "
realpath $CC
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --cc=$CC --prefix=$INSTALL
make -j4
# XXX: we need reliable clean up
# make check -j4 V=1
make install
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/20170420075237.18219-1-f...@redhat.com -> 
patchew/20170420075237.18219-1-f...@redhat.com
Switched to a new branch 'test'
2d5d050 tests: Add test-image-lock
a0d4c3c file-posix: Add image locking in perm operations
17c490b osdep: Add qemu_lock_fd and qemu_unlock_fd
2383a28 block: Workaround drive-backup sync=none for image locking
63541e0 tests: Disable image lock in test-replication
354af7e file-posix: Add 'locking' option
9cf4312 tests: Use null-co:// instead of /dev/null as the dummy image
4f5bbc4 iotests: 172: Use separate images for multiple devices
76ff84d iotests: 091: Quit QEMU before checking image
82cf7cc iotests: 087: Don't attach test image twice
b9329f3 iotests: 085: Avoid image locking conflict
8a52859 iotests: 055: Don't attach the target image already for drive-backup
4159096 iotests: 046: Prepare for image locking
a8af152 iotests: 030: Prepare for image locking
1137b91 qemu-io: Add --unsafe-read option
d66cf70 qemu-img: Update documentation for --unsafe-read
7f30dc1 qemu-img: Add --unsafe-read option to subcommands
d392aa4 block: Don't require BLK_PERM_CONSISTENT_READ when unsafe open
03a43f3 block: Drop consistent read perm if opened unsafe
a45035b block: Introduce BDRV_O_UNSAFE_READ

=== OUTPUT BEGIN ===
=== ENV ===
XDG_SESSION_ID=15468
SHELL=/bin/sh
USER=fam
PATCHEW=/home/fam/patchew/patchew-cli -s http://patchew.org --nodebug
PATH=/usr/bin:/bin
PWD=/var/tmp/patchew-tester-tmp-7vbaovju/src
LANG=en_US.UTF-8
HOME=/home/fam
SHLVL=2
LOGNAME=fam
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1012/bus
XDG_RUNTIME_DIR=/run/user/1012
_=/usr/bin/env
=== PACKAGES ===
gpg-pubkey-873529b8-54e386ff
xz-libs-5.2.2-2.fc24.s390x
libxshmfence-1.2-3.fc24.s390x
giflib-4.1.6-15.fc24.s390x
trousers-lib-0.3.13-6.fc24.s390x
ncurses-base-6.0-6.20160709.fc25.noarch
gmp-6.1.1-1.fc25.s390x
libidn-1.33-1.fc25.s390x
slang-2.3.0-7.fc25.s390x
libsemanage-2.5-8.fc25.s390x
pkgconfig-0.29.1-1.fc25.s390x
alsa-lib-1.1.1-2.fc25.s390x
yum-metadata-parser-1.1.4-17.fc25.s390x
python3-slip-dbus-0.6.4-4.fc25.noarch
python2-cssselect-0.9.2-1.fc25.noarch
python-fedora-0.8.0-2.fc25.noarch
createrepo_c-libs-0.10.0-6.fc25.s390x
initscripts-9.69-1.fc25.s390x
wget-1.18-2.fc25.s390x
dhcp-client-4.3.5-1.fc25.s390x
parted-3.2-21.fc25.s390x
flex-2.6.0-3.fc25.s390x
colord-libs-1.3.4-1.fc25.s390x
python-osbs-client-0.33-3.fc25.noarch
perl-Pod-Simple-3.35-1.fc25.noarch
python2-simplejson-3.10.0-1.fc25.s390x
brltty-5.4-2.fc25.s390x
librados2-10.2.4-2.fc25.s390x
tcp_wrappers-7.6-83.fc25.s390x
libcephfs_jni1-10.2.4-2.fc25.s390x
nettle-devel-3.3-1.fc25.s390x
bzip2-devel-1.0.6-21.fc25.s390x
libuuid-2.28.2-2.fc25.s390x
mesa-libglapi-13.0.3-5.fc25.s390x
pcre-cpp-8.40-5.fc25.s390x
pango-1.40.4-1.fc25.s390x
python3-magic-5.29-3.fc25.noarch
python3-dnf-1.1.10-6.fc25.noarch
cryptsetup-libs-1.7.4-1.fc25.s390x
texlive-kpathsea-doc-svn41139-33.fc25.1.noarch
netpbm-10.77.00-3.fc25.s390x
openssh-7.4p1-4.fc25.s390x
kernel-headers-4.10.5-200.fc25.s390x
texlive-kpathsea-bin-svn40473-33.20160520.fc25.1.s390x
texlive-graphics-svn41015-33.fc25.1.noarch
texlive-dvipdfmx-def-svn40328-33.fc25.1.noarch
texlive-mfware-svn40768-33.fc25.1.noarch
texlive-texlive-scripts-svn41433-33.fc25.1.noarch
texlive-euro-svn22191.1.1-33.fc25.1.noarch
texlive-etex-svn37057.0-33.fc25.1.noarch
texlive-iftex-svn29654.0.2-33.fc25.1.noarch
texlive-palatino-svn31835.0-33.fc25.1.noarch
texlive-texlive-docindex-svn41430-33.fc25.1.noarch
texlive-xunicode-svn30466.0.981-33.fc25.1.noarch
texlive-koma-script-svn41508-33.fc25.1.noarch
texlive-pst-grad-svn15878.1.06-33.fc25.1.noarch
texlive-pst-blur-svn15878.2.0-33.fc25.1.noarch
texlive-jknapltx-svn19440.0-33.fc25.1.noarch
netpbm-progs-10.77.00-3.fc25.s390x
mesa-libgbm-devel-13.0.3-5.fc25.s390x
texinfo-6.1-4.fc25.s390x
openssl-devel-1.0.2k-1.fc25.s390x
python2-sssdconfig-1.15.2-1.fc25.noarch
libaio-0.3.110-6.fc24.s390x
libfontenc-1.1.3-3.fc24.s390x
lzo-2.08-8.fc24.s390x
isl-0.14-5.fc24.s390x
libXau-1.0.8-6.fc24.s390x
linux-atm-libs-2.5.1-14.fc24.s390x
libXext-1.3.3-4.fc24.s390x
libXxf86vm-1.1.4-3.fc24.s390x
bison-3.0.4-4.fc24.s390x
perl-srpm-macros-1-20.fc25.noarch

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-img: check bs_n when use old style option

[Fam Zheng]

On Thu, 04/20 15:59, 858585 jemmy wrote:
> On Thu, Apr 20, 2017 at 3:51 PM, Fam Zheng  wrote:
> > On Thu, 04/20 12:04, jemmy858...@gmail.com wrote:
> >> From: Lidong Chen 
> >>
> >> When use old style option like -o backing_file, img_convert
> >> continue run when bs_n > 1, this patch fix this bug.
> >>
> >> Signed-off-by: Lidong Chen 
> >> ---
> >>  qemu-img.c | 7 +++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/qemu-img.c b/qemu-img.c
> >> index b220cf7..c673aef 100644
> >> --- a/qemu-img.c
> >> +++ b/qemu-img.c
> >> @@ -2225,6 +2225,13 @@ static int img_convert(int argc, char **argv)
> >>  out_baseimg = out_baseimg_param;
> >>  }
> >>
> >> +if (bs_n > 1 && out_baseimg) {
> >> +error_report("-B makes no sense when concatenating multiple input 
> >> "
> >> + "images");
> >> +ret = -1;
> >> +goto out;
> >> +}
> >> +
> >>  /* Check if compression is supported */
> >>  if (compress) {
> >>  bool encryption =
> >> --
> >> 1.8.3.1
> >>
> >>
> >
> > Is this essentially the same as the check a few lines above:
> >
> > ...
> > if (bs_n < 1) {
> > error_exit("Must specify image file name");
> > }
> >
> >
> > if (bs_n > 1 && out_baseimg) {
> > error_report("-B makes no sense when concatenating multiple input "
> >  "images");
> > ret = -1;
> > goto out;
> > }
> >
> > src_flags = 0;
> > ret = bdrv_parse_cache_mode(src_cache, _flags, _writethrough);
> > if (ret < 0) {
> > error_report("Invalid source cache option: %s", src_cache);
> > goto out;
> > }
> > ...
> >
> > How about moving that down?
> moving that down is ok.
> but will exit later if use -B option.
> which way do you think better?

Exiting later is not a problem, I assume? And it's better to avoid duplicating
code if possible.

BTW if you do that way, it's better to "s/-B/Specifying backing image/" in the
error message (to be compatible with -o backing_file syntax).

Fam

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-img: check bs_n when use old style option

[858585 jemmy]

On Thu, Apr 20, 2017 at 3:51 PM, Fam Zheng  wrote:
> On Thu, 04/20 12:04, jemmy858...@gmail.com wrote:
>> From: Lidong Chen 
>>
>> When use old style option like -o backing_file, img_convert
>> continue run when bs_n > 1, this patch fix this bug.
>>
>> Signed-off-by: Lidong Chen 
>> ---
>>  qemu-img.c | 7 +++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index b220cf7..c673aef 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -2225,6 +2225,13 @@ static int img_convert(int argc, char **argv)
>>  out_baseimg = out_baseimg_param;
>>  }
>>
>> +if (bs_n > 1 && out_baseimg) {
>> +error_report("-B makes no sense when concatenating multiple input "
>> + "images");
>> +ret = -1;
>> +goto out;
>> +}
>> +
>>  /* Check if compression is supported */
>>  if (compress) {
>>  bool encryption =
>> --
>> 1.8.3.1
>>
>>
>
> Is this essentially the same as the check a few lines above:
>
> ...
> if (bs_n < 1) {
> error_exit("Must specify image file name");
> }
>
>
> if (bs_n > 1 && out_baseimg) {
> error_report("-B makes no sense when concatenating multiple input "
>  "images");
> ret = -1;
> goto out;
> }
>
> src_flags = 0;
> ret = bdrv_parse_cache_mode(src_cache, _flags, _writethrough);
> if (ret < 0) {
> error_report("Invalid source cache option: %s", src_cache);
> goto out;
> }
> ...
>
> How about moving that down?
moving that down is ok.
but will exit later if use -B option.
which way do you think better?


>
> Fam

[Qemu-block] [PATCH v13 20/20] tests: Add test-image-lock

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 tests/Makefile.include  |   2 +
 tests/test-image-lock.c | 259 
 2 files changed, 261 insertions(+)
 create mode 100644 tests/test-image-lock.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index f3de81f..20e3093 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -58,6 +58,7 @@ check-unit-y += tests/test-hbitmap$(EXESUF)
 gcov-files-test-hbitmap-y = blockjob.c
 check-unit-y += tests/test-blockjob$(EXESUF)
 check-unit-y += tests/test-blockjob-txn$(EXESUF)
+check-unit-y += tests/test-image-lock$(EXESUF)
 check-unit-y += tests/test-x86-cpuid$(EXESUF)
 # all code tested by test-x86-cpuid is inside topology.h
 gcov-files-test-x86-cpuid-y =
@@ -545,6 +546,7 @@ tests/test-aio-multithread$(EXESUF): 
tests/test-aio-multithread.o $(test-block-o
 tests/test-throttle$(EXESUF): tests/test-throttle.o $(test-block-obj-y)
 tests/test-blockjob$(EXESUF): tests/test-blockjob.o $(test-block-obj-y) 
$(test-util-obj-y)
 tests/test-blockjob-txn$(EXESUF): tests/test-blockjob-txn.o 
$(test-block-obj-y) $(test-util-obj-y)
+tests/test-image-lock$(EXESUF): tests/test-image-lock.o $(test-block-obj-y) 
$(libqos-obj-y)
 tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(test-block-obj-y)
 tests/test-iov$(EXESUF): tests/test-iov.o $(test-util-obj-y)
 tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y)
diff --git a/tests/test-image-lock.c b/tests/test-image-lock.c
new file mode 100644
index 000..a118e89
--- /dev/null
+++ b/tests/test-image-lock.c
@@ -0,0 +1,259 @@
+/*
+ * Image lock tests
+ *
+ * Copyright 2016 Red Hat, Inc.
+ *
+ * Authors:
+ *  Fam Zheng 
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "qapi/qmp/qbool.h"
+#include "qapi/qmp/qstring.h"
+#include "sysemu/block-backend.h"
+
+#define DEBUG_IMAGE_LOCK_TEST 0
+#define DPRINTF(...) do { \
+if (DEBUG_IMAGE_LOCK_TEST) { \
+printf(__VA_ARGS__); \
+} \
+} while (0)
+
+#define TEST_IMAGE_SIZE 4096
+static char test_image[] = "/tmp/qtest.XX";
+static int test_image_fd;
+
+static BlockBackend *open_test_image(const char *format,
+ int flags, bool disable_lock, Error 
**errp)
+{
+QDict *opts = qdict_new();
+
+qdict_set_default_str(opts, "driver", format);
+qdict_set_default_str(opts, "file.driver", "file");
+qdict_set_default_str(opts, "file.filename", test_image);
+qdict_set_default_str(opts, "file.locking", disable_lock ? "off" : "on");
+
+return blk_new_open(NULL, NULL, opts, flags, errp);
+}
+
+#define RW true
+#define RO false
+#define SHARE true
+#define EXCLU false
+
+static struct CompatData {
+bool write_1;
+bool share_1;
+bool write_2;
+bool share_2;
+bool compatible;
+} compat_data[] = {
+/* Write 1, Share 1, Write 2, Share 2, Compatible. */
+{ RO,   SHARE,   RO,  SHARE,   true,  },
+{ RO,   SHARE,   RO,  EXCLU,   true,  },
+{ RO,   SHARE,   RW,  SHARE,   true,  },
+{ RO,   SHARE,   RW,  EXCLU,   true,  },
+
+{ RO,   EXCLU,   RO,  EXCLU,   true,  },
+{ RO,   EXCLU,   RW,  SHARE,   false, },
+{ RO,   EXCLU,   RW,  EXCLU,   false, },
+
+{ RW,   SHARE,   RW,  SHARE,   false, },
+{ RW,   SHARE,   RW,  EXCLU,   false, },
+
+{ RW,   EXCLU,   RW,  EXCLU,   false, },
+};
+
+static void dprint_flags(int flags)
+{
+DPRINTF(" %8x", flags);
+if (flags & BDRV_O_RDWR) {
+DPRINTF(" RW");
+} else {
+DPRINTF(" RO");
+}
+if (flags & BDRV_O_UNSAFE_READ) {
+DPRINTF(" SHARE");
+} else {
+DPRINTF(" EXCLU");
+}
+DPRINTF("\n");
+}
+
+/* Test one combination scenario.
+ *
+ * @flags1: The flags of the first blk.
+ * @flags2: The flags of the second blk.
+ * @disable1: The value for raw-posix disable-lock option of the first blk.
+ * @disable2: The value for raw-posix disable-lock option of the second blk.
+ * @from_reopen: Whether or not the first blk should get flags1 from a reopen.
+ * @initial: The source flags from which the blk1 is reopened, only
+ *effective if @from_reopen is true.
+ */
+static void do_test_compat_one(const char *format,
+   int flags1, int flags2,
+   bool disable1, bool disable2,
+   bool from_reopen, int initial_readonly,
+   bool compatible)
+{
+int ret;
+BlockBackend *blk1, *blk2;
+int initial_flags = flags1;
+Error *local_err = NULL;
+
+DPRINTF("\n===\ndo test compat one\n");
+DPRINTF("flags1:");
+dprint_flags(flags1);
+DPRINTF("flags2:");
+

[Qemu-block] [PATCH v13 14/20] tests: Use null-co:// instead of /dev/null as the dummy image

[Fam Zheng]

Signed-off-by: Fam Zheng 
Reviewed-by: Max Reitz 
---
 tests/drive_del-test.c| 2 +-
 tests/nvme-test.c | 2 +-
 tests/usb-hcd-uhci-test.c | 2 +-
 tests/usb-hcd-xhci-test.c | 2 +-
 tests/virtio-blk-test.c   | 2 +-
 tests/virtio-scsi-test.c  | 4 ++--
 6 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c
index 121b9c9..2175139 100644
--- a/tests/drive_del-test.c
+++ b/tests/drive_del-test.c
@@ -92,7 +92,7 @@ static void test_after_failed_device_add(void)
 static void test_drive_del_device_del(void)
 {
 /* Start with a drive used by a device that unplugs instantaneously */
-qtest_start("-drive if=none,id=drive0,file=/dev/null,format=raw"
+qtest_start("-drive if=none,id=drive0,file=null-co://,format=raw"
 " -device virtio-scsi-pci"
 " -device scsi-hd,drive=drive0,id=dev0");
 
diff --git a/tests/nvme-test.c b/tests/nvme-test.c
index c8bece4..7674a44 100644
--- a/tests/nvme-test.c
+++ b/tests/nvme-test.c
@@ -22,7 +22,7 @@ int main(int argc, char **argv)
 g_test_init(, , NULL);
 qtest_add_func("/nvme/nop", nop);
 
-qtest_start("-drive id=drv0,if=none,file=/dev/null,format=raw "
+qtest_start("-drive id=drv0,if=none,file=null-co://,format=raw "
 "-device nvme,drive=drv0,serial=foo");
 ret = g_test_run();
 
diff --git a/tests/usb-hcd-uhci-test.c b/tests/usb-hcd-uhci-test.c
index f25bae5..5b500fe 100644
--- a/tests/usb-hcd-uhci-test.c
+++ b/tests/usb-hcd-uhci-test.c
@@ -79,7 +79,7 @@ int main(int argc, char **argv)
 {
 const char *arch = qtest_get_arch();
 const char *cmd = "-device piix3-usb-uhci,id=uhci,addr=1d.0"
-  " -drive id=drive0,if=none,file=/dev/null,format=raw"
+  " -drive id=drive0,if=none,file=null-co://,format=raw"
   " -device usb-tablet,bus=uhci.0,port=1";
 int ret;
 
diff --git a/tests/usb-hcd-xhci-test.c b/tests/usb-hcd-xhci-test.c
index 22513e9..031764d 100644
--- a/tests/usb-hcd-xhci-test.c
+++ b/tests/usb-hcd-xhci-test.c
@@ -89,7 +89,7 @@ int main(int argc, char **argv)
 qtest_add_func("/xhci/pci/hotplug/usb-uas", test_usb_uas_hotplug);
 
 qtest_start("-device nec-usb-xhci,id=xhci"
-" -drive id=drive0,if=none,file=/dev/null,format=raw");
+" -drive id=drive0,if=none,file=null-co://,format=raw");
 ret = g_test_run();
 qtest_end();
 
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 1eee95d..fd2078c 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -63,7 +63,7 @@ static QOSState *pci_test_start(void)
 const char *arch = qtest_get_arch();
 char *tmp_path;
 const char *cmd = "-drive if=none,id=drive0,file=%s,format=raw "
-  "-drive if=none,id=drive1,file=/dev/null,format=raw "
+  "-drive if=none,id=drive1,file=null-co://,format=raw "
   "-device virtio-blk-pci,id=drv0,drive=drive0,"
   "addr=%x.%x";
 
diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
index 0eabd56..5e2ff14 100644
--- a/tests/virtio-scsi-test.c
+++ b/tests/virtio-scsi-test.c
@@ -35,7 +35,7 @@ typedef struct {
 static QOSState *qvirtio_scsi_start(const char *extra_opts)
 {
 const char *arch = qtest_get_arch();
-const char *cmd = "-drive id=drv0,if=none,file=/dev/null,format=raw "
+const char *cmd = "-drive id=drv0,if=none,file=null-co://,format=raw "
   "-device virtio-scsi-pci,id=vs0 "
   "-device scsi-hd,bus=vs0.0,drive=drv0 %s";
 
@@ -195,7 +195,7 @@ static void hotplug(void)
 QDict *response;
 QOSState *qs;
 
-qs = qvirtio_scsi_start("-drive 
id=drv1,if=none,file=/dev/null,format=raw");
+qs = qvirtio_scsi_start("-drive 
id=drv1,if=none,file=null-co://,format=raw");
 response = qmp("{\"execute\": \"device_add\","
" \"arguments\": {"
"   \"driver\": \"scsi-hd\","
-- 
2.9.3

[Qemu-block] [PATCH v13 19/20] file-posix: Add image locking in perm operations

[Fam Zheng]

virtlockd in libvirt locks the first byte, so we start looking at the
file bytes from 0x10.

The complication is in the transactional interface.  To make the reopen
logic managable, and allow better reuse, the code is internally
organized with a table from old mode to the new one.

Signed-off-by: Fam Zheng 
---
 block/file-posix.c | 744 -
 1 file changed, 741 insertions(+), 3 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 24ea3ff..b85ac9c 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -131,8 +131,54 @@ do { \
 
 #define MAX_BLOCKSIZE  4096
 
+/* Posix file locking bytes. Libvirt takes byte 0, we start from byte 0x10,
+ * leaving a few more bytes for its future use. */
+#define RAW_LOCK_BYTE_MIN 0x10
+#define RAW_LOCK_BYTE_NO_OTHER_WRITER 0x10
+#define RAW_LOCK_BYTE_WRITE   0x11
+#ifdef F_OFD_SETLK
+#define RAW_LOCK_SUPPORTED 1
+#else
+#define RAW_LOCK_SUPPORTED 0
+#endif
+
+/*
+ ** reader that can tolerate writers: Don't do anything
+ *
+ ** reader that can't tolerate writers: Take shared lock on byte 0x10. Test
+ *  byte 0x11 is unlocked.
+ *
+ ** shared writer: Take shared lock on byte 0x11. Test byte 0x10 is unlocked.
+ *
+ ** exclusive writer: Take exclusive locks on both bytes.
+ */
+
+typedef enum {
+/* Read only and accept other writers. */
+RAW_L_READ_SHARE_RW,
+/* Read only and try to forbid other writers. */
+RAW_L_READ,
+/* Read/write and accept other writers. */
+RAW_L_WRITE_SHARE_RW,
+/* Read/write and try to forbid other writers. */
+RAW_L_WRITE,
+} BDRVRawLockMode;
+
+typedef struct BDRVRawLockUpdateState {
+/* A dup of @fd used for acquiring lock. */
+int image_fd;
+int lock_fd;
+int open_flags;
+BDRVRawLockMode new_lock;
+bool use_lock;
+} BDRVRawLockUpdateState;
+
 typedef struct BDRVRawState {
 int fd;
+/* A dup of @fd to make manipulating lock easier, especially during reopen,
+ * where this will accept BDRVRawReopenState.lock_fd. */
+int lock_fd;
+bool use_lock;
 int type;
 int open_flags;
 size_t buf_align;
@@ -147,6 +193,11 @@ typedef struct BDRVRawState {
 bool page_cache_inconsistent:1;
 bool has_fallocate;
 bool needs_alignment;
+/* The current lock mode we are in. Note that in incoming migration this is
+ * the "desired" mode to be applied at bdrv_invalidate_cache. */
+BDRVRawLockMode cur_lock_mode;
+/* Used by raw_check_perm/raw_set_perm. */
+BDRVRawLockUpdateState *lock_update;
 } BDRVRawState;
 
 typedef struct BDRVRawReopenState {
@@ -369,6 +420,64 @@ static void raw_parse_flags(int bdrv_flags, int 
*open_flags)
 }
 }
 
+static int raw_lock_fd(int fd, BDRVRawLockMode mode, Error **errp)
+{
+int ret;
+assert(fd >= 0);
+assert(RAW_LOCK_SUPPORTED);
+switch (mode) {
+case RAW_L_READ_SHARE_RW:
+ret = qemu_unlock_fd(fd, RAW_LOCK_BYTE_MIN, 2);
+if (ret) {
+error_setg_errno(errp, -ret, "Failed to unlock fd");
+goto fail;
+}
+break;
+case RAW_L_READ:
+ret = qemu_lock_fd(fd, RAW_LOCK_BYTE_NO_OTHER_WRITER, 1, false);
+if (ret) {
+error_setg_errno(errp, -ret, "Failed to lock share byte");
+goto fail;
+}
+ret = qemu_lock_fd_test(fd, RAW_LOCK_BYTE_WRITE, 1, true);
+if (ret) {
+error_setg_errno(errp, -ret, "Write byte lock is taken");
+goto fail;
+}
+break;
+case RAW_L_WRITE_SHARE_RW:
+ret = qemu_lock_fd(fd, RAW_LOCK_BYTE_WRITE, 1, true);
+if (ret) {
+error_setg_errno(errp, -ret, "Failed to lock write byte 
exclusively");
+goto fail;
+}
+ret = qemu_lock_fd(fd, RAW_LOCK_BYTE_WRITE, 1, false);
+if (ret) {
+error_setg_errno(errp, -ret, "Failed to downgrade lock write 
byte");
+goto fail;
+}
+ret = qemu_lock_fd_test(fd, RAW_LOCK_BYTE_NO_OTHER_WRITER, 1, true);
+if (ret) {
+error_setg_errno(errp, -ret, "Share byte lock is taken");
+goto fail;
+}
+break;
+case RAW_L_WRITE:
+ret = qemu_lock_fd(fd, RAW_LOCK_BYTE_MIN, 2, true);
+if (ret) {
+error_setg_errno(errp, -ret, "Failed to lock image");
+goto fail;
+}
+break;
+default:
+abort();
+}
+return 0;
+fail:
+qemu_unlock_fd(fd, RAW_LOCK_BYTE_MIN, 2);
+return ret;
+}
+
 static void raw_parse_filename(const char *filename, QDict *options,
Error **errp)
 {
@@ -403,6 +512,23 @@ static QemuOptsList raw_runtime_opts = {
 },
 };
 
+static BDRVRawLockMode raw_get_lock_mode(bool write, bool shared)
+{
+if (write) {
+if (shared) {
+return RAW_L_WRITE_SHARE_RW;
+} else {
+return RAW_L_WRITE;
+}
+} else {

[Qemu-block] [PATCH v13 17/20] block: Workaround drive-backup sync=none for image locking

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 blockdev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 4927914..5831b95 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3223,6 +3223,9 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
BlockJobTxn *txn,
 }
 }
 if (backup->sync == MIRROR_SYNC_MODE_NONE) {
+/* FIXME: block layer should really open target with BDRV_O_NO_BACKING
+ * and reuse source's backing chain, if they share one. */
+flags |= BDRV_O_UNSAFE_READ;
 source = bs;
 }
 
-- 
2.9.3

[Qemu-block] [PATCH v13 09/20] iotests: 055: Don't attach the target image already for drive-backup

[Fam Zheng]

Double attach is not a valid usage of the target image, drive-backup
will open the blockdev itself so skip the add_drive call in this case.

Signed-off-by: Fam Zheng 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/055 | 32 ++--
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index aafcd24..ba4da65 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -458,17 +458,18 @@ class TestDriveCompression(iotests.QMPTestCase):
 except OSError:
 pass
 
-def do_prepare_drives(self, fmt, args):
+def do_prepare_drives(self, fmt, args, attach_target):
 self.vm = iotests.VM().add_drive(test_img)
 
 qemu_img('create', '-f', fmt, blockdev_target_img,
  str(TestDriveCompression.image_len), *args)
-self.vm.add_drive(blockdev_target_img, format=fmt, interface="none")
+if attach_target:
+self.vm.add_drive(blockdev_target_img, format=fmt, 
interface="none")
 
 self.vm.launch()
 
-def do_test_compress_complete(self, cmd, format, **args):
-self.do_prepare_drives(format['type'], format['args'])
+def do_test_compress_complete(self, cmd, format, attach_target, **args):
+self.do_prepare_drives(format['type'], format['args'], attach_target)
 
 self.assert_no_active_block_jobs()
 
@@ -484,15 +485,16 @@ class TestDriveCompression(iotests.QMPTestCase):
 
 def test_complete_compress_drive_backup(self):
 for format in TestDriveCompression.fmt_supports_compression:
-self.do_test_compress_complete('drive-backup', format,
+self.do_test_compress_complete('drive-backup', format, False,
target=blockdev_target_img, 
mode='existing')
 
 def test_complete_compress_blockdev_backup(self):
 for format in TestDriveCompression.fmt_supports_compression:
-self.do_test_compress_complete('blockdev-backup', format, 
target='drive1')
+self.do_test_compress_complete('blockdev-backup', format, True,
+   target='drive1')
 
-def do_test_compress_cancel(self, cmd, format, **args):
-self.do_prepare_drives(format['type'], format['args'])
+def do_test_compress_cancel(self, cmd, format, attach_target, **args):
+self.do_prepare_drives(format['type'], format['args'], attach_target)
 
 self.assert_no_active_block_jobs()
 
@@ -506,15 +508,16 @@ class TestDriveCompression(iotests.QMPTestCase):
 
 def test_compress_cancel_drive_backup(self):
 for format in TestDriveCompression.fmt_supports_compression:
-self.do_test_compress_cancel('drive-backup', format,
+self.do_test_compress_cancel('drive-backup', format, False,
  target=blockdev_target_img, 
mode='existing')
 
 def test_compress_cancel_blockdev_backup(self):
for format in TestDriveCompression.fmt_supports_compression:
-self.do_test_compress_cancel('blockdev-backup', format, 
target='drive1')
+self.do_test_compress_cancel('blockdev-backup', format, True,
+ target='drive1')
 
-def do_test_compress_pause(self, cmd, format, **args):
-self.do_prepare_drives(format['type'], format['args'])
+def do_test_compress_pause(self, cmd, format, attach_target, **args):
+self.do_prepare_drives(format['type'], format['args'], attach_target)
 
 self.assert_no_active_block_jobs()
 
@@ -546,12 +549,13 @@ class TestDriveCompression(iotests.QMPTestCase):
 
 def test_compress_pause_drive_backup(self):
 for format in TestDriveCompression.fmt_supports_compression:
-self.do_test_compress_pause('drive-backup', format,
+self.do_test_compress_pause('drive-backup', format, False,
 target=blockdev_target_img, 
mode='existing')
 
 def test_compress_pause_blockdev_backup(self):
 for format in TestDriveCompression.fmt_supports_compression:
-self.do_test_compress_pause('blockdev-backup', format, 
target='drive1')
+self.do_test_compress_pause('blockdev-backup', format, True,
+target='drive1')
 
 if __name__ == '__main__':
 iotests.main(supported_fmts=['raw', 'qcow2'])
-- 
2.9.3

[Qemu-block] [PATCH v13 15/20] file-posix: Add 'locking' option

[Fam Zheng]

Making this option available even before implementing it will let
converting tests easier: in coming patches they can specify the option
already when necessary, before we actually write code to lock the
images.

Signed-off-by: Fam Zheng 
---
 block/file-posix.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index 0c48968..24ea3ff 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -394,6 +394,11 @@ static QemuOptsList raw_runtime_opts = {
 .type = QEMU_OPT_STRING,
 .help = "host AIO implementation (threads, native)",
 },
+{
+.name = "locking",
+.type = QEMU_OPT_BOOL,
+.help = "lock the file",
+},
 { /* end of list */ }
 },
 };
-- 
2.9.3

[Qemu-block] [PATCH v13 13/20] iotests: 172: Use separate images for multiple devices

[Fam Zheng]

To avoid image lock failures.

Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/172 | 55 +-
 tests/qemu-iotests/172.out | 50 +
 2 files changed, 56 insertions(+), 49 deletions(-)

diff --git a/tests/qemu-iotests/172 b/tests/qemu-iotests/172
index 1b7d3a1..826d6fe 100755
--- a/tests/qemu-iotests/172
+++ b/tests/qemu-iotests/172
@@ -30,6 +30,8 @@ status=1  # failure is the default!
 _cleanup()
 {
_cleanup_test_img
+rm -f "$TEST_IMG.2"
+rm -f "$TEST_IMG.3"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -86,6 +88,9 @@ size=720k
 
 _make_test_img $size
 
+TEST_IMG="$TEST_IMG.2" _make_test_img $size
+TEST_IMG="$TEST_IMG.3" _make_test_img $size
+
 # Default drive semantics:
 #
 # By default you get a single empty floppy drive. You can override it with
@@ -105,7 +110,7 @@ echo === Using -fda/-fdb options ===
 
 check_floppy_qtree -fda "$TEST_IMG"
 check_floppy_qtree -fdb "$TEST_IMG"
-check_floppy_qtree -fda "$TEST_IMG" -fdb "$TEST_IMG"
+check_floppy_qtree -fda "$TEST_IMG" -fdb "$TEST_IMG.2"
 
 
 echo
@@ -114,7 +119,7 @@ echo === Using -drive options ===
 
 check_floppy_qtree -drive if=floppy,file="$TEST_IMG"
 check_floppy_qtree -drive if=floppy,file="$TEST_IMG",index=1
-check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive 
if=floppy,file="$TEST_IMG",index=1
+check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive 
if=floppy,file="$TEST_IMG.2",index=1
 
 echo
 echo
@@ -122,7 +127,7 @@ echo === Using -drive if=none and -global ===
 
 check_floppy_qtree -drive if=none,file="$TEST_IMG" -global isa-fdc.driveA=none0
 check_floppy_qtree -drive if=none,file="$TEST_IMG" -global isa-fdc.driveB=none0
-check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG" \
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG.2" \
-global isa-fdc.driveA=none0 -global isa-fdc.driveB=none1
 
 echo
@@ -131,7 +136,7 @@ echo === Using -drive if=none and -device ===
 
 check_floppy_qtree -drive if=none,file="$TEST_IMG" -device floppy,drive=none0
 check_floppy_qtree -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0,unit=1
-check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG" \
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG.2" \
-device floppy,drive=none0 -device floppy,drive=none1,unit=1
 
 echo
@@ -139,58 +144,58 @@ echo
 echo === Mixing -fdX and -global ===
 
 # Working
-check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveB=none0
-check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveA=none0
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -global 
isa-fdc.driveB=none0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -global 
isa-fdc.driveA=none0
 
 # Conflicting (-fdX wins)
-check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveA=none0
-check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveB=none0
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -global 
isa-fdc.driveA=none0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -global 
isa-fdc.driveB=none0
 
 echo
 echo
 echo === Mixing -fdX and -device ===
 
 # Working
-check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0
-check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0,unit=1
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device 
floppy,drive=none0
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device 
floppy,drive=none0,unit=1
 
-check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0
-check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0,unit=0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device 
floppy,drive=none0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device 
floppy,drive=none0,unit=0
 
 # Conflicting
-check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0,unit=0
-check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0,unit=1
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device 
floppy,drive=none0,unit=0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device 
floppy,drive=none0,unit=1
 
 echo
 echo
 echo === Mixing -drive and -device ===
 
 # Working
-check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG" -device floppy,drive=none0
-check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive 

[Qemu-block] [PATCH v13 18/20] osdep: Add qemu_lock_fd and qemu_unlock_fd

[Fam Zheng]

They are wrappers of POSIX fcntl "file private locking", with a
convenient "try lock" wrapper implemented with F_OFD_GETLK.

Signed-off-by: Fam Zheng 
Reviewed-by: Max Reitz 
---
 include/qemu/osdep.h |  3 +++
 util/osdep.c | 48 
 2 files changed, 51 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 122ff06..1c9f5e2 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -341,6 +341,9 @@ int qemu_close(int fd);
 #ifndef _WIN32
 int qemu_dup(int fd);
 #endif
+int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
+int qemu_unlock_fd(int fd, int64_t start, int64_t len);
+int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive);
 
 #if defined(__HAIKU__) && defined(__i386__)
 #define FMT_pid "%ld"
diff --git a/util/osdep.c b/util/osdep.c
index 06fb1cf..3de4a18 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -140,6 +140,54 @@ static int qemu_parse_fdset(const char *param)
 {
 return qemu_parse_fd(param);
 }
+
+static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
+{
+#ifdef F_OFD_SETLK
+int ret;
+struct flock fl = {
+.l_whence = SEEK_SET,
+.l_start  = start,
+.l_len= len,
+.l_type   = fl_type,
+};
+ret = fcntl(fd, F_OFD_SETLK, );
+return ret == -1 ? -errno : 0;
+#else
+return -ENOTSUP;
+#endif
+}
+
+int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive)
+{
+return qemu_lock_fcntl(fd, start, len, exclusive ? F_WRLCK : F_RDLCK);
+}
+
+int qemu_unlock_fd(int fd, int64_t start, int64_t len)
+{
+return qemu_lock_fcntl(fd, start, len, F_UNLCK);
+}
+
+int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
+{
+#ifdef F_OFD_SETLK
+int ret;
+struct flock fl = {
+.l_whence = SEEK_SET,
+.l_start  = start,
+.l_len= len,
+.l_type   = exclusive ? F_WRLCK : F_RDLCK,
+};
+ret = fcntl(fd, F_OFD_GETLK, );
+if (ret == -1) {
+return -errno;
+} else {
+return fl.l_type == F_UNLCK ? 0 : -EAGAIN;
+}
+#else
+return -ENOTSUP;
+#endif
+}
 #endif
 
 /*
-- 
2.9.3

[Qemu-block] [PATCH v13 11/20] iotests: 087: Don't attach test image twice

[Fam Zheng]

The test scenario doesn't require the same image, instead it focuses on
the duplicated node-name, so use null-co to avoid locking conflict.

Reviewed-by: Max Reitz 
Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/087 | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index 9de57dd..6d52f7d 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -82,8 +82,7 @@ run_qemu -drive 
driver=$IMGFMT,id=disk,node-name=test-node,file="$TEST_IMG" <

[Qemu-block] [PATCH v13 12/20] iotests: 091: Quit QEMU before checking image

[Fam Zheng]

Signed-off-by: Fam Zheng 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/091 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qemu-iotests/091 b/tests/qemu-iotests/091
index 32bbd56..10ac4a8 100755
--- a/tests/qemu-iotests/091
+++ b/tests/qemu-iotests/091
@@ -95,7 +95,9 @@ echo "vm2: qemu process running successfully"
 echo "vm2: flush io, and quit"
 _send_qemu_cmd $h2 'qemu-io disk flush' "(qemu)"
 _send_qemu_cmd $h2 'quit' ""
+_send_qemu_cmd $h1 'quit' ""
 
+wait
 echo "Check image pattern"
 ${QEMU_IO} -c "read -P 0x22 0 4M" "${TEST_IMG}" | _filter_testdir | 
_filter_qemu_io
 
-- 
2.9.3

[Qemu-block] [PATCH v13 08/20] iotests: 046: Prepare for image locking

[Fam Zheng]

The qemu-img info command is executed while VM is running, add -U option
to avoid the image locking error.

Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/046 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/046 b/tests/qemu-iotests/046
index e528b67..f2ebecf 100755
--- a/tests/qemu-iotests/046
+++ b/tests/qemu-iotests/046
@@ -192,7 +192,7 @@ echo "== Verify image content =="
 
 function verify_io()
 {
-if ($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep "compat: 0.10" > 
/dev/null); then
+if ($QEMU_IMG info -U -f "$IMGFMT" "$TEST_IMG" | grep "compat: 0.10" > 
/dev/null); then
 # For v2 images, discarded clusters are read from the backing file
 # Keep the variable empty so that the backing file value can be used as
 # the default below
-- 
2.9.3

[Qemu-block] [PATCH v13 10/20] iotests: 085: Avoid image locking conflict

[Fam Zheng]

In the case where we test the expected error when a blockdev-snapshot
target already has a backing image, the backing chain is opened multiple
times. This will be a problem when we use image locking, so use a
different backing file that is not already open.

Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/085 | 34 --
 tests/qemu-iotests/085.out |  3 ++-
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index c53e97f..cc6efd8 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -45,7 +45,7 @@ _cleanup()
 rm -f "${TEST_DIR}/${i}-${snapshot_virt0}"
 rm -f "${TEST_DIR}/${i}-${snapshot_virt1}"
 done
-rm -f "${TEST_IMG}.1" "${TEST_IMG}.2"
+rm -f "${TEST_IMG}" "${TEST_IMG}.1" "${TEST_IMG}.2" "${TEST_IMG}.base"
 
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
@@ -87,24 +87,26 @@ function create_group_snapshot()
 }
 
 # ${1}: unique identifier for the snapshot filename
-# ${2}: true: open backing images; false: don't open them (default)
+# ${2}: extra_params to the blockdev-add command
+# ${3}: filename
+function do_blockdev_add()
+{
+cmd="{ 'execute': 'blockdev-add', 'arguments':
+   { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${2}
+ 'file':
+ { 'driver': 'file', 'filename': '${3}',
+   'node-name': 'file_${1}' } } }"
+_send_qemu_cmd $h "${cmd}" "return"
+}
+
+# ${1}: unique identifier for the snapshot filename
 function add_snapshot_image()
 {
-if [ "${2}" = "true" ]; then
-extra_params=""
-else
-extra_params="'backing': '', "
-fi
 base_image="${TEST_DIR}/$((${1}-1))-${snapshot_virt0}"
 snapshot_file="${TEST_DIR}/${1}-${snapshot_virt0}"
 _make_test_img -b "${base_image}" "$size"
 mv "${TEST_IMG}" "${snapshot_file}"
-cmd="{ 'execute': 'blockdev-add', 'arguments':
-   { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${extra_params}
- 'file':
- { 'driver': 'file', 'filename': '${snapshot_file}',
-   'node-name': 'file_${1}' } } }"
-_send_qemu_cmd $h "${cmd}" "return"
+do_blockdev_add "$1" "'backing': '', " "${snapshot_file}"
 }
 
 # ${1}: unique identifier for the snapshot filename
@@ -222,7 +224,11 @@ echo === Invalid command - snapshot node has a backing 
image ===
 echo
 
 SNAPSHOTS=$((${SNAPSHOTS}+1))
-add_snapshot_image ${SNAPSHOTS} true
+
+_make_test_img "$size"
+mv "${TEST_IMG}" "${TEST_IMG}.base"
+_make_test_img -b "${TEST_IMG}.base" "$size"
+do_blockdev_add ${SNAPSHOTS} "" "${TEST_IMG}"
 blockdev_snapshot ${SNAPSHOTS} error
 
 echo
diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out
index 182acb4..65b0f68 100644
--- a/tests/qemu-iotests/085.out
+++ b/tests/qemu-iotests/085.out
@@ -78,7 +78,8 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
backing_file=TEST_DIR/
 
 === Invalid command - snapshot node has a backing image ===
 
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
backing_file=TEST_DIR/12-snapshot-v0.IMGFMT
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
backing_file=TEST_DIR/t.IMGFMT.base
 {"return": {}}
 {"error": {"class": "GenericError", "desc": "The snapshot already has a 
backing image"}}
 
-- 
2.9.3