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

[Paolo Bonzini]

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.

> 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?

More long term, I think snapshot functions should be changed to run in
coroutines.  This way they can just take the driver CoMutex.

Paolo

signature.asc
Description: OpenPGP digital signature