Issue Type: | Bug |
---|---|
Priority: | 5 - Low |
Status: | Open |
Created at: | 2022-10-25T14:40:40.657Z |
Updated at: | 2022-10-25T19:37:42.220Z |
Created by: | Dan McDonald |
---|---|
Reported by: | Dan McDonald |
Assigned to: | Dan McDonald |
Recently here: [https://twitter.com/bcantrill/status/1584686915030175744|https://twitter.com/bcantrill/status/1584686915030175744|smart-link]
And here: [https://luqman.ca/blog/multi-kernel-drifting/|https://luqman.ca/blog/multi-kernel-drifting/|smart-link]
Folks discovered that the fixes we had for [https://mnx.atlassian.net/browse/OS-8054|https://mnx.atlassian.net/browse/OS-8054|smart-link] made a change to vnode_t - the addition of v_phantom_count in an unfortunately aligned place - that causes inconsistencies in vnode_t offsets across illumos distros.
Now fixing this to keep vnode_t more consistent across illumos distros again WILL mark a range of both SmartOS and OmniOS as incompatible going forward UNLESS vnode-referencing 3rd-party modules (like FUSE) are built specifically for that range (starting with release-20200604 in SmartOS) in addition to distros/releases with vnode_t consistency. (e.g. Two compiled versions of FUSE!) If SmartOS and OmniOS do NOT fix this, there will continue to be a need for vnode-referencing modules to have distinct versions for SmartOS and OmniOS, and non-OS-8054 distros like OpenIndiana, and (per above) Oxide’s Helios.
Either way requires pain. I’m inclined to fix it in SmartOS (and hopefully have it pulled into OmniOS) where one of the pre-existing HOLEs in vnode_t (there are some, utter echo "::print -ath vnode_t" | mdb -k
in OI or Helios to see.
0 vnode_t { 0 kmutex_t v_lock { 0 void *[1] _opaque } 8 uint_t v_flag c uint_t v_count 10 void *v_data 18 struct vfs *v_vfsp 20 struct stdata *v_stream 28 enum vtype v_type 2c uint32_t <<HOLE>> /* XXX EITHER HERE... */ 30 dev_t v_rdev 38 struct vfs *v_vfsmountedhere 40 struct vnodeops *v_op 48 struct page *v_pages 50 struct filock *v_filocks 58 struct shrlocklist *v_shrlocks 60 krwlock_t v_nbllock { 60 void *[1] _opaque } 68 kcondvar_t v_cv { 68 ushort_t _opaque } 6a unsigned <<HOLE>> :48 /* ... or HERE ... */ 70 void *v_locality 78 struct fem_head *v_femhead 80 char *v_path 88 hrtime_t v_path_stamp 90 uint_t v_rdcnt 94 uint_t v_wrcnt 98 u_longlong_t v_mmap_read a0 u_longlong_t v_mmap_write a8 void *v_mpssdata b0 void *v_fopdata b8 kmutex_t v_vsd_lock { b8 void *[1] _opaque } c0 struct vsd_node *v_vsd c8 struct vnode *v_xattrdir d0 uint_t v_count_dnlc d4 uint32_t <<HOLE>> /* ... OR HERE .... */ }
Given those three choices, I’m further inclined to place it at the first hole, even if it makes reading the structure source slightly jarring. This allows further hole-filling places in the future, including expanding vnode_t if need be.
Fixing it will mark a range of incompatible releases, but that range would be finite.
A quick diff of mdb’s views of vnode_t
illumos-gate vs. illumos-omnios (which uses our vnode.h changes) shows the problem in brutal detail:
nowhere(~/build/illumos-omnios)[0]% diff /tmp/{gate,pre-fixed} 7,19c7,21 < 10 void *v_data < 18 struct vfs *v_vfsp < 20 struct stdata *v_stream < 28 enum vtype v_type < 2c uint32_t <<HOLE>> < 30 dev_t v_rdev < 38 struct vfs *v_vfsmountedhere < 40 struct vnodeops *v_op < 48 struct page *v_pages < 50 struct filock *v_filocks < 58 struct shrlocklist *v_shrlocks < 60 krwlock_t v_nbllock { < 60 void *[1] _opaque --- > 10 uint_t v_phantom_count > 14 uint32_t <<HOLE>> > 18 void *v_data > 20 struct vfs *v_vfsp > 28 struct stdata *v_stream > 30 enum vtype v_type > 34 uint32_t <<HOLE>> > 38 dev_t v_rdev > 40 struct vfs *v_vfsmountedhere > 48 struct vnodeops *v_op > 50 struct page *v_pages > 58 struct filock *v_filocks > 60 struct shrlocklist *v_shrlocks > 68 krwlock_t v_nbllock { > 68 void *[1] _opaque 21,22c23,24 < 68 kcondvar_t v_cv { < 68 ushort_t _opaque --- > 70 kcondvar_t v_cv { > 70 ushort_t _opaque 24,36c26,38 < 6a unsigned <<HOLE>> :48 < 70 void *v_locality < 78 struct fem_head *v_femhead < 80 char *v_path < 88 hrtime_t v_path_stamp < 90 uint_t v_rdcnt < 94 uint_t v_wrcnt < 98 u_longlong_t v_mmap_read < a0 u_longlong_t v_mmap_write < a8 void *v_mpssdata < b0 void *v_fopdata < b8 kmutex_t v_vsd_lock { < b8 void *[1] _opaque --- > 72 unsigned <<HOLE>> :48 > 78 void *v_locality > 80 struct fem_head *v_femhead > 88 char *v_path > 90 hrtime_t v_path_stamp > 98 uint_t v_rdcnt > 9c uint_t v_wrcnt > a0 u_longlong_t v_mmap_read > a8 u_longlong_t v_mmap_write > b0 void *v_mpssdata > b8 void *v_fopdata > c0 kmutex_t v_vsd_lock { > c0 void *[1] _opaque 38,41c40,43 < c0 struct vsd_node *v_vsd < c8 struct vnode *v_xattrdir < d0 uint_t v_count_dnlc < d4 uint32_t <<HOLE>> --- > c8 struct vsd_node *v_vsd > d0 struct vnode *v_xattrdir > d8 uint_t v_count_dnlc > dc uint32_t <<HOLE>> nowhere(~/build/illumos-omnios)[1]%