* [Qemu-devel] [PATCH v8 01/16] block: Release dirty bitmaps in bdrv_close()
2016-01-27 17:59 [Qemu-devel] [PATCH v8 00/16] block: Rework bdrv_close_all() Max Reitz
@ 2016-01-27 17:59 ` Max Reitz
2016-01-28 3:01 ` Fam Zheng
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 02/16] iotests: Add test for eject under NBD server Max Reitz
` (15 subsequent siblings)
16 siblings, 1 reply; 38+ messages in thread
From: Max Reitz @ 2016-01-27 17:59 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Max Reitz, Paolo Bonzini,
Alberto Garcia, John Snow
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>
Reviewed-by: John Snow <jsnow@redhat.com>
---
block.c | 37 +++++++++++++++++++++++++++++--------
1 file changed, 29 insertions(+), 8 deletions(-)
diff --git a/block.c b/block.c
index 5709d3d..9a31e20 100644
--- a/block.c
+++ b/block.c
@@ -88,6 +88,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;
@@ -2157,6 +2159,8 @@ void bdrv_close(BlockDriverState *bs)
notifier_list_notify(&bs->close_notifiers, bs);
+ bdrv_release_all_dirty_bitmaps(bs);
+
if (bs->blk) {
blk_dev_change_media_cb(bs->blk, false);
}
@@ -2366,7 +2370,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);
@@ -3582,21 +3585,39 @@ static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
}
}
-void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap)
{
BdrvDirtyBitmap *bm, *next;
QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
- if (bm == bitmap) {
+ if (!bitmap || bm == bitmap) {
assert(!bdrv_dirty_bitmap_frozen(bm));
- QLIST_REMOVE(bitmap, list);
- hbitmap_free(bitmap->bitmap);
- g_free(bitmap->name);
- g_free(bitmap);
- return;
+ QLIST_REMOVE(bm, list);
+ hbitmap_free(bm->bitmap);
+ g_free(bm->name);
+ g_free(bm);
+
+ if (bitmap) {
+ return;
+ }
}
}
}
+void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+ bdrv_do_release_matching_dirty_bitmap(bs, bitmap);
+}
+
+/**
+ * Release all dirty bitmaps attached to a BDS (for use in bdrv_close()). There
+ * must not be any frozen bitmaps attached.
+ */
+static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs)
+{
+ bdrv_do_release_matching_dirty_bitmap(bs, NULL);
+}
+
void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
{
assert(!bdrv_dirty_bitmap_frozen(bitmap));
--
2.7.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v8 01/16] block: Release dirty bitmaps in bdrv_close()
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 01/16] block: Release dirty bitmaps in bdrv_close() Max Reitz
@ 2016-01-28 3:01 ` Fam Zheng
2016-01-29 13:27 ` Max Reitz
0 siblings, 1 reply; 38+ messages in thread
From: Fam Zheng @ 2016-01-28 3:01 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Alberto Garcia, qemu-block, John Snow, qemu-devel,
Paolo Bonzini
On Wed, 01/27 18:59, 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>
> Reviewed-by: John Snow <jsnow@redhat.com>
> ---
> block.c | 37 +++++++++++++++++++++++++++++--------
> 1 file changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/block.c b/block.c
> index 5709d3d..9a31e20 100644
> --- a/block.c
> +++ b/block.c
> @@ -88,6 +88,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;
>
> @@ -2157,6 +2159,8 @@ void bdrv_close(BlockDriverState *bs)
>
> notifier_list_notify(&bs->close_notifiers, bs);
>
> + bdrv_release_all_dirty_bitmaps(bs);
> +
> if (bs->blk) {
> blk_dev_change_media_cb(bs->blk, false);
> }
> @@ -2366,7 +2370,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);
>
> @@ -3582,21 +3585,39 @@ static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
> }
> }
>
> -void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap)
> {
> BdrvDirtyBitmap *bm, *next;
> QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
> - if (bm == bitmap) {
> + if (!bitmap || bm == bitmap) {
> assert(!bdrv_dirty_bitmap_frozen(bm));
> - QLIST_REMOVE(bitmap, list);
> - hbitmap_free(bitmap->bitmap);
> - g_free(bitmap->name);
> - g_free(bitmap);
> - return;
> + QLIST_REMOVE(bm, list);
> + hbitmap_free(bm->bitmap);
> + g_free(bm->name);
> + g_free(bm);
> +
> + if (bitmap) {
> + return;
> + }
> }
> }
> }
>
> +void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +{
> + bdrv_do_release_matching_dirty_bitmap(bs, bitmap);
> +}
> +
> +/**
> + * Release all dirty bitmaps attached to a BDS (for use in bdrv_close()). There
> + * must not be any frozen bitmaps attached.
Should we assert that? And IIUC the intention of this function is to release
all monitor owned (i.e. user created) dirty bitmaps, which must be named. If
so, can we assert that too?
Fam
> + */
> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs)
> +{
> + bdrv_do_release_matching_dirty_bitmap(bs, NULL);
> +}
> +
> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
> {
> assert(!bdrv_dirty_bitmap_frozen(bitmap));
> --
> 2.7.0
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v8 01/16] block: Release dirty bitmaps in bdrv_close()
2016-01-28 3:01 ` Fam Zheng
@ 2016-01-29 13:27 ` Max Reitz
0 siblings, 0 replies; 38+ messages in thread
From: Max Reitz @ 2016-01-29 13:27 UTC (permalink / raw)
To: Fam Zheng
Cc: Kevin Wolf, Alberto Garcia, qemu-block, John Snow, qemu-devel,
Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 4116 bytes --]
On 28.01.2016 04:01, Fam Zheng wrote:
> On Wed, 01/27 18:59, 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>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>> ---
>> block.c | 37 +++++++++++++++++++++++++++++--------
>> 1 file changed, 29 insertions(+), 8 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 5709d3d..9a31e20 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -88,6 +88,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;
>>
>> @@ -2157,6 +2159,8 @@ void bdrv_close(BlockDriverState *bs)
>>
>> notifier_list_notify(&bs->close_notifiers, bs);
>>
>> + bdrv_release_all_dirty_bitmaps(bs);
>> +
>> if (bs->blk) {
>> blk_dev_change_media_cb(bs->blk, false);
>> }
>> @@ -2366,7 +2370,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);
>>
>> @@ -3582,21 +3585,39 @@ static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>> }
>> }
>>
>> -void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>> +static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
>> + BdrvDirtyBitmap *bitmap)
>> {
>> BdrvDirtyBitmap *bm, *next;
>> QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
>> - if (bm == bitmap) {
>> + if (!bitmap || bm == bitmap) {
>> assert(!bdrv_dirty_bitmap_frozen(bm));
>> - QLIST_REMOVE(bitmap, list);
>> - hbitmap_free(bitmap->bitmap);
>> - g_free(bitmap->name);
>> - g_free(bitmap);
>> - return;
>> + QLIST_REMOVE(bm, list);
>> + hbitmap_free(bm->bitmap);
>> + g_free(bm->name);
>> + g_free(bm);
>> +
>> + if (bitmap) {
>> + return;
>> + }
>> }
>> }
>> }
>>
>> +void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>> +{
>> + bdrv_do_release_matching_dirty_bitmap(bs, bitmap);
>> +}
>> +
>> +/**
>> + * Release all dirty bitmaps attached to a BDS (for use in bdrv_close()). There
>> + * must not be any frozen bitmaps attached.
>
> Should we assert that?
Well, it is asserted in bdrv_do_release_matching_dirty_bitmap().
> And IIUC the intention of this function is to release
> all monitor owned (i.e. user created) dirty bitmaps, which must be named. If
> so, can we assert that too?
Probably we can. I don't really know if it would be worth it, though
(even if it isn't much code). There's not much harm in releasing unnamed
bitmaps here, the main issue would be the question "where did they come
from?".
If I do it, I'll rename the function bdrv_release_named_dirty_bitmaps()
and then assert in bdrv_close() that no bitmaps are attached any more.
Max
>
> Fam
>
>> + */
>> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs)
>> +{
>> + bdrv_do_release_matching_dirty_bitmap(bs, NULL);
>> +}
>> +
>> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>> {
>> assert(!bdrv_dirty_bitmap_frozen(bitmap));
>> --
>> 2.7.0
>>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v8 02/16] iotests: Add test for eject under NBD server
2016-01-27 17:59 [Qemu-devel] [PATCH v8 00/16] block: Rework bdrv_close_all() Max Reitz
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 01/16] block: Release dirty bitmaps in bdrv_close() Max Reitz
@ 2016-01-27 17:59 ` Max Reitz
2016-01-27 20:56 ` Eric Blake
2016-01-28 3:05 ` Fam Zheng
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 03/16] block: Add BB-BDS remove/insert notifiers Max Reitz
` (14 subsequent siblings)
16 siblings, 2 replies; 38+ messages in thread
From: Max Reitz @ 2016-01-27 17:59 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Max Reitz, Paolo Bonzini,
Alberto Garcia, John Snow
This patch adds a test for ejecting the BlockBackend an NBD server is
connected to (the NBD server is supposed to stop).
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/140 | 92 ++++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/140.out | 16 ++++++++
tests/qemu-iotests/group | 1 +
3 files changed, 109 insertions(+)
create mode 100755 tests/qemu-iotests/140
create mode 100644 tests/qemu-iotests/140.out
diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140
new file mode 100755
index 0000000..3434997
--- /dev/null
+++ b/tests/qemu-iotests/140
@@ -0,0 +1,92 @@
+#!/bin/bash
+#
+# Test case for ejecting a BB with an NBD server attached to it
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1 # failure is the default!
+
+_cleanup()
+{
+ _cleanup_test_img
+ rm -f "$TEST_DIR/nbd"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt generic
+_supported_proto file
+_supported_os Linux
+
+_make_test_img 64k
+
+$QEMU_IO -c 'write -P 42 0 64k' "$TEST_IMG" | _filter_qemu_io
+
+keep_stderr=y \
+_launch_qemu -drive if=ide,media=cdrom,id=drv,file="$TEST_IMG",format=$IMGFMT \
+ 2> >(_filter_nbd)
+
+_send_qemu_cmd $QEMU_HANDLE \
+ "{ 'execute': 'qmp_capabilities' }" \
+ 'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+ "{ 'execute': 'nbd-server-start',
+ 'arguments': { 'addr': { 'type': 'unix',
+ 'data': { 'path': '$TEST_DIR/nbd' }}}}" \
+ 'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+ "{ 'execute': 'nbd-server-add',
+ 'arguments': { 'device': 'drv' }}" \
+ 'return'
+
+$QEMU_IO_PROG -f raw -c 'read -P 42 0 64k' \
+ "nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \
+ | _filter_qemu_io | _filter_nbd
+
+_send_qemu_cmd $QEMU_HANDLE \
+ "{ 'execute': 'eject',
+ 'arguments': { 'device': 'drv' }}" \
+ 'return'
+
+$QEMU_IO_PROG -f raw -c close \
+ "nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \
+ | _filter_qemu_io | _filter_nbd
+
+_send_qemu_cmd $QEMU_HANDLE \
+ "{ 'execute': 'quit' }" \
+ 'return'
+
+wait=1 _cleanup_qemu
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/140.out b/tests/qemu-iotests/140.out
new file mode 100644
index 0000000..fdedeb3
--- /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": {}}
+can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: Failed to read export length
+no file open, try 'help open'
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"}
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index ac6a959..ff1ff0d 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -141,5 +141,6 @@
137 rw auto
138 rw auto quick
139 rw auto quick
+140 rw auto quick
142 auto
143 auto quick
--
2.7.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v8 02/16] iotests: Add test for eject under NBD server
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 02/16] iotests: Add test for eject under NBD server Max Reitz
@ 2016-01-27 20:56 ` Eric Blake
2016-01-29 13:07 ` Max Reitz
2016-01-28 3:05 ` Fam Zheng
1 sibling, 1 reply; 38+ messages in thread
From: Eric Blake @ 2016-01-27 20:56 UTC (permalink / raw)
To: Max Reitz, qemu-block
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Alberto Garcia, Paolo Bonzini,
John Snow
[-- Attachment #1: Type: text/plain, Size: 994 bytes --]
On 01/27/2016 10:59 AM, Max Reitz wrote:
> This patch adds a test for ejecting the BlockBackend an NBD server is
> connected to (the NBD server is supposed to stop).
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/140 | 92 ++++++++++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/140.out | 16 ++++++++
> tests/qemu-iotests/group | 1 +
> 3 files changed, 109 insertions(+)
> create mode 100755 tests/qemu-iotests/140
> create mode 100644 tests/qemu-iotests/140.out
>
> diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140
> new file mode 100755
> index 0000000..3434997
> --- /dev/null
> +++ b/tests/qemu-iotests/140
> @@ -0,0 +1,92 @@
> +#!/bin/bash
> +#
> +# Test case for ejecting a BB with an NBD server attached to it
> +#
> +# Copyright (C) 2015 Red Hat, Inc.
Do you want to add 2016 now?
--
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 --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] [PATCH v8 02/16] iotests: Add test for eject under NBD server
2016-01-27 20:56 ` Eric Blake
@ 2016-01-29 13:07 ` Max Reitz
0 siblings, 0 replies; 38+ messages in thread
From: Max Reitz @ 2016-01-29 13:07 UTC (permalink / raw)
To: Eric Blake, qemu-block
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Alberto Garcia, Paolo Bonzini,
John Snow
[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]
On 27.01.2016 21:56, Eric Blake wrote:
> On 01/27/2016 10:59 AM, Max Reitz wrote:
>> This patch adds a test for ejecting the BlockBackend an NBD server is
>> connected to (the NBD server is supposed to stop).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> tests/qemu-iotests/140 | 92 ++++++++++++++++++++++++++++++++++++++++++++++
>> tests/qemu-iotests/140.out | 16 ++++++++
>> tests/qemu-iotests/group | 1 +
>> 3 files changed, 109 insertions(+)
>> create mode 100755 tests/qemu-iotests/140
>> create mode 100644 tests/qemu-iotests/140.out
>>
>> diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140
>> new file mode 100755
>> index 0000000..3434997
>> --- /dev/null
>> +++ b/tests/qemu-iotests/140
>> @@ -0,0 +1,92 @@
>> +#!/bin/bash
>> +#
>> +# Test case for ejecting a BB with an NBD server attached to it
>> +#
>> +# Copyright (C) 2015 Red Hat, Inc.
>
> Do you want to add 2016 now?
Or just replace it; yes, I probably do. :-)
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] [PATCH v8 02/16] iotests: Add test for eject under NBD server
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 02/16] iotests: Add test for eject under NBD server Max Reitz
2016-01-27 20:56 ` Eric Blake
@ 2016-01-28 3:05 ` Fam Zheng
1 sibling, 0 replies; 38+ messages in thread
From: Fam Zheng @ 2016-01-28 3:05 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Alberto Garcia, qemu-block, John Snow, qemu-devel,
Paolo Bonzini
On Wed, 01/27 18:59, Max Reitz wrote:
> This patch adds a test for ejecting the BlockBackend an NBD server is
> connected to (the NBD server is supposed to stop).
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/140 | 92 ++++++++++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/140.out | 16 ++++++++
> tests/qemu-iotests/group | 1 +
> 3 files changed, 109 insertions(+)
> create mode 100755 tests/qemu-iotests/140
> create mode 100644 tests/qemu-iotests/140.out
>
> diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140
> new file mode 100755
> index 0000000..3434997
> --- /dev/null
> +++ b/tests/qemu-iotests/140
> @@ -0,0 +1,92 @@
> +#!/bin/bash
> +#
> +# Test case for ejecting a BB with an NBD server attached to it
> +#
> +# Copyright (C) 2015 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +# creator
> +owner=mreitz@redhat.com
> +
> +seq="$(basename $0)"
> +echo "QA output created by $seq"
> +
> +here="$PWD"
> +tmp=/tmp/$$
> +status=1 # failure is the default!
> +
> +_cleanup()
> +{
> + _cleanup_test_img
> + rm -f "$TEST_DIR/nbd"
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +. ./common.qemu
> +
> +_supported_fmt generic
> +_supported_proto file
> +_supported_os Linux
> +
> +_make_test_img 64k
> +
> +$QEMU_IO -c 'write -P 42 0 64k' "$TEST_IMG" | _filter_qemu_io
> +
> +keep_stderr=y \
> +_launch_qemu -drive if=ide,media=cdrom,id=drv,file="$TEST_IMG",format=$IMGFMT \
> + 2> >(_filter_nbd)
> +
> +_send_qemu_cmd $QEMU_HANDLE \
> + "{ 'execute': 'qmp_capabilities' }" \
> + 'return'
> +
> +_send_qemu_cmd $QEMU_HANDLE \
> + "{ 'execute': 'nbd-server-start',
> + 'arguments': { 'addr': { 'type': 'unix',
> + 'data': { 'path': '$TEST_DIR/nbd' }}}}" \
> + 'return'
> +
> +_send_qemu_cmd $QEMU_HANDLE \
> + "{ 'execute': 'nbd-server-add',
> + 'arguments': { 'device': 'drv' }}" \
> + 'return'
> +
> +$QEMU_IO_PROG -f raw -c 'read -P 42 0 64k' \
> + "nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \
> + | _filter_qemu_io | _filter_nbd
> +
> +_send_qemu_cmd $QEMU_HANDLE \
> + "{ 'execute': 'eject',
> + 'arguments': { 'device': 'drv' }}" \
> + 'return'
> +
> +$QEMU_IO_PROG -f raw -c close \
> + "nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \
> + | _filter_qemu_io | _filter_nbd
> +
> +_send_qemu_cmd $QEMU_HANDLE \
> + "{ 'execute': 'quit' }" \
> + 'return'
> +
> +wait=1 _cleanup_qemu
> +
> +# success, all done
> +echo '*** done'
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/140.out b/tests/qemu-iotests/140.out
> new file mode 100644
> index 0000000..fdedeb3
> --- /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": {}}
> +can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: Failed to read export length
> +no file open, try 'help open'
> +{"return": {}}
> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"}
> +*** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index ac6a959..ff1ff0d 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -141,5 +141,6 @@
> 137 rw auto
> 138 rw auto quick
> 139 rw auto quick
> +140 rw auto quick
> 142 auto
> 143 auto quick
> --
> 2.7.0
>
Reviewed-by: Fam Zheng <famz@redhat.com>
Of course, regardless of the copy right year. :)
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v8 03/16] block: Add BB-BDS remove/insert notifiers
2016-01-27 17:59 [Qemu-devel] [PATCH v8 00/16] block: Rework bdrv_close_all() Max Reitz
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 01/16] block: Release dirty bitmaps in bdrv_close() Max Reitz
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 02/16] iotests: Add test for eject under NBD server Max Reitz
@ 2016-01-27 17:59 ` Max Reitz
2016-01-28 3:06 ` Fam Zheng
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 04/16] virtio-blk: Functions for op blocker management Max Reitz
` (13 subsequent siblings)
16 siblings, 1 reply; 38+ messages in thread
From: Max Reitz @ 2016-01-27 17:59 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Max Reitz, Paolo Bonzini,
Alberto Garcia, John Snow
bdrv_close() no longer signifies ejection of a medium, this is now done
by removing the BDS from the BB. Therefore, we want to have a notifier
for that in the BB instead of a close notifier in the BDS. The former is
added now, the latter is removed later.
Symmetrically, another notifier list is added that is invoked whenever a
BDS is inserted. We will need that for virtio-blk and virtio-scsi, which
can then remove their op blockers on BDS ejection and set them up on
insertion.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block/block-backend.c | 20 ++++++++++++++++++++
include/sysemu/block-backend.h | 2 ++
2 files changed, 22 insertions(+)
diff --git a/block/block-backend.c b/block/block-backend.c
index a4208f1..1872191 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -49,6 +49,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 {
@@ -99,6 +101,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;
}
@@ -167,6 +171,8 @@ static void blk_delete(BlockBackend *blk)
bdrv_unref(blk->bs);
blk->bs = NULL;
}
+ assert(QLIST_EMPTY(&blk->remove_bs_notifiers.notifiers));
+ assert(QLIST_EMPTY(&blk->insert_bs_notifiers.notifiers));
if (blk->root_state.throttle_state) {
g_free(blk->root_state.throttle_group);
throttle_group_unref(blk->root_state.throttle_state);
@@ -345,6 +351,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;
@@ -361,6 +369,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);
}
/*
@@ -1126,6 +1136,16 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
}
}
+void blk_add_remove_bs_notifier(BlockBackend *blk, Notifier *notify)
+{
+ notifier_list_add(&blk->remove_bs_notifiers, notify);
+}
+
+void blk_add_insert_bs_notifier(BlockBackend *blk, Notifier *notify)
+{
+ notifier_list_add(&blk->insert_bs_notifiers, notify);
+}
+
void blk_add_close_notifier(BlockBackend *blk, Notifier *notify)
{
if (blk->bs) {
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 1568554..e12be67 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -164,6 +164,8 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
void *),
void (*detach_aio_context)(void *),
void *opaque);
+void blk_add_remove_bs_notifier(BlockBackend *blk, Notifier *notify);
+void blk_add_insert_bs_notifier(BlockBackend *blk, Notifier *notify);
void blk_add_close_notifier(BlockBackend *blk, Notifier *notify);
void blk_io_plug(BlockBackend *blk);
void blk_io_unplug(BlockBackend *blk);
--
2.7.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v8 03/16] block: Add BB-BDS remove/insert notifiers
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 03/16] block: Add BB-BDS remove/insert notifiers Max Reitz
@ 2016-01-28 3:06 ` Fam Zheng
0 siblings, 0 replies; 38+ messages in thread
From: Fam Zheng @ 2016-01-28 3:06 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Alberto Garcia, qemu-block, John Snow, qemu-devel,
Paolo Bonzini
On Wed, 01/27 18:59, Max Reitz wrote:
> bdrv_close() no longer signifies ejection of a medium, this is now done
> by removing the BDS from the BB. Therefore, we want to have a notifier
> for that in the BB instead of a close notifier in the BDS. The former is
> added now, the latter is removed later.
>
> Symmetrically, another notifier list is added that is invoked whenever a
> BDS is inserted. We will need that for virtio-blk and virtio-scsi, which
> can then remove their op blockers on BDS ejection and set them up on
> insertion.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block/block-backend.c | 20 ++++++++++++++++++++
> include/sysemu/block-backend.h | 2 ++
> 2 files changed, 22 insertions(+)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index a4208f1..1872191 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -49,6 +49,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 {
> @@ -99,6 +101,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;
> }
> @@ -167,6 +171,8 @@ static void blk_delete(BlockBackend *blk)
> bdrv_unref(blk->bs);
> blk->bs = NULL;
> }
> + assert(QLIST_EMPTY(&blk->remove_bs_notifiers.notifiers));
> + assert(QLIST_EMPTY(&blk->insert_bs_notifiers.notifiers));
> if (blk->root_state.throttle_state) {
> g_free(blk->root_state.throttle_group);
> throttle_group_unref(blk->root_state.throttle_state);
> @@ -345,6 +351,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;
> @@ -361,6 +369,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);
> }
>
> /*
> @@ -1126,6 +1136,16 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
> }
> }
>
> +void blk_add_remove_bs_notifier(BlockBackend *blk, Notifier *notify)
> +{
> + notifier_list_add(&blk->remove_bs_notifiers, notify);
> +}
> +
> +void blk_add_insert_bs_notifier(BlockBackend *blk, Notifier *notify)
> +{
> + notifier_list_add(&blk->insert_bs_notifiers, notify);
> +}
> +
> void blk_add_close_notifier(BlockBackend *blk, Notifier *notify)
> {
> if (blk->bs) {
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index 1568554..e12be67 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -164,6 +164,8 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
> void *),
> void (*detach_aio_context)(void *),
> void *opaque);
> +void blk_add_remove_bs_notifier(BlockBackend *blk, Notifier *notify);
> +void blk_add_insert_bs_notifier(BlockBackend *blk, Notifier *notify);
> void blk_add_close_notifier(BlockBackend *blk, Notifier *notify);
> void blk_io_plug(BlockBackend *blk);
> void blk_io_unplug(BlockBackend *blk);
> --
> 2.7.0
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v8 04/16] virtio-blk: Functions for op blocker management
2016-01-27 17:59 [Qemu-devel] [PATCH v8 00/16] block: Rework bdrv_close_all() Max Reitz
` (2 preceding siblings ...)
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 03/16] block: Add BB-BDS remove/insert notifiers Max Reitz
@ 2016-01-27 17:59 ` Max Reitz
2016-01-28 3:09 ` Fam Zheng
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 05/16] virtio-scsi: Catch BDS-BB removal/insertion Max Reitz
` (12 subsequent siblings)
16 siblings, 1 reply; 38+ messages in thread
From: Max Reitz @ 2016-01-27 17:59 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Max Reitz, Paolo Bonzini,
Alberto Garcia, John Snow
Put the code for setting up and removing op blockers into an own
function, respectively. Then, we can invoke those functions whenever a
BDS is removed from an virtio-blk BB or inserted into it.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
hw/block/dataplane/virtio-blk.c | 77 +++++++++++++++++++++++++++++++----------
1 file changed, 59 insertions(+), 18 deletions(-)
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index bc34046..ee0c4d4 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -40,6 +40,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_SOURCE, 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,
@@ -179,22 +229,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_SOURCE, 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;
}
@@ -207,8 +247,9 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
}
virtio_blk_data_plane_stop(s);
- blk_op_unblock_all(s->conf->conf.blk, s->blocker);
- error_free(s->blocker);
+ data_plane_remove_op_blockers(s);
+ notifier_remove(&s->insert_notifier);
+ notifier_remove(&s->remove_notifier);
qemu_bh_delete(s->bh);
object_unref(OBJECT(s->iothread));
g_free(s);
--
2.7.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v8 04/16] virtio-blk: Functions for op blocker management
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 04/16] virtio-blk: Functions for op blocker management Max Reitz
@ 2016-01-28 3:09 ` Fam Zheng
0 siblings, 0 replies; 38+ messages in thread
From: Fam Zheng @ 2016-01-28 3:09 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Alberto Garcia, qemu-block, John Snow, qemu-devel,
Paolo Bonzini
On Wed, 01/27 18:59, Max Reitz wrote:
> Put the code for setting up and removing op blockers into an own
> function, respectively. Then, we can invoke those functions whenever a
> BDS is removed from an virtio-blk BB or inserted into it.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> hw/block/dataplane/virtio-blk.c | 77 +++++++++++++++++++++++++++++++----------
> 1 file changed, 59 insertions(+), 18 deletions(-)
>
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index bc34046..ee0c4d4 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -40,6 +40,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_SOURCE, 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,
> @@ -179,22 +229,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_SOURCE, 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;
> }
> @@ -207,8 +247,9 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
> }
>
> virtio_blk_data_plane_stop(s);
> - blk_op_unblock_all(s->conf->conf.blk, s->blocker);
> - error_free(s->blocker);
> + data_plane_remove_op_blockers(s);
> + notifier_remove(&s->insert_notifier);
> + notifier_remove(&s->remove_notifier);
> qemu_bh_delete(s->bh);
> object_unref(OBJECT(s->iothread));
> g_free(s);
> --
> 2.7.0
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v8 05/16] virtio-scsi: Catch BDS-BB removal/insertion
2016-01-27 17:59 [Qemu-devel] [PATCH v8 00/16] block: Rework bdrv_close_all() Max Reitz
` (3 preceding siblings ...)
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 04/16] virtio-blk: Functions for op blocker management Max Reitz
@ 2016-01-27 17:59 ` Max Reitz
2016-01-28 3:14 ` Fam Zheng
2016-01-29 12:41 ` Kevin Wolf
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 06/16] nbd: Switch from close to eject notifier Max Reitz
` (11 subsequent siblings)
16 siblings, 2 replies; 38+ messages in thread
From: Max Reitz @ 2016-01-27 17:59 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Max Reitz, Paolo Bonzini,
Alberto Garcia, John Snow
Make use of the BDS-BB removal and insertion notifiers to remove or set
up, respectively, virtio-scsi's op blockers.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
hw/scsi/virtio-scsi.c | 55 +++++++++++++++++++++++++++++++++++++++++
include/hw/virtio/virtio-scsi.h | 10 ++++++++
2 files changed, 65 insertions(+)
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 607593c..b508b81 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,25 @@ 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) {
+ notifier_remove(&insert_notifier->n);
+ QTAILQ_REMOVE(&s->insert_notifiers, insert_notifier, next);
+ g_free(insert_notifier);
+ break;
+ }
+ }
+
+ QTAILQ_FOREACH(remove_notifier, &s->remove_notifiers, next) {
+ if (remove_notifier->sd == sd) {
+ notifier_remove(&remove_notifier->n);
+ QTAILQ_REMOVE(&s->remove_notifiers, remove_notifier, next);
+ g_free(remove_notifier);
+ break;
+ }
+ }
+
qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
}
@@ -911,6 +963,9 @@ static void virtio_scsi_device_realize(DeviceState *dev, Error **errp)
add_migration_state_change_notifier(&s->migration_state_notifier);
error_setg(&s->blocker, "block device is in use by data plane");
+
+ QTAILQ_INIT(&s->insert_notifiers);
+ QTAILQ_INIT(&s->remove_notifiers);
}
static void virtio_scsi_instance_init(Object *obj)
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 088fe9f..0394eb2 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -76,6 +76,13 @@ typedef struct VirtIOSCSICommon {
VirtQueue **cmd_vqs;
} VirtIOSCSICommon;
+typedef struct VirtIOSCSIBlkChangeNotifier {
+ Notifier n;
+ struct VirtIOSCSI *s;
+ SCSIDevice *sd;
+ QTAILQ_ENTRY(VirtIOSCSIBlkChangeNotifier) next;
+} VirtIOSCSIBlkChangeNotifier;
+
typedef struct VirtIOSCSI {
VirtIOSCSICommon parent_obj;
@@ -86,6 +93,9 @@ typedef struct VirtIOSCSI {
/* Fields for dataplane below */
AioContext *ctx; /* one iothread per virtio-scsi-pci for now */
+ QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) insert_notifiers;
+ QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) remove_notifiers;
+
/* Vring is used instead of vq in dataplane code, because of the underlying
* memory layer thread safety */
VirtIOSCSIVring *ctrl_vring;
--
2.7.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v8 05/16] virtio-scsi: Catch BDS-BB removal/insertion
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 05/16] virtio-scsi: Catch BDS-BB removal/insertion Max Reitz
@ 2016-01-28 3:14 ` Fam Zheng
2016-01-29 12:41 ` Kevin Wolf
1 sibling, 0 replies; 38+ messages in thread
From: Fam Zheng @ 2016-01-28 3:14 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Alberto Garcia, qemu-block, John Snow, qemu-devel,
Paolo Bonzini
On Wed, 01/27 18:59, Max Reitz wrote:
> Make use of the BDS-BB removal and insertion notifiers to remove or set
> up, respectively, virtio-scsi's op blockers.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> hw/scsi/virtio-scsi.c | 55 +++++++++++++++++++++++++++++++++++++++++
> include/hw/virtio/virtio-scsi.h | 10 ++++++++
> 2 files changed, 65 insertions(+)
>
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 607593c..b508b81 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,25 @@ 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) {
> + notifier_remove(&insert_notifier->n);
> + QTAILQ_REMOVE(&s->insert_notifiers, insert_notifier, next);
> + g_free(insert_notifier);
> + break;
> + }
> + }
> +
> + QTAILQ_FOREACH(remove_notifier, &s->remove_notifiers, next) {
> + if (remove_notifier->sd == sd) {
> + notifier_remove(&remove_notifier->n);
> + QTAILQ_REMOVE(&s->remove_notifiers, remove_notifier, next);
> + g_free(remove_notifier);
> + break;
> + }
> + }
> +
> qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
> }
>
> @@ -911,6 +963,9 @@ static void virtio_scsi_device_realize(DeviceState *dev, Error **errp)
> add_migration_state_change_notifier(&s->migration_state_notifier);
>
> error_setg(&s->blocker, "block device is in use by data plane");
> +
> + QTAILQ_INIT(&s->insert_notifiers);
> + QTAILQ_INIT(&s->remove_notifiers);
> }
>
> static void virtio_scsi_instance_init(Object *obj)
> diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> index 088fe9f..0394eb2 100644
> --- a/include/hw/virtio/virtio-scsi.h
> +++ b/include/hw/virtio/virtio-scsi.h
> @@ -76,6 +76,13 @@ typedef struct VirtIOSCSICommon {
> VirtQueue **cmd_vqs;
> } VirtIOSCSICommon;
>
> +typedef struct VirtIOSCSIBlkChangeNotifier {
> + Notifier n;
> + struct VirtIOSCSI *s;
> + SCSIDevice *sd;
> + QTAILQ_ENTRY(VirtIOSCSIBlkChangeNotifier) next;
> +} VirtIOSCSIBlkChangeNotifier;
> +
> typedef struct VirtIOSCSI {
> VirtIOSCSICommon parent_obj;
>
> @@ -86,6 +93,9 @@ typedef struct VirtIOSCSI {
> /* Fields for dataplane below */
> AioContext *ctx; /* one iothread per virtio-scsi-pci for now */
>
> + QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) insert_notifiers;
> + QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) remove_notifiers;
> +
> /* Vring is used instead of vq in dataplane code, because of the underlying
> * memory layer thread safety */
> VirtIOSCSIVring *ctrl_vring;
> --
> 2.7.0
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v8 05/16] virtio-scsi: Catch BDS-BB removal/insertion
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 05/16] virtio-scsi: Catch BDS-BB removal/insertion Max Reitz
2016-01-28 3:14 ` Fam Zheng
@ 2016-01-29 12:41 ` Kevin Wolf
2016-01-29 14:13 ` Max Reitz
1 sibling, 1 reply; 38+ messages in thread
From: Kevin Wolf @ 2016-01-29 12:41 UTC (permalink / raw)
To: Max Reitz
Cc: Alberto Garcia, qemu-block, qemu-devel, Paolo Bonzini, Fam Zheng,
John Snow
Am 27.01.2016 um 18:59 hat Max Reitz geschrieben:
> Make use of the BDS-BB removal and insertion notifiers to remove or set
> up, respectively, virtio-scsi's op blockers.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> hw/scsi/virtio-scsi.c | 55 +++++++++++++++++++++++++++++++++++++++++
> include/hw/virtio/virtio-scsi.h | 10 ++++++++
> 2 files changed, 65 insertions(+)
>
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 607593c..b508b81 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;
> }
If we take the error path here, won't we have dangling pointers in the
notifier list?
Kevin
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v8 05/16] virtio-scsi: Catch BDS-BB removal/insertion
2016-01-29 12:41 ` Kevin Wolf
@ 2016-01-29 14:13 ` Max Reitz
0 siblings, 0 replies; 38+ messages in thread
From: Max Reitz @ 2016-01-29 14:13 UTC (permalink / raw)
To: Kevin Wolf
Cc: Alberto Garcia, qemu-block, qemu-devel, Paolo Bonzini, Fam Zheng,
John Snow
[-- Attachment #1: Type: text/plain, Size: 2890 bytes --]
On 29.01.2016 13:41, Kevin Wolf wrote:
> Am 27.01.2016 um 18:59 hat Max Reitz geschrieben:
>> Make use of the BDS-BB removal and insertion notifiers to remove or set
>> up, respectively, virtio-scsi's op blockers.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> hw/scsi/virtio-scsi.c | 55 +++++++++++++++++++++++++++++++++++++++++
>> include/hw/virtio/virtio-scsi.h | 10 ++++++++
>> 2 files changed, 65 insertions(+)
>>
>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
>> index 607593c..b508b81 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;
>> }
>
> If we take the error path here, won't we have dangling pointers in the
> notifier list?
Yes, I'll move it below that error path.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v8 06/16] nbd: Switch from close to eject notifier
2016-01-27 17:59 [Qemu-devel] [PATCH v8 00/16] block: Rework bdrv_close_all() Max Reitz
` (4 preceding siblings ...)
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 05/16] virtio-scsi: Catch BDS-BB removal/insertion Max Reitz
@ 2016-01-27 17:59 ` Max Reitz
2016-01-28 3:26 ` Fam Zheng
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 07/16] block: Remove BDS close notifier Max Reitz
` (10 subsequent siblings)
16 siblings, 1 reply; 38+ messages in thread
From: Max Reitz @ 2016-01-27 17:59 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Max Reitz, Paolo Bonzini,
Alberto Garcia, John Snow
The NBD code uses the BDS close notifier to determine when a medium is
ejected. However, now it should use the BB's BDS removal notifier for
that instead of the BDS's close notifier.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
blockdev-nbd.c | 40 +++++-----------------------------------
nbd/server.c | 13 +++++++++++++
2 files changed, 18 insertions(+), 35 deletions(-)
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 4a758ac..9d6a21c 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -45,37 +45,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,19 +87,15 @@ 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);
+ /* The list of named exports has a strong reference to this export now and
+ * our only way of accessing it is through nbd_export_find(), so we can drop
+ * the strong reference that is @exp. */
+ nbd_export_put(exp);
}
void qmp_nbd_server_stop(Error **errp)
{
- while (!QTAILQ_EMPTY(&close_notifiers)) {
- NBDCloseNotifier *cn = QTAILQ_FIRST(&close_notifiers);
- nbd_close_notifier(&cn->n, nbd_export_get_blockdev(cn->exp));
- }
+ nbd_export_close_all();
if (server_fd != -1) {
qemu_set_fd_handler(server_fd, NULL, NULL, NULL);
diff --git a/nbd/server.c b/nbd/server.c
index 5169b59..2045f7c 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -64,6 +64,8 @@ struct NBDExport {
QTAILQ_ENTRY(NBDExport) next;
AioContext *ctx;
+
+ Notifier eject_notifier;
};
static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
@@ -644,6 +646,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)
@@ -666,6 +674,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_INACTIVE is cleared and the image is ready for write
@@ -745,6 +757,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.7.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v8 06/16] nbd: Switch from close to eject notifier
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 06/16] nbd: Switch from close to eject notifier Max Reitz
@ 2016-01-28 3:26 ` Fam Zheng
2016-01-29 13:39 ` Max Reitz
0 siblings, 1 reply; 38+ messages in thread
From: Fam Zheng @ 2016-01-28 3:26 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Alberto Garcia, qemu-block, John Snow, qemu-devel,
Paolo Bonzini
On Wed, 01/27 18:59, Max Reitz wrote:
> The NBD code uses the BDS close notifier to determine when a medium is
> ejected. However, now it should use the BB's BDS removal notifier for
> that instead of the BDS's close notifier.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> blockdev-nbd.c | 40 +++++-----------------------------------
> nbd/server.c | 13 +++++++++++++
> 2 files changed, 18 insertions(+), 35 deletions(-)
>
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 4a758ac..9d6a21c 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -45,37 +45,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,19 +87,15 @@ 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);
> + /* The list of named exports has a strong reference to this export now and
> + * our only way of accessing it is through nbd_export_find(), so we can drop
> + * the strong reference that is @exp. */
Not quite sure about the meaning of "the strong reference that is @exp", I
guess you mean the one reference born in nbd_export_new(), which would match
the code. Other than this,
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v8 06/16] nbd: Switch from close to eject notifier
2016-01-28 3:26 ` Fam Zheng
@ 2016-01-29 13:39 ` Max Reitz
0 siblings, 0 replies; 38+ messages in thread
From: Max Reitz @ 2016-01-29 13:39 UTC (permalink / raw)
To: Fam Zheng
Cc: Kevin Wolf, Alberto Garcia, qemu-block, John Snow, qemu-devel,
Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 2814 bytes --]
On 28.01.2016 04:26, Fam Zheng wrote:
> On Wed, 01/27 18:59, Max Reitz wrote:
>> The NBD code uses the BDS close notifier to determine when a medium is
>> ejected. However, now it should use the BB's BDS removal notifier for
>> that instead of the BDS's close notifier.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> blockdev-nbd.c | 40 +++++-----------------------------------
>> nbd/server.c | 13 +++++++++++++
>> 2 files changed, 18 insertions(+), 35 deletions(-)
>>
>> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
>> index 4a758ac..9d6a21c 100644
>> --- a/blockdev-nbd.c
>> +++ b/blockdev-nbd.c
>> @@ -45,37 +45,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,19 +87,15 @@ 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);
>> + /* The list of named exports has a strong reference to this export now and
>> + * our only way of accessing it is through nbd_export_find(), so we can drop
>> + * the strong reference that is @exp. */
>
> Not quite sure about the meaning of "the strong reference that is @exp", I
> guess you mean the one reference born in nbd_export_new(), which would match
> the code. Other than this,
Yes, the reference returned by nbd_export_new(), which is then stored in
@exp. This is a strong reference, so once @exp goes out of scope, the
reference counter has to be decremented.
Max
> Reviewed-by: Fam Zheng <famz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v8 07/16] block: Remove BDS close notifier
2016-01-27 17:59 [Qemu-devel] [PATCH v8 00/16] block: Rework bdrv_close_all() Max Reitz
` (5 preceding siblings ...)
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 06/16] nbd: Switch from close to eject notifier Max Reitz
@ 2016-01-27 17:59 ` Max Reitz
2016-01-28 3:27 ` Fam Zheng
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 08/16] block: Use blk_remove_bs() in blk_delete() Max Reitz
` (9 subsequent siblings)
16 siblings, 1 reply; 38+ messages in thread
From: Max Reitz @ 2016-01-27 17:59 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Max Reitz, Paolo Bonzini,
Alberto Garcia, John Snow
It is unused now, so we can remove it.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 8 --------
block/block-backend.c | 7 -------
include/block/block.h | 1 -
include/block/block_int.h | 2 --
include/sysemu/block-backend.h | 1 -
5 files changed, 19 deletions(-)
diff --git a/block.c b/block.c
index 9a31e20..a6da333 100644
--- a/block.c
+++ b/block.c
@@ -259,7 +259,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]);
@@ -269,11 +268,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;
@@ -2157,8 +2151,6 @@ void bdrv_close(BlockDriverState *bs)
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);
if (bs->blk) {
diff --git a/block/block-backend.c b/block/block-backend.c
index 1872191..621787c 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1146,13 +1146,6 @@ void blk_add_insert_bs_notifier(BlockBackend *blk, Notifier *notify)
notifier_list_add(&blk->insert_bs_notifiers, notify);
}
-void blk_add_close_notifier(BlockBackend *blk, Notifier *notify)
-{
- if (blk->bs) {
- bdrv_add_close_notifier(blk->bs, notify);
- }
-}
-
void blk_io_plug(BlockBackend *blk)
{
if (blk->bs) {
diff --git a/include/block/block.h b/include/block/block.h
index 25f36dc..c7345de 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -226,7 +226,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 ec31df1..8730cf6 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -403,8 +403,6 @@ struct BlockDriverState {
BdrvChild *backing;
BdrvChild *file;
- NotifierList close_notifiers;
-
/* Callback before write request is processed */
NotifierWithReturnList before_write_notifiers;
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index e12be67..ae4efb4 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -166,7 +166,6 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
void *opaque);
void blk_add_remove_bs_notifier(BlockBackend *blk, Notifier *notify);
void blk_add_insert_bs_notifier(BlockBackend *blk, Notifier *notify);
-void blk_add_close_notifier(BlockBackend *blk, Notifier *notify);
void blk_io_plug(BlockBackend *blk);
void blk_io_unplug(BlockBackend *blk);
BlockAcctStats *blk_get_stats(BlockBackend *blk);
--
2.7.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v8 07/16] block: Remove BDS close notifier
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 07/16] block: Remove BDS close notifier Max Reitz
@ 2016-01-28 3:27 ` Fam Zheng
0 siblings, 0 replies; 38+ messages in thread
From: Fam Zheng @ 2016-01-28 3:27 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Alberto Garcia, qemu-block, John Snow, qemu-devel,
Paolo Bonzini
On Wed, 01/27 18:59, Max Reitz wrote:
> It is unused now, so we can remove it.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block.c | 8 --------
> block/block-backend.c | 7 -------
> include/block/block.h | 1 -
> include/block/block_int.h | 2 --
> include/sysemu/block-backend.h | 1 -
> 5 files changed, 19 deletions(-)
>
> diff --git a/block.c b/block.c
> index 9a31e20..a6da333 100644
> --- a/block.c
> +++ b/block.c
> @@ -259,7 +259,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]);
> @@ -269,11 +268,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;
> @@ -2157,8 +2151,6 @@ void bdrv_close(BlockDriverState *bs)
> 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);
>
> if (bs->blk) {
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 1872191..621787c 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1146,13 +1146,6 @@ void blk_add_insert_bs_notifier(BlockBackend *blk, Notifier *notify)
> notifier_list_add(&blk->insert_bs_notifiers, notify);
> }
>
> -void blk_add_close_notifier(BlockBackend *blk, Notifier *notify)
> -{
> - if (blk->bs) {
> - bdrv_add_close_notifier(blk->bs, notify);
> - }
> -}
> -
> void blk_io_plug(BlockBackend *blk)
> {
> if (blk->bs) {
> diff --git a/include/block/block.h b/include/block/block.h
> index 25f36dc..c7345de 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -226,7 +226,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 ec31df1..8730cf6 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -403,8 +403,6 @@ struct BlockDriverState {
> BdrvChild *backing;
> BdrvChild *file;
>
> - NotifierList close_notifiers;
> -
> /* Callback before write request is processed */
> NotifierWithReturnList before_write_notifiers;
>
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index e12be67..ae4efb4 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -166,7 +166,6 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
> void *opaque);
> void blk_add_remove_bs_notifier(BlockBackend *blk, Notifier *notify);
> void blk_add_insert_bs_notifier(BlockBackend *blk, Notifier *notify);
> -void blk_add_close_notifier(BlockBackend *blk, Notifier *notify);
> void blk_io_plug(BlockBackend *blk);
> void blk_io_unplug(BlockBackend *blk);
> BlockAcctStats *blk_get_stats(BlockBackend *blk);
> --
> 2.7.0
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v8 08/16] block: Use blk_remove_bs() in blk_delete()
2016-01-27 17:59 [Qemu-devel] [PATCH v8 00/16] block: Rework bdrv_close_all() Max Reitz
` (6 preceding siblings ...)
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 07/16] block: Remove BDS close notifier Max Reitz
@ 2016-01-27 17:59 ` Max Reitz
2016-01-28 3:28 ` Fam Zheng
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 09/16] blockdev: Use blk_remove_bs() in do_drive_del() Max Reitz
` (8 subsequent siblings)
16 siblings, 1 reply; 38+ messages in thread
From: Max Reitz @ 2016-01-27 17:59 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Max Reitz, Paolo Bonzini,
Alberto Garcia, John Snow
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block/block-backend.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 621787c..7f5ad59 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -166,10 +166,7 @@ static void blk_delete(BlockBackend *blk)
assert(!blk->refcnt);
assert(!blk->dev);
if (blk->bs) {
- assert(blk->bs->blk == blk);
- blk->bs->blk = NULL;
- bdrv_unref(blk->bs);
- blk->bs = NULL;
+ blk_remove_bs(blk);
}
assert(QLIST_EMPTY(&blk->remove_bs_notifiers.notifiers));
assert(QLIST_EMPTY(&blk->insert_bs_notifiers.notifiers));
@@ -351,6 +348,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.7.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v8 08/16] block: Use blk_remove_bs() in blk_delete()
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 08/16] block: Use blk_remove_bs() in blk_delete() Max Reitz
@ 2016-01-28 3:28 ` Fam Zheng
0 siblings, 0 replies; 38+ messages in thread
From: Fam Zheng @ 2016-01-28 3:28 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Alberto Garcia, qemu-block, John Snow, qemu-devel,
Paolo Bonzini
On Wed, 01/27 18:59, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block/block-backend.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 621787c..7f5ad59 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -166,10 +166,7 @@ static void blk_delete(BlockBackend *blk)
> assert(!blk->refcnt);
> assert(!blk->dev);
> if (blk->bs) {
> - assert(blk->bs->blk == blk);
> - blk->bs->blk = NULL;
> - bdrv_unref(blk->bs);
> - blk->bs = NULL;
> + blk_remove_bs(blk);
> }
> assert(QLIST_EMPTY(&blk->remove_bs_notifiers.notifiers));
> assert(QLIST_EMPTY(&blk->insert_bs_notifiers.notifiers));
> @@ -351,6 +348,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.7.0
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v8 09/16] blockdev: Use blk_remove_bs() in do_drive_del()
2016-01-27 17:59 [Qemu-devel] [PATCH v8 00/16] block: Rework bdrv_close_all() Max Reitz
` (7 preceding siblings ...)
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 08/16] block: Use blk_remove_bs() in blk_delete() Max Reitz
@ 2016-01-27 17:59 ` Max Reitz
2016-01-28 3:29 ` Fam Zheng
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 10/16] block: Make bdrv_close() static Max Reitz
` (7 subsequent siblings)
16 siblings, 1 reply; 38+ messages in thread
From: Max Reitz @ 2016-01-27 17:59 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Max Reitz, Paolo Bonzini,
Alberto Garcia, John Snow
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
blockdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/blockdev.c b/blockdev.c
index 1044a6a..09d4621 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2792,7 +2792,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.7.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v8 09/16] blockdev: Use blk_remove_bs() in do_drive_del()
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 09/16] blockdev: Use blk_remove_bs() in do_drive_del() Max Reitz
@ 2016-01-28 3:29 ` Fam Zheng
0 siblings, 0 replies; 38+ messages in thread
From: Fam Zheng @ 2016-01-28 3:29 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Alberto Garcia, qemu-block, John Snow, qemu-devel,
Paolo Bonzini
On Wed, 01/27 18:59, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
> blockdev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 1044a6a..09d4621 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2792,7 +2792,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.7.0
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v8 10/16] block: Make bdrv_close() static
2016-01-27 17:59 [Qemu-devel] [PATCH v8 00/16] block: Rework bdrv_close_all() Max Reitz
` (8 preceding siblings ...)
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 09/16] blockdev: Use blk_remove_bs() in do_drive_del() Max Reitz
@ 2016-01-27 17:59 ` Max Reitz
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 11/16] block: Add list of all BlockDriverStates Max Reitz
` (6 subsequent siblings)
16 siblings, 0 replies; 38+ messages in thread
From: Max Reitz @ 2016-01-27 17:59 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Max Reitz, Paolo Bonzini,
Alberto Garcia, John Snow
There are no users of bdrv_close() left, except for one of bdrv_open()'s
failure paths, bdrv_close_all() and bdrv_delete(), and that is good.
Make bdrv_close() static so nobody makes the mistake of directly using
bdrv_close() again.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Fam Zheng <famz@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 a6da333..be89e8f 100644
--- a/block.c
+++ b/block.c
@@ -93,6 +93,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)
{
@@ -2134,7 +2136,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 c7345de..4452b79 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -225,7 +225,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.7.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v8 11/16] block: Add list of all BlockDriverStates
2016-01-27 17:59 [Qemu-devel] [PATCH v8 00/16] block: Rework bdrv_close_all() Max Reitz
` (9 preceding siblings ...)
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 10/16] block: Make bdrv_close() static Max Reitz
@ 2016-01-27 17:59 ` Max Reitz
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 12/16] blockdev: Keep track of monitor-owned BDS Max Reitz
` (5 subsequent siblings)
16 siblings, 0 replies; 38+ messages in thread
From: Max Reitz @ 2016-01-27 17:59 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Max Reitz, Paolo Bonzini,
Alberto Garcia, John Snow
We need this list so that bdrv_close_all() can keep track of which BDSs
are still open after having removed the BDSs from all of the BBs and
having released all monitor BDS references.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
block.c | 7 +++++++
include/block/block_int.h | 2 ++
2 files changed, 9 insertions(+)
diff --git a/block.c b/block.c
index be89e8f..f8dd4a3 100644
--- a/block.c
+++ b/block.c
@@ -79,6 +79,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);
@@ -267,6 +270,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;
}
@@ -2370,6 +2375,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 8730cf6..1e4c518 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -443,6 +443,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.7.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v8 12/16] blockdev: Keep track of monitor-owned BDS
2016-01-27 17:59 [Qemu-devel] [PATCH v8 00/16] block: Rework bdrv_close_all() Max Reitz
` (10 preceding siblings ...)
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 11/16] block: Add list of all BlockDriverStates Max Reitz
@ 2016-01-27 17:59 ` Max Reitz
2016-01-28 3:33 ` Fam Zheng
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 13/16] block: Add blk_remove_all_bs() Max Reitz
` (4 subsequent siblings)
16 siblings, 1 reply; 38+ messages in thread
From: Max Reitz @ 2016-01-27 17:59 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Max Reitz, Paolo Bonzini,
Alberto Garcia, John Snow
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
blockdev.c | 26 ++++++++++++++++++++++++++
include/block/block_int.h | 4 ++++
stubs/Makefile.objs | 1 +
stubs/blockdev-close-all-bdrv-states.c | 5 +++++
4 files changed, 36 insertions(+)
create mode 100644 stubs/blockdev-close-all-bdrv-states.c
diff --git a/blockdev.c b/blockdev.c
index 09d4621..ac93f43 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",
@@ -702,6 +705,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)
{
@@ -3875,12 +3891,15 @@ 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)) {
if (blk) {
blk_unref(blk);
} else {
+ QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list);
bdrv_unref(bs);
}
error_setg(errp, "blockdev-add doesn't support encrypted devices");
@@ -3945,11 +3964,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 1e4c518..dd00d12 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -445,6 +445,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;
@@ -707,4 +709,6 @@ bool bdrv_requests_pending(BlockDriverState *bs);
void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out);
void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in);
+void blockdev_close_all_bdrv_states(void);
+
#endif /* BLOCK_INT_H */
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index d7898a0..e922de9 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.7.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v8 12/16] blockdev: Keep track of monitor-owned BDS
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 12/16] blockdev: Keep track of monitor-owned BDS Max Reitz
@ 2016-01-28 3:33 ` Fam Zheng
2016-01-29 13:44 ` Max Reitz
0 siblings, 1 reply; 38+ messages in thread
From: Fam Zheng @ 2016-01-28 3:33 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Alberto Garcia, qemu-block, John Snow, qemu-devel,
Paolo Bonzini
On Wed, 01/27 18:59, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> blockdev.c | 26 ++++++++++++++++++++++++++
> include/block/block_int.h | 4 ++++
> stubs/Makefile.objs | 1 +
> stubs/blockdev-close-all-bdrv-states.c | 5 +++++
> 4 files changed, 36 insertions(+)
> create mode 100644 stubs/blockdev-close-all-bdrv-states.c
>
> diff --git a/blockdev.c b/blockdev.c
> index 09d4621..ac93f43 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",
> @@ -702,6 +705,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)
> {
> @@ -3875,12 +3891,15 @@ 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)) {
> if (blk) {
> blk_unref(blk);
> } else {
> + QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list);
> bdrv_unref(bs);
> }
> error_setg(errp, "blockdev-add doesn't support encrypted devices");
> @@ -3945,11 +3964,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;
> + }
Is this an extra restriction added by this patch? Deserve some words in the
commit message?
> }
>
> 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 1e4c518..dd00d12 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -445,6 +445,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;
>
> @@ -707,4 +709,6 @@ bool bdrv_requests_pending(BlockDriverState *bs);
> void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out);
> void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in);
>
> +void blockdev_close_all_bdrv_states(void);
> +
> #endif /* BLOCK_INT_H */
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index d7898a0..e922de9 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.7.0
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v8 12/16] blockdev: Keep track of monitor-owned BDS
2016-01-28 3:33 ` Fam Zheng
@ 2016-01-29 13:44 ` Max Reitz
2016-01-29 13:49 ` Kevin Wolf
0 siblings, 1 reply; 38+ messages in thread
From: Max Reitz @ 2016-01-29 13:44 UTC (permalink / raw)
To: Fam Zheng
Cc: Kevin Wolf, Alberto Garcia, qemu-block, John Snow, qemu-devel,
Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 4689 bytes --]
On 28.01.2016 04:33, Fam Zheng wrote:
> On Wed, 01/27 18:59, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> blockdev.c | 26 ++++++++++++++++++++++++++
>> include/block/block_int.h | 4 ++++
>> stubs/Makefile.objs | 1 +
>> stubs/blockdev-close-all-bdrv-states.c | 5 +++++
>> 4 files changed, 36 insertions(+)
>> create mode 100644 stubs/blockdev-close-all-bdrv-states.c
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 09d4621..ac93f43 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",
>> @@ -702,6 +705,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)
>> {
>> @@ -3875,12 +3891,15 @@ 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)) {
>> if (blk) {
>> blk_unref(blk);
>> } else {
>> + QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list);
>> bdrv_unref(bs);
>> }
>> error_setg(errp, "blockdev-add doesn't support encrypted devices");
>> @@ -3945,11 +3964,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;
>> + }
>
> Is this an extra restriction added by this patch?
I hope not. This is just an additional check that should not change
behavior; if it does, we did something wrong.
> Deserve some words in the
> commit message?
I'll see if I can come up with something.
Max
>> }
>>
>> 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 1e4c518..dd00d12 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -445,6 +445,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;
>>
>> @@ -707,4 +709,6 @@ bool bdrv_requests_pending(BlockDriverState *bs);
>> void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out);
>> void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in);
>>
>> +void blockdev_close_all_bdrv_states(void);
>> +
>> #endif /* BLOCK_INT_H */
>> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
>> index d7898a0..e922de9 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.7.0
>>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v8 12/16] blockdev: Keep track of monitor-owned BDS
2016-01-29 13:44 ` Max Reitz
@ 2016-01-29 13:49 ` Kevin Wolf
0 siblings, 0 replies; 38+ messages in thread
From: Kevin Wolf @ 2016-01-29 13:49 UTC (permalink / raw)
To: Max Reitz
Cc: Alberto Garcia, qemu-block, qemu-devel, Paolo Bonzini, Fam Zheng,
John Snow
[-- Attachment #1: Type: text/plain, Size: 3240 bytes --]
Am 29.01.2016 um 14:44 hat Max Reitz geschrieben:
> On 28.01.2016 04:33, Fam Zheng wrote:
> > On Wed, 01/27 18:59, Max Reitz wrote:
> >> Signed-off-by: Max Reitz <mreitz@redhat.com>
> >> ---
> >> blockdev.c | 26 ++++++++++++++++++++++++++
> >> include/block/block_int.h | 4 ++++
> >> stubs/Makefile.objs | 1 +
> >> stubs/blockdev-close-all-bdrv-states.c | 5 +++++
> >> 4 files changed, 36 insertions(+)
> >> create mode 100644 stubs/blockdev-close-all-bdrv-states.c
> >>
> >> diff --git a/blockdev.c b/blockdev.c
> >> index 09d4621..ac93f43 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",
> >> @@ -702,6 +705,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)
> >> {
> >> @@ -3875,12 +3891,15 @@ 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)) {
> >> if (blk) {
> >> blk_unref(blk);
> >> } else {
> >> + QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list);
> >> bdrv_unref(bs);
> >> }
> >> error_setg(errp, "blockdev-add doesn't support encrypted devices");
> >> @@ -3945,11 +3964,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;
> >> + }
> >
> > Is this an extra restriction added by this patch?
>
> I hope not. This is just an additional check that should not change
> behavior; if it does, we did something wrong.
Actually, if you were to respin the series, you could remove the check
that it replaces:
if (bs->refcnt > 1 || !QLIST_EMPTY(&bs->parents)) {
The QLIST_EMPTY() check is trying to achieve the same as what you
introduce in a clean way now. It doesn't hurt at the moment, but I had
to get rid of the QLIST_EMPTY() check when I tried to convert BB to
BdrvChild.
Kevin
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v8 13/16] block: Add blk_remove_all_bs()
2016-01-27 17:59 [Qemu-devel] [PATCH v8 00/16] block: Rework bdrv_close_all() Max Reitz
` (11 preceding siblings ...)
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 12/16] blockdev: Keep track of monitor-owned BDS Max Reitz
@ 2016-01-27 17:59 ` Max Reitz
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 14/16] block: Rewrite bdrv_close_all() Max Reitz
` (3 subsequent siblings)
16 siblings, 0 replies; 38+ messages in thread
From: Max Reitz @ 2016-01-27 17:59 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Max Reitz, Paolo Bonzini,
Alberto Garcia, John Snow
When bdrv_close_all() is called, instead of force-closing all root
BlockDriverStates, it is better to just drop the reference from all
BlockBackends and let them be closed automatically. This prevents BDS
from getting closed that are still referenced by other BDS, which may
result in loss of cached data.
This patch adds a function for doing that, but does not yet incorporate
it in bdrv_close_all().
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block/block-backend.c | 15 +++++++++++++++
include/sysemu/block-backend.h | 1 +
2 files changed, 16 insertions(+)
diff --git a/block/block-backend.c b/block/block-backend.c
index 7f5ad59..ebdf78a 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -223,6 +223,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 ae4efb4..ec30331 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.7.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v8 14/16] block: Rewrite bdrv_close_all()
2016-01-27 17:59 [Qemu-devel] [PATCH v8 00/16] block: Rework bdrv_close_all() Max Reitz
` (12 preceding siblings ...)
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 13/16] block: Add blk_remove_all_bs() Max Reitz
@ 2016-01-27 17:59 ` Max Reitz
2016-01-28 4:17 ` Fam Zheng
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 15/16] iotests: Add test for multiple BB on BDS tree Max Reitz
` (2 subsequent siblings)
16 siblings, 1 reply; 38+ messages in thread
From: Max Reitz @ 2016-01-27 17:59 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Max Reitz, Paolo Bonzini,
Alberto Garcia, John Snow
This patch rewrites bdrv_close_all(): Until now, all root BDSs have been
force-closed. This is bad because it can lead to cached data not being
flushed to disk.
Instead, try to make all reference holders relinquish their reference
voluntarily:
1. All BlockBackend users are handled by making all BBs simply eject
their BDS tree. Since a BDS can never be on top of a BB, this will
not cause any of the issues as seen with the force-closing of BDSs.
The references will be relinquished and any further access to the BB
will fail gracefully.
2. All BDSs which are owned by the monitor itself (because they do not
have a BB) are relinquished next.
3. Besides BBs and the monitor, block jobs and other BDSs are the only
things left that can hold a reference to BDSs. After every remaining
block job has been canceled, there should not be any BDSs left (and
the loop added here will always terminate (as long as NDEBUG is not
defined), because either all_bdrv_states will be empty or there will
not be any block job left to cancel, failing the assertion).
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 45 +++++++++++++++++++++++++++++++++++++--------
1 file changed, 37 insertions(+), 8 deletions(-)
diff --git a/block.c b/block.c
index f8dd4a3..478e0db 100644
--- a/block.c
+++ b/block.c
@@ -2145,9 +2145,7 @@ static void bdrv_close(BlockDriverState *bs)
{
BdrvAioNotifier *ban, *ban_next;
- if (bs->job) {
- block_job_cancel_sync(bs->job);
- }
+ assert(!bs->job);
/* Disable I/O limits and drain all pending throttled requests */
if (bs->throttle_state) {
@@ -2213,13 +2211,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.7.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v8 14/16] block: Rewrite bdrv_close_all()
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 14/16] block: Rewrite bdrv_close_all() Max Reitz
@ 2016-01-28 4:17 ` Fam Zheng
2016-01-29 13:54 ` Max Reitz
0 siblings, 1 reply; 38+ messages in thread
From: Fam Zheng @ 2016-01-28 4:17 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Alberto Garcia, qemu-block, John Snow, qemu-devel,
Paolo Bonzini
On Wed, 01/27 18:59, Max Reitz wrote:
> This patch rewrites bdrv_close_all(): Until now, all root BDSs have been
> force-closed. This is bad because it can lead to cached data not being
> flushed to disk.
>
> Instead, try to make all reference holders relinquish their reference
> voluntarily:
>
> 1. All BlockBackend users are handled by making all BBs simply eject
> their BDS tree. Since a BDS can never be on top of a BB, this will
> not cause any of the issues as seen with the force-closing of BDSs.
> The references will be relinquished and any further access to the BB
> will fail gracefully.
> 2. All BDSs which are owned by the monitor itself (because they do not
> have a BB) are relinquished next.
> 3. Besides BBs and the monitor, block jobs and other BDSs are the only
> things left that can hold a reference to BDSs. After every remaining
> block job has been canceled, there should not be any BDSs left (and
> the loop added here will always terminate (as long as NDEBUG is not
> defined), because either all_bdrv_states will be empty or there will
> not be any block job left to cancel, failing the assertion).
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block.c | 45 +++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/block.c b/block.c
> index f8dd4a3..478e0db 100644
> --- a/block.c
> +++ b/block.c
> @@ -2145,9 +2145,7 @@ static void bdrv_close(BlockDriverState *bs)
> {
> BdrvAioNotifier *ban, *ban_next;
>
> - if (bs->job) {
> - block_job_cancel_sync(bs->job);
> - }
> + assert(!bs->job);
>
> /* Disable I/O limits and drain all pending throttled requests */
> if (bs->throttle_state) {
> @@ -2213,13 +2211,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();
This (monitor before BB) doesn't match the order in the commit message (BB
before monitor).
> +
> + /* 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);
Why is this safe without acquiring aio_context? But oh wait, completions of
block jobs are defered to main loop BH, so I think to release the reference,
aio_poll(qemu_get_aio_context(), ...) is the right thing to do.
This is also the problem in block_job_cancel_sync, which can dead loop waiting
for job->completed flag, without processing main loop BH.
Fam
> + }
> + bdrv_unref(bs);
> }
> }
>
> --
> 2.7.0
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v8 14/16] block: Rewrite bdrv_close_all()
2016-01-28 4:17 ` Fam Zheng
@ 2016-01-29 13:54 ` Max Reitz
0 siblings, 0 replies; 38+ messages in thread
From: Max Reitz @ 2016-01-29 13:54 UTC (permalink / raw)
To: Fam Zheng
Cc: Kevin Wolf, Alberto Garcia, qemu-block, John Snow, qemu-devel,
Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 4548 bytes --]
On 28.01.2016 05:17, Fam Zheng wrote:
> On Wed, 01/27 18:59, Max Reitz wrote:
>> This patch rewrites bdrv_close_all(): Until now, all root BDSs have been
>> force-closed. This is bad because it can lead to cached data not being
>> flushed to disk.
>>
>> Instead, try to make all reference holders relinquish their reference
>> voluntarily:
>>
>> 1. All BlockBackend users are handled by making all BBs simply eject
>> their BDS tree. Since a BDS can never be on top of a BB, this will
>> not cause any of the issues as seen with the force-closing of BDSs.
>> The references will be relinquished and any further access to the BB
>> will fail gracefully.
>> 2. All BDSs which are owned by the monitor itself (because they do not
>> have a BB) are relinquished next.
>> 3. Besides BBs and the monitor, block jobs and other BDSs are the only
>> things left that can hold a reference to BDSs. After every remaining
>> block job has been canceled, there should not be any BDSs left (and
>> the loop added here will always terminate (as long as NDEBUG is not
>> defined), because either all_bdrv_states will be empty or there will
>> not be any block job left to cancel, failing the assertion).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>> block.c | 45 +++++++++++++++++++++++++++++++++++++--------
>> 1 file changed, 37 insertions(+), 8 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index f8dd4a3..478e0db 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2145,9 +2145,7 @@ static void bdrv_close(BlockDriverState *bs)
>> {
>> BdrvAioNotifier *ban, *ban_next;
>>
>> - if (bs->job) {
>> - block_job_cancel_sync(bs->job);
>> - }
>> + assert(!bs->job);
>>
>> /* Disable I/O limits and drain all pending throttled requests */
>> if (bs->throttle_state) {
>> @@ -2213,13 +2211,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();
>
> This (monitor before BB) doesn't match the order in the commit message (BB
> before monitor).
Will ask random.org whether to change the order here or in the commit
message. :-)
>> +
>> + /* 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);
>
> Why is this safe without acquiring aio_context? But oh wait, completions of
> block jobs are defered to main loop BH, so I think to release the reference,
> aio_poll(qemu_get_aio_context(), ...) is the right thing to do.
Actually, I think, commit 94db6d2d30962cc0422a69c88c3b3e9981b33e50 made
this loop completely unnecessary. Will investigate.
Max
> This is also the problem in block_job_cancel_sync, which can dead loop waiting
> for job->completed flag, without processing main loop BH.
>
> Fam
>
>> + }
>> + bdrv_unref(bs);
>> }
>> }
>>
>> --
>> 2.7.0
>>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v8 15/16] iotests: Add test for multiple BB on BDS tree
2016-01-27 17:59 [Qemu-devel] [PATCH v8 00/16] block: Rework bdrv_close_all() Max Reitz
` (13 preceding siblings ...)
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 14/16] block: Rewrite bdrv_close_all() Max Reitz
@ 2016-01-27 17:59 ` Max Reitz
2016-01-27 18:00 ` [Qemu-devel] [PATCH v8 16/16] iotests: Add test for block jobs and BDS ejection Max Reitz
2016-01-29 13:45 ` [Qemu-devel] [PATCH v8 00/16] block: Rework bdrv_close_all() Kevin Wolf
16 siblings, 0 replies; 38+ messages in thread
From: Max Reitz @ 2016-01-27 17:59 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Max Reitz, Paolo Bonzini,
Alberto Garcia, John Snow
This adds a test for having multiple BlockBackends in one BDS tree. In
this case, there is one BB for the protocol BDS and one BB for the
format BDS in a simple two-BDS tree (with the protocol BDS and BB added
first).
When bdrv_close_all() is executed, no cached data from any BDS should be
lost; the protocol BDS may not be closed until the format BDS is closed.
Otherwise, metadata updates may be lost.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
tests/qemu-iotests/117 | 86 ++++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/117.out | 14 ++++++++
tests/qemu-iotests/group | 1 +
3 files changed, 101 insertions(+)
create mode 100755 tests/qemu-iotests/117
create mode 100644 tests/qemu-iotests/117.out
diff --git a/tests/qemu-iotests/117 b/tests/qemu-iotests/117
new file mode 100755
index 0000000..7881fc7
--- /dev/null
+++ b/tests/qemu-iotests/117
@@ -0,0 +1,86 @@
+#!/bin/bash
+#
+# Test case for shared BDS between backend trees
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1 # failure is the default!
+
+_cleanup()
+{
+ _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+_make_test_img 64k
+
+_launch_qemu
+
+_send_qemu_cmd $QEMU_HANDLE \
+ "{ 'execute': 'qmp_capabilities' }" \
+ 'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+ "{ 'execute': 'blockdev-add',
+ 'arguments': { 'options': { 'id': 'protocol',
+ 'driver': 'file',
+ 'filename': '$TEST_IMG' } } }" \
+ 'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+ "{ 'execute': 'blockdev-add',
+ 'arguments': { 'options': { 'id': 'format',
+ 'driver': '$IMGFMT',
+ 'file': 'protocol' } } }" \
+ 'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+ "{ 'execute': 'human-monitor-command',
+ 'arguments': { 'command-line': 'qemu-io format \"write -P 42 0 64k\"' } }" \
+ 'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+ "{ 'execute': 'quit' }" \
+ 'return'
+
+wait=1 _cleanup_qemu
+
+_check_test_img
+
+$QEMU_IO -c 'read -P 42 0 64k' "$TEST_IMG" | _filter_qemu_io
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/117.out b/tests/qemu-iotests/117.out
new file mode 100644
index 0000000..f52dc1a
--- /dev/null
+++ b/tests/qemu-iotests/117.out
@@ -0,0 +1,14 @@
+QA output created by 117
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
+{"return": {}}
+{"return": {}}
+{"return": {}}
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"}
+No errors were found on the image.
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index ff1ff0d..e89f076 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.7.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v8 16/16] iotests: Add test for block jobs and BDS ejection
2016-01-27 17:59 [Qemu-devel] [PATCH v8 00/16] block: Rework bdrv_close_all() Max Reitz
` (14 preceding siblings ...)
2016-01-27 17:59 ` [Qemu-devel] [PATCH v8 15/16] iotests: Add test for multiple BB on BDS tree Max Reitz
@ 2016-01-27 18:00 ` Max Reitz
2016-01-29 13:45 ` [Qemu-devel] [PATCH v8 00/16] block: Rework bdrv_close_all() Kevin Wolf
16 siblings, 0 replies; 38+ messages in thread
From: Max Reitz @ 2016-01-27 18:00 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Max Reitz, Paolo Bonzini,
Alberto Garcia, John Snow
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/141 | 186 +++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/141.out | 59 ++++++++++++++
tests/qemu-iotests/group | 1 +
3 files changed, 246 insertions(+)
create mode 100755 tests/qemu-iotests/141
create mode 100644 tests/qemu-iotests/141.out
diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141
new file mode 100755
index 0000000..5788281
--- /dev/null
+++ b/tests/qemu-iotests/141
@@ -0,0 +1,186 @@
+#!/bin/bash
+#
+# Test case for ejecting BDSs with block jobs still running on them
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1 # failure is the default!
+
+_cleanup()
+{
+ _cleanup_test_img
+ rm -f "$TEST_DIR/{b,m,o}.$IMGFMT"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+# Needs backing file and backing format support
+_supported_fmt qcow2 qed
+_supported_proto file
+_supported_os Linux
+
+
+test_blockjob()
+{
+ _send_qemu_cmd $QEMU_HANDLE \
+ "{'execute': 'blockdev-add',
+ 'arguments': {
+ 'options': {
+ 'id': 'drv0',
+ 'driver': '$IMGFMT',
+ 'file': {
+ 'driver': 'file',
+ 'filename': '$TEST_IMG'
+ }}}}" \
+ 'return'
+
+ _send_qemu_cmd $QEMU_HANDLE \
+ "$1" \
+ "$2" \
+ | _filter_img_create
+
+ # We want this to return an error because the block job is still running
+ _send_qemu_cmd $QEMU_HANDLE \
+ "{'execute': 'x-blockdev-remove-medium',
+ 'arguments': {'device': 'drv0'}}" \
+ 'error'
+
+ _send_qemu_cmd $QEMU_HANDLE \
+ "{'execute': 'block-job-cancel',
+ 'arguments': {'device': 'drv0'}}" \
+ "$3"
+
+ _send_qemu_cmd $QEMU_HANDLE \
+ "{'execute': 'x-blockdev-del',
+ 'arguments': {'id': 'drv0'}}" \
+ 'return'
+}
+
+
+TEST_IMG="$TEST_DIR/b.$IMGFMT" _make_test_img 1M
+TEST_IMG="$TEST_DIR/m.$IMGFMT" _make_test_img -b "$TEST_DIR/b.$IMGFMT" 1M
+_make_test_img -b "$TEST_DIR/m.$IMGFMT" 1M
+
+_launch_qemu -nodefaults
+
+_send_qemu_cmd $QEMU_HANDLE \
+ "{'execute': 'qmp_capabilities'}" \
+ 'return'
+
+echo
+echo '=== Testing drive-backup ==='
+echo
+
+# drive-backup will not send BLOCK_JOB_READY by itself, and cancelling the job
+# will consequently result in BLOCK_JOB_CANCELLED being emitted.
+
+test_blockjob \
+ "{'execute': 'drive-backup',
+ 'arguments': {'device': 'drv0',
+ 'target': '$TEST_DIR/o.$IMGFMT',
+ 'format': '$IMGFMT',
+ 'sync': 'none'}}" \
+ 'return' \
+ 'BLOCK_JOB_CANCELLED'
+
+echo
+echo '=== Testing drive-mirror ==='
+echo
+
+# drive-mirror will send BLOCK_JOB_READY basically immediately, and cancelling
+# the job will consequently result in BLOCK_JOB_COMPLETED being emitted.
+
+test_blockjob \
+ "{'execute': 'drive-mirror',
+ 'arguments': {'device': 'drv0',
+ 'target': '$TEST_DIR/o.$IMGFMT',
+ 'format': '$IMGFMT',
+ 'sync': 'none'}}" \
+ 'BLOCK_JOB_READY' \
+ 'BLOCK_JOB_COMPLETED'
+
+echo
+echo '=== Testing active block-commit ==='
+echo
+
+# An active block-commit will send BLOCK_JOB_READY basically immediately, and
+# cancelling the job will consequently result in BLOCK_JOB_COMPLETED being
+# emitted.
+
+test_blockjob \
+ "{'execute': 'block-commit',
+ 'arguments': {'device': 'drv0'}}" \
+ 'BLOCK_JOB_READY' \
+ 'BLOCK_JOB_COMPLETED'
+
+echo
+echo '=== Testing non-active block-commit ==='
+echo
+
+# Give block-commit something to work on, otherwise it would be done
+# immediately, send a BLOCK_JOB_COMPLETED and ejecting the BDS would work just
+# fine without the block job still running.
+
+$QEMU_IO -c 'write 0 1M' "$TEST_DIR/m.$IMGFMT" | _filter_qemu_io
+
+test_blockjob \
+ "{'execute': 'block-commit',
+ 'arguments': {'device': 'drv0',
+ 'top': '$TEST_DIR/m.$IMGFMT',
+ 'speed': 1}}" \
+ 'return' \
+ 'BLOCK_JOB_CANCELLED'
+
+echo
+echo '=== Testing block-stream ==='
+echo
+
+# Give block-stream something to work on, otherwise it would be done
+# immediately, send a BLOCK_JOB_COMPLETED and ejecting the BDS would work just
+# fine without the block job still running.
+
+$QEMU_IO -c 'write 0 1M' "$TEST_DIR/b.$IMGFMT" | _filter_qemu_io
+
+# With some data to stream (and @speed set to 1), block-stream will not complete
+# until we send the block-job-cancel command. Therefore, no event other than
+# BLOCK_JOB_CANCELLED will be emitted.
+
+test_blockjob \
+ "{'execute': 'block-stream',
+ 'arguments': {'device': 'drv0',
+ 'speed': 1}}" \
+ 'return' \
+ 'BLOCK_JOB_CANCELLED'
+
+_cleanup_qemu
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out
new file mode 100644
index 0000000..adceac1
--- /dev/null
+++ b/tests/qemu-iotests/141.out
@@ -0,0 +1,59 @@
+QA output created by 141
+Formatting 'TEST_DIR/b.IMGFMT', fmt=IMGFMT size=1048576
+Formatting 'TEST_DIR/m.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/b.IMGFMT
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/m.IMGFMT
+{"return": {}}
+
+=== Testing drive-backup ===
+
+{"return": {}}
+Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT
+{"return": {}}
+{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: backup"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 0, "speed": 0, "type": "backup"}}
+{"return": {}}
+
+=== Testing drive-mirror ===
+
+{"return": {}}
+Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "mirror"}}
+{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: mirror"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "mirror"}}
+{"return": {}}
+
+=== Testing active block-commit ===
+
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}}
+{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: commit"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}}
+{"return": {}}
+
+=== Testing non-active block-commit ===
+
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": {}}
+{"return": {}}
+{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: commit"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 524288, "speed": 1, "type": "commit"}}
+{"return": {}}
+
+=== Testing block-stream ===
+
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": {}}
+{"return": {}}
+{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: stream"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 524288, "speed": 1, "type": "stream"}}
+{"return": {}}
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index e89f076..cbc970a 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -143,5 +143,6 @@
138 rw auto quick
139 rw auto quick
140 rw auto quick
+141 rw auto quick
142 auto
143 auto quick
--
2.7.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v8 00/16] block: Rework bdrv_close_all()
2016-01-27 17:59 [Qemu-devel] [PATCH v8 00/16] block: Rework bdrv_close_all() Max Reitz
` (15 preceding siblings ...)
2016-01-27 18:00 ` [Qemu-devel] [PATCH v8 16/16] iotests: Add test for block jobs and BDS ejection Max Reitz
@ 2016-01-29 13:45 ` Kevin Wolf
16 siblings, 0 replies; 38+ messages in thread
From: Kevin Wolf @ 2016-01-29 13:45 UTC (permalink / raw)
To: Max Reitz
Cc: Alberto Garcia, qemu-block, qemu-devel, Paolo Bonzini, Fam Zheng,
John Snow
Am 27.01.2016 um 18:59 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.
Apart from patch 5:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 38+ messages in thread