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