[PATCH v2] nfs: only show mountproto in /proc/mounts if set

[PATCH v2] nfs: only show mountproto in /proc/mounts if set

[Scott Mayhew]

The nfs_server->mountd_protocol field doesn't get set when a v3 submount
is created, causing /proc/mounts to show 'mountproto=' without a netid.
This in turn causes umount.nfs to emit the error message "Failed to find
'' protocol" if you manually unmount the submount.

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 fs/nfs/super.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 001796b..b60946d 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -532,7 +532,8 @@ static void nfs_show_mountd_netid(struct seq_file *m, struct nfs_server *nfss,
 {
 	struct sockaddr *sap = (struct sockaddr *) &nfss->mountd_address;
 
-	seq_printf(m, ",mountproto=");
+	if (nfss->mountd_protocol || showdefaults)
+		seq_printf(m, ",mountproto=");
 	switch (sap->sa_family) {
 	case AF_INET:
 		switch (nfss->mountd_protocol) {
-- 
2.4.11

Re: [PATCH v2] nfs: only show mountproto in /proc/mounts if set

[Trond Myklebust]

> On Nov 15, 2016, at 15:59, Scott Mayhew <smayhew@redhat.com> wrote:
>=20
> The nfs_server->mountd_protocol field doesn't get set when a v3 submount
> is created, causing /proc/mounts to show 'mountproto=3D' without a netid.
> This in turn causes umount.nfs to emit the error message "Failed to find
> '' protocol" if you manually unmount the submount.
>=20
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---
> fs/nfs/super.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>=20
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index 001796b..b60946d 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -532,7 +532,8 @@ static void nfs_show_mountd_netid(struct seq_file *m,=
 struct nfs_server *nfss,
> {
> =09struct sockaddr *sap =3D (struct sockaddr *) &nfss->mountd_address;
>=20
> -=09seq_printf(m, ",mountproto=3D");
> +=09if (nfss->mountd_protocol || showdefaults)
> +=09=09seq_printf(m, ",mountproto=3D");
> =09switch (sap->sa_family) {
> =09case AF_INET:
> =09=09switch (nfss->mountd_protocol) {

Does it make sense to call nfs_show_mountd_options() at all for the case of=
 a v3 submount?

Re: [PATCH v2] nfs: only show mountproto in /proc/mounts if set

[Scott Mayhew]

On Tue, 15 Nov 2016, Trond Myklebust wrote:

> 
> > On Nov 15, 2016, at 15:59, Scott Mayhew <smayhew@redhat.com> wrote:
> > 
> > The nfs_server->mountd_protocol field doesn't get set when a v3 submount
> > is created, causing /proc/mounts to show 'mountproto=' without a netid.
> > This in turn causes umount.nfs to emit the error message "Failed to find
> > '' protocol" if you manually unmount the submount.
> > 
> > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > ---
> > fs/nfs/super.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> > index 001796b..b60946d 100644
> > --- a/fs/nfs/super.c
> > +++ b/fs/nfs/super.c
> > @@ -532,7 +532,8 @@ static void nfs_show_mountd_netid(struct seq_file *m, struct nfs_server *nfss,
> > {
> > 	struct sockaddr *sap = (struct sockaddr *) &nfss->mountd_address;
> > 
> > -	seq_printf(m, ",mountproto=");
> > +	if (nfss->mountd_protocol || showdefaults)
> > +		seq_printf(m, ",mountproto=");
> > 	switch (sap->sa_family) {
> > 	case AF_INET:
> > 		switch (nfss->mountd_protocol) {
> 
> Does it make sense to call nfs_show_mountd_options() at all for the case of a v3 submount?
> 
Probably not, but I don't see a way to positively identify that it's a
submount if all you have is the nfs_server struct (I don't see a way to
backtrack to the mount struct).  Or are you suggesting to just check one
of the mountd-related fields (say mountd_addrlen) before calling
nfs_show_mount_options() and skip it altogether if appropriate?

-Scott

Re: [PATCH v2] nfs: only show mountproto in /proc/mounts if set

[Scott Mayhew]

On Wed, 16 Nov 2016, Scott Mayhew wrote:

> On Tue, 15 Nov 2016, Trond Myklebust wrote:
> 
> > 
> > > On Nov 15, 2016, at 15:59, Scott Mayhew <smayhew@redhat.com> wrote:
> > > 
> > > The nfs_server->mountd_protocol field doesn't get set when a v3 submount
> > > is created, causing /proc/mounts to show 'mountproto=' without a netid.
> > > This in turn causes umount.nfs to emit the error message "Failed to find
> > > '' protocol" if you manually unmount the submount.
> > > 
> > > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > > ---
> > > fs/nfs/super.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> > > index 001796b..b60946d 100644
> > > --- a/fs/nfs/super.c
> > > +++ b/fs/nfs/super.c
> > > @@ -532,7 +532,8 @@ static void nfs_show_mountd_netid(struct seq_file *m, struct nfs_server *nfss,
> > > {
> > > 	struct sockaddr *sap = (struct sockaddr *) &nfss->mountd_address;
> > > 
> > > -	seq_printf(m, ",mountproto=");
> > > +	if (nfss->mountd_protocol || showdefaults)
> > > +		seq_printf(m, ",mountproto=");
> > > 	switch (sap->sa_family) {
> > > 	case AF_INET:
> > > 		switch (nfss->mountd_protocol) {
> > 
> > Does it make sense to call nfs_show_mountd_options() at all for the case of a v3 submount?
> > 
> Probably not, but I don't see a way to positively identify that it's a
> submount if all you have is the nfs_server struct (I don't see a way to
> backtrack to the mount struct).  Or are you suggesting to just check one
> of the mountd-related fields (say mountd_addrlen) before calling
> nfs_show_mount_options() and skip it altogether if appropriate?
> 

Having looked at it a little more, I do see a few ways but none of them
are particularly good.

One way would be to dereference nfss->super and walk the s_mounts list
looking for any mount whose mnt_expire list is nonempty... but comment
beside the s_mounts declaration says '_not_ for fs use', plus it doesn't
look like the mount_lock is meant to be used outside of the vfs layer
either, so scratch that.

Another way would be to walk the nfs_automount_list, comparing nfss to
mnt->mnt.mnt_sb->s_fs_info... I made a patch to do that but it works but
it adds the overhead of calling a function just so we can (maybe) avoid
calling nfs_show_mount_options() which is inlined anyways.

A third way would be to add a flag to the  nfs_server->flags field to
indicate that it's a submount... I don't see any use for said flag aside
from checking it in nfs_show_mount_options(), so that seems like wasting
a flag.

I still prefer the original patch since it pretty much restores the
behavior that was there before commit ee671b016, and all I'm trying to
do is squelch a harmless error message (since the umount still
succeeds).

-Scott

[PATCH] NFS: tidy up nfs_show_mountd_netid

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2253 bytes --]


This function is a bit clumsy, incorrectly producing
",mountproto=" if mountd_protocol is 0 and !showdefaults,
and duplicating the code for reporting "auto".

Tidy it up so that it only makes a single seq_printf() call,
and more obviously does the right thing.

Fixes: ee671b016fbf ("NFS: convert proto= option to use netids rather than a protoname")
Signed-off-by: NeilBrown <neilb@suse.com>
---

While I don't object to Scott's patch to fix this issue, this is a
function that could usefully be simplified as well as fixed, so I offer
my own alternative.... partly just to keep the email-thread alive.

Quite apart from whether this function *should* be called for v3
submounts, I think it would be good for the function to be robust.

Thanks,
NeilBrown


 fs/nfs/super.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 6bca17883b93..54e0f9f2dd94 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -531,39 +531,32 @@ static void nfs_show_mountd_netid(struct seq_file *m, struct nfs_server *nfss,
 				  int showdefaults)
 {
 	struct sockaddr *sap = (struct sockaddr *) &nfss->mountd_address;
+	char *proto = NULL;
 
-	seq_printf(m, ",mountproto=");
 	switch (sap->sa_family) {
 	case AF_INET:
 		switch (nfss->mountd_protocol) {
 		case IPPROTO_UDP:
-			seq_printf(m, RPCBIND_NETID_UDP);
+			proto = RPCBIND_NETID_UDP;
 			break;
 		case IPPROTO_TCP:
-			seq_printf(m, RPCBIND_NETID_TCP);
+			proto = RPCBIND_NETID_TCP;
 			break;
-		default:
-			if (showdefaults)
-				seq_printf(m, "auto");
 		}
 		break;
 	case AF_INET6:
 		switch (nfss->mountd_protocol) {
 		case IPPROTO_UDP:
-			seq_printf(m, RPCBIND_NETID_UDP6);
+			proto = RPCBIND_NETID_UDP6;
 			break;
 		case IPPROTO_TCP:
-			seq_printf(m, RPCBIND_NETID_TCP6);
+			proto = RPCBIND_NETID_TCP6;
 			break;
-		default:
-			if (showdefaults)
-				seq_printf(m, "auto");
 		}
 		break;
-	default:
-		if (showdefaults)
-			seq_printf(m, "auto");
 	}
+	if (proto || showdefaults)
+		seq_printf(m, ",mountproto=%s", proto ?: "auto");
 }
 
 static void nfs_show_mountd_options(struct seq_file *m, struct nfs_server *nfss,
-- 
2.11.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]