From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:42447)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <quintela@redhat.com>) id 1Ztcsh-00049l-D1
	for qemu-devel@nongnu.org; Tue, 03 Nov 2015 09:48:19 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <quintela@redhat.com>) id 1Ztcsb-0002ZZ-La
	for qemu-devel@nongnu.org; Tue, 03 Nov 2015 09:48:15 -0500
From: Juan Quintela <quintela@redhat.com>
In-Reply-To: <20151030155253.GD16864@stefanha-x1.localdomain> (Stefan
	Hajnoczi's message of "Fri, 30 Oct 2015 15:52:53 +0000")
References: <1446044465-19312-1-git-send-email-den@openvz.org>
	<1446044465-19312-5-git-send-email-den@openvz.org>
	<20151030155253.GD16864@stefanha-x1.localdomain>
Date: Tue, 03 Nov 2015 15:48:07 +0100
Message-ID: <87fv0nxhp4.fsf@emacs.mitica>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [Qemu-devel] [PATCH 4/4] migration: add missed
	aio_context_acquire into HMP snapshot code
Reply-To: quintela@redhat.com
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@redhat.com>
Cc: Amit Shah <amit.shah@redhat.com>, "Denis V. Lunev" <den@openvz.org>, Paolo Bonzini <pbonzini@redhat.com>, qemu-devel@nongnu.org, qemu-stable@nongnu.org

Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Wed, Oct 28, 2015 at 06:01:05PM +0300, Denis V. Lunev wrote:
>> diff --git a/block/snapshot.c b/block/snapshot.c
>> index 89500f2..f6fa17a 100644
>> --- a/block/snapshot.c
>> +++ b/block/snapshot.c
>> @@ -259,6 +259,9 @@ void bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
>>  {
>>      int ret;
>>      Error *local_err = NULL;
>> +    AioContext *aio_context = bdrv_get_aio_context(bs);
>> +
>> +    aio_context_acquire(aio_context);
>>  
>>      ret = bdrv_snapshot_delete(bs, id_or_name, NULL, &local_err);
>>      if (ret == -ENOENT || ret == -EINVAL) {
>> @@ -267,6 +270,8 @@ void bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
>>          ret = bdrv_snapshot_delete(bs, NULL, id_or_name, &local_err);
>>      }
>>  
>> +    aio_context_release(aio_context);
>> +
>>      if (ret < 0) {
>>          error_propagate(errp, local_err);
>>      }
>
> Please make the caller acquire the AioContext instead of modifying
> bdrv_snapshot_delete_id_or_name() because no other functions in this
> file acquire AioContext and the API should be consistent.

That is wrong (TM).  No other functions in migration/* know what an
aiocontext is, and they are fine, thanks O:-)

So, I guess we would have to get some other function exported from the
block layer, with the aiocontext taken?

Code ends being like this:


     while ((bs = bdrv_next(bs))) {
         if (bdrv_can_snapshot(bs) &&
             bdrv_snapshot_find(bs, snapshot, name) >= 0) {
             AioContext *ctx = bdrv_get_aio_context(bs);

             aio_context_acquire(ctx);
             bdrv_snapshot_delete_by_id_or_name(bs, name, &err);
            aio_context_release(ctx);
         .... some error handling here ...
    }


As discussed on irc, we need to get some function exported from the
block layer that does this.

I am sure that I don't understand the differences between hmp_devlvm()
and del_existing_snapshots().

>
> There's no harm in recursive locking but it is hard to write correct
> code if related functions differ in whether or not they acquire the
> AioContext.  Either all of them should acquire AioContext or none of
> them.

I don't like recursive locking, but that is a different question,
altogether.

Denis, on irc Stefan says that new locking is not valid either, so
working from there.

Thanks, Juan.