From: Sebastien Marie Subject: patch: vfs cleanup - VOP_REMOVE To: tech@openbsd.org Date: Sat, 04 May 2024 11:50:39 +0200 Hi, I am removing some dust on diffs on vfs layer cleanup. My purpose is to properly review the vfs layer and regroup invariants from filesystem-depend parts to vfs part. Here a first diff with VOP_REMOVE(). If people are interested I will do more and do others VOP_* functions. I will comment in the diff, but it should be still appliable (I am commenting between chunks). diff /home/semarie/repos/openbsd/src commit - c4219eb26d9e13b9666f61cd0b7d1c2b001fde6b path + /home/semarie/repos/openbsd/src blob - 3d08b2ec5ca669aedbc2c97e592a4c50af5498bb file + sys/kern/vfs_vops.c --- 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; @@ -328,9 +329,20 @@ VOP_REMOVE(struct vnode *dvp, struct vnode *vp, struct ASSERT_VP_ISLOCKED(vp); if (dvp->v_op->vop_remove == NULL) - return (EOPNOTSUPP); + error = EOPNOTSUPP; + else + error = dvp->v_op->vop_remove(&a); - return ((dvp->v_op->vop_remove)(&a)); + VN_KNOTE(vp, NOTE_DELETE); + VN_KNOTE(dvp, NOTE_WRITE); + + if (dvp == vp) + vrele(vp); + else + vput(vp); + vput(dvp); + + return error; } int I isolated in VOP_REMOVE(): 1. the vrele()/vpu() management no real change here, only tmpfs is a bit different (see below). 2. the knote calls the fs-dependent parts are not really uniform. some are calling them always (error or not), some only for some errors, others only on success, and others do not call them at all. I opted for the conservative approch, and always call them (what nfs did). It might be preferable to call at least knote(NOTE_DELETE) only on success, or both only on success. blob - 3ac15e3a21e85df157537d9fd03f061022abd28d file + sys/isofs/cd9660/cd9660_vnops.c --- sys/isofs/cd9660/cd9660_vnops.c +++ sys/isofs/cd9660/cd9660_vnops.c @@ -809,7 +809,7 @@ const struct vops cd9660_vops = { .vop_kqfilter = cd9660_kqfilter, .vop_revoke = vop_generic_revoke, .vop_fsync = nullop, - .vop_remove = eopnotsupp, + .vop_remove = NULL, .vop_link = cd9660_link, .vop_rename = eopnotsupp, .vop_mkdir = eopnotsupp, Replace eopnotsupp() call to NULL, VOP_REMOVE() will return ENOTSUPP if it is NULL, so no need to add a new indirection layer. blob - 173d70562f64dc55d3ab479d9f536316fd1b7dd9 file + sys/miscfs/fuse/fuse_vnops.c --- sys/miscfs/fuse/fuse_vnops.c +++ sys/miscfs/fuse/fuse_vnops.c @@ -1507,16 +1507,9 @@ fusefs_remove(void *v) goto out; } - VN_KNOTE(vp, NOTE_DELETE); - VN_KNOTE(dvp, NOTE_WRITE); fb_delete(fbuf); out: pool_put(&namei_pool, cnp->cn_pnbuf); - if (dvp == vp) - vrele(vp); - else - vput(vp); - vput(dvp); return (error); } fuse: knote() are called only when succeed. blob - 12d22a95ce0e71f74a7425580296492b4a705d47 file + sys/msdosfs/msdosfs_vnops.c --- sys/msdosfs/msdosfs_vnops.c +++ sys/msdosfs/msdosfs_vnops.c @@ -803,19 +803,11 @@ msdosfs_remove(void *v) else error = removede(ddep, dep); - VN_KNOTE(ap->a_vp, NOTE_DELETE); - VN_KNOTE(ap->a_dvp, NOTE_WRITE); - #ifdef MSDOSFS_DEBUG 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); } msdosfs: kote() are always called (error or not) blob - 7bb05c1f6333d819ea2846bb5054ccd3bec30589 file + sys/nfs/nfs_vnops.c --- sys/nfs/nfs_vnops.c +++ sys/nfs/nfs_vnops.c @@ -1723,13 +1723,6 @@ nfs_remove(void *v) error = nfs_sillyrename(dvp, vp, cnp); pool_put(&namei_pool, cnp->cn_pnbuf); 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); } nfs: knote() are always called. blob - b1c68fdebba338a23f6ed36e305b6c5df930a294 file + sys/tmpfs/tmpfs_vnops.c --- sys/tmpfs/tmpfs_vnops.c +++ sys/tmpfs/tmpfs_vnops.c @@ -742,13 +742,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; } tmpfs: no knote(), and the vput(vp) is first instead of last. I am unsure if it is right to call vput(vp) before vrele(dvp) (if dvp == vp), and if it could lead to problem on vfs pressure. With the diff, the order is the same for all FS. blob - 0097f2380c5cfd9c10bed12e4e3a06b9272022a6 file + sys/ufs/ext2fs/ext2fs_vnops.c --- 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); } ext2fs: no knote() calls. blob - d329284990703d35b484e26d2d59af1a6ac9df73 file + sys/ufs/ufs/ufs_vnops.c --- sys/ufs/ufs/ufs_vnops.c +++ sys/ufs/ufs/ufs_vnops.c @@ -583,24 +583,13 @@ ufs_remove(void *v) struct inode *ip; struct vnode *vp = ap->a_vp; struct vnode *dvp = ap->a_dvp; - int error; ip = VTOI(vp); if (vp->v_type == VDIR || (DIP(ip, flags) & (IMMUTABLE | APPEND)) || - (DIP(VTOI(dvp), flags) & APPEND)) { - error = EPERM; - goto out; - } - error = ufs_dirremove(dvp, ip, ap->a_cnp->cn_flags, 0); - VN_KNOTE(vp, NOTE_DELETE); - VN_KNOTE(dvp, NOTE_WRITE); - out: - if (dvp == vp) - vrele(vp); - else - vput(vp); - vput(dvp); - return (error); + (DIP(VTOI(dvp), flags) & APPEND)) + return EPERM; + + return ufs_dirremove(dvp, ip, ap->a_cnp->cn_flags, 0); } /* ufs: knote() are called with some errors but not all. Comments or OK ? -- Sebastien Marie