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

[Stefan Hajnoczi]

On Tue, Nov 03, 2015 at 03:48:07PM +0100, Juan Quintela wrote:
> 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:-)

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);
}

Stefan

signature.asc
Description: PGP signature