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