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 <jgr...@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
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
