Page MenuHomeFreeBSD

vfs: change si_usecount management to count used vnodes
Needs ReviewPublic

Authored by mjg on Thu, Oct 31, 12:27 AM.

Details

Reviewers
kib
jeff
Summary

Currently si_usecount is effectively a sum of usecounts from all associated vnodes. This is maintained by special-casing for VCHR every time usecount is modified. Apart from complicating the code a little bit, it has a scalability impact since it forces a read from a cacheline shared with said count.

There are no consumers of the feature in the ports tree. In head there are only 2: revoke and devfs_close. Both can get away with a weaker requirement than the exact usecount, namely just the count of active vnodes. Changing the meaning to the latter means we only need to modify it on 0<->1 transitions, avoiding the check plenty of times (and entirely in something like vrefact).

I did a simple verification: just logging in over ssh and logging out runs into vp->v_usecount == 2 and count_dev() == 2 in stock head, and 2 and 1 respectively with the patch.

Test Plan

pho tested the change, no problems found.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 27291

Event Timeline

mjg created this revision.Thu, Oct 31, 12:27 AM
kib added inline comments.Fri, Nov 1, 8:28 AM
sys/fs/devfs/devfs_vnops.c
594

These two checks for v_usecount == 2 are optimizations and can be committed separately.

OTOH I should note that changing v_usecount updates to atomic make these checks racy.

sys/kern/vfs_syscalls.c
4151

This is just an optimization, right ? It can be tested/committed separately before the rest of the patch is reviewed.

mjg added inline comments.Fri, Nov 1, 8:46 AM
sys/fs/devfs/devfs_vnops.c
594

I think the code was racy even beforehand. Moreover, I suspect there is a different bug here as well: if the target device gets a ref from another devfs mount point, this code will decline clearing it which probably was not intended. At least dragonfly only checks the current vnode and changing this would be a noop for typical use cases. That said, I consider the patch to maintain to be bug compatible.

sys/kern/vfs_syscalls.c
4151

With the patch in place, if this is the only vnode, vcount will return 1 on account of v_usecount > 0. Thus the check is needed to actually revoke if there are other users of the vnode.

mjg edited the test plan for this revision. (Show Details)Sun, Nov 3, 11:58 PM
mjg added a comment.Sat, Nov 16, 3:21 AM

ping? the change passed tests by pho