Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 2013-3-12 23:43, Stefan Hajnoczi 写道:
> 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 <xiawenc@linux.vnet.ibm.com>
>>>>>>>>>>> ---
>>>>>>>>>>>   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?
> 
  Yep, this parameter will be used by qemu "live savevm" code later,
but should not expose it to user in qmp interface.

> Stefan
> 


-- 
Best Regards

Wenchao Xia