nullfs: smr-protected hash lookup and locking

[mjg]

Summary

this largely sorts out contention when using poudriere

commit 2246193ba69caf2211f4e7ee26b7d33bb9b415b7
Author: Mateusz Guzik <[e-mail]>
Date:   Wed Oct 1 10:06:39 2025 +0000

    nullfs: avoid the interlock in null_lock with smr
    
    This largely eliminates contention on the vnode interlock.
    
    Note this still does not scale, to be fixed(tm).

commit 3af18ee2ed6ef0584235ae42a905fc8e78119bff
Author: Mateusz Guzik <[e-mail]>
Date:   Tue Sep 30 15:16:30 2025 +0000

    nullfs: smr-protected hash lookup
    
    Vast majority of real-world contention on the hash comes from lookups,
    notably seen during highly parallel poudriere runs.
    
    Lockless lookup largely alleviates the problem.

commit a222eb981209bb939c7c7f085bdb2f476b2b6120
Author: Mateusz Guzik <[e-mail]>
Date:   Wed Oct 1 10:28:48 2025 +0000

    nullfs: assert the vnode is not doomed in null_hashget_locked
    
    While here some style touch ups and fixing a stale name in an assert.

commit 975fdfbf725ca41e878f8be31bb140dcdcae352f
Author: Mateusz Guzik <[e-mail]>
Date:   Wed Oct 1 11:09:50 2025 +0000

    nullfs: remove the vhold/vdrop cycle around unlock
    
    Both lower vnode and null data are safe while the lock is held,
    at the same time neither is touched after unlock.
    
    While here remove stale comment about interlock handling. It is no
    longer legal to pass to unlock.

Test Plan

tested by pho

[kib]

The commit message body is ought to be more detailed than <nul>.

sys/fs/nullfs/null_subr.c

44

Should be ordered alphabetically.

156

This should be also asserted. Both there and in null_hashget_locked().

Check for the v_mount below can be considered to be part of the assert, but it is really strange: IMO we should explicitly check for doomed, and the later check for v_mount != mp actually consumes the check for v_mount != NULL.

[mjg]

• doomed asserts
• cosmetic fixups to null_hashget_locked
• null_lock using smr

[mjg]

sys/fs/nullfs/null_vnops.c

860

This ref trip is no longer of any use, is it?

[mjg]

• also drop ref drip around unlock

sys/fs/nullfs/null_vnops.c

196

this is a profiling aid and wont be committed

[kib]

This is not a complete patch, I think? The other bits except smr lock are missing.

[mjg]

• upload the entire thing

[mjg]

In D52819#1207028, @kib wrote:

This is not a complete patch, I think? The other bits except smr lock are missing.

indeed, sorted out

[mjg]

sys/fs/nullfs/null_vnops.c

894

this needs rewording. VOP_UNLOCK promises to not touch anything past unlock and this routine itself also does not

[mjg]

• patch up commentary
• remove a debug tunable

[kib]

sys/fs/nullfs/null_subr.c

44

Please fix this.

[mjg]

• sort includes
• fix a bug found by pho@ in the lock smr patch: after the lock is acquired, doomed handling has to be the same as with the interlock-protected routine

[kib]

sys/fs/nullfs/null_vnops.c

807

out_bad can be removed. Initialize lvp with NULL, and re-assign it to NULL if vhold_smr() failed.

[mjg]

sys/fs/nullfs/null_vnops.c

807

you mean something like this?

vfs_smr_enter();
  
  lvp = NULL;
  
  nn = VTONULL_SMR(ap->a_vp);
  if (nn != NULL) {
          lvp = nn->null_lowervp;
          if (lvp != NULL) {
                  if (!vhold_smr(lvp)) {
                          lvp = NULL;
                  }
          }
  }
  
  vfs_smr_exit();
  return (lvp);

i think that's less amenable to changes later

anyway i'm not going to argue about this, just spell out the implementation you want and i'll include it

[kib]

sys/fs/nullfs/null_vnops.c

807

For me, this version is more readable. I would even write

if (lvp != NULL && !vhold_smr(lvp))
      lvp = NULL;

[mjg]

• remove goto labels

[kib]

sys/fs/nullfs/null_vnops.c

793

The initialization can be moved out of smr region.

[olce]

Looks good (but please see suggested changes). I've not tested the patch though.

sys/fs/nullfs/null_subr.c

43–44

146

sys/fs/nullfs/null_vnops.c

872

That line could be moved at top of the block.

[mjg]

landed