landed

• remove goto labels

• 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

• patch up commentary
• remove a debug tunable

In D52819#1207028, @kib wrote:

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

• upload the entire thing

• also drop ref drip around unlock

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

I think this is turning into a bikeshed, so just do what you feel.

The above may be true and the pointer may be populated depending on when you got the the inode. I would just add something like "->i_din2 is NULL, some information omitted" or compatible.

maybe print that the pointer is NULL?

I don't have an opinion on the functionality, but the implementation looks avoidably slow.

• patch up some comments
• add the missing acq fence

vput et al are the primary consumers of vdrop.

Huh, so happens vtryrecycle unlocks and then vdrops, so it happens to not run into the problem after all. General remark stands though.

In D52628#1203135, @olce wrote:

In D52628#1203128, @mjg wrote:

For a non-doomed vnode new users can pop up and disappear.

You're repeating yourself, and I repeat: Not in a way that is relevant. If I'm wrong, fine, please show me a case that you think is relevant and is not covered by handling this in vput_final(). And, anyway, deferral does not need to occur at all for non-doomed vnode.

In D52628#1203127, @olce wrote:

Why not put the defer action into vput_final() (and possibly use the already existing deferred inactive mechanism)?

• make new fields static
• expand commentary

This is finished, it just has extra parts to trigger defered drop + a printf that it happened. I'll upload a cleaned up variant shortly.

If your idea boils down to adding some form of lock drain to iput_final, it's not going to work.

See the patch I posted in D52628 .

So I think the patch proposed here is just a minor loss in perf and only fixes some of the problem.

Suppose the vnode is doomed, v_usecount is 2, v_holdcnt is 1 and both usecount holders are issuing vput.
Suppose both are read-locking it.

So here is the situation as I understand it:

note vnodes were not freeable at all and that holding the lock to maintain liveness is probably highly entrenched in the layer

Maybe I missed something in the previous comments.

In D52245#1201483, @markj wrote:

In D52245#1201325, @mjg wrote:

Note I proposed a fix to the panic which is orthogonal to the vput change.

So I slept on it. Per my previous remark about possible extra locking *and* not completely taking care of the issue anyway due to vunref, I don't think there is any benefit of this going in (and there is a visible loss)

What about something like the following (ignoring vunref() for a moment)?

if (refcount_release_if_last(&vp->v_usecount))

Note I proposed a fix to the panic which is orthogonal to the vput change.

This boots: https://people.freebsd.org/~mjg/vnode_unlock_unref.diff

The code can go back to doing unlock followed by unref. The crux of the race was a half-constructed vnode hanging out in the hash and being unexpectedly used by someone else.

fwiw I can confirm offcpu time is gone, instead almost all of it spent contending on pmap pv locks.

I also note I have been hogging the few ok zoo machines for way too long. All of it is pretty old now, but flix1 is still very much usable even for this particular work. I'm happy to free up the box provided it will be put into good use. It has some crap set up there for my own tests, but all of that can be rm -rf'ed no problem.

In D24217#1175350, @kib wrote:

vmpfw means that there are threads trying to fault on the same page. Either the page is not valid, and then the contention is unavoidable, or the page is valid (soft-fault), and then there are two cases: the contending threads faulted at the same virtual address, or not. Do you know which case is the most popular?

I looked into this in the meantime(tm).

Per my above comment the first crash does not look like it is cred-related -- something is going haywire with socket handling instead.

I agree this is unlikely to be related to the panic reported in PR, given the first stacktrace I think something is going wrong in the network stack and after that all bets are off. I would probably start with checking what's the crash site code-wise. The 'Mar 7 09:38:18 pf-cam2 kernel: freeing uidinfo: uid = 884, sbsize = 115664' suggests something is not properly torn down.

can you instead sort the files and require the ordering that smaller is addr is locked first? a dedicated routine could do it

Looking at if_unlink_ifnet:

keeping some % of reserve for root would be good,, equivalent to _falloc_noinstall

this can __assert_unreachable which will in production kernels will tell the compiler the 2 options are the only valid ones