Page MenuHomeFreeBSD

cuse: Improve server cleanup
ClosedPublic

Authored by christos on Mon, Sep 21, 11:31 AM.
Tags
None
Referenced Files
F174836234: D59872.id188171.diff
Tue, Oct 6, 9:09 AM
F174814789: D59872.diff
Tue, Oct 6, 4:19 AM
F174784885: D59872.diff
Mon, Oct 5, 11:49 PM
Unknown Object (File)
Mon, Oct 5, 10:45 AM
Unknown Object (File)
Mon, Oct 5, 9:41 AM
Unknown Object (File)
Mon, Oct 5, 3:29 AM
Unknown Object (File)
Mon, Oct 5, 3:17 AM
Unknown Object (File)
Mon, Oct 5, 12:20 AM
Subscribers

Details

Summary

Factor out cuse_server_unref()'s device cleanup look into a new
cuse_server_free_devs_locked(), and use it in cuse_server_free() too.

In cuse_kern_init(), delete the infinite loop which waits for all open
/dev/cuse instances to exit, and instead call destroy_dev() directly,
which runs their cdevpriv destructor.

MFC after: 1 week
Sponsored by: The FreeBSD Foundation

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 77407
Build 74290: arc lint + arc unit

Event Timeline

BTW, there is almost always a blank line after each code line in cuse.c. It eats the screen space without giving any clarity.

sys/fs/cuse/cuse.c
295

I think MPASS() is enough there.

656

Again, I think you need to destroy_dev() before iterating over the hcli tailq.

1277

I think you should use make_dev_s() there to set si_drv1 atomically when creating the device.

In D59872#1373978, @kib wrote:

BTW, there is almost always a blank line after each code line in cuse.c. It eats the screen space without giving any clarity.

I will do a style(9) and blank line clean up at some point.

sys/fs/cuse/cuse.c
656

The hcli loop sets is_closing, so why would we want to destroy_dev() first?

1277
christos marked an inline comment as done.

Address Konstantin's comments, minus the destroy_dev() comment.

sys/fs/cuse/cuse.c
744

I do not understand this block. Isn't cuse_server_unref() does the same but conditional on the refs?

sys/fs/cuse/cuse.c
744

Seems like that yes. However, cuse_server_unref() performs the full cleanup only when the server's refs are 0 (see comment above cuse_server_unref() below, otherwise it just decrements the ref.

sys/fs/cuse/cuse.c
744

But we should not free() until the ref count goes to zero. This was my point. And if the ref count is zero, calling unref is pointless and probably buggy.

Also, the code probably should grow the checks that we do not unref more than there are references (lile KASSERT(refs > 0) before decrementing) and same for overflow on referencing.

sys/fs/cuse/cuse.c
744

I will add the KASSERTs in a follow-up patch. However, the thing with cuse_server_free() is that it is the cdevpriv destructor callback, not a generic cleanup function. The point is that when the cdevpriv closes, we want to free the devices belonging to the cdevpriv, and as a last step, call cuse_server_unref() which will take the refcount to 0 and perform the full cleanup for the server.

The redundant thing here is that cuse_server_unref() also calls cuse_server_free_devs_locked().

sys/fs/cuse/cuse.c
744

So can we eliminate the redundancy? Also, can we assert that the ref count is zero when freeing?

christos added inline comments.
sys/fs/cuse/cuse.c
744

can we assert that the ref count is zero when freeing?

D60043

christos marked an inline comment as done.

Address Konstantin's comments.

This revision is now accepted and ready to land.Wed, Sep 30, 1:10 AM
This revision was automatically updated to reflect the committed changes.