Re: [Qemu-devel] [PATCH v2] RFC: Add blockdev-del QMP command
Hi, On Wed, Feb 12, 2014 at 6:36 PM, Ian Main im...@redhat.com wrote: This is the sister command to blockdev-add. In Fam's example he uses the drive_del HMP command to clean up but it would be much nicer to have a way to do this via QMP. Is there any news on this subject? It seems like we still need to clean up with drive_del from HMP command when hotremoving a disk. Did I miss something? Thanks, -- William
Re: [Qemu-devel] [PATCH v2] RFC: Add blockdev-del QMP command
Kevin Wolf kw...@redhat.com writes:
Am 12.02.2014 um 18:36 hat Ian Main geschrieben:
This is the sister command to blockdev-add. In Fam's example he uses
the drive_del HMP command to clean up but it would be much nicer to
have a way to do this via QMP.
Signed-off-by: Ian Main im...@redhat.com
diff --git a/qapi-schema.json b/qapi-schema.json
index d22651c..01186cd 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4469,3 +4469,14 @@
# Since: 1.7
##
{ 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
+
+##
+# @blockdev-del:
+#
+# Delete a block device.
+#
+# @device: Identifier for the block device to be deleted.
+#
+# Since: 2.0
+##
+{ 'command': 'blockdev-del', 'data': { 'device': 'str' } }
I believe the full documentation should go here as well, not just in
qmp-commands.hx.
Yes.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index c3ee46a..f08045d 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3442,6 +3442,36 @@ Example (2):
EQMP
{
+.name = blockdev-del,
+.args_type = device:s,
+.mhandler.cmd_new = qmp_marshal_input_blockdev_del,
+},
+
+SQMP
+blockdev-del
+
+
+Remove host block device. The result is that guest generated IO is no
+longer submitted against the host device underlying the disk. Once a
+drive has been deleted, the QEMU Block layer returns -EIO which results
+in IO errors in the guest for applications that are reading/writing to
+the device. These errors are always reported to the guest, regardless
+of the drive's error actions (drive options rerror, werror).
I think we wanted to have different semantics for blockdev-del.
Specifically, it is a backend command that should have no effect on
users of that backend.
Let me paste and comment on some notes I made in a previous blockdev
discussion:
* Make sure that an explicit blockdev-del is needed to remove the
BDS; it shouldn't happen automagically just because the guest
device was unplugged
[done]
* By default, return an error for blockdev-del if reference count 1
Yes.
Deleting a block device that is not in use is a perfectly innocent
operation. Deleting one that is in use isn't; it can corrupt data.
Innocent and dangerous operations should be separate. A force option is
an acceptable separator. Having a separate command would be another.
[ The assumption is here that one reference is held by the
monitor/external user, which isn't true today to my knowledge ]
Here's my working hypothesis on this matter:
blockdev_init() initializes the reference count to 1. This is for the
reference it puts into the list of drives. It also returns a weak
reference.
We pair blockdev_init() with a drive_put_ref(). drive_put_ref() knows
it must be paired with blockdev_init() when the reference count is 1.
Then it deletes from drives in addition to decrementing the count.
Actually, nothing else ever uses the reference count since commit
fa510eb switch block jobs over to the BDS reference count. Worth a
cleanup.
- But have a force option that closes the image file, even if it
breaks the remaining users (e.g. uncooperative guest that doesn't
release its PCI device)
[ Here we need working refcounting including the external user,
because otherwise we don't free the (closed) BDS even when the
guest device is unplugged. It is an open question whether and
how BDSes without an external reference are shown in the
monitor. ]
Despite its name, the purpose of drive_del is not really deleting a
drive, it's revoking access to the host resources backing a drive.
drive_del doesn't delete the drive, it merely detaches the
BlockDriverState from its host resources, and schedules it for automatic
deletion. That way, references beyond drive_del's reach remain valid.
Special case: if it's not in use by a front end, delete it right away.
Assumes that references beyond drive_del's reach exist only while it is
in use by a frone end.
I think there are basic operations struggling to get out of this mess:
* Create block device (and put it into drives)
* Delete block device (and remove it from drives)
Fails if the block device is in use
* Revoke access to host resources
* Start use of block device
* Stop use of block device
* Prevent mixing blockdev-add with drive_del and vice versa
- Ideally drive_add BDSes are exactly those with a DriveInfo
[ not true today, but DriveInfo.enable_auto_del can be used to
distinguish them ]
So I believe we still have some design work to do before we can actually
implement this. I would prefer not to merge this for 2.0.
I'm afraid you're right.
Re: [Qemu-devel] [PATCH v2] RFC: Add blockdev-del QMP command
Am 14.02.2014 um 20:17 hat Ian Main geschrieben:
On Thu, Feb 13, 2014 at 09:59:40AM +0100, Kevin Wolf wrote:
Am 12.02.2014 um 18:36 hat Ian Main geschrieben:
This is the sister command to blockdev-add. In Fam's example he uses
the drive_del HMP command to clean up but it would be much nicer to
have a way to do this via QMP.
Signed-off-by: Ian Main im...@redhat.com
diff --git a/qapi-schema.json b/qapi-schema.json
index d22651c..01186cd 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4469,3 +4469,14 @@
# Since: 1.7
##
{ 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
+
+##
+# @blockdev-del:
+#
+# Delete a block device.
+#
+# @device: Identifier for the block device to be deleted.
+#
+# Since: 2.0
+##
+{ 'command': 'blockdev-del', 'data': { 'device': 'str' } }
I believe the full documentation should go here as well, not just in
qmp-commands.hx.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index c3ee46a..f08045d 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3442,6 +3442,36 @@ Example (2):
EQMP
{
+.name = blockdev-del,
+.args_type = device:s,
+.mhandler.cmd_new = qmp_marshal_input_blockdev_del,
+},
+
+SQMP
+blockdev-del
+
+
+Remove host block device. The result is that guest generated IO is no
+longer submitted against the host device underlying the disk. Once a
+drive has been deleted, the QEMU Block layer returns -EIO which results
+in IO errors in the guest for applications that are reading/writing to
+the device. These errors are always reported to the guest, regardless
+of the drive's error actions (drive options rerror, werror).
I think we wanted to have different semantics for blockdev-del.
Specifically, it is a backend command that should have no effect on
users of that backend.
Let me paste and comment on some notes I made in a previous blockdev
discussion:
* Make sure that an explicit blockdev-del is needed to remove the
BDS; it shouldn't happen automagically just because the guest
device was unplugged
[done]
* By default, return an error for blockdev-del if reference count 1
[ The assumption is here that one reference is held by the
monitor/external user, which isn't true today to my knowledge ]
- But have a force option that closes the image file, even if it
breaks the remaining users (e.g. uncooperative guest that doesn't
release its PCI device)
[ Here we need working refcounting including the external user,
because otherwise we don't free the (closed) BDS even when the
guest device is unplugged. It is an open question whether and
how BDSes without an external reference are shown in the
monitor. ]
* Prevent mixing blockdev-add with drive_del and vice versa
- Ideally drive_add BDSes are exactly those with a DriveInfo
[ not true today, but DriveInfo.enable_auto_del can be used to
distinguish them ]
So I believe we still have some design work to do before we can actually
implement this. I would prefer not to merge this for 2.0.
Kevin
Are we only changing the semantics/implementation of the API, or is the
API itself going to change with these improvements? If that were the
case wouldn't it make some sense to get people using blockdev-del now
and update the semantics later? As it is now consumers will just end
up using blockdev-add/drive_del.
It should be obvious that you can't change the semantics after the fact.
The API is not just a command name and a set of arguments, but clearly
also what happens when you invoke the command.
Doing one thing now and changing it later to behave differently will
break any management tool because it wouldn't be able to know what the
command means for the specific qemu version it's talking to.
Kevin
Re: [Qemu-devel] [PATCH v2] RFC: Add blockdev-del QMP command
On Thu, Feb 13, 2014 at 09:59:40AM +0100, Kevin Wolf wrote:
Am 12.02.2014 um 18:36 hat Ian Main geschrieben:
This is the sister command to blockdev-add. In Fam's example he uses
the drive_del HMP command to clean up but it would be much nicer to
have a way to do this via QMP.
Signed-off-by: Ian Main im...@redhat.com
diff --git a/qapi-schema.json b/qapi-schema.json
index d22651c..01186cd 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4469,3 +4469,14 @@
# Since: 1.7
##
{ 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
+
+##
+# @blockdev-del:
+#
+# Delete a block device.
+#
+# @device: Identifier for the block device to be deleted.
+#
+# Since: 2.0
+##
+{ 'command': 'blockdev-del', 'data': { 'device': 'str' } }
I believe the full documentation should go here as well, not just in
qmp-commands.hx.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index c3ee46a..f08045d 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3442,6 +3442,36 @@ Example (2):
EQMP
{
+.name = blockdev-del,
+.args_type = device:s,
+.mhandler.cmd_new = qmp_marshal_input_blockdev_del,
+},
+
+SQMP
+blockdev-del
+
+
+Remove host block device. The result is that guest generated IO is no
+longer submitted against the host device underlying the disk. Once a
+drive has been deleted, the QEMU Block layer returns -EIO which results
+in IO errors in the guest for applications that are reading/writing to
+the device. These errors are always reported to the guest, regardless
+of the drive's error actions (drive options rerror, werror).
I think we wanted to have different semantics for blockdev-del.
Specifically, it is a backend command that should have no effect on
users of that backend.
Let me paste and comment on some notes I made in a previous blockdev
discussion:
* Make sure that an explicit blockdev-del is needed to remove the
BDS; it shouldn't happen automagically just because the guest
device was unplugged
[done]
* By default, return an error for blockdev-del if reference count 1
[ The assumption is here that one reference is held by the
monitor/external user, which isn't true today to my knowledge ]
- But have a force option that closes the image file, even if it
breaks the remaining users (e.g. uncooperative guest that doesn't
release its PCI device)
[ Here we need working refcounting including the external user,
because otherwise we don't free the (closed) BDS even when the
guest device is unplugged. It is an open question whether and
how BDSes without an external reference are shown in the
monitor. ]
* Prevent mixing blockdev-add with drive_del and vice versa
- Ideally drive_add BDSes are exactly those with a DriveInfo
[ not true today, but DriveInfo.enable_auto_del can be used to
distinguish them ]
So I believe we still have some design work to do before we can actually
implement this. I would prefer not to merge this for 2.0.
Kevin
Are we only changing the semantics/implementation of the API, or is the
API itself going to change with these improvements? If that were the
case wouldn't it make some sense to get people using blockdev-del now
and update the semantics later? As it is now consumers will just end
up using blockdev-add/drive_del.
Ian
Re: [Qemu-devel] [PATCH v2] RFC: Add blockdev-del QMP command
Am 12.02.2014 um 18:36 hat Ian Main geschrieben:
This is the sister command to blockdev-add. In Fam's example he uses
the drive_del HMP command to clean up but it would be much nicer to
have a way to do this via QMP.
Signed-off-by: Ian Main im...@redhat.com
diff --git a/qapi-schema.json b/qapi-schema.json
index d22651c..01186cd 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4469,3 +4469,14 @@
# Since: 1.7
##
{ 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
+
+##
+# @blockdev-del:
+#
+# Delete a block device.
+#
+# @device: Identifier for the block device to be deleted.
+#
+# Since: 2.0
+##
+{ 'command': 'blockdev-del', 'data': { 'device': 'str' } }
I believe the full documentation should go here as well, not just in
qmp-commands.hx.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index c3ee46a..f08045d 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3442,6 +3442,36 @@ Example (2):
EQMP
{
+.name = blockdev-del,
+.args_type = device:s,
+.mhandler.cmd_new = qmp_marshal_input_blockdev_del,
+},
+
+SQMP
+blockdev-del
+
+
+Remove host block device. The result is that guest generated IO is no
+longer submitted against the host device underlying the disk. Once a
+drive has been deleted, the QEMU Block layer returns -EIO which results
+in IO errors in the guest for applications that are reading/writing to
+the device. These errors are always reported to the guest, regardless
+of the drive's error actions (drive options rerror, werror).
I think we wanted to have different semantics for blockdev-del.
Specifically, it is a backend command that should have no effect on
users of that backend.
Let me paste and comment on some notes I made in a previous blockdev
discussion:
* Make sure that an explicit blockdev-del is needed to remove the
BDS; it shouldn't happen automagically just because the guest
device was unplugged
[done]
* By default, return an error for blockdev-del if reference count 1
[ The assumption is here that one reference is held by the
monitor/external user, which isn't true today to my knowledge ]
- But have a force option that closes the image file, even if it
breaks the remaining users (e.g. uncooperative guest that doesn't
release its PCI device)
[ Here we need working refcounting including the external user,
because otherwise we don't free the (closed) BDS even when the
guest device is unplugged. It is an open question whether and
how BDSes without an external reference are shown in the
monitor. ]
* Prevent mixing blockdev-add with drive_del and vice versa
- Ideally drive_add BDSes are exactly those with a DriveInfo
[ not true today, but DriveInfo.enable_auto_del can be used to
distinguish them ]
So I believe we still have some design work to do before we can actually
implement this. I would prefer not to merge this for 2.0.
Kevin
[Qemu-devel] [PATCH v2] RFC: Add blockdev-del QMP command
This is the sister command to blockdev-add. In Fam's example he uses
the drive_del HMP command to clean up but it would be much nicer to
have a way to do this via QMP.
Signed-off-by: Ian Main im...@redhat.com
---
v2:
- s/blockdev-delete/blockdev-del
- Fixed example.
blockdev.c | 52 ++--
qapi-schema.json | 11 +++
qmp-commands.hx | 30 ++
3 files changed, 79 insertions(+), 14 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 7372721..96d0da1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1733,21 +1733,9 @@ void qmp_block_set_io_throttle(const char *device,
int64_t bps, int64_t bps_rd,
}
}
-int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
+/* This is called by both do_drive_del() and qmp_blockdev_del */
+static int drive_del_core(BlockDriverState *bs)
{
-const char *id = qdict_get_str(qdict, id);
-BlockDriverState *bs;
-
-bs = bdrv_find(id);
-if (!bs) {
-qerror_report(QERR_DEVICE_NOT_FOUND, id);
-return -1;
-}
-if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, NULL)) {
-qerror_report(QERR_DEVICE_IN_USE, id);
-return -1;
-}
-
/* quiesce block driver; prevent further io */
bdrv_drain_all();
bdrv_flush(bs);
@@ -1771,6 +1759,25 @@ int do_drive_del(Monitor *mon, const QDict *qdict,
QObject **ret_data)
return 0;
}
+int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+const char *id = qdict_get_str(qdict, id);
+BlockDriverState *bs;
+
+bs = bdrv_find(id);
+if (!bs) {
+qerror_report(QERR_DEVICE_NOT_FOUND, id);
+return -1;
+}
+
+if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, NULL)) {
+qerror_report(QERR_DEVICE_IN_USE, id);
+return -1;
+}
+
+return drive_del_core(bs);
+}
+
void qmp_block_resize(bool has_device, const char *device,
bool has_node_name, const char *node_name,
int64_t size, Error **errp)
@@ -2386,6 +2393,23 @@ fail:
qmp_output_visitor_cleanup(ov);
}
+void qmp_blockdev_del(const char *device, Error **errp)
+{
+BlockDriverState *bs;
+
+bs = bdrv_find(device);
+if (!bs) {
+error_setg(errp, Block device not found);
+return;
+}
+
+if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, NULL)) {
+return;
+}
+
+drive_del_core(bs);
+}
+
static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
{
BlockJobInfoList **prev = opaque;
diff --git a/qapi-schema.json b/qapi-schema.json
index d22651c..01186cd 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4469,3 +4469,14 @@
# Since: 1.7
##
{ 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
+
+##
+# @blockdev-del:
+#
+# Delete a block device.
+#
+# @device: Identifier for the block device to be deleted.
+#
+# Since: 2.0
+##
+{ 'command': 'blockdev-del', 'data': { 'device': 'str' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index c3ee46a..f08045d 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3442,6 +3442,36 @@ Example (2):
EQMP
{
+.name = blockdev-del,
+.args_type = device:s,
+.mhandler.cmd_new = qmp_marshal_input_blockdev_del,
+},
+
+SQMP
+blockdev-del
+
+
+Remove host block device. The result is that guest generated IO is no
+longer submitted against the host device underlying the disk. Once a
+drive has been deleted, the QEMU Block layer returns -EIO which results
+in IO errors in the guest for applications that are reading/writing to
+the device. These errors are always reported to the guest, regardless
+of the drive's error actions (drive options rerror, werror).
+
+Arguments:
+
+- device: Identifier of the block device (json-string)
+
+Example (1):
+
+- { execute: blockdev-del,
+arguments: { device: target0 }
+ }
+- { return: {} }
+
+EQMP
+
+{
.name = query-named-block-nodes,
.args_type = ,
.mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,
--
1.8.3.1
Re: [Qemu-devel] [PATCH v2] RFC: Add blockdev-del QMP command
On 02/12/2014 10:36 AM, Ian Main wrote: This is the sister command to blockdev-add. In Fam's example he uses the drive_del HMP command to clean up but it would be much nicer to have a way to do this via QMP. Signed-off-by: Ian Main im...@redhat.com --- Reviewed-by: Eric Blake ebl...@redhat.com Nice to finally get rid of another HMP command that libvirt had to use. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2] RFC: Add blockdev-del QMP command
On Wed, 02/12 09:36, Ian Main wrote:
This is the sister command to blockdev-add. In Fam's example he uses
the drive_del HMP command to clean up but it would be much nicer to
have a way to do this via QMP.
Signed-off-by: Ian Main im...@redhat.com
Thank you for doing this!
---
v2:
- s/blockdev-delete/blockdev-del
- Fixed example.
blockdev.c | 52 ++--
qapi-schema.json | 11 +++
qmp-commands.hx | 30 ++
3 files changed, 79 insertions(+), 14 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 7372721..96d0da1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1733,21 +1733,9 @@ void qmp_block_set_io_throttle(const char *device,
int64_t bps, int64_t bps_rd,
}
}
-int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
+/* This is called by both do_drive_del() and qmp_blockdev_del */
+static int drive_del_core(BlockDriverState *bs)
{
-const char *id = qdict_get_str(qdict, id);
-BlockDriverState *bs;
-
-bs = bdrv_find(id);
-if (!bs) {
-qerror_report(QERR_DEVICE_NOT_FOUND, id);
-return -1;
-}
-if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, NULL)) {
-qerror_report(QERR_DEVICE_IN_USE, id);
-return -1;
-}
-
/* quiesce block driver; prevent further io */
bdrv_drain_all();
bdrv_flush(bs);
@@ -1771,6 +1759,25 @@ int do_drive_del(Monitor *mon, const QDict *qdict,
QObject **ret_data)
return 0;
}
+int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+const char *id = qdict_get_str(qdict, id);
+BlockDriverState *bs;
+
+bs = bdrv_find(id);
+if (!bs) {
+qerror_report(QERR_DEVICE_NOT_FOUND, id);
+return -1;
+}
+
+if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, NULL)) {
+qerror_report(QERR_DEVICE_IN_USE, id);
+return -1;
+}
+
+return drive_del_core(bs);
+}
+
void qmp_block_resize(bool has_device, const char *device,
bool has_node_name, const char *node_name,
int64_t size, Error **errp)
@@ -2386,6 +2393,23 @@ fail:
qmp_output_visitor_cleanup(ov);
}
+void qmp_blockdev_del(const char *device, Error **errp)
+{
+BlockDriverState *bs;
+
+bs = bdrv_find(device);
+if (!bs) {
+error_setg(errp, Block device not found);
+return;
+}
+
+if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, NULL)) {
+return;
+}
+
+drive_del_core(bs);
+}
+
We could drop drive_del_core and move everything into qmp_blockdev_del().
In the original do_drive_del (maybe rename it to hmp_drive_del as a better
name?), call qmp_blockdev_del with a local_err, and report error with
qerror_report_err().
Thanks,
Fam
