vfs: remove production kernel checks and mp == NULL support from vdrop

[mjg]

Summary

• The only place in the tree which calls getnewvnode with mp == NULL does it for vp_crossmp which will never execute this codepath. Any vnode which legally has ->v_mount == NULL is also doomed, which once more wont execute this code.
• removes an assertion for v_holdcnt from production kernels. it gets taken care of by refcount macros in debug kernels.

Test Plan

Will ask pho. Works for me in stress2.

[kib]

sys/kern/vfs_subr.c

3209 ↗

(On Diff #65357)

vdrop_ret_to_free() I suggest

3223 ↗

(On Diff #65357)

Would it be more reasonable to assert VI_ACTIVE in vdropl() ?

[jeff]

sys/kern/vfs_subr.c

3209 ↗

(On Diff #65357)

vdrop_free() ?

[mjg]

sys/kern/vfs_subr.c

3209 ↗

(On Diff #65357)

vhold would use a clean up of similar nature with splitting the locked/unlocked variants and moving list manipulation elsewhere. with this in mind how about mnt_list_get/mnt_list_put or similar?

3223 ↗

(On Diff #65357)

it would be dependent on VI_DOOMED. i can add the assert after the other change lands

[mjg]

• refactor vhold
• move VI_ACTIVE check early in vdropl

[kib]

Can you commit just the move of the block of code to free the vnode into freevnode() ? The rest of changes would be easier to see then, esp. in reshuffling the asserts, if any.

[mjg]

• rebase on head
• put back checking for VI_ACTIVE. the invariant does not hold because vnlru reclaim can bump hold count without actually moving lists and later abort it if someone else bumped usecount. this is a bug since a used vnodes does not land on the active list. I'll propose a patch which happens to eliminate the problem as a side effect. In the meantime keep the bug-to-bug compatibility

[kib]

Again, can vdrop_deactivate() and perhaps vhold_activate() committed separately ?

[mjg]

vdrop_deactivate contains the material change of not longer accepting mp == NULL and there are no functional changes past that.

asserts:

• production kernels no longer assert anything
• kernels with debug only have added assert that vnode is not doomed in vdrop_deactivate

[kib]

In D22722#497647, @mjg wrote:

vdrop_deactivate contains the material change of not longer accepting mp == NULL and there are no functional changes past that.

asserts:

• production kernels no longer assert anything
• kernels with debug only have added assert that vnode is not doomed in vdrop_deactivate

Which is exactly the reason to split the move of existing code and then make changes listed above a separate diff.

[mjg]

• whack the other stuff. only material changes remain.

[jeff]

https://reviews.freebsd.org/D12126

This was the review that added support for mp == null. I don't understand why we needed it then or why we don't need it now. What conditions changed?

[mjg]

Huh, I should have checked where it came from, I assumed it was stale.

head definitely does not need it. If Isilon needs it for anything, they should provide a fake mount instead. Handling possibly NULL mp will only add cruft to future changes.

[kib]

sys/kern/vfs_subr.c

3198 ↗

(On Diff #65442)

This comment is stale, you need to move it to the proper place.

[mjg]

@bdrewery is the NULL check stuff you still use? can you add a mount point to your vnodes instead?

[mjg]

pho tested this, no issues.

[kib]

I am fine with the code change. Hopefully you will get the answer from Isilon (or move forward if not).

[mjg]

@bdrewery ping