[2/3] block: Add 'blockdev-del' QMP command

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(+)

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,
>

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;

After seeing testBlockBackendUsingNodeName() in patch 3, I don't know
about this. We often have this notion of "If you need a BDS, you can
specify both the BB and the node name", but here it's the other way
around: One "needs" a BB (at least that's what the command will work on)
and one gets it by specifying a node name. That seems a bit strange to me.

Max

> +        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;
> +    }
> +
> +    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;

On Sat 17 Oct 2015 08:23:26 PM CEST, Max Reitz <mreitz@redhat.com> wrote:
>> +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;
>
> After seeing testBlockBackendUsingNodeName() in patch 3, I don't know
> about this. We often have this notion of "If you need a BDS, you can
> specify both the BB and the node name", but here it's the other way
> around: One "needs" a BB (at least that's what the command will work
> on) and one gets it by specifying a node name. That seems a bit
> strange to me.

I'm also not completely sure about this to be honest and I'm not
surprised that you don't like it. I was thinking about this these last
couple of days and I think that this alternative is probably better:

 - If you specify the name of a backend, delete the backend and its BDS
   (if there's any).
 - If you specify the name of a BDS (using its node name), delete the
   BDS and only the BDS.

Berto

On Sat 17 Oct 2015 08:06:19 PM CEST, Max Reitz wrote:
>> +    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.

I agree, I would also like to have a clearer way to do it.

I'll try to go over all possible ways to create a BDS and see other
cases that I might have missed. At the very list I would like to extend
the iotest and make it as comprehensive as possible.

> 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.

Yeah, those would be handy indeed.

> 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...

I think I'll first try to go over all possible cases and see what I
find.

Thanks,

Berto