Re: [Qemu-devel] [PATCH 2/3] block: Add 'blockdev-del' QMP command

[Max Reitz <mreitz@redhat.com>]

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

On 13.10.2015 15:48, Alberto Garcia wrote:
> This is the companion to 'blockdev-add'. It allows deleting a
> BlockBackend with its associated BlockDriverState tree, or a
> BlockDriverState that is not attached to any backend.
> 
> In either case, the command fails if the reference count is greater
> than 1 or the BlockDriverState has any parents.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c           | 42 ++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 21 +++++++++++++++++++++
>  qmp-commands.hx      | 36 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 99 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 2360c1f..c448a0b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3402,6 +3402,48 @@ fail:
>      qmp_output_visitor_cleanup(ov);
>  }
>  
> +void qmp_blockdev_del(const char *device, Error **errp)
> +{
> +    AioContext *aio_context;
> +    BlockBackend *blk;
> +    BlockDriverState *bs;
> +
> +    blk = blk_by_name(device);
> +    if (blk) {
> +        bs = blk_bs(blk);
> +        aio_context = blk_get_aio_context(blk);
> +    } else {
> +        bs = bdrv_find_node(device);
> +        if (!bs) {
> +            error_setg(errp, "Cannot find block device %s", device);
> +            return;
> +        }
> +        blk = bs->blk;
> +        aio_context = bdrv_get_aio_context(bs);
> +    }
> +
> +    aio_context_acquire(aio_context);
> +
> +    if (bs && bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, errp)) {
> +        goto out;
> +    }
> +
> +    if (blk_get_refcnt(blk) > 1 ||
> +        (bs && (bs->refcnt > 1 || !QLIST_EMPTY(&bs->parents)))) {
> +        error_setg(errp, "Block device %s is being used", device);
> +        goto out;
> +    }

I can't think of a way off the top of my head that a BB with a refcount
of one or a BDS with a refcount of one without any parents would not be
monitor-owned, but I don't quite like assuming it just from the refcount
and the parent list anyway.

Especially since I can have two series on hold which are pretty strongly
tied to this patch:

http://lists.nongnu.org/archive/html/qemu-block/2015-03/msg00044.html
(block: Rework bdrv_close_all())

and

http://lists.nongnu.org/archive/html/qemu-block/2015-02/msg00021.html
(blockdev: Further BlockBackend work)

Both have been on hold for more than half a year, since they depend on
the "BlockBackend and media" series, and, well, I guess you know well
about that one's status.

The two patches which are significant for this patch are:
- "blockdev: Keep track of monitor-owned BDS" from the first series
- "blockdev: Add list of monitor-owned BlockBackends" from the second

Explaining what they do and why they are relevant to this patch doesn't
really seem necessary, but anyway: They add one list for the
monitor-owned BDSs and one for the monitor-owned BBs.

In combination with this patch, that means two things:
(1) We should only *_unref() BDSs/BBs if their refcount is exactly one
    *and* they are in their respective list, and
(2) We have to remove the BDS/BB from its respective list.

You don't really have to care about (2), because that's my own problem
for having introduced these lists. But (1)... It would definitely be a
better check than just comparing the refcount.


It's up to you. It probably works just how you implemented it, and
considering the pace of the "BB and media" series (and other series of
mine in the past year), getting those two series merged may be a matter
of decades. So it's probably best to do it like this for now and I'll
fix it up then...

Max

> +
> +    if (blk) {
> +        blk_unref(blk);
> +    } else {
> +        bdrv_unref(bs);
> +    }
> +
> +out:
> +    aio_context_release(aio_context);
> +}
> +
>  BlockJobInfoList *qmp_query_block_jobs(Error **errp)
>  {
>      BlockJobInfoList *head = NULL, **p_next = &head;
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 5f12af7..20897eb 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1877,6 +1877,27 @@
>  { 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
>  
>  ##
> +# @blockdev-del:
> +#
> +# Deletes a block device. The selected device can be either a block
> +# backend or a graph node.
> +#
> +# In the former case the backend will be destroyed, along with its
> +# inserted medium if there's any.
> +#
> +# In the latter case the node will be destroyed, along with the
> +# backend it is attached to if there's any.
> +#
> +# The command will fail if the selected block device is still being
> +# used.
> +#
> +# @device: Name of the block backend or the graph node to be deleted.
> +#
> +# Since: 2.5
> +##
> +{ 'command': 'blockdev-del', 'data': { 'device': 'str' } }
> +
> +##
>  # @blockdev-open-tray:
>  #
>  # Opens a block device's tray. If there is a block driver state tree inserted as
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 4f03d11..b8b133e 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3921,6 +3921,42 @@ Example (2):
>  EQMP
>  
>      {
> +        .name       = "blockdev-del",
> +        .args_type  = "device:s",
> +        .mhandler.cmd_new = qmp_marshal_blockdev_del,
> +    },
> +
> +SQMP
> +blockdev-del
> +------------
> +Since 2.5
> +
> +Deletes a block device. The selected device can be either a block
> +backend or a graph node.
> +
> +In the former case the backend will be destroyed, along with its
> +inserted medium if there's any.
> +
> +In the latter case the node will be destroyed, along with the
> +backend it is attached to if there's any.
> +
> +The command will fail if the selected block device is still being
> +used.
> +
> +Arguments:
> +
> +- "device": Identifier of the block device or graph node name (json-string)
> +
> +Example:
> +
> +-> { "execute": "blockdev-del",
> +                "arguments": { "device": "virtio0" }
> +   }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>          .name       = "blockdev-open-tray",
>          .args_type  = "device:s,force:b?",
>          .mhandler.cmd_new = qmp_marshal_blockdev_open_tray,
> 



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