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

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 4336 bytes --]

On 10.05.23 23:31, Julien Grall wrote:
> 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@suse.com>
>>>> ---
>>>> 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?

We couldn't allocate a small amount of memory (around 64 bytes)! 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.

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().

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


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]