Date: Mon, 26 Aug 2013 16:03:08 -0700 From: Xin Li <delphij@delphij.net> To: Andriy Gapon <avg@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Xin LI <delphij@FreeBSD.org> Subject: Re: svn commit: r254585 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs Message-ID: <521BDEAC.9080909@delphij.net> In-Reply-To: <521B75CE.70103@FreeBSD.org> References: <201308202231.r7KMVERi068300@svn.freebsd.org> <20130825221517.GM24767@caravan.chchile.org> <521B75CE.70103@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <delphij@delphij.net> 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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?521BDEAC.9080909>