| Message ID | 1355725509-5429-4-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 12/16/2012 11:25 PM, Wenchao Xia wrote: > This patch added API to take snapshots in unified style for > both internal or external type. The core structure is based > on transaction, for that there is a qmp interface need to support > , qmp_transaction, so all operations are packed as requests. > In this way a sperate internal layer for snapshot is splitted > out from qmp layer, and now qmp can just translate the user request > and fill in internal API. Internal API use params defined inside > qemu, so other component inside qemu can use it without considering > the qmp's parameter format. > > +typedef struct SNTime { > + uint32_t date_sec; /* UTC date of the snapshot */ Relative to what? Seconds since Epoch? Shouldn't this be 64-bits, to avoid wraparound problems in 2038? > +typedef struct BlkSnapshotInternal { > + /* caller input */ > + const char *sn_name; /* must be set in create/delete. */ > + BlockDriverState *bs; /* must be set in create/delete */ > + SNTime time; /* must be set in create. */ > + uint64_t vm_state_size; /* optional, default is 0, only valid in create. */ > + /* following were used internal */ Prefer present tense: The following are for internal use > + > +/* for simple sync type params were all put here ignoring the difference of > + different operation type as create/delete. */ > +typedef struct BlkTransactionStatesSync { Again, prefer present tense (avoid 'were' in comments). > +/* async snapshot, not supported now */ > +typedef struct BlkTransactionStatesAsync { > + int reserved; > +} BlkTransactionStatesAsync; Why declare a struct if we aren't supporting it yet?
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: > + > +typedef struct SNTime { > + uint32_t date_sec; /* UTC date of the snapshot */ > + uint32_t date_nsec; This two fields are just struct timespec, does it makes sense to use it? > + uint64_t vm_clock_nsec; /* VM clock relative to boot */ > +} SNTime; > + > +typedef enum BlkSnapshotIntStep { > + BLK_SNAPSHOT_INT_START = 0, > + BLK_SNAPSHOT_INT_CREATED, > + BLK_SNAPSHOT_INT_CANCELED, > +} BlkSnapshotIntStep; > + > +typedef struct BlkSnapshotInternal { > + /* caller input */ > + const char *sn_name; /* must be set in create/delete. */ > + BlockDriverState *bs; /* must be set in create/delete */ > + SNTime time; /* must be set in create. */ > + uint64_t vm_state_size; /* optional, default is 0, only valid in create. */ > + /* following were used internal */ > + QEMUSnapshotInfo sn; > + QEMUSnapshotInfo old_sn; > + bool old_sn_exist; > + BlkSnapshotIntStep step; > +} BlkSnapshotInternal; > + > +/* external snapshot */ > + > +typedef enum BlkSnapshotExtStep { > + BLK_SNAPSHOT_EXT_START = 0, > + BLK_SNAPSHOT_EXT_CREATED, > + BLK_SNAPSHOT_EXT_INVALIDATED, > + BLK_SNAPSHOT_EXT_CANCELED, > +} BlkSnapshotExtStep; > + > +typedef struct BlkSnapshotExternal { > + /* caller input */ > + const char *new_image_file; /* must be set in create/delete. */ > + BlockDriverState *old_bs; /* must be set in create/delete. */ > + const char *format; /* must be set in create. */ > + /* following were used internal */ > + BlockDriverState *new_bs; > + BlockDriver *format_drv; > + BlkSnapshotExtStep step; > +} BlkSnapshotExternal; > + > +typedef enum BlkSnapshotType { > + BLK_SNAPSHOT_INTERNAL = 0, > + BLK_SNAPSHOT_EXTERNAL, > + BLK_SNAPSHOT_NOSUPPORT, > +} BlkSnapshotType; > + > +/* for simple sync type params were all put here ignoring the difference of > + different operation type as create/delete. */ > +typedef struct BlkTransactionStatesSync { > + /* caller input */ > + BlkSnapshotType type; > + union { > + BlkSnapshotInternal internal; > + BlkSnapshotExternal external; > + }; > + bool use_existing; > + BlkTransactionOperationSync op; > +} BlkTransactionStatesSync; > + > +/* async snapshot, not supported now */ > +typedef struct BlkTransactionStatesAsync { > + int reserved; > +} BlkTransactionStatesAsync; > + > +/* Core structure for group snapshots, fill in it and then call the API. */ > +typedef struct BlkTransactionStates BlkTransactionStates; > + > +struct BlkTransactionStates { > + /* caller input */ > + bool async; Why do we have this variable? As far as I can see, we only test its value, and set it to false. Are we missing any patch? > + union { > + BlkTransactionStatesSync st_sync; > + BlkTransactionStatesSync st_async; > + }; > + /* following were used internal */ > + Error *err; > + int (*blk_trans_do)(BlkTransactionStates *states, Error **errp); > + int (*blk_trans_invalid)(BlkTransactionStates *states, Error **errp); > + int (*blk_trans_cancel)(BlkTransactionStates *states, Error **errp); > + QSIMPLEQ_ENTRY(BlkTransactionStates) entry; > +}; > + > +typedef QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionStates) \ > + BlkTransactionStatesList; > + > +/* API */ > +BlkTransactionStates *blk_trans_st_new(void); > +void blk_trans_st_delete(BlkTransactionStates **p_st); > +BlkTransactionStatesList *blk_trans_st_list_new(void); > +void blk_trans_st_list_delete(BlkTransactionStatesList **p_list); > + > +/* add a request to list, request would be checked to see if it is valid, > + return -1 when met error and states would not be queued. */ > +int add_transaction(BlkTransactionStatesList *list, > + BlkTransactionStates *states, > + Error **errp); > + > +/* 'Atomic' submit the request. In snapshot creation case, if any > + * fail then we do not pivot any of the devices in the group, and abandon the > + * snapshots > + */ > +int submit_transaction(BlkTransactionStatesList *list, Error **errp); > + > +/* helper */ > +SNTime get_sn_time(void); > +void generate_sn_name_from_time(SNTime *time, char *time_str, int size); > #endif
δΊ 2012-12-22 2:49, Juan Quintela ει: > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: >> + >> +typedef struct SNTime { >> + uint32_t date_sec; /* UTC date of the snapshot */ >> + uint32_t date_nsec; > > This two fields are just struct timespec, does it makes sense to use it? > make sense, I did not notice timespec before, will use it if windows support timespec too. >> + >> +/* Core structure for group snapshots, fill in it and then call the API. */ >> +typedef struct BlkTransactionStates BlkTransactionStates; >> + >> +struct BlkTransactionStates { >> + /* caller input */ >> + bool async; > > Why do we have this variable? As far as I can see, we only test its > value, and set it to false. Are we missing any patch? > No, I just put it here as a reserved option. Maybe live block commit can be considered as async deletion of snapshot, but nevermind I'll delete it for that it is not supported now.
> On 12/16/2012 11:25 PM, Wenchao Xia wrote: >> This patch added API to take snapshots in unified style for >> both internal or external type. The core structure is based >> on transaction, for that there is a qmp interface need to support >> , qmp_transaction, so all operations are packed as requests. >> In this way a sperate internal layer for snapshot is splitted >> out from qmp layer, and now qmp can just translate the user request >> and fill in internal API. Internal API use params defined inside >> qemu, so other component inside qemu can use it without considering >> the qmp's parameter format. >> > >> +typedef struct SNTime { >> + uint32_t date_sec; /* UTC date of the snapshot */ > > Relative to what? Seconds since Epoch? Shouldn't this be 64-bits, to > avoid wraparound problems in 2038? > original code for snapshot time is uint32_t, but I think 64bits is better. >> +typedef struct BlkSnapshotInternal { >> + /* caller input */ >> + const char *sn_name; /* must be set in create/delete. */ >> + BlockDriverState *bs; /* must be set in create/delete */ >> + SNTime time; /* must be set in create. */ >> + uint64_t vm_state_size; /* optional, default is 0, only valid in create. */ >> + /* following were used internal */ > > Prefer present tense: The following are for internal use > OK. >> + >> +/* for simple sync type params were all put here ignoring the difference of >> + different operation type as create/delete. */ >> +typedef struct BlkTransactionStatesSync { > > Again, prefer present tense (avoid 'were' in comments). > >> +/* async snapshot, not supported now */ >> +typedef struct BlkTransactionStatesAsync { >> + int reserved; >> +} BlkTransactionStatesAsync; > > Why declare a struct if we aren't supporting it yet? > Just a reserve value for type completion
diff --git a/blockdev.h b/blockdev.h index d73d552..4a1b508 100644 --- a/blockdev.h +++ b/blockdev.h @@ -66,4 +66,133 @@ void qmp_change_blockdev(const char *device, const char *filename, bool has_format, const char *format, Error **errp); void do_commit(Monitor *mon, const QDict *qdict); int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data); + + +/* snapshot transaction API. + * Split out a layer around core struct BlkTransactionStates, so other + * component in qemu can fill the request and simply use the API to submit, + * QMP may just use part of the API's function, no need to expose all internal + * function to user. + */ + +/* sync snapshot */ + +typedef enum BlkTransactionOperationSync { + BLK_SN_SYNC_CREATE = 0, + BLK_SN_SYNC_DELETE, +} BlkTransactionOperationSync; + +/* internal snapshot */ + +typedef struct SNTime { + uint32_t date_sec; /* UTC date of the snapshot */ + uint32_t date_nsec; + uint64_t vm_clock_nsec; /* VM clock relative to boot */ +} SNTime; + +typedef enum BlkSnapshotIntStep { + BLK_SNAPSHOT_INT_START = 0, + BLK_SNAPSHOT_INT_CREATED, + BLK_SNAPSHOT_INT_CANCELED, +} BlkSnapshotIntStep; + +typedef struct BlkSnapshotInternal { + /* caller input */ + const char *sn_name; /* must be set in create/delete. */ + BlockDriverState *bs; /* must be set in create/delete */ + SNTime time; /* must be set in create. */ + uint64_t vm_state_size; /* optional, default is 0, only valid in create. */ + /* following were used internal */ + QEMUSnapshotInfo sn; + QEMUSnapshotInfo old_sn; + bool old_sn_exist; + BlkSnapshotIntStep step; +} BlkSnapshotInternal; + +/* external snapshot */ + +typedef enum BlkSnapshotExtStep { + BLK_SNAPSHOT_EXT_START = 0, + BLK_SNAPSHOT_EXT_CREATED, + BLK_SNAPSHOT_EXT_INVALIDATED, + BLK_SNAPSHOT_EXT_CANCELED, +} BlkSnapshotExtStep; + +typedef struct BlkSnapshotExternal { + /* caller input */ + const char *new_image_file; /* must be set in create/delete. */ + BlockDriverState *old_bs; /* must be set in create/delete. */ + const char *format; /* must be set in create. */ + /* following were used internal */ + BlockDriverState *new_bs; + BlockDriver *format_drv; + BlkSnapshotExtStep step; +} BlkSnapshotExternal; + +typedef enum BlkSnapshotType { + BLK_SNAPSHOT_INTERNAL = 0, + BLK_SNAPSHOT_EXTERNAL, + BLK_SNAPSHOT_NOSUPPORT, +} BlkSnapshotType; + +/* for simple sync type params were all put here ignoring the difference of + different operation type as create/delete. */ +typedef struct BlkTransactionStatesSync { + /* caller input */ + BlkSnapshotType type; + union { + BlkSnapshotInternal internal; + BlkSnapshotExternal external; + }; + bool use_existing; + BlkTransactionOperationSync op; +} BlkTransactionStatesSync; + +/* async snapshot, not supported now */ +typedef struct BlkTransactionStatesAsync { + int reserved; +} BlkTransactionStatesAsync; + +/* Core structure for group snapshots, fill in it and then call the API. */ +typedef struct BlkTransactionStates BlkTransactionStates; + +struct BlkTransactionStates { + /* caller input */ + bool async; + union { + BlkTransactionStatesSync st_sync; + BlkTransactionStatesSync st_async; + }; + /* following were used internal */ + Error *err; + int (*blk_trans_do)(BlkTransactionStates *states, Error **errp); + int (*blk_trans_invalid)(BlkTransactionStates *states, Error **errp); + int (*blk_trans_cancel)(BlkTransactionStates *states, Error **errp); + QSIMPLEQ_ENTRY(BlkTransactionStates) entry; +}; + +typedef QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionStates) \ + BlkTransactionStatesList; + +/* API */ +BlkTransactionStates *blk_trans_st_new(void); +void blk_trans_st_delete(BlkTransactionStates **p_st); +BlkTransactionStatesList *blk_trans_st_list_new(void); +void blk_trans_st_list_delete(BlkTransactionStatesList **p_list); + +/* add a request to list, request would be checked to see if it is valid, + return -1 when met error and states would not be queued. */ +int add_transaction(BlkTransactionStatesList *list, + BlkTransactionStates *states, + Error **errp); + +/* 'Atomic' submit the request. In snapshot creation case, if any + * fail then we do not pivot any of the devices in the group, and abandon the + * snapshots + */ +int submit_transaction(BlkTransactionStatesList *list, Error **errp); + +/* helper */ +SNTime get_sn_time(void); +void generate_sn_name_from_time(SNTime *time, char *time_str, int size); #endif
This patch added API to take snapshots in unified style for both internal or external type. The core structure is based on transaction, for that there is a qmp interface need to support , qmp_transaction, so all operations are packed as requests. In this way a sperate internal layer for snapshot is splitted out from qmp layer, and now qmp can just translate the user request and fill in internal API. Internal API use params defined inside qemu, so other component inside qemu can use it without considering the qmp's parameter format. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- blockdev.h | 129 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 129 insertions(+), 0 deletions(-)