Message ID | 1452676712-24239-4-git-send-email-xiecl.fnst@cn.fujitsu.com |
---|---|
State | New |
Headers | show |
On Wed, Jan 13, 2016 at 05:18:27PM +0800, Changlong Xie wrote: > diff --git a/blockjob.c b/blockjob.c > index 80adb9d..0c8edfe 100644 > --- a/blockjob.c > +++ b/blockjob.c > @@ -533,3 +533,14 @@ void block_job_txn_add_job(BlockJobTxn *txn, BlockJob *job) > QLIST_INSERT_HEAD(&txn->jobs, job, txn_list); > block_job_txn_ref(txn); > } > + > +void block_job_do_checkpoint(BlockJob *job, Error **errp) > +{ > + if (!job->driver->do_checkpoint) { > + error_setg(errp, "The job %s doesn't support block checkpoint", > + BlockJobType_lookup[job->driver->job_type]); > + return; > + } > + > + job->driver->do_checkpoint(job, errp); > +} > diff --git a/include/block/blockjob.h b/include/block/blockjob.h > index d84ccd8..abdba7c 100644 > --- a/include/block/blockjob.h > +++ b/include/block/blockjob.h > @@ -70,6 +70,9 @@ typedef struct BlockJobDriver { > * never both. > */ > void (*abort)(BlockJob *job); > + > + /** Optional callback for job types that support checkpoint. */ > + void (*do_checkpoint)(BlockJob *job, Error **errp); The COLO/replication-specific callbacks have been moved out of BlockDriver into their own replication struct. Similar reasoning applies to BlockJobDriver: The do_checkpoint() callback is only implemented by one type of job and its purpose is related to COLO rather than jobs. This is a strong indication that this shouldn't be part of the generic BlockJobDriver struct. Please drop changes to the generic blockjob interface. Instead, make backup_do_checkpoint() public and add assert(job->driver->type == BLOCK_JOB_TYPE_BACKUP) into the function. Then the replication filter can call backup_do_checkpoint() directly. Stefan
On 01/28/2016 12:05 AM, Stefan Hajnoczi wrote: > On Wed, Jan 13, 2016 at 05:18:27PM +0800, Changlong Xie wrote: >> diff --git a/blockjob.c b/blockjob.c >> index 80adb9d..0c8edfe 100644 >> --- a/blockjob.c >> +++ b/blockjob.c >> @@ -533,3 +533,14 @@ void block_job_txn_add_job(BlockJobTxn *txn, BlockJob *job) >> QLIST_INSERT_HEAD(&txn->jobs, job, txn_list); >> block_job_txn_ref(txn); >> } >> + >> +void block_job_do_checkpoint(BlockJob *job, Error **errp) >> +{ >> + if (!job->driver->do_checkpoint) { >> + error_setg(errp, "The job %s doesn't support block checkpoint", >> + BlockJobType_lookup[job->driver->job_type]); >> + return; >> + } >> + >> + job->driver->do_checkpoint(job, errp); >> +} >> diff --git a/include/block/blockjob.h b/include/block/blockjob.h >> index d84ccd8..abdba7c 100644 >> --- a/include/block/blockjob.h >> +++ b/include/block/blockjob.h >> @@ -70,6 +70,9 @@ typedef struct BlockJobDriver { >> * never both. >> */ >> void (*abort)(BlockJob *job); >> + >> + /** Optional callback for job types that support checkpoint. */ >> + void (*do_checkpoint)(BlockJob *job, Error **errp); > > The COLO/replication-specific callbacks have been moved out of > BlockDriver into their own replication struct. Similar reasoning > applies to BlockJobDriver: > > The do_checkpoint() callback is only implemented by one type of job and > its purpose is related to COLO rather than jobs. This is a strong > indication that this shouldn't be part of the generic BlockJobDriver > struct. > > Please drop changes to the generic blockjob interface. Instead, make > backup_do_checkpoint() public and add assert(job->driver->type == > BLOCK_JOB_TYPE_BACKUP) into the function. > > Then the replication filter can call backup_do_checkpoint() directly. > Will fix it in next version. Thanks -Xie > Stefan >
diff --git a/block/backup.c b/block/backup.c index 705bb77..0a27d01 100644 --- a/block/backup.c +++ b/block/backup.c @@ -253,11 +253,25 @@ static void backup_abort(BlockJob *job) } } +static void backup_do_checkpoint(BlockJob *job, Error **errp) +{ + BackupBlockJob *backup_job = container_of(job, BackupBlockJob, common); + + if (backup_job->sync_mode != MIRROR_SYNC_MODE_NONE) { + error_setg(errp, "The backup job only supports block checkpoint in" + " sync=none mode"); + return; + } + + hbitmap_reset_all(backup_job->bitmap); +} + static const BlockJobDriver backup_job_driver = { .instance_size = sizeof(BackupBlockJob), .job_type = BLOCK_JOB_TYPE_BACKUP, .set_speed = backup_set_speed, .iostatus_reset = backup_iostatus_reset, + .do_checkpoint = backup_do_checkpoint, .commit = backup_commit, .abort = backup_abort, }; diff --git a/blockjob.c b/blockjob.c index 80adb9d..0c8edfe 100644 --- a/blockjob.c +++ b/blockjob.c @@ -533,3 +533,14 @@ void block_job_txn_add_job(BlockJobTxn *txn, BlockJob *job) QLIST_INSERT_HEAD(&txn->jobs, job, txn_list); block_job_txn_ref(txn); } + +void block_job_do_checkpoint(BlockJob *job, Error **errp) +{ + if (!job->driver->do_checkpoint) { + error_setg(errp, "The job %s doesn't support block checkpoint", + BlockJobType_lookup[job->driver->job_type]); + return; + } + + job->driver->do_checkpoint(job, errp); +} diff --git a/include/block/blockjob.h b/include/block/blockjob.h index d84ccd8..abdba7c 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -70,6 +70,9 @@ typedef struct BlockJobDriver { * never both. */ void (*abort)(BlockJob *job); + + /** Optional callback for job types that support checkpoint. */ + void (*do_checkpoint)(BlockJob *job, Error **errp); } BlockJobDriver; /** @@ -443,4 +446,13 @@ void block_job_txn_unref(BlockJobTxn *txn); */ void block_job_txn_add_job(BlockJobTxn *txn, BlockJob *job); +/** + * block_job_do_checkpoint: + * @job: The job. + * @errp: Error object. + * + * Do block checkpoint on the specified job. + */ +void block_job_do_checkpoint(BlockJob *job, Error **errp); + #endif