vfs: add cheaper vn_isdisk by indicating the state with the VIRF_ISDISK flag

[mjg]

Summary

Fundamental semantic difference to the previous variant is that the flag is only cleared on recycle, meaning a forcibly removed device will still be reported as a disk.

Callers which need an exact answer can use vn_isdisk_error.

[kib]

I do not think this should go as is, esp. the change to vn_isdisk. Callers should be converted one-by-one. I rechecked all callers in vfs_bio.c and vnode_pager.c, they can use new vn_isdisk. Other users should be left at old vn_isdisk.

sys/fs/devfs/devfs_vnops.c

612

Why not move this case to D_TYPE ?

616

I do not quite see how could it not panic for almost all devices. I believe that most of cdevsw in system do not specify types.

sys/kern/vfs_subr.c

5004

Is this diff against FreeBSD ?

[cem]

I think kib’s concerns should be addressed but the general idea lgtm.

sys/fs/devfs/devfs_vnops.c

616

D_NOTYPE is a new define for 0. But I agree it does not make much sense. These are all independent bits rather than an enumerated field; more than one could be set. Some drivers set multiple bits iirc. Maybe that’s wrong, but probably out of scope here.

sys/kern/vfs_subr.c

3960

We do have %b in printf(9) :-)

sys/sys/vnode.h

240

Is this semantic expected for all VIRF flags? Should it be documented like above for VI/VV?

725

Thanks, inline seems right for this. Should we add a lock assert?

[cem]

sys/kern/vfs_subr.c

5004

https://svnweb.freebsd.org/changeset/base/364372 I guess

[mjg]

sys/fs/devfs/devfs_vnops.c

612

This is sorted by value.

616

But TTY/DISK/MEM are all distinct values, I don't see anyone mixing them nor what would they do it.

sys/sys/vnode.h

725

what lock? we only need the vnode pointer to be valid

[mjg]

sys/sys/vnode.h

240

The only requiremed for chaning v_irflag is the interlock. So happens VIRF_DOOMED will only show up when the vnode is also held.

[cem]

sys/sys/vnode.h

240

Can we add a basically identical line to existing VI flags for VIRF then? (Actually why do we have VIRF at all if lifetime is the same as VI?)

725

Is it valid if doomed? I guess we may have checked doomed under lock and grabbed a ref/hold and this would be safe without lock. Nevermind.

[mjg]

sys/sys/vnode.h

240

v_irflag resides in an (almost) read-only area

[kib]

sys/kern/vfs_subr.c

5004

I do not like it quite. I think that old name vn_is_disk() should retain the current semantic, then you can add e.g. macro VP_IS_DISK() or VN_IS_DISK() which checks VIRF_DISK, an convert specific consumers.

sys/sys/vnode.h

240

By design of VFS, vnode can become doomed only under the vnode lock. VI_DOOMED required both vnode lock and vnode interlock, safely checked under either of them.

Same is true for VIRF_DOOMED, and this is a fundamental part of safe reclaim.

VIRF_PGREAD and apparently VIRF_ISDISK are set and then guaranteed to be never cleared. VIRG_PGREAD have to wait for VOP_OPEN() unfortunately, to be set. VIRF_ISDISK should be set before the vnode becomes visible to any consumer.

[mjg]

sys/kern/vfs_subr.c

5004

Note the above commit does not change anything in terms of semantics so there is no rush.

I don't think capitalized name is a solution either. Perhaps something denoting that the return value can only be trusted to the extent that the vnode was pointing to an actual disk would work, but I don't have a good name for it. If anything, I had a name the other way: vn_isdisk is the possibly stale state while vn_isdisk_exact performs all the checks.

[kib]

sys/kern/vfs_subr.c

5004

Then why did you committed the _error() churn ?

Capitalizing the name is in line with it being a macro. But if you want to look at the semantic aspect, current vn_isdisk really takes a lot of efforts to check current state of cdevsw, which is the reason for it taking the dev_mtx.

So might be vn_isdisk() could be the fine name for cheap vnode flag checker indeed, while current vn_isdisk(_error) be something like vn_iscdevdisk or vn_cdev_disk().

[mjg]

sys/kern/vfs_subr.c

5004

Maybe I did not express myself properly. I only protest vn_isdisk vs VN_IS_DISK as it would be quite unclear which is which and why this way and not the other. I figred vn_isdisk_error is a good enough name, but if that's a problem here is another one: vn_isdisk_exact. I have no opinion about vn_cdev_disk or vn_iscdevdisk and I'm happy to update the patch with any of them.

[kib]

sys/kern/vfs_subr.c

5004

I think I expressed my opinion clear before. I believe that vn_isdisk_error() is bad name, something with cdev or cdevsw in the name is much better. I would not need to look into the implementation to remember that it references cdev.

Then VIRF-function can be added, and convertion happens piece-by-piece. I remember that I reviewed some chunks, but already forgot which.