Page MenuHomeFreeBSD

Add clearing function for unr(9), call it from tmpfs.
ClosedPublic

Authored by mjoras on Oct 4 2017, 6:38 PM.

Details

Summary

Previously before you could call unrhdr_delete you needed to
individually free every allocated unit. It is useful to be able to tear
down the unr without having to go through this process, as it is
significantly faster than freeing the individual units.

When unmounting a tmpfs, do not call free_unr.

tmpfs uses unr(9) to allocate inodes. Previously when unmounting it
would individually free the units when it freed each vnode. This is
unnecessary as we can use the newly-added unrhdr_clear function to clear
out the unr in onde go. This measurably reduces the time to unmount a
tmpfs with many files.

This will be committed as two pieces, one for the unr(9) change, one for the
tmpfs usage change.

Test Plan

I tested this using a kld that uses unr, and I also tested it by repeatedly
extracting a ports tree to a tmpfs and umounting that tmpfs. In my testing
the unmount is sped up from ~.31s to ~.24s, on average. Memory does not
appear to be leaking given the output of vmstat -m | grep Unit

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mjoras created this revision.Oct 4 2017, 6:38 PM
mjoras edited the summary of this revision. (Show Details)Oct 4 2017, 7:03 PM
cem added a subscriber: cem.Oct 4 2017, 7:16 PM
cem added inline comments.
share/man/man9/unr.9
77 ↗(On Diff #33700)

Sentences should start on their own line in man page format. A suggestion: use igor from ports to check man pages you write/edit; it should flag this.

sys/fs/tmpfs/tmpfs_subr.c
365 ↗(On Diff #33700)

Why does this become conditional?

sys/kern/subr_unit.c
386–387 ↗(On Diff #33700)

Why not move the assert to the top of the routine? It doesn't seem like the rest of the function affects ppfree.

mjoras added inline comments.Oct 4 2017, 7:43 PM
share/man/man9/unr.9
77 ↗(On Diff #33700)

Thanks, I knew I'd commit some sort of manpage faux pas on my first manpage change :).

sys/fs/tmpfs/tmpfs_subr.c
365 ↗(On Diff #33700)

detach -> unmount, so in the unmount case we don't want to bother freeing the unit for every single inode. Instead we clear it out all at once. Should I add a comment? There's basically no comments in tmpfs, it's pretty sad.

cem added inline comments.Oct 4 2017, 8:00 PM
sys/fs/tmpfs/tmpfs_subr.c
365 ↗(On Diff #33700)

Ah, that makes sense. Yeah, please add a comment.

mjoras updated this revision to Diff 33701.Oct 4 2017, 8:02 PM
  • Fixup manpage.
  • Move KASSERT.
cem added a comment.Oct 4 2017, 8:04 PM

I'd like a comment about the !detach-only free_unr but otherwise LGTM.

mjoras updated this revision to Diff 33702.Oct 4 2017, 8:05 PM
mjoras marked 2 inline comments as done.
  • Add comment explaining the conditional free_unr
mjoras marked an inline comment as done.Oct 4 2017, 8:05 PM
cem accepted this revision.Oct 4 2017, 8:19 PM
This revision is now accepted and ready to land.Oct 4 2017, 8:19 PM
lidl accepted this revision as: lidl.Oct 5 2017, 4:00 AM
rstone accepted this revision.Oct 11 2017, 9:23 PM
This revision was automatically updated to reflect the committed changes.