[PATCH RFC] nfs: remove nfs_show_devname

[PATCH RFC] nfs: remove nfs_show_devname

[Jeff Layton]

The nfs code will currently construct a devname to show in places like
/proc/mounts by turning a dentry into a path. Unfortunately, that's
somewhat problematic if the user ended up mounting through a symlink on
the server. The devname that then shows up in /proc/mounts now doesn't
match the one that was originally passed into the mount request.

For instance, suppose a server is exporting a filesystem "/export". We
then have this in /export:

    $ ls -ld foo bar
    lrwxrwxrwx. 1 root root 3 Jan  9 13:46 bar -> foo
    drwxr-xr-x. 2 root root 6 Jan  9 13:45 foo

Now, on the client we mount up 'bar':

    # mount server:/export/bar /mnt/server

...and 'df' now shows a different devname altogether.

    server:/export/foo                20G   66M   20G   1% /mnt/server

In the "old days", this didn't really matter since /proc/mounts was just
informational. On modern distros /etc/mtab is usually a symlink to
/proc/mounts so this means that unmounting by devname isn't always
reliable.

This patch just removes all of this code and has nfs just use the
kernel's standard facility for tracking the devname. With this, the
actual device name used to do the mount is displayed in
/proc/self/mounts, /proc/self/mountinfo, and /proc/self/mountstats.

Reported-by: Yin.JianHong <jiyin@redhat.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/internal.h  |  1 -
 fs/nfs/nfs4super.c |  1 -
 fs/nfs/super.c     | 18 ------------------
 3 files changed, 20 deletions(-)

diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 8b5cc04..4e17d3f 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -403,7 +403,6 @@ void nfs_clone_super(struct super_block *, struct nfs_mount_info *);
 void nfs_umount_begin(struct super_block *);
 int  nfs_statfs(struct dentry *, struct kstatfs *);
 int  nfs_show_options(struct seq_file *, struct dentry *);
-int  nfs_show_devname(struct seq_file *, struct dentry *);
 int  nfs_show_path(struct seq_file *, struct dentry *);
 int  nfs_show_stats(struct seq_file *, struct dentry *);
 void nfs_put_super(struct super_block *);
diff --git a/fs/nfs/nfs4super.c b/fs/nfs/nfs4super.c
index 65ab0a0..dc09ad2 100644
--- a/fs/nfs/nfs4super.c
+++ b/fs/nfs/nfs4super.c
@@ -58,7 +58,6 @@ static const struct super_operations nfs4_sops = {
 	.evict_inode	= nfs4_evict_inode,
 	.umount_begin	= nfs_umount_begin,
 	.show_options	= nfs_show_options,
-	.show_devname	= nfs_show_devname,
 	.show_path	= nfs_show_path,
 	.show_stats	= nfs_show_stats,
 	.remount_fs	= nfs_remount,
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 910ed90..9490d8ef 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -316,7 +316,6 @@ const struct super_operations nfs_sops = {
 	.evict_inode	= nfs_evict_inode,
 	.umount_begin	= nfs_umount_begin,
 	.show_options	= nfs_show_options,
-	.show_devname	= nfs_show_devname,
 	.show_path	= nfs_show_path,
 	.show_stats	= nfs_show_stats,
 	.remount_fs	= nfs_remount,
@@ -773,23 +772,6 @@ static void show_implementation_id(struct seq_file *m, struct nfs_server *nfss)
 }
 #endif
 
-int nfs_show_devname(struct seq_file *m, struct dentry *root)
-{
-	char *page = (char *) __get_free_page(GFP_KERNEL);
-	char *devname, *dummy;
-	int err = 0;
-	if (!page)
-		return -ENOMEM;
-	devname = nfs_path(&dummy, root, page, PAGE_SIZE, 0);
-	if (IS_ERR(devname))
-		err = PTR_ERR(devname);
-	else
-		seq_escape(m, devname, " \t\n\\");
-	free_page((unsigned long)page);
-	return err;
-}
-EXPORT_SYMBOL_GPL(nfs_show_devname);
-
 int nfs_show_path(struct seq_file *m, struct dentry *dentry)
 {
 	seq_puts(m, "/");
-- 
1.8.4.2

Re: [PATCH RFC] nfs: remove nfs_show_devname

[Trond Myklebust]

On Jan 9, 2014, at 13:59, Jeff Layton <jlayton@redhat.com> wrote:

> The nfs code will currently construct a devname to show in places like
> /proc/mounts by turning a dentry into a path. Unfortunately, that's
> somewhat problematic if the user ended up mounting through a symlink on
> the server. The devname that then shows up in /proc/mounts now doesn't
> match the one that was originally passed into the mount request.

This is 100% according to design. Why is it suddenly a problem?

By displaying the original pathname, you also end up bypassing referral resolution, etc. This is exactly why the .show_devname operation was introduced in the first place.

--
Trond Myklebust
Linux NFS client maintainer

Re: [PATCH RFC] nfs: remove nfs_show_devname

[Jeff Layton]

On Thu, 9 Jan 2014 14:06:54 -0500
Trond Myklebust <trond.myklebust@primarydata.com> wrote:

> 
> On Jan 9, 2014, at 13:59, Jeff Layton <jlayton@redhat.com> wrote:
> 
> > The nfs code will currently construct a devname to show in places like
> > /proc/mounts by turning a dentry into a path. Unfortunately, that's
> > somewhat problematic if the user ended up mounting through a symlink on
> > the server. The devname that then shows up in /proc/mounts now doesn't
> > match the one that was originally passed into the mount request.
> 
> This is 100% according to design. Why is it suddenly a problem?
> 
> By displaying the original pathname, you also end up bypassing referral resolution, etc. This is exactly why the .show_devname operation was introduced in the first place.
> 

AFAIU, the main issue is that when /etc/mtab is a symlink
to /proc/mounts, you can't do this in the example in the patch
description:

    # mount server:/export/bar /mnt/bar
    # umount server:/export/bar

...since umount will try to ID the mountpoint by looking up the devname
in /etc/mtab. What exactly would this break with referrals?

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH RFC] nfs: remove nfs_show_devname

[Trond Myklebust]

On Jan 9, 2014, at 14:12, Jeff Layton <jlayton@redhat.com> wrote:

> On Thu, 9 Jan 2014 14:06:54 -0500
> Trond Myklebust <trond.myklebust@primarydata.com> wrote:
> 
>> 
>> On Jan 9, 2014, at 13:59, Jeff Layton <jlayton@redhat.com> wrote:
>> 
>>> The nfs code will currently construct a devname to show in places like
>>> /proc/mounts by turning a dentry into a path. Unfortunately, that's
>>> somewhat problematic if the user ended up mounting through a symlink on
>>> the server. The devname that then shows up in /proc/mounts now doesn't
>>> match the one that was originally passed into the mount request.
>> 
>> This is 100% according to design. Why is it suddenly a problem?
>> 
>> By displaying the original pathname, you also end up bypassing referral resolution, etc. This is exactly why the .show_devname operation was introduced in the first place.
>> 
> 
> AFAIU, the main issue is that when /etc/mtab is a symlink
> to /proc/mounts, you can't do this in the example in the patch
> description:
> 
>    # mount server:/export/bar /mnt/bar
>    # umount server:/export/bar
> 
> ...since umount will try to ID the mountpoint by looking up the devname
> in /etc/mtab.

The NFS pathname is NOT a devname. It shouldn’t be abused as a substitute for a mount point.

Particularly not so when you consider that the same NFS path can be mounted in several different places with different mount options. The same path can even refer to completely different locations (i.e. different file handles) if, say, the original directory has been renamed on the server.

> What exactly would this break with referrals?
> 

It would “break” in the sense that the admin would have no clue that his client is actually talking to a different server than the one specified in the mount.

--
Trond Myklebust
Linux NFS client maintainer

Re: [PATCH RFC] nfs: remove nfs_show_devname

[Jeff Layton]

On Thu, 9 Jan 2014 14:27:04 -0500
Trond Myklebust <trond.myklebust@primarydata.com> wrote:

> 
> On Jan 9, 2014, at 14:12, Jeff Layton <jlayton@redhat.com> wrote:
> 
> > On Thu, 9 Jan 2014 14:06:54 -0500
> > Trond Myklebust <trond.myklebust@primarydata.com> wrote:
> > 
> >> 
> >> On Jan 9, 2014, at 13:59, Jeff Layton <jlayton@redhat.com> wrote:
> >> 
> >>> The nfs code will currently construct a devname to show in places like
> >>> /proc/mounts by turning a dentry into a path. Unfortunately, that's
> >>> somewhat problematic if the user ended up mounting through a symlink on
> >>> the server. The devname that then shows up in /proc/mounts now doesn't
> >>> match the one that was originally passed into the mount request.
> >> 
> >> This is 100% according to design. Why is it suddenly a problem?
> >> 
> >> By displaying the original pathname, you also end up bypassing referral resolution, etc. This is exactly why the .show_devname operation was introduced in the first place.
> >> 
> > 
> > AFAIU, the main issue is that when /etc/mtab is a symlink
> > to /proc/mounts, you can't do this in the example in the patch
> > description:
> > 
> >    # mount server:/export/bar /mnt/bar
> >    # umount server:/export/bar
> > 
> > ...since umount will try to ID the mountpoint by looking up the devname
> > in /etc/mtab.
> 
> The NFS pathname is NOT a devname. It shouldn’t be abused as a substitute for a mount point.
> 
> Particularly not so when you consider that the same NFS path can be mounted in several different places with different mount options. The same path can even refer to completely different locations (i.e. different file handles) if, say, the original directory has been renamed on the server.
> 
> > What exactly would this break with referrals?
> > 
> 
> It would “break” in the sense that the admin would have no clue that his client is actually talking to a different server than the one specified in the mount.
> 

Good point. Ok, I don't think the problem really rises above the level
of annoyance, so let's just drop this patch.

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>