Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1543817 > unrolled thread

[GIT PULL (resend)] readlink cleanup

Back to article view | Back to linux.kernel

Contents

  [GIT PULL (resend)] readlink cleanup Miklos Szeredi <miklos@szeredi.hu> - 2016-12-16 23:50 +0100
    Re: [GIT PULL (resend)] readlink cleanup Al Viro <viro@ZenIV.linux.org.uk> - 2016-12-17 00:10 +0100
      Re: [GIT PULL (resend)] readlink cleanup Miklos Szeredi <miklos@szeredi.hu> - 2016-12-17 09:50 +0100
    Re: [GIT PULL (resend)] readlink cleanup Linus Torvalds <torvalds@linux-foundation.org> - 2016-12-18 04:30 +0100
      Re: [GIT PULL (resend)] readlink cleanup Linus Torvalds <torvalds@linux-foundation.org> - 2016-12-18 04:30 +0100

#1543817 — [GIT PULL (resend)] readlink cleanup

Al, Linus,

I've got the ACK from Al for this set, except for the readlink signature
conversion.  But I firmly believe that that's also a valid cleanup, so here
goes the unmodified resend.

There's a conflict in ubifs, the obvious resolution is to use the new value
of .get_link and remove the .readlink assignment.

Please consider pulling the following cleanup:

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

This is a rework of the readlink cleanup patchset from the last cycle.  Now
readlink(2) does the following:

 - if i_op->readlink() is non-NULL (only proc and afs mountpoints for now)
   then it calls that

 - otherwise call i_op->get_link()

 - signature of ->readlink() now matches that of ->get_link()

In particular this last bullet point buys us:

 - less complexity, because we already handle the delayed free of the
   buffer and copying to userspace due to ->get_link() being the normal way
   to read the symlink

 - a cleanup of the proc namespace interface

Thanks,
Miklos

---
Miklos Szeredi (10):
      ecryptfs: use vfs_get_link()
      proc/self: 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                                |  8 +---
 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                                    | 58 ++++++++++++++++-----------
 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/inode.c                          |  1 -
 fs/proc/base.c                                | 57 +++++++++-----------------
 fs/proc/inode.c                               |  1 -
 fs/proc/namespaces.c                          | 13 +++---
 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                            |  9 ++---
 include/linux/proc_ns.h                       |  4 +-
 mm/shmem.c                                    |  2 -
 52 files changed, 124 insertions(+), 195 deletions(-)

[toc] | [next] | [standalone]

#1543821

On Fri, Dec 16, 2016 at 11:48:59PM +0100, Miklos Szeredi wrote:

> This is a rework of the readlink cleanup patchset from the last cycle.  Now
> readlink(2) does the following:
> 
>  - if i_op->readlink() is non-NULL (only proc and afs mountpoints for now)
>    then it calls that
> 
>  - otherwise call i_op->get_link()
> 
>  - signature of ->readlink() now matches that of ->get_link()
> 
> In particular this last bullet point buys us:
> 
>  - less complexity, because we already handle the delayed free of the
>    buffer and copying to userspace due to ->get_link() being the normal way
>    to read the symlink

Less complexity where, exactly?  In the caller the life does not become
any simpler - instead of "call ->readlink() and bugger off" you have
"call ->readlink() and go through the same motions as in ->get_link()-based
case".  In the instances it becomes _more_ complex.

What's more, this new signature for ->readlink() makes no sense - instead of
"symlink traversal does not involve resolving a pathname, so we have to
fake one for readlink(2)" you get something resembling ->get_link(), which
would _not_ function as ->get_link() ought to.  But it can be called by the
same codepath that calls ->get_link(), saving us the burden of returning
without doing what ->get_link-based case would - we still get to check if
->readlink() is there, but we rejoin the common path immediately.  And AFAICS
that's the _only_ benefit of that signature change - making it possible to
reuse a few lines that adapt ->get_link() to readlinkat(2) needs.

IOW, I'm still not convinced.  Beginning of the series is fine - having
NULL ->readlink() interpreted for symlinks as "no override, use
generic_readlink()" makes a lot of sense.  This part, IMO, does not.

[toc] | [prev] | [next] | [standalone]

#1543940

On Sat, Dec 17, 2016 at 12:08 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Fri, Dec 16, 2016 at 11:48:59PM +0100, Miklos Szeredi wrote:
>
>> This is a rework of the readlink cleanup patchset from the last cycle.  Now
>> readlink(2) does the following:
>>
>>  - if i_op->readlink() is non-NULL (only proc and afs mountpoints for now)
>>    then it calls that
>>
>>  - otherwise call i_op->get_link()
>>
>>  - signature of ->readlink() now matches that of ->get_link()
>>
>> In particular this last bullet point buys us:
>>
>>  - less complexity, because we already handle the delayed free of the
>>    buffer and copying to userspace due to ->get_link() being the normal way
>>    to read the symlink
>
> Less complexity where, exactly?  In the caller the life does not become
> any simpler - instead of "call ->readlink() and bugger off" you have
> "call ->readlink() and go through the same motions as in ->get_link()-based
> case".  In the instances it becomes _more_ complex.

Have you looked?  Because in actual fact they don't.

 Theoretically it's either:

 - kmalloc + fill + readlink_copy + kfree  -->  kmalloc + fill +
set_delayed_call

 - declare char[] on stack + fill + readlink_copy --> kmalloc + fill +
set_delayed_call

Presumably it's the second one you are talking about becoming more
complex.  There's exactly one instance of that in the tree and it
actually becomes cleaner after the change.

Current code does:

  - guess max link size to be 50 (very scientifically I'm sure, but no
explanation given)
  - call filler
  - hope it didn't get truncated

Which becomes:

  - call filler which allocates correctly sized buffer.

> What's more, this new signature for ->readlink() makes no sense - instead of
> "symlink traversal does not involve resolving a pathname, so we have to
> fake one for readlink(2)" you get something resembling ->get_link(), which
> would _not_ function as ->get_link() ought to.  But it can be called by the
> same codepath that calls ->get_link(), saving us the burden of returning
> without doing what ->get_link-based case would - we still get to check if
> ->readlink() is there, but we rejoin the common path immediately.  And AFAICS
> that's the _only_ benefit of that signature change - making it possible to
> reuse a few lines that adapt ->get_link() to readlinkat(2) needs.

With the signature change we get a consistent interface for reading
the contents of symlinks.  With that it will never make sense to play
the stupid get_ds/set_ds() games that we've had.  And no need to
duplicate helper functions, like page_readlink() that is exactly the
same as page_getlink() only for the different interface.  And no need
to export readlink_copy() which is something the filesystems never
actually want to care about.

Having different interfaces for the same thing is going to be more
complex.  I just don't get it what you are opposed to here.

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1544083

On Fri, Dec 16, 2016 at 2:48 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> Please consider pulling the following cleanup:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git readlink

I pulled the non-controversial part of this branch, and made myself a
batch of popcorn for the rest of the discussion,

               Linus

[toc] | [prev] | [next] | [standalone]

#1544084

On Sat, Dec 17, 2016 at 7:21 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I pulled the non-controversial part of this branch, and made myself a
> batch of popcorn for the rest of the discussion,

Side note: I was pretty close to just pulling it all.

Because  do think the rest of the patch series looks quite reasonable,
but I'll wait for Al to comment more.

               Linus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web