From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53753) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UFRMu-0004Qm-Ow for qemu-devel@nongnu.org; Tue, 12 Mar 2013 11:44:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UFRMo-0008NB-4j for qemu-devel@nongnu.org; Tue, 12 Mar 2013 11:44:00 -0400 Received: from mail-wi0-x22b.google.com ([2a00:1450:400c:c05::22b]:46059) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UFRMn-0008Mr-RY for qemu-devel@nongnu.org; Tue, 12 Mar 2013 11:43:54 -0400 Received: by mail-wi0-f171.google.com with SMTP id hn17so1956531wib.4 for ; Tue, 12 Mar 2013 08:43:51 -0700 (PDT) Date: Tue, 12 Mar 2013 16:43:48 +0100 From: Stefan Hajnoczi Message-ID: <20130312154348.GC21551@stefanha-thinkpad.redhat.com> References: <1357543689-11415-9-git-send-email-xiawenc@linux.vnet.ibm.com> <20130109124433.GA391@stefanha-thinkpad.redhat.com> <50EE33B2.4040504@linux.vnet.ibm.com> <20130110124109.GD30946@stefanha-thinkpad.redhat.com> <50EFAFA4.1030705@linux.vnet.ibm.com> <20130111091253.GA31400@stefanha-thinkpad.muc.redhat.com> <50F373DE.4060709@linux.vnet.ibm.com> <20130114100604.GH11260@stefanha-thinkpad.redhat.com> <50F4FF3C.9000706@linux.vnet.ibm.com> <513EE7B1.6060501@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <513EE7B1.6060501@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia Cc: kwolf@redhat.com, aliguori@us.ibm.com, quintela@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.com, dietmar@proxmox.com On Tue, Mar 12, 2013 at 04:30:41PM +0800, Wenchao Xia wrote: > 于 2013-1-15 15:03, Wenchao Xia 写道: > >于 2013-1-14 18:06, Stefan Hajnoczi 写道: > >>On Mon, Jan 14, 2013 at 10:56:30AM +0800, Wenchao Xia wrote: > >>>于 2013-1-11 17:12, Stefan Hajnoczi 写道: > >>>>On Fri, Jan 11, 2013 at 02:22:28PM +0800, Wenchao Xia wrote: > >>>>>于 2013-1-10 20:41, Stefan Hajnoczi 写道: > >>>>>>On Thu, Jan 10, 2013 at 11:21:22AM +0800, Wenchao Xia wrote: > >>>>>>>于 2013-1-9 20:44, Stefan Hajnoczi 写道: > >>>>>>>>On Mon, Jan 07, 2013 at 03:28:06PM +0800, Wenchao Xia wrote: > >>>>>>>>> This patch switch to internal common API to take group external > >>>>>>>>>snapshots from qmp_transaction interface. qmp layer simply does > >>>>>>>>>a translation from user input. > >>>>>>>>> > >>>>>>>>>Signed-off-by: Wenchao Xia > >>>>>>>>>--- > >>>>>>>>> blockdev.c | 215 > >>>>>>>>>++++++++++++++++++++++++------------------------------------ > >>>>>>>>> 1 files changed, 87 insertions(+), 128 deletions(-) > >>>>>>>> > >>>>>>>>An internal API for snapshots is not necessary. > >>>>>>>>qmp_transaction() is > >>>>>>>>already usable both from the monitor and C code. > >>>>>>>> > >>>>>>>>The QAPI code generator creates structs that can be accessed > >>>>>>>>directly > >>>>>>>>from C. qmp_transaction(), BlockdevAction, and > >>>>>>>BlockdevActionList *is* > >>>>>>>>the snapshot API. It just doesn't support internal snapshots > >>>>>>>>yet, which > >>>>>>>>is what you are trying to add. > >>>>>>>> > >>>>>>>>To add internal snapshot support, define a > >>>>>>>>BlockdevInternalSnapshot type > >>>>>>>>in qapi-schema.json and add internal snapshot support in > >>>>>>>>qmp_transaction(). > >>>>>>>> > >>>>>>>>qmp_transaction() was designed with this in mind from the > >>>>>>>>beginning and > >>>>>>>>dispatches based on BlockdevAction->kind. > >>>>>>>> > >>>>>>>>The patch series will become much smaller while still adding > >>>>>>>>internal > >>>>>>>>snapshot support. > >>>>>>>> > >>>>>>>>Stefan > >>>>>>>> > >>>>>>> > >>>>>>> As API, qmp_transaction have following disadvantages: > >>>>>>>1) interface is based on string not data type inside qemu, that > >>>>>>>means > >>>>>>>other function calling it result in: bdrv->string->bdrv > >>>>>> > >>>>>>Use bdrv_get_device_name(). You already need to fill in filename or > >>>>>>snapshot name strings. This is not a big disadvantage. > >>>>>> > >>>>> Yes, not a big disadvantage, but why not save string operation but > >>>>>use (bdrv*) as much as possible? > >>>>> > >>>>>what happens will be: > >>>>> > >>>>>hmp-snapshot > >>>>> | > >>>>>qmp-snapshot > >>>>> |--------- > >>>>> | > >>>>> qmp-transaction savevm(may be other..) > >>>>> |----------------------| > >>>>> | > >>>>> internal transaction layer > >>>> > >>>>Saving the string operation is not worth duplicating the API. > >>>> > >>> I agree with you for this line:), but, it is a weight on the balance > >>>of choice, pls consider it together with issues below. > >>> > >>>>>>>2) all capability are forced to be exposed. > >>>>>> > >>>>>>Is there something you cannot expose? > >>>>>> > >>>>> As other component in qemu can use it, some option may > >>>>>be used only in qemu not to user. For eg, vm-state-size. > >>>> > >>>>When we hit a limitation of QAPI then it needs to be extended. I'm > >>>>sure > >>>>there's a solution for splitting or hiding parts of the QAPI generated > >>>>API. > >>>> > >>> I can't think it out now, it seems to be a bit tricky. > >>> > >>>>>>>3) need structure to record each transaction state, such as > >>>>>>>BlkTransactionStates. Extending it is equal to add an internal > >>>>>>>layer. > >>>>>> > >>>>>>I agree that extending it is equal coding effort to adding an > >>>>>>internal > >>>>>>layer because you'll need to refactor qmp_transaction() a bit to > >>>>>>really > >>>>>>support additional action types. > >>>>>> > >>>>>>But it's the right thing to do. Don't add unnecessary layers just > >>>>>>because writing new code is more fun than extending existing code. > >>>>>> > >>>>> If this layer is not added but depending only qmp_transaction, there > >>>>>will be many "if else" fragment. I have tried that and the code > >>>>>is awkful, this layer did not bring extra burden only make what > >>>>>happens inside qmp_transaction clearer, I did not add this layer just > >>>>>for fun. > >>>>> > >>>>> > >>>>>>> Actually I started up by use qmp_transaction as API, but soon > >>>>>>>found that work is almost done around BlkTransactionStates, so > >>>>>>>added a layer around it clearly. > >>>> > >>>>The qmp_transaction() implementation can be changed, I'm not saying you > >>>>have to hack in more if statements. It's cleanest to introduce a > >>>>BdrvActionOps abstraction: > >>>> > >>>>typedef struct BdrvActionOps BdrvActionOps; > >>>>typedef struct BdrvTransactionState { > >>>> const BdrvActionOps *ops; > >>>> QLIST_ENTRY(BdrvTransactionState); > >>>>} BdrvTransactionState; > >>>> > >>>>struct BdrvActionOps { > >>>> int (*prepare)(BdrvTransactionState *s, ...); > >>>> int (*commit)(BdrvTransactionState *s, ...); > >>>> int (*rollback)(BdrvTransactionState *s, ...); > >>>>}; > >>>> > >>>>BdrvTransactionState *bdrv_transaction_create(BlockdevAction *action); > >>>> > >>>>Then qmp_transaction() can be generic code that steps through the > >>>>transactions. > >>> With internal API, qmp_transaction can still be generic code with > >>>a translate from bdrv* to char* at caller level. > >>> > >>> This is similar to what your series does and I think it's > >>>>the right direction. > >>>> > >>>>But please don't duplicate the qmp_transaction() and > >>>>BlockdevAction/BlockdevActionList APIs. In other words, change the > >>>>engine, not the whole car. > >>>> > >>>>Stefan > >>>> > >>> > >>> If my understanding is correct, the BdrvActionOps need to be extended > >>>as following: > >>>struct BdrvActionOps { > >>> /* need following for callback functions */ > >>> const char *sn_name; > >>> BlockDriverState *bs; > >>> ... > >>> int (*prepare)(BdrvTransactionState *s, ...); > >>> int (*commit)(BdrvTransactionState *s, ...); > >>> int (*rollback)(BdrvTransactionState *s, ...); > >>>}; > >>>Or an opaque* should used for every BdrvActionOps. > >> > >>It is nice to keep *Ops structs read-only so they can be static const. > >>This way the ops are shared between all instances of the same action > >>type. Also the function pointers can be in read-only memory pages, > >>which is a slight security win since it prevents memory corruption > >>exploits from taking advantage of function pointers to execute arbitrary > >>code. > >> > > Seems good, I will package callback functions into *Ops, thanks. > > > >>In the pseudo-code I posted the sn_name or bs fields go into an > >>action-specific state struct: > >> > >>typedef struct { > >> BdrvTransactionState common; > >> char *backup_sn_name; > >>} InternalSnapshotTransactionState; > >> > >>typedef struct { > >> BdrvTransactionState common; > >> BlockDriverState *old_bs; > >> BlockDriverState *new_bs; > >>} ExternalSnapshotTransactionState; > >> > >>>Comparation: > >>>The way above: > >>> 1) translate from BlockdevAction to BdrvTransactionState by > >>> bdrv_transaction_create(). > >>> 2) enqueue BdrvTransactionState by > >>> some code. > >>> 3) execute them by > >>> a new function, name it as BdrvActionOpsRun(). > >> > >>If you include .prepare() in the transaction creation, then it becomes > >>simpler: > >> > >>states = [] > >>for action in actions: > >> result = bdrv_transaction_create(action) # invokes .prepare() > >> if result is error: > >> for state in states: > >> state.rollback() > >> return > >> states.append(result) > >>for state in states: > >> state.commit() > >> > >>Because we don't wait until BdrvActionOpsRun() before processing the > >>transaction, there's no need to translate from BlockdevAction to > >>BdrvTransactionState. The BdrvTransactionState struct really only has > >>state required to commit/rollback the transaction. > >> > >>(Even if it becomes necessary to keep information from BlockdevAction > >>after .prepare() returns, just keep a pointer to BlockdevAction. Don't > >>duplicate it.) > >> > > OK, *BlockdevAction plus *BlockDriverState and some other > >data used internal will be added in states. > > > >>>Internal API way: > >>> 1) translate BlockdevAction to BlkTransStates by > >>> fill_blk_trs(). > >>> 2) enqueue BlkTransStates to BlkTransStates by > >>> add_transaction(). > >>> 3) execute them by > >>> submit_transaction(). > >>> > >>> It seems the way above will end as something like an internal > >>>layer, but without clear APIs tips what it is doing. Please reconsider > >>>the advantages about a clear internal API layer. > >> > >>I'm not convinced by the internal API approach. It took me a while when > >>reviewing the code before I understood what was actually going on > >>because of the qmp_transaction() and BlockdevAction duplication code. > >> > >>I see the internal API approach as an unnecessary layer of indirection. > >>It makes the code more complicated to understand and maintain. Next > >>time we add something to qmp_transaction() it would also be necessary to > >>duplicate that change for the internal API. It creates unnecessary > >>work. > >> > > Basic process is almost the same in two approaches, I'd like to > >adjust the code to avoid data duplication as much as possible, and > >see if some function can be removed when code keeps clear, in next > >version. > > > >>Just embrace QAPI, the point of it was to eliminate these external <-> > >>internal translations we were doing all the time. > >> > >>Stefan > >> > > > > > Hi, Stefan > I redesigned the structure, Following is the fake code: > > typedef struct BdrvActionOps { > /* check the request's validation, allocate p_opaque if needed */ > int (*check)(BlockdevAction *action, void **p_opaque, Error **errp); > /* take the action */ > int (*submit)(BlockdevAction *action, void *opaque, Error **errp); > /* update emulator */ > int (*commit)(BlockdevAction *action, void *opaque, Error **errp); > /* cancel the action */ > int (*rollback)(BlockdevAction *action, void *opaque, Error **errp); > } BdrvActionOps; > > typedef struct BlkTransactionStates { > BlockdevAction *action; > void *opaque; > BdrvActionOps *ops; > QSIMPLEQ_ENTRY(BlkTransactionStates) entry; > } BlkTransactionStates; > > /* call ops->check and return state* to be enqueued */ > static BlkTransactionStates *transaction_create(BlockdevAction *action, > Error **errp); > > void qmp_transaction(BlockdevActionList *dev_list, Error **errp) > { > BlockdevActionList *dev_entry = dev_list; > BlkTransactionStates *state; > > while (NULL != dev_entry) { > state = transaction_create(dev_entry->value, errp); > /* enqueue */ > dev_entry = dev_entry->next; > } > > /* use queue with submit, commit, rollback callback */ > } > > > In this way, parameter duplication is saved, but one problem remains: > parameter can't be hidden to user such as vm_state_size, but this would > not be a problem if hmp "savevm" use his own code about block snapshot > later, I mean not use qmp_transaction(). What do you think about the > design? Do you have a better way to solve this problem? Can you explain the vm_state_size problem again? Sorry I forgot - I think it had something to do with having an internal parameter in the action that should not be exposed via QMP/HMP? Stefan