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

[Davide Italiano]

On Wed, Aug 28, 2013 at 3:02 AM, Xin Li <delphij at delphij.net> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
[snip]
>
> 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.
>
> 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)
>
> iQEcBAEBCgAGBQJSHUxBAAoJEG80Jeu8UPuzkzEIAKXlfGuTodrPcPkDkxBhhaOj
> QoxoT06jFwMTplysICuCpslQNyyG2Jxq654u9nMh6q5dww370eTtm2FJ0n2QTxk4
> JeWLpZVUu7VHWNXYVJQqrmATaMFHO4wVf5AYpSHn+1iCWo0kQn1MPxPw/oSUt0yw
> 1628jhWTs8n+rxQaYrYN6ewXYeylMwC50ZB0kE/gQpQZ+cKAGmrM/8tur24SQBEo
> WwrHakrv1DGJ8rv3Q53FB2iUZ+zEAZs6MJ28w32lV3vOI20jfQbEK+RR7i7HvxdV
> c/7M96wCd0X4iEqZb87VVfJFSRdQsXy/35scXhhh/BSviT7rervS8XxPGhfpXOs=
> =MzwT
> -----END PGP SIGNATURE-----

Thanks,

-- 
Davide

"There are no solved problems; there are only problems that are more
or less solved" -- Henri Poincare