Re: [Xen-devel] [PATCH v4 3/4] xenstore: rework of transaction handling

[Juergen Gross <jgross@xxxxxxxx>]

On 31/03/17 13:00, Wei Liu wrote:
> On Thu, Mar 30, 2017 at 06:41:11PM +0200, Juergen Gross wrote:
> [...]
>>>
>>>>            node->generation = generation++;
>>>> -          return;
>>>> +          if (conn && !conn->transaction)
>>>> +                  wrl_apply_debit_direct(conn);
>>>> +  }
>>>> +
>>>> +  if (!conn || !conn->transaction) {
>>>> +          if (key)
>>>> +                  set_tdb_key(node->name, key);
>>>> +          return 0;
>>>>    }
>>>>  
>>>>    trans = conn->transaction;
>>>>  
>>>> -  node->generation = generation + trans->trans_gen++;
>>>> +  trans_name = transaction_get_node_name(node, trans, node->name);
>>>> +  if (!trans_name)
>>>> +          goto nomem;
>>>>  
>>>> -  list_for_each_entry(i, &trans->changes, list) {
>>>> -          if (streq(i->node, node->name)) {
>>>> -                  if (recurse)
>>>> -                          i->recurse = recurse;
>>>> -                  return;
>>>> -          }
>>>> -  }
>>>> -
>>>> -  i = talloc(trans, struct changed_node);
>>>> +  i = find_accessed_node(trans, node->name);
>>>>    if (!i) {
>>>> -          /* All we can do is let the transaction fail. */
>>>> -          generation++;
>>>> -          return;
>>>> +          i = talloc_zero(trans, struct accessed_node);
>>>> +          if (!i)
>>>> +                  goto nomem;
>>>> +          i->node = talloc_strdup(i, node->name);
>>>> +          if (!i->node)
>>>> +                  goto nomem;
>>>> +
>>>> +          introduce = true;
>>>> +          i->ta_node = false;
>>>> +
>>>> +          /* We have to verify read nodes only if we didn't write them. */
>>>> +          if (type == NODE_ACCESS_READ) {
>>>> +                  i->generation = node->generation;
>>>> +                  i->check_gen = true;
>>>> +                  if (node->generation != NO_GENERATION) {
>>>> +                          set_tdb_key(trans_name, &local_key);
>>>> +                          ret = write_node_raw(conn, &local_key, node);
>>>> +                          if (ret)
>>>> +                                  goto err;
>>>> +                          i->ta_node = true;
>>>> +                  }
>>>> +          }
>>>
>>> I think the current code structure is a bit confusing.
>>>
>>> For write types, the call to write_node_raw is outside of this function
>>> but for read type it is inside this function.
>>
>> This is due to the different purpose of write_node_raw() in both
>> cases:
>>
>> In the read case we write an _additional_ node into the transaction,
>> while in the write case it is just the user's operation.
>>
>>>
>>>> +          list_add_tail(&i->list, &trans->accessed);
>>>>    }
>>>> -  i->node = talloc_strdup(i, node->name);
>>>> -  if (!i->node) {
>>>> -          /* All we can do is let the transaction fail. */
>>>> -          generation++;
>>>> +
>>>> +  if (type != NODE_ACCESS_READ)
>>>> +          i->modified = true;
>>>> +
>>>> +  if (introduce && type == NODE_ACCESS_DELETE)
>>>> +          /* Nothing to delete. */
>>>> +          return -1;
>>>> +
>>>> +  if (key) {
>>>> +          set_tdb_key(trans_name, key);
>>>> +          if (type == NODE_ACCESS_WRITE)
>>>> +                  i->ta_node = true;
>>>> +          if (type == NODE_ACCESS_DELETE)
>>>> +                  i->ta_node = false;
>>>
>>> The same caveat made me wonder if ta_node was really what it meant to
>>> be because I couldn't find where write_node_raw was.
>>>
>>> Can I suggest you make them consistent? Either lift the write_node_raw
>>> for read to read_node or push down the write_node_raw in delete_node and
>>> write_node here?
>>>
>>> If I misunderstand how this works, please tell me...
>>
>> As I already said: in the read case writing the node is depending
>> on the context and lifting it up would seem rather strange: why
>> should reading a node contain a write_node_raw() call?
>>
>> In the write case I believe we should keep write_node_raw() where
>> it belongs: in the appropriate function modifying the node.
>>
>> All actions in access_node() are transaction specific and _additional_
>> in the transaction case.
> 
> 
> OK, my line of reasoning is that whatever DB operation is necessary to
> make a functionality work should be consistent. That means even having a
> DB write in read is OK.  But I think your explanation is fine, too.
> 
> May I then suggest adding the following to the write for read type in
> access_node?
> 
>   /* Additional transaction-specific node for read type. We only have to 
>    * verify read nodes only if we didn't write them.
>    *
>    * The node is created and written to DB here to distinguish from the
>    * write types.
>    */

Will do.

Thanks,


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel