Re: [PATCH v5 05/14] tools/xenstore: use accounting buffering for node accounting

[Juergen Gross <jgross@xxxxxxxx>]

On 11.05.23 14:07, Julien Grall wrote:
> Hi Juergen,
>
> On 11/05/2023 06:25, Juergen Gross wrote:
> > On 10.05.23 23:31, Julien Grall wrote:
> > > On 10/05/2023 13:54, Juergen Gross wrote:
> > > > On 09.05.23 20:46, Julien Grall wrote:
> > > > > Hi Juergen,
> > > > >
> > > > > On 08/05/2023 12:47, Juergen Gross wrote:
> > > > > > Add the node accounting to the accounting information buffering in
> > > > > > order to avoid having to undo it in case of failure.
> > > > > >
> > > > > > This requires to call domain_nbentry_dec() before any changes to the
> > > > > > data base, as it can return an error now.
> > > > > >
> > > > > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > > > > ---
> > > > > > V5:
> > > > > > - add error handling after domain_nbentry_dec() calls (Julien Grall)
> > > > > > ---
> > > > > >   tools/xenstore/xenstored_core.c   | 29 +++++++----------------------
> > > > > >   tools/xenstore/xenstored_domain.h |  4 ++--
> > > > > >   2 files changed, 9 insertions(+), 24 deletions(-)
> > > > > >
> > > > > > diff --git a/tools/xenstore/xenstored_core.c 
> > > > > > b/tools/xenstore/xenstored_core.c
> > > > > > index 8392bdec9b..22da434e2a 100644
> > > > > > --- a/tools/xenstore/xenstored_core.c
> > > > > > +++ b/tools/xenstore/xenstored_core.c
> > > > > > @@ -1454,7 +1454,6 @@ static void destroy_node_rm(struct connection *conn, 
> > > > > > struct node *node)
> > > > > >   static int destroy_node(struct connection *conn, struct node *node)
> > > > > >   {
> > > > > >       destroy_node_rm(conn, node);
> > > > > > -    domain_nbentry_dec(conn, get_node_owner(node));
> > > > > >       /*
> > > > > >        * It is not possible to easily revert the changes in a transaction.
> > > > > > @@ -1645,6 +1644,9 @@ static int delnode_sub(const void *ctx, struct 
> > > > > > connection *conn,
> > > > > >       if (ret > 0)
> > > > > >           return WALK_TREE_SUCCESS_STOP;
> > > > > > +    if (domain_nbentry_dec(conn, get_node_owner(node)))
> > > > > > +        return WALK_TREE_ERROR_STOP;
> > > > > I think there is a potential issue with the buffering here. In case of 
> > > > > failure, the node could have been removed, but the quota would not be 
> > > > > properly accounted.
> > > > You mean the case where another node has been deleted and due to accounting
> > > > buffering the corrected accounting data wouldn't be committed?
> > > >
> > > > This is no problem, as an error returned by delnode_sub() will result in
> > > > corrupt() being called, which in turn will correct the accounting data.
> > > To me corrupt() is a big hammer and it feels wrong to call it when I think we 
> > > have easier/faster way to deal with the issue. Could we instead call 
> > > acc_commit() before returning?
> > You are aware that this is a very problematic situation we are in?
> It is not very clear from the code. And that's why comments are always useful to 
> clarify why corrupt() is the right call.
I agree. I'll add a comment.

> > We couldn't allocate a small amount of memory (around 64 bytes)! 
> So long this is the only reason then...
>
> Xenstored
> > will probably die within milliseconds. Using the big hammer in such a
> > situation is fine IMO. It will maybe result in solving the problem by
> > freeing of memory (quite unlikely, though), but it won't leave xenstored
> > in a worse state than with your suggestion.
> ... this might be OK. But in the past, we had a place where corrupt() could be 
> reliably triggered by a guest. If you think that's not possible, then it should 
> be properly documented.
Okay, will do so.

> > And calling acc_commit() here wouldn't really help, as accounting data
> > couldn't be recorded, so there are missing updates anyway due to the failed
> > call of domain_nbentry_dec().
> We are removing the node after the accounting is updated. So if the accounting 
> fail, then it should still be correct for anything that was removed before.
Oh, right.

> > > > > Also, I think a comment would be warrant to explain why we are returning 
> > > > > WALK_TREE_ERROR_STOP here when...
> > > > > > +
> > > > > >       /* In case of error stop the walk. */
> > > > > >       if (!ret && do_tdb_delete(conn, &key, &node->acc))
> > > > > >           return WALK_TREE_SUCCESS_STOP;
> > > > > ... this is not the case when do_tdb_delete() fails for some reasons.
> > > > The main idea was that the remove is working from the leafs towards the root.
> > > > In case one entry can't be removed, we should just stop.
> > > >
> > > > OTOH returning WALK_TREE_ERROR_STOP might be cleaner, as this would make sure
> > > > that accounting data is repaired afterwards. Even if do_tdb_delete() can't
> > > > really fail in normal configurations, as the only failure reasons are:
> > > >
> > > > - the node isn't found (quite unlikely, as we just read it before entering
> > > >    delnode_sub()), or
> > > > - writing the updated data base failed (in normal configurations it is in
> > > >    already allocated memory, so no way to fail that)
> > > >
> > > > I think I'll switch to return WALK_TREE_ERROR_STOP here.
> > > See above for a different proposal.
> > Without deleting the node in the data base this would be another accounting
> > data inconsistency, so calling corrupt() is the correct cleanup measure.
> Hmmm... I read this as this is already a bug rather than one introduced by this 
> patch. IIUC, then this should be done in a new commit.
No, previously domain_nbentry_dec() couldn't fail, so this is a new situation.
Or did I misunderstand what you mean?


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature