Date: Wed, 14 Mar 2012 09:15:51 +0000 (UTC) From: Gleb Kurtsou <gleb@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r232960 - head/sys/fs/tmpfs Message-ID: <201203140915.q2E9Fpbq090090@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: gleb Date: Wed Mar 14 09:15:50 2012 New Revision: 232960 URL: http://svn.freebsd.org/changeset/base/232960 Log: Prevent tmpfs_rename() deadlock in a way similar to UFS Unlock vnodes and try to lock them one by one. Relookup fvp and tvp. Approved by: mdf (mentor) Modified: head/sys/fs/tmpfs/tmpfs_subr.c head/sys/fs/tmpfs/tmpfs_vnops.c Modified: head/sys/fs/tmpfs/tmpfs_subr.c ============================================================================== --- head/sys/fs/tmpfs/tmpfs_subr.c Wed Mar 14 08:29:21 2012 (r232959) +++ head/sys/fs/tmpfs/tmpfs_subr.c Wed Mar 14 09:15:50 2012 (r232960) @@ -42,6 +42,7 @@ __FBSDID("$FreeBSD$"); #include <sys/proc.h> #include <sys/stat.h> #include <sys/systm.h> +#include <sys/sysctl.h> #include <sys/vnode.h> #include <sys/vmmeter.h> @@ -56,6 +57,8 @@ __FBSDID("$FreeBSD$"); #include <fs/tmpfs/tmpfs_fifoops.h> #include <fs/tmpfs/tmpfs_vnops.h> +SYSCTL_NODE(_vfs, OID_AUTO, tmpfs, CTLFLAG_RW, 0, "tmpfs file system"); + /* --------------------------------------------------------------------- */ /* Modified: head/sys/fs/tmpfs/tmpfs_vnops.c ============================================================================== --- head/sys/fs/tmpfs/tmpfs_vnops.c Wed Mar 14 08:29:21 2012 (r232959) +++ head/sys/fs/tmpfs/tmpfs_vnops.c Wed Mar 14 09:15:50 2012 (r232960) @@ -46,6 +46,7 @@ __FBSDID("$FreeBSD$"); #include <sys/sf_buf.h> #include <sys/stat.h> #include <sys/systm.h> +#include <sys/sysctl.h> #include <sys/unistd.h> #include <sys/vnode.h> @@ -57,6 +58,13 @@ __FBSDID("$FreeBSD$"); #include <fs/tmpfs/tmpfs_vnops.h> #include <fs/tmpfs/tmpfs.h> +SYSCTL_DECL(_vfs_tmpfs); + +static volatile int tmpfs_rename_restarts; +SYSCTL_INT(_vfs_tmpfs, OID_AUTO, rename_restarts, CTLFLAG_RD, + __DEVOLATILE(int *, &tmpfs_rename_restarts), 0, + "Times rename had to restart due to lock contention"); + /* --------------------------------------------------------------------- */ static int @@ -918,6 +926,139 @@ out: /* --------------------------------------------------------------------- */ +/* + * We acquire all but fdvp locks using non-blocking acquisitions. If we + * fail to acquire any lock in the path we will drop all held locks, + * acquire the new lock in a blocking fashion, and then release it and + * restart the rename. This acquire/release step ensures that we do not + * spin on a lock waiting for release. On error release all vnode locks + * and decrement references the way tmpfs_rename() would do. + */ +static int +tmpfs_rename_relock(struct vnode *fdvp, struct vnode **fvpp, + struct vnode *tdvp, struct vnode **tvpp, + struct componentname *fcnp, struct componentname *tcnp) +{ + struct vnode *nvp; + struct mount *mp; + struct tmpfs_dirent *de; + int error, restarts = 0; + + VOP_UNLOCK(tdvp, 0); + if (*tvpp != NULL && *tvpp != tdvp) + VOP_UNLOCK(*tvpp, 0); + mp = fdvp->v_mount; + +relock: + restarts += 1; + error = vn_lock(fdvp, LK_EXCLUSIVE); + if (error) + goto releout; + if (vn_lock(tdvp, LK_EXCLUSIVE | LK_NOWAIT) != 0) { + VOP_UNLOCK(fdvp, 0); + error = vn_lock(tdvp, LK_EXCLUSIVE); + if (error) + goto releout; + VOP_UNLOCK(tdvp, 0); + goto relock; + } + /* + * Re-resolve fvp to be certain it still exists and fetch the + * correct vnode. + */ + de = tmpfs_dir_lookup(VP_TO_TMPFS_DIR(fdvp), NULL, fcnp); + if (de == NULL) { + VOP_UNLOCK(fdvp, 0); + VOP_UNLOCK(tdvp, 0); + if ((fcnp->cn_flags & ISDOTDOT) != 0 || + (fcnp->cn_namelen == 1 && fcnp->cn_nameptr[0] == '.')) + error = EINVAL; + else + error = ENOENT; + goto releout; + } + error = tmpfs_alloc_vp(mp, de->td_node, LK_EXCLUSIVE | LK_NOWAIT, &nvp); + if (error != 0) { + VOP_UNLOCK(fdvp, 0); + VOP_UNLOCK(tdvp, 0); + if (error != EBUSY) + goto releout; + error = tmpfs_alloc_vp(mp, de->td_node, LK_EXCLUSIVE, &nvp); + if (error != 0) + goto releout; + VOP_UNLOCK(nvp, 0); + /* + * Concurrent rename race. + */ + if (nvp == tdvp) { + vrele(nvp); + error = EINVAL; + goto releout; + } + vrele(*fvpp); + *fvpp = nvp; + goto relock; + } + vrele(*fvpp); + *fvpp = nvp; + VOP_UNLOCK(*fvpp, 0); + /* + * Re-resolve tvp and acquire the vnode lock if present. + */ + de = tmpfs_dir_lookup(VP_TO_TMPFS_DIR(tdvp), NULL, tcnp); + /* + * If tvp disappeared we just carry on. + */ + if (de == NULL && *tvpp != NULL) { + vrele(*tvpp); + *tvpp = NULL; + } + /* + * Get the tvp ino if the lookup succeeded. We may have to restart + * if the non-blocking acquire fails. + */ + if (de != NULL) { + nvp = NULL; + error = tmpfs_alloc_vp(mp, de->td_node, + LK_EXCLUSIVE | LK_NOWAIT, &nvp); + if (*tvpp != NULL) + vrele(*tvpp); + *tvpp = nvp; + if (error != 0) { + VOP_UNLOCK(fdvp, 0); + VOP_UNLOCK(tdvp, 0); + if (error != EBUSY) + goto releout; + error = tmpfs_alloc_vp(mp, de->td_node, LK_EXCLUSIVE, + &nvp); + if (error != 0) + goto releout; + VOP_UNLOCK(nvp, 0); + /* + * fdvp contains fvp, thus tvp (=fdvp) is not empty. + */ + if (nvp == fdvp) { + error = ENOTEMPTY; + goto releout; + } + goto relock; + } + } + tmpfs_rename_restarts += restarts; + + return (0); + +releout: + vrele(fdvp); + vrele(*fvpp); + vrele(tdvp); + if (*tvpp != NULL) + vrele(*tvpp); + tmpfs_rename_restarts += restarts; + + return (error); +} + static int tmpfs_rename(struct vop_rename_args *v) { @@ -927,6 +1068,7 @@ tmpfs_rename(struct vop_rename_args *v) struct vnode *tdvp = v->a_tdvp; struct vnode *tvp = v->a_tvp; struct componentname *tcnp = v->a_tcnp; + struct mount *mp = NULL; char *newname; int error; @@ -942,8 +1084,6 @@ tmpfs_rename(struct vop_rename_args *v) MPASS(fcnp->cn_flags & HASBUF); MPASS(tcnp->cn_flags & HASBUF); - tnode = (tvp == NULL) ? NULL : VP_TO_TMPFS_NODE(tvp); - /* Disallow cross-device renames. * XXX Why isn't this done by the caller? */ if (fvp->v_mount != tdvp->v_mount || @@ -952,9 +1092,6 @@ tmpfs_rename(struct vop_rename_args *v) goto out; } - tmp = VFS_TO_TMPFS(tdvp->v_mount); - tdnode = VP_TO_TMPFS_DIR(tdvp); - /* If source and target are the same file, there is nothing to do. */ if (fvp == tvp) { error = 0; @@ -963,8 +1100,37 @@ tmpfs_rename(struct vop_rename_args *v) /* If we need to move the directory between entries, lock the * source so that we can safely operate on it. */ - if (fdvp != tdvp && fdvp != tvp) - vn_lock(fdvp, LK_EXCLUSIVE | LK_RETRY); + if (fdvp != tdvp && fdvp != tvp) { + if (vn_lock(fdvp, LK_EXCLUSIVE | LK_NOWAIT) != 0) { + mp = tdvp->v_mount; + error = vfs_busy(mp, 0); + if (error != 0) { + mp = NULL; + goto out; + } + error = tmpfs_rename_relock(fdvp, &fvp, tdvp, &tvp, + fcnp, tcnp); + if (error != 0) { + vfs_unbusy(mp); + return (error); + } + ASSERT_VOP_ELOCKED(fdvp, + "tmpfs_rename: fdvp not locked"); + ASSERT_VOP_ELOCKED(tdvp, + "tmpfs_rename: tdvp not locked"); + if (tvp != NULL) + ASSERT_VOP_ELOCKED(tvp, + "tmpfs_rename: tvp not locked"); + if (fvp == tvp) { + error = 0; + goto out_locked; + } + } + } + + tmp = VFS_TO_TMPFS(tdvp->v_mount); + tdnode = VP_TO_TMPFS_DIR(tdvp); + tnode = (tvp == NULL) ? NULL : VP_TO_TMPFS_NODE(tvp); fdnode = VP_TO_TMPFS_DIR(fdvp); fnode = VP_TO_TMPFS_NODE(fvp); de = tmpfs_dir_lookup(fdnode, fnode, fcnp); @@ -1158,6 +1324,9 @@ out: vrele(fdvp); vrele(fvp); + if (mp != NULL) + vfs_unbusy(mp); + return error; }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201203140915.q2E9Fpbq090090>