[Qemu-devel] [PATCH v3 00/11] block: Rework bdrv_close_all()

[Max Reitz <mreitz@redhat.com>]

Currently, bdrv_close_all() force-closes all BDSs with a BlockBackend,
which can lead to data corruption (see the iotest added in the final
patch of this series) and is most certainly very ugly.

This series reworks bdrv_close_all() to instead eject the BDS trees from
all BlockBackends and then close the monitor-owned BDS trees, which are
the only BDSs without a BB. In effect, all BDSs are closed just by
getting closed automatically due to their reference count becoming 0.

The benefit over the approach taken in v1 and v2 is that in device
models we often cannot simply drop the reference to a BB because there
may be some user which we forgot about. By ejecting the BDS trees from
the BB, the BB itself becomes unusable, but in a clean way (it will
return errors when accessed, but nothing will crash). Also, it is much
simpler (no reference tracking necessary).

The only disadvantage (I can see) is that the BBs are leaked; but this
does not matter because the qemu process is about to exit anyway.


This series depends on v2 of my series
"blockdev: BlockBackend and media" (or any later version), and on my
patch "virtio-scsi: Allocate op blocker reason before blocking".


v3:
- Patch 2: Test 109 was broken, fix it
- Patch 5:
  - Add insert notifiers
  - Make use of the insert and remove notifiers in the dataplane
    implementations of virtio-blk and virtio-scsi to set up and remove
    the op blockers (bdrv_delete() asserts there are no op blockers
    left)
  - NBD will not get a bdrv_close_all() notifier (because noone will),
    therefore e.g. rename NBDCloseNotifier to NBDEjectNotifier, etc.
- Dropped old patches 6 ("block: Add bdrv_close_all() notifiers") and 7
  ("block: Add bdrv_close_all() handlers"); they are too complicated and
  error-prone, the approach taken in this version seems (to me) much
  more straightforward
- Patch 9:
  - Moved from the end of the series here, because the next patch will
    already remove all the bdrv_close() calls from bdrv_close_all(), so
    this is needed before
  - Looks different because there are no bdrv_close_all() notifiers any
    more
- Patch 10: This is the new core of the series; instead of everyone
  having to keep track of their BB references, it is much easier to
  simply eject the BDS trees from the BlockBackends, which is what this
  patch does.
- All the other patches missing from v3 (as opposed to v2) will be sent
  in a separate follow-up series because they are not strictly necessary
  (albeit related)


git-backport-diff against v2:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/11:[----] [--] 'iotests: Move _filter_nbd into common.filter'
002/11:[0069] [FC] 'iotests: Do not redirect qemu's stderr'
003/11:[----] [-C] 'iotests: Add test for eject under NBD server'
004/11:[----] [--] 'quorum: Fix close path'
005/11:[0212] [FC] 'block: Move BDS close notifiers into BB'
006/11:[----] [--] 'block: Use blk_remove_bs() in blk_delete()'
007/11:[----] [--] 'blockdev: Use blk_remove_bs() in do_drive_del()'
008/11:[----] [-C] 'block: Make bdrv_close() static'
009/11:[0033] [FC] 'blockdev: Keep track of monitor-owned BDS'
010/11:[down] 'block: Eject BDS tree from BB at bdrv_close_all()'
011/11:[----] [-C] 'iotests: Add test for multiple BB on BDS tree


Max Reitz (11):
  iotests: Move _filter_nbd into common.filter
  iotests: Do not redirect qemu's stderr
  iotests: Add test for eject under NBD server
  quorum: Fix close path
  block: Move BDS close notifiers into BB
  block: Use blk_remove_bs() in blk_delete()
  blockdev: Use blk_remove_bs() in do_drive_del()
  block: Make bdrv_close() static
  blockdev: Keep track of monitor-owned BDS
  block: Eject BDS tree from BB at bdrv_close_all()
  iotests: Add test for multiple BB on BDS tree

 block.c                                | 25 +++------
 block/block-backend.c                  | 41 +++++++++++----
 block/quorum.c                         |  3 +-
 blockdev-nbd.c                         | 36 +------------
 blockdev.c                             | 24 +++++++--
 hw/block/dataplane/virtio-blk.c        | 93 +++++++++++++++++++++++++++-------
 hw/scsi/virtio-scsi.c                  | 64 ++++++++++++++++++++---
 include/block/block.h                  |  2 -
 include/block/block_int.h              |  6 ++-
 include/hw/virtio/virtio-scsi.h        |  8 +++
 include/sysemu/block-backend.h         |  4 +-
 nbd.c                                  | 35 +++++++++++++
 stubs/Makefile.objs                    |  1 +
 stubs/blockdev-close-all-bdrv-states.c |  5 ++
 tests/qemu-iotests/083                 | 13 +----
 tests/qemu-iotests/083.out             | 10 ----
 tests/qemu-iotests/091                 |  3 +-
 tests/qemu-iotests/096                 | 89 ++++++++++++++++++++++++++++++++
 tests/qemu-iotests/096.out             | 16 ++++++
 tests/qemu-iotests/109                 |  3 +-
 tests/qemu-iotests/109.out             | 66 ++++++++++++------------
 tests/qemu-iotests/117                 | 86 +++++++++++++++++++++++++++++++
 tests/qemu-iotests/117.out             | 14 +++++
 tests/qemu-iotests/common.filter       | 12 +++++
 tests/qemu-iotests/common.qemu         |  1 -
 tests/qemu-iotests/group               |  2 +
 26 files changed, 506 insertions(+), 156 deletions(-)
 create mode 100644 stubs/blockdev-close-all-bdrv-states.c
 create mode 100755 tests/qemu-iotests/096
 create mode 100644 tests/qemu-iotests/096.out
 create mode 100755 tests/qemu-iotests/117
 create mode 100644 tests/qemu-iotests/117.out

-- 
2.1.0