OS-8419: Move v_phantom_count to a pre-existing HOLE.

Details

Issue Type:Bug
Priority:5 - Low
Status:Open
Created at:2022-10-25T14:40:40.657Z
Updated at:2022-10-25T19:37:42.220Z

People

Created by:Dan McDonald
Reported by:Dan McDonald
Assigned to:Dan McDonald

Description

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.

Comments

Comment by Dan McDonald
Created at 2022-10-25T18:11:03.250Z

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]% 

Comment by Dan McDonald
Created at 2022-10-25T19:37:42.220Z

After moving the position of v_phantom_count, things get much less different:

nowhere(~/build/illumos-omnios)[0]% diff /tmp/{gate,post-fixed}
11c11
<     2c uint32_t <<HOLE>> 
---
>     2c uint_t v_phantom_count 
nowhere(~/build/illumos-omnios)[1]%