Add 'blockdev-del' command

This function returns the reference count of a given BlockBackend. For convenience, it returns 0 if the BlockBackend pointer is NULL.

Signed-off-by Alberto Garcia <[email protected]>

---
 block/block-backend.c          | 5 +++++
 include/sysemu/block-backend.h | 1 +
 2 files changed, 6 insertions(+)

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 <[email protected]>

---
 blockdev.c           | 42 ++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 21 +++++++++++++++++++++
 qmp-commands.hx      | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+)

blockdev.c +21 -0

index 2360c1f..c448a0b 100644
@@ -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);

blockdev.c +11 -0

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

qapi/block-core.json +21 -0

index 5f12af7..20897eb 100644
@@ -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

qmp-commands.hx +36 -0

index 4f03d11..b8b133e 100644
@@ -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,

blockdev.c fail: · line 3435

Quoted 5 lines

blockdev.c +5 -0

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

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.

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

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 <[email protected]>

---
 blockdev.c           | 42 ++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 21 +++++++++++++++++++++
 qmp-commands.hx      | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+)

blockdev.c +7 -0

index 2360c1f..c448a0b 100644
@@ -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);

blockdev.c +25 -0

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

Quoted 16 lines

blockdev.c +16 -0

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

Quoted 1 line

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

Here's my first attempt at the 'blockdev-del' command.

This series goes on top of Max's "BlockBackend and media" v6:

https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02810.html

With Max's code, 'blockdev-add' can now create a BlockDriverState with or without a BlockBackend (depending on whether the 'id' parameter is passed).

Therefore, 'blockdev-del' can be used to delete a backend and/or a BDS.

The way it works is simple: it receives a single 'device' parameter which can refer to a block backend or a node name.

- If it's a block backend, it will delete it along with its attached BDS (if any).

- If it's a node name, it will delete the BDS along with the backend it is attached to (if any).

- If you're wondering whether it's possible to delete the BDS but not the backend it is attached to, there is already eject or blockdev-remove-medium for that.

If either the backend or the BDS are being used (refcount > 1, or if the BDS has any parents) the command fails.

I've been thinking a bit about the creation and deletion of BlockBackends a bit last week, and honestly it still feels a bit messy (or maybe I just don't fully understand it yet). Anyway, the cover letter of this series seems to be a good place for trying to write it down.

So we seem to have two criteria to distinguish BDSes:

1. Does/Doesn't have a BlockBackend on top.
In the future, multiple BlockBackends are possible, too.

1. BB without BDS
2. BB attached to BDS without monitor reference
3. BB attached to BDS with monitor reference
4. BDS without BB and without monitor reference
5. BDS without BB and with monitor reference

Let me start with the first two questions for all cases:

1. As far as I know, it's currently not possible to directly create a BB without a BDS (or before Max' series with an empty BDS). You have to create a BB with an opened BDS and then eject that. Oops.

blockdev-del is easy: It deletes the BB and nothing else.

2. This is the normal case, easy to produce with blockdev-add. The two options for deleting something are removing the BDS while keeping the BB (this is already covered by 'eject') and dropping the BB (probably makes sense to use 'blockdev-del' here). There is no monitor reference to the BDS anyway, so blockdev-dev can't delete that.

Dropping the BB automatically also drops the BDS in simple cases. In more complicated cases where the BDS has got more references later, it might still exist. Then the blockdev-del wasn't the exact opposite operation of blockdev-add.

Is this a problem for a user/management tool that wants to keep track of which image files are still in use?

3. A BDS with a monitor reference can be created (with some patches) by using blockdev-add with a node-name, but no id. Directly attaching it to a BB is tricky today, even though I'm sure you could do it with some clever use of block jobs... With 1. fixed (i.e. you can create an empty BB), insert-medium should do the job.

Here we have even more choices for deleting something: Delete the BB, but leave the BDS alone; delete the BDS and leave an empty BB behind (this is different from eject because eject would drop the connection between BB and BDS, but both would continue to exist!); delete both at once.

We had two separate blockdev-add commands for the BDS and the BB, so it makes sense to require two separate blockdev-del commands as well. In order to keep blockdev-add/del symmetrical, we would have to make blockdev-del of a node-name delete the BDS and blockdev-del of an id delete the BB.

4. That's the typical bs->file. Created by nested blockdev-add descriptions. blockdev-del errors out, there is no monitor reference to drop, and there is also no BB that the request could be used for.

5. Created by a top-level blockdev-add with node-name and no id (like 3. but without attaching it to a BB afterwards). blockdev-del can only mean deleting the BDS.

I haven't thought about it enough yet, but it seems to me that we can't do the BDS/BB aliasing with blockdev-del, but need to interpret node-name as BDS and id as BB. Perhaps we also shouldn't use a single 'device' option then, but a union of 'node-name' and 'id' (just like the same devices were created in blockdev-add).

Quoted 5 lines

I've been thinking a bit about the creation and deletion of BlockBackends a bit last week, and honestly it still feels a bit messy (or maybe I just don't fully understand it yet). Anyway, the cover letter of this series seems to be a good place for trying to write it down.

Thanks for the summary!

Quoted 4 lines

So we seem to have two criteria to distinguish BDSes:

1. Does/Doesn't have a BlockBackend on top.
In the future, multiple BlockBackends are possible, too.

One single BDS attached to multiple BlockBackends at the same time? What's the use case?

Quoted 5 lines

1. BB without BDS
2. BB attached to BDS without monitor reference
3. BB attached to BDS with monitor reference
4. BDS without BB and without monitor reference
5. BDS without BB and with monitor reference

I would say that the rule that we should follow is: if the outcome is not clear for a particular case, then the command fails. We should try not to guess what the user wants. blockdev-del should only delete something when there's only one sane alternative.

Quoted 7 lines

Let me start with the first two questions for all cases:

1. As far as I know, it's currently not possible to directly create a BB without a BDS (or before Max' series with an empty BDS). You have to create a BB with an opened BDS and then eject that. Oops.

blockdev-del is easy: It deletes the BB and nothing else.

I agree.

Quoted 12 lines

2. This is the normal case, easy to produce with blockdev-add. The two options for deleting something are removing the BDS while keeping the BB (this is already covered by 'eject') and dropping the BB (probably makes sense to use 'blockdev-del' here). There is no monitor reference to the BDS anyway, so blockdev-dev can't delete that.

Dropping the BB automatically also drops the BDS in simple cases. In more complicated cases where the BDS has got more references later, it might still exist. Then the blockdev-del wasn't the exact opposite operation of blockdev-add.

Quoted 2 lines

Is this a problem for a user/management tool that wants to keep track of which image files are still in use?

I would say that if the BDS has additional references then 'blockdev-del' should fail and do nothing (what the current patch does).

Quoted 12 lines

3. A BDS with a monitor reference can be created (with some patches) by using blockdev-add with a node-name, but no id. Directly attaching it to a BB is tricky today, even though I'm sure you could do it with some clever use of block jobs... With 1. fixed (i.e. you can create an empty BB), insert-medium should do the job.

Here we have even more choices for deleting something: Delete the BB, but leave the BDS alone; delete the BDS and leave an empty BB behind (this is different from eject because eject would drop the connection between BB and BDS, but both would continue to exist!); delete both at once.

Quoted 5 lines

We had two separate blockdev-add commands for the BDS and the BB, so it makes sense to require two separate blockdev-del commands as well. In order to keep blockdev-add/del symmetrical, we would have to make blockdev-del of a node-name delete the BDS and blockdev-del of an id delete the BB.

This is a tricky case, thanks for pointing it out. I would also go for the conservative option here: if you create a BDS with blockdev-add and then attach it to a BlockBackend, you're adding one additional reference (via blk_insert_bs()). Therefore blockdev-del would fail like in case 2.

You need to eject the BDS first in order to decide which one you want to delete.

Quoted 4 lines

4. That's the typical bs->file. Created by nested blockdev-add descriptions. blockdev-del errors out, there is no monitor reference to drop, and there is also no BB that the request could be used for.

Correct.

Quoted 3 lines

5. Created by a top-level blockdev-add with node-name and no id (like 3. but without attaching it to a BB afterwards). blockdev-del can only mean deleting the BDS.

Correct.

Quoted 5 lines

I haven't thought about it enough yet, but it seems to me that we can't do the BDS/BB aliasing with blockdev-del, but need to interpret node-name as BDS and id as BB. Perhaps we also shouldn't use a single 'device' option then, but a union of 'node-name' and 'id' (just like the same devices were created in blockdev-add).

Having two separate options could make things more clear, indeed.

Quoted 7 lines

Quoted 4 lines

So we seem to have two criteria to distinguish BDSes:

1. Does/Doesn't have a BlockBackend on top.
In the future, multiple BlockBackends are possible, too.

One single BDS attached to multiple BlockBackends at the same time? What's the use case?

For having multiple users of the BDS, e.g. a guest device and an NBD server.

Or sometimes maybe just because it's the logical thing to happen:
Imagine an image B with a backing file A. Both have a BlockBackend on them. Do a live commit of B into A. On completion, the BDS B is deleted and both BBs point to A.

Quoted 7 lines

Quoted 5 lines

I haven't thought about it enough yet, but it seems to me that we can't do the BDS/BB aliasing with blockdev-del, but need to interpret node-name as BDS and id as BB. Perhaps we also shouldn't use a single 'device' option then, but a union of 'node-name' and 'id' (just like the same devices were created in blockdev-add).

Having two separate options could make things more clear, indeed.

Note that it doesn't improve things with your generalised rule (if I got it right anyway).

So I think these are the options we have:

a) blockdev-del only works on a BDS or empty BB without any references. Before deleting a BDS, it must be ejected from the BB; before deleting a BB, its BDS must be ejected.

b) Your rule: Same as a), but treating BB and BDS as a unit if the BDS is only referenced by the BB.

c) No aliasing. You explicitly refer to a BB or to a BDS. The difference from A is that it works without an explicit eject: When a BDS is deleted, it is automatically ejected from the BB; and when the BB is deleted, the BDS stays around if it still has other references.

Quoted 11 lines

Quoted 7 lines

Quoted 4 lines

So we seem to have two criteria to distinguish BDSes:

1. Does/Doesn't have a BlockBackend on top.
In the future, multiple BlockBackends are possible, too.

One single BDS attached to multiple BlockBackends at the same time? What's the use case?

For having multiple users of the BDS, e.g. a guest device and an NBD server.

Quoted 4 lines

Or sometimes maybe just because it's the logical thing to happen:
Imagine an image B with a backing file A. Both have a BlockBackend on them. Do a live commit of B into A. On completion, the BDS B is deleted and both BBs point to A.

Can you have a BlockBackend on a BDS that is being used as a backing file? What is that for? And can you even write to it?

Quoted 17 lines

Quoted 7 lines

Quoted 5 lines

I haven't thought about it enough yet, but it seems to me that we can't do the BDS/BB aliasing with blockdev-del, but need to interpret node-name as BDS and id as BB. Perhaps we also shouldn't use a single 'device' option then, but a union of 'node-name' and 'id' (just like the same devices were created in blockdev-add).

Having two separate options could make things more clear, indeed.

Note that it doesn't improve things with your generalised rule (if I got it right anyway).

So I think these are the options we have:

a) blockdev-del only works on a BDS or empty BB without any references. Before deleting a BDS, it must be ejected from the BB; before deleting a BB, its BDS must be ejected.

Quoted 7 lines

b) Your rule: Same as a), but treating BB and BDS as a unit if the BDS is only referenced by the BB.

c) No aliasing. You explicitly refer to a BB or to a BDS. The difference from A is that it works without an explicit eject: When a BDS is deleted, it is automatically ejected from the BB; and when the BB is deleted, the BDS stays around if it still has other references.

I'm currently thinking about d), which tries to maintain the symmetry with blockdev-add:

- blockdev-del would have two parameters, 'id' and 'node-name', and only one of them can be set, so you must choose whether you want to delete a backend or a BDS.

- blockdev-add can either create a backend with a BDS, or a BDS alone, so:

- If you created a backend and you try to delete it you can do it (along with its BDS) as long as neither the backend nor the BDS are being used (no extra references, no parents). This means that the operation will fail if there's a BDS that has been created separately and manually attached to the the backend.

- If you created a BDS alone and you try to delete it you can do it as long as no one else is using it. This would delete the BDS and only the BDS (because that's what you create with blockdev-add). If it's currently attached to a backend then the operation fails.

I think this is what best mirrors blockdev-add. It however has the following consequence:

blockdev-add id=drive0 node-name=node0 ...

blockdev-del node-name=node0 <-- This fails, node0 is used by drive0

blockdev-del id=drive0 <-- This works and removes drive0 and node0

Quoted 41 lines

Quoted 17 lines

Quoted 7 lines

Quoted 5 lines

I haven't thought about it enough yet, but it seems to me that we can't do the BDS/BB aliasing with blockdev-del, but need to interpret node-name as BDS and id as BB. Perhaps we also shouldn't use a single 'device' option then, but a union of 'node-name' and 'id' (just like the same devices were created in blockdev-add).

Having two separate options could make things more clear, indeed.

Note that it doesn't improve things with your generalised rule (if I got it right anyway).

So I think these are the options we have:

a) blockdev-del only works on a BDS or empty BB without any references. Before deleting a BDS, it must be ejected from the BB; before deleting a BB, its BDS must be ejected.

Quoted 7 lines

b) Your rule: Same as a), but treating BB and BDS as a unit if the BDS is only referenced by the BB.

c) No aliasing. You explicitly refer to a BB or to a BDS. The difference from A is that it works without an explicit eject: When a BDS is deleted, it is automatically ejected from the BB; and when the BB is deleted, the BDS stays around if it still has other references.

I'm currently thinking about d), which tries to maintain the symmetry with blockdev-add:

- blockdev-del would have two parameters, 'id' and 'node-name', and only one of them can be set, so you must choose whether you want to delete a backend or a BDS.

- blockdev-add can either create a backend with a BDS, or a BDS alone, so:

- If you created a backend and you try to delete it you can do it (along with its BDS) as long as neither the backend nor the BDS are being used (no extra references, no parents). This means that the operation will fail if there's a BDS that has been created separately and manually attached to the the backend.

Quoted 4 lines

- If you created a BDS alone and you try to delete it you can do it as long as no one else is using it. This would delete the BDS and only the BDS (because that's what you create with blockdev-add). If it's currently attached to a backend then the operation fails.

So this is essentially c) with the modification that no implicit eject happens. Either both BB and BDS go away because the BDS is only referenced by the BB or you get an error.

Quoted 16 lines

Quoted 16 lines

I'm currently thinking about d), which tries to maintain the symmetry with blockdev-add:

- blockdev-del would have two parameters, 'id' and 'node-name', and only one of them can be set, so you must choose whether you want to delete a backend or a BDS.

- blockdev-add can either create a backend with a BDS, or a BDS alone, so:

- If you created a backend and you try to delete it you can do it (along with its BDS) as long as neither the backend nor the BDS are being used (no extra references, no parents). This means that the operation will fail if there's a BDS that has been created separately and manually attached to the the backend.

Quoted 8 lines

Quoted 4 lines

- If you created a BDS alone and you try to delete it you can do it as long as no one else is using it. This would delete the BDS and only the BDS (because that's what you create with blockdev-add). If it's currently attached to a backend then the operation fails.

So this is essentially c) with the modification that no implicit eject happens. Either both BB and BDS go away because the BDS is only referenced by the BB or you get an error.

Right, I would say you always get an error.

I'm currently extending the set of tests (I expect to send the updated series later today or tomorrow) and most are quite straightforward and hopefully helpful to prevent surprises in the future. It's also an interesting exercise to test the BlockBackend series by Max.

But there's this case that is not so obvious. It involves the new 'blockdev-snapshot' command I'm working on:

- blockdev-add id=drive0 node-name=node0 file=hd0.qcow2
- qemu-img create -f qcow2 -b hd0.qcow2 overlay0.qcow2
- blockdev-add node-name=overlay0 file=overlay0.qcow2
- blockdev-snapshot node=hd0 overlay=overlay0

At this point you have drive0 with overlay0 inserted, and hd0 as its backing image. All these operation will fail:

- blockdev-del id=drive0 because overlay0 has two references
(monitor and block backend)
- blockdev-del node=overlay0 for the same reason
- blockdev-del node=hd0 because it's a backing image

In order to delete all this you need to:

- eject device=drive0 overlay0 has one reference left
- blockdev-del id=drive0
- blockdev-del node=overlay0 this deletes hd0 as well

Does this make sense, or do we need to rethink the semantics a bit more?

Quoted 16 lines

But there's this case that is not so obvious. It involves the new 'blockdev-snapshot' command I'm working on:

- blockdev-add id=drive0 node-name=node0 file=hd0.qcow2
- qemu-img create -f qcow2 -b hd0.qcow2 overlay0.qcow2
- blockdev-add node-name=overlay0 file=overlay0.qcow2
- blockdev-snapshot node=hd0 overlay=overlay0

At this point you have drive0 with overlay0 inserted, and hd0 as its backing image. All these operation will fail:

- blockdev-del id=drive0 because overlay0 has two references
(monitor and block backend)
- blockdev-del node=overlay0 for the same reason
- blockdev-del node=hd0 because it's a backing image

Quoted 7 lines

In order to delete all this you need to:

- eject device=drive0 overlay0 has one reference left
- blockdev-del id=drive0
- blockdev-del node=overlay0 this deletes hd0 as well

Does this make sense, or do we need to rethink the semantics a bit more?

Well, it's consistent with what you described above.

The confusing part might be that you could blockdev-del id=drive0 originally, but after taking the snapshot it doesn't work any more. The only way I can see to remove this effect is that you always need to eject the BDS first, even if its only reference is from the BB that is going to be deleted.

I guess that would be even clearer rules, but of course it also means that it's a bit more cumbersome to use. If it helps avoiding bugs in management tools, it might be worth it.

But again, I'd like to hear a libvirt opinion from Eric here.

Quoted 32 lines

Quoted 16 lines

But there's this case that is not so obvious. It involves the new 'blockdev-snapshot' command I'm working on:

- blockdev-add id=drive0 node-name=node0 file=hd0.qcow2
- qemu-img create -f qcow2 -b hd0.qcow2 overlay0.qcow2
- blockdev-add node-name=overlay0 file=overlay0.qcow2
- blockdev-snapshot node=hd0 overlay=overlay0

At this point you have drive0 with overlay0 inserted, and hd0 as its backing image. All these operation will fail:

- blockdev-del id=drive0 because overlay0 has two references
(monitor and block backend)
- blockdev-del node=overlay0 for the same reason
- blockdev-del node=hd0 because it's a backing image

Quoted 7 lines

In order to delete all this you need to:

- eject device=drive0 overlay0 has one reference left
- blockdev-del id=drive0
- blockdev-del node=overlay0 this deletes hd0 as well

Does this make sense, or do we need to rethink the semantics a bit more?

Well, it's consistent with what you described above.

The confusing part might be that you could blockdev-del id=drive0 originally, but after taking the snapshot it doesn't work any more. The only way I can see to remove this effect is that you always need to eject the BDS first, even if its only reference is from the BB that is going to be deleted.

Quoted 3 lines

I guess that would be even clearer rules, but of course it also means that it's a bit more cumbersome to use. If it helps avoiding bugs in management tools, it might be worth it.

That would be a good reason to force the user to eject the media first. Note however that in this case you would still need to delete overlay0 manually, as it would still have the monitor reference.

However if the snapshot is created using blockdev-snapshot-sync that problem does not exist because that extra reference is not there.

Am 13.10.2015 um 15:48 hat Alberto Garcia geschrieben:

Quoted 22 lines

Here's my first attempt at the 'blockdev-del' command.

This series goes on top of Max's "BlockBackend and media" v6:

https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02810.html

With Max's code, 'blockdev-add' can now create a BlockDriverState with or without a BlockBackend (depending on whether the 'id' parameter is passed).

Therefore, 'blockdev-del' can be used to delete a backend and/or a BDS.

The way it works is simple: it receives a single 'device' parameter which can refer to a block backend or a node name.

- If it's a block backend, it will delete it along with its attached BDS (if any).

- If it's a node name, it will delete the BDS along with the backend it is attached to (if any).

Quoted 6 lines

- If you're wondering whether it's possible to delete the BDS but not the backend it is attached to, there is already eject or blockdev-remove-medium for that.

If either the backend or the BDS are being used (refcount > 1, or if the BDS has any parents) the command fails.

I've been thinking a bit about the creation and deletion of BlockBackends a bit last week, and honestly it still feels a bit messy (or maybe I just don't fully understand it yet). Anyway, the cover letter of this series seems to be a good place for trying to write it down.

So we seem to have two criteria to distinguish BDSes:

1. Does/Doesn't have a BlockBackend on top.
In the future, multiple BlockBackends are possible, too.

2. Has/Hasn't been created explicitly by the user. Only if it has, the monitor owns a reference.

In addition, we also have BlockBackends without a BDS attached. All BBs are explicitly created by the user. This leads us to the following cases:

1. BB without BDS
2. BB attached to BDS without monitor reference
3. BB attached to BDS with monitor reference
4. BDS without BB and without monitor reference
5. BDS without BB and with monitor reference

The question I've been thinking about for each of the cases is: Can we create this with blockdev-add today, and what should blockdev-del do? Another interesting question is: Can we move between the cases, e.g. by adding a monitor reference retroactively, or do we even want to do that?

Let me start with the first two questions for all cases:

1. As far as I know, it's currently not possible to directly create a BB without a BDS (or before Max' series with an empty BDS). You have to create a BB with an opened BDS and then eject that. Oops.

blockdev-del is easy: It deletes the BB and nothing else.

2. This is the normal case, easy to produce with blockdev-add. The two options for deleting something are removing the BDS while keeping the BB (this is already covered by 'eject') and dropping the BB (probably makes sense to use 'blockdev-del' here). There is no monitor reference to the BDS anyway, so blockdev-dev can't delete that.

Dropping the BB automatically also drops the BDS in simple cases. In more complicated cases where the BDS has got more references later, it might still exist. Then the blockdev-del wasn't the exact opposite operation of blockdev-add.

Is this a problem for a user/management tool that wants to keep track of which image files are still in use?

3. A BDS with a monitor reference can be created (with some patches) by using blockdev-add with a node-name, but no id. Directly attaching it to a BB is tricky today, even though I'm sure you could do it with some clever use of block jobs... With 1. fixed (i.e. you can create an empty BB), insert-medium should do the job.

Here we have even more choices for deleting something: Delete the BB, but leave the BDS alone; delete the BDS and leave an empty BB behind (this is different from eject because eject would drop the connection between BB and BDS, but both would continue to exist!); delete both at once.

We had two separate blockdev-add commands for the BDS and the BB, so it makes sense to require two separate blockdev-del commands as well. In order to keep blockdev-add/del symmetrical, we would have to make blockdev-del of a node-name delete the BDS and blockdev-del of an id delete the BB.

4. That's the typical bs->file. Created by nested blockdev-add descriptions. blockdev-del errors out, there is no monitor reference to drop, and there is also no BB that the request could be used for.

5. Created by a top-level blockdev-add with node-name and no id (like 3. but without attaching it to a BB afterwards). blockdev-del can only mean deleting the BDS.

If I compare this with the semantics described in the cover letter, I must say that I see some problems, especially with case 3.

I haven't thought about it enough yet, but it seems to me that we can't do the BDS/BB aliasing with blockdev-del, but need to interpret node-name as BDS and id as BB. Perhaps we also shouldn't use a single 'device' option then, but a union of 'node-name' and 'id' (just like the same devices were created in blockdev-add).

Kevin

Here's my first attempt at the 'blockdev-del' command.

This series goes on top of Max's "BlockBackend and media" v6:

https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02810.html

With Max's code, 'blockdev-add' can now create a BlockDriverState with or without a BlockBackend (depending on whether the 'id' parameter is passed).

Therefore, 'blockdev-del' can be used to delete a backend and/or a BDS.

The way it works is simple: it receives a single 'device' parameter which can refer to a block backend or a node name.

- If it's a block backend, it will delete it along with its attached BDS (if any).

- If it's a node name, it will delete the BDS along with the backend it is attached to (if any).

- If you're wondering whether it's possible to delete the BDS but not the backend it is attached to, there is already eject or blockdev-remove-medium for that.

If either the backend or the BDS are being used (refcount > 1, or if the BDS has any parents) the command fails.

The series includes bunch of test cases with different scenarios (nodes with and without backend, empty backend, backing images, block jobs, Quorum).

blockdev-add is a big & hairy feature that has taken considerable time to develop, spanning multiple releases, and still isn't quite done (never been closer, though). As such, it's a textbook example of an experimental interface, and should have been x-blockdev-add. We messed that up, and it's probably not worth renaming now.

Quite a few users have been trying to use blockdev-add, and ran into roadblocks sooner or later. Inevitable, given its incomplete state. Perhaps an x- prefix providing fair warning would've saved them trouble, but that's water under the bridge now. We tried to warn them off with scary documentation instead (commit da2cf4e).

Naturally, running into an obvious roadblock early is less bad than running into a subtle one late. The lack of blockdev-del has served as an obvious early one.

Mind, that's not an objection to implementing it now. I think the time's ripe, actually. I just want us to try to erect a proper warning sign where the "no blockdev-del" roadblock used to be. Naming it x-blockdev-del together with a note explaining why it's experimental should do the trick.