[Qemu-devel] [PATCH v2 00/16] block: Protect AIO context change with perm API

Fam Zheng posted 16 patches 7 years ago
Failed in applying to current master (apply log)
Test checkpatch passed
Test docker passed
Test s390x passed
There is a newer version of this series
block.c                         | 12 ++++++---
block/backup.c                  | 11 +++++++-
block/block-backend.c           | 22 +++++++++++++++
block/commit.c                  |  6 +++--
block/mirror.c                  | 13 ++++++++-
block/stream.c                  |  3 ++-
block/vvfat.c                   |  2 +-
blockdev.c                      | 18 -------------
blockjob.c                      |  3 +++
hw/block/dataplane/virtio-blk.c | 15 ++++++++---
hw/scsi/virtio-scsi.c           |  4 +++
include/block/block.h           |  7 ++++-
include/sysemu/block-backend.h  |  1 +
nbd/server.c                    |  6 +++--
tests/Makefile.include          |  2 ++
tests/test-blk-perm.c           | 59 +++++++++++++++++++++++++++++++++++++++++
16 files changed, 149 insertions(+), 35 deletions(-)
create mode 100644 tests/test-blk-perm.c
[Qemu-devel] [PATCH v2 00/16] block: Protect AIO context change with perm API
Posted by Fam Zheng 7 years ago
v2: Address Stefan's comments:

    - Clean up redundancy in bdrv_format_default_perms change.
    - Add a test case to check both success/failure cases.
      A failure case is not possible at user interface level because of other
      checks we have, so write a unit test in tests/test-blk-perm.c.

Eject / change of scsi-cd on a virtio-scsi dataplane bus causes abort() because
the new BDS doesn't get proper bdrv_set_aio_context().

Store the AioContext in BB and do it in blk_insert_bs. That is done by
Vladimir's patch.

Other patches are to make sure such a bdrv_set_aio_context() doesn't interfere
with other BBs using other nodes from this graph.

Fam

Fam Zheng (15):
  block: Define BLK_PERM_AIO_CONTEXT_CHANGE
  block-backend: Add blk_request_perm
  blockjob: Add BLK_PERM_AIO_CONTEXT_CHANGE shared perm on bs
  blockjob: Allow aio context change on intermediate nodes
  block: Propagate BLK_PERM_AIO_CONTEXT_CHANGE down the graph
  backup: Request BLK_PERM_AIO_CONTEXT_CHANGE on target
  backup: Do initial aio context move of target via BB interface
  mirror: Request aio context change permission on target
  commit: Allow aio context change on s->base
  mirror: Do initial aio context move of target via BB interface
  virtio-scsi: Request BLK_PERM_AIO_CONTEXT_CHANGE for dataplane
  virtio-blk: Request BLK_PERM_AIO_CONTEXT_CHANGE for dataplane
  nbd: Allow BLK_PERM_AIO_CONTEXT_CHANGE on BB
  block: Add perm assertion on blk_set_aio_context
  tests: Add test case for BLK_PERM_AIO_CONTEXT_CHANGE

Vladimir Sementsov-Ogievskiy (1):
  blk: fix aio context loss on media change

 block.c                         | 12 ++++++---
 block/backup.c                  | 11 +++++++-
 block/block-backend.c           | 22 +++++++++++++++
 block/commit.c                  |  6 +++--
 block/mirror.c                  | 13 ++++++++-
 block/stream.c                  |  3 ++-
 block/vvfat.c                   |  2 +-
 blockdev.c                      | 18 -------------
 blockjob.c                      |  3 +++
 hw/block/dataplane/virtio-blk.c | 15 ++++++++---
 hw/scsi/virtio-scsi.c           |  4 +++
 include/block/block.h           |  7 ++++-
 include/sysemu/block-backend.h  |  1 +
 nbd/server.c                    |  6 +++--
 tests/Makefile.include          |  2 ++
 tests/test-blk-perm.c           | 59 +++++++++++++++++++++++++++++++++++++++++
 16 files changed, 149 insertions(+), 35 deletions(-)
 create mode 100644 tests/test-blk-perm.c

-- 
2.9.3


Re: [Qemu-devel] [Qemu-block] [PATCH v2 00/16] block: Protect AIO context change with perm API
Posted by Stefan Hajnoczi 6 years, 11 months ago
On Wed, Apr 19, 2017 at 05:43:40PM +0800, Fam Zheng wrote:
> v2: Address Stefan's comments:
> 
>     - Clean up redundancy in bdrv_format_default_perms change.
>     - Add a test case to check both success/failure cases.
>       A failure case is not possible at user interface level because of other
>       checks we have, so write a unit test in tests/test-blk-perm.c.
> 
> Eject / change of scsi-cd on a virtio-scsi dataplane bus causes abort() because
> the new BDS doesn't get proper bdrv_set_aio_context().
> 
> Store the AioContext in BB and do it in blk_insert_bs. That is done by
> Vladimir's patch.
> 
> Other patches are to make sure such a bdrv_set_aio_context() doesn't interfere
> with other BBs using other nodes from this graph.

Looks pretty good.  I had two comments that apply across all patches:

First, it is not safe to enable the new permission without registering
an aio notifier.  Another user could look up the BDS and call
bdrv_set_aio_context() on it.  I believe this bug is present for block
jobs that have additional BDSes like base/target/etc.

Second, patches that post-pone bdrv_set_aio_context() must take care to
acquire the AioContext for BDS accesses that happen before the next
bdrv_set_aio_context() call.