Re: [Qemu-devel] [PATCH V4 0/6] ide: avoid main-loop hang on CDROM/NFS failure

[John Snow <jsnow@redhat.com>]

On 11/12/2015 11:30 AM, Peter Lieven wrote:
> This series aims at avoiding a hanging main-loop if a vserver has a
> CDROM image mounted from a NFS share and that NFS share goes down.
> Typical situation is that users mount an CDROM ISO to install something
> and then forget to eject that CDROM afterwards.
> As a consequence this mounted CD is able to bring down the
> whole vserver if the backend NFS share is unreachable. This is bad
> especially if the CDROM itself is not needed anymore at this point.
> 
> v3->v4: - Patch 1: remove buf argument for cd_read_sector{_sync}
>         - Patch 1: fix iov_base offset for 2352 sector size
>         - Patch 2: fix indent [Fam]
>         - Patch 3: fix leaking of req->iov.iov_base [Fam]
> 
> v2->v3: - adressed Stefans comments on Patch 1
>         - added patches 2,4,5,6
>         - avoided the term cancel in Patch 3. Added an iovec,
>           added a BH [Stefan]
> v1->v2: - fix offset for 2352 byte sector size [Kevin]
>         - use a sync request if we continue an elementary transfer.
>           As John pointed out we enter a race condition between next
>           IDE command and async transfer otherwise. This is sill not
>           optimal, but it fixes the NFS down problems for all cases where
>           the NFS server goes down while there is no PIO CD activity.
>           Of course, it could still happen during a PIO transfer, but I
>           expect this to be the unlikelier case.
>           I spent some effort trying to read more sectors at once and
>           avoiding continuation of elementary transfers, but with
>           whatever I came up it was destroying migration between different
>           Qemu versions. I have a quite hackish patch that works and
>           should survive migration, but I am not happy with it. So I
>           would like to start with this version as it is a big improvement
>           already.
>         - Dropped Patch 5 because it is upstream meanwhile.
> 
> Peter Lieven (6):
>   ide/atapi: make PIO read requests async
>   block: add blk_abort_aio_request
>   ide: add support for IDEBufferedRequest
>   ide: orphan all buffered requests on DMA cancel
>   ide: enable buffered requests for ATAPI devices
>   ide: enable buffered requests for PIO read requests
> 
>  block/block-backend.c          |  17 +++----
>  hw/ide/atapi.c                 | 103 +++++++++++++++++++++++++++++++++++------
>  hw/ide/core.c                  |  51 +++++++++++++++++++-
>  hw/ide/internal.h              |  14 ++++++
>  hw/ide/pci.c                   |  19 ++++++++
>  include/sysemu/block-backend.h |   3 ++
>  6 files changed, 182 insertions(+), 25 deletions(-)
> 

It looks sane to me:

Reviewed-by: John Snow <jsnow@redhat.com>

Fam, Stefan: Do you think this is still sane for 2.5? I am inclined to
get it in as a fix, especially since we've been bouncing it around for
so long.

--js