From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37139) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1byr3u-00024g-Kc for qemu-devel@nongnu.org; Mon, 24 Oct 2016 22:01:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1byr3t-0006PL-I5 for qemu-devel@nongnu.org; Mon, 24 Oct 2016 22:01:58 -0400 Message-ID: <580EBEE0.4010804@cn.fujitsu.com> Date: Tue, 25 Oct 2016 10:09:36 +0800 From: Changlong Xie MIME-Version: 1.0 References: <1476267508-19499-1-git-send-email-xiecl.fnst@cn.fujitsu.com> <580D6781.40603@cn.fujitsu.com> <4737196d-b7d7-c995-880f-e1ba07f32031@redhat.com> In-Reply-To: <4737196d-b7d7-c995-880f-e1ba07f32031@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] nbd: Use CoQueue for free_sema instead of CoMutex List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , qemu devel , qemu block , Stefan Hajnoczi , Fam Zheng , Max Reitz , Kevin Wolf , Eric Blake Cc: zhanghailiang On 10/24/2016 05:36 PM, Paolo Bonzini wrote: > > > On 24/10/2016 03:44, Changlong Xie wrote: >> Ping. Any comments? It's really a problem for NBD. > > Sorry, I haven't been sending pull requests. I'll do it this week. > Thanks : ) > Paolo > >> Thanks >> -Xie >> >> On 10/12/2016 06:18 PM, Changlong Xie wrote: >>> NBD is using the CoMutex in a way that wasn't anticipated. For >>> example, if there are >>> N(N=26, MAX_NBD_REQUESTS=16) nbd write requests, so we will invoke >>> nbd_client_co_pwritev >>> N times. >>> ---------------------------------------------------------------------------------------- >>> >>> time request Actions >>> 1 1 in_flight=1, Coroutine=C1 >>> 2 2 in_flight=2, Coroutine=C2 >>> ... >>> 15 15 in_flight=15, Coroutine=C15 >>> 16 16 in_flight=16, Coroutine=C16, free_sema->holder=C16, >>> mutex->locked=true >>> 17 17 in_flight=16, Coroutine=C17, queue C17 into free_sema->queue >>> 18 18 in_flight=16, Coroutine=C18, queue C18 into free_sema->queue >>> ... >>> 26 N in_flight=16, Coroutine=C26, queue C26 into free_sema->queue >>> ---------------------------------------------------------------------------------------- >>> >>> >>> Once nbd client recieves request No.16' reply, we will re-enter C16. >>> It's ok, because >>> it's equal to 'free_sema->holder'. >>> ---------------------------------------------------------------------------------------- >>> >>> time request Actions >>> 27 16 in_flight=15, Coroutine=C16, free_sema->holder=C16, >>> mutex->locked=false >>> ---------------------------------------------------------------------------------------- >>> >>> >>> Then nbd_coroutine_end invokes qemu_co_mutex_unlock what will pop >>> coroutines from >>> free_sema->queue's head and enter C17. More free_sema->holder is C17 now. >>> ---------------------------------------------------------------------------------------- >>> >>> time request Actions >>> 28 17 in_flight=16, Coroutine=C17, free_sema->holder=C17, >>> mutex->locked=true >>> ---------------------------------------------------------------------------------------- >>> >>> >>> In above scenario, we only recieves request No.16' reply. As time goes >>> by, nbd client will >>> almostly recieves replies from requests 1 to 15 rather than request 17 >>> who owns C17. In this >>> case, we will encounter assert "mutex->holder == self" failed since >>> Kevin's commit 0e438cdc >>> "coroutine: Let CoMutex remember who holds it". For example, if nbd >>> client recieves request >>> No.15' reply, qemu will stop unexpectedly: >>> ---------------------------------------------------------------------------------------- >>> >>> time request Actions >>> 29 15(most case) in_flight=15, Coroutine=C15, free_sema->holder=C17, >>> mutex->locked=false >>> ---------------------------------------------------------------------------------------- >>> >>> >>> Per Paolo's suggestion "The simplest fix is to change it to CoQueue, >>> which is like a condition >>> variable", this patch replaces CoMutex with CoQueue. >>> >>> Cc: Wen Congyang >>> Reported-by: zhanghailiang >>> Suggested-by: Paolo Bonzini >>> Signed-off-by: Changlong Xie >>> --- >>> block/nbd-client.c | 8 ++++---- >>> block/nbd-client.h | 2 +- >>> 2 files changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/block/nbd-client.c b/block/nbd-client.c >>> index 2cf3237..40b28ab 100644 >>> --- a/block/nbd-client.c >>> +++ b/block/nbd-client.c >>> @@ -199,8 +199,8 @@ static void nbd_coroutine_start(NbdClientSession *s, >>> { >>> /* Poor man semaphore. The free_sema is locked when no other >>> request >>> * can be accepted, and unlocked after receiving one reply. */ >>> - if (s->in_flight >= MAX_NBD_REQUESTS - 1) { >>> - qemu_co_mutex_lock(&s->free_sema); >>> + if (s->in_flight == MAX_NBD_REQUESTS) { >>> + qemu_co_queue_wait(&s->free_sema); >>> assert(s->in_flight < MAX_NBD_REQUESTS); >>> } >>> s->in_flight++; >>> @@ -214,7 +214,7 @@ static void nbd_coroutine_end(NbdClientSession *s, >>> int i = HANDLE_TO_INDEX(s, request->handle); >>> s->recv_coroutine[i] = NULL; >>> if (s->in_flight-- == MAX_NBD_REQUESTS) { >>> - qemu_co_mutex_unlock(&s->free_sema); >>> + qemu_co_queue_next(&s->free_sema); >>> } >>> } >>> >>> @@ -386,7 +386,7 @@ int nbd_client_init(BlockDriverState *bs, >>> } >>> >>> qemu_co_mutex_init(&client->send_mutex); >>> - qemu_co_mutex_init(&client->free_sema); >>> + qemu_co_queue_init(&client->free_sema); >>> client->sioc = sioc; >>> object_ref(OBJECT(client->sioc)); >>> >>> diff --git a/block/nbd-client.h b/block/nbd-client.h >>> index 044aca4..307b8b1 100644 >>> --- a/block/nbd-client.h >>> +++ b/block/nbd-client.h >>> @@ -24,7 +24,7 @@ typedef struct NbdClientSession { >>> off_t size; >>> >>> CoMutex send_mutex; >>> - CoMutex free_sema; >>> + CoQueue free_sema; >>> Coroutine *send_coroutine; >>> int in_flight; >>> >>> >> >> >> >> > > > . >