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

[Julien Grall <julien@xxxxxxx>]

Hi,

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

Cheers,

--
Julien Grall