From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51964) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tv0ae-0001oJ-Pb for qemu-devel@nongnu.org; Tue, 15 Jan 2013 02:05:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tv0aa-0002Ak-85 for qemu-devel@nongnu.org; Tue, 15 Jan 2013 02:05:44 -0500 Received: from e28smtp03.in.ibm.com ([122.248.162.3]:50717) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tv0aZ-0002A1-9F for qemu-devel@nongnu.org; Tue, 15 Jan 2013 02:05:40 -0500 Received: from /spool/local by e28smtp03.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 15 Jan 2013 12:34:15 +0530 Received: from d28relay03.in.ibm.com (d28relay03.in.ibm.com [9.184.220.60]) by d28dlp01.in.ibm.com (Postfix) with ESMTP id 4393DE004D for ; Tue, 15 Jan 2013 12:35:48 +0530 (IST) Received: from d28av05.in.ibm.com (d28av05.in.ibm.com [9.184.220.67]) by d28relay03.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r0F75Ol842991832 for ; Tue, 15 Jan 2013 12:35:25 +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 r0F75POg023888 for ; Tue, 15 Jan 2013 18:05:27 +1100 Message-ID: <50F4FF3C.9000706@linux.vnet.ibm.com> Date: Tue, 15 Jan 2013 15:03:24 +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> In-Reply-To: <20130114100604.GH11260@stefanha-thinkpad.redhat.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-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 > -- Best Regards Wenchao Xia