Like most of VFS, these are completely undocumented. Can you elaborate?

Most definitely not.

There is now a LOR here which will have to be addressed...

It's something that is done by most (all?) the file systems in the tree.

See the various unmount functions in the sources under sys/fs/

I don't recall what the reasoning was for it.

Yes, you need to rangelock the whole file. Or at least, from the point where you start reading, to the MAX_OFF_T.

These two lines can be written as vput()

D37966

The only invariant that prevents filesystem unmount in lookup is the locked vnode. After the vnode is unlocked, other thread might unmount fs, and then VFS_VGET() would operate on the free memory, specifically *mp and *(mp->mnt_data).

vn_vget_ino() busies mp in a way that avoids LoR between vnode lock and busy state. Busy filesystem cannot be unmounted. While busy, vn_vget_ino() does VFS_VGET() ensuring that we do not operate on the freed memory.

Why is this tunable, and not a mount option?

I was not able to find the use or initialization to non-NULL values, for both cp and dev members. What for are they?

New files should come with SPDX-License-Identifier there. No idea where to look for the guidance, might be the committers guide has some info.

I believe the convention now is to not add $FreeBSD$ tags (and sys/cdefs.h) for new files.

LK_SHARED is enough for VOP_READ

This is wrong lock order, range lock is before vnode lock.

LK_SHARED is enough for VOP_ACCESS

LK_SHARED is enough for VOP_GETATTR

It is better to unlock the vnode right after VOP_GETATTR(), there is no point of copying data from local va under the vnode lock.

Don't you leak the vnode lock on VOP_GETATTR() error?

uiomove() under the vnode lock is potential deadlock, when e.g. user sets up the buffer as backed by the vnode itself. Can zio node be mapped in the userspace? Is it writeable? It seems that it is accessible by userspace?

malloc() under the vnode lock is potential deadlock in pagedaemon, which might need to write out or free pages owned by the locked vnode, to satisfy the allocation. BTW, why do you need the exclusive lock there?

This is LoR, rangelock should be taken before the vnode lock.

vn_lock/vref is better written as vget()

See https://www.freebsd.org/internal/software-license/

Bump on commit.

https://www.freebsd.org/internal/software-license/

Can you please be more specific?

Can you please be more specific?

Maybe he means section 1 of https://docs.freebsd.org/en/articles/license-guide/ ?

I need to update that article (there's currently like 5 places we recommend stuff). Same answer below.

Yes, that's what I meant. And this isn't the only time I confused those.

I'm sorry but this does not answer my question. What, specifically, do you want me to change?

Leftovers from an earlier version of the driver.

It's read-only, accessible by userspace for debugging purposes. I can remove that if it causes trouble.

Why did you kept them?

What is this vnode for? If it is used, please add a comment.

This is not handled despite marked as such.

Same.

Again not handled.

Decide (based on imp's link) whether the license should just be a copyright notice followed by the SPDX-License-Identifier line, without the explicit language, since that's the preferred license for new files. Then adjust the license comment block if/as needed. (This may also be applicable to any new source files; my focus as a rule is on documentation only.)

I've removed them from my branch, but I haven't updated the review yet because the current version panics.

It's the tarball.

There is no change here.

Uh. Either I was hallucinating, or Phabricator claimed ${NO_WINFINITE_RECURSION} was added. My money is on the latter. But either way, no change shows here now.

I think it's something you want to tune system-wide, not per-mount. But I suppose I can add a mount option that overrides the tunable. Or remove it entirely, as I'm not sure how useful it really is.