From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51891) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YlD2M-0005Nh-TT for qemu-devel@nongnu.org; Thu, 23 Apr 2015 05:03:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YlD2J-0006Y8-4J for qemu-devel@nongnu.org; Thu, 23 Apr 2015 05:03:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42119) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YlD2I-0006Xu-V4 for qemu-devel@nongnu.org; Thu, 23 Apr 2015 05:03:07 -0400 Date: Thu, 23 Apr 2015 10:03:03 +0100 From: Stefan Hajnoczi Message-ID: <20150423090303.GB8811@stefanha-thinkpad.redhat.com> References: <1426069701-1405-1-git-send-email-den@openvz.org> <1426069701-1405-6-git-send-email-den@openvz.org> <20150422123905.GE27617@stefanha-thinkpad.redhat.com> <5537972F.7040808@openvz.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="A6N2fC+uXW/VQSAv" Content-Disposition: inline In-Reply-To: <5537972F.7040808@openvz.org> Subject: Re: [Qemu-devel] [PATCH 05/27] block/parallels: add get_block_status List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" Cc: Kevin Wolf , Stefan Hajnoczi , qemu-devel@nongnu.org, Roman Kagan --A6N2fC+uXW/VQSAv Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Apr 22, 2015 at 03:42:23PM +0300, Denis V. Lunev wrote: > On 22/04/15 15:39, Stefan Hajnoczi wrote: > >On Wed, Mar 11, 2015 at 01:27:59PM +0300, Denis V. Lunev wrote: > >>+static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs, > >>+ int64_t sector_num, int nb_sectors, int *pnum) > >>+{ > >>+ BDRVParallelsState *s = bs->opaque; > >>+ int64_t offset; > >>+ > >>+ qemu_co_mutex_lock(&s->lock); > >>+ offset = seek_to_sector(s, sector_num); > >>+ qemu_co_mutex_unlock(&s->lock); > >The lock isn't necessary here yet. It may become necessary when write > >support is added, but probably not even then since seek_to_sector() > >cannot yield (it's not a coroutine function), so there are no possible > >races with other coroutines. > > > >The same also applies for parallels_co_read(). The lock there isn't > >necessary since the block driver is read-only and has no mutable state - > >there is no shared state that needs lock protection. > do you propose to drop the lock here and add it later with write > support (patch 08)? I'd prefer to stay on the safe side. Yes, the > lock is not mandatory at the moment but in 2 patches it will be > mandatory. This locking approach is very conservative. At the end of the series, the only region that needs a lock is allocate_clusters() because bdrv_write_zeroes() may yield before s->bat_bitmap[] is assigned - we need to prevent simultaneous allocate_clusters() calls to the same clusters. I'm fine with the conservative approach, but please add a doc comment to BDRVParallelsState.lock explaining what this lock protects. This way people reading the code will understand your philosophy and be able to follow it when modifying the code. Stefan --A6N2fC+uXW/VQSAv Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVOLVHAAoJEJykq7OBq3PI6fEIAIJfTqsq3HwgvzfGf9uQLgdG sMSe01C3r3rw8a3dwt3ST0Gp0squ6n+V4YK5xB5qrka701kUxNSem0zcPoDd1OPn hZTWEVtYFpAbQl9gpotk8iVQEIRUrPazSutJ1uwxlw6cPVGrBc/tVafA5b+zwn6A opYNa7RrIkX0BcjRWU/pqhq1sPHSrS9lNWk9Ke8CZ0q6fCHJ9vxIeLlHLESYPEze vnTNCa+dwb3f2RESD37TH+y5ewZWUIvRHPKFcrHJwhzIwYVx9/2kt99rQ+LuOqsK uiueJXgGyAd6k/7lkbyJ3N/k+pOsKUDEZbtyNMXR3U/jleiiTpK9ot6ckOS01xI= =96S7 -----END PGP SIGNATURE----- --A6N2fC+uXW/VQSAv--