Re: [Qemu-devel] [PATCH 4/4] migration: add missed aio_context_acquire into HMP snapshot code

[Juan Quintela]

Stefan Hajnoczi <address@hidden> 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.