From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52324) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YlYxS-00056u-P9 for qemu-devel@nongnu.org; Fri, 24 Apr 2015 04:27:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YlYxP-0008Sl-Ja for qemu-devel@nongnu.org; Fri, 24 Apr 2015 04:27:34 -0400 Received: from mail-wi0-x230.google.com ([2a00:1450:400c:c05::230]:35090) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YlYxP-0008SH-DM for qemu-devel@nongnu.org; Fri, 24 Apr 2015 04:27:31 -0400 Received: by widdi4 with SMTP id di4so13039017wid.0 for ; Fri, 24 Apr 2015 01:27:30 -0700 (PDT) Date: Fri, 24 Apr 2015 09:27:28 +0100 From: Stefan Hajnoczi Message-ID: <20150424082728.GA13278@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> <20150423090303.GB8811@stefanha-thinkpad.redhat.com> <5538BA1F.5050002@openvz.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="qMm9M+Fa2AknHoGS" Content-Disposition: inline In-Reply-To: <5538BA1F.5050002@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 , qemu-devel@nongnu.org, Stefan Hajnoczi , Roman Kagan --qMm9M+Fa2AknHoGS Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Apr 23, 2015 at 12:23:43PM +0300, Denis V. Lunev wrote: > On 23/04/15 12:03, Stefan Hajnoczi wrote: > >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(BlockDrive= rState *bs, > >>>>+ int64_t sector_num, int nb_sectors, int *pnum) > >>>>+{ > >>>>+ BDRVParallelsState *s =3D bs->opaque; > >>>>+ int64_t offset; > >>>>+ > >>>>+ qemu_co_mutex_lock(&s->lock); > >>>>+ offset =3D 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 > ok. sounds good for me. >=20 > I would like to implement the code as conservative as possible at the > beginning and place optimizations later step-by-step to have a > clear path to bisect the introductory patch. >=20 > At the moment I do not think that this locking scheme will affect > the performance but I can be wrong. There are a lot of stuff to > be implemented from the functional point of view before this will > be needed to tweak from my point of view. Okay. Stefan --qMm9M+Fa2AknHoGS Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVOf5wAAoJEJykq7OBq3PIZukIAIYVIzqjj5ktz+0b25bz+Mfe pfl/u7eqBTJADVJa1Q8JqiKaqWVGxzYzdFXntovzL88zYebtY/CHKgi2wybDNGNC SzZ6E+naygLhYkrTZ30IZELu7wkNLWpcxE6ZccBxvyUuY7MoH4sbDMiOU0jfz3XZ IXTgn7/oF24XlrptOUL/Cioh9yyjAYRyBjceyyH1RW0rp2VtixsQbbslTwDWSiGT zf+rAwPL20cqzwCAibHp5lorbO7g80YVxrtZIDjpPMiRmwh5qHsafJxxRJJ4SHRV 7TePAc/qCmip+ICTlELvrzp/gaCwKOaoJBQG5kQbL+dWctRdNwexuaOZsfCe9j4= =SLuY -----END PGP SIGNATURE----- --qMm9M+Fa2AknHoGS--