From patchwork Wed Oct 12 10:18:28 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Changlong Xie X-Patchwork-Id: 681102 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3sv8mJ086Pz9ryZ for ; Wed, 12 Oct 2016 21:11:58 +1100 (AEDT) Received: from localhost ([::1]:60518 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buGVu-0007Q8-Pf for incoming@patchwork.ozlabs.org; Wed, 12 Oct 2016 06:11:54 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53727) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buGVJ-0006zo-GK for qemu-devel@nongnu.org; Wed, 12 Oct 2016 06:11:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1buGVI-0006KW-6Z for qemu-devel@nongnu.org; Wed, 12 Oct 2016 06:11:17 -0400 Received: from [59.151.112.132] (port=22014 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buGVC-0006Di-A3; Wed, 12 Oct 2016 06:11:11 -0400 X-IronPort-AV: E=Sophos;i="5.22,518,1449504000"; d="scan'208";a="11864666" Received: from unknown (HELO cn.fujitsu.com) ([10.167.33.5]) by heian.cn.fujitsu.com with ESMTP; 12 Oct 2016 18:10:55 +0800 Received: from G08CNEXCHPEKD03.g08.fujitsu.local (unknown [10.167.33.85]) by cn.fujitsu.com (Postfix) with ESMTP id 7111D41B5F77; Wed, 12 Oct 2016 18:10:55 +0800 (CST) Received: from changlox.g08.fujitsu.local (10.167.225.55) by G08CNEXCHPEKD03.g08.fujitsu.local (10.167.33.89) with Microsoft SMTP Server (TLS) id 14.3.279.2; Wed, 12 Oct 2016 18:10:54 +0800 From: Changlong Xie To: qemu devel , qemu block , Stefan Hajnoczi , Fam Zheng , Max Reitz , Kevin Wolf , Eric Blake Date: Wed, 12 Oct 2016 18:18:28 +0800 Message-ID: <1476267508-19499-1-git-send-email-xiecl.fnst@cn.fujitsu.com> X-Mailer: git-send-email 1.9.3 MIME-Version: 1.0 X-Originating-IP: [10.167.225.55] X-yoursite-MailScanner-ID: 7111D41B5F77.AF4F8 X-yoursite-MailScanner: Found to be clean X-yoursite-MailScanner-From: xiecl.fnst@cn.fujitsu.com X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 59.151.112.132 Subject: [Qemu-devel] [PATCH] nbd: Use CoQueue for free_sema instead of CoMutex X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: zhanghailiang Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" 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 Reviewed-by: Paolo Bonzini --- 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;