NFS: Add "device" tag to /proc/self/mountstats

PATCH In review Bryan Schumaker 2 participants 7 messages Aug 26, 2011 – Aug 29, 2011

Subsystems (3)

VFS NFS client NFS server

lore

Share & tools

lore

Bryan Schumaker

author

2011-08-26 20:52 UTC #0 lore raw

PATCH 1/1 NFS: Add "device" tag to /proc/self/mountstats

nfsiostat was failing to find mounted filesystems on recent kernels due to changes in the VFS that resulted in a missing "device" tag in the /proc/self/mountstats file. This patch re-adds the "device" tag for NFS mount data in /proc/self/mountstats.

Signed-off-by Bryan Schumaker <[email protected]>

---
 fs/nfs/super.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

fs/nfs/super.c +3 -1

index b961cea..e6a0317 100644
@@ -766,8 +766,10 @@ static int nfs_show_devname(struct seq_file *m, struct vfsmount *mnt)
 	devname = nfs_path(&dummy, mnt->mnt_root, page, PAGE_SIZE);
 	if (IS_ERR(devname))
 		err = PTR_ERR(devname);
-	else
+	else {
+		seq_puts(m, "device ");
 		seq_escape(m, devname, " \t\n\\");
+	}
 	free_page((unsigned long)page);
 	return err;
 }

Signature · 1 line

1.7.6

Chuck Lever

2011-08-27 23:53 UTC #1 lore raw

Quoted 3 lines · Bryan Schumaker

nfsiostat was failing to find mounted filesystems on recent kernels due to changes in the VFS that resulted in a missing "device" tag in the /proc/self/mountstats file.

What was the commit ID that changed this formal kernel API? I'd like to see the justification.

Quoted 2 lines

This patch re-adds the "device" tag for NFS
mount data in /proc/self/mountstats.

The purpose of mountstats is to be an interface that all file systems can use. If we can't add "device" back to all mounts listed in /proc/self/mountstats, it might be better to adjust nfsiostats to cope.

Quoted 23 lines

Signed-off-by Bryan Schumaker <[email protected]>

---
fs/nfs/super.c | 4 +++-

1 files changed, 3 insertions(+), 1 deletions(-)

fs/nfs/super.c +3 -1

index b961cea..e6a0317 100644
@@ -766,8 +766,10 @@ static int nfs_show_devname(struct seq_file *m, struct vfsmount *mnt)
 	devname = nfs_path(&dummy, mnt->mnt_root, page, PAGE_SIZE);
 	if (IS_ERR(devname))
 		err = PTR_ERR(devname);
-	else
+	else {
+		seq_puts(m, "device ");
 		seq_escape(m, devname, " \t\n\\");
+	}
 	free_page((unsigned long)page);
 	return err;
 }

Signature · 1 line

1.7.6

Signature · 2 lines

Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Bryan Schumaker

author

2011-08-29 13:46 UTC #2 lore raw

Quoted 8 lines · Chuck Lever

Quoted 3 lines · Bryan Schumaker
nfsiostat was failing to find mounted filesystems on recent kernels due to changes in the VFS that resulted in a missing "device" tag in the /proc/self/mountstats file.
What was the commit ID that changed this formal kernel API? I'd like to see the justification.

It was changed by this commit:

commit c7f404b40a3665d9f4e9a927cc5c1ee0479ed8f9

Author: Al Viro <[email protected]>
Date:   Wed Mar 16 06:59:40 2011 -0400

vfs: new superblock methods to override /proc/*/mount{s,info}

a) ->show_devname(m, mnt) - what to put into devname columns in mounts, mountinfo and mountstats
b) ->show_path(m, mnt) - what to put into relative path column in mountinfo

Leaving those NULL gives old behaviour. NFS switched to using those.

Signed-off-by Al Viro <[email protected]>

Quoted 30 lines

Quoted 3 lines
This patch re-adds the "device" tag for NFS
mount data in /proc/self/mountstats.
The purpose of mountstats is to be an interface that all file systems can use. If we can't add "device" back to all mounts listed in /proc/self/mountstats, it might be better to adjust nfsiostats to cope.
Quoted 23 lines
Signed-off-by Bryan Schumaker <[email protected]>
---
fs/nfs/super.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
fs/nfs/super.c +3 -1
index b961cea..e6a0317 100644
@@ -766,8 +766,10 @@ static int nfs_show_devname(struct seq_file *m, struct vfsmount *mnt)
 	devname = nfs_path(&dummy, mnt->mnt_root, page, PAGE_SIZE);
 	if (IS_ERR(devname))
 		err = PTR_ERR(devname);
-	else
+	else {
+		seq_puts(m, "device ");
 		seq_escape(m, devname, " \t\n\\");
+	}
 	free_page((unsigned long)page);
 	return err;
 }
Signature · 1 line
1.7.6

Chuck Lever

2011-08-29 14:43 UTC #3 lore raw

Quoted 18 lines · Bryan Schumaker

Quoted 8 lines · Chuck Lever
Quoted 3 lines · Bryan Schumaker
nfsiostat was failing to find mounted filesystems on recent kernels due to changes in the VFS that resulted in a missing "device" tag in the /proc/self/mountstats file.
What was the commit ID that changed this formal kernel API? I'd like to see the justification.
It was changed by this commit:
commit c7f404b40a3665d9f4e9a927cc5c1ee0479ed8f9
Author: Al Viro <[email protected]>
Date:   Wed Mar 16 06:59:40 2011 -0400
vfs: new superblock methods to override /proc/*/mount{s,info}

Quoted 7 lines

a) ->show_devname(m, mnt) - what to put into devname columns in mounts, mountinfo and mountstats
b) ->show_path(m, mnt) - what to put into relative path column in mountinfo
Leaving those NULL gives old behaviour. NFS switched to using those.
Signed-off-by Al Viro <[email protected]>

It looks like dropping the "device" tag from /proc/self/mountstats was not intentional, so this is a kernel API regression. At a guess, 2.6.39 probably works right, but 3.0 is broken. Whatever solution we come up with might need to be cc:d to [email protected]. Be sure to verify which released kernels are broken.

Does changing nfs_show_devname() to add a "device" tag have any effect on the other two /proc files (mounts and mountinfo)? I suspect you do want to fix show_vfsstat() instead of nfs_show_devname(). This should add "device" only where it is needed, and would work for all file systems.

Quoted 33 lines

Quoted 32 lines
Quoted 5 lines
This patch re-adds the "device" tag for NFS
mount data in /proc/self/mountstats.
The purpose of mountstats is to be an interface that all file systems can use. If we can't add "device" back to all mounts listed in /proc/self/mountstats, it might be better to adjust nfsiostats to cope.
Quoted 23 lines
Signed-off-by Bryan Schumaker <[email protected]>
---
fs/nfs/super.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
fs/nfs/super.c +3 -1
index b961cea..e6a0317 100644
@@ -766,8 +766,10 @@ static int nfs_show_devname(struct seq_file *m, struct vfsmount *mnt)
 	devname = nfs_path(&dummy, mnt->mnt_root, page, PAGE_SIZE);
 	if (IS_ERR(devname))
 		err = PTR_ERR(devname);
-	else
+	else {
+		seq_puts(m, "device ");
 		seq_escape(m, devname, " \t\n\\");
+	}
 	free_page((unsigned long)page);
 	return err;
 }
Signature · 1 line
1.7.6

Signature · 2 lines

Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Bryan Schumaker

author

2011-08-29 14:55 UTC #4 lore raw

Quoted 25 lines · Chuck Lever

Quoted 22 lines · Bryan Schumaker
Quoted 8 lines · Chuck Lever
Quoted 3 lines · Bryan Schumaker
nfsiostat was failing to find mounted filesystems on recent kernels due to changes in the VFS that resulted in a missing "device" tag in the /proc/self/mountstats file.
What was the commit ID that changed this formal kernel API? I'd like to see the justification.
It was changed by this commit:
commit c7f404b40a3665d9f4e9a927cc5c1ee0479ed8f9
Author: Al Viro <[email protected]>
Date:   Wed Mar 16 06:59:40 2011 -0400
vfs: new superblock methods to override /proc/*/mount{s,info}
a) ->show_devname(m, mnt) - what to put into devname columns in mounts, mountinfo and mountstats
b) ->show_path(m, mnt) - what to put into relative path column in mountinfo

Quoted 5 lines

Quoted 3 lines
Leaving those NULL gives old behaviour. NFS switched to using those.
Signed-off-by Al Viro <[email protected]>
It looks like dropping the "device" tag from /proc/self/mountstats was not intentional, so this is a kernel API regression. At a guess, 2.6.39 probably works right, but 3.0 is broken. Whatever solution we come up with might need to be cc:d to [email protected]. Be sure to verify which released kernels are broken.

I'll double check that 39 works.

Quoted 2 lines

Does changing nfs_show_devname() to add a "device" tag have any effect on the other two /proc files (mounts and mountinfo)? I suspect you do want to fix show_vfsstat() instead of nfs_show_devname(). This should add "device" only where it is needed, and would work for all file systems.

Yeah, looks like it changes both. I'm looking at show_vfsstat() now, but nfs_show_devname() will return an error if a call to nfs_path() fails. This would probably result in an empty "device " line printed to /proc/self/mountstats. Do you know if that would be a problem for anything? I'm thinking about doing something like this:

fs/namespace.c +1 -0

index 22bfe82..1429114 100644
@@ -1109,6 +1109,7 @@ static int show_vfsstat(struct seq_file *m, void *v)
 
        /* device */
        if (mnt->mnt_sb->s_op->show_devname) {
+               seq_puts(m, "device ");
                err = mnt->mnt_sb->s_op->show_devname(m, mnt);
        } else {
                if (mnt->mnt_devname) {

Quoted 35 lines

Quoted 34 lines
Quoted 33 lines
Quoted 6 lines
This patch re-adds the "device" tag for NFS
mount data in /proc/self/mountstats.
The purpose of mountstats is to be an interface that all file systems can use. If we can't add "device" back to all mounts listed in /proc/self/mountstats, it might be better to adjust nfsiostats to cope.
Quoted 23 lines
Signed-off-by Bryan Schumaker <[email protected]>
---
fs/nfs/super.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
fs/nfs/super.c +3 -1
index b961cea..e6a0317 100644
@@ -766,8 +766,10 @@ static int nfs_show_devname(struct seq_file *m, struct vfsmount *mnt)
 	devname = nfs_path(&dummy, mnt->mnt_root, page, PAGE_SIZE);
 	if (IS_ERR(devname))
 		err = PTR_ERR(devname);
-	else
+	else {
+		seq_puts(m, "device ");
 		seq_escape(m, devname, " \t\n\\");
+	}
 	free_page((unsigned long)page);
 	return err;
 }
Signature · 1 line
1.7.6

Chuck Lever

2011-08-29 15:37 UTC #5 lore raw

Quoted 30 lines · Bryan Schumaker

Quoted 29 lines · Chuck Lever
Quoted 25 lines · Bryan Schumaker
Quoted 8 lines · Chuck Lever
Quoted 3 lines · Bryan Schumaker
nfsiostat was failing to find mounted filesystems on recent kernels due to changes in the VFS that resulted in a missing "device" tag in the /proc/self/mountstats file.
What was the commit ID that changed this formal kernel API? I'd like to see the justification.
It was changed by this commit:
commit c7f404b40a3665d9f4e9a927cc5c1ee0479ed8f9
Author: Al Viro <[email protected]>
Date:   Wed Mar 16 06:59:40 2011 -0400
vfs: new superblock methods to override /proc/*/mount{s,info}
a) ->show_devname(m, mnt) - what to put into devname columns in mounts, mountinfo and mountstats
b) ->show_path(m, mnt) - what to put into relative path column in mountinfo
Leaving those NULL gives old behaviour. NFS switched to using those.
Signed-off-by Al Viro <[email protected]>

Quoted 7 lines

Quoted 1 line
It looks like dropping the "device" tag from /proc/self/mountstats was not intentional, so this is a kernel API regression. At a guess, 2.6.39 probably works right, but 3.0 is broken. Whatever solution we come up with might need to be cc:d to [email protected]. Be sure to verify which released kernels are broken.
I'll double check that 39 works.
Quoted 2 lines
Does changing nfs_show_devname() to add a "device" tag have any effect on the other two /proc files (mounts and mountinfo)? I suspect you do want to fix show_vfsstat() instead of nfs_show_devname(). This should add "device" only where it is needed, and would work for all file systems.
Yeah, looks like it changes both. I'm looking at show_vfsstat() now, but nfs_show_devname() will return an error if a call to nfs_path() fails. This would probably result in an empty "device " line printed to /proc/self/mountstats. Do you know if that would be a problem for anything?

The only error return from nfs_path() is if the name is too long. You could change nfs_show_devname() to output a fixed error string in that case (like "path too long"), or something even more generic.

Quoted 7 lines

I'm thinking about doing something like this:
fs/namespace.c +0 -0
index 22bfe82..1429114 100644

fs/namespace.c static int show_vfsstat(struct seq_file *m, void *v) · line 1115

Quoted 7 lines

fs/namespace.c +1 -0

@@ -1109,6 +1109,7 @@ static int show_vfsstat(struct seq_file *m, void *v)
 
       /* device */
       if (mnt->mnt_sb->s_op->show_devname) {
+               seq_puts(m, "device ");
               err = mnt->mnt_sb->s_op->show_devname(m, mnt);
       } else {
               if (mnt->mnt_devname) {

There probably isn't a clean way to have just one seq_puts(m, "device "); invocation in this function.

Quoted 38 lines

Quoted 37 lines
Quoted 36 lines
Quoted 35 lines
Quoted 8 lines
This patch re-adds the "device" tag for NFS
mount data in /proc/self/mountstats.
The purpose of mountstats is to be an interface that all file systems can use. If we can't add "device" back to all mounts listed in /proc/self/mountstats, it might be better to adjust nfsiostats to cope.
Quoted 23 lines
Signed-off-by Bryan Schumaker <[email protected]>
---
fs/nfs/super.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
fs/nfs/super.c +3 -1
index b961cea..e6a0317 100644
@@ -766,8 +766,10 @@ static int nfs_show_devname(struct seq_file *m, struct vfsmount *mnt)
 	devname = nfs_path(&dummy, mnt->mnt_root, page, PAGE_SIZE);
 	if (IS_ERR(devname))
 		err = PTR_ERR(devname);
-	else
+	else {
+		seq_puts(m, "device ");
 		seq_escape(m, devname, " \t\n\\");
+	}
 	free_page((unsigned long)page);
 	return err;
 }
Signature · 1 line
1.7.6

Signature · 2 lines

Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Bryan Schumaker

author

2011-08-29 15:55 UTC #6 lore raw

Quoted 35 lines · Chuck Lever

Quoted 32 lines · Bryan Schumaker
Quoted 30 lines · Chuck Lever
Quoted 18 lines · Bryan Schumaker
Quoted 8 lines · Chuck Lever
Quoted 3 lines · Bryan Schumaker
nfsiostat was failing to find mounted filesystems on recent kernels due to changes in the VFS that resulted in a missing "device" tag in the /proc/self/mountstats file.
What was the commit ID that changed this formal kernel API? I'd like to see the justification.
It was changed by this commit:
commit c7f404b40a3665d9f4e9a927cc5c1ee0479ed8f9
Author: Al Viro <[email protected]>
Date:   Wed Mar 16 06:59:40 2011 -0400
vfs: new superblock methods to override /proc/*/mount{s,info}
Quoted 7 lines
a) ->show_devname(m, mnt) - what to put into devname columns in mounts, mountinfo and mountstats
b) ->show_path(m, mnt) - what to put into relative path column in mountinfo
Leaving those NULL gives old behaviour. NFS switched to using those.
Signed-off-by Al Viro <[email protected]>
It looks like dropping the "device" tag from /proc/self/mountstats was not intentional, so this is a kernel API regression. At a guess, 2.6.39 probably works right, but 3.0 is broken. Whatever solution we come up with might need to be cc:d to [email protected]. Be sure to verify which released kernels are broken.

Quoted 7 lines

Quoted 5 lines
I'll double check that 39 works.
Quoted 2 lines
Does changing nfs_show_devname() to add a "device" tag have any effect on the other two /proc files (mounts and mountinfo)? I suspect you do want to fix show_vfsstat() instead of nfs_show_devname(). This should add "device" only where it is needed, and would work for all file systems.
Yeah, looks like it changes both. I'm looking at show_vfsstat() now, but nfs_show_devname() will return an error if a call to nfs_path() fails. This would probably result in an empty "device " line printed to /proc/self/mountstats. Do you know if that would be a problem for anything?
The only error return from nfs_path() is if the name is too long. You could change nfs_show_devname() to output a fixed error string in that case (like "path too long"), or something even more generic.

Sounds good to me, I'll work in something.

Quoted 9 lines

Quoted 9 lines
I'm thinking about doing something like this:
fs/namespace.c +0 -0
index 22bfe82..1429114 100644
@@ -1109,6 +1109,7 @@ static int show_vfsstat(struct seq_file *m, void *v)
 

Quoted 8 lines

fs/namespace.c static int show_vfsstat(struct seq_file *m, void *v) · line 1115
Quoted 6 lines
fs/namespace.c +1 -0
       /* device */
       if (mnt->mnt_sb->s_op->show_devname) {
+               seq_puts(m, "device ");
               err = mnt->mnt_sb->s_op->show_devname(m, mnt);
       } else {
               if (mnt->mnt_devname) {
There probably isn't a clean way to have just one seq_puts(m, "device "); invocation in this function.

My thoughts exactly. I'll stick with having two seq_puts(m, "device ") calls since it's easier to figure out what's going on that way.

Quoted 40 lines

Quoted 39 lines
Quoted 38 lines
Quoted 37 lines
Quoted 36 lines
Quoted 9 lines
This patch re-adds the "device" tag for NFS
mount data in /proc/self/mountstats.
The purpose of mountstats is to be an interface that all file systems can use. If we can't add "device" back to all mounts listed in /proc/self/mountstats, it might be better to adjust nfsiostats to cope.
Quoted 23 lines
Signed-off-by Bryan Schumaker <[email protected]>
---
fs/nfs/super.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
fs/nfs/super.c +3 -1
index b961cea..e6a0317 100644
@@ -766,8 +766,10 @@ static int nfs_show_devname(struct seq_file *m, struct vfsmount *mnt)
 	devname = nfs_path(&dummy, mnt->mnt_root, page, PAGE_SIZE);
 	if (IS_ERR(devname))
 		err = PTR_ERR(devname);
-	else
+	else {
+		seq_puts(m, "device ");
 		seq_escape(m, devname, " \t\n\\");
+	}
 	free_page((unsigned long)page);
 	return err;
 }
Signature · 1 line
1.7.6