[Qemu-devel] [PATCH for 2.6 1/1] nbd: fix assert() on qemu-nbd stop

[Qemu-devel] [PATCH for 2.6 1/1] nbd: fix assert() on qemu-nbd stop

[Denis V. Lunev]

From: Pavel Butsykin <pbutsykin@virtuozzo.com>

>From time to time qemu-nbd is crashing on the following assert:
    assert(state == TERMINATING);
    nbd_export_closed
    nbd_export_put
    main
and the state at the moment of the crash is evaluated to TERMINATE.

During shutdown process of the client the nbd_client_thread thread sends
SIGTERM signal and the main thread calls the nbd_client_closed callback.
If the SIGTERM callback will be executed after change the state to
TERMINATING, then the state will once again be TERMINATE.

To solve the issue, we must change the state to TERMINATE only if the state
is RUNNING. In the other case we are shutting down already.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-nbd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index ca4a724..956013f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -213,7 +213,7 @@ static int find_partition(BlockBackend *blk, int partition,
 
 static void termsig_handler(int signum)
 {
-    state = TERMINATE;
+    atomic_cmpxchg(&state, RUNNING, TERMINATE);
     qemu_notify_event();
 }
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH for 2.6 1/1] nbd: fix assert() on qemu-nbd stop

[Paolo Bonzini]

On 14/04/2016 12:20, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> From time to time qemu-nbd is crashing on the following assert:
>     assert(state == TERMINATING);
>     nbd_export_closed
>     nbd_export_put
>     main
> and the state at the moment of the crash is evaluated to TERMINATE.
> 
> During shutdown process of the client the nbd_client_thread thread sends
> SIGTERM signal and the main thread calls the nbd_client_closed callback.
> If the SIGTERM callback will be executed after change the state to
> TERMINATING, then the state will once again be TERMINATE.
> 
> To solve the issue, we must change the state to TERMINATE only if the state
> is RUNNING. In the other case we are shutting down already.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qemu-nbd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index ca4a724..956013f 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -213,7 +213,7 @@ static int find_partition(BlockBackend *blk, int partition,
>  
>  static void termsig_handler(int signum)
>  {
> -    state = TERMINATE;
> +    atomic_cmpxchg(&state, RUNNING, TERMINATE);

Just a simple "if" is enough (the rest of the file is not using any of
atomic{mb_,}{read,set}.

I'm not able to send further pull requests for 2.6, can the block
maintainers help?

Paolo

>      qemu_notify_event();
>  }
>  
> 

Re: [Qemu-devel] [PATCH for 2.6 1/1] nbd: fix assert() on qemu-nbd stop

[Denis V. Lunev]

On 04/14/2016 04:23 PM, Paolo Bonzini wrote:
>
> On 14/04/2016 12:20, Denis V. Lunev wrote:
>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>
>>  From time to time qemu-nbd is crashing on the following assert:
>>      assert(state == TERMINATING);
>>      nbd_export_closed
>>      nbd_export_put
>>      main
>> and the state at the moment of the crash is evaluated to TERMINATE.
>>
>> During shutdown process of the client the nbd_client_thread thread sends
>> SIGTERM signal and the main thread calls the nbd_client_closed callback.
>> If the SIGTERM callback will be executed after change the state to
>> TERMINATING, then the state will once again be TERMINATE.
>>
>> To solve the issue, we must change the state to TERMINATE only if the state
>> is RUNNING. In the other case we are shutting down already.
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   qemu-nbd.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index ca4a724..956013f 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -213,7 +213,7 @@ static int find_partition(BlockBackend *blk, int partition,
>>   
>>   static void termsig_handler(int signum)
>>   {
>> -    state = TERMINATE;
>> +    atomic_cmpxchg(&state, RUNNING, TERMINATE);
> Just a simple "if" is enough (the rest of the file is not using any of
> atomic{mb_,}{read,set}.
>
> I'm not able to send further pull requests for 2.6, can the block
> maintainers help?
>
> Paolo

unfortunately, if () would be not enough. The race is with different
thread which can run on the another CPU. Thus this OP should be
atomic or some locking is required.

Den

P.S. Added more block guys...

Re: [Qemu-devel] [PATCH for 2.6 1/1] nbd: fix assert() on qemu-nbd stop

[Paolo Bonzini]

On 14/04/2016 15:40, Denis V. Lunev wrote:
> On 04/14/2016 04:23 PM, Paolo Bonzini wrote:
>>
>> On 14/04/2016 12:20, Denis V. Lunev wrote:
>>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>>
>>>  From time to time qemu-nbd is crashing on the following assert:
>>>      assert(state == TERMINATING);
>>>      nbd_export_closed
>>>      nbd_export_put
>>>      main
>>> and the state at the moment of the crash is evaluated to TERMINATE.
>>>
>>> During shutdown process of the client the nbd_client_thread thread sends
>>> SIGTERM signal and the main thread calls the nbd_client_closed callback.
>>> If the SIGTERM callback will be executed after change the state to
>>> TERMINATING, then the state will once again be TERMINATE.
>>>
>>> To solve the issue, we must change the state to TERMINATE only if the
>>> state
>>> is RUNNING. In the other case we are shutting down already.
>>>
>>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> CC: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>   qemu-nbd.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>>> index ca4a724..956013f 100644
>>> --- a/qemu-nbd.c
>>> +++ b/qemu-nbd.c
>>> @@ -213,7 +213,7 @@ static int find_partition(BlockBackend *blk, int
>>> partition,
>>>     static void termsig_handler(int signum)
>>>   {
>>> -    state = TERMINATE;
>>> +    atomic_cmpxchg(&state, RUNNING, TERMINATE);
>> Just a simple "if" is enough (the rest of the file is not using any of
>> atomic{mb_,}{read,set}.
>>
>> I'm not able to send further pull requests for 2.6, can the block
>> maintainers help?
>>
>> Paolo
> 
> unfortunately, if () would be not enough. The race is with different
> thread which can run on the another CPU. Thus this OP should be
> atomic or some locking is required.

Oh, qemu-nbd doesn't use qemu_thread_create (qemu_thread_create ensures
that the sigmask is all-blocked except in the main thread)!

Patch is okay then (though we certainly want to revisit it in 2.7).

Paolo

> 
> Den
> 
> P.S. Added more block guys...
> 
> 

Re: [Qemu-devel] [PATCH for 2.6 1/1] nbd: fix assert() on qemu-nbd stop

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1128 bytes --]

On 14.04.2016 12:20, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> From time to time qemu-nbd is crashing on the following assert:
>     assert(state == TERMINATING);
>     nbd_export_closed
>     nbd_export_put
>     main
> and the state at the moment of the crash is evaluated to TERMINATE.
> 
> During shutdown process of the client the nbd_client_thread thread sends
> SIGTERM signal and the main thread calls the nbd_client_closed callback.
> If the SIGTERM callback will be executed after change the state to
> TERMINATING, then the state will once again be TERMINATE.
> 
> To solve the issue, we must change the state to TERMINATE only if the state
> is RUNNING. In the other case we are shutting down already.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qemu-nbd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks Pavel and Denis, I have applied the patch to my block tree:

https://github.com/XanClic/qemu/commits/block

Max


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