From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:55712)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <mjt@tls.msk.ru>) id 1S2lxe-0000Cd-QA
	for qemu-devel@nongnu.org; Wed, 29 Feb 2012 11:01:26 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <mjt@tls.msk.ru>) id 1S2lxJ-00058Q-RZ
	for qemu-devel@nongnu.org; Wed, 29 Feb 2012 11:01:02 -0500
Received: from isrv.corpit.ru ([86.62.121.231]:54854)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <mjt@tls.msk.ru>) id 1S2lxJ-00057K-Jt
	for qemu-devel@nongnu.org; Wed, 29 Feb 2012 11:00:41 -0500
Message-ID: <4F4E4B9F.8060609@msgid.tls.msk.ru>
Date: Wed, 29 Feb 2012 20:00:31 +0400
From: Michael Tokarev <mjt@tls.msk.ru>
MIME-Version: 1.0
References: <1330473276-8975-1-git-send-email-mjt@tls.msk.ru>
	<1330473276-8975-2-git-send-email-mjt@msgid.tls.msk.ru>
	<4F4E49FF.3080602@redhat.com>
In-Reply-To: <4F4E49FF.3080602@redhat.com>
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 1/3] Combine bdrv_read and bdrv_write to
	bdrv_rw
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: Paolo Bonzini <pbonzini@redhat.com>
Cc: kwolf@redhat.com, qemu-devel@nongnu.org

On 29.02.2012 19:53, Paolo Bonzini wrote:
> Il 29/02/2012 00:54, Michael Tokarev ha scritto:
>> -static coroutine_fn int cow_co_write(BlockDriverState *bs, int64_t sector_num,
>> -                                     const uint8_t *buf, int nb_sectors)
>> +static coroutine_fn int cow_co_rw(BlockDriverState *bs, int64_t sector_num,
>> +                                  uint8_t *buf, int nb_sectors, bool is_write)
>>  {
>>      int ret;
>>      BDRVCowState *s = bs->opaque;
>>      qemu_co_mutex_lock(&s->lock);
>> -    ret = cow_write(bs, sector_num, buf, nb_sectors);
>> +    ret = is_write ? cow_write(bs, sector_num, buf, nb_sectors) :
>> +                     cow_read(bs, sector_num, buf, nb_sectors);
>>      qemu_co_mutex_unlock(&s->lock);
>>      return ret;
> 
> NACK,
> 
> the real cleanup here would be to move the lock/unlock inside cow_read
> and cow_write.

And how it will be a cleanup?

The whole cow code (and a few others) is not reenterant.  Merely
moving this lock/unlock stuff inth actual methods eliminates two
current wrappers in cow_co_write() and cow_co_read(), which are
exactly the same now, and moves this exactly the same code into
actual methods, which has nothing to do with locking - they're
not reenterant, and they deal with internal to the format stuff.
Having this common locking layer on top and _outside_ of the
actual work helps removing irrelevant code from important paths.
Also, it will be too easy to forgot to unlock it there by doing
just "return" somewhere.

So that'll be not a cleanup at all.

Thanks,

/mjt