[Qemu-devel] [PATCH v5 00/13] block: Rework bdrv_close_all()

[Qemu-devel] [PATCH v5 00/13] 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.

The benefit over the approach taken in v1 and v2 is that in device
models we often cannot simply drop the reference to a BB because there
may be some user which we forgot about. By ejecting the BDS trees from
the BB, the BB itself becomes unusable, but in a clean way (it will
return errors when accessed, but nothing will crash). Also, it is much
simpler (no reference tracking necessary).

The only disadvantage (I can see) is that the BBs are leaked; but this
does not matter because the qemu process is about to exit anyway.


This series depends on v2 of my series
"blockdev: BlockBackend and media" (or any later version), and on v2 of
my patch "virtio-scsi: Allocate op blocker reason before blocking".
(actually, I rebased it on my local v3 of the "BB and media" series, but
I did not send that one out yet, so I'm writing "v2" for the time being)


v5:
- Patch 5: Because of above mentioned virtio-scsi patch, the helper
  functions for setting up and destroying op blockers on a virtio-scsi
  device are no longer necessary because s->blocker is allocated all the
  time.
- Patch 9: See justification below. [Kevin]
- Patch 10 (was patch 9 in v4): Dropped the modification of
  bdrv_close_all() (moved to patch 12)
- Patch 11 (split off patch 10 from v4): Moved the modification of
  bdrv_close_all() to patch 12 (just like for patch 10)
- Patch 12 (Heavy modification and extension of a part of patch 10 from
  v4): If we don't force-bdrv_close() all BDSs anymore, there may be
  block jobs running; so we have to cancel all block jobs manually.
  However, in order to do that effectively and in a simple way, I want
  the list of BDSs to be completely empty afterwards. For that to
  happen, however, we need to eject all BDSs from the BBs and drop the
  monitor references to bare BDSs first. Therefore, all these changes
  have to be in a single patch (because they depend on each other), and
  that patch is this patch. [Kevin]


Justification for patch 9:

Adding yet another list of BlockDriverStates is ugly, I know. But we
need something to track all the block jobs, and these are the
alternatives:

A. Have a list of all block jobs. Feasible, but this would involve a bit
   more code changes.
B. Only consider the BDSs block jobs may run on; these are the root BDSs
   (bdrv_states) and the named BDSs (graph_bdrv_states). Is a bit uglier
   in that we need to iterate through both lists, but the benefit of
   this approach is that we don't have to add yet another BDS list.
   Also, this approach is a bit faster because we don't have to iterate
   through all the BDS left open, but only through the ones which might
   have block jobs running on them.
   However, the way patch 12 is implemented, bdrv_close_all() (which
   isn't performance-critical anyway...) only iterates through the list
   of all BDSs once the references from the BBs and the monitor are
   gone, so the only BDSs left are the ones on which block jobs are
   running, and the ones which are referenced by those (recursively).
   Therefore, we will not have a performance problem here.
C. Have a list of all BDSs. This is mostly ugly because it means having
   yet another list of BDSs, but other than that, it's okay.

So all these alternatives seem pretty equal, with A maybe being the most
efficient one.

Why did I go for C? As Kevin said, we probably want to have a way to
assert that all BDSs are closed at the end of bdrv_close_all(). Besides
adding a counter for the number of BDSs (which would have been as simple
as it is ugly), this is the only way (I see) to do it. So we get that
for free.

But after this series, there are four lists for BDSs: bdrv_states,
graph_bdrv_states, all_bdrv_states, and monitor_bdrv_states. Urgh.

But the follow-up series to this series removes bdrv_states because they
are covered by the list of BlockBackends. Maybe I could have repurposed
that list to be what all_bdrv_states is right now, but I would find that
confusing. So we're down to three.

And finally, there have been plans for quite a while to give node-names
to all BDSs; this would make graph_bdrv_states and all_bdrv_states just
the same. So with that, we'd be down to two.

So, because using option C allows us to make sure there are no BDSs left
open at the end of bdrv_close_all(), because it's just as good as
solution A and B from a technical perspective, and because it is not
that ugly after all, I chose to go for that.


git-backport-diff against v4:

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/13:[----] [--] 'iotests: Move _filter_nbd into common.filter'
002/13:[----] [--] 'iotests: Make redirecting qemu's stderr optional'
003/13:[----] [--] 'iotests: Add test for eject under NBD server'
004/13:[----] [--] 'quorum: Fix close path'
005/13:[0032] [FC] 'block: Move BDS close notifiers into BB'
006/13:[----] [-C] 'block: Use blk_remove_bs() in blk_delete()'
007/13:[----] [-C] 'blockdev: Use blk_remove_bs() in do_drive_del()'
008/13:[----] [--] 'block: Make bdrv_close() static'
009/13:[down] 'block: Add list of all BlockDriverStates'
010/13:[0003] [FC] 'blockdev: Keep track of monitor-owned BDS'
011/13:[down] 'block: Add blk_remove_all_bs()'
012/13:[down] 'block: Rewrite bdrv_close_all()'
013/13:[----] [-C] 'iotests: Add test for multiple BB on BDS tree'

*** BLURB HERE ***

Max Reitz (13):
  iotests: Move _filter_nbd into common.filter
  iotests: Make redirecting qemu's stderr optional
  iotests: Add test for eject under NBD server
  quorum: Fix close path
  block: Move BDS close notifiers into BB
  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

 block.c                                | 49 ++++++++++++------
 block/block-backend.c                  | 41 ++++++++++++----
 block/quorum.c                         |  3 +-
 blockdev-nbd.c                         | 36 +-------------
 blockdev.c                             | 24 +++++++--
 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/096                 | 90 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/096.out             | 16 ++++++
 tests/qemu-iotests/117                 | 86 ++++++++++++++++++++++++++++++++
 tests/qemu-iotests/117.out             | 14 ++++++
 tests/qemu-iotests/common.filter       | 12 +++++
 tests/qemu-iotests/common.qemu         | 12 ++++-
 tests/qemu-iotests/group               |  2 +
 23 files changed, 474 insertions(+), 113 deletions(-)
 create mode 100644 stubs/blockdev-close-all-bdrv-states.c
 create mode 100755 tests/qemu-iotests/096
 create mode 100644 tests/qemu-iotests/096.out
 create mode 100755 tests/qemu-iotests/117
 create mode 100644 tests/qemu-iotests/117.out

-- 
2.1.0

[Qemu-devel] [PATCH v5 01/13] 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, and it should support URLs of the "nbd://"
format and export names.

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           | 13 +------------
 tests/qemu-iotests/083.out       | 10 ----------
 tests/qemu-iotests/common.filter | 12 ++++++++++++
 3 files changed, 13 insertions(+), 22 deletions(-)

diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083
index 1b2d3f1..aa99278 100755
--- a/tests/qemu-iotests/083
+++ b/tests/qemu-iotests/083
@@ -49,17 +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
@@ -84,7 +73,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
 }
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 012a812..99a1fc2 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -225,5 +225,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 '/nbd\.c:/d' \
+        -e 's#nbd:\(//\)\?127\.0\.0\.1:[0-9]*#nbd:\1127.0.0.1:PORT#g' \
+        -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#'
+}
+
 # make sure this script returns success
 true
-- 
2.1.0

[Qemu-devel] [PATCH v5 02/13] 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>
---
 tests/qemu-iotests/common.qemu | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
index 4e1996c..1b5a554 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -131,6 +131,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.
 #
@@ -153,10 +155,18 @@ function _launch_qemu()
     mkfifo "${fifo_out}"
     mkfifo "${fifo_in}"
 
-    "${QEMU}" -nographic -serial none ${comm} -machine accel=qtest "${@}" \
+    if [ -z "$keep_stderr" ]; then
+        "${QEMU}" -nographic -serial none ${comm} -machine accel=qtest "${@}" \
                                                                 >"${fifo_out}" \
                                                                 2>&1 \
                                                                 <"${fifo_in}" &
+    elif [ "$keep_stderr" = "y" ]; then
+        "${QEMU}" -nographic -serial none ${comm} -machine accel=qtest "${@}" \
+                                                                >"${fifo_out}" \
+                                                                <"${fifo_in}" &
+    else
+        exit 1
+    fi
     QEMU_PID[${_QEMU_HANDLE}]=$!
 
     if [[ "${BASH_VERSINFO[0]}" -ge "5" ||
-- 
2.1.0

[Qemu-devel] [PATCH v5 03/13] 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/096     | 90 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/096.out | 16 +++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 107 insertions(+)
 create mode 100755 tests/qemu-iotests/096
 create mode 100644 tests/qemu-iotests/096.out

diff --git a/tests/qemu-iotests/096 b/tests/qemu-iotests/096
new file mode 100755
index 0000000..bcba0ec
--- /dev/null
+++ b/tests/qemu-iotests/096
@@ -0,0 +1,90 @@
+#!/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
+}
+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': 'inet',
+                                'data': { 'host': '127.0.0.1',
+                                          'port': '10809' }}}}" \
+    '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://127.0.0.1:10809/drv' 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://127.0.0.1:10809/drv' 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/096.out b/tests/qemu-iotests/096.out
new file mode 100644
index 0000000..cc10e51
--- /dev/null
+++ b/tests/qemu-iotests/096.out
@@ -0,0 +1,16 @@
+QA output created by 096
+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://127.0.0.1:PORT/drv: 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 b24502b..8da0e7f 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -102,6 +102,7 @@
 093 auto
 094 rw auto quick
 095 rw auto quick
+096 rw auto quick
 097 rw auto backing
 098 rw auto backing quick
 099 rw auto quick
-- 
2.1.0

[Qemu-devel] [PATCH v5 04/13] quorum: Fix close path

[Max Reitz]

bdrv_unref() can lead to bdrv_close(), which in turn will result in
bdrv_drain_all(). This function will later be called blk_drain_all() and
iterate only over the BlockBackends for which blk_is_inserted() holds
true; therefore, bdrv_is_inserted() and thus quorum_is_inserted() will
probably be called.

This patch makes quorum_is_inserted() aware of the fact that some
children may have been closed already.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/quorum.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/quorum.c b/block/quorum.c
index 7a75cea..5ae2398 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1005,6 +1005,7 @@ static void quorum_close(BlockDriverState *bs)
 
     for (i = 0; i < s->num_children; i++) {
         bdrv_unref(s->bs[i]);
+        s->bs[i] = NULL;
     }
 
     g_free(s->bs);
@@ -1070,7 +1071,7 @@ static bool quorum_is_inserted(BlockDriverState *bs)
     int i;
 
     for (i = 0; i < s->num_children; i++) {
-        if (!bdrv_is_inserted(s->bs[i])) {
+        if (s->bs[i] && !bdrv_is_inserted(s->bs[i])) {
             return false;
         }
     }
-- 
2.1.0

[Qemu-devel] [PATCH v5 05/13] block: Move BDS close notifiers into BB

[Max Reitz]

The only remaining user of the BDS close notifiers is NBD which uses
them to determine when a BDS tree is being ejected. This patch removes
the BDS-level close notifiers and adds a notifier list to the
BlockBackend structure that is invoked whenever a BDS is removed.

Symmetrically to that, another notifier list is added that is invoked
whenever a BDS is inserted. The dataplane implementations for virtio-blk
and virtio-scsi use both notifier types for setting up and removing op
blockers. This is not only important for setting up the op blockers on
insertion, but also for removing them on ejection since bdrv_delete()
asserts that there are no op blockers set up.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                         |  7 ----
 block/block-backend.c           | 19 +++++++---
 blockdev-nbd.c                  | 36 +------------------
 hw/block/dataplane/virtio-blk.c | 77 +++++++++++++++++++++++++++++++----------
 hw/scsi/virtio-scsi.c           | 59 +++++++++++++++++++++++++++++++
 include/block/block.h           |  1 -
 include/block/block_int.h       |  2 --
 include/hw/virtio/virtio-scsi.h | 10 ++++++
 include/sysemu/block-backend.h  |  3 +-
 nbd.c                           | 13 +++++++
 10 files changed, 159 insertions(+), 68 deletions(-)

diff --git a/block.c b/block.c
index d9af3e7..5e2fde3 100644
--- a/block.c
+++ b/block.c
@@ -371,7 +371,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]);
@@ -381,11 +380,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;
@@ -1898,7 +1892,6 @@ void bdrv_close(BlockDriverState *bs)
     bdrv_drain_all(); /* complete I/O */
     bdrv_flush(bs);
     bdrv_drain_all(); /* in case flush left pending I/O */
-    notifier_list_notify(&bs->close_notifiers, bs);
 
     if (bs->drv) {
         if (bs->backing_hd) {
diff --git a/block/block-backend.c b/block/block-backend.c
index 583a1ec..755262d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -47,6 +47,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 {
@@ -97,6 +99,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;
 }
@@ -320,6 +324,8 @@ void blk_remove_bs(BlockBackend *blk)
         return;
     }
 
+    notifier_list_notify(&blk->remove_bs_notifiers, blk);
+
     blk_update_root_state(blk);
 
     bdrv_unref(blk->bs);
@@ -345,6 +351,8 @@ void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
     }
 
     bs->blk = blk;
+
+    notifier_list_notify(&blk->insert_bs_notifiers, blk);
 }
 
 /*
@@ -1067,11 +1075,14 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
     }
 }
 
-void blk_add_close_notifier(BlockBackend *blk, Notifier *notify)
+void blk_add_remove_bs_notifier(BlockBackend *blk, Notifier *notify)
 {
-    if (blk->bs) {
-        bdrv_add_close_notifier(blk->bs, 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_io_plug(BlockBackend *blk)
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 22e95d1..eb5f9a0 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -47,36 +47,11 @@ void qmp_nbd_server_start(SocketAddress *addr, Error **errp)
     }
 }
 
-/* Hook into the BlockDriverState notifiers to close the export when
- * the file 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");
@@ -108,20 +83,11 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
     exp = nbd_export_new(blk, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY, NULL);
 
     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_handler2(server_fd, NULL, NULL, NULL, NULL);
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index cd41478..057498e 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
@@ -138,6 +140,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,
@@ -194,22 +244,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;
 }
@@ -222,8 +262,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);
     object_unref(OBJECT(s->iothread));
     qemu_bh_delete(s->bh);
     g_free(s);
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 4db3b23..ab0f0fa 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -751,6 +751,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)
 {
@@ -759,6 +775,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;
         }
@@ -781,6 +813,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 ((vdev->guest_features >> VIRTIO_SCSI_F_HOTPLUG) & 1) {
         virtio_scsi_push_event(s, sd,
@@ -791,6 +824,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);
 }
 
@@ -905,6 +961,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/block/block.h b/include/block/block.h
index 63000ea..3c0da3f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -207,7 +207,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 8fcfc29..b2c1d87 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -367,8 +367,6 @@ struct BlockDriverState {
     BlockDriverState *backing_hd;
     BlockDriverState *file;
 
-    NotifierList close_notifiers;
-
     /* Callback before write request is processed */
     NotifierWithReturnList before_write_notifiers;
 
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index c122e7a..53ef829 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -177,6 +177,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;
 
@@ -187,6 +194,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;
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index e721cd1..3725e20 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -158,7 +158,8 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
                                                                   void *),
                                      void (*detach_aio_context)(void *),
                                      void *opaque);
-void blk_add_close_notifier(BlockBackend *blk, Notifier *notify);
+void blk_add_remove_bs_notifier(BlockBackend *blk, Notifier *notify);
+void blk_add_insert_bs_notifier(BlockBackend *blk, Notifier *notify);
 void blk_io_plug(BlockBackend *blk);
 void blk_io_unplug(BlockBackend *blk);
 BlockAcctStats *blk_get_stats(BlockBackend *blk);
diff --git a/nbd.c b/nbd.c
index 71159af..e1796c0 100644
--- a/nbd.c
+++ b/nbd.c
@@ -109,6 +109,8 @@ struct NBDExport {
     QTAILQ_ENTRY(NBDExport) next;
 
     AioContext *ctx;
+
+    Notifier eject_notifier;
 };
 
 static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
@@ -964,6 +966,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 *))
 {
@@ -978,6 +986,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
@@ -1031,6 +1043,7 @@ void nbd_export_close(NBDExport *exp)
     nbd_export_set_name(exp, NULL);
     nbd_export_put(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.1.0

[Qemu-devel] [PATCH v5 06/13] block: Use blk_remove_bs() in blk_delete()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/block-backend.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 755262d..76e6893 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -163,12 +163,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);
     /* Avoid double-remove after blk_hide_on_behalf_of_hmp_drive_del() */
     if (blk->name[0]) {
         QTAILQ_REMOVE(&blk_backends, blk, link);
@@ -324,6 +319,8 @@ void blk_remove_bs(BlockBackend *blk)
         return;
     }
 
+    assert(blk->bs->blk == blk);
+
     notifier_list_notify(&blk->remove_bs_notifiers, blk);
 
     blk_update_root_state(blk);
-- 
2.1.0

[Qemu-devel] [PATCH v5 07/13] blockdev: Use blk_remove_bs() in do_drive_del()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 blockdev.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 56498ea..546acfe 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2224,11 +2224,7 @@ int hmp_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
     }
 
     /* quiesce block driver; prevent further io */
-    bdrv_drain_all();
-    if (bs) {
-        bdrv_flush(bs);
-        bdrv_close(bs);
-    }
+    blk_remove_bs(blk);
 
     /* if we have a device attached to this BlockDriverState
      * then we need to make the drive anonymous until the device
-- 
2.1.0

[Qemu-devel] [PATCH v5 08/13] 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>
---
 block.c               | 4 +++-
 include/block/block.h | 1 -
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 5e2fde3..e84fa0c 100644
--- a/block.c
+++ b/block.c
@@ -104,6 +104,8 @@ static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
 /* 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)
 {
@@ -1882,7 +1884,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 3c0da3f..a755cfa 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -206,7 +206,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.1.0

[Qemu-devel] [PATCH v5 09/13] block: Add list of all BlockDriverStates

[Max Reitz]

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

diff --git a/block.c b/block.c
index e84fa0c..9698b1d 100644
--- a/block.c
+++ b/block.c
@@ -94,6 +94,9 @@ static QTAILQ_HEAD(, BlockDriverState) 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);
 
@@ -379,6 +382,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;
 }
 
@@ -2090,6 +2095,9 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
 
     /* keep the same entry in bdrv_states */
     bs_dest->device_list = bs_src->device_list;
+    /* keep the same entry in all_bdrv_states */
+    bs_dest->bs_list = bs_src->bs_list;
+
     bs_dest->blk = bs_src->blk;
 
     memcpy(bs_dest->op_blockers, bs_src->op_blockers,
@@ -2191,6 +2199,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 b2c1d87..622fbfc 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -399,6 +399,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.1.0

[Qemu-devel] [PATCH v5 10/13] blockdev: Keep track of monitor-owned BDS

[Max Reitz]

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

diff --git a/block.c b/block.c
index 9698b1d..fce9d5d 100644
--- a/block.c
+++ b/block.c
@@ -2097,6 +2097,8 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
     bs_dest->device_list = bs_src->device_list;
     /* keep the same entry in all_bdrv_states */
     bs_dest->bs_list = bs_src->bs_list;
+    /* keep the same entry in the list of monitor-owned BDS */
+    bs_dest->monitor_list = bs_src->monitor_list;
 
     bs_dest->blk = bs_src->blk;
 
diff --git a/blockdev.c b/blockdev.c
index 546acfe..a4283a7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -47,6 +47,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",
@@ -646,6 +649,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)
 {
@@ -3161,6 +3177,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)) {
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 622fbfc..f9968da 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -401,6 +401,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;
 
@@ -624,4 +626,6 @@ bool blk_dev_is_tray_open(BlockBackend *blk);
 bool blk_dev_is_medium_locked(BlockBackend *blk);
 void blk_dev_resize_cb(BlockBackend *blk);
 
+void blockdev_close_all_bdrv_states(void);
+
 #endif /* BLOCK_INT_H */
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 5e347d0..9169a09 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 += chr-baum-init.o
 stub-obj-y += chr-msmouse.o
 stub-obj-y += chr-testdev.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.1.0

[Qemu-devel] [PATCH v5 11/13] 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>
---
 block/block-backend.c          | 13 +++++++++++++
 include/sysemu/block-backend.h |  1 +
 2 files changed, 14 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 76e6893..8eedc52 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -207,6 +207,19 @@ 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);
+        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 3725e20..71f5292 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -67,6 +67,7 @@ BlockBackend *blk_new_open(const char *name, const char *filename,
                            Error **errp);
 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.1.0

[Qemu-devel] [PATCH v5 12/13] 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>
---
 block.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index fce9d5d..fe44bd2 100644
--- a/block.c
+++ b/block.c
@@ -1893,9 +1893,8 @@ static void bdrv_close(BlockDriverState *bs)
 {
     BdrvAioNotifier *ban, *ban_next;
 
-    if (bs->job) {
-        block_job_cancel_sync(bs->job);
-    }
+    assert(!bs->job);
+
     bdrv_drain_all(); /* complete I/O */
     bdrv_flush(bs);
     bdrv_drain_all(); /* in case flush left pending I/O */
@@ -1947,13 +1946,24 @@ static void bdrv_close(BlockDriverState *bs)
 void bdrv_close_all(void)
 {
     BlockDriverState *bs;
+    AioContext *aio_context;
 
-    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
-        AioContext *aio_context = bdrv_get_aio_context(bs);
+    blockdev_close_all_bdrv_states();
+    blk_remove_all_bs();
 
-        aio_context_acquire(aio_context);
-        bdrv_close(bs);
-        aio_context_release(aio_context);
+    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) {
+                block_job_cancel_sync(bs->job);
+                aio_context_release(aio_context);
+                break;
+            }
+            aio_context_release(aio_context);
+        }
+        assert(bs);
     }
 }
 
-- 
2.1.0

[Qemu-devel] [PATCH v5 13/13] 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>
---
 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 8da0e7f..84806fb 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
 121 rw auto
 123 rw auto quick
-- 
2.1.0

Re: [Qemu-devel] [PATCH v5 01/13] iotests: Move _filter_nbd into common.filter

[Fam Zheng]

On Tue, 03/03 15:12, Max Reitz wrote:
> _filter_nbd can be useful for other NBD tests, too, therefore it should
> reside in common.filter, and it should support URLs of the "nbd://"
> format and export names.
> 
> 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           | 13 +------------
>  tests/qemu-iotests/083.out       | 10 ----------
>  tests/qemu-iotests/common.filter | 12 ++++++++++++
>  3 files changed, 13 insertions(+), 22 deletions(-)
> 
> diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083
> index 1b2d3f1..aa99278 100755
> --- a/tests/qemu-iotests/083
> +++ b/tests/qemu-iotests/083
> @@ -49,17 +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
> @@ -84,7 +73,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
>  }
> 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 012a812..99a1fc2 100644
> --- a/tests/qemu-iotests/common.filter
> +++ b/tests/qemu-iotests/common.filter
> @@ -225,5 +225,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 '/nbd\.c:/d' \
> +        -e 's#nbd:\(//\)\?127\.0\.0\.1:[0-9]*#nbd:\1127.0.0.1:PORT#g' \
> +        -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#'
> +}
> +
>  # make sure this script returns success
>  true
> -- 
> 2.1.0
> 

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

Re: [Qemu-devel] [PATCH v5 02/13] iotests: Make redirecting qemu's stderr optional

[Fam Zheng]

On Tue, 03/03 15:13, 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>
> ---
>  tests/qemu-iotests/common.qemu | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
> index 4e1996c..1b5a554 100644
> --- a/tests/qemu-iotests/common.qemu
> +++ b/tests/qemu-iotests/common.qemu
> @@ -131,6 +131,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.
>  #
> @@ -153,10 +155,18 @@ function _launch_qemu()
>      mkfifo "${fifo_out}"
>      mkfifo "${fifo_in}"
>  
> -    "${QEMU}" -nographic -serial none ${comm} -machine accel=qtest "${@}" \
> +    if [ -z "$keep_stderr" ]; then
> +        "${QEMU}" -nographic -serial none ${comm} -machine accel=qtest "${@}" \
>                                                                  >"${fifo_out}" \
>                                                                  2>&1 \
>                                                                  <"${fifo_in}" &
> +    elif [ "$keep_stderr" = "y" ]; then
> +        "${QEMU}" -nographic -serial none ${comm} -machine accel=qtest "${@}" \
> +                                                                >"${fifo_out}" \
> +                                                                <"${fifo_in}" &
> +    else
> +        exit 1
> +    fi
>      QEMU_PID[${_QEMU_HANDLE}]=$!
>  
>      if [[ "${BASH_VERSINFO[0]}" -ge "5" ||
> -- 
> 2.1.0
> 
> 

There is some duplication in both branches but I can live with that:

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

Re: [Qemu-devel] [PATCH v5 03/13] iotests: Add test for eject under NBD server

[Fam Zheng]

On Tue, 03/03 15:13, 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/096     | 90 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/096.out | 16 +++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 107 insertions(+)
>  create mode 100755 tests/qemu-iotests/096
>  create mode 100644 tests/qemu-iotests/096.out
> 
> diff --git a/tests/qemu-iotests/096 b/tests/qemu-iotests/096
> new file mode 100755
> index 0000000..bcba0ec
> --- /dev/null
> +++ b/tests/qemu-iotests/096
> @@ -0,0 +1,90 @@
> +#!/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
> +}
> +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': 'inet',
> +                                'data': { 'host': '127.0.0.1',
> +                                          'port': '10809' }}}}" \

Why not nbd+unix? The port could be unavailable.

Fam

Re: [Qemu-devel] [PATCH v5 04/13] quorum: Fix close path

[Fam Zheng]

On Tue, 03/03 15:13, Max Reitz wrote:
> bdrv_unref() can lead to bdrv_close(), which in turn will result in
> bdrv_drain_all(). This function will later be called blk_drain_all() and
> iterate only over the BlockBackends for which blk_is_inserted() holds
> true; therefore, bdrv_is_inserted() and thus quorum_is_inserted() will
> probably be called.
> 
> This patch makes quorum_is_inserted() aware of the fact that some
> children may have been closed already.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/quorum.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/quorum.c b/block/quorum.c
> index 7a75cea..5ae2398 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -1005,6 +1005,7 @@ static void quorum_close(BlockDriverState *bs)
>  
>      for (i = 0; i < s->num_children; i++) {
>          bdrv_unref(s->bs[i]);
> +        s->bs[i] = NULL;
>      }
>  
>      g_free(s->bs);
> @@ -1070,7 +1071,7 @@ static bool quorum_is_inserted(BlockDriverState *bs)
>      int i;
>  
>      for (i = 0; i < s->num_children; i++) {
> -        if (!bdrv_is_inserted(s->bs[i])) {
> +        if (s->bs[i] && !bdrv_is_inserted(s->bs[i])) {
>              return false;
>          }
>      }
> -- 
> 2.1.0
> 

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

Re: [Qemu-devel] [PATCH v5 00/13] block: Rework bdrv_close_all()

[Kevin Wolf]

Am 03.03.2015 um 21:12 hat Max Reitz geschrieben:
> Justification for patch 9:
> 
> Adding yet another list of BlockDriverStates is ugly, I know. But we
> need something to track all the block jobs, and these are the
> alternatives:
> 
> A. Have a list of all block jobs. Feasible, but this would involve a bit
>    more code changes.

In the long run, I guess this is actually what we'll have to switch to,
because bs->job is something we want to get rid of.

Not necessarily a reason for not taking a simpler approach now, though.

Kevin

Re: [Qemu-devel] [PATCH v5 03/13] iotests: Add test for eject under NBD server

[Max Reitz]

On 2015-03-04 at 01:30, Fam Zheng wrote:
> On Tue, 03/03 15:13, 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/096     | 90 ++++++++++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/096.out | 16 +++++++++
>>   tests/qemu-iotests/group   |  1 +
>>   3 files changed, 107 insertions(+)
>>   create mode 100755 tests/qemu-iotests/096
>>   create mode 100644 tests/qemu-iotests/096.out
>>
>> diff --git a/tests/qemu-iotests/096 b/tests/qemu-iotests/096
>> new file mode 100755
>> index 0000000..bcba0ec
>> --- /dev/null
>> +++ b/tests/qemu-iotests/096
>> @@ -0,0 +1,90 @@
>> +#!/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
>> +}
>> +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': 'inet',
>> +                                'data': { 'host': '127.0.0.1',
>> +                                          'port': '10809' }}}}" \
> Why not nbd+unix? The port could be unavailable.

Running the iotests for NBD does use TCP/IP itself, so I assumed we'd 
just presume that port is available when running them.

However, people may not expect this to happen if they don't run the 
tests for NBD (this is a file-specific test after all), so nbd+unix 
seems like a very good alternative (maybe we should do that even for the 
"real" NBD tests...?), so I'll (probably) use that in v6. Thanks!

Max

Re: [Qemu-devel] [PATCH v5 05/13] block: Move BDS close notifiers into BB

[Eric Blake]

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

On 03/03/2015 01:13 PM, Max Reitz wrote:
> The only remaining user of the BDS close notifiers is NBD which uses
> them to determine when a BDS tree is being ejected. This patch removes
> the BDS-level close notifiers and adds a notifier list to the
> BlockBackend structure that is invoked whenever a BDS is removed.
> 
> Symmetrically to that, another notifier list is added that is invoked
> whenever a BDS is inserted. The dataplane implementations for virtio-blk
> and virtio-scsi use both notifier types for setting up and removing op
> blockers. This is not only important for setting up the op blockers on
> insertion, but also for removing them on ejection since bdrv_delete()
> asserts that there are no op blockers set up.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c                         |  7 ----
>  block/block-backend.c           | 19 +++++++---
>  blockdev-nbd.c                  | 36 +------------------
>  hw/block/dataplane/virtio-blk.c | 77 +++++++++++++++++++++++++++++++----------
>  hw/scsi/virtio-scsi.c           | 59 +++++++++++++++++++++++++++++++
>  include/block/block.h           |  1 -
>  include/block/block_int.h       |  2 --
>  include/hw/virtio/virtio-scsi.h | 10 ++++++
>  include/sysemu/block-backend.h  |  3 +-
>  nbd.c                           | 13 +++++++
>  10 files changed, 159 insertions(+), 68 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH v5 09/13] block: Add list of all BlockDriverStates

[Eric Blake]

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

On 03/03/2015 01:13 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c                   | 10 ++++++++++
>  include/block/block_int.h |  2 ++
>  2 files changed, 12 insertions(+)
> 

Might be nice to mention in the commit message why it is useful.  But
the code looked good enough for:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH v5 10/13] blockdev: Keep track of monitor-owned BDS

[Eric Blake]

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

On 03/03/2015 01:13 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c                                |  2 ++
>  blockdev.c                             | 18 ++++++++++++++++++
>  include/block/block_int.h              |  4 ++++
>  stubs/Makefile.objs                    |  1 +
>  stubs/blockdev-close-all-bdrv-states.c |  5 +++++
>  5 files changed, 30 insertions(+)
>  create mode 100644 stubs/blockdev-close-all-bdrv-states.c

Again, might be nice for the commit message to document why adding this
is useful, but doesn't affect the code.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH v5 11/13] block: Add blk_remove_all_bs()

[Eric Blake]

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

On 03/03/2015 01:13 PM, Max Reitz wrote:
> 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>
> ---
>  block/block-backend.c          | 13 +++++++++++++
>  include/sysemu/block-backend.h |  1 +
>  2 files changed, 14 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH v5 10/13] blockdev: Keep track of monitor-owned BDS

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/03/2015 01:13 PM, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block.c                                |  2 ++
>>  blockdev.c                             | 18 ++++++++++++++++++
>>  include/block/block_int.h              |  4 ++++
>>  stubs/Makefile.objs                    |  1 +
>>  stubs/blockdev-close-all-bdrv-states.c |  5 +++++
>>  5 files changed, 30 insertions(+)
>>  create mode 100644 stubs/blockdev-close-all-bdrv-states.c
>
> Again, might be nice for the commit message to document why adding this
> is useful, but doesn't affect the code.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Might be nice?  This absolutely needs an explanation, in the code!

Why do we need a separate list of "monitor-owned BDS"?

What makes a BDS "monitor-owned"?

What are the invariants governing relations among bdrv_states,
graph_bdrv_states (what a horrible name) and blk_backends?

Re: [Qemu-devel] [PATCH v5 10/13] blockdev: Keep track of monitor-owned BDS

[Max Reitz]

On 2015-03-20 at 04:04, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 03/03/2015 01:13 PM, Max Reitz wrote:
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>   block.c                                |  2 ++
>>>   blockdev.c                             | 18 ++++++++++++++++++
>>>   include/block/block_int.h              |  4 ++++
>>>   stubs/Makefile.objs                    |  1 +
>>>   stubs/blockdev-close-all-bdrv-states.c |  5 +++++
>>>   5 files changed, 30 insertions(+)
>>>   create mode 100644 stubs/blockdev-close-all-bdrv-states.c
>> Again, might be nice for the commit message to document why adding this
>> is useful, but doesn't affect the code.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
> Might be nice?  This absolutely needs an explanation, in the code!
>
> Why do we need a separate list of "monitor-owned BDS"?

I thought it self-evident, but you're right, of course: The concept 
being that the monitor can hold references to BDS ("owning them", see 
below), of course it should keep track of those references (which I 
thought didn't need an explanation, because if you hold a reference, 
well, you better actually hold one).

> What makes a BDS "monitor-owned"?

Being created explicitly.

BDS created implicitly because a BB was created (-drive, blockdev-add 
with @id; in these cases, the BBs are monitor-owned) or that are not the 
root of a BDS tree (thus created implicitly in that tree) are not 
monitor-owned. All BDS created explicitly (blockdev-add without @id) are 
monitor-owned.

> What are the invariants governing relations among bdrv_states,
> graph_bdrv_states (what a horrible name) and blk_backends?

bdrv_states is removed in the follow-up series. I think it's legacy 
cruft which we needed at a time where we did not have BlockBackends: It 
tracks all the BDSs with a BB, basically (a BDS is inserted there in 
bdrv_new_root() which is only called from blk_new_with_bs()). Therefore, 
we don't need it, as we have a list of all the BBs already.

graph_bdrv_states tracks all BDSs with a node name. We need that because 
we want to find BDSs by node name.

blk_backends, which supersedes bdrv_states, basically, will, after the 
follow-up, contain every single BlockBackend there is. It will be 
different from monitor_block_backends (follow-up...) in that the latter 
only contains the monitor-owned BBs.

Why we need that difference is a question for the follow-up. However, I 
can answer it here, too: We do have some operations that should actually 
be run over all BlockBackends, like blk_name_taken(), blk_drain_all(), 
blk_flush_all(), and so on. However, all the BBs that are not 
monitor-owned should not be visible to the user (the monitor!), so for 
operations like blk_by_name() or qmp_query_blockstats(), only the 
monitor-owned BBs should be considered.

What makes a BB monitor-owned? It's monitor-owned if it has been created 
using -drive or blockdev-add. Are there any other BBs right now? Except 
for qemu-{img,io,nbd}, there is only xen_disk.c, so practically, no.

What makes a BB monitor-unowned? Deleting its reference through a 
monitor command while there are still other users (in theory that would 
be NBD, block jobs, etc., although I don't know whether that actually 
works that way right now). So it is probably possible to have 
non-monitor-owned BBs, but those should not be visible to the user. 
However, we need to consider them inside of the block layer whenever we 
want to do something for actually all the BBs.

Side note: As far as I know, we currently only have drive_del to drop 
the monitor reference to a BB. What happens there before the follow-up 
is the BB is made anonymous and removed from blk_backends 
(blk_hide_on_behalf_of_hmp_drive_del()) and the root BDS from 
bdrv_states (it's really redundant), so before that series, blk_backends 
is basically the list of monitor-owned BBs (which I don't think is 
right, it should contain all BBs, and we need a separate list for the 
monitor-owned ones).

I hope I was able to explain myself.

Max