From owner-svn-src-all@FreeBSD.ORG Mon Aug 26 23:03:09 2013 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id 651DFB9A; Mon, 26 Aug 2013 23:03:09 +0000 (UTC) (envelope-from delphij@delphij.net) Received: from anubis.delphij.net (anubis.delphij.net [IPv6:2001:470:1:117::25]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 327A42766; Mon, 26 Aug 2013 23:03:09 +0000 (UTC) Received: from zeta.ixsystems.com (unknown [69.198.165.132]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by anubis.delphij.net (Postfix) with ESMTPSA id 7DF8CD15A; Mon, 26 Aug 2013 16:03:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=delphij.net; s=anubis; t=1377558188; bh=sr4J6UhM+ci3yYnuI+QjqVyN2iAeFS8sNg86qJ4gPjU=; h=Date:From:Reply-To:To:CC:Subject:References:In-Reply-To; b=3h/ERDWty+3akVKKg3DAWb1KCLGWPuD8Uo3BT4DCzec3A6ltYgpwE8BaGbKgOVlDB yrRPIbI7pgBWIEOTojqMH8TaVq0vMCsrdWUS2a94I0oKbiwvhV3d9mjv6ejhGVsbLE Zbe60XVOqUOBegVN9C5ReOm22k9DYMfSvltKQJ/0= Message-ID: <521BDEAC.9080909@delphij.net> Date: Mon, 26 Aug 2013 16:03:08 -0700 From: Xin Li Organization: The FreeBSD Project MIME-Version: 1.0 To: Andriy Gapon Subject: Re: svn commit: r254585 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs References: <201308202231.r7KMVERi068300@svn.freebsd.org> <20130825221517.GM24767@caravan.chchile.org> <521B75CE.70103@FreeBSD.org> In-Reply-To: <521B75CE.70103@FreeBSD.org> X-Enigmail-Version: 1.5.1 Content-Type: multipart/mixed; boundary="------------090108010401010706060506" Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Xin LI X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: d@delphij.net List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 26 Aug 2013 23:03:09 -0000 This is a multi-part message in MIME format. --------------090108010401010706060506 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 On 08/26/13 08:35, Andriy Gapon wrote: > on 26/08/2013 01:15 Jeremie Le Hen said the following: >> Hi Xin, >> >> On Tue, Aug 20, 2013 at 10:31:14PM +0000, Xin LI wrote: >>> Author: delphij Date: Tue Aug 20 22:31:13 2013 New Revision: >>> 254585 URL: http://svnweb.freebsd.org/changeset/base/254585 >>> >>> Log: MFV r254220: >>> >>> Illumos ZFS issues: 4039 zfs_rename()/zfs_link() needs stronger >>> test for XDEV >>> >>> Modified: >>> head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c >>> >>> Directory Properties: >>> head/sys/cddl/contrib/opensolaris/ (props changed) >>> >>> Modified: >>> head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c >>> >>> ============================================================================== >>> --- >>> head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c >>> Tue Aug 20 21:47:07 2013 (r254584) +++ >>> head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c >>> Tue Aug 20 22:31:13 2013 (r254585) @@ -21,6 +21,7 @@ /* * >>> Copyright (c) 2005, 2010, Oracle and/or its affiliates. All >>> rights reserved. * Copyright (c) 2013 by Delphix. All rights >>> reserved. + * Copyright 2013 Nexenta Systems, Inc. All rights >>> reserved. */ >>> >>> /* Portions Copyright 2007 Jeremy Teo */ @@ -3727,13 +3728,18 >>> @@ zfs_rename(vnode_t *sdvp, char *snm, vno if >>> (VOP_REALVP(tdvp, &realvp, ct) == 0) tdvp = realvp; >>> >>> - if (tdvp->v_vfsp != sdvp->v_vfsp || zfsctl_is_node(tdvp)) { + >>> tdzp = VTOZ(tdvp); > > The problem with this change, at least on FreeBSD, is that tdvp may > not belong to ZFS. In that case VTOZ(tdvp) does not make any sense > and would produce garbage or trigger an assert. Previously > tdvp->v_vfsp != sdvp->v_vfsp check would catch that situations. Two > side notes: - v_vfsp is actually v_mount on FreeBSD Ah that's good point. Any objection in putting a change to their _freebsd_ counterpart (zfs_freebsd_rename and zfs_freebsd_link) as attached? > - VOP_REALVP is a glorified nop on FreeBSD It's not clear to me what was the intention for having a macro instead of just ifdef'ing the code out -- maybe to prevent unwanted conflict with upstream? These two VOP's are the only consumers of VOP_REALVP in tree. > Another unrelated problem that existed before this change and has > been noted by Davide Italiano is that sdvp is not locked and so it > can potentially be recycled before ZFS_ENTER() enter and for that > reason sdzp and zfsvfs could be invalid. Because sdvp is > referenced, this problem can currently occur only if a forced > unmount runs concurrently to zfs_rename. tdvp should be locked and > thus could be used instead of sdvp as a source for zfsvfs. I think this would need more work, I'll take a look after we have this regression fixed. Cheers, - -- Xin LI https://www.delphij.net/ FreeBSD - The Power to Serve! Live free or die -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.21 (FreeBSD) iQEcBAEBCgAGBQJSG96rAAoJEG80Jeu8UPuzQG4IAK/Qw1McLNoy0egEzelYcsar iBRwoGDXfJuufCy04TEXD5rEz78VdqOl+g0tFqhSMbKHzQj+qEa6P6DIKptEnSsW AtQOQABs0gHY4SZ3MUdvdlEmFlWtyYPTqw471k2jIjRMNEM3wyslVn/SHvfymmwT s9VTI40jkoHWCUMW217jvER5co/niQDU4QL9ZNPb8vzRT02obqiq7ugZ7eqgklAI zqzB46Trn6Oplab+vNt/dWgSK/cuPwDaeTNeRBiw2YQ/uQMsOEdNPB2JqLUA5XgF WezHnotyFT/vdiQCe6dHjatOaR5ui7qWTUKTAwcq4gUrLJQx9FYYV3Im9xmesSM= =FjK7 -----END PGP SIGNATURE----- --------------090108010401010706060506 Content-Type: text/plain; charset=UTF-8; name="zfs-vnops-1.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="zfs-vnops-1.diff" Index: sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c =================================================================== --- sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c (revision 254924) +++ sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c (working copy) @@ -6250,6 +6250,9 @@ zfs_freebsd_rename(ap) ASSERT(ap->a_fcnp->cn_flags & (SAVENAME|SAVESTART)); ASSERT(ap->a_tcnp->cn_flags & (SAVENAME|SAVESTART)); + if (fdvp->v_mount != tdvp->v_mount) + return (EXDEV); + error = zfs_rename(fdvp, ap->a_fcnp->cn_nameptr, tdvp, ap->a_tcnp->cn_nameptr, ap->a_fcnp->cn_cred, NULL, 0); @@ -6308,10 +6311,15 @@ zfs_freebsd_link(ap) } */ *ap; { struct componentname *cnp = ap->a_cnp; + vnode_t *vp = ap->a_vp; + vnode_t *tdvp = ap->a_tdvp; + if (tdvp->v_mount != vp->v_mount) + return (EXDEV); + ASSERT(cnp->cn_flags & SAVENAME); - return (zfs_link(ap->a_tdvp, ap->a_vp, cnp->cn_nameptr, cnp->cn_cred, NULL, 0)); + return (zfs_link(tdvp, vp, cnp->cn_nameptr, cnp->cn_cred, NULL, 0)); } static int --------------090108010401010706060506--