[Qemu-devel] [PATCH v7 for-2.6 00/24] block: Rework bdrv_close_all()

[Qemu-devel] [PATCH v7 for-2.6 00/24] block: Rework bdrv_close_all()

[Max Reitz]

Currently, bdrv_close_all() force-closes all BDSs with a BlockBackend,
which can lead to data corruption (see the iotest added in the final
patch of this series) and is most certainly very ugly.

This series reworks bdrv_close_all() to instead eject the BDS trees from
all BlockBackends and then close the monitor-owned BDS trees, which are
the only BDSs without a BB. In effect, all BDSs are closed just by
getting closed automatically due to their reference count becoming 0.

Note that the approach taken here leaks all BlockBackends. This does not
really matter, however, since qemu is about to exit anyway.


v7:
- Patch 3:
  - Put common functionality of bdrv_release_*dirty_bitmap*() into a new
    function bdrv_do_release_matching_dirty_bitmap() [John]
  - Keep assertion against any bitmap being frozen in
    bdrv_release_all_dirty_bitmaps() [John]
- Patches 4, 5, 6, 7, 8:
  - Split off from old patch 4 [Kevin]
  - Also: More code style changes ("() {" -> "()\n{")
  - Also: Support for nbd+unix URLs
- Patch 10: Use nbd+unix [Fam for v5]
- Patches 11, 12, 13, 14, 15: Split off from old patch 7 [Kevin]
- Patch 24: Added [Paolo]


git-backport-diff against v6:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/24:[----] [--] 'blockdev: Add missing bdrv_unref() in drive-backup'
002/24:[----] [--] 'blockjob: Call bdrv_unref() on creation error'
003/24:[0035] [FC] 'block: Release dirty bitmaps in bdrv_close()'
004/24:[down] 'iotests: Rename filter_nbd to _filter_nbd in 083'
005/24:[down] 'iotests: Change coding style of _filter_nbd in 083'
006/24:[0035] [FC] 'iotests: Move _filter_nbd into common.filter'
007/24:[down] 'iotests: Make _filter_nbd drop log lines'
008/24:[down] 'iotests: Make _filter_nbd support more URL types'
009/24:[----] [--] 'iotests: Make redirecting qemu's stderr optional'
010/24:[0016] [FC] 'iotests: Add test for eject under NBD server'
011/24:[down] 'block: Add BB-BDS remove/insert notifiers'
012/24:[down] 'virtio-blk: Functions for op blocker management'
013/24:[down] 'virtio-scsi: Catch BDS-BB removal/insertion'
014/24:[down] 'nbd: Switch from close to eject notifier'
015/24:[down] 'block: Remove BDS close notifier'
016/24:[----] [-C] 'block: Use blk_remove_bs() in blk_delete()'
017/24:[----] [--] 'blockdev: Use blk_remove_bs() in do_drive_del()'
018/24:[----] [--] 'block: Make bdrv_close() static'
019/24:[----] [--] 'block: Add list of all BlockDriverStates'
020/24:[----] [--] 'blockdev: Keep track of monitor-owned BDS'
021/24:[----] [--] 'block: Add blk_remove_all_bs()'
022/24:[----] [-C] 'block: Rewrite bdrv_close_all()'
023/24:[----] [--] 'iotests: Add test for multiple BB on BDS tree'
024/24:[down] 'iotests: Add test for block jobs and BDS ejection'


Max Reitz (24):
  blockdev: Add missing bdrv_unref() in drive-backup
  blockjob: Call bdrv_unref() on creation error
  block: Release dirty bitmaps in bdrv_close()
  iotests: Rename filter_nbd to _filter_nbd in 083
  iotests: Change coding style of _filter_nbd in 083
  iotests: Move _filter_nbd into common.filter
  iotests: Make _filter_nbd drop log lines
  iotests: Make _filter_nbd support more URL types
  iotests: Make redirecting qemu's stderr optional
  iotests: Add test for eject under NBD server
  block: Add BB-BDS remove/insert notifiers
  virtio-blk: Functions for op blocker management
  virtio-scsi: Catch BDS-BB removal/insertion
  nbd: Switch from close to eject notifier
  block: Remove BDS close notifier
  block: Use blk_remove_bs() in blk_delete()
  blockdev: Use blk_remove_bs() in do_drive_del()
  block: Make bdrv_close() static
  block: Add list of all BlockDriverStates
  blockdev: Keep track of monitor-owned BDS
  block: Add blk_remove_all_bs()
  block: Rewrite bdrv_close_all()
  iotests: Add test for multiple BB on BDS tree
  iotests: Add test for block jobs and BDS ejection

 block.c                                | 100 +++++++++++++++-----
 block/block-backend.c                  |  43 +++++++--
 blockdev-nbd.c                         |  37 +-------
 blockdev.c                             |  28 +++++-
 blockjob.c                             |   1 +
 hw/block/dataplane/virtio-blk.c        |  77 +++++++++++----
 hw/scsi/virtio-scsi.c                  |  59 ++++++++++++
 include/block/block.h                  |   2 -
 include/block/block_int.h              |   8 +-
 include/hw/virtio/virtio-scsi.h        |  10 ++
 include/sysemu/block-backend.h         |   4 +-
 nbd.c                                  |  13 +++
 stubs/Makefile.objs                    |   1 +
 stubs/blockdev-close-all-bdrv-states.c |   5 +
 tests/qemu-iotests/083                 |  13 +--
 tests/qemu-iotests/083.out             |  10 --
 tests/qemu-iotests/117                 |  86 +++++++++++++++++
 tests/qemu-iotests/117.out             |  14 +++
 tests/qemu-iotests/140                 |  92 ++++++++++++++++++
 tests/qemu-iotests/140.out             |  16 ++++
 tests/qemu-iotests/141                 | 166 +++++++++++++++++++++++++++++++++
 tests/qemu-iotests/141.out             |  47 ++++++++++
 tests/qemu-iotests/common.filter       |  13 +++
 tests/qemu-iotests/common.qemu         |  15 ++-
 tests/qemu-iotests/group               |   3 +
 25 files changed, 747 insertions(+), 116 deletions(-)
 create mode 100644 stubs/blockdev-close-all-bdrv-states.c
 create mode 100755 tests/qemu-iotests/117
 create mode 100644 tests/qemu-iotests/117.out
 create mode 100755 tests/qemu-iotests/140
 create mode 100644 tests/qemu-iotests/140.out
 create mode 100755 tests/qemu-iotests/141
 create mode 100644 tests/qemu-iotests/141.out

-- 
2.6.2

[Qemu-devel] [PATCH v7 01/24] blockdev: Add missing bdrv_unref() in drive-backup

[Max Reitz]

All error paths after a successful bdrv_open() of target_bs should
contain a bdrv_unref(target_bs). This one did not yet, so add it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/blockdev.c b/blockdev.c
index 8607df9..3185808 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2914,6 +2914,7 @@ void qmp_drive_backup(const char *device, const char *target,
         bmap = bdrv_find_dirty_bitmap(bs, bitmap);
         if (!bmap) {
             error_setg(errp, "Bitmap '%s' could not be found", bitmap);
+            bdrv_unref(target_bs);
             goto out;
         }
     }
-- 
2.6.2

[Qemu-devel] [PATCH v7 02/24] blockjob: Call bdrv_unref() on creation error

[Max Reitz]

If block_job_create() fails, it should release its reference to the
job's BDS. Normally, this is done in the callback provided by the
caller, but that callback will not be invoked if the block job failed to
be created.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 blockjob.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/blockjob.c b/blockjob.c
index c02fe59..0886a4a 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -70,6 +70,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
         if (local_err) {
             block_job_release(bs);
             error_propagate(errp, local_err);
+            bdrv_unref(bs);
             return NULL;
         }
     }
-- 
2.6.2

[Qemu-devel] [PATCH v7 03/24] block: Release dirty bitmaps in bdrv_close()

[Max Reitz]

bdrv_delete() is not very happy about deleting BlockDriverStates with
dirty bitmaps still attached to them. In the past, we got around that
very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and
bdrv_close() simply ignoring that condition. We should fix that by
releasing all dirty bitmaps in bdrv_close() and drop the assertion in
bdrv_delete().

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index cffac75..94c0ecf 100644
--- a/block.c
+++ b/block.c
@@ -87,6 +87,8 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
                              const BdrvChildRole *child_role, Error **errp);
 
 static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
+static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs);
+
 /* If non-zero, use only whitelisted block drivers */
 static int use_bdrv_whitelist;
 
@@ -1916,6 +1918,8 @@ void bdrv_close(BlockDriverState *bs)
     bdrv_drain(bs); /* in case flush left pending I/O */
     notifier_list_notify(&bs->close_notifiers, bs);
 
+    bdrv_release_all_dirty_bitmaps(bs);
+
     if (bs->blk) {
         blk_dev_change_media_cb(bs->blk, false);
     }
@@ -2123,7 +2127,6 @@ static void bdrv_delete(BlockDriverState *bs)
     assert(!bs->job);
     assert(bdrv_op_blocker_is_empty(bs));
     assert(!bs->refcnt);
-    assert(QLIST_EMPTY(&bs->dirty_bitmaps));
 
     bdrv_close(bs);
 
@@ -3303,21 +3306,39 @@ static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
     }
 }
 
-void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
+                                                  BdrvDirtyBitmap *bitmap)
 {
     BdrvDirtyBitmap *bm, *next;
     QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
-        if (bm == bitmap) {
+        if (!bitmap || bm == bitmap) {
             assert(!bdrv_dirty_bitmap_frozen(bm));
-            QLIST_REMOVE(bitmap, list);
-            hbitmap_free(bitmap->bitmap);
-            g_free(bitmap->name);
-            g_free(bitmap);
-            return;
+            QLIST_REMOVE(bm, list);
+            hbitmap_free(bm->bitmap);
+            g_free(bm->name);
+            g_free(bm);
+
+            if (bitmap) {
+                return;
+            }
         }
     }
 }
 
+void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+    bdrv_do_release_matching_dirty_bitmap(bs, bitmap);
+}
+
+/**
+ * Release all dirty bitmaps attached to a BDS (for use in bdrv_close()). There
+ * must not be any frozen bitmaps attached.
+ */
+static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs)
+{
+    bdrv_do_release_matching_dirty_bitmap(bs, NULL);
+}
+
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
     assert(!bdrv_dirty_bitmap_frozen(bitmap));
-- 
2.6.2

[Qemu-devel] [PATCH v7 04/24] iotests: Rename filter_nbd to _filter_nbd in 083

[Max Reitz]

In the patch after the next, this function is moved to common.filter.
Therefore, its name should be preceded by an underscore to signify its
global availability.

To keep the code motion patch clean, we cannot rename it in the same
patch, so we need to choose some order of renaming vs. motion. It is
better to keep a supposedly global function used by only a single test
in that test than to keep a supposedly local function in a common* file
and use it from a test, so we should rename the function before moving
it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/083 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083
index 1b2d3f1..664f0cf 100755
--- a/tests/qemu-iotests/083
+++ b/tests/qemu-iotests/083
@@ -49,7 +49,7 @@ wait_for_tcp_port() {
 	done
 }
 
-filter_nbd() {
+_filter_nbd() {
 	# nbd.c error messages contain function names and line numbers that are prone
 	# to change.  Message ordering depends on timing between send and receive
 	# callbacks sometimes, making them unreliable.
@@ -84,7 +84,7 @@ EOF
 
 	$PYTHON nbd-fault-injector.py $extra_args "127.0.0.1:$port" "$TEST_DIR/nbd-fault-injector.conf" 2>&1 >/dev/null &
 	wait_for_tcp_port "127\\.0\\.0\\.1:$port"
-	$QEMU_IO -c "read 0 512" "$nbd_url" 2>&1 | _filter_qemu_io | filter_nbd
+	$QEMU_IO -c "read 0 512" "$nbd_url" 2>&1 | _filter_qemu_io | _filter_nbd
 
 	echo
 }
-- 
2.6.2

[Qemu-devel] [PATCH v7 05/24] iotests: Change coding style of _filter_nbd in 083

[Max Reitz]

In order to be able to move _filter_nbd to common.filter in the next
patch, its coding style needs to be adapted to that of common.filter.
That means, we have to convert tabs to four spaces, adjust the alignment
of the last line (done with spaces already, assuming one tab equals
eight spaces), fix the line length of the comment, and add a line break
before the opening brace.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/083 | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083
index 664f0cf..c00a66b 100755
--- a/tests/qemu-iotests/083
+++ b/tests/qemu-iotests/083
@@ -49,15 +49,16 @@ wait_for_tcp_port() {
 	done
 }
 
-_filter_nbd() {
-	# nbd.c error messages contain function names and line numbers that are prone
-	# to change.  Message ordering depends on timing between send and receive
-	# callbacks sometimes, making them unreliable.
-	#
-	# Filter out the TCP port number since this changes between runs.
-	sed -e 's#^.*nbd\.c:.*##g' \
-	    -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \
-            -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#'
+_filter_nbd()
+{
+    # nbd.c error messages contain function names and line numbers that are
+    # prone to change.  Message ordering depends on timing between send and
+    # receive callbacks sometimes, making them unreliable.
+    #
+    # Filter out the TCP port number since this changes between runs.
+    sed -e 's#^.*nbd\.c:.*##g' \
+        -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \
+        -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#'
 }
 
 check_disconnect() {
-- 
2.6.2

[Qemu-devel] [PATCH v7 06/24] iotests: Move _filter_nbd into common.filter

[Max Reitz]

_filter_nbd can be useful for other NBD tests, too, therefore it should
reside in common.filter.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/083           | 12 ------------
 tests/qemu-iotests/common.filter | 12 ++++++++++++
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083
index c00a66b..aa99278 100755
--- a/tests/qemu-iotests/083
+++ b/tests/qemu-iotests/083
@@ -49,18 +49,6 @@ wait_for_tcp_port() {
 	done
 }
 
-_filter_nbd()
-{
-    # nbd.c error messages contain function names and line numbers that are
-    # prone to change.  Message ordering depends on timing between send and
-    # receive callbacks sometimes, making them unreliable.
-    #
-    # Filter out the TCP port number since this changes between runs.
-    sed -e 's#^.*nbd\.c:.*##g' \
-        -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \
-        -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#'
-}
-
 check_disconnect() {
 	event=$1
 	when=$2
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index cfdb633..aa2fb8d 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -230,5 +230,17 @@ _filter_qemu_img_map()
         -e 's/Mapped to *//' | _filter_testdir | _filter_imgfmt
 }
 
+_filter_nbd()
+{
+    # nbd.c error messages contain function names and line numbers that are
+    # prone to change.  Message ordering depends on timing between send and
+    # receive callbacks sometimes, making them unreliable.
+    #
+    # Filter out the TCP port number since this changes between runs.
+    sed -e 's#^.*nbd\.c:.*##g' \
+        -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \
+        -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#'
+}
+
 # make sure this script returns success
 true
-- 
2.6.2

[Qemu-devel] [PATCH v7 07/24] iotests: Make _filter_nbd drop log lines

[Max Reitz]

The NBD log lines ("/your/source/dir/nbd.c:function():line: error")
should not be converted to empty lines but removed altogether.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/083.out       | 10 ----------
 tests/qemu-iotests/common.filter |  2 +-
 2 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/tests/qemu-iotests/083.out b/tests/qemu-iotests/083.out
index 8c1441b..5c9141b 100644
--- a/tests/qemu-iotests/083.out
+++ b/tests/qemu-iotests/083.out
@@ -51,7 +51,6 @@ no file open, try 'help open'
 
 === Check disconnect after neg2 ===
 
-
 read failed: Input/output error
 
 === Check disconnect 8 neg2 ===
@@ -66,42 +65,34 @@ no file open, try 'help open'
 
 === Check disconnect before request ===
 
-
 read failed: Input/output error
 
 === Check disconnect after request ===
 
-
 read failed: Input/output error
 
 === Check disconnect before reply ===
 
-
 read failed: Input/output error
 
 === Check disconnect after reply ===
 
-
 read failed: Input/output error
 
 === Check disconnect 4 reply ===
 
-
 read failed: Input/output error
 
 === Check disconnect 8 reply ===
 
-
 read failed: Input/output error
 
 === Check disconnect before data ===
 
-
 read failed: Input/output error
 
 === Check disconnect after data ===
 
-
 read 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
@@ -132,7 +123,6 @@ no file open, try 'help open'
 
 === Check disconnect after neg-classic ===
 
-
 read failed: Input/output error
 
 *** done
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index aa2fb8d..0fc443a 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -237,7 +237,7 @@ _filter_nbd()
     # receive callbacks sometimes, making them unreliable.
     #
     # Filter out the TCP port number since this changes between runs.
-    sed -e 's#^.*nbd\.c:.*##g' \
+    sed -e '/nbd\.c:/d' \
         -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \
         -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#'
 }
-- 
2.6.2

[Qemu-devel] [PATCH v7 08/24] iotests: Make _filter_nbd support more URL types

[Max Reitz]

This function should support URLs of the "nbd://" format (without
swallowing the export name), and for "nbd:///" URLs it should replace
"?socket=$TEST_DIR" by "?socket=TEST_DIR" because putting the Unix
socket files into the test directory makes sense.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/common.filter | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 0fc443a..bd53cab 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -238,7 +238,8 @@ _filter_nbd()
     #
     # Filter out the TCP port number since this changes between runs.
     sed -e '/nbd\.c:/d' \
-        -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \
+        -e 's#nbd:\(//\)\?127\.0\.0\.1:[0-9]*#nbd:\1127.0.0.1:PORT#g' \
+        -e "s#?socket=$TEST_DIR#?socket=TEST_DIR#g" \
         -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#'
 }
 
-- 
2.6.2

[Qemu-devel] [PATCH v7 09/24] iotests: Make redirecting qemu's stderr optional

[Max Reitz]

Redirecting qemu's stderr to stdout makes working with the stderr output
difficult due to the other file descriptor magic performed in
_launch_qemu ("ambiguous redirect").

Add an option which specifies whether stderr should be redirected to
stdout or not (allowing for other modes to be added in the future).

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/common.qemu | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
index 8bf3969..2548a87 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -129,6 +129,8 @@ function _send_qemu_cmd()
 # $qemu_comm_method: set this variable to 'monitor' (case insensitive)
 #                    to use the QEMU HMP monitor for communication.
 #                    Otherwise, the default of QMP is used.
+# $keep_stderr: Set this variable to 'y' to keep QEMU's stderr output on stderr.
+#               If this variable is empty, stderr will be redirected to stdout.
 # Returns:
 # $QEMU_HANDLE: set to a handle value to communicate with this QEMU instance.
 #
@@ -151,11 +153,20 @@ function _launch_qemu()
     mkfifo "${fifo_out}"
     mkfifo "${fifo_in}"
 
-    QEMU_NEED_PID='y'\
-    ${QEMU} -nographic -serial none ${comm} -machine accel=qtest "${@}" \
+    if [ -z "$keep_stderr" ]; then
+        QEMU_NEED_PID='y'\
+        ${QEMU} -nographic -serial none ${comm} -machine accel=qtest "${@}" \
                                                                 >"${fifo_out}" \
                                                                 2>&1 \
                                                                 <"${fifo_in}" &
+    elif [ "$keep_stderr" = "y" ]; then
+        QEMU_NEED_PID='y'\
+        ${QEMU} -nographic -serial none ${comm} -machine accel=qtest "${@}" \
+                                                                >"${fifo_out}" \
+                                                                <"${fifo_in}" &
+    else
+        exit 1
+    fi
 
     if [[ "${BASH_VERSINFO[0]}" -ge "5" ||
         ("${BASH_VERSINFO[0]}" -ge "4"  &&  "${BASH_VERSINFO[1]}" -ge "1") ]]
-- 
2.6.2

[Qemu-devel] [PATCH v7 10/24] iotests: Add test for eject under NBD server

[Max Reitz]

This patch adds a test for ejecting the BlockBackend an NBD server is
connected to (the NBD server is supposed to stop).

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

diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140
new file mode 100755
index 0000000..3434997
--- /dev/null
+++ b/tests/qemu-iotests/140
@@ -0,0 +1,92 @@
+#!/bin/bash
+#
+# Test case for ejecting a BB with an NBD server attached to it
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@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_DIR/nbd"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt generic
+_supported_proto file
+_supported_os Linux
+
+_make_test_img 64k
+
+$QEMU_IO -c 'write -P 42 0 64k' "$TEST_IMG" | _filter_qemu_io
+
+keep_stderr=y \
+_launch_qemu -drive if=ide,media=cdrom,id=drv,file="$TEST_IMG",format=$IMGFMT \
+    2> >(_filter_nbd)
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'qmp_capabilities' }" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'nbd-server-start',
+       'arguments': { 'addr': { 'type': 'unix',
+                                'data': { 'path': '$TEST_DIR/nbd' }}}}" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'nbd-server-add',
+       'arguments': { 'device': 'drv' }}" \
+    'return'
+
+$QEMU_IO_PROG -f raw -c 'read -P 42 0 64k' \
+    "nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \
+    | _filter_qemu_io | _filter_nbd
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'eject',
+       'arguments': { 'device': 'drv' }}" \
+    'return'
+
+$QEMU_IO_PROG -f raw -c close \
+    "nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \
+    | _filter_qemu_io | _filter_nbd
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'quit' }" \
+    'return'
+
+wait=1 _cleanup_qemu
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/140.out b/tests/qemu-iotests/140.out
new file mode 100644
index 0000000..0087909
--- /dev/null
+++ b/tests/qemu-iotests/140.out
@@ -0,0 +1,16 @@
+QA output created by 140
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": {}}
+{"return": {}}
+{"return": {}}
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "drv", "tray-open": true}}
+{"return": {}}
+qemu-io: can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: Failed to read export length
+no file open, try 'help open'
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index c69265d..993711b 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -139,3 +139,4 @@
 137 rw auto
 138 rw auto quick
 139 rw auto quick
+140 rw auto quick
-- 
2.6.2

[Qemu-devel] [PATCH v7 11/24] block: Add BB-BDS remove/insert notifiers

[Max Reitz]

bdrv_close() no longer signifies ejection of a medium, this is now done
by removing the BDS from the BB. Therefore, we want to have a notifier
for that in the BB instead of a close notifier in the BDS. The former is
added now, the latter is removed later.

Symmetrically, another notifier list is added that is invoked whenever a
BDS is inserted. We will need that for virtio-blk and virtio-scsi, which
can then remove their op blockers on BDS ejection and set them up on
insertion.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c          | 20 ++++++++++++++++++++
 include/sysemu/block-backend.h |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 6f9309f..8f74a54 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -48,6 +48,8 @@ struct BlockBackend {
     BlockdevOnError on_read_error, on_write_error;
     bool iostatus_enabled;
     BlockDeviceIoStatus iostatus;
+
+    NotifierList remove_bs_notifiers, insert_bs_notifiers;
 };
 
 typedef struct BlockBackendAIOCB {
@@ -98,6 +100,8 @@ BlockBackend *blk_new(const char *name, Error **errp)
     blk = g_new0(BlockBackend, 1);
     blk->name = g_strdup(name);
     blk->refcnt = 1;
+    notifier_list_init(&blk->remove_bs_notifiers);
+    notifier_list_init(&blk->insert_bs_notifiers);
     QTAILQ_INSERT_TAIL(&blk_backends, blk, link);
     return blk;
 }
@@ -166,6 +170,8 @@ static void blk_delete(BlockBackend *blk)
         bdrv_unref(blk->bs);
         blk->bs = NULL;
     }
+    assert(QLIST_EMPTY(&blk->remove_bs_notifiers.notifiers));
+    assert(QLIST_EMPTY(&blk->insert_bs_notifiers.notifiers));
     if (blk->root_state.throttle_state) {
         g_free(blk->root_state.throttle_group);
         throttle_group_unref(blk->root_state.throttle_state);
@@ -343,6 +349,8 @@ void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk)
  */
 void blk_remove_bs(BlockBackend *blk)
 {
+    notifier_list_notify(&blk->remove_bs_notifiers, blk);
+
     blk_update_root_state(blk);
 
     blk->bs->blk = NULL;
@@ -359,6 +367,8 @@ void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
     bdrv_ref(bs);
     blk->bs = bs;
     bs->blk = blk;
+
+    notifier_list_notify(&blk->insert_bs_notifiers, blk);
 }
 
 /*
@@ -1105,6 +1115,16 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
     }
 }
 
+void blk_add_remove_bs_notifier(BlockBackend *blk, Notifier *notify)
+{
+    notifier_list_add(&blk->remove_bs_notifiers, notify);
+}
+
+void blk_add_insert_bs_notifier(BlockBackend *blk, Notifier *notify)
+{
+    notifier_list_add(&blk->insert_bs_notifiers, notify);
+}
+
 void blk_add_close_notifier(BlockBackend *blk, Notifier *notify)
 {
     if (blk->bs) {
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index f4a68e2..632cdc0 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -162,6 +162,8 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
                                                                   void *),
                                      void (*detach_aio_context)(void *),
                                      void *opaque);
+void blk_add_remove_bs_notifier(BlockBackend *blk, Notifier *notify);
+void blk_add_insert_bs_notifier(BlockBackend *blk, Notifier *notify);
 void blk_add_close_notifier(BlockBackend *blk, Notifier *notify);
 void blk_io_plug(BlockBackend *blk);
 void blk_io_unplug(BlockBackend *blk);
-- 
2.6.2

[Qemu-devel] [PATCH v7 12/24] virtio-blk: Functions for op blocker management

[Max Reitz]

Put the code for setting up and removing op blockers into an own
function, respectively. Then, we can invoke those functions whenever a
BDS is removed from an virtio-blk BB or inserted into it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 77 +++++++++++++++++++++++++++++++----------
 1 file changed, 59 insertions(+), 18 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index c42ddeb..4c95d5b 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -39,6 +39,8 @@ struct VirtIOBlockDataPlane {
     EventNotifier *guest_notifier;  /* irq */
     QEMUBH *bh;                     /* bh for guest notification */
 
+    Notifier insert_notifier, remove_notifier;
+
     /* Note that these EventNotifiers are assigned by value.  This is
      * fine as long as you do not call event_notifier_cleanup on them
      * (because you don't own the file descriptor or handle; you just
@@ -137,6 +139,54 @@ static void handle_notify(EventNotifier *e)
     blk_io_unplug(s->conf->conf.blk);
 }
 
+static void data_plane_set_up_op_blockers(VirtIOBlockDataPlane *s)
+{
+    assert(!s->blocker);
+    error_setg(&s->blocker, "block device is in use by data plane");
+    blk_op_block_all(s->conf->conf.blk, s->blocker);
+    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_RESIZE, s->blocker);
+    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_DRIVE_DEL, s->blocker);
+    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, s->blocker);
+    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_CHANGE, s->blocker);
+    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, s->blocker);
+    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_TARGET, s->blocker);
+    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EJECT, s->blocker);
+    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
+                   s->blocker);
+    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT,
+                   s->blocker);
+    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
+                   s->blocker);
+    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_MIRROR, s->blocker);
+    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_STREAM, s->blocker);
+    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_REPLACE, s->blocker);
+}
+
+static void data_plane_remove_op_blockers(VirtIOBlockDataPlane *s)
+{
+    if (s->blocker) {
+        blk_op_unblock_all(s->conf->conf.blk, s->blocker);
+        error_free(s->blocker);
+        s->blocker = NULL;
+    }
+}
+
+static void data_plane_blk_insert_notifier(Notifier *n, void *data)
+{
+    VirtIOBlockDataPlane *s = container_of(n, VirtIOBlockDataPlane,
+                                           insert_notifier);
+    assert(s->conf->conf.blk == data);
+    data_plane_set_up_op_blockers(s);
+}
+
+static void data_plane_blk_remove_notifier(Notifier *n, void *data)
+{
+    VirtIOBlockDataPlane *s = container_of(n, VirtIOBlockDataPlane,
+                                           remove_notifier);
+    assert(s->conf->conf.blk == data);
+    data_plane_remove_op_blockers(s);
+}
+
 /* Context: QEMU global mutex held */
 void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
                                   VirtIOBlockDataPlane **dataplane,
@@ -193,22 +243,12 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
     s->ctx = iothread_get_aio_context(s->iothread);
     s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
 
-    error_setg(&s->blocker, "block device is in use by data plane");
-    blk_op_block_all(conf->conf.blk, s->blocker);
-    blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_RESIZE, s->blocker);
-    blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_DRIVE_DEL, s->blocker);
-    blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, s->blocker);
-    blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_CHANGE, s->blocker);
-    blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, s->blocker);
-    blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_COMMIT_TARGET, s->blocker);
-    blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_EJECT, s->blocker);
-    blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, s->blocker);
-    blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, s->blocker);
-    blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
-                   s->blocker);
-    blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_MIRROR, s->blocker);
-    blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_STREAM, s->blocker);
-    blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_REPLACE, s->blocker);
+    s->insert_notifier.notify = data_plane_blk_insert_notifier;
+    s->remove_notifier.notify = data_plane_blk_remove_notifier;
+    blk_add_insert_bs_notifier(conf->conf.blk, &s->insert_notifier);
+    blk_add_remove_bs_notifier(conf->conf.blk, &s->remove_notifier);
+
+    data_plane_set_up_op_blockers(s);
 
     *dataplane = s;
 }
@@ -221,8 +261,9 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
     }
 
     virtio_blk_data_plane_stop(s);
-    blk_op_unblock_all(s->conf->conf.blk, s->blocker);
-    error_free(s->blocker);
+    data_plane_remove_op_blockers(s);
+    notifier_remove(&s->insert_notifier);
+    notifier_remove(&s->remove_notifier);
     qemu_bh_delete(s->bh);
     object_unref(OBJECT(s->iothread));
     g_free(s);
-- 
2.6.2

[Qemu-devel] [PATCH v7 13/24] virtio-scsi: Catch BDS-BB removal/insertion

[Max Reitz]

Make use of the BDS-BB removal and insertion notifiers to remove or set
up, respectively, virtio-scsi's op blockers.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 hw/scsi/virtio-scsi.c           | 59 +++++++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio-scsi.h | 10 +++++++
 2 files changed, 69 insertions(+)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 7655401..02606e6 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -757,6 +757,22 @@ static void virtio_scsi_change(SCSIBus *bus, SCSIDevice *dev, SCSISense sense)
     }
 }
 
+static void virtio_scsi_blk_insert_notifier(Notifier *n, void *data)
+{
+    VirtIOSCSIBlkChangeNotifier *cn = DO_UPCAST(VirtIOSCSIBlkChangeNotifier,
+                                                n, n);
+    assert(cn->sd->conf.blk == data);
+    blk_op_block_all(cn->sd->conf.blk, cn->s->blocker);
+}
+
+static void virtio_scsi_blk_remove_notifier(Notifier *n, void *data)
+{
+    VirtIOSCSIBlkChangeNotifier *cn = DO_UPCAST(VirtIOSCSIBlkChangeNotifier,
+                                                n, n);
+    assert(cn->sd->conf.blk == data);
+    blk_op_unblock_all(cn->sd->conf.blk, cn->s->blocker);
+}
+
 static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                 Error **errp)
 {
@@ -765,6 +781,22 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
     SCSIDevice *sd = SCSI_DEVICE(dev);
 
     if (s->ctx && !s->dataplane_disabled) {
+        VirtIOSCSIBlkChangeNotifier *insert_notifier, *remove_notifier;
+
+        insert_notifier = g_new0(VirtIOSCSIBlkChangeNotifier, 1);
+        insert_notifier->n.notify = virtio_scsi_blk_insert_notifier;
+        insert_notifier->s = s;
+        insert_notifier->sd = sd;
+        blk_add_insert_bs_notifier(sd->conf.blk, &insert_notifier->n);
+        QTAILQ_INSERT_TAIL(&s->insert_notifiers, insert_notifier, next);
+
+        remove_notifier = g_new0(VirtIOSCSIBlkChangeNotifier, 1);
+        remove_notifier->n.notify = virtio_scsi_blk_remove_notifier;
+        remove_notifier->s = s;
+        remove_notifier->sd = sd;
+        blk_add_remove_bs_notifier(sd->conf.blk, &remove_notifier->n);
+        QTAILQ_INSERT_TAIL(&s->remove_notifiers, remove_notifier, next);
+
         if (blk_op_is_blocked(sd->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
             return;
         }
@@ -787,6 +819,7 @@ static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev,
     VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev);
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
     SCSIDevice *sd = SCSI_DEVICE(dev);
+    VirtIOSCSIBlkChangeNotifier *insert_notifier, *remove_notifier;
 
     if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) {
         virtio_scsi_push_event(s, sd,
@@ -797,6 +830,29 @@ static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev,
     if (s->ctx) {
         blk_op_unblock_all(sd->conf.blk, s->blocker);
     }
+
+    QTAILQ_FOREACH(insert_notifier, &s->insert_notifiers, next) {
+        if (insert_notifier->sd == sd) {
+            break;
+        }
+    }
+    if (insert_notifier) {
+        notifier_remove(&insert_notifier->n);
+        QTAILQ_REMOVE(&s->insert_notifiers, insert_notifier, next);
+        g_free(insert_notifier);
+    }
+
+    QTAILQ_FOREACH(remove_notifier, &s->remove_notifiers, next) {
+        if (remove_notifier->sd == sd) {
+            break;
+        }
+    }
+    if (remove_notifier) {
+        notifier_remove(&remove_notifier->n);
+        QTAILQ_REMOVE(&s->remove_notifiers, remove_notifier, next);
+        g_free(remove_notifier);
+    }
+
     qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
 }
 
@@ -911,6 +967,9 @@ static void virtio_scsi_device_realize(DeviceState *dev, Error **errp)
     add_migration_state_change_notifier(&s->migration_state_notifier);
 
     error_setg(&s->blocker, "block device is in use by data plane");
+
+    QTAILQ_INIT(&s->insert_notifiers);
+    QTAILQ_INIT(&s->remove_notifiers);
 }
 
 static void virtio_scsi_instance_init(Object *obj)
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 088fe9f..0394eb2 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -76,6 +76,13 @@ typedef struct VirtIOSCSICommon {
     VirtQueue **cmd_vqs;
 } VirtIOSCSICommon;
 
+typedef struct VirtIOSCSIBlkChangeNotifier {
+    Notifier n;
+    struct VirtIOSCSI *s;
+    SCSIDevice *sd;
+    QTAILQ_ENTRY(VirtIOSCSIBlkChangeNotifier) next;
+} VirtIOSCSIBlkChangeNotifier;
+
 typedef struct VirtIOSCSI {
     VirtIOSCSICommon parent_obj;
 
@@ -86,6 +93,9 @@ typedef struct VirtIOSCSI {
     /* Fields for dataplane below */
     AioContext *ctx; /* one iothread per virtio-scsi-pci for now */
 
+    QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) insert_notifiers;
+    QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) remove_notifiers;
+
     /* Vring is used instead of vq in dataplane code, because of the underlying
      * memory layer thread safety */
     VirtIOSCSIVring *ctrl_vring;
-- 
2.6.2

[Qemu-devel] [PATCH v7 14/24] nbd: Switch from close to eject notifier

[Max Reitz]

The NBD code uses the BDS close notifier to determine when a medium is
ejected. However, now it should use the BB's BDS removal notifier for
that instead of the BDS's close notifier.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 blockdev-nbd.c | 37 +------------------------------------
 nbd.c          | 13 +++++++++++++
 2 files changed, 14 insertions(+), 36 deletions(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index bcdd18b..b28a55b 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -46,37 +46,11 @@ void qmp_nbd_server_start(SocketAddress *addr, Error **errp)
     }
 }
 
-/*
- * Hook into the BlockBackend notifiers to close the export when the
- * backend is closed.
- */
-typedef struct NBDCloseNotifier {
-    Notifier n;
-    NBDExport *exp;
-    QTAILQ_ENTRY(NBDCloseNotifier) next;
-} NBDCloseNotifier;
-
-static QTAILQ_HEAD(, NBDCloseNotifier) close_notifiers =
-    QTAILQ_HEAD_INITIALIZER(close_notifiers);
-
-static void nbd_close_notifier(Notifier *n, void *data)
-{
-    NBDCloseNotifier *cn = DO_UPCAST(NBDCloseNotifier, n, n);
-
-    notifier_remove(&cn->n);
-    QTAILQ_REMOVE(&close_notifiers, cn, next);
-
-    nbd_export_close(cn->exp);
-    nbd_export_put(cn->exp);
-    g_free(cn);
-}
-
 void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
                         Error **errp)
 {
     BlockBackend *blk;
     NBDExport *exp;
-    NBDCloseNotifier *n;
 
     if (server_fd == -1) {
         error_setg(errp, "NBD server not running");
@@ -113,20 +87,11 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
     }
 
     nbd_export_set_name(exp, device);
-
-    n = g_new0(NBDCloseNotifier, 1);
-    n->n.notify = nbd_close_notifier;
-    n->exp = exp;
-    blk_add_close_notifier(blk, &n->n);
-    QTAILQ_INSERT_TAIL(&close_notifiers, n, next);
 }
 
 void qmp_nbd_server_stop(Error **errp)
 {
-    while (!QTAILQ_EMPTY(&close_notifiers)) {
-        NBDCloseNotifier *cn = QTAILQ_FIRST(&close_notifiers);
-        nbd_close_notifier(&cn->n, nbd_export_get_blockdev(cn->exp));
-    }
+    nbd_export_close_all();
 
     if (server_fd != -1) {
         qemu_set_fd_handler(server_fd, NULL, NULL, NULL);
diff --git a/nbd.c b/nbd.c
index b3d9654..ab2e725 100644
--- a/nbd.c
+++ b/nbd.c
@@ -162,6 +162,8 @@ struct NBDExport {
     QTAILQ_ENTRY(NBDExport) next;
 
     AioContext *ctx;
+
+    Notifier eject_notifier;
 };
 
 static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
@@ -1053,6 +1055,12 @@ static void blk_aio_detach(void *opaque)
     exp->ctx = NULL;
 }
 
+static void nbd_eject_notifier(Notifier *n, void *data)
+{
+    NBDExport *exp = container_of(n, NBDExport, eject_notifier);
+    nbd_export_close(exp);
+}
+
 NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
                           uint32_t nbdflags, void (*close)(NBDExport *),
                           Error **errp)
@@ -1075,6 +1083,10 @@ NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
     exp->ctx = blk_get_aio_context(blk);
     blk_ref(blk);
     blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
+
+    exp->eject_notifier.notify = nbd_eject_notifier;
+    blk_add_remove_bs_notifier(blk, &exp->eject_notifier);
+
     /*
      * NBD exports are used for non-shared storage migration.  Make sure
      * that BDRV_O_INCOMING is cleared and the image is ready for write
@@ -1154,6 +1166,7 @@ void nbd_export_put(NBDExport *exp)
         }
 
         if (exp->blk) {
+            notifier_remove(&exp->eject_notifier);
             blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
                                             blk_aio_detach, exp);
             blk_unref(exp->blk);
-- 
2.6.2

[Qemu-devel] [PATCH v7 15/24] block: Remove BDS close notifier

[Max Reitz]

It is unused now, so we can remove it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                        | 7 -------
 block/block-backend.c          | 7 -------
 include/block/block.h          | 1 -
 include/block/block_int.h      | 2 --
 include/sysemu/block-backend.h | 1 -
 5 files changed, 18 deletions(-)

diff --git a/block.c b/block.c
index 94c0ecf..4ee3b01 100644
--- a/block.c
+++ b/block.c
@@ -258,7 +258,6 @@ BlockDriverState *bdrv_new(void)
     for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
         QLIST_INIT(&bs->op_blockers[i]);
     }
-    notifier_list_init(&bs->close_notifiers);
     notifier_with_return_list_init(&bs->before_write_notifiers);
     qemu_co_queue_init(&bs->throttled_reqs[0]);
     qemu_co_queue_init(&bs->throttled_reqs[1]);
@@ -268,11 +267,6 @@ BlockDriverState *bdrv_new(void)
     return bs;
 }
 
-void bdrv_add_close_notifier(BlockDriverState *bs, Notifier *notify)
-{
-    notifier_list_add(&bs->close_notifiers, notify);
-}
-
 BlockDriver *bdrv_find_format(const char *format_name)
 {
     BlockDriver *drv1;
@@ -1916,7 +1910,6 @@ void bdrv_close(BlockDriverState *bs)
     bdrv_drain(bs); /* complete I/O */
     bdrv_flush(bs);
     bdrv_drain(bs); /* in case flush left pending I/O */
-    notifier_list_notify(&bs->close_notifiers, bs);
 
     bdrv_release_all_dirty_bitmaps(bs);
 
diff --git a/block/block-backend.c b/block/block-backend.c
index 8f74a54..a00486c 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1125,13 +1125,6 @@ void blk_add_insert_bs_notifier(BlockBackend *blk, Notifier *notify)
     notifier_list_add(&blk->insert_bs_notifiers, notify);
 }
 
-void blk_add_close_notifier(BlockBackend *blk, Notifier *notify)
-{
-    if (blk->bs) {
-        bdrv_add_close_notifier(blk->bs, notify);
-    }
-}
-
 void blk_io_plug(BlockBackend *blk)
 {
     if (blk->bs) {
diff --git a/include/block/block.h b/include/block/block.h
index 610db92..8d9a049 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -223,7 +223,6 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
 void bdrv_reopen_commit(BDRVReopenState *reopen_state);
 void bdrv_reopen_abort(BDRVReopenState *reopen_state);
 void bdrv_close(BlockDriverState *bs);
-void bdrv_add_close_notifier(BlockDriverState *bs, Notifier *notify);
 int bdrv_read(BlockDriverState *bs, int64_t sector_num,
               uint8_t *buf, int nb_sectors);
 int bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 603145a..617994d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -382,8 +382,6 @@ struct BlockDriverState {
     BdrvChild *backing;
     BdrvChild *file;
 
-    NotifierList close_notifiers;
-
     /* Callback before write request is processed */
     NotifierWithReturnList before_write_notifiers;
 
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 632cdc0..abb52be 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -164,7 +164,6 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
                                      void *opaque);
 void blk_add_remove_bs_notifier(BlockBackend *blk, Notifier *notify);
 void blk_add_insert_bs_notifier(BlockBackend *blk, Notifier *notify);
-void blk_add_close_notifier(BlockBackend *blk, Notifier *notify);
 void blk_io_plug(BlockBackend *blk);
 void blk_io_unplug(BlockBackend *blk);
 BlockAcctStats *blk_get_stats(BlockBackend *blk);
-- 
2.6.2

[Qemu-devel] [PATCH v7 16/24] block: Use blk_remove_bs() in blk_delete()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index a00486c..4523387 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -165,10 +165,7 @@ static void blk_delete(BlockBackend *blk)
     assert(!blk->refcnt);
     assert(!blk->dev);
     if (blk->bs) {
-        assert(blk->bs->blk == blk);
-        blk->bs->blk = NULL;
-        bdrv_unref(blk->bs);
-        blk->bs = NULL;
+        blk_remove_bs(blk);
     }
     assert(QLIST_EMPTY(&blk->remove_bs_notifiers.notifiers));
     assert(QLIST_EMPTY(&blk->insert_bs_notifiers.notifiers));
@@ -349,6 +346,8 @@ void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk)
  */
 void blk_remove_bs(BlockBackend *blk)
 {
+    assert(blk->bs->blk == blk);
+
     notifier_list_notify(&blk->remove_bs_notifiers, blk);
 
     blk_update_root_state(blk);
-- 
2.6.2

[Qemu-devel] [PATCH v7 17/24] blockdev: Use blk_remove_bs() in do_drive_del()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 3185808..a70a684 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2509,7 +2509,7 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
             return;
         }
 
-        bdrv_close(bs);
+        blk_remove_bs(blk);
     }
 
     /* if we have a device attached to this BlockDriverState
-- 
2.6.2

[Qemu-devel] [PATCH v7 18/24] block: Make bdrv_close() static

[Max Reitz]

There are no users of bdrv_close() left, except for one of bdrv_open()'s
failure paths, bdrv_close_all() and bdrv_delete(), and that is good.
Make bdrv_close() static so nobody makes the mistake of directly using
bdrv_close() again.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c               | 4 +++-
 include/block/block.h | 1 -
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 4ee3b01..b5874aa 100644
--- a/block.c
+++ b/block.c
@@ -92,6 +92,8 @@ static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs);
 /* If non-zero, use only whitelisted block drivers */
 static int use_bdrv_whitelist;
 
+static void bdrv_close(BlockDriverState *bs);
+
 #ifdef _WIN32
 static int is_windows_drive_prefix(const char *filename)
 {
@@ -1894,7 +1896,7 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
 }
 
 
-void bdrv_close(BlockDriverState *bs)
+static void bdrv_close(BlockDriverState *bs)
 {
     BdrvAioNotifier *ban, *ban_next;
 
diff --git a/include/block/block.h b/include/block/block.h
index 8d9a049..03551d3 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -222,7 +222,6 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
                         BlockReopenQueue *queue, Error **errp);
 void bdrv_reopen_commit(BDRVReopenState *reopen_state);
 void bdrv_reopen_abort(BDRVReopenState *reopen_state);
-void bdrv_close(BlockDriverState *bs);
 int bdrv_read(BlockDriverState *bs, int64_t sector_num,
               uint8_t *buf, int nb_sectors);
 int bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num,
-- 
2.6.2

[Qemu-devel] [PATCH v7 19/24] block: Add list of all BlockDriverStates

[Max Reitz]

We need this list so that bdrv_close_all() can keep track of which BDSs
are still open after having removed the BDSs from all of the BBs and
having released all monitor BDS references.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                   | 7 +++++++
 include/block/block_int.h | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/block.c b/block.c
index b5874aa..b935dff 100644
--- a/block.c
+++ b/block.c
@@ -78,6 +78,9 @@ struct BdrvStates bdrv_states = QTAILQ_HEAD_INITIALIZER(bdrv_states);
 static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
     QTAILQ_HEAD_INITIALIZER(graph_bdrv_states);
 
+static QTAILQ_HEAD(, BlockDriverState) all_bdrv_states =
+    QTAILQ_HEAD_INITIALIZER(all_bdrv_states);
+
 static QLIST_HEAD(, BlockDriver) bdrv_drivers =
     QLIST_HEAD_INITIALIZER(bdrv_drivers);
 
@@ -266,6 +269,8 @@ BlockDriverState *bdrv_new(void)
     bs->refcnt = 1;
     bs->aio_context = qemu_get_aio_context();
 
+    QTAILQ_INSERT_TAIL(&all_bdrv_states, bs, bs_list);
+
     return bs;
 }
 
@@ -2128,6 +2133,8 @@ static void bdrv_delete(BlockDriverState *bs)
     /* remove from list, if necessary */
     bdrv_make_anon(bs);
 
+    QTAILQ_REMOVE(&all_bdrv_states, bs, bs_list);
+
     g_free(bs);
 }
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 617994d..8c82747 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -422,6 +422,8 @@ struct BlockDriverState {
     QTAILQ_ENTRY(BlockDriverState) node_list;
     /* element of the list of "drives" the guest sees */
     QTAILQ_ENTRY(BlockDriverState) device_list;
+    /* element of the list of all BlockDriverStates (all_bdrv_states) */
+    QTAILQ_ENTRY(BlockDriverState) bs_list;
     QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
     int refcnt;
 
-- 
2.6.2

[Qemu-devel] [PATCH v7 20/24] blockdev: Keep track of monitor-owned BDS

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c                             | 25 +++++++++++++++++++++++++
 include/block/block_int.h              |  4 ++++
 stubs/Makefile.objs                    |  1 +
 stubs/blockdev-close-all-bdrv-states.c |  5 +++++
 4 files changed, 35 insertions(+)
 create mode 100644 stubs/blockdev-close-all-bdrv-states.c

diff --git a/blockdev.c b/blockdev.c
index a70a684..8211eae 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -50,6 +50,9 @@
 #include "trace.h"
 #include "sysemu/arch_init.h"
 
+static QTAILQ_HEAD(, BlockDriverState) monitor_bdrv_states =
+    QTAILQ_HEAD_INITIALIZER(monitor_bdrv_states);
+
 static const char *const if_name[IF_COUNT] = {
     [IF_NONE] = "none",
     [IF_IDE] = "ide",
@@ -662,6 +665,19 @@ fail:
     return NULL;
 }
 
+void blockdev_close_all_bdrv_states(void)
+{
+    BlockDriverState *bs, *next_bs;
+
+    QTAILQ_FOREACH_SAFE(bs, &monitor_bdrv_states, monitor_list, next_bs) {
+        AioContext *ctx = bdrv_get_aio_context(bs);
+
+        aio_context_acquire(ctx);
+        bdrv_unref(bs);
+        aio_context_release(ctx);
+    }
+}
+
 static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to,
                             Error **errp)
 {
@@ -3464,6 +3480,8 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
         if (!bs) {
             goto fail;
         }
+
+        QTAILQ_INSERT_TAIL(&monitor_bdrv_states, bs, monitor_list);
     }
 
     if (bs && bdrv_key_required(bs)) {
@@ -3534,11 +3552,18 @@ void qmp_x_blockdev_del(bool has_id, const char *id,
                        bdrv_get_device_or_node_name(bs));
             goto out;
         }
+
+        if (!blk && !bs->monitor_list.tqe_prev) {
+            error_setg(errp, "Node %s is not owned by the monitor",
+                       bs->node_name);
+            goto out;
+        }
     }
 
     if (blk) {
         blk_unref(blk);
     } else {
+        QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list);
         bdrv_unref(bs);
     }
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8c82747..7408eef 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -424,6 +424,8 @@ struct BlockDriverState {
     QTAILQ_ENTRY(BlockDriverState) device_list;
     /* element of the list of all BlockDriverStates (all_bdrv_states) */
     QTAILQ_ENTRY(BlockDriverState) bs_list;
+    /* element of the list of monitor-owned BDS */
+    QTAILQ_ENTRY(BlockDriverState) monitor_list;
     QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
     int refcnt;
 
@@ -680,4 +682,6 @@ void blk_dev_resize_cb(BlockBackend *blk);
 void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
 bool bdrv_requests_pending(BlockDriverState *bs);
 
+void blockdev_close_all_bdrv_states(void);
+
 #endif /* BLOCK_INT_H */
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 330e1a4..c1f02be 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -1,5 +1,6 @@
 stub-obj-y += arch-query-cpu-def.o
 stub-obj-y += bdrv-commit-all.o
+stub-obj-y += blockdev-close-all-bdrv-states.o
 stub-obj-y += clock-warp.o
 stub-obj-y += cpu-get-clock.o
 stub-obj-y += cpu-get-icount.o
diff --git a/stubs/blockdev-close-all-bdrv-states.c b/stubs/blockdev-close-all-bdrv-states.c
new file mode 100644
index 0000000..12d2442
--- /dev/null
+++ b/stubs/blockdev-close-all-bdrv-states.c
@@ -0,0 +1,5 @@
+#include "block/block_int.h"
+
+void blockdev_close_all_bdrv_states(void)
+{
+}
-- 
2.6.2

[Qemu-devel] [PATCH v7 21/24] block: Add blk_remove_all_bs()

[Max Reitz]

When bdrv_close_all() is called, instead of force-closing all root
BlockDriverStates, it is better to just drop the reference from all
BlockBackends and let them be closed automatically. This prevents BDS
from getting closed that are still referenced by other BDS, which may
result in loss of cached data.

This patch adds a function for doing that, but does not yet incorporate
it in bdrv_close_all().

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c          | 15 +++++++++++++++
 include/sysemu/block-backend.h |  1 +
 2 files changed, 16 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 4523387..7373093 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -221,6 +221,21 @@ void blk_unref(BlockBackend *blk)
     }
 }
 
+void blk_remove_all_bs(void)
+{
+    BlockBackend *blk;
+
+    QTAILQ_FOREACH(blk, &blk_backends, link) {
+        AioContext *ctx = blk_get_aio_context(blk);
+
+        aio_context_acquire(ctx);
+        if (blk->bs) {
+            blk_remove_bs(blk);
+        }
+        aio_context_release(ctx);
+    }
+}
+
 /*
  * Return the BlockBackend after @blk.
  * If @blk is null, return the first one.
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index abb52be..9fb3e54 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -68,6 +68,7 @@ BlockBackend *blk_new_open(const char *name, const char *filename,
 int blk_get_refcnt(BlockBackend *blk);
 void blk_ref(BlockBackend *blk);
 void blk_unref(BlockBackend *blk);
+void blk_remove_all_bs(void);
 const char *blk_name(BlockBackend *blk);
 BlockBackend *blk_by_name(const char *name);
 BlockBackend *blk_next(BlockBackend *blk);
-- 
2.6.2

[Qemu-devel] [PATCH v7 22/24] block: Rewrite bdrv_close_all()

[Max Reitz]

This patch rewrites bdrv_close_all(): Until now, all root BDSs have been
force-closed. This is bad because it can lead to cached data not being
flushed to disk.

Instead, try to make all reference holders relinquish their reference
voluntarily:

1. All BlockBackend users are handled by making all BBs simply eject
   their BDS tree. Since a BDS can never be on top of a BB, this will
   not cause any of the issues as seen with the force-closing of BDSs.
   The references will be relinquished and any further access to the BB
   will fail gracefully.
2. All BDSs which are owned by the monitor itself (because they do not
   have a BB) are relinquished next.
3. Besides BBs and the monitor, block jobs and other BDSs are the only
   things left that can hold a reference to BDSs. After every remaining
   block job has been canceled, there should not be any BDSs left (and
   the loop added here will always terminate (as long as NDEBUG is not
   defined), because either all_bdrv_states will be empty or there will
   not be any block job left to cancel, failing the assertion).

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 45 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 37 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index b935dff..e3d5aea 100644
--- a/block.c
+++ b/block.c
@@ -1905,9 +1905,7 @@ static void bdrv_close(BlockDriverState *bs)
 {
     BdrvAioNotifier *ban, *ban_next;
 
-    if (bs->job) {
-        block_job_cancel_sync(bs->job);
-    }
+    assert(!bs->job);
 
     /* Disable I/O limits and drain all pending throttled requests */
     if (bs->throttle_state) {
@@ -1971,13 +1969,44 @@ static void bdrv_close(BlockDriverState *bs)
 void bdrv_close_all(void)
 {
     BlockDriverState *bs;
+    AioContext *aio_context;
+    int original_refcount = 0;
 
-    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
-        AioContext *aio_context = bdrv_get_aio_context(bs);
+    /* Drop references from requests still in flight, such as canceled block
+     * jobs whose AIO context has not been polled yet */
+    bdrv_drain_all();
 
-        aio_context_acquire(aio_context);
-        bdrv_close(bs);
-        aio_context_release(aio_context);
+    blockdev_close_all_bdrv_states();
+    blk_remove_all_bs();
+
+    /* Cancel all block jobs */
+    while (!QTAILQ_EMPTY(&all_bdrv_states)) {
+        QTAILQ_FOREACH(bs, &all_bdrv_states, bs_list) {
+            aio_context = bdrv_get_aio_context(bs);
+
+            aio_context_acquire(aio_context);
+            if (bs->job) {
+                /* So we can safely query the current refcount */
+                bdrv_ref(bs);
+                original_refcount = bs->refcnt;
+
+                block_job_cancel_sync(bs->job);
+                aio_context_release(aio_context);
+                break;
+            }
+            aio_context_release(aio_context);
+        }
+
+        /* All the remaining BlockDriverStates are referenced directly or
+         * indirectly from block jobs, so there needs to be at least one BDS
+         * directly used by a block job */
+        assert(bs);
+
+        /* Wait for the block job to release its reference */
+        while (bs->refcnt >= original_refcount) {
+            aio_poll(aio_context, true);
+        }
+        bdrv_unref(bs);
     }
 }
 
-- 
2.6.2

[Qemu-devel] [PATCH v7 23/24] iotests: Add test for multiple BB on BDS tree

[Max Reitz]

This adds a test for having multiple BlockBackends in one BDS tree. In
this case, there is one BB for the protocol BDS and one BB for the
format BDS in a simple two-BDS tree (with the protocol BDS and BB added
first).

When bdrv_close_all() is executed, no cached data from any BDS should be
lost; the protocol BDS may not be closed until the format BDS is closed.
Otherwise, metadata updates may be lost.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/117     | 86 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/117.out | 14 ++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 101 insertions(+)
 create mode 100755 tests/qemu-iotests/117
 create mode 100644 tests/qemu-iotests/117.out

diff --git a/tests/qemu-iotests/117 b/tests/qemu-iotests/117
new file mode 100755
index 0000000..7881fc7
--- /dev/null
+++ b/tests/qemu-iotests/117
@@ -0,0 +1,86 @@
+#!/bin/bash
+#
+# Test case for shared BDS between backend trees
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@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
+}
+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
+
+_make_test_img 64k
+
+_launch_qemu
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'qmp_capabilities' }" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'blockdev-add',
+       'arguments': { 'options': { 'id': 'protocol',
+                                   'driver': 'file',
+                                   'filename': '$TEST_IMG' } } }" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'blockdev-add',
+       'arguments': { 'options': { 'id': 'format',
+                                   'driver': '$IMGFMT',
+                                   'file': 'protocol' } } }" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line': 'qemu-io format \"write -P 42 0 64k\"' } }" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'quit' }" \
+    'return'
+
+wait=1 _cleanup_qemu
+
+_check_test_img
+
+$QEMU_IO -c 'read -P 42 0 64k' "$TEST_IMG" | _filter_qemu_io
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/117.out b/tests/qemu-iotests/117.out
new file mode 100644
index 0000000..f52dc1a
--- /dev/null
+++ b/tests/qemu-iotests/117.out
@@ -0,0 +1,14 @@
+QA output created by 117
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
+{"return": {}}
+{"return": {}}
+{"return": {}}
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
+No errors were found on the image.
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 993711b..d157c07 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -122,6 +122,7 @@
 114 rw auto quick
 115 rw auto
 116 rw auto quick
+117 rw auto
 118 rw auto
 119 rw auto quick
 120 rw auto quick
-- 
2.6.2

[Qemu-devel] [PATCH v7 24/24] iotests: Add test for block jobs and BDS ejection

[Max Reitz]

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/141     | 166 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/141.out |  47 +++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 214 insertions(+)
 create mode 100755 tests/qemu-iotests/141
 create mode 100644 tests/qemu-iotests/141.out

diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141
new file mode 100755
index 0000000..6a32d56
--- /dev/null
+++ b/tests/qemu-iotests/141
@@ -0,0 +1,166 @@
+#!/bin/bash
+#
+# Test case for ejecting BDSs with block jobs still running on them
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@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_DIR/{b,o}.$IMGFMT"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+# Needs backing file support
+_supported_fmt qcow qcow2 qed
+_supported_proto file
+_supported_os Linux
+
+
+test_blockjob()
+{
+    _send_qemu_cmd $QEMU_HANDLE \
+        "{'execute': 'blockdev-add',
+          'arguments': {
+              'options': {
+                  'id': 'drv0',
+                  'driver': '$IMGFMT',
+                  'file': {
+                      'driver': 'file',
+                      'filename': '$TEST_IMG'
+                  }}}}" \
+        'return'
+
+    _send_qemu_cmd $QEMU_HANDLE \
+        "$1" \
+        "$2" \
+        | _filter_img_create
+
+    # We want this to return an error because the block job is still running
+    _send_qemu_cmd $QEMU_HANDLE \
+        "{'execute': 'blockdev-remove-medium',
+          'arguments': {'device': 'drv0'}}" \
+        'error'
+
+    _send_qemu_cmd $QEMU_HANDLE \
+        "{'execute': 'block-job-cancel',
+          'arguments': {'device': 'drv0'}}" \
+        "$3"
+
+    _send_qemu_cmd $QEMU_HANDLE \
+        "{'execute': 'x-blockdev-del',
+          'arguments': {'id': 'drv0'}}" \
+        'return'
+}
+
+
+TEST_IMG="$TEST_DIR/b.$IMGFMT" _make_test_img 1M
+_make_test_img -b "$TEST_DIR/b.$IMGFMT" 1M
+
+_launch_qemu -nodefaults
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{'execute': 'qmp_capabilities'}" \
+    'return'
+
+echo
+echo '=== Testing drive-backup ==='
+echo
+
+# drive-backup will not send BLOCK_JOB_READY by itself, and cancelling the job
+# will consequently result in BLOCK_JOB_CANCELLED being emitted.
+
+test_blockjob \
+    "{'execute': 'drive-backup',
+      'arguments': {'device': 'drv0',
+                    'target': '$TEST_DIR/o.$IMGFMT',
+                    'format': '$IMGFMT',
+                    'sync': 'none'}}" \
+    'return' \
+    'BLOCK_JOB_CANCELLED'
+
+echo
+echo '=== Testing drive-mirror ==='
+echo
+
+# drive-mirror will send BLOCK_JOB_READY basically immediately, and cancelling
+# the job will consequently result in BLOCK_JOB_COMPLETED being emitted.
+
+test_blockjob \
+    "{'execute': 'drive-mirror',
+      'arguments': {'device': 'drv0',
+                    'target': '$TEST_DIR/o.$IMGFMT',
+                    'format': '$IMGFMT',
+                    'sync': 'none'}}" \
+    'BLOCK_JOB_READY' \
+    'BLOCK_JOB_COMPLETED'
+
+echo
+echo '=== Testing block-commit ==='
+echo
+
+# block-commit will send BLOCK_JOB_READY basically immediately, and cancelling
+# the job will consequently result in BLOCK_JOB_COMPLETED being emitted.
+
+test_blockjob \
+    "{'execute': 'block-commit',
+      'arguments': {'device': 'drv0'}}" \
+    'BLOCK_JOB_READY' \
+    'BLOCK_JOB_COMPLETED'
+
+echo
+echo '=== Testing block-stream ==='
+echo
+
+# Give block-stream something to work on, otherwise it would be done
+# immediately, send a BLOCK_JOB_COMPLETED and ejecting the BDS would work just
+# fine without the block job still running.
+
+$QEMU_IO -c 'write 0 1M' "$TEST_DIR/b.$IMGFMT" | _filter_qemu_io
+
+# With some data to stream (and @speed set to 1), block-stream will not complete
+# until we send the block-job-cancel command. Therefore, no event other than
+# BLOCK_JOB_CANCELLED will be emitted.
+
+test_blockjob \
+    "{'execute': 'block-stream',
+      'arguments': {'device': 'drv0',
+                    'speed': 1}}" \
+    'return' \
+    'BLOCK_JOB_CANCELLED'
+
+_cleanup_qemu
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out
new file mode 100644
index 0000000..6aa0cf0
--- /dev/null
+++ b/tests/qemu-iotests/141.out
@@ -0,0 +1,47 @@
+QA output created by 141
+Formatting 'TEST_DIR/b.IMGFMT', fmt=IMGFMT size=1048576
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/b.IMGFMT
+{"return": {}}
+
+=== Testing drive-backup ===
+
+{"return": {}}
+Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT
+{"return": {}}
+{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: backup"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 0, "speed": 0, "type": "backup"}}
+{"return": {}}
+
+=== Testing drive-mirror ===
+
+{"return": {}}
+Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "mirror"}}
+{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: mirror"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "mirror"}}
+{"return": {}}
+
+=== Testing block-commit ===
+
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}}
+{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: commit"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}}
+{"return": {}}
+
+=== Testing block-stream ===
+
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": {}}
+{"return": {}}
+{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: stream"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 524288, "speed": 1, "type": "stream"}}
+{"return": {}}
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index d157c07..58b79f2 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -141,3 +141,4 @@
 138 rw auto quick
 139 rw auto quick
 140 rw auto quick
+141 rw auto quick
-- 
2.6.2

Re: [Qemu-devel] [PATCH v7 20/24] blockdev: Keep track of monitor-owned BDS

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4104 bytes --]

On 09.11.2015 23:39, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c                             | 25 +++++++++++++++++++++++++
>  include/block/block_int.h              |  4 ++++
>  stubs/Makefile.objs                    |  1 +
>  stubs/blockdev-close-all-bdrv-states.c |  5 +++++
>  4 files changed, 35 insertions(+)
>  create mode 100644 stubs/blockdev-close-all-bdrv-states.c
> 
> diff --git a/blockdev.c b/blockdev.c
> index a70a684..8211eae 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -50,6 +50,9 @@
>  #include "trace.h"
>  #include "sysemu/arch_init.h"
>  
> +static QTAILQ_HEAD(, BlockDriverState) monitor_bdrv_states =
> +    QTAILQ_HEAD_INITIALIZER(monitor_bdrv_states);
> +
>  static const char *const if_name[IF_COUNT] = {
>      [IF_NONE] = "none",
>      [IF_IDE] = "ide",
> @@ -662,6 +665,19 @@ fail:
>      return NULL;
>  }
>  
> +void blockdev_close_all_bdrv_states(void)
> +{
> +    BlockDriverState *bs, *next_bs;
> +
> +    QTAILQ_FOREACH_SAFE(bs, &monitor_bdrv_states, monitor_list, next_bs) {
> +        AioContext *ctx = bdrv_get_aio_context(bs);
> +
> +        aio_context_acquire(ctx);
> +        bdrv_unref(bs);
> +        aio_context_release(ctx);
> +    }
> +}
> +
>  static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to,
>                              Error **errp)
>  {
> @@ -3464,6 +3480,8 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
>          if (!bs) {
>              goto fail;
>          }
> +
> +        QTAILQ_INSERT_TAIL(&monitor_bdrv_states, bs, monitor_list);

This is missing a QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list)
in the if (bs && bdrv_key_required(bs)) { if (!blk) { ... } } branch
below this place.

Max

>      }
>  
>      if (bs && bdrv_key_required(bs)) {
> @@ -3534,11 +3552,18 @@ void qmp_x_blockdev_del(bool has_id, const char *id,
>                         bdrv_get_device_or_node_name(bs));
>              goto out;
>          }
> +
> +        if (!blk && !bs->monitor_list.tqe_prev) {
> +            error_setg(errp, "Node %s is not owned by the monitor",
> +                       bs->node_name);
> +            goto out;
> +        }
>      }
>  
>      if (blk) {
>          blk_unref(blk);
>      } else {
> +        QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list);
>          bdrv_unref(bs);
>      }
>  
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 8c82747..7408eef 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -424,6 +424,8 @@ struct BlockDriverState {
>      QTAILQ_ENTRY(BlockDriverState) device_list;
>      /* element of the list of all BlockDriverStates (all_bdrv_states) */
>      QTAILQ_ENTRY(BlockDriverState) bs_list;
> +    /* element of the list of monitor-owned BDS */
> +    QTAILQ_ENTRY(BlockDriverState) monitor_list;
>      QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
>      int refcnt;
>  
> @@ -680,4 +682,6 @@ void blk_dev_resize_cb(BlockBackend *blk);
>  void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
>  bool bdrv_requests_pending(BlockDriverState *bs);
>  
> +void blockdev_close_all_bdrv_states(void);
> +
>  #endif /* BLOCK_INT_H */
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 330e1a4..c1f02be 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -1,5 +1,6 @@
>  stub-obj-y += arch-query-cpu-def.o
>  stub-obj-y += bdrv-commit-all.o
> +stub-obj-y += blockdev-close-all-bdrv-states.o
>  stub-obj-y += clock-warp.o
>  stub-obj-y += cpu-get-clock.o
>  stub-obj-y += cpu-get-icount.o
> diff --git a/stubs/blockdev-close-all-bdrv-states.c b/stubs/blockdev-close-all-bdrv-states.c
> new file mode 100644
> index 0000000..12d2442
> --- /dev/null
> +++ b/stubs/blockdev-close-all-bdrv-states.c
> @@ -0,0 +1,5 @@
> +#include "block/block_int.h"
> +
> +void blockdev_close_all_bdrv_states(void)
> +{
> +}
> 



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

Re: [Qemu-devel] [PATCH v7 03/24] block: Release dirty bitmaps in bdrv_close()

[John Snow]

On 11/09/2015 05:39 PM, Max Reitz wrote:
> bdrv_delete() is not very happy about deleting BlockDriverStates with
> dirty bitmaps still attached to them. In the past, we got around that
> very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and
> bdrv_close() simply ignoring that condition. We should fix that by
> releasing all dirty bitmaps in bdrv_close() and drop the assertion in
> bdrv_delete().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 37 +++++++++++++++++++++++++++++--------
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/block.c b/block.c
> index cffac75..94c0ecf 100644
> --- a/block.c
> +++ b/block.c
> @@ -87,6 +87,8 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
>                               const BdrvChildRole *child_role, Error **errp);
>  
>  static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs);
> +
>  /* If non-zero, use only whitelisted block drivers */
>  static int use_bdrv_whitelist;
>  
> @@ -1916,6 +1918,8 @@ void bdrv_close(BlockDriverState *bs)
>      bdrv_drain(bs); /* in case flush left pending I/O */
>      notifier_list_notify(&bs->close_notifiers, bs);
>  
> +    bdrv_release_all_dirty_bitmaps(bs);
> +
>      if (bs->blk) {
>          blk_dev_change_media_cb(bs->blk, false);
>      }
> @@ -2123,7 +2127,6 @@ static void bdrv_delete(BlockDriverState *bs)
>      assert(!bs->job);
>      assert(bdrv_op_blocker_is_empty(bs));
>      assert(!bs->refcnt);
> -    assert(QLIST_EMPTY(&bs->dirty_bitmaps));
>  
>      bdrv_close(bs);
>  
> @@ -3303,21 +3306,39 @@ static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>      }
>  }
>  
> -void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
> +                                                  BdrvDirtyBitmap *bitmap)
>  {
>      BdrvDirtyBitmap *bm, *next;
>      QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
> -        if (bm == bitmap) {
> +        if (!bitmap || bm == bitmap) {
>              assert(!bdrv_dirty_bitmap_frozen(bm));
> -            QLIST_REMOVE(bitmap, list);
> -            hbitmap_free(bitmap->bitmap);
> -            g_free(bitmap->name);
> -            g_free(bitmap);
> -            return;
> +            QLIST_REMOVE(bm, list);
> +            hbitmap_free(bm->bitmap);
> +            g_free(bm->name);
> +            g_free(bm);
> +
> +            if (bitmap) {
> +                return;
> +            }
>          }
>      }
>  }
>  
> +void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +{
> +    bdrv_do_release_matching_dirty_bitmap(bs, bitmap);
> +}
> +
> +/**
> + * Release all dirty bitmaps attached to a BDS (for use in bdrv_close()). There
> + * must not be any frozen bitmaps attached.
> + */
> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs)
> +{
> +    bdrv_do_release_matching_dirty_bitmap(bs, NULL);
> +}
> +
>  void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>  {
>      assert(!bdrv_dirty_bitmap_frozen(bitmap));
> 

Thanks!

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

Re: [Qemu-devel] [PATCH v7 10/24] iotests: Add test for eject under NBD server

[John Snow]

On 11/09/2015 05:39 PM, Max Reitz wrote:
> This patch adds a test for ejecting the BlockBackend an NBD server is
> connected to (the NBD server is supposed to stop).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/140     | 92 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/140.out | 16 ++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 109 insertions(+)
>  create mode 100755 tests/qemu-iotests/140
>  create mode 100644 tests/qemu-iotests/140.out
> 
> diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140
> new file mode 100755
> index 0000000..3434997
> --- /dev/null
> +++ b/tests/qemu-iotests/140
> @@ -0,0 +1,92 @@
> +#!/bin/bash
> +#
> +# Test case for ejecting a BB with an NBD server attached to it
> +#
> +# Copyright (C) 2015 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +# creator
> +owner=mreitz@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_DIR/nbd"
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +. ./common.qemu
> +
> +_supported_fmt generic
> +_supported_proto file
> +_supported_os Linux
> +
> +_make_test_img 64k
> +
> +$QEMU_IO -c 'write -P 42 0 64k' "$TEST_IMG" | _filter_qemu_io
> +
> +keep_stderr=y \
> +_launch_qemu -drive if=ide,media=cdrom,id=drv,file="$TEST_IMG",format=$IMGFMT \
> +    2> >(_filter_nbd)
> +
> +_send_qemu_cmd $QEMU_HANDLE \
> +    "{ 'execute': 'qmp_capabilities' }" \
> +    'return'
> +
> +_send_qemu_cmd $QEMU_HANDLE \
> +    "{ 'execute': 'nbd-server-start',
> +       'arguments': { 'addr': { 'type': 'unix',
> +                                'data': { 'path': '$TEST_DIR/nbd' }}}}" \
> +    'return'
> +
> +_send_qemu_cmd $QEMU_HANDLE \
> +    "{ 'execute': 'nbd-server-add',
> +       'arguments': { 'device': 'drv' }}" \
> +    'return'
> +
> +$QEMU_IO_PROG -f raw -c 'read -P 42 0 64k' \
> +    "nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \
> +    | _filter_qemu_io | _filter_nbd
> +
> +_send_qemu_cmd $QEMU_HANDLE \
> +    "{ 'execute': 'eject',
> +       'arguments': { 'device': 'drv' }}" \
> +    'return'
> +
> +$QEMU_IO_PROG -f raw -c close \
> +    "nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \
> +    | _filter_qemu_io | _filter_nbd
> +
> +_send_qemu_cmd $QEMU_HANDLE \
> +    "{ 'execute': 'quit' }" \
> +    'return'
> +
> +wait=1 _cleanup_qemu
> +
> +# success, all done
> +echo '*** done'
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/140.out b/tests/qemu-iotests/140.out
> new file mode 100644
> index 0000000..0087909
> --- /dev/null
> +++ b/tests/qemu-iotests/140.out
> @@ -0,0 +1,16 @@
> +QA output created by 140
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
> +wrote 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +{"return": {}}
> +{"return": {}}
> +{"return": {}}
> +read 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "drv", "tray-open": true}}
> +{"return": {}}
> +qemu-io: can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: Failed to read export length
> +no file open, try 'help open'
> +{"return": {}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
> +*** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index c69265d..993711b 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -139,3 +139,4 @@
>  137 rw auto
>  138 rw auto quick
>  139 rw auto quick
> +140 rw auto quick
> 

4-10:

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

Re: [Qemu-devel] [PATCH v7 01/24] blockdev: Add missing bdrv_unref() in drive-backup

[Fam Zheng]

On Mon, 11/09 23:39, Max Reitz wrote:
> All error paths after a successful bdrv_open() of target_bs should
> contain a bdrv_unref(target_bs). This one did not yet, so add it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 8607df9..3185808 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2914,6 +2914,7 @@ void qmp_drive_backup(const char *device, const char *target,
>          bmap = bdrv_find_dirty_bitmap(bs, bitmap);
>          if (!bmap) {
>              error_setg(errp, "Bitmap '%s' could not be found", bitmap);
> +            bdrv_unref(target_bs);
>              goto out;
>          }
>      }
> -- 
> 2.6.2
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v7 02/24] blockjob: Call bdrv_unref() on creation error

[Fam Zheng]

On Mon, 11/09 23:39, Max Reitz wrote:
> If block_job_create() fails, it should release its reference to the
> job's BDS. Normally, this is done in the callback provided by the
> caller, but that callback will not be invoked if the block job failed to
> be created.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockjob.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/blockjob.c b/blockjob.c
> index c02fe59..0886a4a 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -70,6 +70,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
>          if (local_err) {
>              block_job_release(bs);
>              error_propagate(errp, local_err);
> +            bdrv_unref(bs);
>              return NULL;
>          }
>      }
> -- 
> 2.6.2
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v7 03/24] block: Release dirty bitmaps in bdrv_close()

[Fam Zheng]

On Mon, 11/09 23:39, Max Reitz wrote:
> bdrv_delete() is not very happy about deleting BlockDriverStates with
> dirty bitmaps still attached to them. In the past, we got around that
> very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and
> bdrv_close() simply ignoring that condition. We should fix that by
> releasing all dirty bitmaps in bdrv_close() and drop the assertion in
> bdrv_delete().

What bitmaps are attached when bdrv_close() is called?  The ones created from
the monitor should probably be removed by the monitor, and the internal ones
like in migration and block jobs should probably be removed by stopping the
respective job.

Fam

Re: [Qemu-devel] [PATCH v7 04/24] iotests: Rename filter_nbd to _filter_nbd in 083

[Fam Zheng]

On Mon, 11/09 23:39, Max Reitz wrote:
> In the patch after the next, this function is moved to common.filter.
> Therefore, its name should be preceded by an underscore to signify its
> global availability.
> 
> To keep the code motion patch clean, we cannot rename it in the same
> patch, so we need to choose some order of renaming vs. motion. It is
> better to keep a supposedly global function used by only a single test
> in that test than to keep a supposedly local function in a common* file
> and use it from a test, so we should rename the function before moving
> it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/083 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083
> index 1b2d3f1..664f0cf 100755
> --- a/tests/qemu-iotests/083
> +++ b/tests/qemu-iotests/083
> @@ -49,7 +49,7 @@ wait_for_tcp_port() {
>  	done
>  }
>  
> -filter_nbd() {
> +_filter_nbd() {
>  	# nbd.c error messages contain function names and line numbers that are prone
>  	# to change.  Message ordering depends on timing between send and receive
>  	# callbacks sometimes, making them unreliable.
> @@ -84,7 +84,7 @@ EOF
>  
>  	$PYTHON nbd-fault-injector.py $extra_args "127.0.0.1:$port" "$TEST_DIR/nbd-fault-injector.conf" 2>&1 >/dev/null &
>  	wait_for_tcp_port "127\\.0\\.0\\.1:$port"
> -	$QEMU_IO -c "read 0 512" "$nbd_url" 2>&1 | _filter_qemu_io | filter_nbd
> +	$QEMU_IO -c "read 0 512" "$nbd_url" 2>&1 | _filter_qemu_io | _filter_nbd
>  
>  	echo
>  }
> -- 
> 2.6.2
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v7 05/24] iotests: Change coding style of _filter_nbd in 083

[Fam Zheng]

On Mon, 11/09 23:39, Max Reitz wrote:
> In order to be able to move _filter_nbd to common.filter in the next
> patch, its coding style needs to be adapted to that of common.filter.
> That means, we have to convert tabs to four spaces, adjust the alignment
> of the last line (done with spaces already, assuming one tab equals
> eight spaces), fix the line length of the comment, and add a line break
> before the opening brace.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/083 | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083
> index 664f0cf..c00a66b 100755
> --- a/tests/qemu-iotests/083
> +++ b/tests/qemu-iotests/083
> @@ -49,15 +49,16 @@ wait_for_tcp_port() {
>  	done
>  }
>  
> -_filter_nbd() {
> -	# nbd.c error messages contain function names and line numbers that are prone
> -	# to change.  Message ordering depends on timing between send and receive
> -	# callbacks sometimes, making them unreliable.
> -	#
> -	# Filter out the TCP port number since this changes between runs.
> -	sed -e 's#^.*nbd\.c:.*##g' \
> -	    -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \
> -            -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#'
> +_filter_nbd()
> +{
> +    # nbd.c error messages contain function names and line numbers that are
> +    # prone to change.  Message ordering depends on timing between send and
> +    # receive callbacks sometimes, making them unreliable.
> +    #
> +    # Filter out the TCP port number since this changes between runs.
> +    sed -e 's#^.*nbd\.c:.*##g' \
> +        -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \
> +        -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#'
>  }
>  
>  check_disconnect() {
> -- 
> 2.6.2
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v7 06/24] iotests: Move _filter_nbd into common.filter

[Fam Zheng]

On Mon, 11/09 23:39, Max Reitz wrote:
> _filter_nbd can be useful for other NBD tests, too, therefore it should
> reside in common.filter.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/083           | 12 ------------
>  tests/qemu-iotests/common.filter | 12 ++++++++++++
>  2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083
> index c00a66b..aa99278 100755
> --- a/tests/qemu-iotests/083
> +++ b/tests/qemu-iotests/083
> @@ -49,18 +49,6 @@ wait_for_tcp_port() {
>  	done
>  }
>  
> -_filter_nbd()
> -{
> -    # nbd.c error messages contain function names and line numbers that are
> -    # prone to change.  Message ordering depends on timing between send and
> -    # receive callbacks sometimes, making them unreliable.
> -    #
> -    # Filter out the TCP port number since this changes between runs.
> -    sed -e 's#^.*nbd\.c:.*##g' \
> -        -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \
> -        -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#'
> -}
> -
>  check_disconnect() {
>  	event=$1
>  	when=$2
> diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
> index cfdb633..aa2fb8d 100644
> --- a/tests/qemu-iotests/common.filter
> +++ b/tests/qemu-iotests/common.filter
> @@ -230,5 +230,17 @@ _filter_qemu_img_map()
>          -e 's/Mapped to *//' | _filter_testdir | _filter_imgfmt
>  }
>  
> +_filter_nbd()
> +{
> +    # nbd.c error messages contain function names and line numbers that are
> +    # prone to change.  Message ordering depends on timing between send and
> +    # receive callbacks sometimes, making them unreliable.
> +    #
> +    # Filter out the TCP port number since this changes between runs.
> +    sed -e 's#^.*nbd\.c:.*##g' \
> +        -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \
> +        -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#'
> +}
> +
>  # make sure this script returns success
>  true
> -- 
> 2.6.2
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v7 07/24] iotests: Make _filter_nbd drop log lines

[Fam Zheng]

On Mon, 11/09 23:39, Max Reitz wrote:
> The NBD log lines ("/your/source/dir/nbd.c:function():line: error")
> should not be converted to empty lines but removed altogether.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/083.out       | 10 ----------
>  tests/qemu-iotests/common.filter |  2 +-
>  2 files changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/tests/qemu-iotests/083.out b/tests/qemu-iotests/083.out
> index 8c1441b..5c9141b 100644
> --- a/tests/qemu-iotests/083.out
> +++ b/tests/qemu-iotests/083.out
> @@ -51,7 +51,6 @@ no file open, try 'help open'
>  
>  === Check disconnect after neg2 ===
>  
> -
>  read failed: Input/output error
>  
>  === Check disconnect 8 neg2 ===
> @@ -66,42 +65,34 @@ no file open, try 'help open'
>  
>  === Check disconnect before request ===
>  
> -
>  read failed: Input/output error
>  
>  === Check disconnect after request ===
>  
> -
>  read failed: Input/output error
>  
>  === Check disconnect before reply ===
>  
> -
>  read failed: Input/output error
>  
>  === Check disconnect after reply ===
>  
> -
>  read failed: Input/output error
>  
>  === Check disconnect 4 reply ===
>  
> -
>  read failed: Input/output error
>  
>  === Check disconnect 8 reply ===
>  
> -
>  read failed: Input/output error
>  
>  === Check disconnect before data ===
>  
> -
>  read failed: Input/output error
>  
>  === Check disconnect after data ===
>  
> -
>  read 512/512 bytes at offset 0
>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  
> @@ -132,7 +123,6 @@ no file open, try 'help open'
>  
>  === Check disconnect after neg-classic ===
>  
> -
>  read failed: Input/output error
>  
>  *** done
> diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
> index aa2fb8d..0fc443a 100644
> --- a/tests/qemu-iotests/common.filter
> +++ b/tests/qemu-iotests/common.filter
> @@ -237,7 +237,7 @@ _filter_nbd()
>      # receive callbacks sometimes, making them unreliable.
>      #
>      # Filter out the TCP port number since this changes between runs.
> -    sed -e 's#^.*nbd\.c:.*##g' \
> +    sed -e '/nbd\.c:/d' \
>          -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \
>          -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#'
>  }
> -- 
> 2.6.2
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v7 08/24] iotests: Make _filter_nbd support more URL types

[Fam Zheng]

On Mon, 11/09 23:39, Max Reitz wrote:
> This function should support URLs of the "nbd://" format (without
> swallowing the export name), and for "nbd:///" URLs it should replace
> "?socket=$TEST_DIR" by "?socket=TEST_DIR" because putting the Unix
> socket files into the test directory makes sense.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/common.filter | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
> index 0fc443a..bd53cab 100644
> --- a/tests/qemu-iotests/common.filter
> +++ b/tests/qemu-iotests/common.filter
> @@ -238,7 +238,8 @@ _filter_nbd()
>      #
>      # Filter out the TCP port number since this changes between runs.
>      sed -e '/nbd\.c:/d' \
> -        -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \
> +        -e 's#nbd:\(//\)\?127\.0\.0\.1:[0-9]*#nbd:\1127.0.0.1:PORT#g' \
> +        -e "s#?socket=$TEST_DIR#?socket=TEST_DIR#g" \
>          -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#'
>  }
>  
> -- 
> 2.6.2
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v7 09/24] iotests: Make redirecting qemu's stderr optional

[Fam Zheng]

On Mon, 11/09 23:39, Max Reitz wrote:
> Redirecting qemu's stderr to stdout makes working with the stderr output
> difficult due to the other file descriptor magic performed in
> _launch_qemu ("ambiguous redirect").
> 
> Add an option which specifies whether stderr should be redirected to
> stdout or not (allowing for other modes to be added in the future).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/common.qemu | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
> index 8bf3969..2548a87 100644
> --- a/tests/qemu-iotests/common.qemu
> +++ b/tests/qemu-iotests/common.qemu
> @@ -129,6 +129,8 @@ function _send_qemu_cmd()
>  # $qemu_comm_method: set this variable to 'monitor' (case insensitive)
>  #                    to use the QEMU HMP monitor for communication.
>  #                    Otherwise, the default of QMP is used.
> +# $keep_stderr: Set this variable to 'y' to keep QEMU's stderr output on stderr.
> +#               If this variable is empty, stderr will be redirected to stdout.
>  # Returns:
>  # $QEMU_HANDLE: set to a handle value to communicate with this QEMU instance.
>  #
> @@ -151,11 +153,20 @@ function _launch_qemu()
>      mkfifo "${fifo_out}"
>      mkfifo "${fifo_in}"
>  
> -    QEMU_NEED_PID='y'\
> -    ${QEMU} -nographic -serial none ${comm} -machine accel=qtest "${@}" \
> +    if [ -z "$keep_stderr" ]; then
> +        QEMU_NEED_PID='y'\
> +        ${QEMU} -nographic -serial none ${comm} -machine accel=qtest "${@}" \
>                                                                  >"${fifo_out}" \
>                                                                  2>&1 \
>                                                                  <"${fifo_in}" &
> +    elif [ "$keep_stderr" = "y" ]; then
> +        QEMU_NEED_PID='y'\
> +        ${QEMU} -nographic -serial none ${comm} -machine accel=qtest "${@}" \
> +                                                                >"${fifo_out}" \
> +                                                                <"${fifo_in}" &
> +    else
> +        exit 1
> +    fi
>  
>      if [[ "${BASH_VERSINFO[0]}" -ge "5" ||
>          ("${BASH_VERSINFO[0]}" -ge "4"  &&  "${BASH_VERSINFO[1]}" -ge "1") ]]
> -- 
> 2.6.2
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v7 10/24] iotests: Add test for eject under NBD server

[Fam Zheng]

On Mon, 11/09 23:39, Max Reitz wrote:
> This patch adds a test for ejecting the BlockBackend an NBD server is
> connected to (the NBD server is supposed to stop).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/140     | 92 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/140.out | 16 ++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 109 insertions(+)
>  create mode 100755 tests/qemu-iotests/140
>  create mode 100644 tests/qemu-iotests/140.out
> 
> diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140
> new file mode 100755
> index 0000000..3434997
> --- /dev/null
> +++ b/tests/qemu-iotests/140
> @@ -0,0 +1,92 @@
> +#!/bin/bash
> +#
> +# Test case for ejecting a BB with an NBD server attached to it
> +#
> +# Copyright (C) 2015 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +# creator
> +owner=mreitz@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_DIR/nbd"
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +. ./common.qemu
> +
> +_supported_fmt generic
> +_supported_proto file
> +_supported_os Linux
> +
> +_make_test_img 64k
> +
> +$QEMU_IO -c 'write -P 42 0 64k' "$TEST_IMG" | _filter_qemu_io
> +
> +keep_stderr=y \
> +_launch_qemu -drive if=ide,media=cdrom,id=drv,file="$TEST_IMG",format=$IMGFMT \
> +    2> >(_filter_nbd)
> +
> +_send_qemu_cmd $QEMU_HANDLE \
> +    "{ 'execute': 'qmp_capabilities' }" \
> +    'return'
> +
> +_send_qemu_cmd $QEMU_HANDLE \
> +    "{ 'execute': 'nbd-server-start',
> +       'arguments': { 'addr': { 'type': 'unix',
> +                                'data': { 'path': '$TEST_DIR/nbd' }}}}" \
> +    'return'
> +
> +_send_qemu_cmd $QEMU_HANDLE \
> +    "{ 'execute': 'nbd-server-add',
> +       'arguments': { 'device': 'drv' }}" \
> +    'return'
> +
> +$QEMU_IO_PROG -f raw -c 'read -P 42 0 64k' \
> +    "nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \
> +    | _filter_qemu_io | _filter_nbd
> +
> +_send_qemu_cmd $QEMU_HANDLE \
> +    "{ 'execute': 'eject',
> +       'arguments': { 'device': 'drv' }}" \
> +    'return'
> +
> +$QEMU_IO_PROG -f raw -c close \
> +    "nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \
> +    | _filter_qemu_io | _filter_nbd
> +
> +_send_qemu_cmd $QEMU_HANDLE \
> +    "{ 'execute': 'quit' }" \
> +    'return'
> +
> +wait=1 _cleanup_qemu
> +
> +# success, all done
> +echo '*** done'
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/140.out b/tests/qemu-iotests/140.out
> new file mode 100644
> index 0000000..0087909
> --- /dev/null
> +++ b/tests/qemu-iotests/140.out
> @@ -0,0 +1,16 @@
> +QA output created by 140
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
> +wrote 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +{"return": {}}
> +{"return": {}}
> +{"return": {}}
> +read 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "drv", "tray-open": true}}
> +{"return": {}}
> +qemu-io: can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: Failed to read export length
> +no file open, try 'help open'
> +{"return": {}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
> +*** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index c69265d..993711b 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -139,3 +139,4 @@
>  137 rw auto
>  138 rw auto quick
>  139 rw auto quick
> +140 rw auto quick
> -- 
> 2.6.2
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v7 18/24] block: Make bdrv_close() static

[Fam Zheng]

On Mon, 11/09 23:39, Max Reitz wrote:
> There are no users of bdrv_close() left, except for one of bdrv_open()'s
> failure paths, bdrv_close_all() and bdrv_delete(), and that is good.
> Make bdrv_close() static so nobody makes the mistake of directly using
> bdrv_close() again.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c               | 4 +++-
>  include/block/block.h | 1 -
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 4ee3b01..b5874aa 100644
> --- a/block.c
> +++ b/block.c
> @@ -92,6 +92,8 @@ static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs);
>  /* If non-zero, use only whitelisted block drivers */
>  static int use_bdrv_whitelist;
>  
> +static void bdrv_close(BlockDriverState *bs);
> +
>  #ifdef _WIN32
>  static int is_windows_drive_prefix(const char *filename)
>  {
> @@ -1894,7 +1896,7 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
>  }
>  
>  
> -void bdrv_close(BlockDriverState *bs)
> +static void bdrv_close(BlockDriverState *bs)
>  {
>      BdrvAioNotifier *ban, *ban_next;
>  
> diff --git a/include/block/block.h b/include/block/block.h
> index 8d9a049..03551d3 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -222,7 +222,6 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
>                          BlockReopenQueue *queue, Error **errp);
>  void bdrv_reopen_commit(BDRVReopenState *reopen_state);
>  void bdrv_reopen_abort(BDRVReopenState *reopen_state);
> -void bdrv_close(BlockDriverState *bs);
>  int bdrv_read(BlockDriverState *bs, int64_t sector_num,
>                uint8_t *buf, int nb_sectors);
>  int bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num,
> -- 
> 2.6.2
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v7 19/24] block: Add list of all BlockDriverStates

[Fam Zheng]

On Mon, 11/09 23:39, Max Reitz wrote:
> We need this list so that bdrv_close_all() can keep track of which BDSs
> are still open after having removed the BDSs from all of the BBs and
> having released all monitor BDS references.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Is graph_bdrv_states becoming a subset of this list, just with non-empty
node_name?

> ---
>  block.c                   | 7 +++++++
>  include/block/block_int.h | 2 ++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/block.c b/block.c
> index b5874aa..b935dff 100644
> --- a/block.c
> +++ b/block.c
> @@ -78,6 +78,9 @@ struct BdrvStates bdrv_states = QTAILQ_HEAD_INITIALIZER(bdrv_states);
>  static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
>      QTAILQ_HEAD_INITIALIZER(graph_bdrv_states);
>  
> +static QTAILQ_HEAD(, BlockDriverState) all_bdrv_states =
> +    QTAILQ_HEAD_INITIALIZER(all_bdrv_states);
> +
>  static QLIST_HEAD(, BlockDriver) bdrv_drivers =
>      QLIST_HEAD_INITIALIZER(bdrv_drivers);
>  
> @@ -266,6 +269,8 @@ BlockDriverState *bdrv_new(void)
>      bs->refcnt = 1;
>      bs->aio_context = qemu_get_aio_context();
>  
> +    QTAILQ_INSERT_TAIL(&all_bdrv_states, bs, bs_list);
> +
>      return bs;
>  }
>  
> @@ -2128,6 +2133,8 @@ static void bdrv_delete(BlockDriverState *bs)
>      /* remove from list, if necessary */
>      bdrv_make_anon(bs);
>  
> +    QTAILQ_REMOVE(&all_bdrv_states, bs, bs_list);
> +
>      g_free(bs);
>  }
>  
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 617994d..8c82747 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -422,6 +422,8 @@ struct BlockDriverState {
>      QTAILQ_ENTRY(BlockDriverState) node_list;
>      /* element of the list of "drives" the guest sees */
>      QTAILQ_ENTRY(BlockDriverState) device_list;
> +    /* element of the list of all BlockDriverStates (all_bdrv_states) */
> +    QTAILQ_ENTRY(BlockDriverState) bs_list;
>      QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
>      int refcnt;
>  
> -- 
> 2.6.2
> 
> 

Re: [Qemu-devel] [PATCH v7 22/24] block: Rewrite bdrv_close_all()

[Fam Zheng]

On Mon, 11/09 23:39, Max Reitz wrote:
> This patch rewrites bdrv_close_all(): Until now, all root BDSs have been
> force-closed. This is bad because it can lead to cached data not being
> flushed to disk.
> 
> Instead, try to make all reference holders relinquish their reference
> voluntarily:
> 
> 1. All BlockBackend users are handled by making all BBs simply eject
>    their BDS tree. Since a BDS can never be on top of a BB, this will
>    not cause any of the issues as seen with the force-closing of BDSs.
>    The references will be relinquished and any further access to the BB
>    will fail gracefully.
> 2. All BDSs which are owned by the monitor itself (because they do not
>    have a BB) are relinquished next.
> 3. Besides BBs and the monitor, block jobs and other BDSs are the only
>    things left that can hold a reference to BDSs. After every remaining
>    block job has been canceled, there should not be any BDSs left (and
>    the loop added here will always terminate (as long as NDEBUG is not
>    defined), because either all_bdrv_states will be empty or there will
>    not be any block job left to cancel, failing the assertion).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 45 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/block.c b/block.c
> index b935dff..e3d5aea 100644
> --- a/block.c
> +++ b/block.c
> @@ -1905,9 +1905,7 @@ static void bdrv_close(BlockDriverState *bs)
>  {
>      BdrvAioNotifier *ban, *ban_next;
>  
> -    if (bs->job) {
> -        block_job_cancel_sync(bs->job);
> -    }
> +    assert(!bs->job);
>  
>      /* Disable I/O limits and drain all pending throttled requests */
>      if (bs->throttle_state) {
> @@ -1971,13 +1969,44 @@ static void bdrv_close(BlockDriverState *bs)
>  void bdrv_close_all(void)
>  {
>      BlockDriverState *bs;
> +    AioContext *aio_context;
> +    int original_refcount = 0;
>  
> -    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> -        AioContext *aio_context = bdrv_get_aio_context(bs);
> +    /* Drop references from requests still in flight, such as canceled block
> +     * jobs whose AIO context has not been polled yet */
> +    bdrv_drain_all();
>  
> -        aio_context_acquire(aio_context);
> -        bdrv_close(bs);
> -        aio_context_release(aio_context);
> +    blockdev_close_all_bdrv_states();
> +    blk_remove_all_bs();
> +
> +    /* Cancel all block jobs */
> +    while (!QTAILQ_EMPTY(&all_bdrv_states)) {
> +        QTAILQ_FOREACH(bs, &all_bdrv_states, bs_list) {
> +            aio_context = bdrv_get_aio_context(bs);
> +
> +            aio_context_acquire(aio_context);
> +            if (bs->job) {
> +                /* So we can safely query the current refcount */
> +                bdrv_ref(bs);
> +                original_refcount = bs->refcnt;
> +
> +                block_job_cancel_sync(bs->job);
> +                aio_context_release(aio_context);
> +                break;
> +            }
> +            aio_context_release(aio_context);
> +        }
> +
> +        /* All the remaining BlockDriverStates are referenced directly or
> +         * indirectly from block jobs, so there needs to be at least one BDS
> +         * directly used by a block job */
> +        assert(bs);
> +
> +        /* Wait for the block job to release its reference */
> +        while (bs->refcnt >= original_refcount) {
> +            aio_poll(aio_context, true);
> +        }
> +        bdrv_unref(bs);

If at this point bs->refcnt is greater than 1, why don't we care where are the
remaining references from?

Fam

>      }
>  }
>  
> -- 
> 2.6.2
> 
> 

Re: [Qemu-devel] [PATCH v7 03/24] block: Release dirty bitmaps in bdrv_close()

[John Snow]

On 11/12/2015 01:23 AM, Fam Zheng wrote:
> On Mon, 11/09 23:39, Max Reitz wrote:
>> bdrv_delete() is not very happy about deleting BlockDriverStates with
>> dirty bitmaps still attached to them. In the past, we got around that
>> very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and
>> bdrv_close() simply ignoring that condition. We should fix that by
>> releasing all dirty bitmaps in bdrv_close() and drop the assertion in
>> bdrv_delete().
> 
> What bitmaps are attached when bdrv_close() is called?  The ones created from
> the monitor should probably be removed by the monitor, and the internal ones
> like in migration and block jobs should probably be removed by stopping the
> respective job.
> 
> Fam
> 

Well in this case at least it appears we are still asserting that the
BDS has no job attached, so it shouldn't have any internal bitmaps
weighing it down, which just leaves the ones created by the QMP interface.

How important is it that we ask the user to remove all of those bitmaps
themselves?

It might become more important in the future when persistence is an
option and we go to close a transient bitmap -- but persistent bitmaps I
am sure it will be safe to just close out and flush to disk.

--js

Re: [Qemu-devel] [PATCH v7 03/24] block: Release dirty bitmaps in bdrv_close()

[Fam Zheng]

On Fri, 11/13 17:49, John Snow wrote:
> 
> 
> On 11/12/2015 01:23 AM, Fam Zheng wrote:
> > On Mon, 11/09 23:39, Max Reitz wrote:
> >> bdrv_delete() is not very happy about deleting BlockDriverStates with
> >> dirty bitmaps still attached to them. In the past, we got around that
> >> very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and
> >> bdrv_close() simply ignoring that condition. We should fix that by
> >> releasing all dirty bitmaps in bdrv_close() and drop the assertion in
> >> bdrv_delete().
> > 
> > What bitmaps are attached when bdrv_close() is called?  The ones created from
> > the monitor should probably be removed by the monitor, and the internal ones
> > like in migration and block jobs should probably be removed by stopping the
> > respective job.
> > 
> > Fam
> > 
> 
> Well in this case at least it appears we are still asserting that the
> BDS has no job attached, so it shouldn't have any internal bitmaps
> weighing it down, which just leaves the ones created by the QMP interface.
> 
> How important is it that we ask the user to remove all of those bitmaps
> themselves?
> 
> It might become more important in the future when persistence is an
> option and we go to close a transient bitmap -- but persistent bitmaps I
> am sure it will be safe to just close out and flush to disk.
> 

Yes, I was not saying we should expecting user to do that manually, but it
would be good to be clear about different types of instances in the code.

For now, it's unlikely a problem.

Fam

Re: [Qemu-devel] [PATCH v7 19/24] block: Add list of all BlockDriverStates

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

On 12.11.2015 08:12, Fam Zheng wrote:
> On Mon, 11/09 23:39, Max Reitz wrote:
>> We need this list so that bdrv_close_all() can keep track of which BDSs
>> are still open after having removed the BDSs from all of the BBs and
>> having released all monitor BDS references.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>
> 
> Is graph_bdrv_states becoming a subset of this list, just with non-empty
> node_name?

Yes, that is correct. There shouldn't be any BDS that isn't in
all_bdrv_states.

Max


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

Re: [Qemu-devel] [PATCH v7 22/24] block: Rewrite bdrv_close_all()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 5619 bytes --]

On 12.11.2015 08:34, Fam Zheng wrote:
> On Mon, 11/09 23:39, Max Reitz wrote:
>> This patch rewrites bdrv_close_all(): Until now, all root BDSs have been
>> force-closed. This is bad because it can lead to cached data not being
>> flushed to disk.
>>
>> Instead, try to make all reference holders relinquish their reference
>> voluntarily:
>>
>> 1. All BlockBackend users are handled by making all BBs simply eject
>>    their BDS tree. Since a BDS can never be on top of a BB, this will
>>    not cause any of the issues as seen with the force-closing of BDSs.
>>    The references will be relinquished and any further access to the BB
>>    will fail gracefully.
>> 2. All BDSs which are owned by the monitor itself (because they do not
>>    have a BB) are relinquished next.
>> 3. Besides BBs and the monitor, block jobs and other BDSs are the only
>>    things left that can hold a reference to BDSs. After every remaining
>>    block job has been canceled, there should not be any BDSs left (and
>>    the loop added here will always terminate (as long as NDEBUG is not
>>    defined), because either all_bdrv_states will be empty or there will
>>    not be any block job left to cancel, failing the assertion).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  block.c | 45 +++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 37 insertions(+), 8 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index b935dff..e3d5aea 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1905,9 +1905,7 @@ static void bdrv_close(BlockDriverState *bs)
>>  {
>>      BdrvAioNotifier *ban, *ban_next;
>>  
>> -    if (bs->job) {
>> -        block_job_cancel_sync(bs->job);
>> -    }
>> +    assert(!bs->job);
>>  
>>      /* Disable I/O limits and drain all pending throttled requests */
>>      if (bs->throttle_state) {
>> @@ -1971,13 +1969,44 @@ static void bdrv_close(BlockDriverState *bs)
>>  void bdrv_close_all(void)
>>  {
>>      BlockDriverState *bs;
>> +    AioContext *aio_context;
>> +    int original_refcount = 0;
>>  
>> -    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
>> -        AioContext *aio_context = bdrv_get_aio_context(bs);
>> +    /* Drop references from requests still in flight, such as canceled block
>> +     * jobs whose AIO context has not been polled yet */
>> +    bdrv_drain_all();
>>  
>> -        aio_context_acquire(aio_context);
>> -        bdrv_close(bs);
>> -        aio_context_release(aio_context);
>> +    blockdev_close_all_bdrv_states();
>> +    blk_remove_all_bs();
>> +
>> +    /* Cancel all block jobs */
>> +    while (!QTAILQ_EMPTY(&all_bdrv_states)) {
>> +        QTAILQ_FOREACH(bs, &all_bdrv_states, bs_list) {
>> +            aio_context = bdrv_get_aio_context(bs);
>> +
>> +            aio_context_acquire(aio_context);
>> +            if (bs->job) {
>> +                /* So we can safely query the current refcount */
>> +                bdrv_ref(bs);
>> +                original_refcount = bs->refcnt;
>> +
>> +                block_job_cancel_sync(bs->job);
>> +                aio_context_release(aio_context);
>> +                break;
>> +            }
>> +            aio_context_release(aio_context);
>> +        }
>> +
>> +        /* All the remaining BlockDriverStates are referenced directly or
>> +         * indirectly from block jobs, so there needs to be at least one BDS
>> +         * directly used by a block job */
>> +        assert(bs);
>> +
>> +        /* Wait for the block job to release its reference */
>> +        while (bs->refcnt >= original_refcount) {
>> +            aio_poll(aio_context, true);
>> +        }
>> +        bdrv_unref(bs);
> 
> If at this point bs->refcnt is greater than 1, why don't we care where are the
> remaining references from?

We do care. A BDS will not be removed from all_bdrv_states until it is
deleted (i.e. its refcount becomes 0). Therefore, this loop will
continue until all BDSs have been deleted.

So where might additional references come from? Since this loop only
cares about direct or indirect references from block jobs, that's
exactly it:

(1) You might have multiple block jobs running on a BDS in the future.
    Then, you'll cancel them one by one, and after having canceled the
    first one, the refcount will still be greater than one before the
    bdrv_unref().

(2) Imagine a BDS A with a parent BDS B. There are block jobs running on
    both of them. Now, B is referenced by both its block job and by A
    (indirectly by the block job referencing A). If we cancel the job on
    B before the one on A, then the refcount on B will still be greater
    than 1 before bdrv_unref() because it is still referenced by its
    parent (until the block job on A is canceled, too).

The first cannot happen right now, but the second one may, I'm not sure
(depending on whether op blockers allow it).


And we do make sure that there are no additional references besides:
- directly from the monitor (monitor-owned BDSs)
- from a BB
- from block jobs
- (+ everything transitively through the respective BDS tree)

If there were additional references, the inner loop would at some point
no longer find a BDS with a block job while all_bdrv_states is still not
empty. That's what the "assert(bs)" after the inner loop is for.

If you can imagine another way where a reference to a BDS may come from,
that would be a bug in this patch and we have to make sure to respect
that case, too.

Max


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

Re: [Qemu-devel] [PATCH v7 03/24] block: Release dirty bitmaps in bdrv_close()

[John Snow]

On 11/15/2015 08:27 PM, Fam Zheng wrote:
> On Fri, 11/13 17:49, John Snow wrote:
>>
>>
>> On 11/12/2015 01:23 AM, Fam Zheng wrote:
>>> On Mon, 11/09 23:39, Max Reitz wrote:
>>>> bdrv_delete() is not very happy about deleting BlockDriverStates with
>>>> dirty bitmaps still attached to them. In the past, we got around that
>>>> very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and
>>>> bdrv_close() simply ignoring that condition. We should fix that by
>>>> releasing all dirty bitmaps in bdrv_close() and drop the assertion in
>>>> bdrv_delete().
>>>
>>> What bitmaps are attached when bdrv_close() is called?  The ones created from
>>> the monitor should probably be removed by the monitor, and the internal ones
>>> like in migration and block jobs should probably be removed by stopping the
>>> respective job.
>>>
>>> Fam
>>>
>>
>> Well in this case at least it appears we are still asserting that the
>> BDS has no job attached, so it shouldn't have any internal bitmaps
>> weighing it down, which just leaves the ones created by the QMP interface.
>>
>> How important is it that we ask the user to remove all of those bitmaps
>> themselves?
>>
>> It might become more important in the future when persistence is an
>> option and we go to close a transient bitmap -- but persistent bitmaps I
>> am sure it will be safe to just close out and flush to disk.
>>
> 
> Yes, I was not saying we should expecting user to do that manually, but it
> would be good to be clear about different types of instances in the code.
> 
> For now, it's unlikely a problem.
> 
> Fam
> 

OK, just pointing out that I think it's unlikely we have any internal
bitmaps at this point since we assert that we have no jobs.

We can actually test this, since internal bitmaps are anonymous and
user-created ones must always have a name.

Tangent question: If a user closes a BDS node with a transient bitmap
attached, should we take any special action?

i.e.; do we offer the user a last chance to save the bitmap somewhere,
or do we just do what we were asked and hope the user is competent?

(I assume: No, we let the user shoot themselves in the foot if they want
to, but I wanted to ask the question.)

--js

Re: [Qemu-devel] [PATCH v7 03/24] block: Release dirty bitmaps in bdrv_close()

[Fam Zheng]

On Mon, 11/16 12:07, John Snow wrote:
> 
> 
> On 11/15/2015 08:27 PM, Fam Zheng wrote:
> > On Fri, 11/13 17:49, John Snow wrote:
> >>
> >>
> >> On 11/12/2015 01:23 AM, Fam Zheng wrote:
> >>> On Mon, 11/09 23:39, Max Reitz wrote:
> >>>> bdrv_delete() is not very happy about deleting BlockDriverStates with
> >>>> dirty bitmaps still attached to them. In the past, we got around that
> >>>> very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and
> >>>> bdrv_close() simply ignoring that condition. We should fix that by
> >>>> releasing all dirty bitmaps in bdrv_close() and drop the assertion in
> >>>> bdrv_delete().
> >>>
> >>> What bitmaps are attached when bdrv_close() is called?  The ones created from
> >>> the monitor should probably be removed by the monitor, and the internal ones
> >>> like in migration and block jobs should probably be removed by stopping the
> >>> respective job.
> >>>
> >>> Fam
> >>>
> >>
> >> Well in this case at least it appears we are still asserting that the
> >> BDS has no job attached, so it shouldn't have any internal bitmaps
> >> weighing it down, which just leaves the ones created by the QMP interface.
> >>
> >> How important is it that we ask the user to remove all of those bitmaps
> >> themselves?
> >>
> >> It might become more important in the future when persistence is an
> >> option and we go to close a transient bitmap -- but persistent bitmaps I
> >> am sure it will be safe to just close out and flush to disk.
> >>
> > 
> > Yes, I was not saying we should expecting user to do that manually, but it
> > would be good to be clear about different types of instances in the code.
> > 
> > For now, it's unlikely a problem.
> > 
> > Fam
> > 
> 
> OK, just pointing out that I think it's unlikely we have any internal
> bitmaps at this point since we assert that we have no jobs.
> 
> We can actually test this, since internal bitmaps are anonymous and
> user-created ones must always have a name.
> 
> Tangent question: If a user closes a BDS node with a transient bitmap
> attached, should we take any special action?
> 
> i.e.; do we offer the user a last chance to save the bitmap somewhere,
> or do we just do what we were asked and hope the user is competent?
> 
> (I assume: No, we let the user shoot themselves in the foot if they want
> to, but I wanted to ask the question.)
> 
> --js
> 

For persistent dirty bitmaps, we must provide an interface with which user can
make sure that, upon the shutting down of VM, the dirty bitmap file is in
consistent with the image it is tracking.

With embedded dirty bitmap in qcow2 image, this shouldn't be a problem though,
because qcow2_close can handle this internally.  But what about the senario
with "raw image" + "incremental backup"?

Fam

[Qemu-devel] Closing Bitmaps (Was: Re: [PATCH v7 03/24] block: Release dirty bitmaps in bdrv_close())

[John Snow]

On 11/16/2015 11:22 PM, Fam Zheng wrote:
> On Mon, 11/16 12:07, John Snow wrote:
>>
>>
>> On 11/15/2015 08:27 PM, Fam Zheng wrote:
>>> On Fri, 11/13 17:49, John Snow wrote:
>>>>
>>>>
>>>> On 11/12/2015 01:23 AM, Fam Zheng wrote:
>>>>> On Mon, 11/09 23:39, Max Reitz wrote:
>>>>>> bdrv_delete() is not very happy about deleting BlockDriverStates with
>>>>>> dirty bitmaps still attached to them. In the past, we got around that
>>>>>> very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and
>>>>>> bdrv_close() simply ignoring that condition. We should fix that by
>>>>>> releasing all dirty bitmaps in bdrv_close() and drop the assertion in
>>>>>> bdrv_delete().
>>>>>
>>>>> What bitmaps are attached when bdrv_close() is called?  The ones created from
>>>>> the monitor should probably be removed by the monitor, and the internal ones
>>>>> like in migration and block jobs should probably be removed by stopping the
>>>>> respective job.
>>>>>
>>>>> Fam
>>>>>
>>>>
>>>> Well in this case at least it appears we are still asserting that the
>>>> BDS has no job attached, so it shouldn't have any internal bitmaps
>>>> weighing it down, which just leaves the ones created by the QMP interface.
>>>>
>>>> How important is it that we ask the user to remove all of those bitmaps
>>>> themselves?
>>>>
>>>> It might become more important in the future when persistence is an
>>>> option and we go to close a transient bitmap -- but persistent bitmaps I
>>>> am sure it will be safe to just close out and flush to disk.
>>>>
>>>
>>> Yes, I was not saying we should expecting user to do that manually, but it
>>> would be good to be clear about different types of instances in the code.
>>>
>>> For now, it's unlikely a problem.
>>>
>>> Fam
>>>
>>
>> OK, just pointing out that I think it's unlikely we have any internal
>> bitmaps at this point since we assert that we have no jobs.
>>
>> We can actually test this, since internal bitmaps are anonymous and
>> user-created ones must always have a name.
>>
>> Tangent question: If a user closes a BDS node with a transient bitmap
>> attached, should we take any special action?
>>
>> i.e.; do we offer the user a last chance to save the bitmap somewhere,
>> or do we just do what we were asked and hope the user is competent?
>>
>> (I assume: No, we let the user shoot themselves in the foot if they want
>> to, but I wanted to ask the question.)
>>
>> --js
>>
> 
> For persistent dirty bitmaps, we must provide an interface with which user can
> make sure that, upon the shutting down of VM, the dirty bitmap file is in
> consistent with the image it is tracking.
> 
> With embedded dirty bitmap in qcow2 image, this shouldn't be a problem though,
> because qcow2_close can handle this internally.  But what about the senario
> with "raw image" + "incremental backup"?
> 
> Fam
> 

Still the subject of debate on-list, but the thought is roughly this:

Bitmaps will be able to flush-to-file on close. (If they have no
persistence data, it's a no-op (maybe.)) This might mean being flushed
to their own BDS -- the one they are describing -- as a qcow2 extension.
Or, it could be to an arbitrary new standalone file format designed for
the sole purpose of containing bitmap data.

The discussion hasn't progressed beyond "Max and Kevin do not think
storing arbitrary bitmaps in .qcow2 files is a good idea." The logical
conclusion is "We need a new standalone format, then" but we haven't
decided what that format will look like or how it will be used.

Then, Through CLI arguments or QMP arguments, you can modify the
persistence attributes of bitmaps -- choosing where to store them.
Bitmaps for qcow2 nodes can be stored in their own node, bitmaps
describing raw files will need to be stored in an external file.

(Is it possible to create a block driver that doesn't implement
read/write primitives, and only implements theoretical bitmap load/save
primitives? We could create a block driver for a standalone bitmap
container in this way...)

--js

Re: [Qemu-devel] Closing Bitmaps (Was: Re: [PATCH v7 03/24] block: Release dirty bitmaps in bdrv_close())

[Fam Zheng]

On Tue, 11/17 12:05, John Snow wrote:
> Still the subject of debate on-list, but the thought is roughly this:

Thanks for summerizing this!

> 
> Bitmaps will be able to flush-to-file on close. (If they have no
> persistence data, it's a no-op (maybe.)) This might mean being flushed
> to their own BDS -- the one they are describing -- as a qcow2 extension.
> Or, it could be to an arbitrary new standalone file format designed for
> the sole purpose of containing bitmap data.
> 
> The discussion hasn't progressed beyond "Max and Kevin do not think
> storing arbitrary bitmaps in .qcow2 files is a good idea." The logical
> conclusion is "We need a new standalone format, then" but we haven't
> decided what that format will look like or how it will be used.

I second that. While storing dirty bitmap in the image itself sounds fine, it
is unlikely the best way for any other formats. Given that we will have a
standalone format for raw or other formats, it's probably not worth to extend
qcow2 specifically.

> 
> Then, Through CLI arguments or QMP arguments, you can modify the
> persistence attributes of bitmaps -- choosing where to store them.
> Bitmaps for qcow2 nodes can be stored in their own node, bitmaps
> describing raw files will need to be stored in an external file.
> 
> (Is it possible to create a block driver that doesn't implement
> read/write primitives, and only implements theoretical bitmap load/save
> primitives? We could create a block driver for a standalone bitmap
> container in this way...)

What about implementing it as a filter? It would work similar to blkdebug, with
.bdrv_co_writev to mark the in memory dirty bits (or even manage a
sophisticated cache to support large images efficiently), and .bdrv_close
to flush data to disk.

But that depends on the dynamic reconfigiration feature of block nodes, for
hot add/remove of dirty bitmap to work.

Fam

Re: [Qemu-devel] [PATCH v7 22/24] block: Rewrite bdrv_close_all()

[Fam Zheng]

On Mon, 11/16 17:15, Max Reitz wrote:
> >> @@ -1971,13 +1969,44 @@ static void bdrv_close(BlockDriverState *bs)
> >>  void bdrv_close_all(void)
> >>  {
> >>      BlockDriverState *bs;
> >> +    AioContext *aio_context;
> >> +    int original_refcount = 0;
> >>  
> >> -    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> >> -        AioContext *aio_context = bdrv_get_aio_context(bs);
> >> +    /* Drop references from requests still in flight, such as canceled block
> >> +     * jobs whose AIO context has not been polled yet */
> >> +    bdrv_drain_all();
> >>  
> >> -        aio_context_acquire(aio_context);
> >> -        bdrv_close(bs);
> >> -        aio_context_release(aio_context);
> >> +    blockdev_close_all_bdrv_states();
> >> +    blk_remove_all_bs();
> >> +
> >> +    /* Cancel all block jobs */
> >> +    while (!QTAILQ_EMPTY(&all_bdrv_states)) {
> >> +        QTAILQ_FOREACH(bs, &all_bdrv_states, bs_list) {
> >> +            aio_context = bdrv_get_aio_context(bs);
> >> +
> >> +            aio_context_acquire(aio_context);
> >> +            if (bs->job) {
> >> +                /* So we can safely query the current refcount */
> >> +                bdrv_ref(bs);
> >> +                original_refcount = bs->refcnt;
> >> +
> >> +                block_job_cancel_sync(bs->job);
> >> +                aio_context_release(aio_context);
> >> +                break;
> >> +            }
> >> +            aio_context_release(aio_context);
> >> +        }
> >> +
> >> +        /* All the remaining BlockDriverStates are referenced directly or
> >> +         * indirectly from block jobs, so there needs to be at least one BDS
> >> +         * directly used by a block job */
> >> +        assert(bs);
> >> +
> >> +        /* Wait for the block job to release its reference */
> >> +        while (bs->refcnt >= original_refcount) {
> >> +            aio_poll(aio_context, true);
> >> +        }
> >> +        bdrv_unref(bs);
> > 
> > If at this point bs->refcnt is greater than 1, why don't we care where are the
> > remaining references from?
> 
> We do care. A BDS will not be removed from all_bdrv_states until it is
> deleted (i.e. its refcount becomes 0). Therefore, this loop will
> continue until all BDSs have been deleted.
> 
> So where might additional references come from? Since this loop only
> cares about direct or indirect references from block jobs, that's
> exactly it:
> 
> (1) You might have multiple block jobs running on a BDS in the future.
>     Then, you'll cancel them one by one, and after having canceled the
>     first one, the refcount will still be greater than one before the
>     bdrv_unref().
> 
> (2) Imagine a BDS A with a parent BDS B. There are block jobs running on
>     both of them. Now, B is referenced by both its block job and by A
>     (indirectly by the block job referencing A). If we cancel the job on
>     B before the one on A, then the refcount on B will still be greater
>     than 1 before bdrv_unref() because it is still referenced by its
>     parent (until the block job on A is canceled, too).
> 
> The first cannot happen right now, but the second one may, I'm not sure
> (depending on whether op blockers allow it).

OK, that makes sense. The all_bdrv_states is the central place to make sure all
refcnts reaching 0.

> 
> 
> And we do make sure that there are no additional references besides:
> - directly from the monitor (monitor-owned BDSs)
> - from a BB
> - from block jobs
> - (+ everything transitively through the respective BDS tree)
> 
> If there were additional references, the inner loop would at some point
> no longer find a BDS with a block job while all_bdrv_states is still not
> empty. That's what the "assert(bs)" after the inner loop is for.
> 
> If you can imagine another way where a reference to a BDS may come from,
> that would be a bug in this patch and we have to make sure to respect
> that case, too.

I think the only one reference I'm not sure is in xen_disk. blk_unref is called
in the .free and .disconnect call backs but I have no idea if they are called
before bdrv_close_all.

Otherwise this patch looks good for me.

Thanks,

Fam

Re: [Qemu-devel] Closing Bitmaps (Was: Re: [PATCH v7 03/24] block: Release dirty bitmaps in bdrv_close())

[Kevin Wolf]

Am 17.11.2015 um 18:05 hat John Snow geschrieben:
> Still the subject of debate on-list, but the thought is roughly this:
> 
> Bitmaps will be able to flush-to-file on close. (If they have no
> persistence data, it's a no-op (maybe.)) This might mean being flushed
> to their own BDS -- the one they are describing -- as a qcow2 extension.
> Or, it could be to an arbitrary new standalone file format designed for
> the sole purpose of containing bitmap data.
> 
> The discussion hasn't progressed beyond "Max and Kevin do not think
> storing arbitrary bitmaps in .qcow2 files is a good idea." The logical
> conclusion is "We need a new standalone format, then" but we haven't
> decided what that format will look like or how it will be used.

I think the actual logical conclusion is that you use qcow2 images in
order to use the feature.

Kevin

Re: [Qemu-devel] [PATCH v7 02/24] blockjob: Call bdrv_unref() on creation error

[Kevin Wolf]

Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> If block_job_create() fails, it should release its reference to the
> job's BDS. Normally, this is done in the callback provided by the
> caller, but that callback will not be invoked if the block job failed to
> be created.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Commit 18930ba3 already fixed this. Now adding another bdrv_unref()
would be wrong, so I guess we have to simply drop the patch.

NACK

Re: [Qemu-devel] [PATCH v7 01/24] blockdev: Add missing bdrv_unref() in drive-backup

[Kevin Wolf]

Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> All error paths after a successful bdrv_open() of target_bs should
> contain a bdrv_unref(target_bs). This one did not yet, so add it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Thanks, applied for 2.5.

Kevin

Re: [Qemu-devel] Closing Bitmaps (Was: Re: [PATCH v7 03/24] block: Release dirty bitmaps in bdrv_close())

[John Snow]

On 11/17/2015 09:29 PM, Fam Zheng wrote:
> On Tue, 11/17 12:05, John Snow wrote:
>> Still the subject of debate on-list, but the thought is roughly this:
> 
> Thanks for summerizing this!
> 
>>
>> Bitmaps will be able to flush-to-file on close. (If they have no
>> persistence data, it's a no-op (maybe.)) This might mean being flushed
>> to their own BDS -- the one they are describing -- as a qcow2 extension.
>> Or, it could be to an arbitrary new standalone file format designed for
>> the sole purpose of containing bitmap data.
>>
>> The discussion hasn't progressed beyond "Max and Kevin do not think
>> storing arbitrary bitmaps in .qcow2 files is a good idea." The logical
>> conclusion is "We need a new standalone format, then" but we haven't
>> decided what that format will look like or how it will be used.
> 
> I second that. While storing dirty bitmap in the image itself sounds fine, it
> is unlikely the best way for any other formats. Given that we will have a
> standalone format for raw or other formats, it's probably not worth to extend
> qcow2 specifically.
> 

We do already have the code, though, and for simple desktop use cases
it's very, very convenient, though it does leave us with two usage cases
for bitmaps.

I believe Denis and Vladimir plan to allow for Parallels to support the
inline bitmap load/store primitive, too.

I think the model of "Store in the image if you can, use an external
otherwise" is perfectly fine -- if not slightly more complicated than
what the bare minimum feature requires. I think the usability boost it
provides to users is worth the maintenance and review burden.

For now, I think Vladimir is working on the qcow2 extension first, and
we'll visit the generic file format later.

Is it possible to create a block driver that *only* supports the bitmap
load/store primitives? (I.e. -- no read/write support at all?)

>>
>> Then, Through CLI arguments or QMP arguments, you can modify the
>> persistence attributes of bitmaps -- choosing where to store them.
>> Bitmaps for qcow2 nodes can be stored in their own node, bitmaps
>> describing raw files will need to be stored in an external file.
>>
>> (Is it possible to create a block driver that doesn't implement
>> read/write primitives, and only implements theoretical bitmap load/save
>> primitives? We could create a block driver for a standalone bitmap
>> container in this way...)
> 
> What about implementing it as a filter? It would work similar to blkdebug, with
> .bdrv_co_writev to mark the in memory dirty bits (or even manage a
> sophisticated cache to support large images efficiently), and .bdrv_close
> to flush data to disk.
> 
> But that depends on the dynamic reconfigiration feature of block nodes, for
> hot add/remove of dirty bitmap to work.
> 
> Fam
> 

Filters I think would be the ideal path, but if the QMP and CLI features
around dynamic reconfiguration are not quite ready, that route may not
be open to us... or not in a timely fashion, anyway.

Re: [Qemu-devel] Closing Bitmaps (Was: Re: [PATCH v7 03/24] block: Release dirty bitmaps in bdrv_close())

[John Snow]

On 11/18/2015 10:03 AM, Kevin Wolf wrote:
> Am 17.11.2015 um 18:05 hat John Snow geschrieben:
>> Still the subject of debate on-list, but the thought is roughly this:
>>
>> Bitmaps will be able to flush-to-file on close. (If they have no
>> persistence data, it's a no-op (maybe.)) This might mean being flushed
>> to their own BDS -- the one they are describing -- as a qcow2 extension.
>> Or, it could be to an arbitrary new standalone file format designed for
>> the sole purpose of containing bitmap data.
>>
>> The discussion hasn't progressed beyond "Max and Kevin do not think
>> storing arbitrary bitmaps in .qcow2 files is a good idea." The logical
>> conclusion is "We need a new standalone format, then" but we haven't
>> decided what that format will look like or how it will be used.
> 
> I think the actual logical conclusion is that you use qcow2 images in
> order to use the feature.
> 
> Kevin
> 

It's fine to say "To hell with raw," but for networked filesystems and
other configurations that aren't using the qcow2 driver, I think it
won't be a sufficient answer.

qcow2 support is my first priority, but I think it can't be my only one.

Re: [Qemu-devel] [PATCH v7 12/24] virtio-blk: Functions for op blocker management

[Kevin Wolf]

Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> Put the code for setting up and removing op blockers into an own
> function, respectively. Then, we can invoke those functions whenever a
> BDS is removed from an virtio-blk BB or inserted into it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Do you know of a case where this is observable? I don't think you can
change the medium of a virtio-blk device, and blk_set_bs() isn't
converted to a wrapper around blk_remove/insert_bs() yet. If this patch
is necessary to fix something observable, the latter is probably a bug.

> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index c42ddeb..4c95d5b 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -39,6 +39,8 @@ struct VirtIOBlockDataPlane {
>      EventNotifier *guest_notifier;  /* irq */
>      QEMUBH *bh;                     /* bh for guest notification */
>  
> +    Notifier insert_notifier, remove_notifier;
> +
>      /* Note that these EventNotifiers are assigned by value.  This is
>       * fine as long as you do not call event_notifier_cleanup on them
>       * (because you don't own the file descriptor or handle; you just
> @@ -137,6 +139,54 @@ static void handle_notify(EventNotifier *e)
>      blk_io_unplug(s->conf->conf.blk);
>  }
>  
> +static void data_plane_set_up_op_blockers(VirtIOBlockDataPlane *s)
> +{
> +    assert(!s->blocker);
> +    error_setg(&s->blocker, "block device is in use by data plane");
> +    blk_op_block_all(s->conf->conf.blk, s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_RESIZE, s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_DRIVE_DEL, s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_CHANGE, s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_TARGET, s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EJECT, s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
> +                   s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT,
> +                   s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
> +                   s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_MIRROR, s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_STREAM, s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_REPLACE, s->blocker);
> +}

This makes me wonder: What do we even block here any more? If I didn't
miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure
why this needs to be blocked, or if we simply forgot to enable it.

Kevin

Re: [Qemu-devel] [PATCH v7 12/24] virtio-blk: Functions for op blocker management

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3429 bytes --]

On 25.11.2015 16:57, Kevin Wolf wrote:
> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
>> Put the code for setting up and removing op blockers into an own
>> function, respectively. Then, we can invoke those functions whenever a
>> BDS is removed from an virtio-blk BB or inserted into it.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> Do you know of a case where this is observable?

Actually, no.

>                                                 I don't think you can
> change the medium of a virtio-blk device, and blk_set_bs() isn't
> converted to a wrapper around blk_remove/insert_bs() yet. If this patch
> is necessary to fix something observable, the latter is probably a bug.

So I guess that implies "Otherwise, this patch should be dropped"?

>> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
>> index c42ddeb..4c95d5b 100644
>> --- a/hw/block/dataplane/virtio-blk.c
>> +++ b/hw/block/dataplane/virtio-blk.c
>> @@ -39,6 +39,8 @@ struct VirtIOBlockDataPlane {
>>      EventNotifier *guest_notifier;  /* irq */
>>      QEMUBH *bh;                     /* bh for guest notification */
>>  
>> +    Notifier insert_notifier, remove_notifier;
>> +
>>      /* Note that these EventNotifiers are assigned by value.  This is
>>       * fine as long as you do not call event_notifier_cleanup on them
>>       * (because you don't own the file descriptor or handle; you just
>> @@ -137,6 +139,54 @@ static void handle_notify(EventNotifier *e)
>>      blk_io_unplug(s->conf->conf.blk);
>>  }
>>  
>> +static void data_plane_set_up_op_blockers(VirtIOBlockDataPlane *s)
>> +{
>> +    assert(!s->blocker);
>> +    error_setg(&s->blocker, "block device is in use by data plane");
>> +    blk_op_block_all(s->conf->conf.blk, s->blocker);
>> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_RESIZE, s->blocker);
>> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_DRIVE_DEL, s->blocker);
>> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, s->blocker);
>> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_CHANGE, s->blocker);
>> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, s->blocker);
>> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_TARGET, s->blocker);
>> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EJECT, s->blocker);
>> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
>> +                   s->blocker);
>> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT,
>> +                   s->blocker);
>> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
>> +                   s->blocker);
>> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_MIRROR, s->blocker);
>> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_STREAM, s->blocker);
>> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_REPLACE, s->blocker);
>> +}
> 
> This makes me wonder: What do we even block here any more? If I didn't
> miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure
> why this needs to be blocked, or if we simply forgot to enable it.

Well, even though in practice this wall of code doesn't make much sense,
of course it will be safe for potential additions of new op blockers.

And of course we actually don't want these blockers at all anymore...

Max


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

Re: [Qemu-devel] [PATCH v7 13/24] virtio-scsi: Catch BDS-BB removal/insertion

[Kevin Wolf]

Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> Make use of the BDS-BB removal and insertion notifiers to remove or set
> up, respectively, virtio-scsi's op blockers.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

> @@ -797,6 +830,29 @@ static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      if (s->ctx) {
>          blk_op_unblock_all(sd->conf.blk, s->blocker);
>      }
> +
> +    QTAILQ_FOREACH(insert_notifier, &s->insert_notifiers, next) {
> +        if (insert_notifier->sd == sd) {
> +            break;
> +        }
> +    }
> +    if (insert_notifier) {
> +        notifier_remove(&insert_notifier->n);
> +        QTAILQ_REMOVE(&s->insert_notifiers, insert_notifier, next);
> +        g_free(insert_notifier);
> +    }

Why a separate if block instead of just doing that inside the loop?

> +    QTAILQ_FOREACH(remove_notifier, &s->remove_notifiers, next) {
> +        if (remove_notifier->sd == sd) {
> +            break;
> +        }
> +    }
> +    if (remove_notifier) {
> +        notifier_remove(&remove_notifier->n);
> +        QTAILQ_REMOVE(&s->remove_notifiers, remove_notifier, next);
> +        g_free(remove_notifier);
> +    }
> +
>      qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
>  }

Kevin

Re: [Qemu-devel] [PATCH v7 13/24] virtio-scsi: Catch BDS-BB removal/insertion

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1811 bytes --]

On 25.11.2015 17:03, Kevin Wolf wrote:
> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
>> Make use of the BDS-BB removal and insertion notifiers to remove or set
>> up, respectively, virtio-scsi's op blockers.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
>> @@ -797,6 +830,29 @@ static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>      if (s->ctx) {
>>          blk_op_unblock_all(sd->conf.blk, s->blocker);
>>      }
>> +
>> +    QTAILQ_FOREACH(insert_notifier, &s->insert_notifiers, next) {
>> +        if (insert_notifier->sd == sd) {
>> +            break;
>> +        }
>> +    }
>> +    if (insert_notifier) {
>> +        notifier_remove(&insert_notifier->n);
>> +        QTAILQ_REMOVE(&s->insert_notifiers, insert_notifier, next);
>> +        g_free(insert_notifier);
>> +    }
> 
> Why a separate if block instead of just doing that inside the loop?

Because I unconsciously try to reduce indentation.

Also, because when I wrote the code, to me it was "Find the relevant
notifier -- destroy that notifier", and that's how this pattern came about.

I personally still like it more the way I did it, but I can very well
imagine that I'm the only one who thinks so. Therefore, I wouldn't mind
changing it (besides the effort involved to change it).

Max

>> +    QTAILQ_FOREACH(remove_notifier, &s->remove_notifiers, next) {
>> +        if (remove_notifier->sd == sd) {
>> +            break;
>> +        }
>> +    }
>> +    if (remove_notifier) {
>> +        notifier_remove(&remove_notifier->n);
>> +        QTAILQ_REMOVE(&s->remove_notifiers, remove_notifier, next);
>> +        g_free(remove_notifier);
>> +    }
>> +
>>      qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
>>  }
> 
> Kevin
> 



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

Re: [Qemu-devel] [PATCH v7 for-2.6 00/24] block: Rework bdrv_close_all()

[Kevin Wolf]

Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> Currently, bdrv_close_all() force-closes all BDSs with a BlockBackend,
> which can lead to data corruption (see the iotest added in the final
> patch of this series) and is most certainly very ugly.
> 
> This series reworks bdrv_close_all() to instead eject the BDS trees from
> all BlockBackends and then close the monitor-owned BDS trees, which are
> the only BDSs without a BB. In effect, all BDSs are closed just by
> getting closed automatically due to their reference count becoming 0.
> 
> Note that the approach taken here leaks all BlockBackends. This does not
> really matter, however, since qemu is about to exit anyway.

Patches 4-11:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v7 12/24] virtio-blk: Functions for op blocker management

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 4229 bytes --]

Am 25.11.2015 um 17:03 hat Max Reitz geschrieben:
> On 25.11.2015 16:57, Kevin Wolf wrote:
> > Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> >> Put the code for setting up and removing op blockers into an own
> >> function, respectively. Then, we can invoke those functions whenever a
> >> BDS is removed from an virtio-blk BB or inserted into it.
> >>
> >> Signed-off-by: Max Reitz <mreitz@redhat.com>
> > 
> > Do you know of a case where this is observable?
> 
> Actually, no.
> 
> >                                                 I don't think you can
> > change the medium of a virtio-blk device, and blk_set_bs() isn't
> > converted to a wrapper around blk_remove/insert_bs() yet. If this patch
> > is necessary to fix something observable, the latter is probably a bug.
> 
> So I guess that implies "Otherwise, this patch should be dropped"?

I'm not sure. I guess you had a reason to include these patches other
than putting the notifiers to use?

With blk_set_bs() changed, I think it would have an effect: The op
blockers would move from the old BDS to the new top-level one. This
sounds desirable to me.

> >> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> >> index c42ddeb..4c95d5b 100644
> >> --- a/hw/block/dataplane/virtio-blk.c
> >> +++ b/hw/block/dataplane/virtio-blk.c
> >> @@ -39,6 +39,8 @@ struct VirtIOBlockDataPlane {
> >>      EventNotifier *guest_notifier;  /* irq */
> >>      QEMUBH *bh;                     /* bh for guest notification */
> >>  
> >> +    Notifier insert_notifier, remove_notifier;
> >> +
> >>      /* Note that these EventNotifiers are assigned by value.  This is
> >>       * fine as long as you do not call event_notifier_cleanup on them
> >>       * (because you don't own the file descriptor or handle; you just
> >> @@ -137,6 +139,54 @@ static void handle_notify(EventNotifier *e)
> >>      blk_io_unplug(s->conf->conf.blk);
> >>  }
> >>  
> >> +static void data_plane_set_up_op_blockers(VirtIOBlockDataPlane *s)
> >> +{
> >> +    assert(!s->blocker);
> >> +    error_setg(&s->blocker, "block device is in use by data plane");
> >> +    blk_op_block_all(s->conf->conf.blk, s->blocker);
> >> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_RESIZE, s->blocker);
> >> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_DRIVE_DEL, s->blocker);
> >> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, s->blocker);
> >> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_CHANGE, s->blocker);
> >> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, s->blocker);
> >> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_TARGET, s->blocker);
> >> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EJECT, s->blocker);
> >> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
> >> +                   s->blocker);
> >> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT,
> >> +                   s->blocker);
> >> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
> >> +                   s->blocker);
> >> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_MIRROR, s->blocker);
> >> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_STREAM, s->blocker);
> >> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_REPLACE, s->blocker);
> >> +}
> > 
> > This makes me wonder: What do we even block here any more? If I didn't
> > miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure
> > why this needs to be blocked, or if we simply forgot to enable it.
> 
> Well, even though in practice this wall of code doesn't make much sense,
> of course it will be safe for potential additions of new op blockers.
> 
> And of course we actually don't want these blockers at all anymore...

Yes, but dataplane shouldn't really be special enough any more that we
want to disable features for it initially. By now it sounds more like an
easy way to forget unblocking a new feature even though it would work.

So perhaps we should really just remove the blockers from dataplane.
Then we don't have to answer the question above...

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v7 12/24] virtio-blk: Functions for op blocker management

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2684 bytes --]

On 25.11.2015 17:18, Kevin Wolf wrote:
> Am 25.11.2015 um 17:03 hat Max Reitz geschrieben:
>> On 25.11.2015 16:57, Kevin Wolf wrote:
>>> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
>>>> Put the code for setting up and removing op blockers into an own
>>>> function, respectively. Then, we can invoke those functions whenever a
>>>> BDS is removed from an virtio-blk BB or inserted into it.
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>
>>> Do you know of a case where this is observable?
>>
>> Actually, no.
>>
>>>                                                 I don't think you can
>>> change the medium of a virtio-blk device, and blk_set_bs() isn't
>>> converted to a wrapper around blk_remove/insert_bs() yet. If this patch
>>> is necessary to fix something observable, the latter is probably a bug.
>>
>> So I guess that implies "Otherwise, this patch should be dropped"?
> 
> I'm not sure. I guess you had a reason to include these patches other
> than putting the notifiers to use?

I'm not sure, it has been so long. :-)

I can very well imagine having included it because a similar change is
necessary to virtio-scsi, so I included it just because I was already
doing work for virtio. Maybe I imagined that virtio-blk may reasonably
offer tray devices in the future, but I just now read you saying
somewhere else:

> Please write your code so that it makes sense today.

So that doesn't hold up. :-)

Maybe I just saw it unblocking the EJECT op blocker, and so I thought
there might be a reason for that.

> With blk_set_bs() changed, I think it would have an effect: The op
> blockers would move from the old BDS to the new top-level one. This
> sounds desirable to me.

Hm, thanks for saving me. Yes, that seems useful indeed.

[...]

>>> This makes me wonder: What do we even block here any more? If I didn't
>>> miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure
>>> why this needs to be blocked, or if we simply forgot to enable it.
>>
>> Well, even though in practice this wall of code doesn't make much sense,
>> of course it will be safe for potential additions of new op blockers.
>>
>> And of course we actually don't want these blockers at all anymore...
> 
> Yes, but dataplane shouldn't really be special enough any more that we
> want to disable features for it initially. By now it sounds more like an
> easy way to forget unblocking a new feature even though it would work.
> 
> So perhaps we should really just remove the blockers from dataplane.
> Then we don't have to answer the question above...

Well, maybe. I guess this is up to Stefan.

Max


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

Re: [Qemu-devel] [PATCH v7 12/24] virtio-blk: Functions for op blocker management

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1425 bytes --]

On Wed, Nov 25, 2015 at 05:26:02PM +0100, Max Reitz wrote:
> On 25.11.2015 17:18, Kevin Wolf wrote:
> > Am 25.11.2015 um 17:03 hat Max Reitz geschrieben:
> >> On 25.11.2015 16:57, Kevin Wolf wrote:
> >>> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> >>> This makes me wonder: What do we even block here any more? If I didn't
> >>> miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure
> >>> why this needs to be blocked, or if we simply forgot to enable it.
> >>
> >> Well, even though in practice this wall of code doesn't make much sense,
> >> of course it will be safe for potential additions of new op blockers.
> >>
> >> And of course we actually don't want these blockers at all anymore...
> > 
> > Yes, but dataplane shouldn't really be special enough any more that we
> > want to disable features for it initially. By now it sounds more like an
> > easy way to forget unblocking a new feature even though it would work.
> > 
> > So perhaps we should really just remove the blockers from dataplane.
> > Then we don't have to answer the question above...
> 
> Well, maybe. I guess this is up to Stefan.

At this point blockdev.c and block jobs acquire/release AioContext,
hence all these op blockers are being unblocked.  I think we can switch
from whitelisting (unblocking) nearly everything to blacklisting
(blocking) only things that aren't supported yet.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v7 12/24] virtio-blk: Functions for op blocker management

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1719 bytes --]

Am 26.11.2015 um 08:48 hat Stefan Hajnoczi geschrieben:
> On Wed, Nov 25, 2015 at 05:26:02PM +0100, Max Reitz wrote:
> > On 25.11.2015 17:18, Kevin Wolf wrote:
> > > Am 25.11.2015 um 17:03 hat Max Reitz geschrieben:
> > >> On 25.11.2015 16:57, Kevin Wolf wrote:
> > >>> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> > >>> This makes me wonder: What do we even block here any more? If I didn't
> > >>> miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure
> > >>> why this needs to be blocked, or if we simply forgot to enable it.
> > >>
> > >> Well, even though in practice this wall of code doesn't make much sense,
> > >> of course it will be safe for potential additions of new op blockers.
> > >>
> > >> And of course we actually don't want these blockers at all anymore...
> > > 
> > > Yes, but dataplane shouldn't really be special enough any more that we
> > > want to disable features for it initially. By now it sounds more like an
> > > easy way to forget unblocking a new feature even though it would work.
> > > 
> > > So perhaps we should really just remove the blockers from dataplane.
> > > Then we don't have to answer the question above...
> > 
> > Well, maybe. I guess this is up to Stefan.
> 
> At this point blockdev.c and block jobs acquire/release AioContext,
> hence all these op blockers are being unblocked.  I think we can switch
> from whitelisting (unblocking) nearly everything to blacklisting
> (blocking) only things that aren't supported yet.

As I said, there is only one operation that isn't whitelisted today,
BLOCK_OP_TYPE_BACKUP_TARGET, and I would be surprised if it weren't just
a bug that it's not whitelisted yet.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v7 14/24] nbd: Switch from close to eject notifier

[Kevin Wolf]

Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> The NBD code uses the BDS close notifier to determine when a medium is
> ejected. However, now it should use the BB's BDS removal notifier for
> that instead of the BDS's close notifier.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  blockdev-nbd.c | 37 +------------------------------------
>  nbd.c          | 13 +++++++++++++
>  2 files changed, 14 insertions(+), 36 deletions(-)
> 
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index bcdd18b..b28a55b 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -46,37 +46,11 @@ void qmp_nbd_server_start(SocketAddress *addr, Error **errp)
>      }
>  }
>  
> -/*
> - * Hook into the BlockBackend notifiers to close the export when the
> - * backend is closed.
> - */
> -typedef struct NBDCloseNotifier {
> -    Notifier n;
> -    NBDExport *exp;
> -    QTAILQ_ENTRY(NBDCloseNotifier) next;
> -} NBDCloseNotifier;
> -
> -static QTAILQ_HEAD(, NBDCloseNotifier) close_notifiers =
> -    QTAILQ_HEAD_INITIALIZER(close_notifiers);
> -
> -static void nbd_close_notifier(Notifier *n, void *data)
> -{
> -    NBDCloseNotifier *cn = DO_UPCAST(NBDCloseNotifier, n, n);
> -
> -    notifier_remove(&cn->n);
> -    QTAILQ_REMOVE(&close_notifiers, cn, next);
> -
> -    nbd_export_close(cn->exp);
> -    nbd_export_put(cn->exp);

Here you remove a close/put pair, but in the new code you only add the
close call. Why is this correct?

Kevin

Re: [Qemu-devel] [PATCH v7 15/24] block: Remove BDS close notifier

[Kevin Wolf]

Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> It is unused now, so we can remove it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v7 24/24] iotests: Add test for block jobs and BDS ejection

[Kevin Wolf]

Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/141     | 166 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/141.out |  47 +++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 214 insertions(+)
>  create mode 100755 tests/qemu-iotests/141
>  create mode 100644 tests/qemu-iotests/141.out
> 
> diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141
> new file mode 100755
> index 0000000..6a32d56
> --- /dev/null
> +++ b/tests/qemu-iotests/141
> @@ -0,0 +1,166 @@
> +#!/bin/bash
> +#
> +# Test case for ejecting BDSs with block jobs still running on them
> +#
> +# Copyright (C) 2015 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +# creator
> +owner=mreitz@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_DIR/{b,o}.$IMGFMT"
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +. ./common.qemu
> +
> +# Needs backing file support
> +_supported_fmt qcow qcow2 qed

The test doesn't work for me on qcow1.

> +echo
> +echo '=== Testing block-commit ==='
> +echo
> +
> +# block-commit will send BLOCK_JOB_READY basically immediately, and cancelling
> +# the job will consequently result in BLOCK_JOB_COMPLETED being emitted.
> +
> +test_blockjob \
> +    "{'execute': 'block-commit',
> +      'arguments': {'device': 'drv0'}}" \
> +    'BLOCK_JOB_READY' \
> +    'BLOCK_JOB_COMPLETED'

This is commit of the active layer, i.e. just a mirror in disguise.
Should we test a "real" commit block job as well?

Anyway, with qcow1 removed from the list:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v7 14/24] nbd: Switch from close to eject notifier

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2914 bytes --]

On 30.11.2015 16:36, Kevin Wolf wrote:
> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
>> The NBD code uses the BDS close notifier to determine when a medium is
>> ejected. However, now it should use the BB's BDS removal notifier for
>> that instead of the BDS's close notifier.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  blockdev-nbd.c | 37 +------------------------------------
>>  nbd.c          | 13 +++++++++++++
>>  2 files changed, 14 insertions(+), 36 deletions(-)
>>
>> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
>> index bcdd18b..b28a55b 100644
>> --- a/blockdev-nbd.c
>> +++ b/blockdev-nbd.c
>> @@ -46,37 +46,11 @@ void qmp_nbd_server_start(SocketAddress *addr, Error **errp)
>>      }
>>  }
>>  
>> -/*
>> - * Hook into the BlockBackend notifiers to close the export when the
>> - * backend is closed.
>> - */
>> -typedef struct NBDCloseNotifier {
>> -    Notifier n;
>> -    NBDExport *exp;
>> -    QTAILQ_ENTRY(NBDCloseNotifier) next;
>> -} NBDCloseNotifier;
>> -
>> -static QTAILQ_HEAD(, NBDCloseNotifier) close_notifiers =
>> -    QTAILQ_HEAD_INITIALIZER(close_notifiers);
>> -
>> -static void nbd_close_notifier(Notifier *n, void *data)
>> -{
>> -    NBDCloseNotifier *cn = DO_UPCAST(NBDCloseNotifier, n, n);
>> -
>> -    notifier_remove(&cn->n);
>> -    QTAILQ_REMOVE(&close_notifiers, cn, next);
>> -
>> -    nbd_export_close(cn->exp);
>> -    nbd_export_put(cn->exp);
> 
> Here you remove a close/put pair, but in the new code you only add the
> close call. Why is this correct?

Thanks for putting so much unfounded faith in me. :-)

After having put quite some time into looking into it, first I have to
say that it's a very good question.

First off, it's wrong. This is because I thought every export would have
a refcount of one. Turns out this is wrong, they have a refcount of two:
It is created with a refcount of one, and then it gains another by
giving it a name and entering it into the export list.

I did know about the second but I completely forgot about the former.
And indeed I do think it is wrong. qmp_nbd_server_add() does not keep
the reference to the export, once the function returns, it is gone.
Therefore, it should call nbd_export_put() before returning.

So in my opinion the current code is wrong and I didn't fix it enough.
*cough*

So, with the nbd_export_put() added to qmp_nbd_server_add(), every
export will have a refcount of 1 + |clients|. The eject notifier will
call nbd_close_notifier(), which first disconnects the clients, thus
reducing the refcount by |clients|, and then sets the name to NULL, thus
dropping the final refcount and destroying the export.

In the old code, we needed another nbd_export_put() because of the
superfluous reference from nbd_export_new(), which doesn't actually
exist for blockdev-nbd (it does for qemu-nbd, though).

Max


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

Re: [Qemu-devel] [PATCH v7 24/24] iotests: Add test for block jobs and BDS ejection

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3146 bytes --]

On 30.11.2015 17:23, Kevin Wolf wrote:
> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  tests/qemu-iotests/141     | 166 +++++++++++++++++++++++++++++++++++++++++++++
>>  tests/qemu-iotests/141.out |  47 +++++++++++++
>>  tests/qemu-iotests/group   |   1 +
>>  3 files changed, 214 insertions(+)
>>  create mode 100755 tests/qemu-iotests/141
>>  create mode 100644 tests/qemu-iotests/141.out
>>
>> diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141
>> new file mode 100755
>> index 0000000..6a32d56
>> --- /dev/null
>> +++ b/tests/qemu-iotests/141
>> @@ -0,0 +1,166 @@
>> +#!/bin/bash
>> +#
>> +# Test case for ejecting BDSs with block jobs still running on them
>> +#
>> +# Copyright (C) 2015 Red Hat, Inc.
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 2 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +#
>> +
>> +# creator
>> +owner=mreitz@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_DIR/{b,o}.$IMGFMT"
>> +}
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +# get standard environment, filters and checks
>> +. ./common.rc
>> +. ./common.filter
>> +. ./common.qemu
>> +
>> +# Needs backing file support
>> +_supported_fmt qcow qcow2 qed
> 
> The test doesn't work for me on qcow1.

Hm, and I thought I had tested it. Well, block jobs creating an overlay
file not being supported on qcow1 is probably all right.

>> +echo
>> +echo '=== Testing block-commit ==='
>> +echo
>> +
>> +# block-commit will send BLOCK_JOB_READY basically immediately, and cancelling
>> +# the job will consequently result in BLOCK_JOB_COMPLETED being emitted.
>> +
>> +test_blockjob \
>> +    "{'execute': 'block-commit',
>> +      'arguments': {'device': 'drv0'}}" \
>> +    'BLOCK_JOB_READY' \
>> +    'BLOCK_JOB_COMPLETED'
> 
> This is commit of the active layer, i.e. just a mirror in disguise.
> Should we test a "real" commit block job as well?

Well, the op blocker we are testing is set by block_job_create(), so a
single block job would have sufficed. But now that I'm trying to test
them all, there's no reason not to test the real commit job, too.

> Anyway, with qcow1 removed from the list:
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Thanks!

Max


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

Re: [Qemu-devel] [PATCH v7 14/24] nbd: Switch from close to eject notifier

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 3285 bytes --]

Am 30.11.2015 um 18:22 hat Max Reitz geschrieben:
> On 30.11.2015 16:36, Kevin Wolf wrote:
> > Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> >> The NBD code uses the BDS close notifier to determine when a medium is
> >> ejected. However, now it should use the BB's BDS removal notifier for
> >> that instead of the BDS's close notifier.
> >>
> >> Signed-off-by: Max Reitz <mreitz@redhat.com>
> >> ---
> >>  blockdev-nbd.c | 37 +------------------------------------
> >>  nbd.c          | 13 +++++++++++++
> >>  2 files changed, 14 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> >> index bcdd18b..b28a55b 100644
> >> --- a/blockdev-nbd.c
> >> +++ b/blockdev-nbd.c
> >> @@ -46,37 +46,11 @@ void qmp_nbd_server_start(SocketAddress *addr, Error **errp)
> >>      }
> >>  }
> >>  
> >> -/*
> >> - * Hook into the BlockBackend notifiers to close the export when the
> >> - * backend is closed.
> >> - */
> >> -typedef struct NBDCloseNotifier {
> >> -    Notifier n;
> >> -    NBDExport *exp;
> >> -    QTAILQ_ENTRY(NBDCloseNotifier) next;
> >> -} NBDCloseNotifier;
> >> -
> >> -static QTAILQ_HEAD(, NBDCloseNotifier) close_notifiers =
> >> -    QTAILQ_HEAD_INITIALIZER(close_notifiers);
> >> -
> >> -static void nbd_close_notifier(Notifier *n, void *data)
> >> -{
> >> -    NBDCloseNotifier *cn = DO_UPCAST(NBDCloseNotifier, n, n);
> >> -
> >> -    notifier_remove(&cn->n);
> >> -    QTAILQ_REMOVE(&close_notifiers, cn, next);
> >> -
> >> -    nbd_export_close(cn->exp);
> >> -    nbd_export_put(cn->exp);
> > 
> > Here you remove a close/put pair, but in the new code you only add the
> > close call. Why is this correct?
> 
> Thanks for putting so much unfounded faith in me. :-)
> 
> After having put quite some time into looking into it, first I have to
> say that it's a very good question.
> 
> First off, it's wrong. This is because I thought every export would have
> a refcount of one. Turns out this is wrong, they have a refcount of two:
> It is created with a refcount of one, and then it gains another by
> giving it a name and entering it into the export list.
> 
> I did know about the second but I completely forgot about the former.
> And indeed I do think it is wrong. qmp_nbd_server_add() does not keep
> the reference to the export, once the function returns, it is gone.
> Therefore, it should call nbd_export_put() before returning.
> 
> So in my opinion the current code is wrong and I didn't fix it enough.
> *cough*
> 
> So, with the nbd_export_put() added to qmp_nbd_server_add(), every
> export will have a refcount of 1 + |clients|. The eject notifier will
> call nbd_close_notifier(), which first disconnects the clients, thus
> reducing the refcount by |clients|, and then sets the name to NULL, thus
> dropping the final refcount and destroying the export.
> 
> In the old code, we needed another nbd_export_put() because of the
> superfluous reference from nbd_export_new(), which doesn't actually
> exist for blockdev-nbd (it does for qemu-nbd, though).

Okay, that makes sense to me. So first a patch that moves the existing
nbd_export_put() call from nbd_close_notifier() to qmp_nbd_server_add()
and then this one on top?

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v7 14/24] nbd: Switch from close to eject notifier

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3444 bytes --]

On 01.12.2015 14:16, Kevin Wolf wrote:
> Am 30.11.2015 um 18:22 hat Max Reitz geschrieben:
>> On 30.11.2015 16:36, Kevin Wolf wrote:
>>> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
>>>> The NBD code uses the BDS close notifier to determine when a medium is
>>>> ejected. However, now it should use the BB's BDS removal notifier for
>>>> that instead of the BDS's close notifier.
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>  blockdev-nbd.c | 37 +------------------------------------
>>>>  nbd.c          | 13 +++++++++++++
>>>>  2 files changed, 14 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
>>>> index bcdd18b..b28a55b 100644
>>>> --- a/blockdev-nbd.c
>>>> +++ b/blockdev-nbd.c
>>>> @@ -46,37 +46,11 @@ void qmp_nbd_server_start(SocketAddress *addr, Error **errp)
>>>>      }
>>>>  }
>>>>  
>>>> -/*
>>>> - * Hook into the BlockBackend notifiers to close the export when the
>>>> - * backend is closed.
>>>> - */
>>>> -typedef struct NBDCloseNotifier {
>>>> -    Notifier n;
>>>> -    NBDExport *exp;
>>>> -    QTAILQ_ENTRY(NBDCloseNotifier) next;
>>>> -} NBDCloseNotifier;
>>>> -
>>>> -static QTAILQ_HEAD(, NBDCloseNotifier) close_notifiers =
>>>> -    QTAILQ_HEAD_INITIALIZER(close_notifiers);
>>>> -
>>>> -static void nbd_close_notifier(Notifier *n, void *data)
>>>> -{
>>>> -    NBDCloseNotifier *cn = DO_UPCAST(NBDCloseNotifier, n, n);
>>>> -
>>>> -    notifier_remove(&cn->n);
>>>> -    QTAILQ_REMOVE(&close_notifiers, cn, next);
>>>> -
>>>> -    nbd_export_close(cn->exp);
>>>> -    nbd_export_put(cn->exp);
>>>
>>> Here you remove a close/put pair, but in the new code you only add the
>>> close call. Why is this correct?
>>
>> Thanks for putting so much unfounded faith in me. :-)
>>
>> After having put quite some time into looking into it, first I have to
>> say that it's a very good question.
>>
>> First off, it's wrong. This is because I thought every export would have
>> a refcount of one. Turns out this is wrong, they have a refcount of two:
>> It is created with a refcount of one, and then it gains another by
>> giving it a name and entering it into the export list.
>>
>> I did know about the second but I completely forgot about the former.
>> And indeed I do think it is wrong. qmp_nbd_server_add() does not keep
>> the reference to the export, once the function returns, it is gone.
>> Therefore, it should call nbd_export_put() before returning.
>>
>> So in my opinion the current code is wrong and I didn't fix it enough.
>> *cough*
>>
>> So, with the nbd_export_put() added to qmp_nbd_server_add(), every
>> export will have a refcount of 1 + |clients|. The eject notifier will
>> call nbd_close_notifier(), which first disconnects the clients, thus
>> reducing the refcount by |clients|, and then sets the name to NULL, thus
>> dropping the final refcount and destroying the export.
>>
>> In the old code, we needed another nbd_export_put() because of the
>> superfluous reference from nbd_export_new(), which doesn't actually
>> exist for blockdev-nbd (it does for qemu-nbd, though).
> 
> Okay, that makes sense to me. So first a patch that moves the existing
> nbd_export_put() call from nbd_close_notifier() to qmp_nbd_server_add()
> and then this one on top?

I would have put both in the same patch, but this does make more sense.
I'll do so.

Max


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