Date: Sat, 25 Oct 2014 17:42:45 +0000 (UTC) From: Josh Paetzel <jpaetzel@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r273641 - in head/sys/cddl: compat/opensolaris/kern contrib/opensolaris/uts/common/fs contrib/opensolaris/uts/common/fs/zfs contrib/opensolaris/uts/common/sys Message-ID: <201410251742.s9PHgjwf029487@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: jpaetzel Date: Sat Oct 25 17:42:44 2014 New Revision: 273641 URL: https://svnweb.freebsd.org/changeset/base/273641 Log: 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 MFC after: 2 weeks Sponsored by: iXsystems Modified: head/sys/cddl/compat/opensolaris/kern/opensolaris_lookup.c head/sys/cddl/contrib/opensolaris/uts/common/fs/gfs.c head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c head/sys/cddl/contrib/opensolaris/uts/common/sys/gfs.h Modified: head/sys/cddl/compat/opensolaris/kern/opensolaris_lookup.c ============================================================================== --- head/sys/cddl/compat/opensolaris/kern/opensolaris_lookup.c Sat Oct 25 17:07:35 2014 (r273640) +++ head/sys/cddl/compat/opensolaris/kern/opensolaris_lookup.c Sat Oct 25 17:42:44 2014 (r273641) @@ -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: head/sys/cddl/contrib/opensolaris/uts/common/fs/gfs.c ============================================================================== --- head/sys/cddl/contrib/opensolaris/uts/common/fs/gfs.c Sat Oct 25 17:07:35 2014 (r273640) +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/gfs.c Sat Oct 25 17:42:44 2014 (r273641) @@ -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: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c ============================================================================== --- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c Sat Oct 25 17:07:35 2014 (r273640) +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c Sat Oct 25 17:42:44 2014 (r273641) @@ -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: head/sys/cddl/contrib/opensolaris/uts/common/sys/gfs.h ============================================================================== --- head/sys/cddl/contrib/opensolaris/uts/common/sys/gfs.h Sat Oct 25 17:07:35 2014 (r273640) +++ head/sys/cddl/contrib/opensolaris/uts/common/sys/gfs.h Sat Oct 25 17:42:44 2014 (r273641) @@ -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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201410251742.s9PHgjwf029487>