Date: Fri, 30 Aug 2013 15:30:08 -0700 From: Xin Li <delphij@delphij.net> To: Davide Italiano <davide@freebsd.org> Cc: src-committers@freebsd.org, Xin LI <d@delphij.net>, svn-src-all@freebsd.org, Andriy Gapon <avg@freebsd.org>, Xin LI <delphij@freebsd.org>, svn-src-head@freebsd.org, Konstantin Belousov <kib@freebsd.org> Subject: Re: svn commit: r254585 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs Message-ID: <52211CF0.1070807@delphij.net> In-Reply-To: <CACYV=-HT_XfS9hOaPcJ-bv0-TkMNCXuQHWNk8MO7ZGQbkv%2BHeQ@mail.gmail.com> References: <201308202231.r7KMVERi068300@svn.freebsd.org> <20130825221517.GM24767@caravan.chchile.org> <521B75CE.70103@FreeBSD.org> <521BDEAC.9080909@delphij.net> <521C5CAC.2060400@FreeBSD.org> <CACYV=-G0i3%2BU3ubWnPSGQpEvKsuJShYJZOMvKQnjBA1aLftJsA@mail.gmail.com> <521D4C41.3060208@delphij.net> <CACYV=-HT_XfS9hOaPcJ-bv0-TkMNCXuQHWNk8MO7ZGQbkv%2BHeQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
-----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 <delphij@delphij.net> > 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 <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) iQEcBAEBCgAGBQJSIRzwAAoJEG80Jeu8UPuzDA4H/R5UEFJveI9lDKIwM8ldCQbV dayXCPDSE7b3T098LMh5gA5FlDz60E633qb7Uu9eFb8Ln+vmCaBGV88AKjv+P1c4 6NIU+oJLt4uKHXqn3C+9CC5zZFZLRIoGalwtCxHEoePteF+HOwmfFOEEM0v2cSro KJgFsNIeQfWtIfeZgMNXYGeKmp26baJm9AqTmUh/tOTu7lg0i5IUV4Xv0TnsBFeX OJ+Oan/2lBRvDnxZxkHKQlRTok7Lq3aD0qhit1aOFkdPGQ+9yzrPS1/B86e2JcoA riasxJbvdTccGANj/KKiRebuoCt6v9Mwt6CaYb6UlDKotXIm1oLBD52BmdSkolk= =vvE0 -----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?52211CF0.1070807>