From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38088) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W6jTQ-0007Rx-1h for qemu-devel@nongnu.org; Fri, 24 Jan 2014 11:19:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W6jTJ-0003lL-QN for qemu-devel@nongnu.org; Fri, 24 Jan 2014 11:19:15 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37508) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W6jTJ-0003lF-JB for qemu-devel@nongnu.org; Fri, 24 Jan 2014 11:19:09 -0500 Date: Fri, 24 Jan 2014 17:18:57 +0100 From: Kevin Wolf Message-ID: <20140124161857.GN3342@dhcp-200-207.str.redhat.com> References: <1389968119-24771-1-git-send-email-kwolf@redhat.com> <1389968119-24771-22-git-send-email-kwolf@redhat.com> <20140124160920.GD3087@irqsave.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140124160920.GD3087@irqsave.net> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 21/29] block: Assert serialisation assumptions in pwritev List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Beno=EEt?= Canet Cc: pl@kamp.de, qemu-devel@nongnu.org, mreitz@redhat.com, stefanha@redhat.com, pbonzini@redhat.com, xiawenc@linux.vnet.ibm.com Am 24.01.2014 um 17:09 hat Beno=EEt Canet geschrieben: > Le Friday 17 Jan 2014 =E0 15:15:11 (+0100), Kevin Wolf a =E9crit : > > If a request calls wait_serialising_requests() and actually has to wa= it > > in this function (i.e. a coroutine yield), other requests can run and > > previously read data (like the head or tail buffer) could become > > outdated. In this case, we would have to restart from the beginning t= o > > read in the updated data. > >=20 > > However, we're lucky and don't actually need to do that: A request ca= n > > only wait in the first call of wait_serialising_requests() because we > > mark it as serialising before that call, so any later requests would > > wait. So as we don't wait in practice, we don't have to reload the da= ta. > >=20 > > This is an important assumption that may not be broken or data > > corruption will happen. Document it with some assertions. > >=20 > > Signed-off-by: Kevin Wolf > > --- > > block.c | 16 ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > >=20 > > diff --git a/block.c b/block.c > > index 859e1aa..53d9bd5 100644 > > --- a/block.c > > +++ b/block.c > > @@ -2123,14 +2123,15 @@ static bool tracked_request_overlaps(BdrvTrac= kedRequest *req, > > return true; > > } > > =20 > > -static void coroutine_fn wait_serialising_requests(BdrvTrackedReques= t *self) > > +static bool coroutine_fn wait_serialising_requests(BdrvTrackedReques= t *self) > > { > > BlockDriverState *bs =3D self->bs; > > BdrvTrackedRequest *req; > > bool retry; > > + bool waited =3D false; > > =20 > > if (!bs->serialising_in_flight) { > > - return; > > + return false; > > } > > =20 > > do { > > @@ -2156,11 +2157,14 @@ static void coroutine_fn wait_serialising_req= uests(BdrvTrackedRequest *self) > > qemu_co_queue_wait(&req->wait_queue); > > self->waiting_for =3D NULL; > > retry =3D true; > > + waited =3D true; > > break; > > } > > } > > } > > } while (retry); > > + > > + return waited; > > } > > =20 > > /* > > @@ -3011,6 +3015,7 @@ static int coroutine_fn bdrv_aligned_pwritev(Bl= ockDriverState *bs, > > QEMUIOVector *qiov, int flags) > > { > > BlockDriver *drv =3D bs->drv; > > + bool waited; > > int ret; > > =20 > > int64_t sector_num =3D offset >> BDRV_SECTOR_BITS; > > @@ -3019,7 +3024,8 @@ static int coroutine_fn bdrv_aligned_pwritev(Bl= ockDriverState *bs, > > assert((offset & (BDRV_SECTOR_SIZE - 1)) =3D=3D 0); > > assert((bytes & (BDRV_SECTOR_SIZE - 1)) =3D=3D 0); > > =20 > > - wait_serialising_requests(req); > > + waited =3D wait_serialising_requests(req); > > + assert(!waited || !req->serialising); >=20 > I we apply De Morgan's law to the expression we have: >=20 > assert(!(waited && req->serialising)); >=20 > Is it really the condition we want ? Yes. If req->serialising is true here, it's an RMW case and we already had a mark_request_serialising() + wait_serialising_requests() pair. So we have already waited for earlier requests and newer requests must be waiting for us. Therefore, it can't happen that a serialising request has to wait here. Kevin