From owner-freebsd-fs@FreeBSD.ORG Sat Oct 30 08:16:10 2010 Return-Path: Delivered-To: freebsd-fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E41921065673; Sat, 30 Oct 2010 08:16:09 +0000 (UTC) (envelope-from avg@icyb.net.ua) Received: from citadel.icyb.net.ua (citadel.icyb.net.ua [212.40.38.140]) by mx1.freebsd.org (Postfix) with ESMTP id EBDA98FC1F; Sat, 30 Oct 2010 08:16:08 +0000 (UTC) Received: from porto.topspin.kiev.ua (porto-e.starpoint.kiev.ua [212.40.38.100]) by citadel.icyb.net.ua (8.8.8p3/ICyb-2.3exp) with ESMTP id LAA18380; Sat, 30 Oct 2010 11:16:04 +0300 (EEST) (envelope-from avg@icyb.net.ua) Received: from localhost.topspin.kiev.ua ([127.0.0.1]) by porto.topspin.kiev.ua with esmtp (Exim 4.34 (FreeBSD)) id 1PC6bc-000J4e-Ad; Sat, 30 Oct 2010 11:16:04 +0300 Message-ID: <4CCBD443.7010305@icyb.net.ua> Date: Sat, 30 Oct 2010 11:16:03 +0300 From: Andriy Gapon User-Agent: Mozilla/5.0 (X11; U; FreeBSD amd64; en-US; rv:1.9.2.11) Gecko/20101021 Lightning/1.0b2 Thunderbird/3.1.5 MIME-Version: 1.0 To: Artemiev Igor References: <3D1C350B94A44E5D95BAA1596D1EBF13@vosz.local> <20101029090417.GA17537@two.kliksys.ru> <4CCABFC2.3040701@icyb.net.ua> <4CCADD37.7000306@icyb.net.ua> <20101029152602.GA18613@two.kliksys.ru> <4CCAF0EB.4080001@icyb.net.ua> <20101029175105.GB18613@two.kliksys.ru> In-Reply-To: <20101029175105.GB18613@two.kliksys.ru> X-Enigmail-Version: 1.1.2 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: freebsd-fs@freebsd.org, freebsd-stable@freebsd.org Subject: Re: 8.1-STABLE: zfs and sendfile: problem still exists X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 30 Oct 2010 08:16:10 -0000 on 29/10/2010 20:51 Artemiev Igor said the following: > On Fri, Oct 29, 2010 at 07:06:03PM +0300, Andriy Gapon wrote: >> Probably yes, but have to be careful there. >> First, do vm_page_grab only for UIO_NOCOPY case. >> Second, the first page is already "shared busy" after vm_page_io_start() call in >> kern_sendfile; so you might need VM_ALLOC_IGN_SBUSY for that page to avoid a deadlock. > > RELENG_8 doesn`t have VM_ALLOC_IGN_SBUSY, it appeared only in HEAD. > Can you review this patch, Whether correctly I have understood? (didnt test it yet) > > --- sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c.orig 2010-10-29 18:18:23.921078337 +0200 > +++ sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c 2010-10-29 19:23:48.142513084 +0200 > @@ -449,7 +449,7 @@ > int bytes = MIN(PAGESIZE - off, len); > > again: > - if ((m = vm_page_lookup(obj, OFF_TO_IDX(start))) != NULL && > + if (uio->uio_segflg != UIO_NOCOPY && (m = vm_page_lookup(obj, OFF_TO_IDX(start))) != NULL && > vm_page_is_valid(m, off, bytes)) { > if (vm_page_sleep_if_busy(m, FALSE, "zfsmrb")) > goto again; > @@ -464,7 +464,7 @@ > uiomove_fromphys(&m, off, bytes, uio); > VM_OBJECT_LOCK(obj); > vm_page_wakeup(m); > - } else if (m != NULL && uio->uio_segflg == UIO_NOCOPY) { > + } else if (uio->uio_segflg == UIO_NOCOPY) { > /* > * The code below is here to make sendfile(2) work > * correctly with ZFS. As pointed out by ups@ > @@ -472,11 +472,9 @@ > * but it pessimize performance of sendfile/UFS, that's > * why I handle this special case in ZFS code. > */ > - KASSERT(off == 0, > - ("unexpected offset in mappedread for sendfile")); > - if (vm_page_sleep_if_busy(m, FALSE, "zfsmrb")) > - goto again; > - vm_page_busy(m); > + if((m = vm_page_lookup(obj, OFF_TO_IDX(start))) == NULL || !vm_page_is_valid(m, off, bytes)) > + m = vm_page_grab(obj, OFF_TO_IDX(start), VM_ALLOC_NORMAL|VM_ALLOC_RETRY); > + > VM_OBJECT_UNLOCK(obj); > if (dirbytes > 0) { > error = dmu_read_uio(os, zp->z_id, uio, Or maybe something like the following? It looks a little bit cleaner to me, but still is not perfect, as I have not handled unnecessary busy-ing of the pages where something more lightweight could have sufficed (e.g. wiring and shared busying). Index: sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c =================================================================== --- sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c (revision 214318) +++ sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c (working copy) @@ -67,6 +67,7 @@ #include #include #include +#include /* * Programming rules. @@ -464,7 +465,7 @@ uiomove_fromphys(&m, off, bytes, uio); VM_OBJECT_LOCK(obj); vm_page_wakeup(m); - } else if (m != NULL && uio->uio_segflg == UIO_NOCOPY) { + } else if (uio->uio_segflg == UIO_NOCOPY) { /* * The code below is here to make sendfile(2) work * correctly with ZFS. As pointed out by ups@ @@ -476,6 +477,16 @@ ("unexpected offset in mappedread for sendfile")); if (vm_page_sleep_if_busy(m, FALSE, "zfsmrb")) goto again; + if (m == NULL) { + m = vm_page_alloc(obj, OFF_TO_IDX(start), + VM_ALLOC_NOBUSY | VM_ALLOC_SYSTEM); + if (m == NULL) { + VM_OBJECT_UNLOCK(obj); + VM_WAIT; + VM_OBJECT_LOCK(obj); + goto again; + } + } vm_page_busy(m); VM_OBJECT_UNLOCK(obj); if (dirbytes > 0) { -- Andriy Gapon