svn commit: r254585 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs

[Xin Li]

-----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 at 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 at 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-----