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

Lint

Lint Skipped

Unit

Tests Skipped

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	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

s/int/bool/

290

return (false); instead of goto

293

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

sys/kern/kern_descrip.c

832

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

863

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	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	this can't be bool as the routine returns -1/0/1
293	ok, this was simply copied from the original
sys/kern/kern_descrip.c
832	I don't think this should be "advertised". The comment in the header says to not use it.
863	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	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.