Commit 2288ccfa authored by Sergio Lopez's avatar Sergio Lopez Committed by Kevin Wolf
Browse files

blockdev: unify qmp_drive_backup and drive-backup transaction paths 



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: default avatarSergio Lopez <slp@redhat.com>
Reviewed-by: default avatarKevin Wolf <kwolf@redhat.com>
Signed-off-by: default avatarKevin Wolf <kwolf@redhat.com>
parent 471ded69

blockdev.c

+100 −124
Original line number Diff line number Diff line
@@ -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)

void qmp_drive_backup(DriveBackup *backup, 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)

{



    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)

tests/qemu-iotests/141.out

+2 −0
Original line number Diff line number Diff line
@@ -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'}}

tests/qemu-iotests/185.out

+2 −0
Original line number Diff line number Diff line
@@ -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": {}}

tests/qemu-iotests/219

+5 −2
Original line number Diff line number Diff line
@@ -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',

tests/qemu-iotests/219.out

+8 −0
Original line number Diff line number Diff line
@@ -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"}]}