tech-kern archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
VOP_*LOCK* cleanup proposal
The vnode lock operations still carry some arguments and semantics from
our old lockmgr(9). I propose to clean them up as:
1) VOP_LOCK(vp, flags): Limit the set of allowed flags to LK_EXCLUSIVE,
LK_SHARED and LK_NOWAIT. LK_INTERLOCK is no longer allowed as it
makes no sense here.
2) VOP_UNLOCK(vp, flags): Remove the flags argument. It must be zero
for some time now.
3) VOP_ISLOCKED(vp): Remove the for some time unused return value
LK_EXCLOTHER. Mark this operation as "diagnostic only". Making a
lock decision based on this operation is no longer allowed.
A diff covering 1) and 3) is attached. The result of the substitution
's/\(VOP_UNLOCK([^,]*\), 0/\1/' to kill the second argument of
VOP_UNLOCK() is omitted as it is completely mechanic.
Comments or objections anyone?
--
Juergen Hannken-Illjes - hannken%eis.cs.tu-bs.de@localhost - TU Braunschweig
(Germany)
Index: share/man/man9/vnodeops.9
===================================================================
RCS file: /cvsroot/src/share/man/man9/vnodeops.9,v
retrieving revision 1.78
diff -p -u -4 -r1.78 vnodeops.9
--- share/man/man9/vnodeops.9 19 May 2010 13:20:32 -0000 1.78
+++ share/man/man9/vnodeops.9 22 Jun 2010 08:08:29 -0000
@@ -26,9 +26,9 @@
.\" CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
.\" ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
.\" POSSIBILITY OF SUCH DAMAGE.
.\"
-.Dd May 19, 2010
+.Dd June 21, 2010
.Dt VNODEOPS 9
.Os
.Sh NAME
.Nm vnodeops ,
@@ -1062,11 +1062,18 @@ The argument
.Fa vp
is the vnode of the file to be locked.
The argument
.Fa flags
-is a set of
-.Xr lockmgr 9
-flags.
+is
+.Dv LK_EXCLUSIVE
+to take the lock exclusively or
+.Dv LK_SHARED
+to take a shared lock.
+If
+.Fa flags
+contains
+.Dv LK_NOWAIT
+and the lock is busy, the operation will return immediately with an error code.
If the operation is successful zero is returned, otherwise an
appropriate error code is returned.
.Fn VOP_LOCK
is used to serialize access to the file system such as to prevent two
@@ -1099,8 +1106,11 @@ Possible return values are
.Dv LK_EXCLUSIVE ,
.Dv LK_SHARED
or 0 for lock held exclusively by the calling thread, shared lock held
by anyone or unlocked, respectively.
+.Pp
+This function must never be used to make locking decisions at run time:
+it is provided only for diagnostic purposes.
.It Fn VOP_BMAP "vp" "bn" "vpp" "bnp" "runp"
Convert the logical block number
.Fa bn
of a file specified by vnode
Index: sys/coda/coda_vnops.c
===================================================================
RCS file: /cvsroot/src/sys/coda/coda_vnops.c,v
retrieving revision 1.71
diff -p -u -4 -r1.71 coda_vnops.c
--- sys/coda/coda_vnops.c 23 Nov 2009 02:13:44 -0000 1.71
+++ sys/coda/coda_vnops.c 22 Jun 2010 08:08:34 -0000
@@ -2017,8 +2017,9 @@ coda_getpages(void *v)
* figure out whether getpages ever is called holding the
* lock, and if we should serialize getpages calls by some
* mechanism.
*/
+ /* XXX VOP_ISLOCKED() may not be used for lock decisions. */
waslocked = VOP_ISLOCKED(vp);
/* Drop the vmobject lock. */
mutex_exit(&vp->v_uobj.vmobjlock);
Index: sys/fs/nilfs/nilfs_vnops.c
===================================================================
RCS file: /cvsroot/src/sys/fs/nilfs/nilfs_vnops.c,v
retrieving revision 1.4
diff -p -u -4 -r1.4 nilfs_vnops.c
--- sys/fs/nilfs/nilfs_vnops.c 8 Jan 2010 11:35:08 -0000 1.4
+++ sys/fs/nilfs/nilfs_vnops.c 22 Jun 2010 08:08:36 -0000
@@ -1160,16 +1160,24 @@ nilfs_do_link(struct vnode *dvp, struct
dir_node = VTOI(dvp);
nilfs_node = VTOI(vp);
error = VOP_GETATTR(vp, &vap, FSCRED);
- if (error)
+ if (error) {
+ VOP_UNLOCK(vp, 0);
return error;
+ }
/* check link count overflow */
- if (vap.va_nlink >= (1<<16)-1) /* uint16_t */
+ if (vap.va_nlink >= (1<<16)-1) { /* uint16_t */
+ VOP_UNLOCK(vp, 0);
return EMLINK;
+ }
- return nilfs_dir_attach(dir_node->ump, dir_node, nilfs_node, &vap, cnp);
+ error = nilfs_dir_attach(dir_node->ump, dir_node, nilfs_node,
+ &vap, cnp);
+ if (error)
+ VOP_UNLOCK(vp, 0);
+ return error;
}
int
nilfs_link(void *v)
@@ -1187,11 +1195,8 @@ nilfs_link(void *v)
error = nilfs_do_link(dvp, vp, cnp);
if (error)
VOP_ABORTOP(dvp, cnp);
- if ((vp != dvp) && (VOP_ISLOCKED(vp) == LK_EXCLUSIVE))
- VOP_UNLOCK(vp, 0);
-
VN_KNOTE(vp, NOTE_LINK);
VN_KNOTE(dvp, NOTE_WRITE);
vput(dvp);
Index: sys/fs/udf/udf_vnops.c
===================================================================
RCS file: /cvsroot/src/sys/fs/udf/udf_vnops.c,v
retrieving revision 1.57
diff -p -u -4 -r1.57 udf_vnops.c
--- sys/fs/udf/udf_vnops.c 8 Jan 2010 11:35:09 -0000 1.57
+++ sys/fs/udf/udf_vnops.c 22 Jun 2010 08:08:36 -0000
@@ -1508,16 +1508,23 @@ udf_do_link(struct vnode *dvp, struct vn
dir_node = VTOI(dvp);
udf_node = VTOI(vp);
error = VOP_GETATTR(vp, &vap, FSCRED);
- if (error)
+ if (error) {
+ VOP_UNLOCK(vp, 0);
return error;
+ }
/* check link count overflow */
- if (vap.va_nlink >= (1<<16)-1) /* uint16_t */
+ if (vap.va_nlink >= (1<<16)-1) { /* uint16_t */
+ VOP_UNLOCK(vp, 0);
return EMLINK;
+ }
- return udf_dir_attach(dir_node->ump, dir_node, udf_node, &vap, cnp);
+ error = udf_dir_attach(dir_node->ump, dir_node, udf_node, &vap, cnp);
+ if (error)
+ VOP_UNLOCK(vp, 0);
+ return error;
}
int
udf_link(void *v)
@@ -1535,11 +1542,8 @@ udf_link(void *v)
error = udf_do_link(dvp, vp, cnp);
if (error)
VOP_ABORTOP(dvp, cnp);
- if ((vp != dvp) && (VOP_ISLOCKED(vp) == LK_EXCLUSIVE))
- VOP_UNLOCK(vp, 0);
-
VN_KNOTE(vp, NOTE_LINK);
VN_KNOTE(dvp, NOTE_WRITE);
vput(dvp);
Index: sys/fs/unionfs/unionfs_subr.c
===================================================================
RCS file: /cvsroot/src/sys/fs/unionfs/unionfs_subr.c,v
retrieving revision 1.7
diff -p -u -4 -r1.7 unionfs_subr.c
--- sys/fs/unionfs/unionfs_subr.c 18 Jun 2010 16:29:01 -0000 1.7
+++ sys/fs/unionfs/unionfs_subr.c 22 Jun 2010 08:08:36 -0000
@@ -483,9 +483,9 @@ unionfs_node_update(struct unionfs_node
* lock update
*/
mutex_enter(&vp->v_interlock);
unp->un_uppervp = uvp;
- KASSERT(rw_write_held(&lvp->v_lock.vl_lock));
+ KASSERT(VOP_ISLOCKED(lvp) == LK_EXCLUSIVE);
mutex_exit(&vp->v_interlock);
}
/*
Index: sys/fs/unionfs/unionfs_vnops.c
===================================================================
RCS file: /cvsroot/src/sys/fs/unionfs/unionfs_vnops.c,v
retrieving revision 1.1
diff -p -u -4 -r1.1 unionfs_vnops.c
--- sys/fs/unionfs/unionfs_vnops.c 18 Feb 2008 16:44:22 -0000 1.1
+++ sys/fs/unionfs/unionfs_vnops.c 22 Jun 2010 08:08:37 -0000
@@ -470,24 +470,19 @@ static int
unionfs_close(void *v)
{
struct vop_close_args *ap = v;
int error;
- int locked;
struct unionfs_node *unp;
struct unionfs_node_status *unsp;
kauth_cred_t cred;
struct vnode *ovp;
UNIONFS_INTERNAL_DEBUG("unionfs_close: enter\n");
- locked = 0;
+ KASSERT(VOP_ISLOCKED(ap->a_vp) == LK_EXCLUSIVE);
unp = VTOUNIONFS(ap->a_vp);
cred = ap->a_cred;
- if (VOP_ISLOCKED(ap->a_vp) != LK_EXCLUSIVE) {
- vn_lock(ap->a_vp, LK_EXCLUSIVE | LK_RETRY);
- locked = 1;
- }
unionfs_get_node_status(unp, &unsp);
if (unsp->uns_lower_opencnt <= 0 && unsp->uns_upper_opencnt <= 0) {
#ifdef DIAGNOSTIC
@@ -521,11 +516,8 @@ unionfs_close(void *v)
unionfs_close_abort:
unionfs_tryrem_node_status(unp, unsp);
- if (locked != 0)
- VOP_UNLOCK(ap->a_vp, 0);
-
UNIONFS_INTERNAL_DEBUG("unionfs_close: leave (%d)\n", error);
return (error);
}
@@ -1537,13 +1529,8 @@ unionfs_lock(void *v)
uvp = unp->un_uppervp;
flags = ap->a_flags;
error = 0;
- if ((flags & LK_INTERLOCK) != 0) {
- mutex_exit(&ap->a_vp->v_interlock);
- flags &= ~LK_INTERLOCK;
- }
-
if (lvp != NULLVP) {
error = VOP_LOCK(lvp, flags);
}
if (error == 0 && uvp != NULLVP) {
Index: sys/kern/vfs_subr.c
===================================================================
RCS file: /cvsroot/src/sys/kern/vfs_subr.c,v
retrieving revision 1.405
diff -p -u -4 -r1.405 vfs_subr.c
--- sys/kern/vfs_subr.c 18 Jun 2010 16:29:02 -0000 1.405
+++ sys/kern/vfs_subr.c 22 Jun 2010 08:08:37 -0000
@@ -346,17 +346,10 @@ try_nextlist:
vpanic(vp, "list head mismatch");
}
if (!mutex_tryenter(&vp->v_interlock))
continue;
- /*
- * Our lwp might hold the underlying vnode
- * locked, so don't try to reclaim a VI_LAYER
- * node if it's locked.
- */
- if ((vp->v_iflag & VI_XLOCK) == 0 &&
- ((vp->v_iflag & VI_LAYER) == 0 || VOP_ISLOCKED(vp) == 0)) {
+ if ((vp->v_iflag & VI_XLOCK) == 0)
break;
- }
mutex_exit(&vp->v_interlock);
}
if (vp == NULL) {
@@ -1900,9 +1893,10 @@ vclean(vnode_t *vp, int flags)
vp->v_iflag &= ~(VI_TEXT|VI_EXECMAP);
active = (vp->v_usecount > 1);
/* XXXAD should not lock vnode under layer */
- VOP_LOCK(vp, LK_EXCLUSIVE | LK_INTERLOCK);
+ mutex_exit(&vp->v_interlock);
+ VOP_LOCK(vp, LK_EXCLUSIVE);
/*
* Clean out any cached data associated with the vnode.
* If purging an active vnode, it must be closed and
Index: sys/miscfs/genfs/genfs_vnops.c
===================================================================
RCS file: /cvsroot/src/sys/miscfs/genfs/genfs_vnops.c,v
retrieving revision 1.178
diff -p -u -4 -r1.178 genfs_vnops.c
--- sys/miscfs/genfs/genfs_vnops.c 6 Jun 2010 08:01:31 -0000 1.178
+++ sys/miscfs/genfs/genfs_vnops.c 22 Jun 2010 08:08:37 -0000
@@ -288,13 +288,8 @@ genfs_lock(void *v)
} */ *ap = v;
struct vnode *vp = ap->a_vp;
int flags = ap->a_flags;
- if ((flags & LK_INTERLOCK) != 0) {
- flags &= ~LK_INTERLOCK;
- mutex_exit(&vp->v_interlock);
- }
-
return (vlockmgr(&vp->v_lock, flags));
}
/*
Index: sys/nfs/nfs_vfsops.c
===================================================================
RCS file: /cvsroot/src/sys/nfs/nfs_vfsops.c,v
retrieving revision 1.212
diff -p -u -4 -r1.212 nfs_vfsops.c
--- sys/nfs/nfs_vfsops.c 15 May 2010 20:24:57 -0000 1.212
+++ sys/nfs/nfs_vfsops.c 22 Jun 2010 08:08:38 -0000
@@ -964,16 +964,21 @@ loop:
if (vp->v_mount != mp || vismarker(vp))
continue;
mutex_enter(&vp->v_interlock);
/* XXX MNT_LAZY cannot be right? */
- if (waitfor == MNT_LAZY || VOP_ISLOCKED(vp) ||
+ if (waitfor == MNT_LAZY ||
(LIST_EMPTY(&vp->v_dirtyblkhd) &&
UVM_OBJ_IS_CLEAN(&vp->v_uobj))) {
mutex_exit(&vp->v_interlock);
continue;
}
mutex_exit(&mntvnode_lock);
- if (vget(vp, LK_EXCLUSIVE | LK_INTERLOCK)) {
+ error = vget(vp, LK_EXCLUSIVE | LK_NOWAIT | LK_INTERLOCK);
+ if (error != 0) {
+ if (error != ENOENT) {
+ mutex_enter(&mntvnode_lock);
+ continue;
+ }
(void)vunmark(mvp);
goto loop;
}
error = VOP_FSYNC(vp, cred,
Index: sys/sys/lock.h
===================================================================
RCS file: /cvsroot/src/sys/sys/lock.h,v
retrieving revision 1.84
diff -p -u -4 -r1.84 lock.h
--- sys/sys/lock.h 18 Jun 2010 16:29:02 -0000 1.84
+++ sys/sys/lock.h 22 Jun 2010 08:08:38 -0000
@@ -76,9 +76,8 @@
#define LK_TYPE_MASK 0x0000000f /* type of lock sought */
#define LK_SHARED 0x00000001 /* shared lock */
#define LK_EXCLUSIVE 0x00000002 /* exclusive lock */
#define LK_RELEASE 0x00000006 /* release any type of lock */
-#define LK_EXCLOTHER 0x00000008 /* other process holds lock */
#define LK_NOWAIT 0x00000010 /* do not sleep to await lock */
#define LK_INTERLOCK 0x00010000 /* unlock passed simple lock
after
getting lk_interlock */
Index: sys/ufs/lfs/lfs_subr.c
===================================================================
RCS file: /cvsroot/src/sys/ufs/lfs/lfs_subr.c,v
retrieving revision 1.74
diff -p -u -4 -r1.74 lfs_subr.c
--- sys/ufs/lfs/lfs_subr.c 16 Feb 2010 23:20:30 -0000 1.74
+++ sys/ufs/lfs/lfs_subr.c 22 Jun 2010 08:08:38 -0000
@@ -360,19 +360,19 @@ lfs_unmark_dirop(struct lfs *fs)
for (ip = TAILQ_FIRST(&fs->lfs_dchainhd); ip != NULL; ip = nip) {
nip = TAILQ_NEXT(ip, i_lfs_dchain);
vp = ITOV(ip);
- if (VOP_ISLOCKED(vp) == LK_EXCLOTHER)
- continue;
if ((VTOI(vp)->i_flag & (IN_ADIROP | IN_ALLMOD)) == 0) {
+ if (vn_lock(vp, LK_EXCLUSIVE | LK_NOWAIT | LK_RETRY))
+ continue;
--lfs_dirvcount;
--fs->lfs_dirvcount;
vp->v_uflag &= ~VU_DIROP;
TAILQ_REMOVE(&fs->lfs_dchainhd, ip, i_lfs_dchain);
wakeup(&lfs_dirvcount);
fs->lfs_unlockvp = vp;
mutex_exit(&lfs_lock);
- vrele(vp);
+ vput(vp);
mutex_enter(&lfs_lock);
fs->lfs_unlockvp = NULL;
}
}
Index: sys/ufs/lfs/lfs_vnops.c
===================================================================
RCS file: /cvsroot/src/sys/ufs/lfs/lfs_vnops.c,v
retrieving revision 1.227
diff -p -u -4 -r1.227 lfs_vnops.c
--- sys/ufs/lfs/lfs_vnops.c 29 Mar 2010 13:11:34 -0000 1.227
+++ sys/ufs/lfs/lfs_vnops.c 22 Jun 2010 08:08:38 -0000
@@ -1226,9 +1226,8 @@ lfs_flush_dirops(struct lfs *fs)
struct inode *ip, *nip;
struct vnode *vp;
extern int lfs_dostats;
struct segment *sp;
- int waslocked;
ASSERT_MAYBE_SEGLOCK(fs);
KASSERT(fs->lfs_nadirop == 0);
@@ -1288,9 +1287,11 @@ lfs_flush_dirops(struct lfs *fs)
if (vp->v_iflag & VI_XLOCK) {
mutex_enter(&lfs_lock);
continue;
}
- waslocked = VOP_ISLOCKED(vp);
+ /* XXX see below
+ * waslocked = VOP_ISLOCKED(vp);
+ */
if (vp->v_type != VREG &&
((ip->i_flag & IN_ALLMOD) || !VPISEMPTY(vp))) {
lfs_writefile(fs, sp, vp);
if (!VPISEMPTY(vp) && !WRITEINPROG(vp) &&
@@ -1302,10 +1303,14 @@ lfs_flush_dirops(struct lfs *fs)
}
KDASSERT(ip->i_number != LFS_IFILE_INUM);
(void) lfs_writeinode(fs, sp, ip);
mutex_enter(&lfs_lock);
- if (waslocked == LK_EXCLOTHER)
- LFS_SET_UINO(ip, IN_MODIFIED);
+ /*
+ * XXX
+ * LK_EXCLOTHER is dead -- what is intended here?
+ * if (waslocked == LK_EXCLOTHER)
+ * LFS_SET_UINO(ip, IN_MODIFIED);
+ */
}
mutex_exit(&lfs_lock);
/* We've written all the dirops there are */
((SEGSUM *)(sp->segsum))->ss_flags &= ~(SS_CONT);
@@ -1382,9 +1387,9 @@ lfs_flush_pchain(struct lfs *fs)
if (lfs_vref(vp))
continue;
mutex_exit(&lfs_lock);
- if (VOP_ISLOCKED(vp)) {
+ if (vn_lock(vp, LK_EXCLUSIVE | LK_NOWAIT | LK_RETRY) != 0) {
lfs_vunref(vp);
mutex_enter(&lfs_lock);
continue;
}
@@ -1398,8 +1403,9 @@ lfs_flush_pchain(struct lfs *fs)
}
KDASSERT(ip->i_number != LFS_IFILE_INUM);
(void) lfs_writeinode(fs, sp, ip);
+ VOP_UNLOCK(vp, 0);
lfs_vunref(vp);
if (error == EAGAIN) {
lfs_writeseg(fs, sp);
@@ -2207,8 +2213,9 @@ lfs_putpages(void *v)
(vp->v_uflag & VU_DIROP)) {
int locked;
DLOG((DLOG_PAGE, "lfs_putpages: flushing VU_DIROP\n"));
+ /* XXX VOP_ISLOCKED() may not be used for lock decisions. */
locked = (VOP_ISLOCKED(vp) == LK_EXCLUSIVE);
mutex_exit(&vp->v_interlock);
lfs_writer_enter(fs, "ppdirop");
if (locked)
@@ -2217,13 +2224,11 @@ lfs_putpages(void *v)
mutex_enter(&lfs_lock);
lfs_flush_fs(fs, sync ? SEGM_SYNC : 0);
mutex_exit(&lfs_lock);
+ if (locked)
+ VOP_LOCK(vp, LK_EXCLUSIVE);
mutex_enter(&vp->v_interlock);
- if (locked) {
- VOP_LOCK(vp, LK_EXCLUSIVE | LK_INTERLOCK);
- mutex_enter(&vp->v_interlock);
- }
lfs_writer_leave(fs);
/* XXX the flush should have taken care of this one too! */
}
Home |
Main Index |
Thread Index |
Old Index