From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34921) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aqi4A-0003sX-UO for qemu-devel@nongnu.org; Thu, 14 Apr 2016 10:16:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aqi46-0003aS-FT for qemu-devel@nongnu.org; Thu, 14 Apr 2016 10:16:18 -0400 Received: from mail-pa0-x243.google.com ([2607:f8b0:400e:c03::243]:34189) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aqi46-0003aF-8I for qemu-devel@nongnu.org; Thu, 14 Apr 2016 10:16:14 -0400 Received: by mail-pa0-x243.google.com with SMTP id hb4so6967697pac.1 for ; Thu, 14 Apr 2016 07:16:14 -0700 (PDT) Sender: Paolo Bonzini References: <1460629215-11567-1-git-send-email-den@openvz.org> <570F99C5.3050801@redhat.com> <570F9DCF.1070502@openvz.org> From: Paolo Bonzini Message-ID: <570FA62A.20003@redhat.com> Date: Thu, 14 Apr 2016 16:16:10 +0200 MIME-Version: 1.0 In-Reply-To: <570F9DCF.1070502@openvz.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH for 2.6 1/1] nbd: fix assert() on qemu-nbd stop List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" , qemu-devel@nongnu.org Cc: Kevin Wolf , Max Reitz , Stefan Hajnoczi , Pavel Butsykin 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 >>> >>> 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 >>> Signed-off-by: Denis V. Lunev >>> CC: Paolo Bonzini >>> --- >>> 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... > >