From: Miod Vallat <miod@online.fr>
Subject: Re: patch: vfs cleanup - VOP_REMOVE
To: Sebastien Marie <semarie@kapouay.eu.org>
Cc: tech@openbsd.org
Date: Sat, 4 May 2024 19:20:52 +0000

> Here a first diff with VOP_REMOVE(). If people are interested I will do
> more and do others VOP_* functions.

Please do.

> 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.

Agreed.

> 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.

I'd rather not see NULL pointers in struct vops, and the explicit NULL
check eventually removed.

> 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.

I'd rather see vrele before vput, which your diff ensures.

> Comments or OK ?

Apart from the assignment of NULL to .vop_remove instead of eopnotsupp,
I like this diff.