From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:54451)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <den@openvz.org>) id 1YktzF-0001lR-KU
	for qemu-devel@nongnu.org; Wed, 22 Apr 2015 08:42:42 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <den@openvz.org>) id 1YktzC-00062d-7A
	for qemu-devel@nongnu.org; Wed, 22 Apr 2015 08:42:41 -0400
Received: from relay.parallels.com ([195.214.232.42]:55935)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <den@openvz.org>) id 1YktzC-00061g-0W
	for qemu-devel@nongnu.org; Wed, 22 Apr 2015 08:42:38 -0400
Message-ID: <5537972F.7040808@openvz.org>
Date: Wed, 22 Apr 2015 15:42:23 +0300
From: "Denis V. Lunev" <den@openvz.org>
MIME-Version: 1.0
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>
In-Reply-To: <20150422123905.GE27617@stefanha-thinkpad.redhat.com>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 05/27] block/parallels: add get_block_status
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Stefan Hajnoczi <stefanha@gmail.com>
Cc: Kevin Wolf <kwolf@redhat.com>, qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>, Roman Kagan <rkagan@parallels.com>

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.

I can extend the comment to clarify this.