Re: [Qemu-devel] [RFC PATCH 00/12] block: Protect block jobs with lock / unlock API

[Paolo Bonzini <pbonzini@redhat.com>]

On 29/05/2015 12:53, Fam Zheng wrote:
> This is the partial work to introduce bdrv_lock / bdrv_unlock and use them in
> block jobs where exclusive access to a BDS is necessary. It address the same
> category of problems as [1] with a different API, as the idea proposed by Paolo
> and Kevin.
> 
> What's implemented in this series is also very close to [1], i.e. pausing
> ioeventfd and NBD server, with a notifier list.

I haven't yet fully digested patch 2 but, in the meanwhile, I can point
out how I would like the patch series to be structured.

Patch 1 is okay.

Patch 2 should be a very simple change that introduces the API simply as

   bdrv_lock(bs) { bdrv_drain(bs); }
   bdrv_unlock(bs) {}

Then you can introduce bdrv_lock/unlock (patches 3-4-5-6-12) which
remove most calls to bdrv_drain. BTW you can also remove the
bdrv_drain_all in qmp_transaction.  These patches introduce the new API
but have no semantic change.  If we agree that the API makes sense, they
can also be committed right now.

Only then you introduce full-blown locking and notifiers, and patches
7-8-9-10-11.

Thanks for your work!

Paolo

> The important missing pieces are converting bdrv_lock/unlock callers to
> coroutine, so that they can wait in the bs->lock_queue.  In other words, this
> API is not complete without that, because we can't handle the case where two
> non-coroutine callers have contention, which is now asserted in bdrv_lock as
> practically impossible.
> 
> [1]: https://lists.gnu.org/archive/html/qemu-block/2015-05/msg00800.html
> 
> Thanks,
> 
> Fam
> 
> 
> Fam Zheng (12):
>   block: Use bdrv_drain to replace uncessary bdrv_drain_all
>   block: Introduce bdrv_lock and bdrv_unlock API
>   blockdev: Lock BDS during internal snapshot transaction
>   blockdev: Lock BDS during external snapshot transaction
>   blockdev: Lock BDS during drive-backup transaction
>   blockdev: Lock BDS during blockdev-backup transaction
>   block-backend: Add blk_add_lock_unlock_notifier
>   virtio-blk: Move complete_request to 'ops' structure
>   virtio-blk: Don't handle output when backend is locked
>   virtio-scsi-dataplane: Add backend lock listener
>   nbd-server: Clear "can_read" when backend is locked
>   mirror: Protect source between bdrv_drain and bdrv_swap
> 
>  block.c                         | 16 +++++++--
>  block/block-backend.c           |  6 ++++
>  block/io.c                      | 69 ++++++++++++++++++++++++++++++++++++
>  block/mirror.c                  | 18 ++++++++--
>  block/snapshot.c                |  2 +-
>  blockdev.c                      | 17 +++++++--
>  hw/block/dataplane/virtio-blk.c | 36 ++++++++++++++++---
>  hw/block/virtio-blk.c           | 63 +++++++++++++++++++++++++++++++--
>  hw/scsi/virtio-scsi-dataplane.c | 78 ++++++++++++++++++++++++++++++-----------
>  hw/scsi/virtio-scsi.c           |  3 ++
>  include/block/block.h           | 39 +++++++++++++++++++++
>  include/block/block_int.h       |  5 +++
>  include/hw/virtio/virtio-blk.h  | 17 +++++++--
>  include/hw/virtio/virtio-scsi.h |  3 ++
>  include/sysemu/block-backend.h  |  1 +
>  migration/block.c               |  2 +-
>  nbd.c                           | 21 +++++++++++
>  17 files changed, 356 insertions(+), 40 deletions(-)
>