From nobody Sun Jul 13 21:22:08 2025 Delivered-To: importer2@patchew.org Authentication-Results: mx.zohomail.com; dkim=fail; spf=pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer2=patchew.org@nongnu.org Return-Path: Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mx.zohomail.com with SMTPS id 1678213782694668.249851426476; Tue, 7 Mar 2023 10:29:42 -0800 (PST) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pZc29-0006jJ-Aa; Tue, 07 Mar 2023 13:27:33 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pZc1x-0006e7-I5 for qemu-devel@nongnu.org; Tue, 07 Mar 2023 13:27:21 -0500 Received: from desiato.infradead.org ([2001:8b0:10b:1:d65d:64ff:fe57:4e05]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pZc1s-0005xQ-NG for qemu-devel@nongnu.org; Tue, 07 Mar 2023 13:27:21 -0500 Received: from i7.infradead.org ([2001:8b0:10b:1:21e:67ff:fecb:7a92]) by desiato.infradead.org with esmtpsa (Exim 4.96 #2 (Red Hat Linux)) id 1pZc1n-00H8T3-0Y; Tue, 07 Mar 2023 18:27:11 +0000 Received: from dwoodhou by i7.infradead.org with local (Exim 4.96 #2 (Red Hat Linux)) id 1pZc1n-009e8R-0A; Tue, 07 Mar 2023 18:27:11 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=desiato.20200630; h=Sender:Content-Transfer-Encoding: MIME-Version:References:In-Reply-To:Message-Id:Date:Subject:Cc:To:From: Reply-To:Content-Type:Content-ID:Content-Description; bh=BkYMrTPfiwe8ful5EeikJA8cZYk3rSUcxns06e108Rc=; b=dY2QlCm7IXh6WsBhjK0nKl8xzd f2yogSGW7lsj22Vgrp2OBPQczHQYgkiNjy/zqevo7dbEl4RhXI7T6Xo251XW8aq623BwgQQFyUOq3 1olI71ioh9h+/cI6xOYajuMwugxOX31vjcHWkjGTkHar12xiSa4+iPTh7jQR2ezRkk58jJETJRC5O 2UzwbdKHTc50oL0gnzqfDGOZHTlE/4yfIG8M60SK/ShP7fpBSiFs7m6ov/PKTi4DJHcdE63yXFw6v zgfVIUaHUZvYD5j+PboWamizZrq2kfhDEQ0ZlQLhYZ4zomM7mRtIaZmN6GLRGfUDmEdl13FT1LKDn h5UzpXyg==; From: David Woodhouse To: Peter Maydell Cc: qemu-devel@nongnu.org, Paolo Bonzini , Paul Durrant , Joao Martins , Ankur Arora , Stefano Stabellini , vikram.garhwal@amd.com, Anthony Perard , xen-devel@lists.xenproject.org, Juan Quintela , "Dr . David Alan Gilbert" Subject: [PULL 05/27] hw/xen: Watches on XenStore transactions Date: Tue, 7 Mar 2023 18:26:45 +0000 Message-Id: <20230307182707.2298618-6-dwmw2@infradead.org> X-Mailer: git-send-email 2.39.0 In-Reply-To: <20230307182707.2298618-1-dwmw2@infradead.org> References: <20230307182707.2298618-1-dwmw2@infradead.org> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-SRS-Rewrite: SMTP reverse-path rewritten from by desiato.infradead.org. See http://www.infradead.org/rpr.html Received-SPF: pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) client-ip=209.51.188.17; envelope-from=qemu-devel-bounces+importer2=patchew.org@nongnu.org; helo=lists.gnu.org; Received-SPF: none client-ip=2001:8b0:10b:1:d65d:64ff:fe57:4e05; envelope-from=BATV+98a25f4d4d04c9e21499+7135+infradead.org+dwmw2@desiato.srs.infradead.org; helo=desiato.infradead.org X-Spam_score_int: -43 X-Spam_score: -4.4 X-Spam_bar: ---- X-Spam_report: (-4.4 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_MED=-2.3, SPF_HELO_NONE=0.001, SPF_NONE=0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+importer2=patchew.org@nongnu.org Sender: qemu-devel-bounces+importer2=patchew.org@nongnu.org X-ZohoMail-DKIM: fail (Header signature does not verify) X-ZM-MESSAGEID: 1678213784959100005 Content-Type: text/plain; charset="utf-8" From: David Woodhouse Firing watches on the nodes that still exist is relatively easy; just walk the tree and look at the nodes with refcount of one. Firing watches on *deleted* nodes is more fun. We add 'modified_in_tx' and 'deleted_in_tx' flags to each node. Nodes with those flags cannot be shared, as they will always be unique to the transaction in which they were created. When xs_node_walk would need to *create* a node as scaffolding and it encounters a deleted_in_tx node, it can resurrect it simply by clearing its deleted_in_tx flag. If that node originally had any *data*, they're gone, and the modified_in_tx flag will have been set when it was first deleted. We then attempt to send appropriate watches when the transaction is committed, properly delete the deleted_in_tx nodes, and remove the modified_in_tx flag from the others. Signed-off-by: David Woodhouse Reviewed-by: Paul Durrant --- hw/i386/kvm/xenstore_impl.c | 151 ++++++++++++++++++++++- tests/unit/test-xs-node.c | 231 +++++++++++++++++++++++++++++++++++- 2 files changed, 380 insertions(+), 2 deletions(-) diff --git a/hw/i386/kvm/xenstore_impl.c b/hw/i386/kvm/xenstore_impl.c index 0812e367b0..60f42f61d6 100644 --- a/hw/i386/kvm/xenstore_impl.c +++ b/hw/i386/kvm/xenstore_impl.c @@ -32,6 +32,8 @@ typedef struct XsNode { GByteArray *content; GHashTable *children; uint64_t gencnt; + bool deleted_in_tx; + bool modified_in_tx; #ifdef XS_NODE_UNIT_TEST gchar *name; /* debug only */ #endif @@ -153,6 +155,13 @@ static XsNode *xs_node_copy(XsNode *old) XsNode *n =3D xs_node_new(); =20 n->gencnt =3D old->gencnt; + +#ifdef XS_NODE_UNIT_TEST + if (n->name) { + n->name =3D g_strdup(old->name); + } +#endif + if (old->children) { n->children =3D g_hash_table_new_full(g_str_hash, g_str_equal, g_f= ree, (GDestroyNotify)xs_node_unref); @@ -221,6 +230,9 @@ struct walk_op { bool mutating; bool create_dirs; bool in_transaction; + + /* Tracking during recursion so we know which is first. */ + bool deleted_in_tx; }; =20 static void fire_watches(struct walk_op *op, bool parents) @@ -277,6 +289,9 @@ static int xs_node_add_content(XsNode **n, struct walk_= op *op) g_byte_array_unref((*n)->content); } (*n)->content =3D g_byte_array_ref(data); + if (op->tx_id !=3D XBT_NULL) { + (*n)->modified_in_tx =3D true; + } return 0; } =20 @@ -333,10 +348,62 @@ static int node_rm_recurse(gpointer key, gpointer val= ue, gpointer user_data) return this_inplace; } =20 +static XsNode *xs_node_copy_deleted(XsNode *old, struct walk_op *op); +static void copy_deleted_recurse(gpointer key, gpointer value, + gpointer user_data) +{ + struct walk_op *op =3D user_data; + GHashTable *siblings =3D op->op_opaque2; + XsNode *n =3D xs_node_copy_deleted(value, op); + + /* + * Reinsert the deleted_in_tx copy of the node into the parent's + * 'children' hash table. Having stashed it from op->op_opaque2 + * before the recursive call to xs_node_copy_deleted() scribbled + * over it. + */ + g_hash_table_insert(siblings, g_strdup(key), n); +} + +static XsNode *xs_node_copy_deleted(XsNode *old, struct walk_op *op) +{ + XsNode *n =3D xs_node_new(); + + n->gencnt =3D old->gencnt; + +#ifdef XS_NODE_UNIT_TEST + if (old->name) { + n->name =3D g_strdup(old->name); + } +#endif + + if (old->children) { + n->children =3D g_hash_table_new_full(g_str_hash, g_str_equal, g_f= ree, + (GDestroyNotify)xs_node_unref); + op->op_opaque2 =3D n->children; + g_hash_table_foreach(old->children, copy_deleted_recurse, op); + } + n->deleted_in_tx =3D true; + /* If it gets resurrected we only fire a watch if it lost its content = */ + if (old->content) { + n->modified_in_tx =3D true; + } + op->new_nr_nodes--; + return n; +} + static int xs_node_rm(XsNode **n, struct walk_op *op) { bool this_inplace =3D op->inplace; =20 + if (op->tx_id !=3D XBT_NULL) { + /* It's not trivial to do inplace handling for this one */ + XsNode *old =3D *n; + *n =3D xs_node_copy_deleted(old, op); + xs_node_unref(old); + return 0; + } + /* Fire watches for, and count, nodes in the subtree which get deleted= */ if ((*n)->children) { g_hash_table_foreach_remove((*n)->children, node_rm_recurse, op); @@ -408,6 +475,10 @@ static int xs_node_walk(XsNode **n, struct walk_op *op) } =20 if (child) { + if (child->deleted_in_tx) { + assert(child->ref =3D=3D 1); + /* Cannot actually set child->deleted_in_tx =3D false until la= ter */ + } xs_node_ref(child); /* * Now we own it too. But if we can modify inplace, that's going to @@ -475,6 +546,15 @@ static int xs_node_walk(XsNode **n, struct walk_op *op) xs_node_unref(old); } =20 + /* + * If we resurrected a deleted_in_tx node, we can mark it as no longer + * deleted now that we know the overall operation has succeeded. + */ + if (op->create_dirs && child && child->deleted_in_tx) { + op->new_nr_nodes++; + child->deleted_in_tx =3D false; + } + /* * The child may be NULL here, for a remove operation. Either way, * xs_node_add_child() will do the right thing and return a value @@ -709,8 +789,69 @@ int xs_impl_transaction_start(XenstoreImplState *s, un= signed int dom_id, return 0; } =20 +static gboolean tx_commit_walk(gpointer key, gpointer value, + gpointer user_data) +{ + struct walk_op *op =3D user_data; + int path_len =3D strlen(op->path); + int key_len =3D strlen(key); + bool fire_parents =3D true; + XsWatch *watch; + XsNode *n =3D value; + + if (n->ref !=3D 1) { + return false; + } + + if (n->deleted_in_tx) { + /* + * We fire watches on our parents if we are the *first* node + * to be deleted (the topmost one). This matches the behaviour + * when deleting in the live tree. + */ + fire_parents =3D !op->deleted_in_tx; + + /* Only used on the way down so no need to clear it later */ + op->deleted_in_tx =3D true; + } + + assert(key_len + path_len + 2 <=3D sizeof(op->path)); + op->path[path_len] =3D '/'; + memcpy(op->path + path_len + 1, key, key_len + 1); + + watch =3D g_hash_table_lookup(op->s->watches, op->path); + if (watch) { + op->watches =3D g_list_append(op->watches, watch); + } + + if (n->children) { + g_hash_table_foreach_remove(n->children, tx_commit_walk, op); + } + + if (watch) { + op->watches =3D g_list_remove(op->watches, watch); + } + + /* + * Don't fire watches if this node was only copied because a + * descendent was changed. The modified_in_tx flag indicates the + * ones which were really changed. + */ + if (n->modified_in_tx || n->deleted_in_tx) { + fire_watches(op, fire_parents); + n->modified_in_tx =3D false; + } + op->path[path_len] =3D '\0'; + + /* Deleted nodes really do get expunged when we commit */ + return n->deleted_in_tx; +} + static int transaction_commit(XenstoreImplState *s, XsTransaction *tx) { + struct walk_op op; + XsNode **n; + if (s->root_tx !=3D tx->base_tx) { return EAGAIN; } @@ -720,10 +861,18 @@ static int transaction_commit(XenstoreImplState *s, X= sTransaction *tx) s->root_tx =3D tx->tx_id; s->nr_nodes =3D tx->nr_nodes; =20 + init_walk_op(s, &op, XBT_NULL, tx->dom_id, "/", &n); + op.deleted_in_tx =3D false; + op.mutating =3D true; + /* - * XX: Walk the new root and fire watches on any node which has a + * Walk the new root and fire watches on any node which has a * refcount of one (which is therefore unique to this transaction). */ + if (s->root->children) { + g_hash_table_foreach_remove(s->root->children, tx_commit_walk, &op= ); + } + return 0; } =20 diff --git a/tests/unit/test-xs-node.c b/tests/unit/test-xs-node.c index 3c3654550a..02c72baa62 100644 --- a/tests/unit/test-xs-node.c +++ b/tests/unit/test-xs-node.c @@ -347,7 +347,13 @@ static void do_test_xs_node_tx(bool fail, bool commit) } else { g_assert(!err); } - g_assert(!watches->len); + if (commit && !fail) { + g_assert(!strcmp(watches->str, + "some/relative/pathwatch")); + g_string_truncate(watches, 0); + } else { + g_assert(!watches->len); + } g_assert(s->nr_nodes =3D=3D 7); =20 err =3D xs_impl_unwatch(s, DOMID_GUEST, "some", "watch", @@ -386,6 +392,226 @@ static void test_xs_node_tx_succeed(void) do_test_xs_node_tx(false, true); } =20 +static void test_xs_node_tx_rm(void) +{ + XenstoreImplState *s =3D setup(); + GString *watches =3D g_string_new(NULL); + GByteArray *data =3D g_byte_array_new(); + unsigned int tx_id =3D XBT_NULL; + int err; + + g_assert(s); + + /* Set a watch */ + err =3D xs_impl_watch(s, DOMID_GUEST, "some", "watch", + watch_cb, watches); + g_assert(!err); + g_assert(watches->len =3D=3D strlen("somewatch")); + g_assert(!strcmp(watches->str, "somewatch")); + g_string_truncate(watches, 0); + + /* Write something */ + err =3D write_str(s, DOMID_GUEST, XBT_NULL, "some/deep/dark/relative/p= ath", + "something"); + g_assert(!err); + g_assert(s->nr_nodes =3D=3D 9); + g_assert(!strcmp(watches->str, + "some/deep/dark/relative/pathwatch")); + g_string_truncate(watches, 0); + + /* Create a transaction */ + err =3D xs_impl_transaction_start(s, DOMID_GUEST, &tx_id); + g_assert(!err); + + /* Delete the tree in the transaction */ + err =3D xs_impl_rm(s, DOMID_GUEST, tx_id, "some/deep/dark"); + g_assert(!err); + g_assert(s->nr_nodes =3D=3D 9); + g_assert(!watches->len); + + err =3D xs_impl_read(s, DOMID_GUEST, XBT_NULL, "some/deep/dark/relativ= e/path", + data); + g_assert(!err); + g_assert(data->len =3D=3D strlen("something")); + g_assert(!memcmp(data->data, "something", data->len)); + g_byte_array_set_size(data, 0); + + /* Commit the transaction */ + err =3D xs_impl_transaction_end(s, DOMID_GUEST, tx_id, true); + g_assert(!err); + g_assert(s->nr_nodes =3D=3D 6); + + g_assert(!strcmp(watches->str, "some/deep/darkwatch")); + g_string_truncate(watches, 0); + + /* Now the node is gone */ + err =3D xs_impl_read(s, DOMID_GUEST, XBT_NULL, "some/deep/dark/relativ= e/path", + data); + g_assert(err =3D=3D ENOENT); + g_byte_array_unref(data); + + err =3D xs_impl_unwatch(s, DOMID_GUEST, "some", "watch", + watch_cb, watches); + g_assert(!err); + + g_string_free(watches, true); + xs_impl_delete(s); +} + +static void test_xs_node_tx_resurrect(void) +{ + XenstoreImplState *s =3D setup(); + GString *watches =3D g_string_new(NULL); + GByteArray *data =3D g_byte_array_new(); + unsigned int tx_id =3D XBT_NULL; + int err; + + g_assert(s); + + /* Write something */ + err =3D write_str(s, DOMID_GUEST, XBT_NULL, "some/deep/dark/relative/p= ath", + "something"); + g_assert(!err); + g_assert(s->nr_nodes =3D=3D 9); + + /* This node will be wiped and resurrected */ + err =3D write_str(s, DOMID_GUEST, XBT_NULL, "some/deep/dark", + "foo"); + g_assert(!err); + g_assert(s->nr_nodes =3D=3D 9); + + /* Set a watch */ + err =3D xs_impl_watch(s, DOMID_GUEST, "some", "watch", + watch_cb, watches); + g_assert(!err); + g_assert(watches->len =3D=3D strlen("somewatch")); + g_assert(!strcmp(watches->str, "somewatch")); + g_string_truncate(watches, 0); + + /* Create a transaction */ + err =3D xs_impl_transaction_start(s, DOMID_GUEST, &tx_id); + g_assert(!err); + + /* Delete the tree in the transaction */ + err =3D xs_impl_rm(s, DOMID_GUEST, tx_id, "some/deep"); + g_assert(!err); + g_assert(s->nr_nodes =3D=3D 9); + g_assert(!watches->len); + + /* Resurrect part of it */ + err =3D write_str(s, DOMID_GUEST, tx_id, "some/deep/dark/different/pat= h", + "something"); + g_assert(!err); + g_assert(s->nr_nodes =3D=3D 9); + + /* Commit the transaction */ + err =3D xs_impl_transaction_end(s, DOMID_GUEST, tx_id, true); + g_assert(!err); + g_assert(s->nr_nodes =3D=3D 9); + + /* lost data */ + g_assert(strstr(watches->str, "some/deep/dark/different/pathwatch")); + /* topmost deleted */ + g_assert(strstr(watches->str, "some/deep/dark/relativewatch")); + /* lost data */ + g_assert(strstr(watches->str, "some/deep/darkwatch")); + + g_string_truncate(watches, 0); + + /* Now the node is gone */ + err =3D xs_impl_read(s, DOMID_GUEST, XBT_NULL, "some/deep/dark/relativ= e/path", + data); + g_assert(err =3D=3D ENOENT); + g_byte_array_unref(data); + + err =3D xs_impl_unwatch(s, DOMID_GUEST, "some", "watch", + watch_cb, watches); + g_assert(!err); + + g_string_free(watches, true); + xs_impl_delete(s); +} + +static void test_xs_node_tx_resurrect2(void) +{ + XenstoreImplState *s =3D setup(); + GString *watches =3D g_string_new(NULL); + GByteArray *data =3D g_byte_array_new(); + unsigned int tx_id =3D XBT_NULL; + int err; + + g_assert(s); + + /* Write something */ + err =3D write_str(s, DOMID_GUEST, XBT_NULL, "some/deep/dark/relative/p= ath", + "something"); + g_assert(!err); + g_assert(s->nr_nodes =3D=3D 9); + + /* Another node to remain shared */ + err =3D write_str(s, DOMID_GUEST, XBT_NULL, "some/place/safe", "keepme= "); + g_assert(!err); + g_assert(s->nr_nodes =3D=3D 11); + + /* This node will be wiped and resurrected */ + err =3D write_str(s, DOMID_GUEST, XBT_NULL, "some/deep/dark", + "foo"); + g_assert(!err); + g_assert(s->nr_nodes =3D=3D 11); + + /* Set a watch */ + err =3D xs_impl_watch(s, DOMID_GUEST, "some", "watch", + watch_cb, watches); + g_assert(!err); + g_assert(watches->len =3D=3D strlen("somewatch")); + g_assert(!strcmp(watches->str, "somewatch")); + g_string_truncate(watches, 0); + + /* Create a transaction */ + err =3D xs_impl_transaction_start(s, DOMID_GUEST, &tx_id); + g_assert(!err); + + /* Delete the tree in the transaction */ + err =3D xs_impl_rm(s, DOMID_GUEST, tx_id, "some/deep"); + g_assert(!err); + g_assert(s->nr_nodes =3D=3D 11); + g_assert(!watches->len); + + /* Resurrect part of it */ + err =3D write_str(s, DOMID_GUEST, tx_id, "some/deep/dark/relative/path= ", + "something"); + g_assert(!err); + g_assert(s->nr_nodes =3D=3D 11); + + /* Commit the transaction */ + err =3D xs_impl_transaction_end(s, DOMID_GUEST, tx_id, true); + g_assert(!err); + g_assert(s->nr_nodes =3D=3D 11); + + /* lost data */ + g_assert(strstr(watches->str, "some/deep/dark/relative/pathwatch")); + /* lost data */ + g_assert(strstr(watches->str, "some/deep/darkwatch")); + + g_string_truncate(watches, 0); + + /* Now the node is gone */ + err =3D xs_impl_read(s, DOMID_GUEST, XBT_NULL, "some/deep/dark/relativ= e/path", + data); + g_assert(!err); + g_assert(data->len =3D=3D strlen("something")); + g_assert(!memcmp(data->data, "something", data->len)); + + g_byte_array_unref(data); + + err =3D xs_impl_unwatch(s, DOMID_GUEST, "some", "watch", + watch_cb, watches); + g_assert(!err); + + g_string_free(watches, true); + xs_impl_delete(s); +} + int main(int argc, char **argv) { g_test_init(&argc, &argv, NULL); @@ -395,6 +621,9 @@ int main(int argc, char **argv) g_test_add_func("/xs_node/tx_abort", test_xs_node_tx_abort); g_test_add_func("/xs_node/tx_fail", test_xs_node_tx_fail); g_test_add_func("/xs_node/tx_succeed", test_xs_node_tx_succeed); + g_test_add_func("/xs_node/tx_rm", test_xs_node_tx_rm); + g_test_add_func("/xs_node/tx_resurrect", test_xs_node_tx_resurrect); + g_test_add_func("/xs_node/tx_resurrect2", test_xs_node_tx_resurrect2); =20 return g_test_run(); } --=20 2.39.0