From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:59054) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UFKlK-000669-80 for qemu-devel@nongnu.org; Tue, 12 Mar 2013 04:40:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UFKlH-0004wE-DP for qemu-devel@nongnu.org; Tue, 12 Mar 2013 04:40:46 -0400 Received: from e28smtp05.in.ibm.com ([122.248.162.5]:49756) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UFKcz-0001Qp-2M for qemu-devel@nongnu.org; Tue, 12 Mar 2013 04:32:10 -0400 Received: from /spool/local by e28smtp05.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 12 Mar 2013 13:59:33 +0530 Received: from d28relay04.in.ibm.com (d28relay04.in.ibm.com [9.184.220.61]) by d28dlp03.in.ibm.com (Postfix) with ESMTP id 587F7125804F for ; Tue, 12 Mar 2013 14:03:02 +0530 (IST) Received: from d28av05.in.ibm.com (d28av05.in.ibm.com [9.184.220.67]) by d28relay04.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r2C8VtpI29032702 for ; Tue, 12 Mar 2013 14:01:55 +0530 Received: from d28av05.in.ibm.com (loopback [127.0.0.1]) by d28av05.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r2C8Vu7K013362 for ; Tue, 12 Mar 2013 19:31:56 +1100 Message-ID: <513EE7B1.6060501@linux.vnet.ibm.com> Date: Tue, 12 Mar 2013 16:30:41 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1357543689-11415-1-git-send-email-xiawenc@linux.vnet.ibm.com> <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> In-Reply-To: <50F4FF3C.9000706@linux.vnet.ibm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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: Stefan Hajnoczi Cc: kwolf@redhat.com, aliguori@us.ibm.com, quintela@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.com, dietmar@proxmox.com 于 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? -- Best Regards Wenchao Xia