On 15/10/21(Fri) 09:27, Sebastien Marie wrote:
> Hi,
>
> The following diff removes VLOCKSWORK flag.
Nice.
> 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.
I wonder if we shouldn't get rid of those checks and instead make
VOP_ISLOCKED() deal with that.
VOP_ISLOCKED() is inconsistent. It returns the value of rrw_status(9)
or EOPNOTSUPP if `vop_islocked' is NULL.
But this is a change in behavior that has a broader scope, so it should
be done separately.
> 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 ?
ok mpi@
> 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
>
>
