From owner-freebsd-stable@FreeBSD.ORG Mon Nov 18 09:31:16 2013 Return-Path: Delivered-To: freebsd-stable@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 174B385F for ; Mon, 18 Nov 2013 09:31:16 +0000 (UTC) Received: from mail3.transactionware.com (mail3.transactionware.com [202.68.173.211]) by mx1.freebsd.org (Postfix) with SMTP id 5A1CA2A75 for ; Mon, 18 Nov 2013 09:31:14 +0000 (UTC) Received: (qmail 2197 invoked by uid 907); 18 Nov 2013 09:24:32 -0000 Received: from eth222.nsw.adsl.internode.on.net (HELO [192.168.1.32]) (150.101.196.221) (smtp-auth username janm, mechanism plain) by mail3.transactionware.com (qpsmtpd/0.84) with (AES128-SHA encrypted) ESMTPSA; Mon, 18 Nov 2013 20:24:32 +1100 Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 7.0 \(1812\)) Subject: Re: Help with filing a [maybe] ZFS/mmap bug. From: Jan Mikkelsen In-Reply-To: <52889105.7040404@FreeBSD.org> Date: Mon, 18 Nov 2013 20:24:29 +1100 Content-Transfer-Encoding: quoted-printable Message-Id: <4B5798F5-269A-4E71-9799-E1B4E0C1545F@transactionware.com> References: <20967.760.95825.310085@gargle.gargle.HOWL><51E80B30.1090004@FreeBSD.org><20968.10645.880772.30501@gargle.gargle.HOWL><520202E5.30300@FreeBSD.org><20994.55913.93606.436124@gargle.gargle.HOWL> <21111.12085.958991.356982@gargle.gargle.HOWL> <4EB902F80CE84DD2BF36C85EF4CE8EF8@multiplay.co.uk> <5284B8A5.8040604@FreeBSD.org> <52889105.7040404@FreeBSD.org> To: Andriy Gapon X-Mailer: Apple Mail (2.1812) Cc: freebsd-stable@FreeBSD.org, Steven Hartland , hartzell@alerce.com X-BeenThere: freebsd-stable@freebsd.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: Production branch of FreeBSD source code List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Nov 2013 09:31:16 -0000 On 17 Nov 2013, at 8:48 pm, Andriy Gapon wrote: > on 14/11/2013 13:48 Andriy Gapon said the following: >> HOWEVER. I think that there is a bug that I introduced in r246293. >> Specifically I changed >> vm_page_undirty(pp); >> to >> pmap_remove_write(pp); >> vm_page_clear_dirty(pp, off, nbytes); >>=20 >> vm_page_undirty() would be a very serious (and probably obvious) bug, = if it were >> not a NOP in effect. The details are explained in the commit = message. >> But when I used vm_page_clear_dirty I completely missed the fact that = *extends* >> the range to DEV_BSIZE aligned boundaries[*]. So, given the = described behavior >> and that pmap_remove_write clears the page modified bit, it is = possible that the >> data dirty data in the extended areas will be marked as clean. >=20 > Here is a patch (for head) that should fix the described above issue: >=20 > diff --git = a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c > b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c > index 2e2cbd6..4fcd571 100644 > --- a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c > +++ b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c > @@ -328,6 +328,20 @@ page_busy(vnode_t *vp, int64_t start, int64_t = off, int64_t > nbytes) > { > vm_object_t obj; > vm_page_t pp; > + int64_t end; > + > + /* > + * At present vm_page_clear_dirty extends the cleared range to = DEV_BSIZE > + * aligned boundaries, if the range is not aligned. As a result = a > + * DEV_BSIZE subrange with partially dirty data may get marked = as clean. > + * It may happen that all DEV_BSIZE subranges are marked clean = and thus > + * the whole page would be considred clean despite have some = dirty data. > + * For this reason we should shrink the range to DEV_BSIZE = aligned > + * boundaries before calling vm_page_clear_dirty. > + */ > + end =3D rounddown2(off + nbytes, DEV_BSIZE); > + off =3D roundup2(off, DEV_BSIZE); > + nbytes =3D end - off; >=20 > obj =3D vp->v_object; > zfs_vmobject_assert_wlocked(obj); > @@ -362,7 +376,8 @@ page_busy(vnode_t *vp, int64_t start, int64_t off, = int64_t > nbytes) > ASSERT3U(pp->valid, =3D=3D, VM_PAGE_BITS_ALL); > vm_object_pip_add(obj, 1); > pmap_remove_write(pp); > - vm_page_clear_dirty(pp, off, nbytes); > + if (nbytes !=3D 0) > + vm_page_clear_dirty(pp, off, nbytes); > } > break; > } 9.2 does not seem to have a rounddown2() macro.