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

Lint
Lint Skipped
Unit
Unit Tests Skipped

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

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

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

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

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

lag ? Did you mean 'flag' ?.

I think it is better to remove braces, and say '...part of a union stack, either the "union" ...' .

190

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.