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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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.
   */

Wei.

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