From: Sebastien Marie <semarie@kapouay.eu.org>
Subject: Re: patch: vfs cleanup - VOP_REMOVE
To: tech@openbsd.org
Date: Mon, 13 May 2024 08:05:48 +0200

Hi,

New interation on the VOP_REMOVE cleanup.

the following diff only move vnode unlocking and ref dropping to
FS-indep part.

while here, it ensures all vop_remove field are setted, and always call
the function.

the change is very conservative: it only adds vnode ref drop/unlock
where it was absent because it should be unreachable (and if it wasn't,
it should fix things).

I let the VN_KNOTE() part outside of this diff for now, as it could lead
to behaviour change. it will be addressed in a separated diff (which
could be more easily reverted).

Comments or OK ?
-- 
Sebastien Marie

diff b30de97d4acfcd79cc3c5aa8ebe33237055e4d04 refs/heads/vfs2
commit - b30de97d4acfcd79cc3c5aa8ebe33237055e4d04
commit + 32bd54f0b6349b702168bcd6fdbcbb1be5d8d085
blob - 244c781a0dd00eb4ce8152af5e97efb759e09d5e
blob + c1b80006058ee204726390881acaf4fb0e87402e
--- sys/isofs/udf/udf_vnops.c
+++ sys/isofs/udf/udf_vnops.c
@@ -84,7 +84,7 @@ const struct vops udf_vops = {
 	.vop_fsync	= NULL,
 	.vop_link	= NULL,
 	.vop_mknod	= NULL,
-	.vop_remove	= NULL,
+	.vop_remove	= eopnotsupp,
 	.vop_rename	= NULL,
 	.vop_revoke	= NULL,
 	.vop_mkdir	= NULL,
blob - 4412725a90c89e63443f543df1ef3c3bb64228d9
blob + 8cd1484d17850691bc342bfaa867c44b7d787b03
--- sys/kern/vfs_sync.c
+++ sys/kern/vfs_sync.c
@@ -238,7 +238,7 @@ const struct vops sync_vops = {
 	.vop_read	= NULL,
 	.vop_readdir	= NULL,
 	.vop_readlink	= NULL,
-	.vop_remove	= NULL,
+	.vop_remove	= eopnotsupp,
 	.vop_rename	= NULL,
 	.vop_revoke	= NULL,
 	.vop_mkdir	= NULL,
blob - 3d08b2ec5ca669aedbc2c97e592a4c50af5498bb
blob + 7c9c59b04bdc735b6abc1b9580812c9b6b28c100
--- sys/kern/vfs_vops.c
+++ sys/kern/vfs_vops.c
@@ -319,6 +319,7 @@ VOP_FSYNC(struct vnode *vp, struct ucred *cred, int wa
 int
 VOP_REMOVE(struct vnode *dvp, struct vnode *vp, struct componentname *cnp)
 {
+	int error;
 	struct vop_remove_args a;
 	a.a_dvp = dvp;
         a.a_vp = vp;
@@ -327,10 +328,15 @@ VOP_REMOVE(struct vnode *dvp, struct vnode *vp, struct
 	ASSERT_VP_ISLOCKED(dvp);
 	ASSERT_VP_ISLOCKED(vp);
 
-	if (dvp->v_op->vop_remove == NULL)
-		return (EOPNOTSUPP);
+	error = dvp->v_op->vop_remove(&a);
 
-	return ((dvp->v_op->vop_remove)(&a));
+	if (dvp == vp)
+		vrele(vp);
+	else
+		vput(vp);
+	vput(dvp);
+
+	return error;
 }
 
 int
blob - 173d70562f64dc55d3ab479d9f536316fd1b7dd9
blob + 71634d26d1751115cd7999f41db81a005cf6a0f3
--- sys/miscfs/fuse/fuse_vnops.c
+++ sys/miscfs/fuse/fuse_vnops.c
@@ -1512,11 +1512,6 @@ fusefs_remove(void *v)
 	fb_delete(fbuf);
 out:
 	pool_put(&namei_pool, cnp->cn_pnbuf);
-	if (dvp == vp)
-		vrele(vp);
-	else
-		vput(vp);
-	vput(dvp);
 	return (error);
 }
 
blob - 12d22a95ce0e71f74a7425580296492b4a705d47
blob + 8ab2d25c81ddf85f5868d121a11c5ca18df05da8
--- sys/msdosfs/msdosfs_vnops.c
+++ sys/msdosfs/msdosfs_vnops.c
@@ -810,12 +810,7 @@ msdosfs_remove(void *v)
 	printf("msdosfs_remove(), dep %p, v_usecount %d\n", dep,
 	    ap->a_vp->v_usecount);
 #endif
-	if (ddep == dep)
-		vrele(ap->a_vp);
-	else
-		vput(ap->a_vp);	/* causes msdosfs_inactive() to be called
-				 * via vrele() */
-	vput(ap->a_dvp);
+
 	return (error);
 }
 
blob - 7bb05c1f6333d819ea2846bb5054ccd3bec30589
blob + bacefd79af8a56f4a4ed018dafaa1ba13bd888d8
--- sys/nfs/nfs_vnops.c
+++ sys/nfs/nfs_vnops.c
@@ -1725,11 +1725,6 @@ nfs_remove(void *v)
 	NFS_INVALIDATE_ATTRCACHE(np);
 	VN_KNOTE(vp, NOTE_DELETE);
 	VN_KNOTE(dvp, NOTE_WRITE);
-	if (vp == dvp)
-		vrele(vp);
-	else
-		vput(vp);
-	vput(dvp);
 	return (error);
 }
 
blob - af3a9898af7aa7695be87a604ec35ba8cea24eda
blob + 04b741f9854a6905314c6d5badcdb2efac3aa5eb
--- sys/ntfs/ntfs_vnops.c
+++ sys/ntfs/ntfs_vnops.c
@@ -689,7 +689,7 @@ const struct vops ntfs_vops = {
 	.vop_link	= NULL,
 	.vop_mknod	= NULL,
 	.vop_readlink	= NULL,
-	.vop_remove	= NULL,
+	.vop_remove	= eopnotsupp,
 	.vop_rename	= NULL,
 	.vop_revoke	= NULL,
 	.vop_mkdir	= NULL,
blob - b1c68fdebba338a23f6ed36e305b6c5df930a294
blob + ae825d9606777c71d9b95ac7619e11389a2423b1
--- sys/tmpfs/tmpfs_vnops.c
+++ sys/tmpfs/tmpfs_vnops.c
@@ -692,8 +692,6 @@ tmpfs_remove(void *v)
 	tmpfs_dirent_t *de;
 	int error;
 
-	KASSERT(VOP_ISLOCKED(dvp));
-	KASSERT(VOP_ISLOCKED(vp));
 	KASSERT(cnp->cn_flags & HASBUF);
 
 	if (vp->v_type == VDIR) {
@@ -742,13 +740,6 @@ tmpfs_remove(void *v)
 	error = 0;
 out:
 	pool_put(&namei_pool, cnp->cn_pnbuf);
-	/* Drop the references and unlock the vnodes. */
-	vput(vp);
-	if (dvp == vp) {
-		vrele(dvp);
-	} else {
-		vput(dvp);
-	}
 	return error;
 }
 
blob - 0097f2380c5cfd9c10bed12e4e3a06b9272022a6
blob + f25607dd206957ea6523be7694d6a44e8a4705e2
--- sys/ufs/ext2fs/ext2fs_vnops.c
+++ sys/ufs/ext2fs/ext2fs_vnops.c
@@ -410,11 +410,6 @@ ext2fs_remove(void *v)
 		ip->i_flag |= IN_CHANGE;
 	}
 out:
-	if (dvp == vp)
-		vrele(vp);
-	else
-		vput(vp);
-	vput(dvp);
 	return (error);
 }
 
blob - d329284990703d35b484e26d2d59af1a6ac9df73
blob + ce54a340bcbc345c9b2c37d654846209a8f62afe
--- sys/ufs/ufs/ufs_vnops.c
+++ sys/ufs/ufs/ufs_vnops.c
@@ -595,11 +595,6 @@ ufs_remove(void *v)
 	VN_KNOTE(vp, NOTE_DELETE);
 	VN_KNOTE(dvp, NOTE_WRITE);
  out:
-	if (dvp == vp)
-		vrele(vp);
-	else
-		vput(vp);
-	vput(dvp);
 	return (error);
 }