Hi,
The following diff removes VLOCKSWORK flag.
This flag is currently used to mark or unmark a vnode to actively
check vnode locking semantic (when compiled with VFSLCKDEBUG).
Currently, VLOCKSWORK flag isn't properly set for several FS
implementation which have full locking support, specially:
- cd9660
- udf
- fuse
- msdosfs
- tmpfs
Instead of using a particular flag, I propose to directly check if
v_op->vop_islocked is nullop or not to activate or not the vnode
locking checks.
Some alternate methods might be possible, like having a specific
member inside struct vops. But it will only duplicate the fact that
nullop is used as lock mecanism.
I also slightly changed ASSERT_VP_ISLOCKED(vp) macro:
- evaluate vp argument only once
- explicitly check if VOP_ISLOCKED() != LK_EXCLUSIVE (it might returns
error or 'locked by some else', and it doesn't mean "locked by me")
- show the VOP_ISLOCKED returned code in panic message
Some code are using ASSERT_VP_ISLOCKED() like code. I kept them simple.
The direct impact on snapshots should be low as VFSLCKDEBUG isn't set
by default.
Comments or OK ?
--
Sebastien Marie
diff e44725a8dd99f82f94f37ecff5c0e710c4dba97e
/home/semarie/repos/openbsd/sys-clean
blob - c752dd99e9ef62b05162cfeda67913ab5bccf06e
file + kern/vfs_subr.c
--- kern/vfs_subr.c
+++ kern/vfs_subr.c
@@ -1075,9 +1075,6 @@ vclean(struct vnode *vp, int flags, struct proc *p)
vp->v_op = &dead_vops;
VN_KNOTE(vp, NOTE_REVOKE);
vp->v_tag = VT_NON;
-#ifdef VFSLCKDEBUG
- vp->v_flag &= ~VLOCKSWORK;
-#endif
mtx_enter(&vnode_mtx);
vp->v_lflag &= ~VXLOCK;
if (vp->v_lflag & VXWANT) {
@@ -1930,7 +1927,7 @@ vinvalbuf(struct vnode *vp, int flags, struct ucred *c
int s, error;
#ifdef VFSLCKDEBUG
- if ((vp->v_flag & VLOCKSWORK) && !VOP_ISLOCKED(vp))
+ if ((vp->v_op->vop_islocked != nullop) && !VOP_ISLOCKED(vp))
panic("%s: vp isn't locked, vp %p", __func__, vp);
#endif
blob - caf2dc327bfc2f5a001bcee80edd90938497ef99
file + kern/vfs_vops.c
--- kern/vfs_vops.c
+++ kern/vfs_vops.c
@@ -48,11 +48,15 @@
#include <sys/systm.h>
#ifdef VFSLCKDEBUG
-#define ASSERT_VP_ISLOCKED(vp) do { \
- if (((vp)->v_flag & VLOCKSWORK) && !VOP_ISLOCKED(vp)) { \
- VOP_PRINT(vp); \
- panic("vp not locked"); \
- } \
+#define ASSERT_VP_ISLOCKED(vp) do { \
+ struct vnode *_vp = (vp); \
+ int r; \
+ if (_vp->v_op->vop_islocked == nullop) \
+ break; \
+ if ((r = VOP_ISLOCKED(_vp)) != LK_EXCLUSIVE) { \
+ VOP_PRINT(_vp); \
+ panic("%s: vp not locked, vp %p, %d", __func__, _vp, r);\
+ } \
} while (0)
#else
#define ASSERT_VP_ISLOCKED(vp) /* nothing */
blob - 81b900e83d2071d8450f35cfae42c6cb91f1a414
file + nfs/nfs_node.c
--- nfs/nfs_node.c
+++ nfs/nfs_node.c
@@ -133,9 +133,6 @@ loop:
}
vp = nvp;
-#ifdef VFSLCKDEBUG
- vp->v_flag |= VLOCKSWORK;
-#endif
rrw_init_flags(&np->n_lock, "nfsnode", RWL_DUPOK | RWL_IS_VNODE);
vp->v_data = np;
/* we now have an nfsnode on this vnode */
blob - 3668f954a9aab3fd49ed5e41e7d4ab51b4bf0a90
file + sys/vnode.h
--- sys/vnode.h
+++ sys/vnode.h
@@ -146,8 +146,7 @@ struct vnode {
#define VCLONED 0x0400 /* vnode was cloned */
#define VALIASED 0x0800 /* vnode has an alias */
#define VLARVAL 0x1000 /* vnode data not yet set up by higher
level */
-#define VLOCKSWORK 0x4000 /* FS supports locking discipline */
-#define VCLONE 0x8000 /* vnode is a clone */
+#define VCLONE 0x4000 /* vnode is a clone */
/*
* (v_bioflag) Flags that may be manipulated by interrupt handlers
blob - d859d216b40ebb2f5cce1eb5cf0becbfff21a638
file + ufs/ext2fs/ext2fs_subr.c
--- ufs/ext2fs/ext2fs_subr.c
+++ ufs/ext2fs/ext2fs_subr.c
@@ -170,9 +170,6 @@ ext2fs_vinit(struct mount *mp, struct vnode **vpp)
nvp->v_data = vp->v_data;
vp->v_data = NULL;
vp->v_op = &spec_vops;
-#ifdef VFSLCKDEBUG
- vp->v_flag &= ~VLOCKSWORK;
-#endif
vrele(vp);
vgone(vp);
/* Reinitialize aliased vnode. */
blob - 2aedef06acfdc500a4019c3f75a986b648c5b36a
file + ufs/ffs/ffs_subr.c
--- ufs/ffs/ffs_subr.c
+++ ufs/ffs/ffs_subr.c
@@ -272,9 +272,6 @@ ffs_vinit(struct mount *mntp, struct vnode **vpp)
nvp->v_data = vp->v_data;
vp->v_data = NULL;
vp->v_op = &spec_vops;
-#ifdef VFSLCKDEBUG
- vp->v_flag &= ~VLOCKSWORK;
-#endif
vrele(vp);
vgone(vp);
/*
blob - 48303e6a811c99141707d1b1459f49fd06c2aa55
file + ufs/ffs/ffs_vfsops.c
--- ufs/ffs/ffs_vfsops.c
+++ ufs/ffs/ffs_vfsops.c
@@ -1324,9 +1324,6 @@ retry:
return (error);
}
-#ifdef VFSLCKDEBUG
- vp->v_flag |= VLOCKSWORK;
-#endif
ip = pool_get(&ffs_ino_pool, PR_WAITOK|PR_ZERO);
rrw_init_flags(&ip->i_lock, "inode", RWL_DUPOK | RWL_IS_VNODE);
ip->i_ump = ump;
blob - 5fc969c3ff77f79f429c6ad6815e3492bca4b26a
file + uvm/uvm_vnode.c
--- uvm/uvm_vnode.c
+++ uvm/uvm_vnode.c
@@ -1328,7 +1328,7 @@ uvm_vnp_uncache(struct vnode *vp)
* carry over sanity check from old vnode pager: the vnode should
* be VOP_LOCK'd, and we confirm it here.
*/
- if ((vp->v_flag & VLOCKSWORK) && !VOP_ISLOCKED(vp))
+ if ((vp->v_op->vop_islocked != nullop) && !VOP_ISLOCKED(vp))
panic("uvm_vnp_uncache: vnode not locked!");
#endif
