Re: [PATCH 00/17] clean up readlinks

[Al Viro <viro@ZenIV.linux.org.uk>]

On Tue, Sep 27, 2016 at 11:38:33AM +0200, Miklos Szeredi wrote:
> >         I have no problem with "let's get rid of generic_readlink" - not that
> > it bought us much, but sure, if you want to have decision made based upon
> > the combination of flags, let's do it.  Just make NULL ->readlink + non-NULL
> > ->get_link() mean generic_readlink(), and we are done.
> 
> Indeed.  Except it really should be the other way round:
> 
> - .get_link always returning the symlink body
> - only proc setting .jump_link to do its thing
> - RIP .readlink

> But that's an extra branch in the symlink following.  I was worried
> about that and hence gone for the unification of the two.

Symlink traversal is a much hotter path than readlink() would ever be.
What's more, we do have jumps on normal symlink traversal - after all,
absolute symlinks are exactly that; it's "jump to root, then traverse
the following sequence of components".  So having ->get_link() that
includes jumps is not that much of a stretch (note that it could both
jump and return a relative pathname to traverse after that; none of the
procfs-style ones do that, but there's no reason to prohibit that).

What I'd prefer is
	* it's a symlink iff it has ->get_link()
	* readlink(2) on a symlink is normally just using generic_readlink()
	* that can be overridden by supplying a ->readlink() method.
	* the first time readlink() hits a symlink it will check both
->get_link() and ->readlink() presence.  Then, if it's a normal symlink,
the inode will get marked as such and all subsequent calls will just call
generic_readlink().  IOW, I would go for
	if (unlikely(!marked)) {
		if ->readlink is present
			call ->readlink and return
		if ->get_link is absent
			fail
		mark
	}
	call generic_readlink

> Yeah.  We can do your above suggestion, it's certainly less brittle.
> But I think it's rather confusing, having ->get_link normally do
> readlink, except for proc, where readlink is done by ->readlink.

	->readlink() is just an override for the cases when readlink(2) wants
to fake something (or, as in case of AFS ugliness, is used on non-symlinks).
The primary function of symlinks is traversal, not readlink...