Re: [Qemu-block] [PATCH 11/11] block/snapshot: do not take AioContext lock

[Stefan Hajnoczi]

On Thu, Jul 06, 2017 at 06:38:28PM +0200, Paolo Bonzini wrote:
> Snapshots are only created/destroyed/loaded under the BQL, while no
> other I/O is happening.  Snapshot information could be accessed while
> other I/O is happening, but also under the BQL so they cannot be
> modified concurrently.  The AioContext lock is overkill.  If needed,
> in the future the BQL could be split to a separate lock covering all
> snapshot operations, and the create/destroy/goto callbacks changed
> to run in a coroutine (so the driver can do mutual exclusion as usual).
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  block/snapshot.c          | 28 +---------------------------
>  blockdev.c                | 43 ++++++++++++-------------------------------
>  hmp.c                     |  7 -------
>  include/block/block_int.h |  5 +++++
>  include/block/snapshot.h  |  4 +---
>  migration/savevm.c        | 22 ----------------------
>  monitor.c                 | 10 ++--------
>  7 files changed, 21 insertions(+), 98 deletions(-)
> 
> diff --git a/block/snapshot.c b/block/snapshot.c
> index a46564e7b7..08c59d6166 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -384,9 +384,7 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState 
> *bs,
>  }
>  
>  
> -/* Group operations. All block drivers are involved.
> - * These functions will properly handle dataplane (take aio_context_acquire
> - * when appropriate for appropriate block drivers) */
> +/* Group operations. All block drivers are involved.  */

Perhaps "These functions must be called under the BQL"?

My concern with this patch series in general is that it will lead to
bugs due to inconsistencies and lack of locking documentation:

bdrv_all_delete_snapshot() is called by hmp_delvm() outside a
bdrv_drained_begin() region.  That's okay because internally
bdrv_snapshot_delete() will call bdrv_drained_begin() for the crucial
operations that require a quiesced BDS.

Compare that with bdrv_all_goto_snapshot(), which is called inside a
bdrv_drained_begin() region by load_snapshot().  Internally it doesn't
drain.

Previously the bdrv_all_*() functions behaved consistently.  We could
say that they will acquire AioContexts themselves.  Now they behave
inconsistently and while the code currently happens to work, there is no
structure that will keep it working as it is modified.

I think we're reaching a point where every BlockDriver callback and
every bdrv_*() function needs the following information:

1. Must be called under BQL?
2. Can I/O requests be in flight?
3. Is it thread-safe?

Otherwise it will be a nightmare to modify the code since these
constraints are not enforced by the compiler and undocumented.

signature.asc
Description: PGP signature