From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from zeniv.linux.org.uk ([195.92.253.2]:34598 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755538AbcI1CR3 (ORCPT ); Tue, 27 Sep 2016 22:17:29 -0400 Date: Wed, 28 Sep 2016 03:17:27 +0100 From: Al Viro To: Miklos Szeredi Cc: linux-fsdevel , lkml , David Howells , Tyler Hicks Subject: Re: [PATCH 00/17] clean up readlinks Message-ID: <20160928021727.GN19539@ZenIV.linux.org.uk> References: <1473708559-12714-1-git-send-email-mszeredi@redhat.com> <20160927031019.GF19539@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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...