From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38839) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOyXc-00074w-Cp for qemu-devel@nongnu.org; Tue, 02 Sep 2014 20:35:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XOyXW-0000fY-6m for qemu-devel@nongnu.org; Tue, 02 Sep 2014 20:35:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:11103) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOyXW-0000fA-09 for qemu-devel@nongnu.org; Tue, 02 Sep 2014 20:35:10 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s830Z7xv001704 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Tue, 2 Sep 2014 20:35:08 -0400 Date: Wed, 3 Sep 2014 08:35:05 +0800 From: Fam Zheng Message-ID: <20140903003505.GC13769@T430.redhat.com> References: <1409107756-5967-1-git-send-email-famz@redhat.com> <20140902150946.GL29067@stefanha-thinkpad.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140902150946.GL29067@stefanha-thinkpad.redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 0/8] block: Asynchronous request cancellation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , Paolo Bonzini , qemu-devel@nongnu.org On Tue, 09/02 16:09, Stefan Hajnoczi wrote: > On Wed, Aug 27, 2014 at 10:49:08AM +0800, Fam Zheng wrote: > > v3: Drop "RFC". > > Improvements according to Paolo's comments: > > 05: Just use THREAD_DONE and ret =3D -ECANCELED in thread-pool.c > > 06: Don't check dbs->cancelled for twice. > > Don't set dbs->acb to NULL. > >=20 > > v2: Drop the unfinished scsi part, which was broken in v1. (Paolo) > > Add refcnt in BlockDriverAIOCB to maintain invariant of acb avail= ability in > > bdrv_aio_cancel_async. (Paolo) > > Drop blkdebug change. (Stefan) > >=20 > > This series adds a new block layer API: > >=20 > > void bdrv_aio_cancel_async(BlockDriverAIOCB *acb); > >=20 > > which is similar to existing bdrv_aio_cancel in that it cancels an AI= O request, > > but different that it doesn't block until the request is completely c= ancelled > > or done. > >=20 > > More importantly, the completion callback, BlockDriverAIOCB.cb, is gu= aranteed > > to be called, so that the cb can take care of resource releasing and = status > > reporting to guest, etc. > >=20 > > In the following work, scsi emulation code will be shifted to use the= async > > cancelling. > >=20 > > One major benefit would be that when guest tries to cancel a request,= where the > > request cannot be cancelled easily, (due to throttled BlockDriverStat= e, a lost > > connection, or a large request queue), we don't need to block the who= le vm with > > a busy loop, which is how bdrv_aio_cancel is implemented now. > >=20 > > A test case that is easy to reproduce is, throttle a scsi-disk to a v= ery low > > limit, for example 50 bps, then stress the guest block device with dd= or fio. > >=20 > > Currently, the vm will quickly hang when it loses patience and send a= tmf > > command to cancel the request, at which point we will busy wait in > > bdrv_aio_cancel, until the request is slowly spit out from throttled_= reqs. > >=20 > > Later, we will change scsi device code to make this asynchronous, on = top of > > bdrv_aio_cancel_async. >=20 > We need to get rid of .bdrv_aio_cancel().=A0 Keeping both > .bdrv_aio_cancel() and .bdrv_aio_cancel_async() around is problematic > because they have slightly different semantics. >=20 > This patch series makes block driver cancellation more complex because > we support both approaches :(. >=20 > Could we do something like: >=20 > void bdrv_aio_cancel(BdrvAIOCB *acb) > { > bdrv_aiocb_ref(acb); > bdrv_aio_cancel_async(acb); > while (acb->ref > 1) { > aio_poll(bdrv_get_aio_context(acb->bs), true); > } > bdrv_aiocb_release(acb); > } >=20 > (pseudo-code) >=20 > Then .bdrv_aio_cancel() should be deleted. >=20 OK, I will drop .io_cancel field from AIOCBInfo, and convert all AIO implementations to only support .io_cancel_async. That way we can emulate bdrv_aio_cancel with bdrv_aio_cancel_async. Thanks for reviewing! Fam