From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57431) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZxHzW-0002FW-2l for qemu-devel@nongnu.org; Fri, 13 Nov 2015 12:18:27 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZxHzV-0007Bw-1P for qemu-devel@nongnu.org; Fri, 13 Nov 2015 12:18:26 -0500 References: <1447345846-15624-1-git-send-email-pl@kamp.de> <5645C5E0.6060601@ilande.co.uk> From: John Snow Message-ID: <56461B57.90607@redhat.com> Date: Fri, 13 Nov 2015 12:18:15 -0500 MIME-Version: 1.0 In-Reply-To: <5645C5E0.6060601@ilande.co.uk> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V4 0/6] ide: avoid main-loop hang on CDROM/NFS failure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mark Cave-Ayland , Peter Lieven , qemu-devel@nongnu.org, qemu-block@nongnu.org Cc: kwolf@redhat.com, stefanha@gmail.com, jcody@redhat.com, famz@redhat.com On 11/13/2015 06:13 AM, Mark Cave-Ayland wrote: > On 12/11/15 16:30, 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(-) > > Patches 4-6 look like they may handle issues I've found with migration > in DMA requests on PPC g3beige and mac99 machines. Unfortunately due to > alignment issues, the macio controller has implement its own versions of > some of these routines, so should parts of these also be applied > hw/ide/macio.c aswell? > > > ATB, > > Mark. > I still might be suffering delusions that I can sneak this into 2.5 -- I wouldn't mind reworking this to share with macio later if that's the case. I don't think I'll make acceptance of this patchset conditional on macio inclusion either way, though. --js