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

[Stefan Hajnoczi]

On Mon, Jul 10, 2017 at 06:27:27PM +0200, Paolo Bonzini wrote:
> On 10/07/2017 18:24, Stefan Hajnoczi wrote:
> > 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.
> 
> I think generally we should move bdrv_drained_begin/end calls _out_ of
> block.c and into qmp_*.  If you agree, I can add this before this patch.

Yes, I think drained regions should be in callers.  That way callers can
perform multiple operations in sequence and the result is atomic.

> > 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.
> 
> Good point.  Are (1) and (3) different ways to say the same thing or do
> you have other differences in mind?

There is a difference between (1) and (3).  If a function is thread-safe
then callers can invoke it without any constraints.  If a function
requires a specific lock to be held then that it puts a constraint on
the caller.

We do have thread-safe functions in the block layer: simple functions
that fetch a BDS field atomically.  There is no need to hold the BQL for
them.

signature.asc
Description: PGP signature