On Fri, Jan 18, 2019 at 08:57:14AM +0100, Christian Brauner wrote: > since we want to have each binderfs mount to be a new instance. > That means binderfs_set_super() and binderfs_test_super are gone. > So there's no weird mucking with sget_userns() and s_fs_info anymore > going on in binderfs_mount(). > That means your points from 5) to 9) should be taken care of. > > I have already a patch that remove unnecessary checks and an invalid > comment that you pointed out int 10) to 12). > > I'm currently fixing the double put issues 1) to 5) based on your > suggestions from your private mail. Thanks for that! OK... As for the cleanups on failure exits, the basic idea is to (re)use the cleanups ->kill_sb() has to do anyway. And AFAICS for binderfs it's as simple as * grab ipcns reference right after you've allocated ->s_fs_info and stick it in there right away. * make all failure exits in binderfs_fill_super() plain returns * make binderfs_kill_super() pick ->s_fs_info, do kill_litter_super(), then, if info wasn't NULL, drop ipcns ref and free info. All there is to it... We used to have tons of code duplication between the failure exits of mount and normal behaviour on umount; that caused a lot of rarely tested boilerplate code. One can still follow that model (stick the teardown on umount into ->put_super(), duplicate its parts on failure exits in ->mount()), but it can be more convenient to use an explicit ->kill_sb() instead. That's the reason why ->kill_sb() is always called for anything that made it out of sget(), whether fully set up or not. You still need to be careful about doing initialization and teardown in compatible orders, but usually that's not hard to do. Sure, it's not guaranteed to turn all failure exits into plain returns (e.g. if one does a temporary allocation that is freed in the end on setup, ->kill_sb() won't be freeing it, so any failure exits will have to take care of that), but the amount and complexity of cleanups is less that way. The same considerations, BTW, had been the reason for calling conventions of d_make_root() - simpler cleanup on failures that way...
