Re: [Qemu-devel] [PATCH v3 00/27] Block layer cleanup & fixes

[Kevin Wolf]

Am 06.09.2011 18:58, schrieb Markus Armbruster:
> This patch series looks bigger than it is.  All the patches are small
> and hopefully easy to review.
> 
> Objectives:
> 
> * Push BlockDriverState members locked, tray_open, media_changed into
>   device models, where they belong.  Kevin picked the patches pushing
>   media_changed from v2, so that part's gone already.
> 
> * BlockDriverState member removable is a confusing mess, replace it.
> 
> * Improve eject -f.
> 
> Also clean up minor messes as they get in the way.
> 
> It is based on Kevin's block branch.
> 
> Part I: Move tray state to device models
> PATCH 01-05 IDE tray open/closed
> PATCH 06-07 SCSI tray open/closed
> PATCH 08-09 Kill BlockDriverState tray_open
> PATCH 10-11 IDE & SCSI track tray lock
> PATCH 12-14 Kill BlockDriverState locked
> PATCH 15-16 IDE & SCSI tray bug fixes
> PATCH 17    IDE migrate tray state
> 
> Part II: Miscellaneous
> PATCH 18-19 Replace BlockDriverState removable
> PATCH 20    Cover tray open/closed in info block
> PATCH 21-25 Reduce unclean use of block_int.h
> PATCH 26-27 Improve eject -f
> 
> Naturally, I want all parts applied.  But I did my best to make
> applying only a prefix workable, too.
> 
> Review invited from:
> 
> * Kevin, Christoph and Amit reviewed previous versions.
> 
> * Hannes ACKed the SCSI stuff in v2.
> 
> * Luiz reviewed the patches that affect QMP's query-block.  I renamed
>   response member "ejected" to "tray-open" since then.
> 
> * Paolo commented PATCH 17 `ide/atapi: Preserve tray state on
>   migration'.
> 
> * Stefano reviewed v1 of PATCH 18 (affects "-drive if=xen").
> 
> Testing
> 
> * Linux installs from CD to empty disk, then boots fine from disk.
> 
> * For both IDE and SCSI:
> 
>   - info block reports tray state correctly
> 
>   - Guest locking the tray stops eject (without -f) and change
> 
>   - eject -f; change works even while tray is locked by guest
> 
>   - Reading /dev/sr0 with tray open behaves as before: IDE closes the
>     tray and reads (matches bare metal), SCSI reports no medium
> 
>   - Tray state is migrated correctly (tested with savevm/loadvm)
> 
> * Guest still notices CD media change (IDE only, SCSI doesn't work
>   before or after my patches because GESN isn't implemented)
> 
> * Migrating ide-cd to older version works when tray is closed and
>   unlocked, else fails (tested with savevm/loadvm)
> 
> 
> v3:
> 
> * Rebased to block branch cfc606da
>   - Old PATCH 01-05,25,28-34,40 already there, drop
>   - a couple of simple conflicts in hw/scsi-disk.c
> 
> * Drop old PATCH v2 27 scsi-disk: Preserve tray state on migration,
>   because it doesn't make much sense without working SCSI migration.
> 
> * Drop old PATCH v2 22 ide/atapi: Avoid physical/virtual tray state
>   mismatch, because it has a bug, how to best fix it isn't obvious,
>   and it's not essential to this series.  Drop related PATCH v2 20,24,
>   too.  I plan to revisit them later.
> 
> * Clean up `ide: Use a table to declare which drive kinds accept each
>   command' a bit (Blue & Kevin)
> 
> * Hannes's advice:
>   - Rename some SCSISense variables
> 
> * Kevin's advice:
>   - Add comments to explain MMC-5 jargon loej
>   - Make bdrv_lock_medium() parameter locked bool.
> 
> v2:
> 
> * Rebased to block branch; non-trivial conflicts:
>   - Old PATCH 01-02,06-09 already there, drop
>   - `block: Attach non-qdev devices as well':
>     - cover new pci_piix3_xen_ide_unplug()
>     - hw/nand has been qdefivied, drop hunk
>     - onenand_init() changed, rewrite hunk
>   - pci_piix3_xen_ide_unplug() needs new PATCH 33.
> 
> * Drop old PATCH 18 `scsi-disk: Reject CD-specific SCSI commands to
>   disks' because Hannes wants to do it differently, and it's not
>   essential to this series.
> 
> * Christoph's advice:
>   - Rework `ide: Update command code definitions as per ACS-2'
>   - Add comment to `ide: Fix ATA command READ to set ATAPI signature
>     for CD-ROM'
>   - Squash `ide/atapi: Track tray open/close state' and `ide/atapi:
>     Switch from BlockDriverState's tray_open to own'
>   - Squash `ide/atapi: Track tray locked state' and `ide/atapi: Switch
>     from BlockDriverState's locked to own tray_locked'
>   - Squash `scsi-disk: Track tray locked state' and `scsi-disk: Switch
>     from BlockDriverState's locked to own tray_locked'
>   - Drop `block: Move BlockDriverAIOCB & friends from block_int.h to
>     block.h'
> 
> * Luiz's advice:
>   - Change query-block to always include "ejected" for removable
>     devices.  Requires moving `block: Show whether the guest ejected
>     the medium in info block', which causes a bunch of conflicts.
> 
> * A few cosmetic improvements
> 
> 
> Markus Armbruster (27):
>   ide: Fix ATA command READ to set ATAPI signature for CD-ROM
>   ide: Use a table to declare which drive kinds accept each command
>   ide: Reject ATA commands specific to drive kinds
>   ide/atapi: Clean up misleading name in cmd_start_stop_unit()
>   ide/atapi: Track tray open/close state
>   scsi-disk: Factor out scsi_disk_emulate_start_stop()
>   scsi-disk: Track tray open/close state
>   block: Revert entanglement of bdrv_is_inserted() with tray status
>   block: Drop tray status tracking, no longer used
>   ide/atapi: Track tray locked state
>   scsi-disk: Track tray locked state
>   block: Leave enforcing tray lock to device models
>   block: Drop medium lock tracking, ask device models instead
>   block: Rename bdrv_set_locked() to bdrv_lock_medium()
>   ide/atapi: Don't fail eject when tray is already open
>   scsi-disk: Fix START_STOP to fail when it can't eject
>   ide/atapi: Preserve tray state on migration
>   block: Clean up remaining users of "removable"
>   block: Drop BlockDriverState member removable
>   block: Show whether the virtual tray is open in info block
>   block: Move BlockConf & friends from block_int.h to block.h
>   hw: Trim superfluous #include "block_int.h"
>   block: New bdrv_set_buffer_alignment()
>   block: Reset buffer alignment on detach
>   nbd: Clean up use of block_int.h
>   block: New change_media_cb() parameter load
>   ide/atapi scsi-disk: Make monitor eject -f, then change work
> 
>  block.c             |  104 ++++++++++++++++++---------------
>  block.h             |   63 ++++++++++++++++++--
>  block/nbd.c         |    1 +
>  block/raw-posix.c   |    8 +-
>  block/raw.c         |    6 +-
>  block_int.h         |   40 +------------
>  blockdev.c          |   10 +--
>  hw/fdc.c            |    4 +-
>  hw/ide/atapi.c      |   58 +++++++++----------
>  hw/ide/cmd646.c     |    1 -
>  hw/ide/core.c       |  160 +++++++++++++++++++++++++++++++++++++++++---------
>  hw/ide/ich.c        |    1 -
>  hw/ide/internal.h   |    3 +-
>  hw/ide/isa.c        |    1 -
>  hw/ide/macio.c      |    1 -
>  hw/ide/microdrive.c |    1 -
>  hw/ide/mmio.c       |    1 -
>  hw/ide/pci.c        |    1 -
>  hw/ide/via.c        |    1 -
>  hw/lsi53c895a.c     |    1 -
>  hw/scsi-bus.c       |   10 +++
>  hw/scsi-disk.c      |   69 +++++++++++++++++++---
>  hw/scsi-generic.c   |    1 -
>  hw/scsi.h           |    5 +-
>  hw/sd.c             |    2 +-
>  hw/virtio-blk.c     |    3 +-
>  hw/virtio.h         |    2 +-
>  nbd.c               |    1 +
>  nbd.h               |    2 -
>  qmp-commands.hx     |    2 +
>  trace-events        |    1 +
>  31 files changed, 367 insertions(+), 197 deletions(-)

Thanks, applied all to the block branch after fixing up patch 2 and 14
as discussed.

Kevin