From owner-svn-src-head@FreeBSD.ORG Fri Aug 30 22:30:16 2013 Return-Path: Delivered-To: svn-src-head@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 ESMTP id 36100747; Fri, 30 Aug 2013 22:30:16 +0000 (UTC) (envelope-from delphij@delphij.net) Received: from anubis.delphij.net (anubis.delphij.net [64.62.153.212]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 111C523ED; Fri, 30 Aug 2013 22:30:15 +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 3254CE55C; Fri, 30 Aug 2013 15:30:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=delphij.net; s=anubis; t=1377901809; bh=VvF8OYivPwKHyXrgEPTLP9pBjVllfH4Q0Pl9G4uQdYs=; h=Date:From:Reply-To:To:CC:Subject:References:In-Reply-To; b=BXgu22Y3kAejzPi+U9LaAorCjJjj2UY4+C/PPil5zOOW5AFJAhXyFRA4LYlpH/8iu DvMRBZwXtDxo8NLLPJAE7M9FNgwpkFtNpFJKuRBsLbm/irWLBq0Rdc5VhQQO7MdqkS +uBI45acDS7dTe2y0JIc4L8OK4N3B9tPcbw7tgqw= Message-ID: <52211CF0.1070807@delphij.net> Date: Fri, 30 Aug 2013 15:30:08 -0700 From: Xin Li Organization: The FreeBSD Project MIME-Version: 1.0 To: Davide Italiano 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> <521BDEAC.9080909@delphij.net> <521C5CAC.2060400@FreeBSD.org> <521D4C41.3060208@delphij.net> In-Reply-To: X-Enigmail-Version: 1.5.1 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: src-committers@freebsd.org, Xin LI , svn-src-all@freebsd.org, Andriy Gapon , Xin LI , svn-src-head@freebsd.org, Konstantin Belousov X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: d@delphij.net List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 30 Aug 2013 22:30:16 -0000 -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 Hi, Davide, On 08/28/13 03:19, Davide Italiano wrote: > On Wed, Aug 28, 2013 at 3:02 AM, Xin Li > wrote: On 08/27/13 01:59, Davide Italiano wrote: >>>> I've written a patch that attempts to fix the second >>>> problem. >>>> http://people.freebsd.org/~davide/review/zfsrename_recycle.diff >>>> The culprit is that we're dereferencing sdvp without the >>>> vnode lock held, so data-stability is not guaranteed. tdvp, >>>> instead, is locked at the entry point of VOP_RENAME() (at >>>> least according to vop_rename_pre()) so it can be safely >>>> used. As pointed out by Andriy ZFS has some different >>>> mechanisms wrt other file systems that allow the vnode to not >>>> be recycled when it's referenced (ZFS_ENTER), so once we get >>>> zfsvfs_t * from tdzp we can call ZFS_ENTER and dereference >>>> sdvp in a safe manner. > > I'm not sure that this is right. Now we have: > > tdzp = VTOZ(tdvp); ZFS_VERIFY_ZP(tdzp); zfsvfs = tdzp->z_zfsvfs; > ZFS_ENTER(zfsvfs); // tdzp's z_zfsvfs entered zilog = > zfsvfs->z_log; sdzp = VTOZ(sdvp); ZFS_VERIFY_ZP(sdzp); > // (*) > >> Well, if your concern is that the running thread can exit from a >> different context than the one it entered, maybe partly inlining >> ZFS_VERIFY_ZP() might workaround the problem. > >> if (sdzp->z_sa_hdl == NULL) { ZFS_EXIT(zfsvfs); /* this is the >> right context */ return (EIO); } > >> Does this make sense for you? If not, can you propose an >> alternative? I'll be away for a couple of days but I will be >> happy to discuss this further when I'll come back. > > > Note that in the (*) step, when sdzp is invalid and sdzp have > different z_zfsvfs than tdzp (for instance when the node is in the > snapshot directory; the code later would test this), we could end > up with ZFS_EXIT()'ing the wrong z_zfsvfs. I've re-examined the code with some instruments and it looks like the case where tdzp and sdzp have different z_zfsvfs were caught by upper layer in our VFS. We probably should assert this fact in our version though, as it's not very obvious for reader. Note that this also means we can define out the EXDEV check along with the VOP_REALVP call, as they do not apply to FreeBSD. Will you commit the change against the tree? 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) iQEcBAEBCgAGBQJSIRzwAAoJEG80Jeu8UPuzDA4H/R5UEFJveI9lDKIwM8ldCQbV dayXCPDSE7b3T098LMh5gA5FlDz60E633qb7Uu9eFb8Ln+vmCaBGV88AKjv+P1c4 6NIU+oJLt4uKHXqn3C+9CC5zZFZLRIoGalwtCxHEoePteF+HOwmfFOEEM0v2cSro KJgFsNIeQfWtIfeZgMNXYGeKmp26baJm9AqTmUh/tOTu7lg0i5IUV4Xv0TnsBFeX OJ+Oan/2lBRvDnxZxkHKQlRTok7Lq3aD0qhit1aOFkdPGQ+9yzrPS1/B86e2JcoA riasxJbvdTccGANj/KKiRebuoCt6v9Mwt6CaYb6UlDKotXIm1oLBD52BmdSkolk= =vvE0 -----END PGP SIGNATURE-----