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...
