Bug#669314: NFS: kernel forces trailing slash for export in /proc/self/mounts
I've applied the patch to Linus' master HEAD (43c422eda99b894f18d1cca17bcd2401efaf7bd0, at the time) and the patch seems to work fine. /proc/self/mounts correctly reflects whether or not the user specified to use a trailing slash - and nothing is obviously broken. If the patch looks fine to others, I propose we move this patch into wherever is the next step for testing it more broadly: e.g. Debian experimental, and wherever it needs to go to end up in Linus' master branch. Thanks, Chris smime.p7s Description: S/MIME cryptographic signature
Bug#669314: NFS: kernel forces trailing slash for export in /proc/self/mounts
I haven't had a chance to test it yet thanks. I'll be too busy until at least mid-next week, but I will test it then if nobody beats me to it. -Chris On Sep 30, 2012, at 2:23 PM, Jonathan Nieder jrnie...@gmail.com wrote: Thanks for looking it over. Did you get a chance to test Ben's patch? Curious, Jonathan smime.p7s Description: S/MIME cryptographic signature
Bug#669314: NFS: kernel forces trailing slash for export in /proc/self/mounts
Hi Chris, Chris Hiestand wrote: On Sep 16, 2012, at 7:00 AM, Ben Hutchings wrote: This was my first thought - but what if userland provides a device name with a slash on the end? I think we have to report it back with the slash in that case. [...] As a point of comparison, matching the given input is the behavior of 2.6.32-5 in Debian Squeeze. So I think your approach is better. Thanks for looking it over. Did you get a chance to test Ben's patch? Curious, Jonathan -- To UNSUBSCRIBE, email to debian-kernel-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/20120930212341.GA32505@elie.Belkin
Bug#669314: NFS: kernel forces trailing slash for export in /proc/self/mounts
I had a couple friends over today and we made a trivial patch to remove trailing slashes. We do not know C and have never created a patch for the kernel before, so there is undoubtedly a better way to do it. However we hope this helps in your efforts. 0001-Fixes-trailing-slash-in-nfs-devname.patch Description: Binary data This patch was created from the offending commit (c7f404b40a366). But I've also applied it to to Linus Torvalds' master HEAD (3f0c3c8fe30c7) with success. -Chris smime.p7s Description: S/MIME cryptographic signature
Bug#669314: NFS: kernel forces trailing slash for export in /proc/self/mounts
On Sun, 2012-09-16 at 03:24 -0700, Chris Hiestand wrote: I had a couple friends over today and we made a trivial patch to remove trailing slashes. We do not know C and have never created a patch for the kernel before, so there is undoubtedly a better way to do it. However we hope this helps in your efforts. This patch was created from the offending commit (c7f404b40a366). But I've also applied it to to Linus Torvalds' master HEAD (3f0c3c8fe30c7) with success. This was my first thought - but what if userland provides a device name with a slash on the end? I think we have to report it back with the slash in that case. Ben. -- Ben Hutchings Experience is what causes a person to make new mistakes instead of old ones. signature.asc Description: This is a digitally signed message part
Bug#669314: NFS: kernel forces trailing slash for export in /proc/self/mounts
On Sep 16, 2012, at 7:00 AM, Ben Hutchings wrote: This was my first thought - but what if userland provides a device name with a slash on the end? I think we have to report it back with the slash in that case. That is a great point. We could not find any standard as to whether or not there should be a trailing slash. And from all the examples I could find, there seems to be a convention of omitting a trailing slash. However, if userland provides a trailing slash, it would seem appropriate to retain it. As a point of comparison, matching the given input is the behavior of 2.6.32-5 in Debian Squeeze. So I think your approach is better. -Chris smime.p7s Description: S/MIME cryptographic signature
Bug#669314: NFS: kernel forces trailing slash for export in /proc/self/mounts
On Tue, 2012-06-19 at 12:43 -0700, Chris Hiestand wrote:
Hi Alexander Viro et al,
This is an escalation of Debian Bug #669314 http://bugs.debian.org/669314,
which I will
re-elaborate in this email for your convenience.
You committed a change to the way the linux kernel reports NFS mounts - now
with a
trailing slash for the remote export (among other changes). The change
happened here:
commit c7f404b40a3665d9f4e9a927cc5c1ee0479ed8f9
Author: Al Viro v...@zeniv.linux.org.uk
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 v...@zeniv.linux.org.uk
The problematic behavior is that NFS exports now have a trailing slash in
/proc/self/mounts.
[...]
If there is a new convention to display the trailing slash, we need to update
our tools to handle this change. If there is not a new convention, I'd argue
this is a bug.
So is this a new convention or not? What is the appropriate way for
Debian to move forward?
Al, Trond, what's going on here? This sure looks like it broke
userland, in which case we need to revert the change in behaviour.
How about the following (untested) change?
Ben.
---
Subject: nfs: Show original device name verbatim in /proc/*/mount{s,info}
Since commit c7f404b ('vfs: new superblock methods to override
/proc/*/mount{s,info}'), nfs_path() is used to generate the mounted
device name reported back to userland.
nfs_path() always generates a trailing slash when the given dentry is
the root of an NFS mount, but userland may expect the original device
name to be returned verbatim (as it used to be). Make this
canonicalisation optional and change the callers accordingly.
Signed-off-by: Ben Hutchings b...@decadent.org.uk
Cc: sta...@vger.kernel.org # v2.6.39+
---
fs/nfs/internal.h |4 ++--
fs/nfs/namespace.c | 15 ++-
fs/nfs/nfs4namespace.c |2 +-
fs/nfs/super.c |2 +-
4 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index d554438..c38224a 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -354,7 +354,7 @@ extern void nfs_sb_deactive(struct super_block *sb);
/* namespace.c */
extern char *nfs_path(char **p, struct dentry *dentry,
- char *buffer, ssize_t buflen);
+ char *buffer, ssize_t buflen, bool canonical);
extern struct vfsmount *nfs_d_automount(struct path *path);
struct vfsmount *nfs_submount(struct nfs_server *, struct dentry *,
struct nfs_fh *, struct nfs_fattr *);
@@ -491,7 +491,7 @@ static inline char *nfs_devname(struct dentry *dentry,
char *buffer, ssize_t buflen)
{
char *dummy;
- return nfs_path(dummy, dentry, buffer, buflen);
+ return nfs_path(dummy, dentry, buffer, buflen, true);
}
/*
diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
index 6559253..059975e 100644
--- a/fs/nfs/namespace.c
+++ b/fs/nfs/namespace.c
@@ -33,6 +33,8 @@ int nfs_mountpoint_expiry_timeout = 500 * HZ;
* @dentry - pointer to dentry
* @buffer - result buffer
* @buflen - length of buffer
+ * @canonical - ensure there is exactly one slash after the original
+ * device (export) name; if false, return it verbatim
*
* Helper function for constructing the server pathname
* by arbitrary hashed dentry.
@@ -41,7 +43,8 @@ int nfs_mountpoint_expiry_timeout = 500 * HZ;
* server side when automounting on top of an existing partition
* and in generating /proc/mounts and friends.
*/
-char *nfs_path(char **p, struct dentry *dentry, char *buffer, ssize_t buflen)
+char *nfs_path(char **p, struct dentry *dentry, char *buffer, ssize_t buflen,
+ bool canonical)
{
char *end;
int namelen;
@@ -74,7 +77,7 @@ rename_retry:
rcu_read_unlock();
goto rename_retry;
}
- if (*end != '/') {
+ if (canonical *end != '/') {
if (--buflen 0) {
spin_unlock(dentry-d_lock);
rcu_read_unlock();
@@ -91,9 +94,11 @@ rename_retry:
return end;
}
namelen = strlen(base);
- /* Strip off excess slashes in base string */
- while (namelen 0 base[namelen - 1] == '/')
- namelen--;
+ if (canonical) {
+ /* Strip off excess slashes in base string */
+ while (namelen 0 base[namelen - 1] == '/')
+ namelen--;
+ }
buflen -= namelen;
if (buflen 0) {
spin_unlock(dentry-d_lock);
diff --git
Bug#669314: NFS: kernel forces trailing slash for export in /proc/self/mounts
Hi Chris, Chris Hiestand wrote: [Subject: No Reply From Alexander Viro] Please keep in mind that these appear as emails in a crowded inbox, so the subject ine can be a good place to put valuable context. Just an update, I emailed Alexander Viro twice and I haven't heard back from Alexander Viro or anyone on the kernel development mailing lists. I suppose we should just come up with a patch to propose. I'll be happy to look into that, but I can't promise that it will be soon (and if someone else gets to it sooner, I won't mind). Thanks again for your help, Jonathan -- To UNSUBSCRIBE, email to debian-kernel-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/20120915015604.ga...@mannheim-rule.att.net
Bug#669314: NFS: kernel forces trailing slash for export in /proc/self/mounts
Hi Alexander Viro et al,
This is an escalation of Debian Bug #669314 http://bugs.debian.org/669314,
which I will
re-elaborate in this email for your convenience.
You committed a change to the way the linux kernel reports NFS mounts - now
with a
trailing slash for the remote export (among other changes). The change happened
here:
commit c7f404b40a3665d9f4e9a927cc5c1ee0479ed8f9
Author: Al Viro v...@zeniv.linux.org.uk
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 v...@zeniv.linux.org.uk
The problematic behavior is that NFS exports now have a trailing slash in
/proc/self/mounts.
This still seems to be the case in newer kernels, for example in Debian's
3.3.2-1~experimental.1.
and HEAD in Linus Torvalds' master branch, presently commit:
02edf6abe01610a5fb379df442de3c837ad99467
I believe it is/was convention to leave a trailing slash off of the nfs export
in /etc/fstab, e.g.:
nfsserver:/srv/ubuntu-32 /mnt/ubuntu-32 nfs
ro,nfsvers=3,soft,intr,tcp,nodev,noatime,nosuid,rsize=32768,wsize=32768
and I'd also expect the same in /proc/self/mounts
Expected Result:
I expect /proc/self/mounts to show (notice no trailing slash on the export):
nfsserver:/srv/ubuntu-32 /mnt/ubuntu-32 nfs
ro,nosuid,nodev,noatime,vers=3,rsize=32768,wsize=32768,namlen=255,soft,proto=tcp,timeo=600,retrans=2,sec=sys,mountaddr=198.202.1.1,mountvers=3,mountport=41576,mountproto=tcp,local_lock=none,addr=198.202.1.1
0 0
Actual Result:
But instead in /proc/self/mounts I get (notice the trailing slash):
nfsserver:/srv/ubuntu-32/ /mnt/ubuntu-32 nfs
ro,nosuid,nodev,noatime,vers=3,rsize=32768,wsize=32768,namlen=255,soft,proto=tcp,timeo=600,retrans=2,sec=sys,mountaddr=198.202.1.1,mountvers=3,mountport=41576,mountproto=tcp,local_lock=none,addr=198.202.1.1
0 0
Rammifications:
This simple change has a lot of implications because lots of things parse
/etc/fstab and /proc/self/mounts, for example system configuration tools
and mount.nfs and umount.nfs.
If you use the former convention and try to umount an export on a newer
kernel it will fail:
user@hostname:/proc/self$ sudo umount.nfs nfsserver:/srv/ubuntu-32
umount.nfs: nfsserver:/srv/ubuntu-32: not found
And if you run sudo mount -va, mount will not recognize that the fstab mounts
have already been mounted; mounting all mounts twice on the same mount point.
This quickly gets messy.
If there is a new convention to display the trailing slash, we need to update
our tools to handle this change. If there is not a new convention, I'd argue
this is a bug.
So is this a new convention or not? What is the appropriate way for Debian to
move forward?
Thanks,
Chris Hiestand
--
To UNSUBSCRIBE, email to debian-kernel-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Archive: http://lists.debian.org/8f8193bd-84c6-4905-8789-de7eb2579...@salk.edu
