Page MenuHomeFreeBSD

Add devfs(5) support for VOP_MKDIR(9) and VOP_RMDIR(9)
Needs ReviewPublic

Authored by trasz on May 25 2019, 7:34 PM.

Details

Reviewers
des
kib
Summary

Add devfs(5) support for VOP_MKDIR(9) and VOP_RMDIR(9).
This is required for Linuxulator (to be able to create
/dev/shm mountpoint), and would have been useful for
reroot, instead of the /dev/reroot/reroot hack.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 24509
Build 23306: arc lint + arc unit

Event Timeline

trasz created this revision.May 25 2019, 7:34 PM
kib added a comment.May 25 2019, 8:13 PM

How did you tested this ? I am curious if you tried massively-parallel mkdir/rmdir for the same name. Perhaps try to ask Peter Holm to test this.

mjg added a subscriber: mjg.May 25 2019, 8:50 PM

I think arbitrary mkdir/rmdir is a can of worms, perfectly avoidable for the stated purpose. lindevfs module could be created to extend devfs mount points with whatever is necessary. Preferably this would be a completely separate fs, but that's probably too problematic. i.e. currently the module would make_dev("shm") on it's own. the func can create directories and if it insists on getting a device, a separate variant can be added.

tijl added a subscriber: tijl.May 26 2019, 10:31 AM
In D20411#440580, @mjg wrote:

I think arbitrary mkdir/rmdir is a can of worms, perfectly avoidable for the stated purpose.

Arbitrary symlinks are already supported. Why would mkdir be any different?

kib added a comment.May 26 2019, 10:40 AM
In D20411#440658, @tijl wrote:
In D20411#440580, @mjg wrote:

I think arbitrary mkdir/rmdir is a can of worms, perfectly avoidable for the stated purpose.

Arbitrary symlinks are already supported. Why would mkdir be any different?

Indeed, I think this position should be elaborated. I do not see much problems with allowing directories creation, but I think that implementation might be much simpler e.g. by making a pseudo-driver which can be directed by VOP_MKDIR to create the <path to dir>/.hidden node. Anyway, I am curious about the testing results for the provided patch.

trasz added a comment.EditedMay 26 2019, 10:56 AM
In D20411#440660, @kib wrote:
In D20411#440658, @tijl wrote:
In D20411#440580, @mjg wrote:

I think arbitrary mkdir/rmdir is a can of worms, perfectly avoidable for the stated purpose.

Arbitrary symlinks are already supported. Why would mkdir be any different?

Good point. Also, this is a root-only interface.

Indeed, I think this position should be elaborated. I do not see much problems with allowing directories creation, but I think that implementation might be much simpler e.g. by making a pseudo-driver which can be directed by VOP_MKDIR to create the <path to dir>/.hidden node. Anyway, I am curious about the testing results for the provided patch.

I don't see how that would be any simpler, given that you'd need a way to communicate with that driver. Besides - this is just 150 lines of code, largely copy/pasted from devfs_symlink() and devfs_remove().

Also, regarding mjg@'s suggestion: I've tried that, it is simple, but it's got a number of problems: it breaks stuff for people who want /dev/shm to be a symlink; you need to make sure to load linux_common.ko before trying to mount /dev/shm; weird stuff happens when you unload it with /dev/shm still mounted, etc. Making mkdir work is just nicer from the design point of view.

pho added a subscriber: pho.May 26 2019, 4:28 PM

I ran all of the devfs tests I have.
I added a new parallel mkdir() and rmdir() test with VM pressure.
No problems seen.

trasz added a comment.May 27 2019, 1:50 PM
In D20411#440676, @pho wrote:

I ran all of the devfs tests I have.
I added a new parallel mkdir() and rmdir() test with VM pressure.
No problems seen.

Thank you.

kib added a comment.EditedJun 1 2019, 3:01 PM

Imagine that there is no pseudo ttys in the system, and user called mkdir('/dev/pts'). Then a new pty pair is created, which makes a device node in /dev/pts. On device destruction, the node is removed, and its contained directory is removed if empty. But '/dev/pts' was created by user, not my makedev(9).

In fact, the way in which you did it is even more complicated. Your stray directories are attached to specific mount point. This means that the directory visible to some dmp's when they populate but not other, More important, when the last device node in the stray directory is removed, it should be removed only in some dmp's and not another.

sys/fs/devfs/devfs_vnops.c
1079

Put int after pointers.

1087

Space before '('.

1614

Do not use initializations in local declarations block.

1617

These asserts are rather pointless, VOP wrappers do the checks.

1619

You must hold dmp there, because devfs_delete() might drop dmp lock internally.

1628

I believe that you may remove non-empty directory.

1648

You must hold de around devfs_delete call, since it might drop sx_xlock. But I think that you should reconsider your copy/paste and reuse devfs_rmdir_empty() somehow. May be add a flag to the function to indicate that it should not remove parents.

1655

How could de_cdp be non-NULL for a directory ?