[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[xen master] tools/xenstore: change per-domain node accounting interface



commit 4c1f92d664c49442af8c743bd06360bda3412b3c
Author:     Juergen Gross <jgross@xxxxxxxx>
AuthorDate: Wed Jan 18 10:50:07 2023 +0100
Commit:     Julien Grall <jgrall@xxxxxxxxxx>
CommitDate: Fri Jan 20 09:23:51 2023 +0000

    tools/xenstore: change per-domain node accounting interface
    
    Rework the interface and the internals of the per-domain node
    accounting:
    
    - rename the functions to domain_nbentry_*() in order to better match
      the related counter name
    
    - switch from node pointer to domid as interface, as all nodes have the
      owner filled in
    
    - use a common internal function for adding a value to the counter
    
    For the transaction case add a helper function to get the list head
    of the per-transaction changed domains, enabling to eliminate the
    transaction_entry_*() functions.
    
    Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
    Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>
---
 tools/xenstore/xenstored_core.c        |  28 ++++----
 tools/xenstore/xenstored_core.h        |   6 ++
 tools/xenstore/xenstored_domain.c      | 126 +++++++++++++++------------------
 tools/xenstore/xenstored_domain.h      |  10 ++-
 tools/xenstore/xenstored_transaction.c |  15 +---
 tools/xenstore/xenstored_transaction.h |   7 +-
 6 files changed, 87 insertions(+), 105 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index c82fb6e3d5..4582ee39e1 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -738,13 +738,13 @@ struct node *read_node(struct connection *conn, const 
void *ctx,
 
        /* Permissions are struct xs_permissions. */
        node->perms.p = hdr->perms;
-       node->acc.domid = node->perms.p[0].id;
+       node->acc.domid = get_node_owner(node);
        node->acc.memory = data.dsize;
        if (domain_adjust_node_perms(node))
                goto error;
 
        /* If owner is gone reset currently accounted memory size. */
-       if (node->acc.domid != node->perms.p[0].id)
+       if (node->acc.domid != get_node_owner(node))
                node->acc.memory = 0;
 
        /* Data is binary blob (usually ascii, no nul). */
@@ -1445,7 +1445,7 @@ 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_entry_dec(conn, node);
+       domain_nbentry_dec(conn, get_node_owner(node));
 
        /*
         * It is not possible to easily revert the changes in a transaction.
@@ -1484,7 +1484,7 @@ static struct node *create_node(struct connection *conn, 
const void *ctx,
        for (i = node; i; i = i->parent) {
                /* i->parent is set for each new node, so check quota. */
                if (i->parent &&
-                   domain_entry(conn) >= quota_nb_entry_per_domain) {
+                   domain_nbentry(conn) >= quota_nb_entry_per_domain) {
                        ret = ENOSPC;
                        goto err;
                }
@@ -1495,7 +1495,7 @@ static struct node *create_node(struct connection *conn, 
const void *ctx,
 
                /* Account for new node */
                if (i->parent) {
-                       if (domain_entry_inc(conn, i)) {
+                       if (domain_nbentry_inc(conn, get_node_owner(i))) {
                                destroy_node_rm(conn, i);
                                return NULL;
                        }
@@ -1648,7 +1648,7 @@ static int delnode_sub(const void *ctx, struct connection 
*conn,
        watch_exact = strcmp(root, node->name);
        fire_watches(conn, ctx, node->name, node, watch_exact, NULL);
 
-       domain_entry_dec(conn, node);
+       domain_nbentry_dec(conn, get_node_owner(node));
 
        return WALK_TREE_RM_CHILDENTRY;
 }
@@ -1784,29 +1784,29 @@ static int do_set_perms(const void *ctx, struct 
connection *conn,
 
        /* Unprivileged domains may not change the owner. */
        if (domain_is_unprivileged(conn) &&
-           perms.p[0].id != node->perms.p[0].id)
+           perms.p[0].id != get_node_owner(node))
                return EPERM;
 
        old_perms = node->perms;
-       domain_entry_dec(conn, node);
+       domain_nbentry_dec(conn, get_node_owner(node));
        node->perms = perms;
-       if (domain_entry_inc(conn, node)) {
+       if (domain_nbentry_inc(conn, get_node_owner(node))) {
                node->perms = old_perms;
                /*
                 * This should never fail because we had a reference on the
                 * domain before and Xenstored is single-threaded.
                 */
-               domain_entry_inc(conn, node);
+               domain_nbentry_inc(conn, get_node_owner(node));
                return ENOMEM;
        }
 
        if (write_node(conn, node, false)) {
                int saved_errno = errno;
 
-               domain_entry_dec(conn, node);
+               domain_nbentry_dec(conn, get_node_owner(node));
                node->perms = old_perms;
                /* No failure possible as above. */
-               domain_entry_inc(conn, node);
+               domain_nbentry_inc(conn, get_node_owner(node));
 
                errno = saved_errno;
                return errno;
@@ -2378,7 +2378,7 @@ void setup_structure(bool live_update)
                manual_node("/tool/xenstored", NULL);
                manual_node("@releaseDomain", NULL);
                manual_node("@introduceDomain", NULL);
-               domain_entry_fix(dom0_domid, 5, true);
+               domain_nbentry_fix(dom0_domid, 5, true);
        }
 
        check_store();
@@ -3386,7 +3386,7 @@ void read_state_node(const void *ctx, const void *state)
        if (write_node_raw(NULL, &key, node, true))
                barf("write node error restoring node");
 
-       if (domain_entry_inc(&conn, node))
+       if (domain_nbentry_inc(&conn, get_node_owner(node)))
                barf("node accounting error restoring node");
 
        talloc_free(node);
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 89055cbb21..62d8ee96bd 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -232,6 +232,12 @@ char *canonicalize(struct connection *conn, const void 
*ctx, const char *node);
 unsigned int perm_for_conn(struct connection *conn,
                           const struct node_perms *perms);
 
+/* Get owner of a node. */
+static inline unsigned int get_node_owner(const struct node *node)
+{
+       return node->perms.p[0].id;
+}
+
 /* Write a node to the tdb data base. */
 int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node,
                   bool no_quota_check);
diff --git a/tools/xenstore/xenstored_domain.c 
b/tools/xenstore/xenstored_domain.c
index 1d765ceffa..703ddeec4e 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -249,7 +249,7 @@ static int domain_tree_remove_sub(const void *ctx, struct 
connection *conn,
                domain->nbentry--;
                node->perms.p[0].id = priv_domid;
                node->acc.memory = 0;
-               domain_entry_inc(NULL, node);
+               domain_nbentry_inc(NULL, priv_domid);
                if (write_node_raw(NULL, &key, node, true)) {
                        /* That's unfortunate. We only can try to continue. */
                        syslog(LOG_ERR,
@@ -561,7 +561,7 @@ int acc_fix_domains(struct list_head *head, bool update)
        int cnt;
 
        list_for_each_entry(cd, head, list) {
-               cnt = domain_entry_fix(cd->domid, cd->nbentry, update);
+               cnt = domain_nbentry_fix(cd->domid, cd->nbentry, update);
                if (!update) {
                        if (cnt >= quota_nb_entry_per_domain)
                                return ENOSPC;
@@ -606,8 +606,8 @@ static struct changed_domain *acc_get_changed_domain(const 
void *ctx,
        return cd;
 }
 
-int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val,
-                       unsigned int domid)
+static int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int 
val,
+                              unsigned int domid)
 {
        struct changed_domain *cd;
 
@@ -991,30 +991,6 @@ void domain_deinit(void)
                xenevtchn_unbind(xce_handle, virq_port);
 }
 
-int domain_entry_inc(struct connection *conn, struct node *node)
-{
-       struct domain *d;
-       unsigned int domid;
-
-       if (!node->perms.p)
-               return 0;
-
-       domid = node->perms.p[0].id;
-
-       if (conn && conn->transaction) {
-               transaction_entry_inc(conn->transaction, domid);
-       } else {
-               d = (conn && domid == conn->id && conn->domain) ? conn->domain
-                   : find_or_alloc_existing_domain(domid);
-               if (d)
-                       d->nbentry++;
-               else
-                       return ENOMEM;
-       }
-
-       return 0;
-}
-
 /*
  * Check whether a domain was created before or after a specific generation
  * count (used for testing whether a node permission is older than a domain).
@@ -1082,62 +1058,76 @@ int domain_adjust_node_perms(struct node *node)
        return 0;
 }
 
-void domain_entry_dec(struct connection *conn, struct node *node)
+static int domain_nbentry_add(struct connection *conn, unsigned int domid,
+                             int add, bool no_dom_alloc)
 {
        struct domain *d;
-       unsigned int domid;
-
-       if (!node->perms.p)
-               return;
+       struct list_head *head;
+       int ret;
 
-       domid = node->perms.p ? node->perms.p[0].id : conn->id;
+       if (conn && domid == conn->id && conn->domain)
+               d = conn->domain;
+       else if (no_dom_alloc) {
+               d = find_domain_struct(domid);
+               if (!d) {
+                       errno = ENOENT;
+                       corrupt(conn, "Missing domain %u\n", domid);
+                       return -1;
+               }
+       } else {
+               d = find_or_alloc_existing_domain(domid);
+               if (!d) {
+                       errno = ENOMEM;
+                       return -1;
+               }
+       }
 
        if (conn && conn->transaction) {
-               transaction_entry_dec(conn->transaction, domid);
-       } else {
-               d = (conn && domid == conn->id && conn->domain) ? conn->domain
-                   : find_domain_struct(domid);
-               if (d) {
-                       d->nbentry--;
-               } else {
-                       errno = ENOENT;
-                       corrupt(conn,
-                               "Node \"%s\" owned by non-existing domain %u\n",
-                               node->name, domid);
+               head = transaction_get_changed_domains(conn->transaction);
+               ret = acc_add_dom_nbentry(conn->transaction, head, add, domid);
+               if (errno) {
+                       fail_transaction(conn->transaction);
+                       return -1;
                }
+               /*
+                * In a transaction when a node is being added/removed AND the
+                * same node has been added/removed outside the transaction in
+                * parallel, the resulting number of nodes will be wrong. This
+                * is no problem, as the transaction will fail due to the
+                * resulting conflict.
+                * In the node remove case the resulting number can be even
+                * negative, which should be avoided.
+                */
+               return max(d->nbentry + ret, 0);
        }
+
+       d->nbentry += add;
+
+       return d->nbentry;
 }
 
-int domain_entry_fix(unsigned int domid, int num, bool update)
+int domain_nbentry_inc(struct connection *conn, unsigned int domid)
 {
-       struct domain *d;
-       int cnt;
+       return (domain_nbentry_add(conn, domid, 1, false) < 0) ? errno : 0;
+}
 
-       if (update) {
-               d = find_domain_struct(domid);
-               assert(d);
-       } else {
-               /*
-                * We are called first with update == false in order to catch
-                * any error. So do a possible allocation and check for error
-                * only in this case, as in the case of update == true nothing
-                * can go wrong anymore as the allocation already happened.
-                */
-               d = find_or_alloc_existing_domain(domid);
-               if (!d)
-                       return -1;
-       }
+int domain_nbentry_dec(struct connection *conn, unsigned int domid)
+{
+       return (domain_nbentry_add(conn, domid, -1, true) < 0) ? errno : 0;
+}
 
-       cnt = d->nbentry + num;
-       assert(cnt >= 0);
+int domain_nbentry_fix(unsigned int domid, int num, bool update)
+{
+       int ret;
 
-       if (update)
-               d->nbentry = cnt;
+       ret = domain_nbentry_add(NULL, domid, update ? num : 0, update);
+       if (ret < 0 || update)
+               return ret;
 
-       return domid_is_unprivileged(domid) ? cnt : 0;
+       return domid_is_unprivileged(domid) ? ret + num : 0;
 }
 
-int domain_entry(struct connection *conn)
+int domain_nbentry(struct connection *conn)
 {
        return (domain_is_unprivileged(conn))
                ? conn->domain->nbentry
diff --git a/tools/xenstore/xenstored_domain.h 
b/tools/xenstore/xenstored_domain.h
index 9e20d2b17d..1e402f2609 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -66,10 +66,10 @@ int domain_adjust_node_perms(struct node *node);
 int domain_alloc_permrefs(struct node_perms *perms);
 
 /* Quota manipulation */
-int domain_entry_inc(struct connection *conn, struct node *);
-void domain_entry_dec(struct connection *conn, struct node *);
-int domain_entry_fix(unsigned int domid, int num, bool update);
-int domain_entry(struct connection *conn);
+int domain_nbentry_inc(struct connection *conn, unsigned int domid);
+int domain_nbentry_dec(struct connection *conn, unsigned int domid);
+int domain_nbentry_fix(unsigned int domid, int num, bool update);
+int domain_nbentry(struct connection *conn);
 int domain_memory_add(unsigned int domid, int mem, bool no_quota_check);
 
 /*
@@ -99,8 +99,6 @@ void domain_outstanding_domid_dec(unsigned int domid);
 int domain_get_quota(const void *ctx, struct connection *conn,
                     unsigned int domid);
 int acc_fix_domains(struct list_head *head, bool update);
-int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val,
-                       unsigned int domid);
 
 /* Write rate limiting */
 
diff --git a/tools/xenstore/xenstored_transaction.c 
b/tools/xenstore/xenstored_transaction.c
index c009c67989..82e5e66c18 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -548,20 +548,9 @@ int do_transaction_end(const void *ctx, struct connection 
*conn,
        return 0;
 }
 
-void transaction_entry_inc(struct transaction *trans, unsigned int domid)
+struct list_head *transaction_get_changed_domains(struct transaction *trans)
 {
-       if (!acc_add_dom_nbentry(trans, &trans->changed_domains, 1, domid)) {
-               /* Let the transaction fail. */
-               trans->fail = true;
-       }
-}
-
-void transaction_entry_dec(struct transaction *trans, unsigned int domid)
-{
-       if (!acc_add_dom_nbentry(trans, &trans->changed_domains, -1, domid)) {
-               /* Let the transaction fail. */
-               trans->fail = true;
-       }
+       return &trans->changed_domains;
 }
 
 void fail_transaction(struct transaction *trans)
diff --git a/tools/xenstore/xenstored_transaction.h 
b/tools/xenstore/xenstored_transaction.h
index 3417303f94..b6f8cb7d0a 100644
--- a/tools/xenstore/xenstored_transaction.h
+++ b/tools/xenstore/xenstored_transaction.h
@@ -36,10 +36,6 @@ int do_transaction_end(const void *ctx, struct connection 
*conn,
 
 struct transaction *transaction_lookup(struct connection *conn, uint32_t id);
 
-/* inc/dec entry number local to trans while changing a node */
-void transaction_entry_inc(struct transaction *trans, unsigned int domid);
-void transaction_entry_dec(struct transaction *trans, unsigned int domid);
-
 /* This node was accessed. */
 int __must_check access_node(struct connection *conn, struct node *node,
                              enum node_access_type type, TDB_DATA *key);
@@ -54,6 +50,9 @@ void transaction_prepend(struct connection *conn, const char 
*name,
 /* Mark the transaction as failed. This will prevent it to be committed. */
 void fail_transaction(struct transaction *trans);
 
+/* Get the list head of the changed domains. */
+struct list_head *transaction_get_changed_domains(struct transaction *trans);
+
 void conn_delete_all_transactions(struct connection *conn);
 int check_transactions(struct hashtable *hash);
 
--
generated by git-patchbot for /home/xen/git/xen.git#master



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.