[PATCH 0/2] blockdev-add transaction

[PATCH 0/2] blockdev-add transaction

[Vladimir Sementsov-Ogievskiy]

Hi all!

If we want to do incremental backups with help of copy-before-write
filter bitmap parameter introduced in my in-flight series
"[PATCH v4 00/18] Make image fleecing more usable", we actually need to
create filter, insert it into graph and do some operations with bitmaps
in one transaction.

So, here is basic support for blockdev-add transaction, which may be
useful for any scenarios.

Also, I'll need to resend my "[PATCH RFC v2 0/4] blockdev-replace" with
transaction support as well.

Why I say that support is "basic"? Ideally we want an ability to call
several blockdev-add / blockdev-replace commands not updating the
permission and only then update permissions for all touched nodes. That
is more flexible and will allow more strict and simple permission
requirements in filter drivers. Still this means to implement
bdrv_open() transaction action (using Transaction API) which doesn't
update permissions (like most of internal graph update functions works
now). That's of course is more complicated than this patch. So, let's
start with something simple.

Vladimir Sementsov-Ogievskiy (2):
  block: transaction support for blockdev-add
  iotests: add blockdev-add-transaction

 qapi/transaction.json                         | 11 +++
 blockdev.c                                    | 80 +++++++++++++------
 .../tests/blockdev-add-transaction            | 52 ++++++++++++
 .../tests/blockdev-add-transaction.out        |  6 ++
 4 files changed, 124 insertions(+), 25 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/blockdev-add-transaction
 create mode 100644 tests/qemu-iotests/tests/blockdev-add-transaction.out

-- 
2.31.1

[PATCH 1/2] block: transaction support for blockdev-add

[Vladimir Sementsov-Ogievskiy]

Simply do blockdev_add() in .prepare() and bdrv_unref() in .abort() and
that's it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/transaction.json | 11 ++++++
 blockdev.c            | 80 +++++++++++++++++++++++++++++--------------
 2 files changed, 66 insertions(+), 25 deletions(-)

diff --git a/qapi/transaction.json b/qapi/transaction.json
index 381a2df782..a938dc7d10 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -53,6 +53,7 @@
 # @blockdev-snapshot-internal-sync: Since 1.7
 # @blockdev-snapshot-sync: since 1.1
 # @drive-backup: Since 1.6
+# @blockdev-add: since 7.0
 #
 # Features:
 # @deprecated: Member @drive-backup is deprecated.  Use member
@@ -66,6 +67,7 @@
             'block-dirty-bitmap-disable', 'block-dirty-bitmap-merge',
             'blockdev-backup', 'blockdev-snapshot',
             'blockdev-snapshot-internal-sync', 'blockdev-snapshot-sync',
+            'blockdev-add',
             { 'name': 'drive-backup', 'features': [ 'deprecated' ] } ] }
 
 ##
@@ -140,6 +142,14 @@
 { 'struct': 'DriveBackupWrapper',
   'data': { 'data': 'DriveBackup' } }
 
+##
+# @BlockdevAddWrapper:
+#
+# Since: 7.0
+##
+{ 'struct': 'BlockdevAddWrapper',
+  'data': { 'data': 'BlockdevOptions' } }
+
 ##
 # @TransactionAction:
 #
@@ -163,6 +173,7 @@
        'blockdev-snapshot': 'BlockdevSnapshotWrapper',
        'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternalWrapper',
        'blockdev-snapshot-sync': 'BlockdevSnapshotSyncWrapper',
+       'blockdev-add': 'BlockdevAddWrapper',
        'drive-backup': 'DriveBackupWrapper'
    } }
 
diff --git a/blockdev.c b/blockdev.c
index 42e098b458..eb9ad9cb89 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2180,6 +2180,55 @@ static void abort_commit(BlkActionState *common)
     g_assert_not_reached(); /* this action never succeeds */
 }
 
+static BlockDriverState *blockdev_add(BlockdevOptions *options, Error **errp)
+{
+    BlockDriverState *bs = NULL;
+    QObject *obj;
+    Visitor *v = qobject_output_visitor_new(&obj);
+    QDict *qdict;
+
+    visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
+    visit_complete(v, &obj);
+    qdict = qobject_to(QDict, obj);
+
+    qdict_flatten(qdict);
+
+    if (!qdict_get_try_str(qdict, "node-name")) {
+        error_setg(errp, "'node-name' must be specified for the root node");
+        goto fail;
+    }
+
+    bs = bds_tree_init(qdict, errp);
+    if (!bs) {
+        goto fail;
+    }
+
+    bdrv_set_monitor_owned(bs);
+
+fail:
+    visit_free(v);
+    return bs;
+}
+
+typedef struct BlockdevAddState {
+    BlkActionState common;
+    BlockDriverState *bs;
+} BlockdevAddState;
+
+static void blockdev_add_prepare(BlkActionState *common, Error **errp)
+{
+    BlockdevAddState *s = DO_UPCAST(BlockdevAddState, common, common);
+
+    s->bs = blockdev_add(common->action->u.blockdev_add.data, errp);
+}
+
+static void blockdev_add_abort(BlkActionState *common)
+{
+    BlockdevAddState *s = DO_UPCAST(BlockdevAddState, common, common);
+
+    bdrv_unref(s->bs);
+}
+
 static const BlkActionOps actions[] = {
     [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT] = {
         .instance_size = sizeof(ExternalSnapshotState),
@@ -2253,6 +2302,11 @@ static const BlkActionOps actions[] = {
         .commit = block_dirty_bitmap_remove_commit,
         .abort = block_dirty_bitmap_remove_abort,
     },
+    [TRANSACTION_ACTION_KIND_BLOCKDEV_ADD] = {
+        .instance_size = sizeof(BlockdevAddState),
+        .prepare = blockdev_add_prepare,
+        .abort = blockdev_add_abort,
+    },
     /* Where are transactions for MIRROR, COMMIT and STREAM?
      * Although these blockjobs use transaction callbacks like the backup job,
      * these jobs do not necessarily adhere to transaction semantics.
@@ -3499,31 +3553,7 @@ out:
 
 void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
 {
-    BlockDriverState *bs;
-    QObject *obj;
-    Visitor *v = qobject_output_visitor_new(&obj);
-    QDict *qdict;
-
-    visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
-    visit_complete(v, &obj);
-    qdict = qobject_to(QDict, obj);
-
-    qdict_flatten(qdict);
-
-    if (!qdict_get_try_str(qdict, "node-name")) {
-        error_setg(errp, "'node-name' must be specified for the root node");
-        goto fail;
-    }
-
-    bs = bds_tree_init(qdict, errp);
-    if (!bs) {
-        goto fail;
-    }
-
-    bdrv_set_monitor_owned(bs);
-
-fail:
-    visit_free(v);
+    blockdev_add(options, errp);
 }
 
 void qmp_blockdev_reopen(BlockdevOptionsList *reopen_list, Error **errp)
-- 
2.31.1

[PATCH 2/2] iotests: add blockdev-add-transaction

[Vladimir Sementsov-Ogievskiy]

Add a test for transaction support of blockdev-add.

Test is format-agnostic, so limit it to qcow2 to avoid extra test runs.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 .../tests/blockdev-add-transaction            | 52 +++++++++++++++++++
 .../tests/blockdev-add-transaction.out        |  6 +++
 2 files changed, 58 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/blockdev-add-transaction
 create mode 100644 tests/qemu-iotests/tests/blockdev-add-transaction.out

diff --git a/tests/qemu-iotests/tests/blockdev-add-transaction b/tests/qemu-iotests/tests/blockdev-add-transaction
new file mode 100755
index 0000000000..ce3c1c069b
--- /dev/null
+++ b/tests/qemu-iotests/tests/blockdev-add-transaction
@@ -0,0 +1,52 @@
+#!/usr/bin/env python3
+#
+# Test blockdev-add transaction action
+#
+# Copyright (c) 2022 Virtuozzo International GmbH.
+#
+# 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 iotests
+from iotests import log
+
+iotests.script_initialize(supported_fmts=['qcow2'])
+
+with iotests.VM() as vm:
+    vm.launch()
+
+    # Use same node-name for nodes, neither one should appear.
+    vm.qmp_log('transaction', actions=[
+        {'type': 'blockdev-add',
+         'data': {'node-name': 'node0', 'driver': 'null-co',
+                  'size': 1024 * 1024}},
+        {'type': 'blockdev-add',
+         'data': {'node-name': 'node0', 'driver': 'null-co',
+                  'size': 1024 * 1024}}
+    ])
+
+    n = len(vm.qmp('query-named-block-nodes')['return'])
+    log(f'Created {n} nodes')
+
+    vm.qmp_log('transaction', actions=[
+        {'type': 'blockdev-add',
+         'data': {'node-name': 'node0', 'driver': 'null-co',
+                  'size': 1024 * 1024}},
+        {'type': 'blockdev-add',
+         'data': {'node-name': 'node1', 'driver': 'null-co',
+                  'size': 1024 * 1024}}
+    ])
+
+    n = len(vm.qmp('query-named-block-nodes')['return'])
+    log(f'Created {n} nodes')
diff --git a/tests/qemu-iotests/tests/blockdev-add-transaction.out b/tests/qemu-iotests/tests/blockdev-add-transaction.out
new file mode 100644
index 0000000000..7e6cd5a9a3
--- /dev/null
+++ b/tests/qemu-iotests/tests/blockdev-add-transaction.out
@@ -0,0 +1,6 @@
+{"execute": "transaction", "arguments": {"actions": [{"data": {"driver": "null-co", "node-name": "node0", "size": 1048576}, "type": "blockdev-add"}, {"data": {"driver": "null-co", "node-name": "node0", "size": 1048576}, "type": "blockdev-add"}]}}
+{"error": {"class": "GenericError", "desc": "Duplicate nodes with node-name='node0'"}}
+Created 0 nodes
+{"execute": "transaction", "arguments": {"actions": [{"data": {"driver": "null-co", "node-name": "node0", "size": 1048576}, "type": "blockdev-add"}, {"data": {"driver": "null-co", "node-name": "node1", "size": 1048576}, "type": "blockdev-add"}]}}
+{"return": {}}
+Created 2 nodes
-- 
2.31.1