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 <[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]>
>
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
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.
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
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
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 <[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]>
> >
>
> 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 <[email protected]>
Cc: <[email protected]> # 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 a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
index 017b4b0..94e8652 100644
--- a/fs/nfs/nfs4namespace.c
+++ b/fs/nfs/nfs4namespace.c
@@ -81,7 +81,7 @@ static char *nfs_path_component(const char *nfspath, const char *end)
static char *nfs4_path(struct dentry *dentry, char *buffer, ssize_t buflen)
{
char *limit;
- char *path = nfs_path(&limit, dentry, buffer, buflen);
+ char *path = nfs_path(&limit, dentry, buffer, buflen, true);
if (!IS_ERR(path)) {
char *path_component = nfs_path_component(path, limit);
if (path_component)
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index d2c7f5d..630a1e2 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -765,7 +765,7 @@ int nfs_show_devname(struct seq_file *m, struct dentry *root)
int err = 0;
if (!page)
return -ENOMEM;
- devname = nfs_path(&dummy, root, page, PAGE_SIZE);
+ devname = nfs_path(&dummy, root, page, PAGE_SIZE, false);
if (IS_ERR(devname))
err = PTR_ERR(devname);
else
--
Ben Hutchings
Experience is what causes a person to make new mistakes instead of old ones.
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
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
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 <[email protected]> wrote:
> Thanks for looking it over. Did you get a chance to test Ben's patch?
>
> Curious,
> Jonathan
T24gU3VuLCAyMDEyLTEwLTIxIGF0IDE3OjAxIC0wNzAwLCBKb25hdGhhbiBOaWVkZXIgd3JvdGU6
DQo+IChjYy1pbmcgTGF1cmVudCBpbiBjYXNlIGhlIHdhbnRzIHRvIHRlc3QuICBMYXVyZW50LCBh
ICJnaXQgYW0iLXJlYWR5DQo+ICBwYXRjaCBpcyBhdCBbMV0pDQo+IEJlbiBIdXRjaGluZ3Mgd3Jv
dGU6DQo+IA0KPiA+IFNpbmNlIGNvbW1pdCBjN2Y0MDRiICgndmZzOiBuZXcgc3VwZXJibG9jayBt
ZXRob2RzIHRvIG92ZXJyaWRlDQo+ID4gL3Byb2MvKi9tb3VudHtzLGluZm99JyksIG5mc19wYXRo
KCkgaXMgdXNlZCB0byBnZW5lcmF0ZSB0aGUgbW91bnRlZA0KPiA+IGRldmljZSBuYW1lIHJlcG9y
dGVkIGJhY2sgdG8gdXNlcmxhbmQuDQo+ID4NCj4gPiBuZnNfcGF0aCgpIGFsd2F5cyBnZW5lcmF0
ZXMgYSB0cmFpbGluZyBzbGFzaCB3aGVuIHRoZSBnaXZlbiBkZW50cnkgaXMNCj4gPiB0aGUgcm9v
dCBvZiBhbiBORlMgbW91bnQsIGJ1dCB1c2VybGFuZCBtYXkgZXhwZWN0IHRoZSBvcmlnaW5hbCBk
ZXZpY2UNCj4gPiBuYW1lIHRvIGJlIHJldHVybmVkIHZlcmJhdGltIChhcyBpdCB1c2VkIHRvIGJl
KS4gIE1ha2UgdGhpcw0KPiA+IGNhbm9uaWNhbGlzYXRpb24gb3B0aW9uYWwgYW5kIGNoYW5nZSB0
aGUgY2FsbGVycyBhY2NvcmRpbmdseS4NCj4gPg0KPiA+IFJlcG9ydGVkLWFuZC10ZXN0ZWQtYnk6
IENocmlzIEhpZXN0YW5kIDxjaGllc3RhbmRAc2Fsay5lZHU+DQo+ID4gUmVmZXJlbmNlOiBodHRw
Oi8vYnVncy5kZWJpYW4ub3JnLzY2OTMxNA0KPiA+IFNpZ25lZC1vZmYtYnk6IEJlbiBIdXRjaGlu
Z3MgPGJlbkBkZWNhZGVudC5vcmcudWs+DQo+ID4gQ2M6IDxzdGFibGVAdmdlci5rZXJuZWwub3Jn
PiAjIHYyLjYuMzkrDQoNCg0KVGhpcyBwYXRjaCB3aWxsIG5vdCBhcHBseSB0byB0aGUgdXBzdHJl
YW0ga2VybmVsOyBpdCBzZWVtcyByYXRoZXIgdG8NCmhhdmUgYmVlbiBnZW5lcmF0ZWQgaW5jcmVt
ZW50YWxseSBhZ2FpbnN0IGFuIG9sZGVyIHBhdGNoIGF0dGVtcHQuDQoNClBsZWFzZSByZWdlbmVy
YXRlIGFuZCByZXNlbmQgaWYgeW91IHdhbnQgdGhpcyBtZXJnZWQuLi4NCg0KLS0gDQpUcm9uZCBN
eWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lcg0KDQpOZXRBcHANClRyb25kLk15
a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNvbQ0K
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.
Reported-and-tested-by: Chris Hiestand <[email protected]>
Reference: http://bugs.debian.org/669314
Signed-off-by: Ben Hutchings <[email protected]>
Cc: <[email protected]> # 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 31fdb03..5eaf902 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 a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
index 017b4b0..94e8652 100644
--- a/fs/nfs/nfs4namespace.c
+++ b/fs/nfs/nfs4namespace.c
@@ -81,7 +81,7 @@ static char *nfs_path_component(const char *nfspath, const char *end)
static char *nfs4_path(struct dentry *dentry, char *buffer, ssize_t buflen)
{
char *limit;
- char *path = nfs_path(&limit, dentry, buffer, buflen);
+ char *path = nfs_path(&limit, dentry, buffer, buflen, true);
if (!IS_ERR(path)) {
char *path_component = nfs_path_component(path, limit);
if (path_component)
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index ac6a3c5..6087ed0 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -730,7 +730,7 @@ int nfs_show_devname(struct seq_file *m, struct dentry *root)
int err = 0;
if (!page)
return -ENOMEM;
- devname = nfs_path(&dummy, root, page, PAGE_SIZE);
+ devname = nfs_path(&dummy, root, page, PAGE_SIZE, false);
if (IS_ERR(devname))
err = PTR_ERR(devname);
else
From: Ben Hutchings <[email protected]>
Date: Sun, 21 Oct 2012 19:23:52 +0100
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.
[[email protected]: use flag instead of bool argument]
Reported-and-tested-by: Chris Hiestand <[email protected]>
Reference: http://bugs.debian.org/669314
Signed-off-by: Ben Hutchings <[email protected]>
Cc: <[email protected]> # v2.6.39+
Signed-off-by: Jonathan Nieder <[email protected]>
---
Myklebust, Trond wrote:
> This patch will not apply to the upstream kernel; it seems rather to
> have been generated incrementally against an older patch attempt.
I assume you mean the patch
nfs: convert boolean nfs_path() argument to a flag word
does not apply against master. It is just an optional code style
improvement on top of Ben's
nfs: Show original device name verbatim in /proc/*/mount{s,info}
Here are the two squashed together for convenience.
Thanks,
Jonathan
fs/nfs/internal.h | 5 +++--
fs/nfs/namespace.c | 19 ++++++++++++++-----
fs/nfs/nfs4namespace.c | 3 ++-
fs/nfs/super.c | 2 +-
4 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 59b133c5d652..a54fe51c1dfb 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -353,8 +353,9 @@ extern void nfs_sb_active(struct super_block *sb);
extern void nfs_sb_deactive(struct super_block *sb);
/* namespace.c */
+#define NFS_PATH_CANONICAL 1
extern char *nfs_path(char **p, struct dentry *dentry,
- char *buffer, ssize_t buflen);
+ char *buffer, ssize_t buflen, unsigned flags);
extern struct vfsmount *nfs_d_automount(struct path *path);
struct vfsmount *nfs_submount(struct nfs_server *, struct dentry *,
struct nfs_fh *, struct nfs_fattr *);
@@ -498,7 +499,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, NFS_PATH_CANONICAL);
}
/*
diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
index 655925373b91..dd057bc6b65b 100644
--- a/fs/nfs/namespace.c
+++ b/fs/nfs/namespace.c
@@ -33,6 +33,7 @@ int nfs_mountpoint_expiry_timeout = 500 * HZ;
* @dentry - pointer to dentry
* @buffer - result buffer
* @buflen - length of buffer
+ * @flags - options (see below)
*
* Helper function for constructing the server pathname
* by arbitrary hashed dentry.
@@ -40,8 +41,14 @@ int nfs_mountpoint_expiry_timeout = 500 * HZ;
* This is mainly for use in figuring out the path on the
* server side when automounting on top of an existing partition
* and in generating /proc/mounts and friends.
+ *
+ * Supported flags:
+ * NFS_PATH_CANONICAL: ensure there is exactly one slash after
+ * the original device (export) name
+ * (if unset, the original name is returned verbatim)
*/
-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,
+ unsigned flags)
{
char *end;
int namelen;
@@ -74,7 +81,7 @@ rename_retry:
rcu_read_unlock();
goto rename_retry;
}
- if (*end != '/') {
+ if ((flags & NFS_PATH_CANONICAL) && *end != '/') {
if (--buflen < 0) {
spin_unlock(&dentry->d_lock);
rcu_read_unlock();
@@ -91,9 +98,11 @@ rename_retry:
return end;
}
namelen = strlen(base);
- /* Strip off excess slashes in base string */
- while (namelen > 0 && base[namelen - 1] == '/')
- namelen--;
+ if (flags & NFS_PATH_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 a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
index 79fbb61ce202..1e09eb78543b 100644
--- a/fs/nfs/nfs4namespace.c
+++ b/fs/nfs/nfs4namespace.c
@@ -81,7 +81,8 @@ static char *nfs_path_component(const char *nfspath, const char *end)
static char *nfs4_path(struct dentry *dentry, char *buffer, ssize_t buflen)
{
char *limit;
- char *path = nfs_path(&limit, dentry, buffer, buflen);
+ char *path = nfs_path(&limit, dentry, buffer, buflen,
+ NFS_PATH_CANONICAL);
if (!IS_ERR(path)) {
char *path_component = nfs_path_component(path, limit);
if (path_component)
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index e831bce49766..13c2a5be4765 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -771,7 +771,7 @@ int nfs_show_devname(struct seq_file *m, struct dentry *root)
int err = 0;
if (!page)
return -ENOMEM;
- devname = nfs_path(&dummy, root, page, PAGE_SIZE);
+ devname = nfs_path(&dummy, root, page, PAGE_SIZE, 0);
if (IS_ERR(devname))
err = PTR_ERR(devname);
else
--
1.8.0
(cc-ing Laurent in case he wants to test. Laurent, a "git am"-ready
patch is at [1])
Ben Hutchings wrote:
> 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.
>
> Reported-and-tested-by: Chris Hiestand <[email protected]>
> Reference: http://bugs.debian.org/669314
> Signed-off-by: Ben Hutchings <[email protected]>
> Cc: <[email protected]> # v2.6.39+
Changing the content of /proc/mounts broke
user@hostname:/proc/self$ sudo umount.nfs nfsserver:/srv/ubuntu-32
umount.nfs: nfsserver:/srv/ubuntu-32: not found
with nfs2 and nfs3 and this looks like the minimal change to get it
working again, so for what it's worth,
Reviewed-by: Jonathan Nieder <[email protected]>
How about something like the following on top?
-- >8 --
Subject: nfs: convert boolean nfs_path() argument to a flag word
If nfs_path() gains any other boolean settings, they can share the
flag argument, and this way call sites look like "nfs_path(&limit,
dentry, buffer, buflen, NFS_PATH_CANONICAL);" so the reader does not
need to guess the sense of true and false. No functional change
intended.
Signed-off-by: Jonathan Nieder <[email protected]>
---
[1] http://download.gmane.org/gmane.linux.nfs/52755/52756
fs/nfs/internal.h | 5 +++--
fs/nfs/namespace.c | 14 +++++++++-----
fs/nfs/nfs4namespace.c | 3 ++-
fs/nfs/super.c | 2 +-
4 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 9c9603373d64..a54fe51c1dfb 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -353,8 +353,9 @@ extern void nfs_sb_active(struct super_block *sb);
extern void nfs_sb_deactive(struct super_block *sb);
/* namespace.c */
+#define NFS_PATH_CANONICAL 1
extern char *nfs_path(char **p, struct dentry *dentry,
- char *buffer, ssize_t buflen, bool canonical);
+ char *buffer, ssize_t buflen, unsigned flags);
extern struct vfsmount *nfs_d_automount(struct path *path);
struct vfsmount *nfs_submount(struct nfs_server *, struct dentry *,
struct nfs_fh *, struct nfs_fattr *);
@@ -498,7 +499,7 @@ static inline char *nfs_devname(struct dentry *dentry,
char *buffer, ssize_t buflen)
{
char *dummy;
- return nfs_path(&dummy, dentry, buffer, buflen, true);
+ return nfs_path(&dummy, dentry, buffer, buflen, NFS_PATH_CANONICAL);
}
/*
diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
index 059975e492e1..dd057bc6b65b 100644
--- a/fs/nfs/namespace.c
+++ b/fs/nfs/namespace.c
@@ -33,8 +33,7 @@ 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
+ * @flags - options (see below)
*
* Helper function for constructing the server pathname
* by arbitrary hashed dentry.
@@ -42,9 +41,14 @@ int nfs_mountpoint_expiry_timeout = 500 * HZ;
* This is mainly for use in figuring out the path on the
* server side when automounting on top of an existing partition
* and in generating /proc/mounts and friends.
+ *
+ * Supported flags:
+ * NFS_PATH_CANONICAL: ensure there is exactly one slash after
+ * the original device (export) name
+ * (if unset, the original name is returned verbatim)
*/
char *nfs_path(char **p, struct dentry *dentry, char *buffer, ssize_t buflen,
- bool canonical)
+ unsigned flags)
{
char *end;
int namelen;
@@ -77,7 +81,7 @@ rename_retry:
rcu_read_unlock();
goto rename_retry;
}
- if (canonical && *end != '/') {
+ if ((flags & NFS_PATH_CANONICAL) && *end != '/') {
if (--buflen < 0) {
spin_unlock(&dentry->d_lock);
rcu_read_unlock();
@@ -94,7 +98,7 @@ rename_retry:
return end;
}
namelen = strlen(base);
- if (canonical) {
+ if (flags & NFS_PATH_CANONICAL) {
/* Strip off excess slashes in base string */
while (namelen > 0 && base[namelen - 1] == '/')
namelen--;
diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
index 2f6f16331769..1e09eb78543b 100644
--- a/fs/nfs/nfs4namespace.c
+++ b/fs/nfs/nfs4namespace.c
@@ -81,7 +81,8 @@ static char *nfs_path_component(const char *nfspath, const char *end)
static char *nfs4_path(struct dentry *dentry, char *buffer, ssize_t buflen)
{
char *limit;
- char *path = nfs_path(&limit, dentry, buffer, buflen, true);
+ char *path = nfs_path(&limit, dentry, buffer, buflen,
+ NFS_PATH_CANONICAL);
if (!IS_ERR(path)) {
char *path_component = nfs_path_component(path, limit);
if (path_component)
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index c89a73da13d9..13c2a5be4765 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -771,7 +771,7 @@ int nfs_show_devname(struct seq_file *m, struct dentry *root)
int err = 0;
if (!page)
return -ENOMEM;
- devname = nfs_path(&dummy, root, page, PAGE_SIZE, false);
+ devname = nfs_path(&dummy, root, page, PAGE_SIZE, 0);
if (IS_ERR(devname))
err = PTR_ERR(devname);
else
--
1.8.0.rc3