From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:34502) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RXb77-0001DI-3Z for qemu-devel@nongnu.org; Mon, 05 Dec 2011 11:10:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RXb75-0006O9-S4 for qemu-devel@nongnu.org; Mon, 05 Dec 2011 11:09:57 -0500 Received: from mx1.redhat.com ([209.132.183.28]:14106) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RXb75-0006Nx-K9 for qemu-devel@nongnu.org; Mon, 05 Dec 2011 11:09:55 -0500 Date: Mon, 5 Dec 2011 14:09:31 -0200 From: Marcelo Tosatti Message-ID: <20111205160931.GA31162@amt.cnet> References: <1322048878-26348-1-git-send-email-stefanha@linux.vnet.ibm.com> <1322048878-26348-4-git-send-email-stefanha@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1322048878-26348-4-git-send-email-stefanha@linux.vnet.ibm.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 3/8] block: add request tracking 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 Wed, Nov 23, 2011 at 11:47:53AM +0000, Stefan Hajnoczi wrote: > The block layer does not know about pending requests. This information > is necessary for copy-on-read since overlapping requests must be > serialized to prevent races that corrupt the image. >=20 > The BlockDriverState gets a new tracked_request list field which > contains all pending requests. Each request is a BdrvTrackedRequest > record with sector_num, nb_sectors, and is_write fields. >=20 > Note that request tracking is always enabled but hopefully this extra > work is so small that it doesn't justify adding an enable/disable flag. >=20 > Signed-off-by: Stefan Hajnoczi > --- > block.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++- > block_int.h | 4 ++++ > 2 files changed, 51 insertions(+), 1 deletions(-) >=20 > diff --git a/block.c b/block.c > index 0df7eb9..27c4e84 100644 > --- a/block.c > +++ b/block.c > @@ -1071,6 +1071,42 @@ void bdrv_commit_all(void) > } > } > =20 > +struct BdrvTrackedRequest { > + BlockDriverState *bs; > + int64_t sector_num; > + int nb_sectors; > + bool is_write; > + QLIST_ENTRY(BdrvTrackedRequest) list; > +}; > + > +/** > + * Remove an active request from the tracked requests list > + * > + * This function should be called when a tracked request is completing. > + */ > +static void tracked_request_end(BdrvTrackedRequest *req) > +{ > + QLIST_REMOVE(req, list); > +} > + > +/** > + * Add an active request to the tracked requests list > + */ > +static void tracked_request_begin(BdrvTrackedRequest *req, > + BlockDriverState *bs, > + int64_t sector_num, > + int nb_sectors, bool is_write) > +{ > + *req =3D (BdrvTrackedRequest){ > + .bs =3D bs, > + .sector_num =3D sector_num, > + .nb_sectors =3D nb_sectors, > + .is_write =3D is_write, > + }; > + > + QLIST_INSERT_HEAD(&bs->tracked_requests, req, list); > +} > + > /* > * Return values: > * 0 - success > @@ -1350,6 +1386,8 @@ static int coroutine_fn bdrv_co_do_readv(BlockDri= verState *bs, > int64_t sector_num, int nb_sectors, QEMUIOVector *qiov) > { > BlockDriver *drv =3D bs->drv; > + BdrvTrackedRequest req; > + int ret; > =20 > if (!drv) { > return -ENOMEDIUM; > @@ -1363,7 +1401,10 @@ static int coroutine_fn bdrv_co_do_readv(BlockDr= iverState *bs, > bdrv_io_limits_intercept(bs, false, nb_sectors); > } > =20 > - return drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov); > + tracked_request_begin(&req, bs, sector_num, nb_sectors, false); > + ret =3D drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov); > + tracked_request_end(&req); > + return ret; > } Stefan, On earlier discussion, you replied to me: " >> =A0 =A0 =A0req =3D tracked_request_add(bs, sector_num, nb_sectors, fal= se); > > The tracked request should include cluster round info? When checking A and B for overlap, only one of them needs to be rounded in order for overlap detection to be correct. Therefore we can store the original request [start, length) in tracked_requests and only round the new request. " The problem AFAICS is this: - Store a non-cluster-aligned request in the tracked request list. - Wait on that non-cluster-aligned request (wait_for_overlapping_requests). - Submit cluster-aligned request for COR request. So, the tracked request list does not properly reflect the in-flight=20 COR requests. Which can result in: 1) guest reads sector 10. 2) added to tracked request list. 3) COR code submits read for 4) unrelated guest operation writes to sector 13, nb_sectors=3D1. That is allowed to proceed without waiting because tracked request list does not reflect what COR in-flight requests. Am i missing something here?