Re: [PATCH 00/17] clean up readlinks

[Miklos Szeredi]

On Wed, Sep 28, 2016 at 4:17 AM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:

> 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

Redid patchset confirming to the above.

Also moved the overlayfs/ecryptfs fixes to the front of the queue, as
they are independent of the rest of the cleanups.

The last part is new: changing ->readlink signature to match that of
->get_link.  This moves the user buffer handling out of filesystems,
simplifying the implementation in the process.

Force pushed new set to:

  git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git #readlink

It's got a couple of trivial conflicts against #rename2 and
#overlayfs-next.  I can do the merge and send a big pull request if
you want.

Thanks,
Miklos

---
Miklos Szeredi (14):
      vfs: add vfs_get_link() helper
      ovl: use vfs_get_link()
      ecryptfs: use vfs_get_link()
      ovl: use generic_readlink
      proc/self: use generic_readlink
      bad_inode: use generic_readlink
      vfs: replace calling i_op->readlink with vfs_readlink()
      vfs: default to generic_readlink()
      vfs: remove ".readlink = generic_readlink" assignments
      vfs: make generic_readlink() static
      vfs: convert ->readlink to same signature as ->get_link
      vfs: remove page_readlink()
      vfs: make readlink_copy() static
      nsfs: clean up ns_get_name() interface

---
 Documentation/filesystems/Locking             |  6 +-
 Documentation/filesystems/porting             |  7 +++
 Documentation/filesystems/vfs.txt             | 14 +++--
 drivers/staging/lustre/lustre/llite/symlink.c |  1 -
 fs/9p/vfs_inode.c                             |  1 -
 fs/9p/vfs_inode_dotl.c                        |  1 -
 fs/affs/symlink.c                             |  1 -
 fs/afs/mntpt.c                                |  2 +-
 fs/autofs4/symlink.c                          |  1 -
 fs/bad_inode.c                                |  7 ---
 fs/btrfs/inode.c                              |  1 -
 fs/ceph/inode.c                               |  1 -
 fs/cifs/cifsfs.c                              |  1 -
 fs/coda/cnode.c                               |  1 -
 fs/configfs/symlink.c                         |  1 -
 fs/ecryptfs/inode.c                           | 30 ++++------
 fs/ext2/symlink.c                             |  2 -
 fs/ext4/symlink.c                             |  3 -
 fs/f2fs/namei.c                               |  2 -
 fs/fuse/dir.c                                 |  1 -
 fs/gfs2/inode.c                               |  1 -
 fs/hostfs/hostfs_kern.c                       |  1 -
 fs/jffs2/symlink.c                            |  1 -
 fs/jfs/symlink.c                              |  2 -
 fs/kernfs/symlink.c                           |  1 -
 fs/libfs.c                                    |  1 -
 fs/minix/inode.c                              |  1 -
 fs/namei.c                                    | 83 +++++++++++++++++++--------
 fs/ncpfs/inode.c                              |  1 -
 fs/nfs/symlink.c                              |  1 -
 fs/nfsd/nfs4xdr.c                             |  8 +--
 fs/nfsd/vfs.c                                 |  6 +-
 fs/nilfs2/namei.c                             |  1 -
 fs/nsfs.c                                     | 17 ++++--
 fs/ocfs2/symlink.c                            |  1 -
 fs/orangefs/symlink.c                         |  1 -
 fs/overlayfs/copy_up.c                        | 46 ++-------------
 fs/overlayfs/inode.c                          | 30 +---------
 fs/proc/base.c                                | 57 +++++++-----------
 fs/proc/inode.c                               |  1 -
 fs/proc/namespaces.c                          | 15 +++--
 fs/proc/self.c                                | 13 -----
 fs/proc/thread_self.c                         | 14 -----
 fs/reiserfs/namei.c                           |  1 -
 fs/squashfs/symlink.c                         |  1 -
 fs/stat.c                                     |  8 ++-
 fs/sysv/inode.c                               |  1 -
 fs/ubifs/file.c                               |  1 -
 fs/xfs/xfs_ioctl.c                            |  4 +-
 fs/xfs/xfs_iops.c                             |  2 -
 include/linux/fs.h                            | 10 ++--
 include/linux/proc_ns.h                       |  4 +-
 mm/shmem.c                                    |  2 -
 53 files changed, 157 insertions(+), 264 deletions(-)