From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41413) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YROEI-0008Ct-2d for qemu-devel@nongnu.org; Fri, 27 Feb 2015 11:57:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YROEG-00039Q-Vl for qemu-devel@nongnu.org; Fri, 27 Feb 2015 11:57:34 -0500 Date: Fri, 27 Feb 2015 16:57:23 +0000 From: Stefan Hajnoczi Message-ID: <20150227165723.GB32542@stefanha-thinkpad.redhat.com> References: <1425045947-9271-1-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="i9LlY+UWpKt15+FH" Content-Disposition: inline In-Reply-To: <1425045947-9271-1-git-send-email-mreitz@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2] block/vdi: Add locking for parallel requests List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: Kevin Wolf , Paolo Bonzini , qemu-devel@nongnu.org, qemu-block@nongnu.org, qemu-stable@nongnu.org --i9LlY+UWpKt15+FH Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Feb 27, 2015 at 09:05:47AM -0500, Max Reitz wrote: > Concurrently modifying the bmap does not seem to be a good idea; this pat= ch adds > a lock for it. See https://bugs.launchpad.net/qemu/+bug/1422307 for what > can go wrong without. >=20 > Cc: qemu-stable > Signed-off-by: Max Reitz > --- > v2: > - Make the mutex cover vdi_co_write() completely [Kevin] > - Add a TODO comment [Kevin] > --- > block/vdi.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) >=20 > diff --git a/block/vdi.c b/block/vdi.c > index 74030c6..f5f42ef 100644 > --- a/block/vdi.c > +++ b/block/vdi.c > @@ -51,6 +51,7 @@ > =20 > #include "qemu-common.h" > #include "block/block_int.h" > +#include "block/coroutine.h" > #include "qemu/module.h" > #include "migration/migration.h" > =20 > @@ -196,6 +197,8 @@ typedef struct { > /* VDI header (converted to host endianness). */ > VdiHeader header; > =20 > + CoMutex bmap_lock; > + > Error *migration_blocker; > } BDRVVdiState; > =20 > @@ -498,6 +501,8 @@ static int vdi_open(BlockDriverState *bs, QDict *opti= ons, int flags, > goto fail_free_bmap; > } > =20 > + qemu_co_mutex_init(&s->bmap_lock); > + > /* Disable migration when vdi images are used */ > error_set(&s->migration_blocker, > QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED, > @@ -607,6 +612,9 @@ static int vdi_co_write(BlockDriverState *bs, > =20 > logout("\n"); > =20 > + /* TODO: Figure out why this is necessary */ > + qemu_co_mutex_lock(&s->bmap_lock); If we don't know why bmap_lock works, it would be more approprate to take the same approach as VMDK and VHDX where there is a simply s->lock that protects all reads and writes. That way we know for sure there is no parallel I/O going on. (Since the problem is not understood, maybe reads in parallel with writes could also cause problems. Better to really do a coarse lock instead of just bmap_lock in write.) Stefan --i9LlY+UWpKt15+FH Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJU8KHzAAoJEJykq7OBq3PIVEIH/inyoEwNINqEdB1IeRsM6tnt nfS9Miqd/VrYzoC5C31Css4cuEUSp7GXaVuf4xzly89AuCRLf8xhCOxzo0Opzpyp o6+qPe9Gahd0nr39vTvxirQ0JPfJmfUcEWoaxGYJGdaHYkCTd3RzdcsvpT/LW7Hz vK+PYg0m6RW6qXzreAU18Anm4xLep+nthkr3bjyLwst1pV3UsisQoslA8ioT39nP bqPDvXuujVzCB/R7gm83NtgAfLLKvlsBI4hiV7yJcs204AmApXO/4W/pj4Jwt9ku c+Fcxr5r/vstpkzjMOKg4J3qXe6dj7hoC+GFHFw9GB1mKlDv7mllqTM/jh2752I= =V2/O -----END PGP SIGNATURE----- --i9LlY+UWpKt15+FH--