Page MenuHomeFreeBSD

vfs: provide F_ISUNIONSTACK as a kludge for libc
ClosedPublic

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mjg created this revision.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.

mjg updated this revision to Diff 66713.Jan 14 2020, 1:09 AM
  • 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)?

mjg updated this revision to Diff 66872.Jan 16 2020, 10:46 PM
  • 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.

This revision is now accepted and ready to land.Jan 17 2020, 10:58 AM
This revision was automatically updated to reflect the committed changes.