From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58131) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZyPOf-00041h-EK for qemu-devel@nongnu.org; Mon, 16 Nov 2015 14:25:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZyPOe-0007Gw-Dy for qemu-devel@nongnu.org; Mon, 16 Nov 2015 14:25:01 -0500 References: <1447345846-15624-1-git-send-email-pl@kamp.de> <564667C3.8010306@redhat.com> <20151116061732.GC20672@ad.usersys.redhat.com> From: John Snow Message-ID: <564A2D85.7020202@redhat.com> Date: Mon, 16 Nov 2015 14:24:53 -0500 MIME-Version: 1.0 In-Reply-To: <20151116061732.GC20672@ad.usersys.redhat.com> 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: Fam Zheng Cc: kwolf@redhat.com, qemu-block@nongnu.org, jcody@redhat.com, Peter Lieven , qemu-devel@nongnu.org, Stefan Hajnoczi On 11/16/2015 01:17 AM, Fam Zheng wrote: > On Fri, 11/13 17:44, John Snow wrote: >> >> >> 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 >> >> 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. > > I have no objection here, there is no new feature in the series, and guest > hanging right after installing something is admittedly annoying to users. > > Thanks, > Fam > I will send a PR. Fam, should I add your R-B? --js