| Message ID | 20190703215542.16123-3-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | bitmaps: introduce 'bitmap' sync mode | expand |
On 03.07.19 23:55, John Snow wrote: > Create a common core that comprises the actual meat of what the backup API > boundary needs to do, and then switch drive-backup to use it. > > Questions: > - do_drive_backup now acquires and releases the aio_context in addition > to do_backup_common doing the same. Can I drop this from drive_backup? I wonder why you don’t just make it a requirement that do_backup_common() is called with the context acquired? > Signed-off-by: John Snow <jsnow@redhat.com> > --- > blockdev.c | 138 ++++++++++++++++++++++++++++++++--------------------- > 1 file changed, 83 insertions(+), 55 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index 4d141e9a1f..5fd663a7e5 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -3425,6 +3425,86 @@ out: > aio_context_release(aio_context); > } > > +static BlockJob *do_backup_common(BackupCommon *backup, > + BlockDriverState *target_bs, > + JobTxn *txn, Error **errp) > +{ [...] > + job = backup_job_create(backup->job_id, bs, target_bs, backup->speed, > + backup->sync, bmap, backup->compress, > + backup->on_source_error, backup->on_target_error, > + job_flags, NULL, NULL, txn, &local_err); > + if (local_err != NULL) { > + error_propagate(errp, local_err); > + goto out; > + } Below, you change do_drive_backup() to just pass errp instead of local_err and not do error handling. Why not do the same here? Other than that: Reviewed-by: Max Reitz <mreitz@redhat.com> > + > +out: > + aio_context_release(aio_context); > + return job; > +}
On 7/4/19 11:06 AM, Max Reitz wrote: > On 03.07.19 23:55, John Snow wrote: >> Create a common core that comprises the actual meat of what the backup API >> boundary needs to do, and then switch drive-backup to use it. >> >> Questions: >> - do_drive_backup now acquires and releases the aio_context in addition >> to do_backup_common doing the same. Can I drop this from drive_backup? > > I wonder why you don’t just make it a requirement that > do_backup_common() is called with the context acquired? > Ehm, it just honestly didn't occur to me that I could acquire the context before doing the input sanitizing. In this case, it is OK to do it, so I will hoist it back up into do_blockdev_backup. --js >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> blockdev.c | 138 ++++++++++++++++++++++++++++++++--------------------- >> 1 file changed, 83 insertions(+), 55 deletions(-) >> >> diff --git a/blockdev.c b/blockdev.c >> index 4d141e9a1f..5fd663a7e5 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -3425,6 +3425,86 @@ out: >> aio_context_release(aio_context); >> } >> >> +static BlockJob *do_backup_common(BackupCommon *backup, >> + BlockDriverState *target_bs, >> + JobTxn *txn, Error **errp) >> +{ > > [...] > >> + job = backup_job_create(backup->job_id, bs, target_bs, backup->speed, >> + backup->sync, bmap, backup->compress, >> + backup->on_source_error, backup->on_target_error, >> + job_flags, NULL, NULL, txn, &local_err); >> + if (local_err != NULL) { >> + error_propagate(errp, local_err); >> + goto out; >> + } > > Below, you change do_drive_backup() to just pass errp instead of > local_err and not do error handling. Why not do the same here? > Inertia. > Other than that: > > Reviewed-by: Max Reitz <mreitz@redhat.com> > Suggestions applied, thank you.
On 7/4/19 11:06 AM, Max Reitz wrote: > On 03.07.19 23:55, John Snow wrote: >> Create a common core that comprises the actual meat of what the backup API >> boundary needs to do, and then switch drive-backup to use it. >> >> Questions: >> - do_drive_backup now acquires and releases the aio_context in addition >> to do_backup_common doing the same. Can I drop this from drive_backup? > > I wonder why you don’t just make it a requirement that > do_backup_common() is called with the context acquired? > Oh, I remember: I wasn't actually sure if any of the calls that drive-backup makes actually requires the AioContext. If that's true, no reason to acquire it in two places and pass it in to the common function. Probably bdrv_getlength and bdrv_refresh_filename both need it, though. (Or at least, I can't promise they never wouldn't.) --js
diff --git a/blockdev.c b/blockdev.c index 4d141e9a1f..5fd663a7e5 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3425,6 +3425,86 @@ out: aio_context_release(aio_context); } +static BlockJob *do_backup_common(BackupCommon *backup, + BlockDriverState *target_bs, + JobTxn *txn, Error **errp) +{ + BlockDriverState *bs; + BlockJob *job = NULL; + BdrvDirtyBitmap *bmap = NULL; + AioContext *aio_context; + Error *local_err = NULL; + int job_flags = JOB_DEFAULT; + int ret; + + if (!backup->has_speed) { + backup->speed = 0; + } + if (!backup->has_on_source_error) { + backup->on_source_error = BLOCKDEV_ON_ERROR_REPORT; + } + if (!backup->has_on_target_error) { + backup->on_target_error = BLOCKDEV_ON_ERROR_REPORT; + } + if (!backup->has_job_id) { + backup->job_id = NULL; + } + if (!backup->has_auto_finalize) { + backup->auto_finalize = true; + } + if (!backup->has_auto_dismiss) { + backup->auto_dismiss = true; + } + if (!backup->has_compress) { + backup->compress = false; + } + + bs = bdrv_lookup_bs(backup->device, backup->device, errp); + if (!bs) { + return NULL; + } + + aio_context = bdrv_get_aio_context(bs); + aio_context_acquire(aio_context); + + ret = bdrv_try_set_aio_context(target_bs, aio_context, errp); + if (ret < 0) { + bdrv_unref(target_bs); + goto out; + } + + if (backup->has_bitmap) { + bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap); + if (!bmap) { + error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap); + goto out; + } + if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_DEFAULT, errp)) { + goto out; + } + } + + if (!backup->auto_finalize) { + job_flags |= JOB_MANUAL_FINALIZE; + } + if (!backup->auto_dismiss) { + job_flags |= JOB_MANUAL_DISMISS; + } + + job = backup_job_create(backup->job_id, bs, target_bs, backup->speed, + backup->sync, bmap, backup->compress, + backup->on_source_error, backup->on_target_error, + job_flags, NULL, NULL, txn, &local_err); + if (local_err != NULL) { + error_propagate(errp, local_err); + goto out; + } + +out: + aio_context_release(aio_context); + return job; +} + static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, Error **errp) { @@ -3432,39 +3512,16 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, BlockDriverState *target_bs; BlockDriverState *source = NULL; BlockJob *job = NULL; - BdrvDirtyBitmap *bmap = NULL; AioContext *aio_context; QDict *options = NULL; Error *local_err = NULL; - int flags, job_flags = JOB_DEFAULT; + int flags; int64_t size; bool set_backing_hd = false; - int ret; - if (!backup->has_speed) { - backup->speed = 0; - } - if (!backup->has_on_source_error) { - backup->on_source_error = BLOCKDEV_ON_ERROR_REPORT; - } - if (!backup->has_on_target_error) { - backup->on_target_error = BLOCKDEV_ON_ERROR_REPORT; - } if (!backup->has_mode) { backup->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS; } - if (!backup->has_job_id) { - backup->job_id = NULL; - } - if (!backup->has_auto_finalize) { - backup->auto_finalize = true; - } - if (!backup->has_auto_dismiss) { - backup->auto_dismiss = true; - } - if (!backup->has_compress) { - backup->compress = false; - } bs = bdrv_lookup_bs(backup->device, backup->device, errp); if (!bs) { @@ -3541,12 +3598,6 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, goto out; } - ret = bdrv_try_set_aio_context(target_bs, aio_context, errp); - if (ret < 0) { - bdrv_unref(target_bs); - goto out; - } - if (set_backing_hd) { bdrv_set_backing_hd(target_bs, source, &local_err); if (local_err) { @@ -3554,31 +3605,8 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, } } - if (backup->has_bitmap) { - bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap); - if (!bmap) { - error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap); - goto unref; - } - if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_DEFAULT, errp)) { - goto unref; - } - } - if (!backup->auto_finalize) { - job_flags |= JOB_MANUAL_FINALIZE; - } - if (!backup->auto_dismiss) { - job_flags |= JOB_MANUAL_DISMISS; - } - - job = backup_job_create(backup->job_id, bs, target_bs, backup->speed, - backup->sync, bmap, backup->compress, - backup->on_source_error, backup->on_target_error, - job_flags, NULL, NULL, txn, &local_err); - if (local_err != NULL) { - error_propagate(errp, local_err); - goto unref; - } + job = do_backup_common(qapi_DriveBackup_base(backup), + target_bs, txn, errp); unref: bdrv_unref(target_bs);
Create a common core that comprises the actual meat of what the backup API boundary needs to do, and then switch drive-backup to use it. Questions: - do_drive_backup now acquires and releases the aio_context in addition to do_backup_common doing the same. Can I drop this from drive_backup? Signed-off-by: John Snow <jsnow@redhat.com> --- blockdev.c | 138 ++++++++++++++++++++++++++++++++--------------------- 1 file changed, 83 insertions(+), 55 deletions(-)