Change Details

Commit ab05a1cf321aca0f intended to revert commit 8733bc277a383cf5 ("vfs: don't provoke recycling non-free vnodes without a good reason"), but due to intervening changes in commit 054f45e026d898bd ("vfs: further speed up continuous free vnode recycle"), it also had to revert part of it. In particular, while removing the whole 'if (vn_alloc_cyclecount != 0)' block, it inadvertantly removed the code block resetting 'vn_alloc_cyclecount' to 0 and skipping direct vnode reclamation (done further below through vnlru_free_locked_direct()), which had been outside the 'if' before the intervening commit. Removing this block instead of reinstating it in practice causes 'vn_alloc_cyclecount' to (almost) never be 0, making vn_alloc() always call vn_alloc_hard(), which takes the 'vnode_list_mtx' mutex. In other words, this disables the fast path. [The reverted commit, which introduced the 'if (vn_alloc_cyclecount != 0)' guarding this block, actually never executed it because it also had the bug that 'vn_alloc_cyclecount' would always stay at 0, hiding its usefulness.] Additionally, not skipping direct vnode reclamation even when there are less vnodes than 'kern.maxvnodes' not only causes unnecessary contention but also plain deadlocks as vnlru_free_locked_direct() does not itself check whether there are actually "free" (not referenced) vnodes to be deallocated, and will blindly browse all the vnode list until it finds one (which it may not, or only a few ones at the end). Not skipping direct vnode reclamation was arguably a bug introduced by the intervening commit (054f45e026d898bd), but was mitigated by vn_alloc_hard() not being called in the fast path. The revert commit, by disabling the fast path, made it significantly annoying (to the point of getting 1 to 2 deadlocks/livelocks a week in some of my workloads). Restore the reset of 'vn_alloc_cyclecount' to 0 and skip direct reclamation when the current number of vnodes is below the 'kern.maxvnodes' limit, indicating we can start allocating a new 'struct vnode' right away. While here, fix the comparison with the limit when 'bumped' is true. Fixes: ab05a1cf321aca0f (revert of "vfs: don't provoke recycling non-free vnodes without a good reason") Fixes: 054f45e026d898bd ("vfs: further speed up continuous free vnode recycle")

Commit ab05a1cf321aca0f intended to revert commit 8733bc277a383cf5 ("vfs: don't provoke recycling non-free vnodes without a good reason"), but due to intervening changes in commit 054f45e026d898bd ("vfs: further speed up continuous free vnode recycle"), it also had to revert part of it. In particular, while removing the whole 'if (vn_alloc_cyclecount != 0)' block, it inadvertantly removed the code block resetting 'vn_alloc_cyclecount' to 0 and skipping direct vnode reclamation (done further below through vnlru_free_locked_direct()), which had been outside the 'if' before the intervening commit. Removing this block instead of reinstating it in practice causes 'vn_alloc_cyclecount' to (almost) never be 0, making vn_alloc() always call vn_alloc_hard(), which takes the 'vnode_list_mtx' mutex. In other words, this disables the fast path. [The reverted commit, which introduced the 'if (vn_alloc_cyclecount != 0)' guarding this block, actually never executed it because it also had the bug that 'vn_alloc_cyclecount' would always stay at 0, hiding its usefulness.] Additionally, not skipping direct vnode reclamation even when there are less vnodes than 'kern.maxvnodes' not only causes unnecessary contention but also plain livelocks as vnlru_free_locked_direct() does not itself check whether there are actually "free" (not referenced) vnodes to be deallocated, and will blindly browse all the vnode list until it finds one (which it may not, or only a few ones at the end). As the fast path was disabled, all threads in the system would soon be competing for the vnode list lock, outpacing the vnlru process that could never actually recycle vnodes in a more agressive manner (i.e., even if they have a non-zero hold count). And we could more easily get into this situation, as each vnode allocation was reducing the count of "free" vnodes, even if entirely new vnodes could be allocated instead. This part was mitigated by the vnlru process (before the tipping point described above), which explains why the mechanism would not always livelock. Not skipping direct vnode reclamation was arguably a bug introduced by the intervening commit (054f45e026d898bd), but was mitigated by vn_alloc_hard() not being called in the fast path. The revert commit, by disabling the fast path, made it significantly annoying (to the point of getting a few livelocks a week in some of my workloads). Restore the reset of 'vn_alloc_cyclecount' to 0 and skip direct reclamation when the current number of vnodes is below the 'kern.maxvnodes' limit, indicating we can start allocating a new 'struct vnode' right away. While here, fix the comparison with the limit when 'bumped' is true. Fixes: ab05a1cf321aca0f (revert of "vfs: don't provoke recycling non-free vnodes without a good reason") Fixes: 054f45e026d898bd ("vfs: further speed up continuous free vnode recycle")

Commit ab05a1cf321aca0f intended to revert commit 8733bc277a383cf5 ("vfs: don't provoke recycling non-free vnodes without a good reason"), but due to intervening changes in commit 054f45e026d898bd ("vfs: further speed up continuous free vnode recycle"), it also had to revert part of it. In particular, while removing the whole 'if (vn_alloc_cyclecount != 0)' block, it inadvertantly removed the code block resetting 'vn_alloc_cyclecount' to 0 and skipping direct vnode reclamation (done further below through vnlru_free_locked_direct()), which had been outside the 'if' before the intervening commit. Removing this block instead of reinstating it in practice causes 'vn_alloc_cyclecount' to (almost) never be 0, making vn_alloc() always call vn_alloc_hard(), which takes the 'vnode_list_mtx' mutex. In other words, this disables the fast path. [The reverted commit, which introduced the 'if (vn_alloc_cyclecount != 0)' guarding this block, actually never executed it because it also had the bug that 'vn_alloc_cyclecount' would always stay at 0, hiding its usefulness.] Additionally, not skipping direct vnode reclamation even when there are less vnodes than 'kern.maxvnodes' not only causes unnecessary contention but also plain deadlivelocks as vnlru_free_locked_direct() does not itself check whether there are actually "free" (not referenced) vnodes to be deallocated, and will blindly browse all the vnode list until it finds one (which it may not, or only a few ones at the end). Not skippingAs the fast path direct vnode reclamation was arguably a bug introduced bywas disabled, all threads in the system would soon be competing for the intervening commit (054f45e026d898bd)vnode list lock, but was mitigated byoutpacing the vnlru process that could never actually recycle vnodes in a more agressive manner (i.e., even if they have a non-zero hold count). And we could more easily get into this situation, as each vnode allocation was reducing the count of "free" vnodes, even if entirely new vnodes could be allocated instead. This part was mitigated by the vnlru process (before the tipping point described above), which explains why the mechanism would not always livelock. Not skipping direct vnode reclamation was arguably a bug introduced by the intervening commit (054f45e026d898bd), but was mitigated by vn_alloc_hard() not being called in the fast path. The revert commit, by disabling the fast path, made it significantly annoying (to the point of getting 1 to 2 deadlocks/a few livelocks a week in some of my workloads). Restore the reset of 'vn_alloc_cyclecount' to 0 and skip direct reclamation when the current number of vnodes is below the 'kern.maxvnodes' limit, indicating we can start allocating a new 'struct vnode' right away. While here, fix the comparison with the limit when 'bumped' is true. Fixes: ab05a1cf321aca0f (revert of "vfs: don't provoke recycling non-free vnodes without a good reason") Fixes: 054f45e026d898bd ("vfs: further speed up continuous free vnode recycle")