commit eb653d9eb4f12b869f9f102b8d7e499d6bb86f5e (HEAD -> vfsversion)
Author: Mateusz Guzik <mjg@FreeBSD.org>
Date: Thu Jul 6 15:24:18 2023 +0000
This will facilitate decrufting the interface without having unpatched
modules load by accident.
It also makes unpatched modules fail to compile with KPI got bumped, for
example:
fs/msdosfs/msdosfs_vfsops.c:1198:1: error: static assertion failed due to requirement '539166470 == 539166471': filesystem uses an outdated KPI version
VFS_SET(msdosfs_vfsops, msdosfs, 0, VFS_KPI_VERSION_1);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sys/mount.h:966:2: note: expanded from macro 'VFS_SET'
_Static_assert(kpi_version == VFS_KPI_VERSION_2, \
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
Reviewed by:
Differential Revision:
commit 26300743388c50b9cad7e25f1ef4ef72ebf092d9
Author: Mateusz Guzik <mjg@FreeBSD.org>
Date: Thu Jul 6 15:12:02 2023 +0000
vfs: s/VFS_VERSION/VFS_KBI_VERSION
To make room for KPI versioning.
Reviewed by:
Differential Revision:Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
So shouldn't KBI version just be bumped with the different KPI versions change?
I'm having trouble with the mental model here, so maybe you could explain more when one or the other (or both) need to change.
In general, I like this idea, I'm just trying to get a good mental model of which of these to bump for what kinds of changes.
It's also weird that the modules depend on the programming interface, and not the binary interface. Could you explain that too please?
All this suggests at least a comment near where you define the new symbols.
| sys/sys/mount.h | ||
|---|---|---|
| 634 | you are changing the first two elements. | |
Comment Actions
no because it alters vfsconf, thus breaking KBI
I'm having trouble with the mental model here, so maybe you could explain more when one or the other (or both) need to change.
In general, I like this idea, I'm just trying to get a good mental model of which of these to bump for what kinds of changes.
It's also weird that the modules depend on the programming interface, and not the binary interface. Could you explain that too please?
All this suggests at least a comment near where you define the new symbols.
kbi discrepancies clear themselves out with mere recompilation, while kpi require patching the code and they may be of a nature which does not break compilation of unpatched code
| sys/sys/mount.h | ||
|---|---|---|
| 634 | i'll probably alter the comment to state 3 one-time taking the plunge here to avoid adding a padding gap, i don't think it is a big deal for this case the comment wants to ensure mismatch is properly reported, thus versions and fs name have to be always accessible in the same manner. again, one-time breakage is fine here imo | |
Comment Actions
But... KPI changes usually change KBI....
If, as I think, you don't need to encode the KPI into things, you don't need to change vfsconf at all...
I'm having trouble with the mental model here, so maybe you could explain more when one or the other (or both) need to change.
In general, I like this idea, I'm just trying to get a good mental model of which of these to bump for what kinds of changes.
It's also weird that the modules depend on the programming interface, and not the binary interface. Could you explain that too please?
All this suggests at least a comment near where you define the new symbols.kbi discrepancies clear themselves out with mere recompilation, while kpi require patching the code and they may be of a nature which does not break compilation of unpatched code
This makes no sense. We should be bumping the KBI version if the KBI changes (or name it something else). We don't want old KBI drivers loaded with the new KBI, at least for the normal usage of KBI versions.
KPI change will almost certainly change KBI, if nothing else with having things needing to be recompiled (and maybe recoded if rebuild fails). That too can be handled with a new KBI number.
I'm having trouble seeing the value here, but I suspect it's terminology that suggests different naming. After all, it looks like you are always testing both, and could just get by bumping the KBi version more agressively... and/or testing in the macro at compile time that the KPI version is right if you want to force the source to be proactively updated...
But maybe an example use case would help me understand why you chose this route rather than the other alternatives...
Comment Actions
I am curious what usage would this have.
So far we managed to either provide compat VOPs/VFS_OPs, or make old filesystem modules not compilable. Adding magic numbers without examples and guides to manage them has doubtful value.
Comment Actions
But I do, see below.
I'm having trouble with the mental model here, so maybe you could explain more when one or the other (or both) need to change.
In general, I like this idea, I'm just trying to get a good mental model of which of these to bump for what kinds of changes.
It's also weird that the modules depend on the programming interface, and not the binary interface. Could you explain that too please?
All this suggests at least a comment near where you define the new symbols.kbi discrepancies clear themselves out with mere recompilation, while kpi require patching the code and they may be of a nature which does not break compilation of unpatched code
This makes no sense. We should be bumping the KBI version if the KBI changes (or name it something else). We don't want old KBI drivers loaded with the new KBI, at least for the normal usage of KBI versions.
I agree it should getting bumped even though it normally is not, but you the above has no bearing on my proposal. Here I'm saying updated KBI is picked up automatically without having to *patch* any given filesystem. This is not true for KPI changes.
KPI change will almost certainly change KBI, if nothing else with having things needing to be recompiled (and maybe recoded if rebuild fails). That too can be handled with a new KBI number.
I'm having trouble seeing the value here, but I suspect it's terminology that suggests different naming. After all, it looks like you are always testing both, and could just get by bumping the KBi version more agressively... and/or testing in the macro at compile time that the KPI version is right if you want to force the source to be proactively updated...
But maybe an example use case would help me understand why you chose this route rather than the other alternatives...
You do realize VFS_SET contains:
#define VFS_SET(vfsops, fsname, flags) \
static struct vfsconf fsname ## _vfsconf = { \
.vfc_version = VFS_VERSION, \As in whatever you bump the KBI version to, it will be automagically set on recompilation of the filesystem?
With this in mind here is an example of something which landed without breaking squat KBI-wise: introduction of v_state. this resulted in a requirement to add vn_set_state calls here and there. All filesystems still compile fine, but without patching they will crash under debug.
As mentioned above, even if I bumped the KBI version it would only prevent stale binaries from loading, but recompiling them without patching anything would bypass the KBI check and still load filesystems which are *not* updated to the new requirement.
Here are examples of stuff which may or may not land which may or may not require changes and does not break compilation:
- requiring mount point != NULL for getnewvnode. passing NULL is technically illegal for years and is only allowed because the vp_crossmp hack is using it. should it disappear, the requirement can be strictly enforced. but if a filesystem abuses it, you will only find out about it with a crash when using it. on the other hand if KPI check was implemented it you will get a report on load time.
- ->vfs_root calls without the filesystem being vfs_busy'ed and allowing the root vnode to only be refed, not locked. again no compilation-breaking changes. and again if i bump KBI, a non-conformant filesystem only needs to get recompiled to bypass the KBI check and fail to work because it was not updated to the new KPI. On the other hand if I add kpi_version, even the recompiled module will fail to load until manually patched.
Perhaps I should have also noted that indicating versions will also provide space for a short log what is the new requirement.
All that said, I think I can improve on the patch by causing a compilation failure with indicated KPI does not match which would be a great addition. I'll experiment with it.
Comment Actions
thanks for your continued patience.
I have a number of thoughts on this, but my wife's been out of town and things around the house have scattered my mind and eaten my spare time.
I should have a few hours in the morning that I can finally focus on things like answering this review.
Sorry my chaotic personal life has delayed my response.
Comment Actions
I like the idea because it allows to indicate a change in contract that is not reflected in the KPI, such as some required locking status before or after the call or particular restrictions on arguments, as happens in Mateusz's examples. These problems could be detected at run-time instead, but doing so at compile-time is simply more productive and less surprising.
That said, this does not alone solve the problem of supporting different conventions in parallel, if the envisioned changes can't be realized at once, e.g., when changing VFS_ROOT() to return an unlocked vnode for *all* filesystems (including possibly out-of-tree ones, such as in ports or at some vendor's). But it's a step towards that, should it be needed. Indeed, one can envision a two-step phase where, taking again the VFS_ROOT() example, a new VFS_ROOT_REFED() is introduced, co-existing with VFS_ROOT() for a while so that the new VFS op can be implemented everywhere needed. When it's deemed done, or perhaps more formally when backwards-compatibility can be broken, VFS_ROOT_REFED() implementations are used for VFS_ROOT() instead, and the old implementations of VFS_ROOT() are simply discarded and the VFS op removed. I would like to point out that, in this case, bumping the KBI twice (__FreeBSD_version), once after introducing VFS_ROOT_REFED() and once after retiring it, is not enough since old source code will still compile with the version after VFS_ROOT_REFED() removal (assuming the signature of VFS_ROOT() did not change, and thus that that of VFS_ROOT_REFED() was identical to it). To avoid that, the KPI version has to be bumped after "renaming" VFS_ROOT_REFED() to VFS_ROOT().
So, really, this change provides guarantees that simply cannot be provided by any number of KBI version increases.
| sys/sys/mount.h | ||
|---|---|---|
| 634 | I understand the will to avoid the padding gap, but given the low number of vfsconf structures (one per filesystem type, IIRC), it doesn't make much difference. That said, the one-time breakage is not horrible indeed, and I think can be dealt with by just bumping the KBI. | |
Comment Actions
So for any bump of VFS_KPI_VERSION, we have to:
- inspect all consumers of the suspected KPI
- bump all in-tree VFS_SET() lines.
I do not see any benefit there from the second action, it only adds more churn. Also, it does not help third-party in any way: they probably would blindly bump the VFS_KPI_VERSION in their sources if before they blindly recompiled the code.
For VFS_KBI_VERSION, why existing __FreeBSD_version is not enough?
Comment Actions
After inspection and before bumping the version in its corresponding VFS_SET(), a filesystem has to be adapted to the (new) KPI contract.
I do not see any benefit there from the second action, it only adds more churn.
Globally, this change essentially serves the purpose of preventing successful compilation against the VFS after some change in contract not reflected in (old) VFS ops' signatures. This is arguably mostly useful for out-of-tree modules.
What could be the alternative to VFS_SET() with the KPI version of the FS as an additional argument? Having a variant of VFS_SET() specific to in-tree code, which automatically adjust to the current KPI version, and another one for out-of-tree code? But then, the risk is simply that out-of-tree code will use the in-tree-specific primitive, if only by copy-paste, defeating the approach. And I think this risk is high.
Also, it does not help third-party in any way: they probably would blindly bump the VFS_KPI_VERSION in their sources if before they blindly recompiled the code.
Doing so indeed would defeat the whole purpose of this change. But would they really do that? I certainly wouldn't without first understanding what this VFS KPI versioning is all about. At the very least, they would know that something must be going on and try to find some doc.
This is different from blindly recompiling stuff in case of a KBI change, which should never be a problem (in this case, why should they care about anything else than recompiling?).
Maybe more guidance is warranted, perhaps directly in the asserts' messages, saying one has to adapt the implementation to the new KPI before bumping VFS_KPI_VERSION_* in VFS_SET()?
For VFS_KBI_VERSION, why existing __FreeBSD_version is not enough?
Bumping __FreeBSD_version is indeed enough from the strict functionality point of view.
So maybe having two fields in struct vfsops is not that necessary. An alternative would be to just keep vfc_version but use it instead for the KPI version (and then failing at compile-time on version mismatch, still passing the FS version in VFS_SET(), as currently proposed). The KBI would be controlled by bumping __FreeBSD_version.
I may have been too quick in validating the current set of changes as is, I rather wanted to validate the high-level approach, which I think is worth it (provided there are actual use cases to be committed after).