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

[Qemu-devel] [PATCH v6 for-2.6 00/15] 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.


v5 is here (yes, it has been a while):
http://lists.nongnu.org/archive/html/qemu-block/2015-03/msg00044.html


v6:
- Patch 1: Added (leakage of a BDS in a drive-backup error case, becomes
  apparent with this series)
- Patch 2: Added (leakage of a BDS when a block job failed to be
  created)
- Patch 3: Added (we hacked our way around this so far, but now we have
  to fix it, see the commit message)
- Patch 5: Rebase conflicts
- Patch 6: Renamed test from 096 to 140 (rebase conflict)
- Patch 7:
  - Contextual rebase conflicts
  - Rebase conflict in a comment being removed
  - Dropping the reference to exp->blk has been moved from
    nbd_export_close() to nbd_export_put(), so the notifier_remove()
    call must be moved there, too
- Patch 8: blk_remove_bs() no longer silently ignores blk->bs being
  NULL, therefore we have to keep it wrapped in if (blk->bs)
- Patch 9:
  - The bdrv_drain_all() and bdrv_flush() calls have been removed from
    hmp_drive_del() already, so we don't need to do it anymore.
  - As in patch 8, blk_remove_bs() must stay in the if (blk->bs) block
- Patch 11: Do not move the list entry in all_bdrv_states in
  bdrv_move_feature_fields(); every BDS is part of that list, so there
  is absolutely no point in swapping their positions here
- Patch 12:
  - Do not move the list entry in the list of monitor-owned BDS in
    bdrv_move_feature_fields(); if a BDS is owned by the monitor, that
    will not change by putting another BDS on top of it or by putting it
    on top of another BDS
  - Add test to qmp_x_blockdev_del() whether the BDS to be deleted (if a
    node is to be deleted) is actually monitor-owned
  - If qmp_x_blockdev_del() unrefs a node, it must be dropped from the
    list of monitor-owned BDS
- Patch 13: Enclose blk_remove_bs() in an if (blk->bs) block
  (just like in patches 8 and 9)
- Patch 14:
  - One semi-contextual conflict in which an empty line addition is
    dropped, because now there already is an empty line
  - Changes to bdrv_close_all():
    - Add a bdrv_drain_all() at the beginning; it may not do anything,
      but it looked like a reasonable thing to do to me
    - Add a comment what the loop is for
    - A block job will not immediately release its BDS reference once it
      is canceled (maybe it did back in March, but now it doesn't), so
      we need to call aio_poll() repeatedly until the reference is
      dropped


git-backport-diff against v5:

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/15:[down] 'blockdev: Add missing bdrv_unref() in drive-backup'
002/15:[down] 'blockjob: Call bdrv_unref() on creation error'
003/15:[down] 'block: Release dirty bitmaps in bdrv_close()'
004/15:[----] [--] 'iotests: Move _filter_nbd into common.filter'
005/15:[0009] [FC] 'iotests: Make redirecting qemu's stderr optional'
006/15:[0004] [FC] 'iotests: Add test for eject under NBD server'
007/15:[0007] [FC] 'block: Move BDS close notifiers into BB'
008/15:[0004] [FC] 'block: Use blk_remove_bs() in blk_delete()'
009/15:[0006] [FC] 'blockdev: Use blk_remove_bs() in do_drive_del()'
010/15:[----] [-C] 'block: Make bdrv_close() static'
011/15:[0003] [FC] 'block: Add list of all BlockDriverStates'
012/15:[0009] [FC] 'blockdev: Keep track of monitor-owned BDS'
013/15:[0004] [FC] 'block: Add blk_remove_all_bs()'
014/15:[0025] [FC] 'block: Rewrite bdrv_close_all()'
015/15:[----] [-C] 'iotests: Add test for multiple BB on BDS tree'


Max Reitz (15):
  blockdev: Add missing bdrv_unref() in drive-backup
  blockjob: Call bdrv_unref() on creation error
  block: Release dirty bitmaps in bdrv_close()
  iotests: Move _filter_nbd into common.filter
  iotests: Make redirecting qemu's stderr optional
  iotests: Add test for eject under NBD server
  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                                | 83 ++++++++++++++++++++++++-------
 block/block-backend.c                  | 41 +++++++++++++---
 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                 | 90 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/140.out             | 16 ++++++
 tests/qemu-iotests/common.filter       | 12 +++++
 tests/qemu-iotests/common.qemu         | 15 +++++-
 tests/qemu-iotests/group               |  2 +
 23 files changed, 518 insertions(+), 109 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

-- 
2.6.2

[Qemu-devel] [PATCH v6 01/15] 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>
---
 blockdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/blockdev.c b/blockdev.c
index ac2a537..4eda49c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2899,6 +2899,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 v6 02/15] 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>
---
 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 v6 03/15] 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 | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 3493501..23448ed 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);
 
@@ -3318,6 +3321,21 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
     }
 }
 
+/**
+ * Release all dirty bitmaps attached to a BDS, independently of whether they
+ * are frozen or not (for use in bdrv_close()).
+ */
+static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs)
+{
+    BdrvDirtyBitmap *bm, *next;
+    QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
+        QLIST_REMOVE(bm, list);
+        hbitmap_free(bm->bitmap);
+        g_free(bm->name);
+        g_free(bm);
+    }
+}
+
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
     assert(!bdrv_dirty_bitmap_frozen(bitmap));
-- 
2.6.2

[Qemu-devel] [PATCH v6 04/15] 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 cfdb633..0a270ca 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 '/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.6.2

[Qemu-devel] [PATCH v6 05/15] 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 | 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 v6 06/15] 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     | 90 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/140.out | 16 +++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 107 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..bcba0ec
--- /dev/null
+++ b/tests/qemu-iotests/140
@@ -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/140.out b/tests/qemu-iotests/140.out
new file mode 100644
index 0000000..d98a660
--- /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://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 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 v6 07/15] 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                  | 37 +-------------------
 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(+), 69 deletions(-)

diff --git a/block.c b/block.c
index 23448ed..067bc39 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 6f9309f..38580f7 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;
 }
@@ -343,6 +347,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 +365,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,11 +1113,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 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/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);
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/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 6a3f64d..4e0ed15 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/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;
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index f4a68e2..abb52be 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -162,7 +162,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 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 v6 08/15] block: Use blk_remove_bs() in blk_delete()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@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 38580f7..5702cc1 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);
     }
     if (blk->root_state.throttle_state) {
         g_free(blk->root_state.throttle_group);
@@ -347,6 +344,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 v6 09/15] blockdev: Use blk_remove_bs() in do_drive_del()

[Max Reitz]

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

diff --git a/blockdev.c b/blockdev.c
index 4eda49c..a97a881 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2494,7 +2494,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 v6 10/15] 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 067bc39..57e4b15 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 v6 11/15] block: Add list of all BlockDriverStates

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@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 57e4b15..cf5dadc 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 4e0ed15..bde93e3 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -419,6 +419,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 v6 12/15] blockdev: Keep track of monitor-owned BDS

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@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 a97a881..a5ce8c2 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)
 {
@@ -3449,6 +3465,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)) {
@@ -3519,11 +3537,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 bde93e3..5b7c47d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -421,6 +421,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;
 
@@ -677,4 +679,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 251443b..5fb489e 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 v6 13/15] 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          | 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 5702cc1..a63a83a 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -219,6 +219,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 v6 14/15] 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 | 45 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 37 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index cf5dadc..95a7f98 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->io_limits_enabled) {
@@ -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 v6 15/15] 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 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

Re: [Qemu-devel] [PATCH v6 14/15] block: Rewrite bdrv_close_all()

[Paolo Bonzini]

On 04/11/2015 19:57, Max Reitz wrote:
>      BdrvAioNotifier *ban, *ban_next;
>  
> -    if (bs->job) {
> -        block_job_cancel_sync(bs->job);
> -    }
> +    assert(!bs->job);
>  

Who does this when ejecting a BDS from a BB?

Paolo

Re: [Qemu-devel] [PATCH v6 14/15] block: Rewrite bdrv_close_all()

[Max Reitz]

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

On 05.11.2015 18:15, Paolo Bonzini wrote:
> 
> 
> On 04/11/2015 19:57, Max Reitz wrote:
>>      BdrvAioNotifier *ban, *ban_next;
>>  
>> -    if (bs->job) {
>> -        block_job_cancel_sync(bs->job);
>> -    }
>> +    assert(!bs->job);
>>  
> 
> Who does this when ejecting a BDS from a BB?

Nobody, which one can see as good or bad, depending on whether you liked
it or not (it's mostly bad, though).

So by removing this without replacement, block jobs continue to work on
their BDS (and own it) until they're completed.

One stance you can take is: Block jobs work on the BDS, so ejecting the
BDS from the BB should not have an impact anyway (the BB name is only
used as a shorthand for identifying the BDS).

Another stance you can take is: The user only sees that the block job
takes a device name, so it looks like it's working on the BB level and
should behave as such.

The first point becomes moot when one realizes that you can no longer
complete a running block job once its BDS has been detached from the BB,
since the block job commands take BB names.

However, let's just try it:

$ x86_64-softmmu/qemu-system-x86_64 -qmp stdio \
  -drive if=none,file=test.qcow2,id=drive0,node-name=node0
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2},
"package": ""}, "capabilities": []}}
{'execute':'qmp_capabilities'}
{"return": {}}
{'execute':'drive-backup','arguments':{'device':'drive0','target':'out.qcow2','format':'qcow2','sync':'none'}}
Formatting 'out.qcow2', fmt=qcow2 size=67108864 backing_file=test.qcow2
backing_fmt=qcow2 encryption=off cluster_size=65536 lazy_refcounts=off
refcount_bits=16
{"return": {}}
{'execute':'eject','arguments':{'device':'drive0'}}
{"error": {"class": "GenericError", "desc": "Node 'drive0' is busy:
block device is in use by block job: backup"}}

So... Nobody does it, but apparently nobody has to because you cannot
eject (to be more specific: use blockdev-remove-medium) a BDS from a BB
while a block job is running on the BDS.

Max


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

Re: [Qemu-devel] [PATCH v6 14/15] block: Rewrite bdrv_close_all()

[Paolo Bonzini]

On 05/11/2015 18:37, Max Reitz wrote:
> $ x86_64-softmmu/qemu-system-x86_64 -qmp stdio \ -drive
> if=none,file=test.qcow2,id=drive0,node-name=node0 {"QMP":
> {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, 
> "package": ""}, "capabilities": []}} 
> {'execute':'qmp_capabilities'} {"return": {}} 
> {'execute':'drive-backup','arguments':{'device':'drive0','target':'out.qcow2','format':'qcow2','sync':'none'}}
>
> 
Formatting 'out.qcow2', fmt=qcow2 size=67108864 backing_file=test.qcow2
> backing_fmt=qcow2 encryption=off cluster_size=65536
> lazy_refcounts=off refcount_bits=16 {"return": {}} 
> {'execute':'eject','arguments':{'device':'drive0'}} {"error":
> {"class": "GenericError", "desc": "Node 'drive0' is busy: block
> device is in use by block job: backup"}}
> 
> So... Nobody does it, but apparently nobody has to because you
> cannot eject (to be more specific: use blockdev-remove-medium) a
> BDS from a BB while a block job is running on the BDS.

If you test it with all jobs, then it's okay.  It's a regression, but
not introduced by your patch and apparently nobody noticed.

Even if nobody noticed, I wonder if this "Node 'foo' is busy" kind of
error deserves its own ErrorClass.  Eric, what do you think?

Paolo

Re: [Qemu-devel] [PATCH v6 14/15] block: Rewrite bdrv_close_all()

[Eric Blake]

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

On 11/05/2015 10:40 AM, Paolo Bonzini wrote:
> 
> 
> On 05/11/2015 18:37, Max Reitz wrote:
>> $ x86_64-softmmu/qemu-system-x86_64 -qmp stdio \ -drive
>> if=none,file=test.qcow2,id=drive0,node-name=node0 {"QMP":
>> {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, 
>> "package": ""}, "capabilities": []}} 
>> {'execute':'qmp_capabilities'} {"return": {}} 
>> {'execute':'drive-backup','arguments':{'device':'drive0','target':'out.qcow2','format':'qcow2','sync':'none'}}
>>
>>
> Formatting 'out.qcow2', fmt=qcow2 size=67108864 backing_file=test.qcow2
>> backing_fmt=qcow2 encryption=off cluster_size=65536
>> lazy_refcounts=off refcount_bits=16 {"return": {}} 
>> {'execute':'eject','arguments':{'device':'drive0'}} {"error":
>> {"class": "GenericError", "desc": "Node 'drive0' is busy: block
>> device is in use by block job: backup"}}
>>
>> So... Nobody does it, but apparently nobody has to because you
>> cannot eject (to be more specific: use blockdev-remove-medium) a
>> BDS from a BB while a block job is running on the BDS.
> 
> If you test it with all jobs, then it's okay.  It's a regression, but
> not introduced by your patch and apparently nobody noticed.
> 
> Even if nobody noticed, I wonder if this "Node 'foo' is busy" kind of
> error deserves its own ErrorClass.  Eric, what do you think?

Needing a unique ErrorClass is only important if we expect a client
(libvirt) would behave differently based on that error class (clients
are not allowed to parse the error message).  But what is the scenario
that we are trying to test here, rewritten in terms of libvirt API
commands?  Should libvirt behave any differently because a blockjob was
running than for any other failure, if the end result is still that
libvirt can't eject or hot-unplug the disk because of a failure?

-- 
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 v6 14/15] block: Rewrite bdrv_close_all()

[Paolo Bonzini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 05/11/2015 18:44, Eric Blake wrote:
>>> If you test it with all jobs, then it's okay.  It's a
>>> regression, but not introduced by your patch and apparently
>>> nobody noticed.
>>> 
>>> Even if nobody noticed, I wonder if this "Node 'foo' is busy"
>>> kind of error deserves its own ErrorClass.  Eric, what do you
>>> think?
> Needing a unique ErrorClass is only important if we expect a
> client (libvirt) would behave differently based on that error class
> (clients are not allowed to parse the error message).  But what is
> the scenario that we are trying to test here, rewritten in terms of
> libvirt API commands?  Should libvirt behave any differently
> because a blockjob was running than for any other failure, if the
> end result is still that libvirt can't eject or hot-unplug the disk
> because of a failure?

It may want to cancel the job and redo the operation.  Or it may
trigger an assertion failure.  I don't know...  that's why I asked. :)

Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJWO5e4AAoJEL/70l94x66DbSQH/31+AI5zFN9UtbQCMgzEKQfA
EYm2gqZOQtOyaRRQI1VKOzekKTy60Y1Z1iT84PrZz7pI3PhG/qoEGG5aOeKxqjc8
tkl0DxYd4y1Mhf2Hgm4bNcswcEx5wshy0hIbqFQJUVokE0e7bx297ePw5zoTU1uY
HOI0298gEHV7DA0Ux4koMi+88rIA5oPAWf3Hlxpf2A4152KXrVyh24ErELCkClCR
p5EVy0urZgwscpm38GK+a2xXq8IQXRYbJZbnTxGaCLY4TAvuaEWhJ90B0mhvnNch
GFKQPHMfrtR7N0b31hX4Ok2sRUKH/0/kKrjp/NpFxohNL0Rp9XS5JvQuGe+i3+s=
=bl13
-----END PGP SIGNATURE-----

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

[John Snow]

On 11/04/2015 01:57 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 | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 3493501..23448ed 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);
>  
> @@ -3318,6 +3321,21 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>      }
>  }
>  
> +/**
> + * Release all dirty bitmaps attached to a BDS, independently of whether they
> + * are frozen or not (for use in bdrv_close()).
> + */

This comment caught my attention ...

> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs)
> +{
> +    BdrvDirtyBitmap *bm, *next;
> +    QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
> +        QLIST_REMOVE(bm, list);
> +        hbitmap_free(bm->bitmap);
> +        g_free(bm->name);
> +        g_free(bm);
> +    }
> +}
> +
>  void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>  {
>      assert(!bdrv_dirty_bitmap_frozen(bitmap));
> 

Currently, the only way a bitmap could/should be frozen is if it is in
use by a job. If a job is running, you probably shouldn't delete stuff
it is using out from under it.

I am assuming by now that it's actually likely that you've canceled any
jobs attached to this node before you go through the motions of deleting
it, and it should be safe to just call bdrv_release_dirty_bitmap ...

We don't want the two foreach loops though, so maybe just factor out a
helper that bdrv_release_all_dirty_bitmaps and bdrv_release_dirty_bitmap
can share. You can leave the frozen assertion
in just bdrv_release_dirty_bitmap before it invokes the helper, so that
bdrv_delete always succeeds in case something gets out-of-sync in the
internals.

(Hmm, to prevent leaks, if you delete a bitmap that is frozen, you
should probably call the helper on its child object too... or just make
the helper recursive.)

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

[Alberto Garcia]

On Wed 04 Nov 2015 07:57:33 PM CET, Max Reitz <mreitz@redhat.com> 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>

Berto

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

[Alberto Garcia]

On Wed 04 Nov 2015 07:57:34 PM CET, 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>

Berto

Re: [Qemu-devel] [PATCH v6 10/15] block: Make bdrv_close() static

[Alberto Garcia]

On Wed 04 Nov 2015 07:57:42 PM CET, 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>

Berto

Re: [Qemu-devel] [PATCH v6 12/15] blockdev: Keep track of monitor-owned BDS

[Alberto Garcia]

On Wed 04 Nov 2015 07:57:44 PM CET, Max Reitz wrote:
> @@ -3519,11 +3537,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);
>      }

blk_unref(blk) will also unref the BDS (if there's any), so you also
need to update monitor_bdrv_states in that case, don't you?

Anyway, wouldn't it make more sense to do this in bdrv_delete() ?

Berto

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

[Kevin Wolf]

Am 04.11.2015 um 19:57 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 1-2, 5-6, 8-15:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>

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

[Kevin Wolf]

Am 04.11.2015 um 19:57 hat Max Reitz geschrieben:
> 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()

This doesn't sound right. If there was a dirty bitmap, there must be a
user associated with it. Now that we simply free the bitmap, that user
has a dangling pointer.

An exception would be if we knew that the only "user" of this bitmap is
the monitor because the monitor doesn't actually maintain its own list
of bitmaps. However, it's doubtful whether bdrv_close() should remove
something that the QMP client added explicitly.

> and drop the assertion in bdrv_delete().

Why? It should still hold true.

Kevin

Re: [Qemu-devel] [PATCH v6 04/15] iotests: Move _filter_nbd into common.filter

[Kevin Wolf]

Am 04.11.2015 um 19:57 hat Max Reitz geschrieben:
> _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>

Code motion and modification in the same patch is bad style. The changes
look good, though.

Kevin

Re: [Qemu-devel] [PATCH v6 07/15] block: Move BDS close notifiers into BB

[Kevin Wolf]

Am 04.11.2015 um 19:57 hat Max Reitz geschrieben:
> 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>

I think this needs to be split into smaller patches:

1. Add the new BlockBackend notifiers
2. Use them in virtio-blk in order to fix... removable virtio-blk
   devices, or what is it?
3. Convert NBD
4. Remove old close notifiers

>  block.c                         |  7 ----
>  block/block-backend.c           | 19 +++++++---
>  blockdev-nbd.c                  | 37 +-------------------
>  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(+), 69 deletions(-)

> diff --git a/block/block-backend.c b/block/block-backend.c
> index 6f9309f..38580f7 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;
>  }
> @@ -343,6 +347,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 +365,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);
>  }

Do we want to notify on BB deletion, too? It's also some kind of removal
of a connection between BB and BDS.  In other words, should blk_delete()
call blk_remove_bs() rather than bdrv_unref()?

[ Edit: I see that's what the next patch does. Good. ]

Should blk_unref() also assert that the notifier list is empty?
Otherwise we would be leaking notifiers.

Kevin

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

[Max Reitz]

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

On 06.11.2015 19:59, John Snow wrote:
> 
> 
> On 11/04/2015 01:57 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 | 20 +++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/block.c b/block.c
>> index 3493501..23448ed 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);
>>  
>> @@ -3318,6 +3321,21 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>>      }
>>  }
>>  
>> +/**
>> + * Release all dirty bitmaps attached to a BDS, independently of whether they
>> + * are frozen or not (for use in bdrv_close()).
>> + */
> 
> This comment caught my attention ...
> 
>> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs)
>> +{
>> +    BdrvDirtyBitmap *bm, *next;
>> +    QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
>> +        QLIST_REMOVE(bm, list);
>> +        hbitmap_free(bm->bitmap);
>> +        g_free(bm->name);
>> +        g_free(bm);
>> +    }
>> +}
>> +
>>  void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>>  {
>>      assert(!bdrv_dirty_bitmap_frozen(bitmap));
>>
> 
> Currently, the only way a bitmap could/should be frozen is if it is in
> use by a job. If a job is running, you probably shouldn't delete stuff
> it is using out from under it.

Right. Good thing I'm fixing everything and bdrv_close() will no longer
be called unless there are no references to the BDS anymore (and each
block job holds an own reference to its BDS). :-)

(At least that's how it's supposed to be; also, there is an
assert(!bs->job) in bdrv_close())

Also, I wanted to mirror current behavior. Right now, we are just
leaking all dirty bitmaps on bdrv_close() (bdrv_delete() asserts that
there are none, but if you invoke bdrv_close() directly...).

> I am assuming by now that it's actually likely that you've canceled any
> jobs attached to this node before you go through the motions of deleting
> it, and it should be safe to just call bdrv_release_dirty_bitmap ...

Well, yes, but then I'd have to iterate through all the dirty bitmaps to
call bdrv_release_dirty_bitmap() for each of them, and then
bdrv_release_dirty_bitmap() iterates through all of them again to check
whether it really belongs to the BDS. That seemed a bit like a waste.

> We don't want the two foreach loops though, so maybe just factor out a
> helper that bdrv_release_all_dirty_bitmaps and bdrv_release_dirty_bitmap
> can share. You can leave the frozen assertion
> in just bdrv_release_dirty_bitmap before it invokes the helper, so that
> bdrv_delete always succeeds in case something gets out-of-sync in the
> internals.

Hm, yes, that should do just fine, thanks.

> (Hmm, to prevent leaks, if you delete a bitmap that is frozen, you
> should probably call the helper on its child object too... or just make
> the helper recursive.)

I think it'll be actually better to just keep the assertion in and thus
not allow releasing frozen dirty bitmaps in bdrv_close(). In addition
I'll add an assertion that !bs->refcount to bdrv_close().

Max


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

Re: [Qemu-devel] [PATCH v6 12/15] blockdev: Keep track of monitor-owned BDS

[Kevin Wolf]

Am 09.11.2015 um 16:05 hat Alberto Garcia geschrieben:
> On Wed 04 Nov 2015 07:57:44 PM CET, Max Reitz wrote:
> > @@ -3519,11 +3537,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);
> >      }
> 
> blk_unref(blk) will also unref the BDS (if there's any), so you also
> need to update monitor_bdrv_states in that case, don't you?

No, in that case the BDS referenced wasn't owned by the monitor in the
first place. It was owned by the BB.

Kevin

Re: [Qemu-devel] [PATCH v6 12/15] blockdev: Keep track of monitor-owned BDS

[Max Reitz]

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

On 09.11.2015 16:05, Alberto Garcia wrote:
> On Wed 04 Nov 2015 07:57:44 PM CET, Max Reitz wrote:
>> @@ -3519,11 +3537,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);
>>      }
> 
> blk_unref(blk) will also unref the BDS (if there's any), so you also
> need to update monitor_bdrv_states in that case, don't you?

If we get to blk_unref(blk), that means there is a BB attached to that
BDS (if there is any). If that BDS is monitor-owned, that means its
refcount is at least 2 (one from the BB, one from the monitor).
Therefore, blockdev-del will fail before that point.

> Anyway, wouldn't it make more sense to do this in bdrv_delete() ?

No, I don't think so. The monitor_bdrv_states is not a plain list of
BDSs, but actually contains strong references. Every time a BDS is
entered there, that reference should be counted (which we don't actually
have to call bdrv_ref() for since we're just keeping the initial
refcount of 1 in qmp_blockdev_add()), and every time a BDS is removed,
its reference should be dropped, just like is done here.

That's the issue I had with this implementation of blockdev-del. It's
just dropping references without actually having those references. Now
we at least have the reference through monitor_bdrv_states, and in the
future, we'll have such a list of monitor references for BBs, too.

Max


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

Re: [Qemu-devel] [PATCH v6 12/15] blockdev: Keep track of monitor-owned BDS

[Alberto Garcia]

On Mon 09 Nov 2015 05:26:38 PM CET, Kevin Wolf wrote:

>> >      if (blk) {
>> >          blk_unref(blk);
>> >      } else {
>> > +        QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list);
>> >          bdrv_unref(bs);
>> >      }
>> 
>> blk_unref(blk) will also unref the BDS (if there's any), so you also
>> need to update monitor_bdrv_states in that case, don't you?
>
> No, in that case the BDS referenced wasn't owned by the monitor in the
> first place. It was owned by the BB.

I see, I hadn't noticed that the BDS is added to monitor_bdrv_states
only if it is created without a BB.

You're right then, thanks!

Berto

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

[Max Reitz]

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

On 09.11.2015 17:04, Kevin Wolf wrote:
> Am 04.11.2015 um 19:57 hat Max Reitz geschrieben:
>> 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()
> 
> This doesn't sound right. If there was a dirty bitmap, there must be a
> user associated with it. Now that we simply free the bitmap, that user
> has a dangling pointer.

Well, having an assertion there means that we already assumed that case
to be impossible. Even though it isn't, as you yourself describe:

> An exception would be if we knew that the only "user" of this bitmap is
> the monitor because the monitor doesn't actually maintain its own list
> of bitmaps. However, it's doubtful whether bdrv_close() should remove
> something that the QMP client added explicitly.

So you are proposing that bdrv_close() should fail if there are still
dirty bitmaps attached? I don't like that either.

The bitmaps are attached to the BDS, that much is exposed over QMP, too.
If the BDS is released it's only natural to assume that all its bitmaps
are released, too. If you don't want that, you need to make sure that
the monitor has a reference to the BDS itself so the user can defer the
call to blockdev-del until he's/she's ready.

Maybe we need some QMP command to fetch a reference for the monitor for
that to be more usable, I don't know. It will work with blockdev-add
alone, too, though.

>> and drop the assertion in bdrv_delete().
> 
> Why? It should still hold true.

But it does not for user-added bitmaps.

Actually, on master, you can't break that assertion by adding a bitmap
through QMP, but with the BB and media series, you can. And that's
because as of that series, eject will no longer force-call bdrv_close()
(which bypasses the assertion althogether) but bdrv_unref() instead
(leading to bdrv_delete(), and that gets you the assertion).

I'm not really keen on fixing that in that series, though, since I
consider leaking not much better than a failed assertion, especially
considering I'm trying to actually fix it here anyway.

Max


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

Re: [Qemu-devel] [PATCH v6 07/15] block: Move BDS close notifiers into BB

[Max Reitz]

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

On 09.11.2015 17:04, Kevin Wolf wrote:
> Am 04.11.2015 um 19:57 hat Max Reitz geschrieben:
>> 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>
> 
> I think this needs to be split into smaller patches:
> 
> 1. Add the new BlockBackend notifiers
> 2. Use them in virtio-blk in order to fix... removable virtio-blk
>    devices, or what is it?
> 3. Convert NBD
> 4. Remove old close notifiers

I'll do my best.

>>  block.c                         |  7 ----
>>  block/block-backend.c           | 19 +++++++---
>>  blockdev-nbd.c                  | 37 +-------------------
>>  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(+), 69 deletions(-)
> 
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 6f9309f..38580f7 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;
>>  }
>> @@ -343,6 +347,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 +365,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);
>>  }
> 
> Do we want to notify on BB deletion, too? It's also some kind of removal
> of a connection between BB and BDS.  In other words, should blk_delete()
> call blk_remove_bs() rather than bdrv_unref()?
> 
> [ Edit: I see that's what the next patch does. Good. ]
> 
> Should blk_unref() also assert that the notifier list is empty?
> Otherwise we would be leaking notifiers.

You mean blk_delete()? I can do that, yes.

Max


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

Re: [Qemu-devel] [PATCH v6 07/15] block: Move BDS close notifiers into BB

[Kevin Wolf]

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

Am 09.11.2015 um 17:50 hat Max Reitz geschrieben:
> On 09.11.2015 17:04, Kevin Wolf wrote:
> > Am 04.11.2015 um 19:57 hat Max Reitz geschrieben:
> >> 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>
> > 
> > I think this needs to be split into smaller patches:
> > 
> > 1. Add the new BlockBackend notifiers
> > 2. Use them in virtio-blk in order to fix... removable virtio-blk
> >    devices, or what is it?
> > 3. Convert NBD
> > 4. Remove old close notifiers
> 
> I'll do my best.
> 
> >>  block.c                         |  7 ----
> >>  block/block-backend.c           | 19 +++++++---
> >>  blockdev-nbd.c                  | 37 +-------------------
> >>  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(+), 69 deletions(-)
> > 
> >> diff --git a/block/block-backend.c b/block/block-backend.c
> >> index 6f9309f..38580f7 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;
> >>  }
> >> @@ -343,6 +347,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 +365,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);
> >>  }
> > 
> > Do we want to notify on BB deletion, too? It's also some kind of removal
> > of a connection between BB and BDS.  In other words, should blk_delete()
> > call blk_remove_bs() rather than bdrv_unref()?
> > 
> > [ Edit: I see that's what the next patch does. Good. ]
> > 
> > Should blk_unref() also assert that the notifier list is empty?
> > Otherwise we would be leaking notifiers.
> 
> You mean blk_delete()? I can do that, yes.

Yes, sorry, that's what I meant.

Kevin

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

Re: [Qemu-devel] [PATCH v6 04/15] iotests: Move _filter_nbd into common.filter

[Max Reitz]

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

On 09.11.2015 17:04, Kevin Wolf wrote:
> Am 04.11.2015 um 19:57 hat Max Reitz geschrieben:
>> _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>
> 
> Code motion and modification in the same patch is bad style. The changes
> look good, though.

Considering splitting this into two patches will result basically in
both of them each changing just as much as this single patch does
(because test 083 uses tabs instead of spaces) I'm inclined to just
change the commit title to "Remove filter_nbd and add _filter_nbd" instead.

There actually is no good style for this patch. One could argue that the
coding style in all of test 083 is broken since it uses tabs instead of
spaces, so first I'd need to fix up the style of 083 completely. Then,
in a second patch, I can drop those empty lines, and in a third patch I
can move the function. I consider that horrible when it's just about
getting the code to common.filter.

The second variant would be not to move the code, but to "move" it, i.e.
leave the coding style in 083 and just convert the style of this
function when moving it to common.filter. Well, if I'm already doing
that, I might just as well fix that empty line thing on the way.

The third variant would be to fix that empty line thing in 083, and fix
the code style of that single function along with it, and then move it
to common.filter in a separate patch.

And then we have the fourth way which would be to move nbd_filter to
common.filter as it is, and then fix up the coding style there.

So let's look at my opinion for each of them:
(1) I find it horrible, but I can do that.
(2) That's what I did and that's what I'd do again.
(3) I'm opposed to change the style of that one function inside of 083
    without changing the rest of the file, but not strongly enough not
    to do it.
(4) I will definitely not insert tabs, even if it's just code movement.

I still consider 2 very reasonable for the issue at hand since the
function is very small and it will have to be completely “rewritten”
anyway in some patch (because the tabs to spaces change is absolutely
necessary at some point when moving it from 083 to common.filter).
Therefore, I don't think reviewers gain anything from doing it any other
way.

I consider 1 (fixing up all of 083 just so that I can move this little
function) so horrible that I won't do it unless there is another way,
and 2 already is another way, so that's that.

I guess I'll go for 3 just so you can see why I chose 2 before. I can do
1 in v8 if you insist, so you can get to experience that, too.

Max


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

Re: [Qemu-devel] [PATCH v6 04/15] iotests: Move _filter_nbd into common.filter

[Max Reitz]

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

On 09.11.2015 19:17, Max Reitz wrote:
> On 09.11.2015 17:04, Kevin Wolf wrote:
>> Am 04.11.2015 um 19:57 hat Max Reitz geschrieben:
>>> _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>
>>
>> Code motion and modification in the same patch is bad style. The changes
>> look good, though.
> 
> Considering splitting this into two patches will result basically in
> both of them each changing just as much as this single patch does
> (because test 083 uses tabs instead of spaces) I'm inclined to just
> change the commit title to "Remove filter_nbd and add _filter_nbd" instead.
> 
> There actually is no good style for this patch. One could argue that the
> coding style in all of test 083 is broken since it uses tabs instead of
> spaces, so first I'd need to fix up the style of 083 completely. Then,
> in a second patch, I can drop those empty lines, and in a third patch I
> can move the function. I consider that horrible when it's just about
> getting the code to common.filter.
> 
> The second variant would be not to move the code, but to "move" it, i.e.
> leave the coding style in 083 and just convert the style of this
> function when moving it to common.filter. Well, if I'm already doing
> that, I might just as well fix that empty line thing on the way.
> 
> The third variant would be to fix that empty line thing in 083, and fix
> the code style of that single function along with it, and then move it
> to common.filter in a separate patch.
> 
> And then we have the fourth way which would be to move nbd_filter to
> common.filter as it is, and then fix up the coding style there.
> 
> So let's look at my opinion for each of them:
> (1) I find it horrible, but I can do that.
> (2) That's what I did and that's what I'd do again.
> (3) I'm opposed to change the style of that one function inside of 083
>     without changing the rest of the file, but not strongly enough not
>     to do it.
> (4) I will definitely not insert tabs, even if it's just code movement.
> 
> I still consider 2 very reasonable for the issue at hand since the
> function is very small and it will have to be completely “rewritten”
> anyway in some patch (because the tabs to spaces change is absolutely
> necessary at some point when moving it from 083 to common.filter).
> Therefore, I don't think reviewers gain anything from doing it any other
> way.
> 
> I consider 1 (fixing up all of 083 just so that I can move this little
> function) so horrible that I won't do it unless there is another way,
> and 2 already is another way, so that's that.
> 
> I guess I'll go for 3 just so you can see why I chose 2 before. I can do
> 1 in v8 if you insist, so you can get to experience that, too.

Oh, even better, I'll go for a mix of 1, 3 and 4: I'll first fix the
code style of that function alone, then I'll rename it to _filter_nbd,
then I'll move it, and then I'll fix it. Even more patches than 1, but
not as many changes, as I don't have to fix all of 083.

Max


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

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

[Max Reitz]

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

On 09.11.2015 17:21, Max Reitz wrote:
> On 06.11.2015 19:59, John Snow wrote:
>>
>>
>> On 11/04/2015 01:57 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 | 20 +++++++++++++++++++-
>>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 3493501..23448ed 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);
>>>  
>>> @@ -3318,6 +3321,21 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>>>      }
>>>  }
>>>  
>>> +/**
>>> + * Release all dirty bitmaps attached to a BDS, independently of whether they
>>> + * are frozen or not (for use in bdrv_close()).
>>> + */
>>
>> This comment caught my attention ...
>>
>>> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs)
>>> +{
>>> +    BdrvDirtyBitmap *bm, *next;
>>> +    QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
>>> +        QLIST_REMOVE(bm, list);
>>> +        hbitmap_free(bm->bitmap);
>>> +        g_free(bm->name);
>>> +        g_free(bm);
>>> +    }
>>> +}
>>> +
>>>  void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>>>  {
>>>      assert(!bdrv_dirty_bitmap_frozen(bitmap));
>>>
>>
>> Currently, the only way a bitmap could/should be frozen is if it is in
>> use by a job. If a job is running, you probably shouldn't delete stuff
>> it is using out from under it.
> 
> Right. Good thing I'm fixing everything and bdrv_close() will no longer
> be called unless there are no references to the BDS anymore (and each
> block job holds an own reference to its BDS). :-)
> 
> (At least that's how it's supposed to be; also, there is an
> assert(!bs->job) in bdrv_close())
> 
> Also, I wanted to mirror current behavior. Right now, we are just
> leaking all dirty bitmaps on bdrv_close() (bdrv_delete() asserts that
> there are none, but if you invoke bdrv_close() directly...).
> 
>> I am assuming by now that it's actually likely that you've canceled any
>> jobs attached to this node before you go through the motions of deleting
>> it, and it should be safe to just call bdrv_release_dirty_bitmap ...
> 
> Well, yes, but then I'd have to iterate through all the dirty bitmaps to
> call bdrv_release_dirty_bitmap() for each of them, and then
> bdrv_release_dirty_bitmap() iterates through all of them again to check
> whether it really belongs to the BDS. That seemed a bit like a waste.
> 
>> We don't want the two foreach loops though, so maybe just factor out a
>> helper that bdrv_release_all_dirty_bitmaps and bdrv_release_dirty_bitmap
>> can share. You can leave the frozen assertion
>> in just bdrv_release_dirty_bitmap before it invokes the helper, so that
>> bdrv_delete always succeeds in case something gets out-of-sync in the
>> internals.
> 
> Hm, yes, that should do just fine, thanks.
> 
>> (Hmm, to prevent leaks, if you delete a bitmap that is frozen, you
>> should probably call the helper on its child object too... or just make
>> the helper recursive.)
> 
> I think it'll be actually better to just keep the assertion in and thus
> not allow releasing frozen dirty bitmaps in bdrv_close(). In addition
> I'll add an assertion that !bs->refcount to bdrv_close().

OK, I can't actually do that yet. There is a bdrv_close() in one of the
fail paths of bdrv_open_inherit(), and the refcount of that BDS will
probably be 1. There is generally no way that BDS has any bitmaps
attached to it, so we're fine in that regard.

I'll revisit this after this series and its follow-up.

Max


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

Re: [Qemu-devel] [PATCH v6 04/15] iotests: Move _filter_nbd into common.filter

[Kevin Wolf]

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

Am 09.11.2015 um 19:17 hat Max Reitz geschrieben:
> On 09.11.2015 17:04, Kevin Wolf wrote:
> > Am 04.11.2015 um 19:57 hat Max Reitz geschrieben:
> >> _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>
> > 
> > Code motion and modification in the same patch is bad style. The changes
> > look good, though.
> 
> Considering splitting this into two patches will result basically in
> both of them each changing just as much as this single patch does
> (because test 083 uses tabs instead of spaces) I'm inclined to just
> change the commit title to "Remove filter_nbd and add _filter_nbd" instead.

You're confusing "changing much" with "touching many lines". What I'm
asking is to split this in two: One mostly mechanical patch that doesn't
change semantics, and one patch for the functional change. This makes it
much easier to spot the functional changes that are actually made.

For example, whitespace changes during code motion are not a problem at
all. I use git show -w routinely to review those. I can also cope with
other minor things like style changes during code motion.

The hard part during review is just finding the 10% of actual functional
change in the middle of the 90% that change nothing semantically,
especially if multiple hunks are involved in the functional change.

Kevin

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