Page MenuHomeFreeBSD
Differential D59872

cuse: Improve server cleanup
Needs ReviewPublic
Actions

Authored by christos on Mon, Sep 21, 11:31 AM.
Tags
None
Referenced Files
F173313473: D59872.id187326.diff
Fri, Sep 25, 4:45 AM
Unknown Object (File)
Thu, Sep 24, 9:06 PM
Unknown Object (File)
Thu, Sep 24, 8:32 PM
Unknown Object (File)
Thu, Sep 24, 6:24 PM
Unknown Object (File)
Thu, Sep 24, 4:47 PM
Unknown Object (File)
Thu, Sep 24, 4:05 PM
Unknown Object (File)
Thu, Sep 24, 3:28 PM
Unknown Object (File)
Thu, Sep 24, 2:46 PM
View All 14 Files
Subscribers
imp

Details

Reviewers
markj
emaste
kib
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 77134
Build 74017: arc lint + arc unit

Event Timeline

christos created this revision.Mon, Sep 21, 11:31 AM
Herald added a subscriber: imp. · View Herald TranscriptMon, Sep 21, 11:31 AM
christos requested review of this revision.Mon, Sep 21, 11:31 AM
Harbormaster completed remote builds in B77134: Diff 187326.Mon, Sep 21, 11:31 AM
kib added a comment.Mon, Sep 21, 12:14 PM

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
301

I think MPASS() is enough there.

661

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

1281

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

christos marked an inline comment as done.Mon, Sep 21, 12:27 PM
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
661

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

1281

D59874

christos updated this revision to Diff 187360.Mon, Sep 21, 9:38 PM
christos marked an inline comment as done.

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

Harbormaster completed remote builds in B77147: Diff 187360.Mon, Sep 21, 9:38 PM
kib added inline comments.Tue, Sep 22, 12:43 PM
sys/fs/cuse/cuse.c
748

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

christos added inline comments.Tue, Sep 22, 3:46 PM
sys/fs/cuse/cuse.c
748

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.

christos added a comment.Fri, Sep 25, 10:28 AM

Is this good to go?

kib added inline comments.Fri, Sep 25, 7:48 PM
sys/fs/cuse/cuse.c
748

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.

Revision Contents
Changeset List

PathSize
sys/
fs/
cuse/
56 lines

Diff 187326

View Options

sys/fs/cuse/cuse.c

Loading...