1. Patchew
  2. Xen
  3. [PATCH v4 00/13] tools/xenstore: rework internal accounting
  4. View patch

[PATCH v4 05/13] tools/xenstore: use accounting buffering for node accounting

Juergen Gross posted 13 patches 2 years, 10 months ago
There is a newer version of this series
[PATCH v4 05/13] tools/xenstore: use accounting buffering for node accounting
Posted by Juergen Gross 2 years, 10 months ago 
Add the node accounting to the accounting information buffering in
order to avoid having to undo it in case of failure.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/xenstored_core.c   | 21 ++-------------------
 tools/xenstore/xenstored_domain.h |  4 ++--
 2 files changed, 4 insertions(+), 21 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 84335f5f3d..92a40ccf3f 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1452,7 +1452,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.
@@ -1797,27 +1796,11 @@ static int do_set_perms(const void *ctx, struct connection *conn,
 	old_perms = node->perms;
 	domain_nbentry_dec(conn, get_node_owner(node));
 	node->perms = perms;
-	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_nbentry_inc(conn, get_node_owner(node));
+	if (domain_nbentry_inc(conn, get_node_owner(node)))
 		return ENOMEM;
-	}
 
-	if (write_node(conn, node, false)) {
-		int saved_errno = errno;
-
-		domain_nbentry_dec(conn, get_node_owner(node));
-		node->perms = old_perms;
-		/* No failure possible as above. */
-		domain_nbentry_inc(conn, get_node_owner(node));
-
-		errno = saved_errno;
+	if (write_node(conn, node, false))
 		return errno;
-	}
 
 	fire_watches(conn, ctx, name, node, false, &old_perms);
 	send_ack(conn, XS_SET_PERMS);
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 6355ad4f37..e669f57b80 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -25,9 +25,9 @@
  * a per transaction array.
  */
 enum accitem {
+	ACC_NODES,
 	ACC_REQ_N,		/* Number of elements per request. */
-	ACC_NODES = ACC_REQ_N,
-	ACC_TR_N,		/* Number of elements per transaction. */
+	ACC_TR_N = ACC_REQ_N,	/* Number of elements per transaction. */
 	ACC_N = ACC_TR_N,	/* Number of elements per domain. */
 };
 
-- 
2.35.3
Re: [PATCH v4 05/13] tools/xenstore: use accounting buffering for node accounting
Posted by Julien Grall 2 years, 9 months ago 
Hi Juergen,

On 05/04/2023 08:03, Juergen Gross wrote:
> Add the node accounting to the accounting information buffering in
> order to avoid having to undo it in case of failure.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>   tools/xenstore/xenstored_core.c   | 21 ++-------------------
>   tools/xenstore/xenstored_domain.h |  4 ++--
>   2 files changed, 4 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 84335f5f3d..92a40ccf3f 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -1452,7 +1452,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.
> @@ -1797,27 +1796,11 @@ static int do_set_perms(const void *ctx, struct connection *conn,
>   	old_perms = node->perms;
>   	domain_nbentry_dec(conn, get_node_owner(node));

IIRC, we originally said that domain_nbentry_dec() could never fail in a 
non-transaction case. But with your current rework, the function can now 
fail because of an allocation failure.

Therefore, shouldn't we now check the error? (Possibly in a patch 
beforehand).

>   	node->perms = perms;
> -	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_nbentry_inc(conn, get_node_owner(node));
> +	if (domain_nbentry_inc(conn, get_node_owner(node)))
>   		return ENOMEM;
> -	}
>   
> -	if (write_node(conn, node, false)) {
> -		int saved_errno = errno;
> -
> -		domain_nbentry_dec(conn, get_node_owner(node));
> -		node->perms = old_perms;
> -		/* No failure possible as above. */
> -		domain_nbentry_inc(conn, get_node_owner(node));
> -
> -		errno = saved_errno;
> +	if (write_node(conn, node, false))
>   		return errno;
> -	}
>   
>   	fire_watches(conn, ctx, name, node, false, &old_perms);
>   	send_ack(conn, XS_SET_PERMS);
> diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
> index 6355ad4f37..e669f57b80 100644
> --- a/tools/xenstore/xenstored_domain.h
> +++ b/tools/xenstore/xenstored_domain.h
> @@ -25,9 +25,9 @@
>    * a per transaction array.
>    */
>   enum accitem {
> +	ACC_NODES,
>   	ACC_REQ_N,		/* Number of elements per request. */
> -	ACC_NODES = ACC_REQ_N,
> -	ACC_TR_N,		/* Number of elements per transaction. */
> +	ACC_TR_N = ACC_REQ_N,	/* Number of elements per transaction. */
>   	ACC_N = ACC_TR_N,	/* Number of elements per domain. */
>   };
>   

Cheers,

-- 
Julien Grall
Re: [PATCH v4 05/13] tools/xenstore: use accounting buffering for node accounting
Posted by Juergen Gross 2 years, 9 months ago 
On 02.05.23 20:55, Julien Grall wrote:
> Hi Juergen,
> 
> On 05/04/2023 08:03, Juergen Gross wrote:
>> Add the node accounting to the accounting information buffering in
>> order to avoid having to undo it in case of failure.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   tools/xenstore/xenstored_core.c   | 21 ++-------------------
>>   tools/xenstore/xenstored_domain.h |  4 ++--
>>   2 files changed, 4 insertions(+), 21 deletions(-)
>>
>> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
>> index 84335f5f3d..92a40ccf3f 100644
>> --- a/tools/xenstore/xenstored_core.c
>> +++ b/tools/xenstore/xenstored_core.c
>> @@ -1452,7 +1452,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.
>> @@ -1797,27 +1796,11 @@ static int do_set_perms(const void *ctx, struct 
>> connection *conn,
>>       old_perms = node->perms;
>>       domain_nbentry_dec(conn, get_node_owner(node));
> 
> IIRC, we originally said that domain_nbentry_dec() could never fail in a 
> non-transaction case. But with your current rework, the function can now fail 
> because of an allocation failure.

How would that be possible to happen?

domain_nbentry_dec() can only be called if a node is being owned by an already
known domain. So allocation is impossible to happen, as this would be a major
error in xenstored.

> Therefore, shouldn't we now check the error? (Possibly in a patch beforehand).

I don't think so. I can add a comment if you want.


Juergen
Re: [PATCH v4 05/13] tools/xenstore: use accounting buffering for node accounting
Posted by Julien Grall 2 years, 9 months ago 
Hi Juergen,

On 03/05/2023 06:13, Juergen Gross wrote:
> On 02.05.23 20:55, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 05/04/2023 08:03, Juergen Gross wrote:
>>> Add the node accounting to the accounting information buffering in
>>> order to avoid having to undo it in case of failure.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>>   tools/xenstore/xenstored_core.c   | 21 ++-------------------
>>>   tools/xenstore/xenstored_domain.h |  4 ++--
>>>   2 files changed, 4 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/tools/xenstore/xenstored_core.c 
>>> b/tools/xenstore/xenstored_core.c
>>> index 84335f5f3d..92a40ccf3f 100644
>>> --- a/tools/xenstore/xenstored_core.c
>>> +++ b/tools/xenstore/xenstored_core.c
>>> @@ -1452,7 +1452,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.
>>> @@ -1797,27 +1796,11 @@ static int do_set_perms(const void *ctx, 
>>> struct connection *conn,
>>>       old_perms = node->perms;
>>>       domain_nbentry_dec(conn, get_node_owner(node));
>>
>> IIRC, we originally said that domain_nbentry_dec() could never fail in 
>> a non-transaction case. But with your current rework, the function can 
>> now fail because of an allocation failure.
> 
> How would that be possible to happen?
> 
> domain_nbentry_dec() can only be called if a node is being owned by an 
> already
> known domain. So allocation is impossible to happen, as this would be a 
> major
> error in xenstored.

 From my understanding, the nodes accounting will be temporary and then 
committed at the end of the request.

So we would call acc_add_changed_dom() which may require allocation to 
hold the temporary accounting.

> 
>> Therefore, shouldn't we now check the error? (Possibly in a patch 
>> beforehand).
> 
> I don't think so. I can add a comment if you want.

See above.

Cheers,

-- 
Julien Grall
Re: [PATCH v4 05/13] tools/xenstore: use accounting buffering for node accounting
Posted by Juergen Gross 2 years, 9 months ago 
On 03.05.23 10:02, Julien Grall wrote:
> Hi Juergen,
> 
> On 03/05/2023 06:13, Juergen Gross wrote:
>> On 02.05.23 20:55, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> On 05/04/2023 08:03, Juergen Gross wrote:
>>>> Add the node accounting to the accounting information buffering in
>>>> order to avoid having to undo it in case of failure.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>>   tools/xenstore/xenstored_core.c   | 21 ++-------------------
>>>>   tools/xenstore/xenstored_domain.h |  4 ++--
>>>>   2 files changed, 4 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
>>>> index 84335f5f3d..92a40ccf3f 100644
>>>> --- a/tools/xenstore/xenstored_core.c
>>>> +++ b/tools/xenstore/xenstored_core.c
>>>> @@ -1452,7 +1452,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.
>>>> @@ -1797,27 +1796,11 @@ static int do_set_perms(const void *ctx, struct 
>>>> connection *conn,
>>>>       old_perms = node->perms;
>>>>       domain_nbentry_dec(conn, get_node_owner(node));
>>>
>>> IIRC, we originally said that domain_nbentry_dec() could never fail in a 
>>> non-transaction case. But with your current rework, the function can now fail 
>>> because of an allocation failure.
>>
>> How would that be possible to happen?
>>
>> domain_nbentry_dec() can only be called if a node is being owned by an already
>> known domain. So allocation is impossible to happen, as this would be a major
>> error in xenstored.
> 
>  From my understanding, the nodes accounting will be temporary and then 
> committed at the end of the request.
> 
> So we would call acc_add_changed_dom() which may require allocation to hold the 
> temporary accounting.

Ah, right, good catch!

Will add checking the return value and move the calls ahead of the tdb changes.


Juergen

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.