[Qemu-devel] [PATCH 0/3] Add 'blockdev-del' command

[Qemu-devel] [PATCH 0/3] Add 'blockdev-del' command

[Alberto Garcia]

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

Regards,

Berto

Alberto Garcia (3):
  block: Add blk_get_refcnt()
  block: Add 'blockdev-del' QMP command
  iotests: Add test for the blockdev-del command

 block/block-backend.c          |   5 ++
 blockdev.c                     |  42 +++++++++
 include/sysemu/block-backend.h |   1 +
 qapi/block-core.json           |  21 +++++
 qmp-commands.hx                |  36 ++++++++
 tests/qemu-iotests/139         | 189 +++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/139.out     |   5 ++
 tests/qemu-iotests/group       |   1 +
 8 files changed, 300 insertions(+)
 create mode 100644 tests/qemu-iotests/139
 create mode 100644 tests/qemu-iotests/139.out

-- 
2.6.1

[Qemu-devel] [PATCH 1/3] block: Add blk_get_refcnt()

[Alberto Garcia]

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 <berto@igalia.com>
---
 block/block-backend.c          | 5 +++++
 include/sysemu/block-backend.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 10e4d71..5f1e395 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -189,6 +189,11 @@ static void drive_info_del(DriveInfo *dinfo)
     g_free(dinfo);
 }
 
+int blk_get_refcnt(BlockBackend *blk)
+{
+    return blk ? blk->refcnt : 0;
+}
+
 /*
  * Increment @blk's reference count.
  * @blk must not be null.
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 14a6d32..8a6413d 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -65,6 +65,7 @@ BlockBackend *blk_new_with_bs(const char *name, Error **errp);
 BlockBackend *blk_new_open(const char *name, const char *filename,
                            const char *reference, QDict *options, int flags,
                            Error **errp);
+int blk_get_refcnt(BlockBackend *blk);
 void blk_ref(BlockBackend *blk);
 void blk_unref(BlockBackend *blk);
 const char *blk_name(BlockBackend *blk);
-- 
2.6.1

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

[Alberto Garcia]

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

[Qemu-devel] [PATCH 3/3] iotests: Add test for the blockdev-del command

[Alberto Garcia]

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/139     | 189 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/139.out |   5 ++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 195 insertions(+)
 create mode 100644 tests/qemu-iotests/139
 create mode 100644 tests/qemu-iotests/139.out

diff --git a/tests/qemu-iotests/139 b/tests/qemu-iotests/139
new file mode 100644
index 0000000..325ac7a
--- /dev/null
+++ b/tests/qemu-iotests/139
@@ -0,0 +1,189 @@
+#!/usr/bin/env python
+#
+# Test cases for the QMP 'blockdev-del' command
+#
+# Copyright (C) 2015 Igalia, S.L.
+# Author: Alberto Garcia <berto@igalia.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import iotests
+import time
+
+base_img = os.path.join(iotests.test_dir, 'base.img')
+new_img = os.path.join(iotests.test_dir, 'new.img')
+
+class TestBlockdevDel(iotests.QMPTestCase):
+
+    def setUp(self):
+        iotests.qemu_img('create', '-f', iotests.imgfmt, base_img, '1M')
+        self.vm = iotests.VM()
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(base_img)
+        if os.path.isfile(new_img):
+            os.remove(new_img)
+
+    def addBlockDev(self, block_backend = True):
+        opts = {'node-name': 'node0',
+                'driver': iotests.imgfmt,
+                'file': {'filename': base_img,
+                         'driver': 'file'}}
+        if block_backend:
+            opts['id'] = 'drive0'
+        result = self.vm.qmp('blockdev-add', conv_keys = False, options = opts)
+        self.assert_qmp(result, 'return', {})
+
+    def delBlockDev(self, use_node_name = False, expect_error = False):
+        if use_node_name:
+            result = self.vm.qmp('blockdev-del', device = 'node0')
+        else:
+            result = self.vm.qmp('blockdev-del', device = 'drive0')
+        if expect_error:
+            self.assert_qmp(result, 'error/class', 'GenericError')
+        else:
+            self.assert_qmp(result, 'return', {})
+
+    def addDeviceModel(self):
+        result = self.vm.qmp('device_add', id = 'device0',
+                             driver = 'virtio-blk-pci', drive = 'drive0')
+        self.assert_qmp(result, 'return', {})
+
+    def delDeviceModel(self):
+        result = self.vm.qmp('device_del', id = 'device0')
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('system_reset')
+        self.assert_qmp(result, 'return', {})
+
+        device_path = '/machine/peripheral/device0/virtio-backend'
+        event = self.vm.event_wait(name="DEVICE_DELETED",
+                                   match={'data': {'path': device_path}})
+        self.assertNotEqual(event, None)
+
+        event = self.vm.event_wait(name="DEVICE_DELETED",
+                                   match={'data': {'device': 'device0'}})
+        self.assertNotEqual(event, None)
+
+    def ejectDrive(self, expect_error = False):
+        result = self.vm.qmp('eject', device = 'drive0')
+        if expect_error:
+            self.assert_qmp(result, 'error/class', 'GenericError')
+        else:
+            self.assert_qmp(result, 'return', {})
+
+    def createSnapshot(self):
+        opts = {'node-name': 'node0',
+                'snapshot-file': new_img,
+                'snapshot-node-name': 'overlay0',
+                'format': iotests.imgfmt}
+        result = self.vm.qmp('blockdev-snapshot-sync', conv_keys=False, **opts)
+        self.assert_qmp(result, 'return', {})
+
+    def createMirror(self):
+        opts = {'device': 'drive0',
+                'target': new_img,
+                'sync': 'top',
+                'format': iotests.imgfmt}
+        result = self.vm.qmp('drive-mirror', conv_keys=False, **opts)
+        self.assert_qmp(result, 'return', {})
+
+    def completeBlockJob(self):
+        result = self.vm.qmp('block-job-complete', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+    def addQuorumDrive(self):
+        iotests.qemu_img('create', '-f', iotests.imgfmt, new_img, '1M')
+        drive_0 = {'driver': iotests.imgfmt,
+                   'node-name': 'node0',
+                   'file': {'driver': 'file',
+                            'filename': base_img}}
+        drive_1 = {'driver': iotests.imgfmt,
+                   'node-name': 'node1',
+                   'file': {'driver': 'file',
+                            'filename': new_img}}
+        opts = {'driver': 'quorum',
+                'id': 'drive0',
+                'vote-threshold': 1,
+                'children': [ drive_0, drive_1 ]}
+        result = self.vm.qmp('blockdev-add', conv_keys = False, options = opts)
+        self.assert_qmp(result, 'return', {})
+
+    def testNodeWithoutBackend(self):
+        self.addBlockDev(block_backend = False)
+        self.delBlockDev(use_node_name = False, expect_error = True)
+        self.delBlockDev(use_node_name = True)
+
+    def testBlockBackendUsingDeviceName(self):
+        self.addBlockDev(block_backend = True)
+        self.delBlockDev(use_node_name = False)
+
+    def testBlockBackendUsingNodeName(self):
+        self.addBlockDev(block_backend = True)
+        self.delBlockDev(use_node_name = True)
+
+    def testEject(self):
+        self.addBlockDev()
+        self.ejectDrive()
+        self.delBlockDev()
+
+    def testDeviceModel(self):
+        self.addBlockDev()
+        self.addDeviceModel()
+        self.ejectDrive(expect_error = True)
+        self.delBlockDev(expect_error = True)
+        self.delDeviceModel()
+        self.delBlockDev()
+
+    def testDriveEject(self):
+        self.addBlockDev()
+        self.addDeviceModel()
+        self.delDeviceModel()
+        self.ejectDrive()
+        self.delBlockDev(use_node_name = True, expect_error = True)
+        self.delBlockDev()
+
+    def testSnapshot(self):
+        self.addBlockDev()
+        self.createSnapshot()
+        # This fails because 'node0' is now being used as a backing image
+        self.delBlockDev(use_node_name = True, expect_error = True)
+        # This succeeds because the backend now points to the overlay image
+        self.delBlockDev()
+
+    def testMirror(self):
+        self.addBlockDev()
+        self.createMirror()
+        # The block job prevents removing the device
+        self.delBlockDev(use_node_name = False, expect_error = True)
+        self.delBlockDev(use_node_name = True,  expect_error = True)
+        self.completeBlockJob()
+        # This fails because the mirror operation got rid of 'node0'
+        self.delBlockDev(use_node_name = True, expect_error = True)
+        # This succeeds because the backend now points to the new image
+        self.delBlockDev()
+
+    def testQuorum(self):
+        self.addQuorumDrive()
+        # 'node0' is a children of the Quorum device, we cannot remove it
+        self.delBlockDev(use_node_name = True, expect_error = True)
+        # 'drive0' is the Quorum device itself
+        self.delBlockDev(use_node_name = False)
+
+if __name__ == '__main__':
+    iotests.main(supported_fmts=["qcow2"])
diff --git a/tests/qemu-iotests/139.out b/tests/qemu-iotests/139.out
new file mode 100644
index 0000000..dae404e
--- /dev/null
+++ b/tests/qemu-iotests/139.out
@@ -0,0 +1,5 @@
+.........
+----------------------------------------------------------------------
+Ran 9 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 85329af..bf0b6c6 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -137,3 +137,4 @@
 135 rw auto
 137 rw auto
 138 rw auto quick
+139 rw auto quick
-- 
2.6.1

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

[Max Reitz]

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

Re: [Qemu-devel] [PATCH 1/3] block: Add blk_get_refcnt()

[Max Reitz]

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

On 13.10.2015 15:48, Alberto Garcia wrote:
> 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 <berto@igalia.com>
> ---
>  block/block-backend.c          | 5 +++++
>  include/sysemu/block-backend.h | 1 +
>  2 files changed, 6 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2470 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;

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;


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

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

[Alberto Garcia]

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

Re: [Qemu-devel] [PATCH 0/3] Add 'blockdev-del' command

[Kevin Wolf]

Am 13.10.2015 um 15:48 hat Alberto Garcia geschrieben:
> 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.

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

Re: [Qemu-devel] [PATCH 0/3] Add 'blockdev-del' command

[Alberto Garcia]

On Mon 19 Oct 2015 01:27:45 PM CEST, Kevin Wolf wrote:
> 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!

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

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

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

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

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

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

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.

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

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

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

Berto

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

[Alberto Garcia]

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

Re: [Qemu-devel] [PATCH 0/3] Add 'blockdev-del' command

[Max Reitz]

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

On 19.10.2015 13:27, Kevin Wolf wrote:
> Am 13.10.2015 um 15:48 hat Alberto Garcia geschrieben:
>> 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.
> 
> 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.

And keep in mind that in this case the monitor has two references, one
to the BB and one to the BDS, so in case these are the only "external"
references, the BB will have a refcount of 1 and the BDS a refcount of
2. Therefore, without any magic, you'll need two blockdev-del
invocations anyway, one for the BB and one for the BDS.

As Berto said, with patch 2 as it is, either blockdev-del will fail, and
you need to call remove-medium/eject first, which seems fine to me.

Max

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



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

Re: [Qemu-devel] [PATCH 0/3] Add 'blockdev-del' command

[Kevin Wolf]

Am 19.10.2015 um 16:15 hat Alberto Garcia geschrieben:
> On Mon 19 Oct 2015 01:27:45 PM CEST, Kevin Wolf wrote:
> > 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!
> 
> > 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.

> > 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.
> 
> > 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.
> 
> > 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?
> 
> I would say that if the BDS has additional references then
> 'blockdev-del' should fail and do nothing (what the current patch does).

Sorry for not having read the patches yet. I tried to figure out what
the right thing to do is before I could start reviewing whether the
patches do the thing right.

I agree with your solution, which addresses my question.

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

Okay, so is the following your generalised rule?

If device refers to a BDS without a BB or to a BB without a BDS, then
just delete it. Otherwise, try to treat one BDS with one BB as a unit
and remove that unit. They can only be treated as a unit if the BDS is
only referenced by the BB (i.e. not by the monitor either). If they
can't be treated as a unit, error out.

I think that's safe and allows using the usual aliasing (using the BB
name or the node-name of its BDS means the same thing), which is good
for consistency.

My only concern with this is whether it isn't too clever. Management
tools need to be prepared to handle BDSes with more than one reference
differently from BDSes only referenced by a (single) BB. The common case
is the simple one where removing BB and BDS at once works; so it may be
an invitation to ignore the other case, which would then surprisingly
fail.

On the other hand, always requiring an eject before blockdev-del would
break the symmetry of blockdev-add and blockdev-del and isn't very nice
either.

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

Kevin

Re: [Qemu-devel] [PATCH 0/3] Add 'blockdev-del' command

[Alberto Garcia]

On Mon 19 Oct 2015 05:04:50 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote:
>> > 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.

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?

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

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

Berto

Re: [Qemu-devel] [PATCH 0/3] Add 'blockdev-del' command

[Markus Armbruster]

Alberto Garcia <berto@igalia.com> writes:

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

I almost certainly won't have the time to review this series, but I can
still indulge in a little drive-by shooting :)

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.

Re: [Qemu-devel] [PATCH 0/3] Add 'blockdev-del' command

[Alberto Garcia]

On Wed 21 Oct 2015 10:57:32 AM CEST, Markus Armbruster <armbru@redhat.com> wrote:
> 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.

That works for me, I can rename it in my next submission and add the
appropriate warnings.

Berto

Re: [Qemu-devel] [PATCH 0/3] Add 'blockdev-del' command

[Kevin Wolf]

Am 20.10.2015 um 17:02 hat Alberto Garcia geschrieben:
> On Mon 19 Oct 2015 05:04:50 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote:
> >> > 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.
> 
> 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?

I think you can't create that currently, but it would be useful indeed.
You need it for cases like taking a snapshot and then making the
snapshot available through the NBD server.

Writing to backing files, though... Probably not a good idea.

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

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.

> 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

I think this is okay. As you said, it mirrors blockdev-add.

Eric, would this work for you?

Kevin

Re: [Qemu-devel] [PATCH 0/3] Add 'blockdev-del' command

[Alberto Garcia]

On Thu 22 Oct 2015 12:38:24 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote:
>> >> > 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.
>> 
>> 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.
>
> 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?

Berto

Re: [Qemu-devel] [PATCH 0/3] Add 'blockdev-del' command

[Kevin Wolf]

Am 22.10.2015 um 13:08 hat Alberto Garcia geschrieben:
> >> 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.
> >
> > 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.

Awesome. As you know, I love test cases.

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

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.

Kevin

Re: [Qemu-devel] [PATCH 0/3] Add 'blockdev-del' command

[Alberto Garcia]

On Thu 22 Oct 2015 01:25:05 PM CEST, Kevin Wolf wrote:
>> 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?
>
> 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.

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.

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

Sure, me too.

Berto

Re: [Qemu-devel] [PATCH 0/3] Add 'blockdev-del' command

[Kevin Wolf]

Am 22.10.2015 um 13:31 hat Alberto Garcia geschrieben:
> On Thu 22 Oct 2015 01:25:05 PM CEST, Kevin Wolf wrote:
> >> 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?
> >
> > 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.
> 
> 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.

Yes, but I think that's expected because you had a separate blockdev-add.

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

Hm... Actually I see a good question here. It's not clear to me that a
BDS created with blockdev-snapshot-sync shouldn't be considered
explicit, especially if a node-name was passed. I guess you can bring up
good arguments for either behaviour.

Kevin