vfs: provide F_ISUNIONSTACK as a kludge for libc
ClosedPublic
Actions

Authored by mjg on Jan 14 2020, 12:49 AM.

Details

Reviewers

kib
jeff

Group Reviewers

manpages

Commits

rS356830: vfs: provide F_ISUNIONSTACK as a kludge for libc

Summary

See the comments in the patch. Given that the problem is not going away any time soon the least we can do is lessen the impact. readdir calls this over 180k times during buildkernel.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

mjg created this revision.Jan 14 2020, 12:49 AM

Herald added a subscriber: imp. · View Herald TranscriptJan 14 2020, 12:49 AM

Harbormaster completed remote builds in B28648: Diff 66712.Jan 14 2020, 12:49 AM

mjg added inline comments.Jan 14 2020, 12:53 AM

sys/kern/kern_descrip.c
856 ↗	(On Diff #66712)	In principle this is buggy. While we never clear ->mnt_vfc, it can race enough that it points to a filesystem module which is now being unloaded. This can be fixed by providing a dummy "unallocated vfc" to be set in mount destroy.

play it safer and vfs_busy around it. it's not a big deal since it's per-cpu.

kib added inline comments.Jan 16 2020, 2:41 PM

lib/libc/gen/opendir.c

276 ↗

(On Diff #66713)

s/int/bool/

290 ↗

(On Diff #66713)

return (false); instead of goto

293 ↗

(On Diff #66713)

should be on this line. Also style suggests using strcmp() == 0.

sys/kern/kern_descrip.c

832 ↗

(On Diff #66713)

Some variant of this text must appear in fcntl(2) and not in the comment.

863 ↗

(On Diff #66713)

Since you are tweaking this part, add a MNTK_UNION_FS flag and check for it instead of doing strcmp.

This would also remove any vague reasoning behind vfs_busy() usefulness.

kib added inline comments.Jan 16 2020, 2:42 PM

lib/libc/gen/opendir.c
293 ↗	(On Diff #66713)	This is weird phab formatting of the comment. It should have been `\|\|` should be on this line. Also ...

mjg added inline comments.Jan 16 2020, 6:50 PM

lib/libc/gen/opendir.c
276 ↗	(On Diff #66713)	this can't be bool as the routine returns -1/0/1
293 ↗	(On Diff #66713)	ok, this was simply copied from the original
sys/kern/kern_descrip.c
832 ↗	(On Diff #66713)	I don't think this should be "advertised". The comment in the header says to not use it.
863 ↗	(On Diff #66713)	you mean literally a dedicated flag just for unionfs? so the check would be mnt_flag & (MNT_UNION \| MNT_UNION_FS)?

style
manpage
add MNTK_UNIONFS flag

kib accepted this revision.Jan 17 2020, 10:58 AM

kib added inline comments.

lib/libc/gen/opendir.c
276 ↗	(On Diff #66713)	I think for the case of fstatfs failure more reasonable behavior is to treat the directory as not having union(fs) property. This note holds both for old and new code.
lib/libc/sys/fcntl.2
189 ↗	(On Diff #66872)	lag ? Did you mean 'flag' ?. I think it is better to remove braces, and say '...part of a union stack, either the "union" ...' .
190 ↗	(On Diff #66872)	Start the new sentence from a new line. Bump .Dd.