Re: [PATCH 01/16] devpts: Attempting to get it right

[ebiederm@xmission.com (Eric W. Biederman)]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> So I've looked at the devpts patches some more, and I'm still not happy 
> with them.
>
> And one thing I really detest about them is that the 16-patch series 
> didn't really make me warm and fuzzy in general. Some of the patches were 
> fine, but on the whole it all still looked rather hacky.
>
> So I started looking at the code with the intent of trying to clean things 
> up _gradually_, knowing roughly where we want to end up, but also trying 
> to make single patches that look sane on their own, and can be judged on 
> their own without any other patches or even any semantic arguments.
>
> And this appended patch is I think the first step.
>
> What this does is get rid of the horrible notion of having that
>
>     struct inode *ptmx_inode
>
> be the interface between the pty code and devpts. By de-emphasizing the 
> ptmx inode, a lot of things actually get cleaner, and we will have a much 
> saner way forward.
>
> The patch itself is actually fairly straightforward, and apart from some 
> locking cleanups it's pretty mechanical:
>
>  - the interfaces that devpts exposes all take "struct pts_fs_info *" 
>    instead of "struct inode *ptmx_inode" now.
>
>    NOTE! The "struct pts_fs_info" thing is a completely opaque structure 
>    as far as the pty driver is concerned: it's still declared entirely 
>    internally to devpts. So the pty code can't actually access it in any 
>    way, just pass it as a "cookie" to the devpts code.
>
>  - the "look up the pts fs info" is now a single clear operation, that 
>    also does the reference count increment on the pts superblock.
>
>    So "devpts_add/del_ref()" is gone, and replaced by a "lookup and get 
>    ref" operation (devpts_get_ref(inode)), along with a "put ref" op
>    (devpts_put_ref()).
>
>  - the pty master "tty->driver_data" field now contains the pts_fs_info, 
>    not the ptmx inode.
>
>  - because we don't care about the ptmx inode any more as some kind of 
>    base index, the ref counting can now drop the inode games - it just 
>    gets the ref on the superblock.
>
>  - the pts_fs_info now has a back-pointer to the super_block. That's so 
>    that we can easily look up the information we actually need. Although 
>    quite often, the pts fs info was actually all we wanted, and not having 
>    to look it up based on some magical inode makes things more
>    straightforward.
>
> Now, I haven't actually *tested* the patch very much: it compiles, it 
> boots, and my (fairly limited) environment still works, but that's by no 
> means exhaustive. So I may have screwed something up, but most of this was 
> really fairly straightforward.
>
> But more importantly, I think it all makes sense independently of anything 
> else. In particular, now that "devpts_get_ref(inode)" operation should 
> really be the *only* place we need to look up what devpts instance we're 
> associated with, and we do it exactly once, at ptmx_open() time.
>
> So I think this is a good step forward, while avoiding anything that could 
> be at all controversial.
>
> Comments about the patch?

Mostly I think it is six of one half dozen of the other, but given
driver_data is used so few times and that those times are very clear
whether we are accessing a master or a slave worth doing just for
clarity.

I have work inspired by this rolled into my code.  I will post shortly
after a little more testing.

Eric