From 4a03ab2a6cc4974d8d43240d1297b09160818af3 Mon Sep 17 00:00:00 2001

From: Sergio Lopez Pascual <slp@redhat.com>

Date: Fri, 7 Feb 2020 11:27:42 +0000

Subject: [PATCH 09/18] blockdev: unify qmp_drive_backup and drive-backup

 transaction paths

RH-Author: Sergio Lopez Pascual <slp@redhat.com>

Message-id: <20200207112749.25073-3-slp@redhat.com>

Patchwork-id: 93755

O-Subject: [RHEL-AV-8.2.0 qemu-kvm PATCH v2 2/9] blockdev: unify qmp_drive_backup and drive-backup transaction paths

Bugzilla: 1745606 1746217 1773517 1779036 1782111 1782175 1783965

RH-Acked-by: Stefano Garzarella <sgarzare@redhat.com>

RH-Acked-by: Paolo Bonzini <pbonzini@redhat.com>

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

RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com>

Issuing a drive-backup from qmp_drive_backup takes a slightly

different path than when it's issued from a transaction. In the code,

this is manifested as some redundancy between do_drive_backup() and

drive_backup_prepare().

This change unifies both paths, merging do_drive_backup() and

drive_backup_prepare(), and changing qmp_drive_backup() to create a

transaction instead of calling do_backup_common() direcly.

As a side-effect, now qmp_drive_backup() is executed inside a drained

section, as it happens when creating a drive-backup transaction. This

change is visible from the user's perspective, as the job gets paused

and immediately resumed before starting the actual work.

Also fix tests 141, 185 and 219 to cope with the extra

JOB_STATUS_CHANGE lines.

Signed-off-by: Sergio Lopez <slp@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

(cherry picked from commit 2288ccfac96281c316db942d10e3f921c1373064)

Signed-off-by: Sergio Lopez <slp@redhat.com>

Signed-off-by: Danilo C. L. de Paula <ddepaula@redhat.com>

---

 blockdev.c                 | 224 ++++++++++++++++++++-------------------------

 tests/qemu-iotests/141.out |   2 +

 tests/qemu-iotests/185.out |   2 +

 tests/qemu-iotests/219     |   7 +-

 tests/qemu-iotests/219.out |   8 ++

 5 files changed, 117 insertions(+), 126 deletions(-)

diff --git a/blockdev.c b/blockdev.c

index 553e315..5e85fc0 100644

--- a/blockdev.c

+++ b/blockdev.c

@@ -1761,39 +1761,128 @@ typedef struct DriveBackupState {

     BlockJob *job;

 } DriveBackupState;

-static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,

-                            Error **errp);

+static BlockJob *do_backup_common(BackupCommon *backup,

+                                  BlockDriverState *bs,

+                                  BlockDriverState *target_bs,

+                                  AioContext *aio_context,

+                                  JobTxn *txn, Error **errp);

 static void drive_backup_prepare(BlkActionState *common, Error **errp)

 {

     DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);

-    BlockDriverState *bs;

     DriveBackup *backup;

+    BlockDriverState *bs;

+    BlockDriverState *target_bs;

+    BlockDriverState *source = NULL;

     AioContext *aio_context;

+    QDict *options;

     Error *local_err = NULL;

+    int flags;

+    int64_t size;

+    bool set_backing_hd = false;

     assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);

     backup = common->action->u.drive_backup.data;

+    if (!backup->has_mode) {

+        backup->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;

+    }

+

     bs = bdrv_lookup_bs(backup->device, backup->device, errp);

     if (!bs) {

         return;

     }

+    if (!bs->drv) {

+        error_setg(errp, "Device has no medium");

+        return;

+    }

+

     aio_context = bdrv_get_aio_context(bs);

     aio_context_acquire(aio_context);

     /* Paired with .clean() */

     bdrv_drained_begin(bs);

-    state->bs = bs;

+    if (!backup->has_format) {

+        backup->format = backup->mode == NEW_IMAGE_MODE_EXISTING ?

+                         NULL : (char *) bs->drv->format_name;

+    }

+

+    /* Early check to avoid creating target */

+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {

+        goto out;

+    }

+

+    flags = bs->open_flags | BDRV_O_RDWR;

+

+    /*

+     * See if we have a backing HD we can use to create our new image

+     * on top of.

+     */

+    if (backup->sync == MIRROR_SYNC_MODE_TOP) {

+        source = backing_bs(bs);

+        if (!source) {

+            backup->sync = MIRROR_SYNC_MODE_FULL;

+        }

+    }

+    if (backup->sync == MIRROR_SYNC_MODE_NONE) {

+        source = bs;

+        flags |= BDRV_O_NO_BACKING;

+        set_backing_hd = true;

+    }

+

+    size = bdrv_getlength(bs);

+    if (size < 0) {

+        error_setg_errno(errp, -size, "bdrv_getlength failed");

+        goto out;

+    }

+

+    if (backup->mode != NEW_IMAGE_MODE_EXISTING) {

+        assert(backup->format);

+        if (source) {

+            bdrv_refresh_filename(source);

+            bdrv_img_create(backup->target, backup->format, source->filename,

+                            source->drv->format_name, NULL,

+                            size, flags, false, &local_err);

+        } else {

+            bdrv_img_create(backup->target, backup->format, NULL, NULL, NULL,

+                            size, flags, false, &local_err);

+        }

+    }

-    state->job = do_drive_backup(backup, common->block_job_txn, &local_err);

     if (local_err) {

         error_propagate(errp, local_err);

         goto out;

     }

+    options = qdict_new();

+    qdict_put_str(options, "discard", "unmap");

+    qdict_put_str(options, "detect-zeroes", "unmap");

+    if (backup->format) {

+        qdict_put_str(options, "driver", backup->format);

+    }

+

+    target_bs = bdrv_open(backup->target, NULL, options, flags, errp);

+    if (!target_bs) {

+        goto out;

+    }

+

+    if (set_backing_hd) {

+        bdrv_set_backing_hd(target_bs, source, &local_err);

+        if (local_err) {

+            goto unref;

+        }

+    }

+

+    state->bs = bs;

+

+    state->job = do_backup_common(qapi_DriveBackup_base(backup),

+                                  bs, target_bs, aio_context,

+                                  common->block_job_txn, errp);

+

+unref:

+    bdrv_unref(target_bs);

 out:

     aio_context_release(aio_context);

 }

@@ -3587,126 +3676,13 @@ static BlockJob *do_backup_common(BackupCommon *backup,

     return job;

 }

-static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,

-                                 Error **errp)

-{

-    BlockDriverState *bs;

-    BlockDriverState *target_bs;

-    BlockDriverState *source = NULL;

-    BlockJob *job = NULL;

-    AioContext *aio_context;

-    QDict *options;

-    Error *local_err = NULL;

-    int flags;

-    int64_t size;

-    bool set_backing_hd = false;

-

-    if (!backup->has_mode) {

-        backup->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;

-    }

-

-    bs = bdrv_lookup_bs(backup->device, backup->device, errp);

-    if (!bs) {

-        return NULL;

-    }

-

-    if (!bs->drv) {

-        error_setg(errp, "Device has no medium");

-        return NULL;

-    }

-

-    aio_context = bdrv_get_aio_context(bs);

-    aio_context_acquire(aio_context);

-

-    if (!backup->has_format) {

-        backup->format = backup->mode == NEW_IMAGE_MODE_EXISTING ?

-                         NULL : (char *) bs->drv->format_name;

-    }

-

-    /* Early check to avoid creating target */

-    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {

-        goto out;

-    }

-

-    flags = bs->open_flags | BDRV_O_RDWR;

-

-    /*

-     * See if we have a backing HD we can use to create our new image

-     * on top of.

-     */

-    if (backup->sync == MIRROR_SYNC_MODE_TOP) {

-        source = backing_bs(bs);

-        if (!source) {

-            backup->sync = MIRROR_SYNC_MODE_FULL;

-        }

-    }

-    if (backup->sync == MIRROR_SYNC_MODE_NONE) {

-        source = bs;

-        flags |= BDRV_O_NO_BACKING;

-        set_backing_hd = true;

-    }

-

-    size = bdrv_getlength(bs);

-    if (size < 0) {

-        error_setg_errno(errp, -size, "bdrv_getlength failed");

-        goto out;

-    }

-

-    if (backup->mode != NEW_IMAGE_MODE_EXISTING) {

-        assert(backup->format);

-        if (source) {

-            bdrv_refresh_filename(source);

-            bdrv_img_create(backup->target, backup->format, source->filename,

-                            source->drv->format_name, NULL,

-                            size, flags, false, &local_err);

-        } else {

-            bdrv_img_create(backup->target, backup->format, NULL, NULL, NULL,

-                            size, flags, false, &local_err);

-        }

-    }

-

-    if (local_err) {

-        error_propagate(errp, local_err);

-        goto out;

-    }

-

-    options = qdict_new();

-    qdict_put_str(options, "discard", "unmap");

-    qdict_put_str(options, "detect-zeroes", "unmap");

-    if (backup->format) {

-        qdict_put_str(options, "driver", backup->format);

-    }

-

-    target_bs = bdrv_open(backup->target, NULL, options, flags, errp);

-    if (!target_bs) {

-        goto out;

-    }

-

-    if (set_backing_hd) {

-        bdrv_set_backing_hd(target_bs, source, &local_err);

-        if (local_err) {

-            goto unref;

-        }

-    }

-

-    job = do_backup_common(qapi_DriveBackup_base(backup),

-                           bs, target_bs, aio_context, txn, errp);

-

-unref:

-    bdrv_unref(target_bs);

-out:

-    aio_context_release(aio_context);

-    return job;

-}

-

-void qmp_drive_backup(DriveBackup *arg, Error **errp)

+void qmp_drive_backup(DriveBackup *backup, Error **errp)

 {

-

-    BlockJob *job;

-    job = do_drive_backup(arg, NULL, errp);

-    if (job) {

-        job_start(&job->job);

-    }

+    TransactionAction action = {

+        .type = TRANSACTION_ACTION_KIND_DRIVE_BACKUP,

+        .u.drive_backup.data = backup,

+    };

+    blockdev_do_action(&action, errp);

 }

 BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)

diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out

index 3645675..263b680 100644

--- a/tests/qemu-iotests/141.out

+++ b/tests/qemu-iotests/141.out

@@ -13,6 +13,8 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/m.

 Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT

 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}

 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}}

+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "paused", "id": "job0"}}

+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}}

 {'execute': 'blockdev-del', 'arguments': {'node-name': 'drv0'}}

 {"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: node is used as backing hd of 'NODE_NAME'"}}

 {'execute': 'block-job-cancel', 'arguments': {'device': 'job0'}}

diff --git a/tests/qemu-iotests/185.out b/tests/qemu-iotests/185.out

index 8379ac5..9a3b657 100644

--- a/tests/qemu-iotests/185.out

+++ b/tests/qemu-iotests/185.out

@@ -65,6 +65,8 @@ Formatting 'TEST_DIR/t.qcow2.copy', fmt=qcow2 size=67108864 cluster_size=65536 l

 Formatting 'TEST_DIR/t.qcow2.copy', fmt=qcow2 size=67108864 cluster_size=65536 lazy_refcounts=off refcount_bits=16

 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "disk"}}

 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "disk"}}

+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "paused", "id": "disk"}}

+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "disk"}}

 {"return": {}}

 { 'execute': 'quit' }

 {"return": {}}

diff --git a/tests/qemu-iotests/219 b/tests/qemu-iotests/219

index e0c5166..655f54d 100755

--- a/tests/qemu-iotests/219

+++ b/tests/qemu-iotests/219

@@ -63,7 +63,7 @@ def test_pause_resume(vm):

                 # logged immediately

                 iotests.log(vm.qmp('query-jobs'))

-def test_job_lifecycle(vm, job, job_args, has_ready=False):

+def test_job_lifecycle(vm, job, job_args, has_ready=False, is_mirror=False):

     global img_size

     iotests.log('')

@@ -135,6 +135,9 @@ def test_job_lifecycle(vm, job, job_args, has_ready=False):

     iotests.log('Waiting for PENDING state...')

     iotests.log(iotests.filter_qmp_event(vm.event_wait('JOB_STATUS_CHANGE')))

     iotests.log(iotests.filter_qmp_event(vm.event_wait('JOB_STATUS_CHANGE')))

+    if is_mirror:

+        iotests.log(iotests.filter_qmp_event(vm.event_wait('JOB_STATUS_CHANGE')))

+        iotests.log(iotests.filter_qmp_event(vm.event_wait('JOB_STATUS_CHANGE')))

     if not job_args.get('auto-finalize', True):

         # PENDING state:

@@ -218,7 +221,7 @@ with iotests.FilePath('disk.img') as disk_path, \

     for auto_finalize in [True, False]:

         for auto_dismiss in [True, False]:

-            test_job_lifecycle(vm, 'drive-backup', job_args={

+            test_job_lifecycle(vm, 'drive-backup', is_mirror=True, job_args={

                 'device': 'drive0-node',

                 'target': copy_path,

                 'sync': 'full',

diff --git a/tests/qemu-iotests/219.out b/tests/qemu-iotests/219.out

index 8ebd3fe..0ea5d0b 100644

--- a/tests/qemu-iotests/219.out

+++ b/tests/qemu-iotests/219.out

@@ -135,6 +135,8 @@ Pause/resume in RUNNING

 {"return": {}}

 Waiting for PENDING state...

+{"data": {"id": "job0", "status": "paused"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

+{"data": {"id": "job0", "status": "running"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

 {"data": {"id": "job0", "status": "waiting"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

 {"data": {"id": "job0", "status": "pending"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

 {"data": {"id": "job0", "status": "concluded"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

@@ -186,6 +188,8 @@ Pause/resume in RUNNING

 {"return": {}}

 Waiting for PENDING state...

+{"data": {"id": "job0", "status": "paused"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

+{"data": {"id": "job0", "status": "running"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

 {"data": {"id": "job0", "status": "waiting"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

 {"data": {"id": "job0", "status": "pending"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

 {"data": {"id": "job0", "status": "concluded"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

@@ -245,6 +249,8 @@ Pause/resume in RUNNING

 {"return": {}}

 Waiting for PENDING state...

+{"data": {"id": "job0", "status": "paused"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

+{"data": {"id": "job0", "status": "running"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

 {"data": {"id": "job0", "status": "waiting"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

 {"data": {"id": "job0", "status": "pending"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

 {"return": [{"current-progress": 4194304, "id": "job0", "status": "pending", "total-progress": 4194304, "type": "backup"}]}

@@ -304,6 +310,8 @@ Pause/resume in RUNNING

 {"return": {}}

 Waiting for PENDING state...

+{"data": {"id": "job0", "status": "paused"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

+{"data": {"id": "job0", "status": "running"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

 {"data": {"id": "job0", "status": "waiting"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

 {"data": {"id": "job0", "status": "pending"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

 {"return": [{"current-progress": 4194304, "id": "job0", "status": "pending", "total-progress": 4194304, "type": "backup"}]}

-- 

1.8.3.1