| Message ID | 1392226573-14901-1-git-send-email-imain@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <imain@redhat.com> > --- > Reviewed-by: Eric Blake <eblake@redhat.com> Nice to finally get rid of another HMP command that libvirt had to use.
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 <imain@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
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 <imain@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
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 <imain@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
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 <imain@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
Kevin Wolf <kwolf@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 <imain@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.
Hi, On Wed, Feb 12, 2014 at 6:36 PM, Ian Main <imain@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,
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,
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 <imain@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(-)