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

["Denis V. Lunev" <den@openvz.org>]

On 11/03/2015 06:30 PM, Stefan Hajnoczi wrote:
> On Tue, Nov 03, 2015 at 03:48:07PM +0100, Juan Quintela wrote:
>> 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:-)
> To clarify my comment:
>
> APIs should have a consistent locking strategy.  Either all of the the
> block/snapshot.c public functions should take the lock or none of them
> should.
>
> With an inconsistent locking strategy it's really hard to review code
> and ensure it is correct because you need to look up for each function
> whether or not it takes the lock internally.
>
>> 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().
> On IRC I commented when you posted this code because there's a bug:
>
> bdrv_can_snapshot() and bdrv_snapshot_find() must be called with
> AioContext acquired.  So the function should actually be:
>
> while ((bs = bdrv_next(bs))) {
>      AioContext *ctx = bdrv_get_aio_context(ctx);
>
>      if (bdrv_can_snapshot(bs) &&
>          ...
>
>      aio_context_release(ctx);
> }
this is not that necessary after patch 6 which adds
guard into direct AIO submission code.

Anyway, I would tend to agree that locking
MUST be consistent and it MUST be ENFORCED
in proper places.

Truly speaking we must place assert(aio_context_is_owner)
at the beginning of each externally visible function in
block layer. Though this is too late in 2.5.

Den