From patchwork Wed Jan 8 14:31:32 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sergio Lopez X-Patchwork-Id: 11323809 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 381AA109A for ; Wed, 8 Jan 2020 14:36:07 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id F176820692 for ; Wed, 8 Jan 2020 14:36:06 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="KokFpylG" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org F176820692 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Received: from localhost ([::1]:44900 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ipCRJ-00064h-No for patchwork-qemu-devel@patchwork.kernel.org; Wed, 08 Jan 2020 09:36:05 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:51186) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ipCNJ-0000go-Az for qemu-devel@nongnu.org; Wed, 08 Jan 2020 09:31:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ipCNG-0005on-No for qemu-devel@nongnu.org; Wed, 08 Jan 2020 09:31:57 -0500 Received: from us-smtp-1.mimecast.com ([207.211.31.81]:49213 helo=us-smtp-delivery-1.mimecast.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1ipCNG-0005nZ-Ik for qemu-devel@nongnu.org; Wed, 08 Jan 2020 09:31:54 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1578493912; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=R4PDYY4drR8SW04eaejEMfh7wey6OLKt5bw/SwLJNlE=; b=KokFpylGoEtz5vU9ed/PlUxYzuAGylGZEp6WAJuYLHePrI36DoB1+cR0ivnVx8NKIVN8nr 0LpzbQ4p7Y7tbYY7IDjY5tvXQjHJM246oo/47svX4PlC33hMHVQG4m+g7+Q5LCAkI0gqmw A3xkfW7/OFNTwDkfGtjAt0WPjynsdx8= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-355-rfyr3q0RPdOXPpTWjSPF7g-1; Wed, 08 Jan 2020 09:31:51 -0500 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id A5BE41800D4E; Wed, 8 Jan 2020 14:31:50 +0000 (UTC) Received: from dritchie.redhat.com (unknown [10.33.36.107]) by smtp.corp.redhat.com (Postfix) with ESMTP id CB9C36FF0F; Wed, 8 Jan 2020 14:31:48 +0000 (UTC) From: Sergio Lopez To: qemu-devel@nongnu.org Subject: [PATCH v6 2/8] blockdev: unify qmp_drive_backup and drive-backup transaction paths Date: Wed, 8 Jan 2020 15:31:32 +0100 Message-Id: <20200108143138.129909-3-slp@redhat.com> In-Reply-To: <20200108143138.129909-1-slp@redhat.com> References: <20200108143138.129909-1-slp@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-MC-Unique: rfyr3q0RPdOXPpTWjSPF7g-1 X-Mimecast-Spam-Score: 0 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 207.211.31.81 X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Kevin Wolf , qemu-block@nongnu.org, Sergio Lopez , Markus Armbruster , Max Reitz , John Snow Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" 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 Reviewed-by: Kevin Wolf --- 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 553e315972..5e85fc042e 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 3645675ce8..263b680bdf 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 8379ac5854..9a3b65782b 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 e0c51662c0..655f54d881 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 8ebd3fee60..0ea5d0b9d5 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"}]}