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-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?


-- 
Best Regards

Wenchao Xia