Convert bunch of macros from sys/mount.h into static inlines

[kib]

Summary

This gives type safety without loosing compiler optimizations

vfs: convert vfs_op_thread_* macros to static inlines


sys/vnode.h: convert VN_KNOTE() to function


vfs: convert VFS_OPs from macros to static inlines


sys/vnode.h: remove stale comment

The source sweep is not going to happen.

[markj]

sys/sys/mount.h

865

sys/sys/vnode.h

232 ↗

(On Diff #176310)

The list-is-empty check is no longer inlined, now it requires an extra function call in the common case where there are no knotes present.

I'd actually like to add a new VIRF flag which gets set when the knote list is non-empty, similar to VIRF_INOTIFY. Then, instead of checking v->pollinfo != NULL && !KNLIST_EMPTY(&...) and vp->v_irflag & VIRF_INOTIFY separately in various VOP _post implementations, they can look like this:

void
vop_open_post(void *ap, int rc)
{
    struct vop_open_args *a = ap;

    if (rc == 0 && __predict_false((a->a_vp->v_irflag & (VIRF_INOTIFY | VIRF_KNOTE)))) {
        <slow path>
    }
}

[kib]

Coded VIRF_KNOTE.

The issue I see is that for vn_pollrecord() must interlock the enqueuing of of a knote with setting the VIRF_KNOTE flag, which creates the order between the vnode interlock and pollinfo vpi_lock mutex. I do not think there was an order before, then it should be fine for now. But even if it is, it is somewhat limiting the future code modifications.

[markj]

In D56611#1296692, @kib wrote:

Coded VIRF_KNOTE.

The issue I see is that for vn_pollrecord() must interlock the enqueuing of of a knote with setting the VIRF_KNOTE flag, which creates the order between the vnode interlock and pollinfo vpi_lock mutex. I do not think there was an order before, then it should be fine for now. But even if it is, it is somewhat limiting the future code modifications.

Hmm but the knote list is locked by the vnode lock, not by the pollinfo lock. See vfs_knllock().

I'm not sure why it was done this way. Maybe it's an optimization, since many VOPs already hold the vnode lock when they scan the list?

sys/kern/vfs_subr.c

5326

Shouldn't we set this in vfs_kqfilter(), not here?

5818

Maybe vop_check_pollinfo()? And I would annotate this with __predict_false.

[kib]

In D56611#1297270, @markj wrote:

In D56611#1296692, @kib wrote:

Coded VIRF_KNOTE.

The issue I see is that for vn_pollrecord() must interlock the enqueuing of of a knote with setting the VIRF_KNOTE flag, which creates the order between the vnode interlock and pollinfo vpi_lock mutex. I do not think there was an order before, then it should be fine for now. But even if it is, it is somewhat limiting the future code modifications.

Hmm but the knote list is locked by the vnode lock, not by the pollinfo lock. See vfs_knllock().

Right. And with the correction of setting the flag in vfs_kqfilter, the flag is naturally protected by the vnode lock. This makes it more natural to have it in v_vflag than in v_irflag (but I have same opinion about VIRF_INOTIFY).

I'm not sure why it was done this way. Maybe it's an optimization, since many VOPs already hold the vnode lock when they scan the list?

[kib]

Set VIRF_KNOTE in vfs_kqfilter(), where it should be, on inserting knote into the knlist.
Rename vop_check_knote() to vop_check_pollinfo().

[kib]

Do not set VIRF_KNOTE in knlist unlock, vfs_kqfilter() should be enough.

[markj]

In D56611#1297281, @kib wrote:

In D56611#1297270, @markj wrote:

In D56611#1296692, @kib wrote:

Coded VIRF_KNOTE.

The issue I see is that for vn_pollrecord() must interlock the enqueuing of of a knote with setting the VIRF_KNOTE flag, which creates the order between the vnode interlock and pollinfo vpi_lock mutex. I do not think there was an order before, then it should be fine for now. But even if it is, it is somewhat limiting the future code modifications.

Hmm but the knote list is locked by the vnode lock, not by the pollinfo lock. See vfs_knllock().

Right. And with the correction of setting the flag in vfs_kqfilter, the flag is naturally protected by the vnode lock. This makes it more natural to have it in v_vflag than in v_irflag (but I have same opinion about VIRF_INOTIFY).

My main reason for putting it in v_irflag was because the flag is checked "frequently." If we had a frequently read flag field which is synchronized by the vnode lock, then that would be fine too.

[kib]

In D56611#1297289, @markj wrote:

In D56611#1297281, @kib wrote:

Right. And with the correction of setting the flag in vfs_kqfilter, the flag is naturally protected by the vnode lock. This makes it more natural to have it in v_vflag than in v_irflag (but I have same opinion about VIRF_INOTIFY).

My main reason for putting it in v_irflag was because the flag is checked "frequently." If we had a frequently read flag field which is synchronized by the vnode lock, then that would be fine too.

We have 7 bytes unused in the v_rl rangelock member of struct vnode. I will propose something after this batch is landed.

[markj]

sys/kern/vfs_subr.c

6709

Why vholdl()?

When I run the regression test suite, the sys/kern/path_test tests trigger a panic:

VNASSERT failed: mtx_owned(VI_MTX(vp)) not true at /home/markj/sb/main/src/sys/kern/vfs_subr.c:5761 (assert_vi_locked)                                         
0xfffffe019af43dc0: type VREG state VSTATE_CONSTRUCTED op 0xffffffff830c6380                                                                                   
    usecount 1, writecount 0, refcount 3 seqc users 0                                                                                                          
    hold count flags ()                                                                                                                                        
    flags (VIRF_PGREAD|VMP_LAZYLIST)                                                                                                                           
    v_object 0xfffffe01991cec30 ref 0 pages 1 cleanbuf 0 dirtybuf 1                                                                                            
    lock type ufs: UNLOCKED                                                                                                                                    
        nlink=1, effnlink=1, size=1024, extsize=0                                                                                                              
        generation=604727da, uid=0, gid=0, flags=0x0                                                                                                                                                                                                                                                                          
        ino 886830, on dev gpt/rootfs                                                                                                                          
panic: vfs_kqfilter: vnode interlock is not locked but should be                                                                                               
cpuid = 11                                                                                                                                                     
time = 1777327121                                                                                                                                              
KDB: stack backtrace:                                                                                                                                          
db_trace_self_wrapper() at db_trace_self_wrapper+0xa5/frame 0xfffffe00f17cdfd0                                                                                 
kdb_backtrace() at kdb_backtrace+0xc6/frame 0xfffffe00f17ce130                                                                                                                                                                                                                                                                
vpanic() at vpanic+0x214/frame 0xfffffe00f17ce2d0                                                                                                                                                                                                                                                                             
panic() at panic+0xb5/frame 0xfffffe00f17ce390                                                                                                                 
vfs_kqfilter() at vfs_kqfilter+0x2a7/frame 0xfffffe00f17ce440                                                                                                  
VOP_KQFILTER_APV() at VOP_KQFILTER_APV+0x7d/frame 0xfffffe00f17ce470                                                                                           
vn_kqfilter_opath() at vn_kqfilter_opath+0xe1/frame 0xfffffe00f17ce530                                                                                         
kqueue_register() at kqueue_register+0xa24/frame 0xfffffe00f17ce690                                                                                            
kqueue_kevent() at kqueue_kevent+0x1b4/frame 0xfffffe00f17ce9c0                                                                                                
kern_kevent_fp() at kern_kevent_fp+0xce/frame 0xfffffe00f17cea10                                                                                                                                                                                                                                                              
kern_kevent() at kern_kevent+0x12f/frame 0xfffffe00f17ceaf0                                                                                                                                                                                                                                                                   
kern_kevent_generic() at kern_kevent_generic+0x135/frame 0xfffffe00f17cebd0                                                                                                                                                                                                                                                   
sys_kevent() at sys_kevent+0x232/frame 0xfffffe00f17ced10                                                                                                                                                                                                                                                                     
amd64_syscall() at amd64_syscall+0x3d5/frame 0xfffffe00f17cef30                                                                                                                                                                                                                                                               
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe00f17cef30                                                                                     
--- syscall (560, FreeBSD ELF64, kevent), rip = 0xdd0a4b1420a, rsp = 0xdd0a201fc88, rbp = 0xdd0a2020110 ---

[kib]

sys/kern/vfs_subr.c

6709

Sorry for it. The change was a left-over from using the interlock for setting the flag, before I changed to vnode lock.

[kib]

vhold

[markj]

I think a portion of the diff is missing, is it possible to re-upload?

[kib]

Re-open and restore the diff.

[markj]

sys/sys/vnode.h

228 ↗

(On Diff #177127)

style(9) puts the do { on the previous line.

235 ↗

(On Diff #177127)

These are basically the same as VFS_KNOTE_* except for KNF_NOKQLOCK. Do you know why the NOKQLOCK flag is needed?

1041 ↗

(On Diff #177127)

Probably this should get a pollinfo check too?

1064 ↗

(On Diff #177127)

We should instead check for pollinfo with if (vop_check_pollinfo((ap)->a_vp) && noffset > ooffset) {...

[markj]

Some of the tests/sys/kern/inotify_test tests fails after this change, but I can't immediately see why.

[kib]

sys/sys/vnode.h

235 ↗

(On Diff #177127)

commit fe1d3f15f6b68135ca25a5c67caf135cd137e435
Author: Stanislav Sedov <[e-mail]>
Date:   Sun Jun 28 21:49:43 2009 +0000

    - Turn the third (islocked) argument of the knote call into flags parameter.
      Introduce the new flag KNF_NOKQLOCK to allow event callers to be called
      without KQ_LOCK mtx held.
    - Modify VFS knote calls to always use KNF_NOKQLOCK flag.  This is required
      for ZFS as its getattr implementation may sleep.

[kib]

In D56611#1302088, @markj wrote:

Some of the tests/sys/kern/inotify_test tests fails after this change, but I can't immediately see why.

There were two issues, one is mine, one pre-existing:

• vop_check_pollinfo() should also check VIRF_INOTIFY_PARENT
• vop_read_pgchache_post() did not generated inotify IN_ACCESS, which probably went unnoticed because all tests for inotify were run on zfs.

[kib]

Move vop_check_pollinfo() into sys/vnode.h for vnode_if.c
Add several more vop_check_pollinfo() uses pointed out by markj.
Add VIRF_INOTIFY_PARENT check for vop_check_pollinfo()
Add inotify hook to vop_read_pgcache_post().

[markj]

In D56611#1302324, @kib wrote:

In D56611#1302088, @markj wrote:

Some of the tests/sys/kern/inotify_test tests fails after this change, but I can't immediately see why.

There were two issues, one is mine, one pre-existing:

• vop_check_pollinfo() should also check VIRF_INOTIFY_PARENT
• vop_read_pgchache_post() did not generated inotify IN_ACCESS, which probably went unnoticed because all tests for inotify were run on zfs.

When I run regression tests I usually run on ufs. When I developed the tests I tested on at least zfs, tmpfs, ufs, p9fs.

I looked a bit more at inotify_event_access_file, ufs_read_pgcache() is returning EJUSTRETURN. There, we are calling read(fd, buf, 16), where the file backed by fd contains only 4 bytes. So, uio->uio_resid != 0 at the end of ufs_read_pgcache(), and the VFS falls back to plain VOP_READ, and so the test passes despite the bug.

[markj]

sys/sys/vnode.h

1000 ↗

(On Diff #177331)

I am not sure if this helper is worth it. We are not using it before e.g., INOTIFY_NAME, and the inconsistency is confusing.

[kib]

sys/sys/vnode.h

1000 ↗

(On Diff #177331)

I am not sure what do you mean there. Are you proposing to open-code the flags check into each place where vop_check_pollinfo() is currently used?

[markj]

sys/sys/vnode.h

1000 ↗

(On Diff #177331)

No, just revert to open-coding the rc == 0 check and let INOTIFY*, VFS_KNOTE* handle the flag checks (with __predict_false annotations).

[kib]

sys/sys/vnode.h

1000 ↗

(On Diff #177331)

IOW remove the vop_check_pollinfo()? I prefer not to. The next planned step is to move flags into vnode-locked field, and despite removal would not be blocking that, it is much less churn overall.

[kib]

sys/sys/vnode.h

1000 ↗

(On Diff #177331)

I retract. After trying to move the VIRF_INOTIFY into a vnode-locked member of the struct vnode, I see that currently there is too much reliance on the interlock, in particular in the cache (but in vfs_inotify.c as well).

So I would only move VIRF_KNOTE for now. And then indeed vop_check_pollinfo() is pointless and even detrimental.

[kib]

Drop vop_check_pollinfo().