svn commit: r274326 - in stable/10/sys/cddl: compat/opensolaris/kern contrib/opensolaris/uts/common/fs contrib/opensolaris/uts/common/fs/zfs contrib/opensolaris/uts/common/sys
Josh Paetzel
jpaetzel at FreeBSD.org
Sun Nov 9 20:04:30 UTC 2014
Author: jpaetzel
Date: Sun Nov 9 20:04:29 2014
New Revision: 274326
URL: https://svnweb.freebsd.org/changeset/base/274326
Log:
MFC: 273641
This change addresses 4 bugs in ZFS exposed by Richard Kojedzinszky's
crash.sh script attached to FreeNAS bug 4109:
https://bugs.freenas.org/issues/4109
Three are in the snapshot layer:
a) AVG explains in his notes: https://wiki.freebsd.org/AvgVfsSolarisVsFreeBSD
"VOP_INACTIVE must not do any destructive actions to a vnode
and its filesystem node, nor invalidate them in any way."
gfs_vop_inactive and zfsctl_snapshot_inactive did just that. In
OpenSolaris VOP_INACTIVE is much closer to FreeBSD's VOP_RECLAIM.
Rename & move them to gfs_vop_reclaim and zfsctl_snapshot_reclaim
and merge in the requisite vnode_destroy from zfsctl_common_reclaim.
b) gfs_lookup_dot and various zfsctl functions do not honor the
FreeBSD VFS convention of only locking from the root downward. When
looking up ".." the convention is to drop the current leaf vnode lock before
acquiring the directory vnode and then subsequently re-acquiring the lock on the
leaf vnode. This fixes that in all the places that our exercised by crash.sh.
c) The snapshot may already be unmounted when the directory vnode is reclaimed.
Check for this case and return.
One in the common layer:
d) Callers of traverse expect the reference to the vnode passed in to be
maintained. Don't release it.
This last one may be an unclear contract. There may in fact be some callers that
do expect the reference to be dropped on success in addition to callers that
expect it to be released. In this case a further audit of the callers is needed
and a consensus on the correct behavior.
PR: 184677
Submitted by: kmacy
Reviewed by: delphij, will, avg
Sponsored by: iXsystems
Modified:
stable/10/sys/cddl/compat/opensolaris/kern/opensolaris_lookup.c
stable/10/sys/cddl/contrib/opensolaris/uts/common/fs/gfs.c
stable/10/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c
stable/10/sys/cddl/contrib/opensolaris/uts/common/sys/gfs.h
Directory Properties:
stable/10/ (props changed)
Modified: stable/10/sys/cddl/compat/opensolaris/kern/opensolaris_lookup.c
==============================================================================
--- stable/10/sys/cddl/compat/opensolaris/kern/opensolaris_lookup.c Sun Nov 9 20:04:12 2014 (r274325)
+++ stable/10/sys/cddl/compat/opensolaris/kern/opensolaris_lookup.c Sun Nov 9 20:04:29 2014 (r274326)
@@ -91,11 +91,11 @@ traverse(vnode_t **cvpp, int lktype)
error = vfs_busy(vfsp, 0);
/*
* tvp is NULL for *cvpp vnode, which we can't unlock.
+ * At least some callers expect the reference to be
+ * maintained to the original *cvpp
*/
if (tvp != NULL)
vput(cvp);
- else
- vrele(cvp);
if (error)
return (error);
Modified: stable/10/sys/cddl/contrib/opensolaris/uts/common/fs/gfs.c
==============================================================================
--- stable/10/sys/cddl/contrib/opensolaris/uts/common/fs/gfs.c Sun Nov 9 20:04:12 2014 (r274325)
+++ stable/10/sys/cddl/contrib/opensolaris/uts/common/fs/gfs.c Sun Nov 9 20:04:29 2014 (r274326)
@@ -90,7 +90,7 @@
* gfs_dir_lookup()
* gfs_dir_readdir()
*
- * gfs_vop_inactive()
+ * gfs_vop_reclaim()
* gfs_vop_lookup()
* gfs_vop_readdir()
* gfs_vop_map()
@@ -435,6 +435,8 @@ gfs_readdir_fini(gfs_readdir_state_t *st
int
gfs_lookup_dot(vnode_t **vpp, vnode_t *dvp, vnode_t *pvp, const char *nm)
{
+ int ltype;
+
if (*nm == '\0' || strcmp(nm, ".") == 0) {
VN_HOLD(dvp);
*vpp = dvp;
@@ -444,11 +446,15 @@ gfs_lookup_dot(vnode_t **vpp, vnode_t *d
ASSERT(dvp->v_flag & VROOT);
VN_HOLD(dvp);
*vpp = dvp;
+ ASSERT_VOP_ELOCKED(dvp, "gfs_lookup_dot: non-locked dvp");
} else {
+ ltype = VOP_ISLOCKED(dvp);
+ VOP_UNLOCK(dvp, 0);
VN_HOLD(pvp);
*vpp = pvp;
+ vn_lock(*vpp, LK_EXCLUSIVE | LK_RETRY);
+ vn_lock(dvp, ltype | LK_RETRY);
}
- vn_lock(*vpp, LK_EXCLUSIVE | LK_RETRY);
return (0);
}
@@ -618,7 +624,7 @@ gfs_root_create_file(size_t size, vfs_t
/*
* gfs_file_inactive()
*
- * Called from the VOP_INACTIVE() routine. If necessary, this routine will
+ * Called from the VOP_RECLAIM() routine. If necessary, this routine will
* remove the given vnode from the parent directory and clean up any references
* in the VFS layer.
*
@@ -1215,15 +1221,15 @@ gfs_vop_map(vnode_t *vp, offset_t off, s
#endif /* sun */
/*
- * gfs_vop_inactive: VOP_INACTIVE() entry point
+ * gfs_vop_reclaim: VOP_RECLAIM() entry point (solaris' VOP_INACTIVE())
*
* Given a vnode that is a GFS file or directory, call gfs_file_inactive() or
* gfs_dir_inactive() as necessary, and kmem_free()s associated private data.
*/
/* ARGSUSED */
int
-gfs_vop_inactive(ap)
- struct vop_inactive_args /* {
+gfs_vop_reclaim(ap)
+ struct vop_reclaim_args /* {
struct vnode *a_vp;
struct thread *a_td;
} */ *ap;
@@ -1236,6 +1242,7 @@ gfs_vop_inactive(ap)
else
gfs_file_inactive(vp);
+ vnode_destroy_vobject(vp);
VI_LOCK(vp);
vp->v_data = NULL;
VI_UNLOCK(vp);
Modified: stable/10/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c
==============================================================================
--- stable/10/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c Sun Nov 9 20:04:12 2014 (r274325)
+++ stable/10/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c Sun Nov 9 20:04:29 2014 (r274326)
@@ -603,16 +603,27 @@ zfsctl_freebsd_root_lookup(ap)
int nameiop = ap->a_cnp->cn_nameiop;
char nm[NAME_MAX + 1];
int err;
+ int ltype;
if ((flags & ISLASTCN) && (nameiop == RENAME || nameiop == CREATE))
return (EOPNOTSUPP);
ASSERT(ap->a_cnp->cn_namelen < sizeof(nm));
strlcpy(nm, ap->a_cnp->cn_nameptr, ap->a_cnp->cn_namelen + 1);
-
err = zfsctl_root_lookup(dvp, nm, vpp, NULL, 0, NULL, cr, NULL, NULL, NULL);
- if (err == 0 && (nm[0] != '.' || nm[1] != '\0'))
+ if (err == 0 && (nm[0] != '.' || nm[1] != '\0')) {
+ ltype = VOP_ISLOCKED(dvp);
+ if (flags & ISDOTDOT) {
+ VN_HOLD(*vpp);
+ VOP_UNLOCK(dvp, 0);
+ }
vn_lock(*vpp, LK_EXCLUSIVE | LK_RETRY);
+ if (flags & ISDOTDOT) {
+ VN_RELE(*vpp);
+ vn_lock(dvp, ltype| LK_RETRY);
+ }
+ }
+
return (err);
}
@@ -625,8 +636,8 @@ static struct vop_vector zfsctl_ops_root
.vop_access = zfsctl_common_access,
.vop_readdir = gfs_vop_readdir,
.vop_lookup = zfsctl_freebsd_root_lookup,
- .vop_inactive = gfs_vop_inactive,
- .vop_reclaim = zfsctl_common_reclaim,
+ .vop_inactive = VOP_NULL,
+ .vop_reclaim = gfs_vop_reclaim,
#ifdef TODO
.vop_pathconf = zfsctl_pathconf,
#endif
@@ -679,7 +690,7 @@ zfsctl_unmount_snap(zfs_snapentry_t *sep
* the sd_lock mutex held by our caller.
*/
ASSERT(svp->v_count == 1);
- gfs_vop_inactive(svp, cr, NULL);
+ gfs_vop_reclaim(svp, cr, NULL);
kmem_free(sep->se_name, strlen(sep->se_name) + 1);
kmem_free(sep, sizeof (zfs_snapentry_t));
@@ -949,7 +960,7 @@ zfsctl_snapdir_lookup(ap)
avl_index_t where;
zfsvfs_t *zfsvfs = dvp->v_vfsp->vfs_data;
int err;
- int flags = 0;
+ int ltype, flags = 0;
/*
* No extended attributes allowed under .zfs
@@ -973,7 +984,6 @@ zfsctl_snapdir_lookup(ap)
return (SET_ERROR(ENOENT));
ZFS_ENTER(zfsvfs);
-
if (gfs_lookup_dot(vpp, dvp, zfsvfs->z_ctldir, nm) == 0) {
ZFS_EXIT(zfsvfs);
return (0);
@@ -1420,8 +1430,8 @@ static struct vop_vector zfsctl_ops_shar
.vop_access = zfsctl_common_access,
.vop_readdir = zfsctl_shares_readdir,
.vop_lookup = zfsctl_shares_lookup,
- .vop_inactive = gfs_vop_inactive,
- .vop_reclaim = zfsctl_common_reclaim,
+ .vop_inactive = VOP_NULL,
+ .vop_reclaim = gfs_vop_reclaim,
.vop_fid = zfsctl_shares_fid,
};
#endif /* !sun */
@@ -1449,8 +1459,9 @@ zfsctl_snapshot_mknode(vnode_t *pvp, uin
return (vp);
}
+
static int
-zfsctl_snapshot_inactive(ap)
+zfsctl_snapshot_reclaim(ap)
struct vop_inactive_args /* {
struct vnode *a_vp;
struct thread *a_td;
@@ -1458,19 +1469,20 @@ zfsctl_snapshot_inactive(ap)
{
vnode_t *vp = ap->a_vp;
cred_t *cr = ap->a_td->td_ucred;
- struct vop_inactive_args iap;
+ struct vop_reclaim_args iap;
zfsctl_snapdir_t *sdp;
zfs_snapentry_t *sep, *next;
int locked;
vnode_t *dvp;
- if (vp->v_count > 0)
- goto end;
-
VERIFY(gfs_dir_lookup(vp, "..", &dvp, cr, 0, NULL, NULL) == 0);
sdp = dvp->v_data;
VOP_UNLOCK(dvp, 0);
-
+ /* this may already have been unmounted */
+ if (sdp == NULL) {
+ VN_RELE(dvp);
+ return (0);
+ }
if (!(locked = MUTEX_HELD(&sdp->sd_lock)))
mutex_enter(&sdp->sd_lock);
@@ -1494,7 +1506,6 @@ zfsctl_snapshot_inactive(ap)
mutex_exit(&sdp->sd_lock);
VN_RELE(dvp);
-end:
/*
* Dispose of the vnode for the snapshot mount point.
* This is safe to do because once this entry has been removed
@@ -1503,7 +1514,9 @@ end:
* creating a new vnode.
*/
iap.a_vp = vp;
- return (gfs_vop_inactive(&iap));
+ gfs_vop_reclaim(&iap);
+ return (0);
+
}
static int
@@ -1587,8 +1600,15 @@ zfsctl_snapshot_lookup(ap)
error = zfsctl_root_lookup(zfsvfs->z_ctldir, "snapshot", vpp,
NULL, 0, NULL, cr, NULL, NULL, NULL);
- if (error == 0)
+ if (error == 0) {
+ int ltype = VOP_ISLOCKED(dvp);
+ VN_HOLD(*vpp);
+ VOP_UNLOCK(dvp, 0);
vn_lock(*vpp, LK_EXCLUSIVE | LK_RETRY);
+ VN_RELE(*vpp);
+ vn_lock(dvp, ltype | LK_RETRY);
+ }
+
return (error);
}
@@ -1640,9 +1660,9 @@ zfsctl_snapshot_vptocnp(struct vop_vptoc
*/
static struct vop_vector zfsctl_ops_snapshot = {
.vop_default = &default_vnodeops,
- .vop_inactive = zfsctl_snapshot_inactive,
+ .vop_inactive = VOP_NULL,
.vop_lookup = zfsctl_snapshot_lookup,
- .vop_reclaim = zfsctl_common_reclaim,
+ .vop_reclaim = zfsctl_snapshot_reclaim,
.vop_getattr = zfsctl_snapshot_getattr,
.vop_fid = zfsctl_snapshot_fid,
.vop_vptocnp = zfsctl_snapshot_vptocnp,
Modified: stable/10/sys/cddl/contrib/opensolaris/uts/common/sys/gfs.h
==============================================================================
--- stable/10/sys/cddl/contrib/opensolaris/uts/common/sys/gfs.h Sun Nov 9 20:04:12 2014 (r274325)
+++ stable/10/sys/cddl/contrib/opensolaris/uts/common/sys/gfs.h Sun Nov 9 20:04:29 2014 (r274326)
@@ -149,7 +149,7 @@ extern int gfs_get_parent_ino(vnode_t *,
extern int gfs_lookup_dot(vnode_t **, vnode_t *, vnode_t *, const char *);
extern int gfs_vop_readdir(struct vop_readdir_args *);
-extern int gfs_vop_inactive(struct vop_inactive_args *);
+extern int gfs_vop_reclaim(struct vop_reclaim_args *);
#ifdef __cplusplus
More information about the svn-src-stable-10
mailing list