[PATCH] nfsmount.conf: New variables that explicitly set default

[PATCH] nfsmount.conf: New variables that explicitly set default

[Steve Dickson]

This patch introduces two new mount configuration variables used
to set the default protocol version and network transport.

Currently when the nfsvers or proto is set in the nfsmount.conf
file the mount will fail if the server does not support 
the given version or  transport. No negation with the server occurs.

With default values, they define where the server negation should start.
So the 'default_nfsvers=' and default_proto=' will now define where the
server negation will start. Meaning just because they are set to a 
certain value does not meant that will be the final value used in
mount. 

comments?

steved.

commit a0e50cbe0e24abe0226ef240bd547e5e9e67de3f
Author: Steve Dickson <steved@redhat.com>
Date:   Fri Oct 9 13:54:52 2009 -0400

    Introduces a new type of configuration file variable
    that will explicitly set the default protocol version
    and network transport. These variables will set where
    the negotiation with the server will start.
    
    Signed-off-by: Steve Dickson <steved@redhat.com>

diff --git a/support/include/conffile.h b/support/include/conffile.h
index 672020a..da8e836 100644
--- a/support/include/conffile.h
+++ b/support/include/conffile.h
@@ -75,4 +75,12 @@ static inline void upper2lower(char *str)
 	while ((c = tolower(*str)))
 		*str++ = c;
 }
+
+/*
+ * Default Mount options
+ */
+#define DEFAULT_PROTO 0x1
+#define DEFAULT_VERS  0x2
+extern int config_default_opts;
+
 #endif				/* _CONFFILE_H_ */
diff --git a/utils/mount/configfile.c b/utils/mount/configfile.c
index d3285f8..c336670 100644
--- a/utils/mount/configfile.c
+++ b/utils/mount/configfile.c
@@ -185,7 +185,7 @@ void free_all(void)
 		free(entry);
 	}
 }
-static char *versions[] = {"v2", "v3", "v4", "vers", "nfsvers", NULL};
+char *versions[] = {"v2", "v3", "v4", "vers", "nfsvers", NULL};
 int inline check_vers(char *mopt, char *field)
 {
 	int i;
@@ -197,6 +197,34 @@ int inline check_vers(char *mopt, char *field)
 	}
 	return 0;
 }
+
+/*
+ * See if there are any default values being set.
+ * If so mark the bit and turn the option into a 
+ * valid option.
+ */
+int config_default_opts;
+void inline set_default_bits(char *mopt)
+{
+	int i;
+	char *field;
+
+	if (strncasecmp(mopt, "default_", strlen("default_")) != 0)
+		return;
+
+	field = strchr(mopt, '_');
+	field++; /* skip pass '_' */
+	if (strncasecmp(field, "proto", strlen("proto")) == 0) {
+		config_default_opts |= DEFAULT_PROTO;
+		strcpy(mopt, field);
+	} else for (i=0; versions[i]; i++) {
+		if (strncasecmp(field, versions[i], strlen(versions[i])) == 0) {
+			config_default_opts |= DEFAULT_VERS;
+			strcpy(mopt, field);
+			break;
+		}
+	}
+}
 /*
  * Parse the given section of the configuration 
  * file to if there are any mount options set.
@@ -325,6 +353,7 @@ char *conf_get_mntopts(char *spec, char *mount_point,
 		strcat(config_opts, ",");
 	}
 	SLIST_FOREACH(entry, &head, entries) {
+		set_default_bits(entry->opt);
 		strcat(config_opts, entry->opt);
 		strcat(config_opts, ",");
 	}
diff --git a/utils/mount/nfsmount.conf b/utils/mount/nfsmount.conf
index 991838f..af76d49 100644
--- a/utils/mount/nfsmount.conf
+++ b/utils/mount/nfsmount.conf
@@ -28,10 +28,24 @@
 # This statically named section defines global mount 
 # options that can be applied on all NFS mount.
 #
-# Protocol Version [2,3]
-# Nfsvers=3
+# Protocol Version [2,3,4]
+# This defines the default protocol version which will
+# be used to start the negotiation with the server.
+# Default_nfsvers=4
+#
+# Setting this option makes it mandatory the server supports the
+# given version. The mount will fail if the given version is 
+# not support by the server. 
+# Nfsvers=4
 #
 # Network Transport [udp,tcp,rdma] (Note: values are case sensitive)
+# This defines the default network transport which will
+# be used to start the negotiation with the server.
+# Default_Proto=tcp
+#
+# Setting this option makes it mandatory the server supports the
+# given transport. The mount will fail if the given transport
+# is not supported by the server.
 # Proto=tcp
 #
 # The number of times a request will be retired before 
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 069bdc1..b203414 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -92,6 +92,13 @@ struct nfsmount_info {
 				child;		/* forked bg child? */
 };
 
+#ifdef MOUNT_CONFIG
+#include "conffile.h"
+int inline config_defaults(struct nfsmount_info *mi);
+#else
+int inline config_defaults(struct nfsmount_info *mi) {return 0;}
+#endif
+
 /*
  * Obtain a retry timeout value based on the value of the "retry=" option.
  *
@@ -610,10 +617,22 @@ static int nfs_try_mount(struct nfsmount_info *mi)
 	case 2:
 	case 3:
 		result = nfs_try_mount_v3v2(mi);
+		if (result)
+			break;
+
+		if (config_defaults(mi))
+			result = nfs_try_mount_v3v2(mi);
 		break;
+
 	case 4:
 		result = nfs_try_mount_v4(mi);
+		if (result)
+			break;
+
+		if (config_defaults(mi))
+			result = nfs_try_mount_v3v2(mi);
 		break;
+
 	default:
 		errno = EIO;
 	}
@@ -819,3 +838,38 @@ int nfsmount_string(const char *spec, const char *node, const char *type,
 	free(mi.hostname);
 	return retval;
 }
+
+#ifdef MOUNT_CONFIG
+/*
+ * If the mount fails due to a ENOSUPPORT type of error
+ * and a default options were set in the configuration file
+ * try the mount again without that default options set.
+ */
+int config_defaults(struct nfsmount_info *mi)
+{
+	int i;
+	extern char *versions[];
+
+	if (!config_default_opts)
+		return 0;
+
+	switch(mi->version) {
+	case 4:
+		if (errno != EPROTONOSUPPORT)
+			return 0;
+		break;
+	case 3:
+		if (errno != ESPIPE)
+			return 0;
+		break;
+	}
+	if (config_default_opts && DEFAULT_VERS) {
+		for (i=0; versions[i]; i++) 
+			po_remove_all(mi->options, versions[i]);
+	}
+	if (config_default_opts && DEFAULT_PROTO)
+		po_remove_all(mi->options, "proto");
+
+	return 1;
+}
+#endif

 

Re: [PATCH] nfsmount.conf: New variables that explicitly set default

[Chuck Lever]

On Oct 9, 2009, at 2:16 PM, Steve Dickson wrote:
> This patch introduces two new mount configuration variables used
> to set the default protocol version and network transport.
>
> Currently when the nfsvers or proto is set in the nfsmount.conf
> file the mount will fail if the server does not support
> the given version or  transport. No negation with the server occurs.
>
> With default values, they define where the server negation should  
> start.
> So the 'default_nfsvers=' and default_proto=' will now define where  
> the
> server negation will start. Meaning just because they are set to a
> certain value does not meant that will be the final value used in
> mount.
>
> comments?

I worry that this kind of thing will be difficult to support when we  
move version and transport negotiation into the kernel.

More below.

> steved.
>
> commit a0e50cbe0e24abe0226ef240bd547e5e9e67de3f
> Author: Steve Dickson <steved@redhat.com>
> Date:   Fri Oct 9 13:54:52 2009 -0400
>
>    Introduces a new type of configuration file variable
>    that will explicitly set the default protocol version
>    and network transport. These variables will set where
>    the negotiation with the server will start.
>
>    Signed-off-by: Steve Dickson <steved@redhat.com>
>
> diff --git a/support/include/conffile.h b/support/include/conffile.h
> index 672020a..da8e836 100644
> --- a/support/include/conffile.h
> +++ b/support/include/conffile.h
> @@ -75,4 +75,12 @@ static inline void upper2lower(char *str)
> 	while ((c = tolower(*str)))
> 		*str++ = c;
> }
> +
> +/*
> + * Default Mount options
> + */
> +#define DEFAULT_PROTO 0x1
> +#define DEFAULT_VERS  0x2
> +extern int config_default_opts;
> +
> #endif				/* _CONFFILE_H_ */
> diff --git a/utils/mount/configfile.c b/utils/mount/configfile.c
> index d3285f8..c336670 100644
> --- a/utils/mount/configfile.c
> +++ b/utils/mount/configfile.c
> @@ -185,7 +185,7 @@ void free_all(void)
> 		free(entry);
> 	}
> }
> -static char *versions[] = {"v2", "v3", "v4", "vers", "nfsvers",  
> NULL};
> +char *versions[] = {"v2", "v3", "v4", "vers", "nfsvers", NULL};
> int inline check_vers(char *mopt, char *field)
> {
> 	int i;
> @@ -197,6 +197,34 @@ int inline check_vers(char *mopt, char *field)
> 	}
> 	return 0;
> }
> +
> +/*
> + * See if there are any default values being set.
> + * If so mark the bit and turn the option into a
> + * valid option.
> + */
> +int config_default_opts;
> +void inline set_default_bits(char *mopt)
> +{
> +	int i;
> +	char *field;
> +
> +	if (strncasecmp(mopt, "default_", strlen("default_")) != 0)
> +		return;
> +
> +	field = strchr(mopt, '_');
> +	field++; /* skip pass '_' */
> +	if (strncasecmp(field, "proto", strlen("proto")) == 0) {
> +		config_default_opts |= DEFAULT_PROTO;
> +		strcpy(mopt, field);
> +	} else for (i=0; versions[i]; i++) {
> +		if (strncasecmp(field, versions[i], strlen(versions[i])) == 0) {
> +			config_default_opts |= DEFAULT_VERS;
> +			strcpy(mopt, field);
> +			break;
> +		}
> +	}
> +}
> /*
>  * Parse the given section of the configuration
>  * file to if there are any mount options set.
> @@ -325,6 +353,7 @@ char *conf_get_mntopts(char *spec, char  
> *mount_point,
> 		strcat(config_opts, ",");
> 	}
> 	SLIST_FOREACH(entry, &head, entries) {
> +		set_default_bits(entry->opt);
> 		strcat(config_opts, entry->opt);
> 		strcat(config_opts, ",");
> 	}
> diff --git a/utils/mount/nfsmount.conf b/utils/mount/nfsmount.conf
> index 991838f..af76d49 100644
> --- a/utils/mount/nfsmount.conf
> +++ b/utils/mount/nfsmount.conf
> @@ -28,10 +28,24 @@
> # This statically named section defines global mount
> # options that can be applied on all NFS mount.
> #
> -# Protocol Version [2,3]
> -# Nfsvers=3
> +# Protocol Version [2,3,4]
> +# This defines the default protocol version which will
> +# be used to start the negotiation with the server.
> +# Default_nfsvers=4
> +#
> +# Setting this option makes it mandatory the server supports the
> +# given version. The mount will fail if the given version is
> +# not support by the server.
> +# Nfsvers=4
> #
> # Network Transport [udp,tcp,rdma] (Note: values are case sensitive)
> +# This defines the default network transport which will
> +# be used to start the negotiation with the server.
> +# Default_Proto=tcp
> +#
> +# Setting this option makes it mandatory the server supports the
> +# given transport. The mount will fail if the given transport
> +# is not supported by the server.
> # Proto=tcp
> #
> # The number of times a request will be retired before
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index 069bdc1..b203414 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -92,6 +92,13 @@ struct nfsmount_info {
> 				child;		/* forked bg child? */
> };
>
> +#ifdef MOUNT_CONFIG
> +#include "conffile.h"
> +int inline config_defaults(struct nfsmount_info *mi);
> +#else
> +int inline config_defaults(struct nfsmount_info *mi) {return 0;}
> +#endif
> +
> /*
>  * Obtain a retry timeout value based on the value of the "retry="  
> option.
>  *
> @@ -610,10 +617,22 @@ static int nfs_try_mount(struct nfsmount_info  
> *mi)
> 	case 2:
> 	case 3:
> 		result = nfs_try_mount_v3v2(mi);
> +		if (result)
> +			break;
> +
> +		if (config_defaults(mi))
> +			result = nfs_try_mount_v3v2(mi);
> 		break;
> +
> 	case 4:
> 		result = nfs_try_mount_v4(mi);
> +		if (result)
> +			break;
> +
> +		if (config_defaults(mi))
> +			result = nfs_try_mount_v3v2(mi);
> 		break;
> +
> 	default:
> 		errno = EIO;
> 	}

A better approach to all of this might be to use the config file to  
set mi->version.  mi->version already controls which versions are  
tried here.  Do away with the idea of adding mount options to get  
default version behavior.

If mi->version is 0, we get full v4/v3/v2 negotiation.  If mi->version  
is 3, we get just v2 and v3.  Do we need any more flexibility than that?

All you have to do is this: in nfs_validate_options(), after we've  
looked at the user option string to see what version is specified,  
check: if mi->version is still 0, then look in the config file to see  
what kind of negotiation behavior is desired.  Done.

I'd say you should split the "default protocol" option into a separate  
patch, because something quite different will be needed there.  But  
I'm still not clear on why someone wouldn't just say "proto=udp" or  
"proto=rdma" to avoid negotiating.

> @@ -819,3 +838,38 @@ int nfsmount_string(const char *spec, const  
> char *node, const char *type,
> 	free(mi.hostname);
> 	return retval;
> }
> +
> +#ifdef MOUNT_CONFIG
> +/*
> + * If the mount fails due to a ENOSUPPORT type of error
> + * and a default options were set in the configuration file
> + * try the mount again without that default options set.
> + */
> +int config_defaults(struct nfsmount_info *mi)
> +{
> +	int i;
> +	extern char *versions[];
> +
> +	if (!config_default_opts)
> +		return 0;
> +
> +	switch(mi->version) {
> +	case 4:
> +		if (errno != EPROTONOSUPPORT)
> +			return 0;
> +		break;
> +	case 3:
> +		if (errno != ESPIPE)
> +			return 0;
> +		break;
> +	}
> +	if (config_default_opts && DEFAULT_VERS) {
> +		for (i=0; versions[i]; i++)
> +			po_remove_all(mi->options, versions[i]);
> +	}
> +	if (config_default_opts && DEFAULT_PROTO)
> +		po_remove_all(mi->options, "proto");
> +
> +	return 1;
> +}
> +#endif

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

Re: [PATCH] nfsmount.conf: New variables that explicitly set default

[Steve Dickson]

First of all thank you for you comments so soon.. 
They are very  much appreciated! 

On 10/09/2009 04:29 PM, Chuck Lever wrote:
> On Oct 9, 2009, at 2:16 PM, Steve Dickson wrote:
>> This patch introduces two new mount configuration variables used
>> to set the default protocol version and network transport.
>>
>> Currently when the nfsvers or proto is set in the nfsmount.conf
>> file the mount will fail if the server does not support
>> the given version or  transport. No negation with the server occurs.
>>
>> With default values, they define where the server negation should start.
>> So the 'default_nfsvers=' and default_proto=' will now define where the
>> server negation will start. Meaning just because they are set to a
>> certain value does not meant that will be the final value used in
>> mount.
>>
>> comments?
> 
> I worry that this kind of thing will be difficult to support when we
> move version and transport negotiation into the kernel.
There was a lot of positive feed back on being able to control
things like defining the default version and transport from user 
level configuration files. But I do understand your point, so maybe
its not a good idea to move negotiation down to the kernel. I think its
clear that is much easier to support and change these types of 
negotiations from user level...

>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>> index 069bdc1..b203414 100644
>> --- a/utils/mount/stropts.c
>> +++ b/utils/mount/stropts.c
>> @@ -92,6 +92,13 @@ struct nfsmount_info {
>>                 child;        /* forked bg child? */
>> };
>>
>> +#ifdef MOUNT_CONFIG
>> +#include "conffile.h"
>> +int inline config_defaults(struct nfsmount_info *mi);
>> +#else
>> +int inline config_defaults(struct nfsmount_info *mi) {return 0;}
>> +#endif
>> +
>> /*
>>  * Obtain a retry timeout value based on the value of the "retry="
>> option.
>>  *
>> @@ -610,10 +617,22 @@ static int nfs_try_mount(struct nfsmount_info *mi)
>>     case 2:
>>     case 3:
>>         result = nfs_try_mount_v3v2(mi);
>> +        if (result)
>> +            break;
>> +
>> +        if (config_defaults(mi))
>> +            result = nfs_try_mount_v3v2(mi);
>>         break;
>> +
>>     case 4:
>>         result = nfs_try_mount_v4(mi);
>> +        if (result)
>> +            break;
>> +
>> +        if (config_defaults(mi))
>> +            result = nfs_try_mount_v3v2(mi);
>>         break;
>> +
>>     default:
>>         errno = EIO;
>>     }
> 
> A better approach to all of this might be to use the config file to set
> mi->version.  mi->version already controls which versions are tried
> here.  Do away with the idea of adding mount options to get default
> version behavior.
> 
> If mi->version is 0, we get full v4/v3/v2 negotiation.  If mi->version
> is 3, we get just v2 and v3.  Do we need any more flexibility than that?
Well at the point where the config file is being read, there is no
access to the struct nfsmount_info since it local to the mount/stropts.c
file. That could change but I kinda like the way its designed... 

> 
> All you have to do is this: in nfs_validate_options(), after we've
> looked at the user option string to see what version is specified,
> check: if mi->version is still 0, then look in the config file to see
> what kind of negotiation behavior is desired.  Done.
This does not jive with how the config parsing code works. The code
appends to the current options (if they don't already exists), well
before nfs_validate_options() is called.. 

I'm not saying using the mi->version to start the negotiation is
bad idea, its just not available when the configuration file
is read..  
 
> 
> I'd say you should split the "default protocol" option into a separate
> patch, because something quite different will be needed there.  
yeah, that was laziness on my part... But if figured the code was
not that complicated so I figured I could get away with it.. 

> But I'm still not clear on why someone wouldn't just say "proto=udp" or
> "proto=rdma" to avoid negotiating.
They can and that's the way it works today... What I'm introducing is a 
negotiable variable that will allow mount to succeed when servers don't
support the client first offering.

steved.

Re: [PATCH] nfsmount.conf: New variables that explicitly set default

[Chuck Lever]

On Oct 9, 2009, at 7:16 PM, Steve Dickson wrote:

> First of all thank you for you comments so soon..
> They are very  much appreciated!
>
> On 10/09/2009 04:29 PM, Chuck Lever wrote:
>> On Oct 9, 2009, at 2:16 PM, Steve Dickson wrote:
>>> This patch introduces two new mount configuration variables used
>>> to set the default protocol version and network transport.
>>>
>>> Currently when the nfsvers or proto is set in the nfsmount.conf
>>> file the mount will fail if the server does not support
>>> the given version or  transport. No negation with the server occurs.
>>>
>>> With default values, they define where the server negation should  
>>> start.
>>> So the 'default_nfsvers=' and default_proto=' will now define  
>>> where the
>>> server negation will start. Meaning just because they are set to a
>>> certain value does not meant that will be the final value used in
>>> mount.
>>>
>>> comments?
>>
>> I worry that this kind of thing will be difficult to support when we
>> move version and transport negotiation into the kernel.
> There was a lot of positive feed back on being able to control
> things like defining the default version and transport from user
> level configuration files. But I do understand your point, so maybe
> its not a good idea to move negotiation down to the kernel. I think  
> its
> clear that is much easier to support and change these types of
> negotiations from user level...

You'd have to take that up with Trond.  Our goal for several years has  
been to handle as much of this as possible in the kernel.

Using a config file in this particular way could make it tougher down  
the road for a kernel implementation.  There might be other mechanisms  
for accomplishing this kind of tuning that might be easier to deal  
with in the kernel.  I'd just like us to think carefully about the  
user interface and API choices we are making right now so we don't  
paint ourselves into a corner.

>>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>>> index 069bdc1..b203414 100644
>>> --- a/utils/mount/stropts.c
>>> +++ b/utils/mount/stropts.c
>>> @@ -92,6 +92,13 @@ struct nfsmount_info {
>>>                child;        /* forked bg child? */
>>> };
>>>
>>> +#ifdef MOUNT_CONFIG
>>> +#include "conffile.h"
>>> +int inline config_defaults(struct nfsmount_info *mi);
>>> +#else
>>> +int inline config_defaults(struct nfsmount_info *mi) {return 0;}
>>> +#endif
>>> +
>>> /*
>>> * Obtain a retry timeout value based on the value of the "retry="
>>> option.
>>> *
>>> @@ -610,10 +617,22 @@ static int nfs_try_mount(struct  
>>> nfsmount_info *mi)
>>>    case 2:
>>>    case 3:
>>>        result = nfs_try_mount_v3v2(mi);
>>> +        if (result)
>>> +            break;
>>> +
>>> +        if (config_defaults(mi))
>>> +            result = nfs_try_mount_v3v2(mi);
>>>        break;
>>> +
>>>    case 4:
>>>        result = nfs_try_mount_v4(mi);
>>> +        if (result)
>>> +            break;
>>> +
>>> +        if (config_defaults(mi))
>>> +            result = nfs_try_mount_v3v2(mi);
>>>        break;
>>> +
>>>    default:
>>>        errno = EIO;
>>>    }
>>
>> A better approach to all of this might be to use the config file to  
>> set
>> mi->version.  mi->version already controls which versions are tried
>> here.  Do away with the idea of adding mount options to get default
>> version behavior.
>>
>> If mi->version is 0, we get full v4/v3/v2 negotiation.  If mi- 
>> >version
>> is 3, we get just v2 and v3.  Do we need any more flexibility than  
>> that?
> Well at the point where the config file is being read, there is no
> access to the struct nfsmount_info since it local to the mount/ 
> stropts.c
> file. That could change but I kinda like the way its designed...
>
>>
>> All you have to do is this: in nfs_validate_options(), after we've
>> looked at the user option string to see what version is specified,
>> check: if mi->version is still 0, then look in the config file to see
>> what kind of negotiation behavior is desired.  Done.
> This does not jive with how the config parsing code works. The code
> appends to the current options (if they don't already exists), well
> before nfs_validate_options() is called..

Right, don't do this particular trick by appending additional mount  
options.  Setting mount's "default version" is not something you want  
to do via existing mount options, for exactly the reasons you stated  
in the patch description.  Likewise with a default protocol.

We have a handful of mount options that adjust mount.nfs's behavior  
rather than specifying the behavior of the mountpoint: bg, fg, and  
retry= being the most obvious examples.  Maybe we want a new mount  
option like defaultvers or defaultproto, which would fall into the  
same category.  But the semantics of the existing vers= and proto=  
options just don't support this kind of thing, and I think we should  
be extremely careful about abusing them like this.

> I'm not saying using the mi->version to start the negotiation is
> bad idea, its just not available when the configuration file
> is read..

What I'm suggesting is that by the time you do get to  
nfs_validate_options() you can read the config file again and look for  
the default version setting.

Alternately, you can read and parse the config file at the start, and  
stuff all that info into a data structure.  Then any place in the code  
can call an API to get what little piece of data they need.

I've seen plenty of apps that read through the config file at whatever  
point they want to look for a particular setting.

>> I'd say you should split the "default protocol" option into a  
>> separate
>> patch, because something quite different will be needed there.
> yeah, that was laziness on my part... But if figured the code was
> not that complicated so I figured I could get away with it..
>
>> But I'm still not clear on why someone wouldn't just say  
>> "proto=udp" or
>> "proto=rdma" to avoid negotiating.
> They can and that's the way it works today... What I'm introducing  
> is a
> negotiable variable that will allow mount to succeed when servers  
> don't
> support the client first offering.

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

Re: [PATCH] nfsmount.conf: New variables that explicitly set default

[Steve Dickson]

On 10/09/2009 08:38 PM, Chuck Lever wrote:
> 
> On Oct 9, 2009, at 7:16 PM, Steve Dickson wrote:
>>>
>>> I worry that this kind of thing will be difficult to support when we
>>> move version and transport negotiation into the kernel.
>> There was a lot of positive feed back on being able to control
>> things like defining the default version and transport from user
>> level configuration files. But I do understand your point, so maybe
>> its not a good idea to move negotiation down to the kernel. I think its
>> clear that is much easier to support and change these types of
>> negotiations from user level...
> 
> You'd have to take that up with Trond.  Our goal for several years has
> been to handle as much of this as possible in the kernel.
> 
> Using a config file in this particular way could make it tougher down
> the road for a kernel implementation.  There might be other mechanisms
> for accomplishing this kind of tuning that might be easier to deal with
> in the kernel.  I'd just like us to think carefully about the user
> interface and API choices we are making right now so we don't paint
> ourselves into a corner.
Regardless of how much is moved down into the kernel, there
will always be a mount command that will supply information to
the kernel via the mount system call. All this config file
does is provide another way (other than the command line) to supply 
mount with that information. A configuration file [or command line]
feed the API, they don't define it... 
 

>>>
>>> All you have to do is this: in nfs_validate_options(), after we've
>>> looked at the user option string to see what version is specified,
>>> check: if mi->version is still 0, then look in the config file to see
>>> what kind of negotiation behavior is desired.  Done.
>> This does not jive with how the config parsing code works. The code
>> appends to the current options (if they don't already exists), well
>> before nfs_validate_options() is called..
> 
> Right, don't do this particular trick by appending additional mount
> options.  Setting mount's "default version" is not something you want to
> do via existing mount options, for exactly the reasons you stated in the
> patch description.  Likewise with a default protocol.
Well that is *exactly* what people want, have complete control
of their environment, at least that is the feed back I've gotten...

I realize you are philosophically opposed  to having a configure
file and allowing people to define defaults. You have made that 
clear (at least that's my interpretation) from our previous discussions.
I understand that and truly do respect that opinion... But there has 
been an overwhelming acceptance from other parts of the community 
so I'm hard pressed to let this one opinion stand in the way of 
that acceptance.... So with that said... 

I would like to stay focus on the technical merit of this patch. 
Meaning are there any holes in that will cause the mount to drop
core; is there anything I'm missing that will cause mounts to
unnecessarily fail. Things of that nature...     
 
> 
> We have a handful of mount options that adjust mount.nfs's behavior
> rather than specifying the behavior of the mountpoint: bg, fg, and
> retry= being the most obvious examples.  Maybe we want a new mount
> option like defaultvers or defaultproto, which would fall into the same
> category.  But the semantics of the existing vers= and proto= options
> just don't support this kind of thing, and I think we should be
> extremely careful about abusing them like this.
More mount options??? No... that is not the answer... IMHO..

> 
>> I'm not saying using the mi->version to start the negotiation is
>> bad idea, its just not available when the configuration file
>> is read..
> 
> What I'm suggesting is that by the time you do get to
> nfs_validate_options() you can read the config file again and look for
> the default version setting.
After look at this, I see it really does not matter when and where
the file is read. And yes if the file was read from nfs_validate_options()
than yes I could set mi->version to do the negotiation, but that does
not take care of the network transport negotiation part of it... Meaning
the "proto" would still have to be removed to do the renegotiation... 

So the purposed patch is the better approach, at this point, since it handle
both types of negotiation in the same place, in the same way... 

> 
> Alternately, you can read and parse the config file at the start, and
> stuff all that info into a data structure.  Then any place in the code
> can call an API to get what little piece of data they need.
I don't want to add this type of complexity... Its simply not needed,
to solve this issue...  

steved.

Re: [PATCH] nfsmount.conf: New variables that explicitly set default

[Chuck Lever]

On Oct 12, 2009, at 5:24 AM, Steve Dickson wrote:

> On 10/09/2009 08:38 PM, Chuck Lever wrote:
>>
>> On Oct 9, 2009, at 7:16 PM, Steve Dickson wrote:
>>>>
>>>> I worry that this kind of thing will be difficult to support when  
>>>> we
>>>> move version and transport negotiation into the kernel.
>>> There was a lot of positive feed back on being able to control
>>> things like defining the default version and transport from user
>>> level configuration files. But I do understand your point, so maybe
>>> its not a good idea to move negotiation down to the kernel. I  
>>> think its
>>> clear that is much easier to support and change these types of
>>> negotiations from user level...
>>
>> You'd have to take that up with Trond.  Our goal for several years  
>> has
>> been to handle as much of this as possible in the kernel.
>>
>> Using a config file in this particular way could make it tougher down
>> the road for a kernel implementation.  There might be other  
>> mechanisms
>> for accomplishing this kind of tuning that might be easier to deal  
>> with
>> in the kernel.  I'd just like us to think carefully about the user
>> interface and API choices we are making right now so we don't paint
>> ourselves into a corner.
> Regardless of how much is moved down into the kernel, there
> will always be a mount command that will supply information to
> the kernel via the mount system call. All this config file
> does is provide another way (other than the command line) to supply
> mount with that information. A configuration file [or command line]
> feed the API, they don't define it...


>>>> All you have to do is this: in nfs_validate_options(), after we've
>>>> looked at the user option string to see what version is specified,
>>>> check: if mi->version is still 0, then look in the config file to  
>>>> see
>>>> what kind of negotiation behavior is desired.  Done.
>>> This does not jive with how the config parsing code works. The code
>>> appends to the current options (if they don't already exists), well
>>> before nfs_validate_options() is called..
>>
>> Right, don't do this particular trick by appending additional mount
>> options.  Setting mount's "default version" is not something you  
>> want to
>> do via existing mount options, for exactly the reasons you stated  
>> in the
>> patch description.  Likewise with a default protocol.
> Well that is *exactly* what people want, have complete control
> of their environment, at least that is the feed back I've gotten...

> I realize you are philosophically opposed  to having a configure
> file and allowing people to define defaults. You have made that
> clear (at least that's my interpretation) from our previous  
> discussions.

You are reading way too much into this.  I'm not at all trying to stop  
work on the config file.  If I were, why would I have given you clean  
patches to support vers=4 and version negotiation?  Read my lips: I'm  
over it, I've been over it for a while, and I'm now trying to help you  
get it right.

There is no hidden agenda here.  My interest is a useful and clean  
implementation that is easy to maintain, and fits in with our other  
plans for NFS mounting.  And besides, what possible political  
advantage would there be for me to stymie your project?

I might make a similar assumption about your arguments: you are  
philosophically opposed to doing NFS mount processing in the kernel  
(and you've said as much, recently, on this mailing list).  Now you  
are trying your damndest to keep the remaining bits in user space.   
You are fighting an uphill battle, I have to say, since pretty much  
every in-kernel file system does its mount option processing in the  
kernel these days.

So, this is not _my_ idea.  It's the way Linux file systems are  
implemented now.  Thus, we should try to make the mount.nfs config  
file conform to the Linux universe, and not the other way around.

> I would like to stay focus on the technical merit of this patch.
> Meaning are there any holes in that will cause the mount to drop
> core; is there anything I'm missing that will cause mounts to
> unnecessarily fail. Things of that nature...

I am directly addressing the technical merits of your patch: doing it  
your way adds unneeded complexity, breaks aspects of the current  
implementation, goes against the general architecture of mounting a  
Linux file system, will make future work in this area more difficult,  
*and* it will likely prevent us from moving this work into the kernel.

I'm not opposed to specifying defaults in the config file, but please  
don't do it this way.  I have more of a problem with the specific  
implementation you've proposed rather than the idea of setting defaults.

We need to approach this with the idea that version and transport  
negotiation will be handled in the kernel, going forward.  The config  
file is a user interface that will need to be designed with the Linux  
mount(2) API in mind.

>> We have a handful of mount options that adjust mount.nfs's behavior
>> rather than specifying the behavior of the mountpoint: bg, fg, and
>> retry= being the most obvious examples.  Maybe we want a new mount
>> option like defaultvers or defaultproto, which would fall into the  
>> same
>> category.  But the semantics of the existing vers= and proto= options
>> just don't support this kind of thing, and I think we should be
>> extremely careful about abusing them like this.
> More mount options??? No... that is not the answer... IMHO..

I can't see another way of providing a default version and transport  
to the kernel.

Besides, why would you go to the trouble of adding a global, site, and  
server specific section in the config file, and then limit all of  
these to one default version and protocol?

Instead, you could add a different "defaultvers" setting to each  
section, _and_ have one on a specific mount point in /etc/fstab or in  
an automounter map.  Such a default would then be accessible to every  
level of mount processing, and would provide the maximum flexibility  
to users.  What's not to like about that?

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

Re: [PATCH] nfsmount.conf: New variables that explicitly set default

[Steve Dickson]

On 10/12/2009 11:16 AM, Chuck Lever wrote:
> 
> On Oct 12, 2009, at 5:24 AM, Steve Dickson wrote:
> 
>> I realize you are philosophically opposed  to having a configure
>> file and allowing people to define defaults. You have made that
>> clear (at least that's my interpretation) from our previous discussions.
> 
> You are reading way too much into this.  I'm not at all trying to stop
> work on the config file.  If I were, why would I have given you clean
> patches to support vers=4 and version negotiation?  Read my lips: I'm
> over it, I've been over it for a while, and I'm now trying to help you
> get it right.
Great!!! Lets move on...

> 
> I might make a similar assumption about your arguments: you are
> philosophically opposed to doing NFS mount processing in the kernel (and
> you've said as much, recently, on this mailing list).  
Yes... I think its a major mistake when it comes to supporting, changing 
and enhancing to push things like this into the kernel.. Remember I
live in world where its literally impossible to changing a kernel but
its fairly easy to replace a package...
  
> Now you are trying your damndest to keep the remaining bits in user space.
> You are fighting an uphill battle, I have to say, since pretty much every
> in-kernel file system does its mount option processing in the kernel
> these days.
Which of those file systems have a network in between their data and media, 
plus do server negation with multiple daemons over multiple network 
transports? Point being... One size does not fit all when it comes to mounting...
 
> 
> So, this is not _my_ idea.  It's the way Linux file systems are
> implemented now.  Thus, we should try to make the mount.nfs config file
> conform to the Linux universe, and not the other way around.
I'll repeat... one size does not fit all... Plus replacing the 
binary interface with the text based interface brought us in
lock step with the rest of the community... IMHO.. 

> 
>> I would like to stay focus on the technical merit of this patch.
>> Meaning are there any holes in that will cause the mount to drop
>> core; is there anything I'm missing that will cause mounts to
>> unnecessarily fail. Things of that nature...
> 
> I am directly addressing the technical merits of your patch: doing it
> your way adds unneeded complexity, breaks aspects of the current implementation, 
> goes against the general architecture of mounting a Linux file system, 
Please.. I'm doing none of the above... All I'm doing is adding a few lines
to allow a default values to be set... Lets not go to the extreme... ;-)

> will make future work in this area more difficult, *and* it will likely 
> prevent us from moving this work into the kernel.
Again, I think this a stretch... The kernel has and always will have 
the final say in how mounts are done.. When kernels changes the user 
level code will follow suite... That's how its been and that's how it 
will be...
 
> 
> I'm not opposed to specifying defaults in the config file, but please
> don't do it this way.  I have more of a problem with the specific
> implementation you've proposed rather than the idea of setting defaults.
You don't like global variables?? That's the only concept I'm
introducing that does not already exist. Building all those interface
you are talking to avoid using one global variable simply not worth
it.. IMHO..    

> 
> We need to approach this with the idea that version and transport
> negotiation will be handled in the kernel, going forward.  The config
> file is a user interface that will need to be designed with the Linux
> mount(2) API in mind.
Like I said, when kernel changes, so will mount.nfs... 

> 
>>> We have a handful of mount options that adjust mount.nfs's behavior
>>> rather than specifying the behavior of the mountpoint: bg, fg, and
>>> retry= being the most obvious examples.  Maybe we want a new mount
>>> option like defaultvers or defaultproto, which would fall into the same
>>> category.  But the semantics of the existing vers= and proto= options
>>> just don't support this kind of thing, and I think we should be
>>> extremely careful about abusing them like this.
>> More mount options??? No... that is not the answer... IMHO..
> 
> I can't see another way of providing a default version and transport to
> the kernel.
> 
> Besides, why would you go to the trouble of adding a global, site, and
> server specific section in the config file, and then limit all of these
> to one default version and protocol?
No. The ideas is to allow those section to explicitly set versions/transport
(similar to setting them on the command line). I see those sections wanting 
to be more static than dynamic... But only time will tell... 

> 
> Instead, you could add a different "defaultvers" setting to each
> section, _and_ have one on a specific mount point in /etc/fstab or in an
> automounter map.  Such a default would then be accessible to every level
> of mount processing, and would provide the maximum flexibility to
> users.  What's not to like about that?
Cool, when that functionally ready to go, the mount command will take 
advantage of it... 

Lets comprise... I'll change the 'default_vers' and 'default_proto'
to 'defaultvers' and 'defaultproto' in the configuration file. So when
the above functionality is available it will be a seamless transition.. 
So in the end, this will just become a bridge patch... Surely you can
stomach that... 8-) 

steved.

Re: [PATCH] nfsmount.conf: New variables that explicitly set default

[Chuck Lever]

Hi Steve-

Thanks for giving me the opportunity to respond.

On Oct 12, 2009, at 1:10 PM, Steve Dickson wrote:
> On 10/12/2009 11:16 AM, Chuck Lever wrote:
>> So, this is not _my_ idea.  It's the way Linux file systems are
>> implemented now.  Thus, we should try to make the mount.nfs config  
>> file
>> conform to the Linux universe, and not the other way around.
> I'll repeat... one size does not fit all... Plus replacing the
> binary interface with the text based interface brought us in
> lock step with the rest of the community... IMHO..

NFS is not in lock step yet.  The text-based mount option  
implementation is not finished, since there are still significant  
pieces of it done in user space.  The v2/v3 version and transport  
negotiation needs to go into the kernel as well; and now that we have  
v4/v3/v2 negotiation, all that has to go in too, in order to be  
completely consistent.  This (at least the v2/v3 part) has been in  
plan for a long while, and it's why I am watching the config file  
patches carefully.

>>> I would like to stay focus on the technical merit of this patch.
>>> Meaning are there any holes in that will cause the mount to drop
>>> core; is there anything I'm missing that will cause mounts to
>>> unnecessarily fail. Things of that nature...
>>
>> I am directly addressing the technical merits of your patch: doing it
>> your way adds unneeded complexity, breaks aspects of the current  
>> implementation,
>> goes against the general architecture of mounting a Linux file  
>> system,
> Please.. I'm doing none of the above... All I'm doing is adding a  
> few lines
> to allow a default values to be set... Lets not go to the  
> extreme... ;-)

The current patches ignore the principle that I stated (repeatedly)  
last week: you can't diddle the master list of mount options inside  
the fg/bg retry loop.  That's why we need to operate on a fresh copy  
of that list every time through the loop.

See, once config_defaults() removes the vers= and proto= options from  
mi->options, subsequent iterations of the fg/bg retry loop will no  
longer see those options.  So later mount retries won't be starting  
from the same list of options that the user passed in.  Not to mention  
what that potentially does to this mount's entry in /etc/mtab if the  
first mount attempt doesn't succeed.

Thus: the patches break aspects of the current implementation, and  
ignore the architecture of text-based NFS mounts.  The last claim  
above was due to a misunderstanding of your patch set.

>> I'm not opposed to specifying defaults in the config file, but please
>> don't do it this way.  I have more of a problem with the specific
>> implementation you've proposed rather than the idea of setting  
>> defaults.
> You don't like global variables??  That's the only concept I'm
> introducing that does not already exist.

We've never before allowed the user to control how mount.nfs  
negotiates unspecified version or transport settings.  Today, the  
strategy mount.nfs uses for unspecified version/transport/port  
settings always works the same way.  And it feels to me like the  
defaultvers  is actually changing the semantics of an existing mount  
option (vers=) when defaultvers is set.  So, you _are_ introducing  
something entirely new here.

I think that having both defaultvers: and nfsvers: is really  
confusing.  What happens when a user uncomments both of them?  You  
have to shape the user interface so users can set only meaningful  
settings, and the UI naturally excludes settings that are not  
meaningful.  It would be better, just as an example, to combine  
nfsvers and defaultvers into a single setting, since a user would want  
exactly one of these behaviors:  only v2, only v3, only v4, negotiate  
v2/v3, or negotiate v2/v3/v4.

Having a global nfsvers= setting basically means that no NFS mount on  
that client can ever negotiate the version, doesn't it?  Basically,  
you can't disable that global nfsvers= setting on an individual mount  
to allow it to negotiate -- you can only specify a different specific  
NFS version.  Even setting a vers= on the global, site, or server  
specific option lists has this same behavior, doesn't it?  I posit  
therefore that the only version setting we might want in the config  
file is the defaultvers setting (and not nfsvers: ), and it should be  
allowed in each of the three categories, and on individual mount  
points.  If/when the kernel supports a defaultvers= mount option, we  
get that behavior anyway.

What prevents config_defaults() from removing a vers= or proto= option  
specified by he user for that individual mount, when the default  
version bit is set?  That would be undesirable behavior, yes?  It  
doesn't seem to deal with the synonyms of vers= (nfsvers= and v2 and  
v3), either.

The patches ignore the way I designed the switch statement in  
nfs_try_mount().  It's already able to do the correct thing, looking  
at mi->version: if it's 0, it does v4/v3/v2; if it's 2 or 3, it does  
v3/v2.  If there is a vers= option in the option string,  
nfs_validate_options() already sets mi->version properly.  There is no  
need to add any extra logic here whatever for changing the version  
negotiation strategy.  I designed this specifically to support  
switching strategies correctly with minimal fuss, and without making  
additional copies of the options list.  See below to read why I think  
the default version setting has different requirements than the  
default protocol setting.

If you're passing these global default bit settings around, why can't  
you pass the actual default version and protocol settings instead?  If  
you have the default version setting in a global variable, all you  
have to do is check the value of the global variable in  
nfs_validate_option(), and set mi->version as needed.  That takes care  
of defaultvers= in about 4 lines of very straightforward code (after  
you've added support in configfile.c to set the global variables).

----

We also need to nail down exactly what the "default protocol" setting  
does.  I can easily see how a default version setting in the config  
file would be good, but I'm not clear on why a default protocol  
setting is needed.  Do you have any examples of how this setting might  
be useful?

Does this setting specify a protocol name, or a netid (see mre's  
feature request, RH bz 479350)?  If it's a netid, it could also affect  
the way DNS resolution is done for all mounts on that client -- all  
servers would be contacted by IPv4 or all by IPv6.  If it's not a  
netid, there's no way to specify udp6 or tcp6 for the default value of  
proto=.

How will NFSv4 mounts, which do not negotiate the transport protocol  
at all, use this setting?  Today, in fact, NFSv4 on Linux uses TCP  
almost to the exclusion of any other transport.  Should mount.nfs  
append the default setting as a proto= option and be done with it, or  
just ignore it?  What happens if the default setting is "rdma" which  
NFSv4 may not support?

Does the default protocol setting also affect the behavior of  
mountproto=?  Recall that if neither proto= nor mountproto= is  
specified, mount.nfs will choose UDP for the MNT protocol, and TCP for  
the NFS protocol.  If the proto= option is specified, that sets both  
the MNT transport and the NFS transport to the same value.  The user  
can't get that nice default negotiation behavior back once they've set  
the global Proto: setting (just like the nfsvers: setting).

The correct place to handle a default protocol setting, therefore, is  
in the version-specific negotiation logic ie., _under_  
nfs_try_mount(), not as part of nfs_try_mount(), since this setting  
will almost certainly behave differently for NFSv4 and NFSv2/v3.  I  
think default version strategy switching is different enough than  
default protocol strategy switching that we are much better off  
keeping the two entirely separate.  Doing otherwise feels a lot like a  
layering violation to me.

Consider looking at nfs_probe_bothports() as the place to use a  
default protocol setting to its fullest effectiveness.

Some potential pitfalls with this implementation:

ESPIPE is returned in some cases when the server can't be reached....  
this is exactly the case where having config_defaults() remove  
"proto=" from the master option list when a default bit is set can  
result in unexpected results subsequent later iterations of the fg/bg  
loop.

What happens if there's a "udp" or "tcp" option specified by the user  
(ie. not "proto=tcp", but "tcp")?  config_defaults() seems to ignore  
that possibility.

How does the retry logic in config_defaults() behave if the server  
isn't responding?  Since it doesn't look at mount.nfs's retry timeout  
timer between tries, wouldn't this introduce longer mount hangs  
(especially in the NFSv4 case)?  Another reason why this probably  
would be better handled in version-specific logic well below  
nfs_try_mount().

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

Re: [PATCH] nfsmount.conf: New variables that explicitly set default

[Steve Dickson]

On 10/15/2009 11:01 AM, Chuck Lever wrote:
> 
>>>> I would like to stay focus on the technical merit of this patch.
>>>> Meaning are there any holes in that will cause the mount to drop
>>>> core; is there anything I'm missing that will cause mounts to
>>>> unnecessarily fail. Things of that nature...
>>>
>>> I am directly addressing the technical merits of your patch: doing it
>>> your way adds unneeded complexity, breaks aspects of the current
>>> implementation,
>>> goes against the general architecture of mounting a Linux file system,
>> Please.. I'm doing none of the above... All I'm doing is adding a few
>> lines
>> to allow a default values to be set... Lets not go to the extreme... ;-)
> 
> The current patches ignore the principle that I stated (repeatedly) last
> week: you can't diddle the master list of mount options inside the fg/bg
> retry loop.  That's why we need to operate on a fresh copy of that list
> every time through the loop.
> 
> See, once config_defaults() removes the vers= and proto= options from
> mi->options, subsequent iterations of the fg/bg retry loop will no
> longer see those options.  So later mount retries won't be starting from
> the same list of options that the user passed in.  Not to mention what
> that potentially does to this mount's entry in /etc/mtab if the first
> mount attempt doesn't succeed.
> 
> Thus: the patches break aspects of the current implementation, and
> ignore the architecture of text-based NFS mounts.  The last claim above
> was due to a misunderstanding of your patch set.
All this goes away with this release 
    http://marc.info/?l=linux-nfs&m=125578663226042&w=2
Please comment by Tuesday (Oct 20) if so incline to...

> 
>>> I'm not opposed to specifying defaults in the config file, but please
>>> don't do it this way.  I have more of a problem with the specific
>>> implementation you've proposed rather than the idea of setting defaults.
>> You don't like global variables??  That's the only concept I'm
>> introducing that does not already exist.
> 
> We've never before allowed the user to control how mount.nfs negotiates
> unspecified version or transport settings.  Today, the strategy
> mount.nfs uses for unspecified version/transport/port settings always
> works the same way.  And it feels to me like the defaultvers  is
> actually changing the semantics of an existing mount option (vers=) when
> defaultvers is set.  So, you _are_ introducing something entirely new here.
Yes, We are now giving the user more control over how NFS will
negotiate with the server, something on the implementations has
had for years...
  
> 
> I think that having both defaultvers: and nfsvers: is really confusing. 
> What happens when a user uncomments both of them?
nfsvers has precedence... 


> It would be better, just as an example, to combine nfsvers and defaultvers 
> into a single setting, since a user would want exactly one of these behaviors: 
> only v2, only v3, only v4, negotiate v2/v3, or negotiate v2/v3/v4.
No. Setting nfsvers on either the command line or in the config file
means the same thing... No negotiation. When the server does not 
support the given version, the mount will fail. With defaultvers 
there will be a negotiation.

> 
> Having a global nfsvers= setting basically means that no NFS mount on
> that client can ever negotiate the version, doesn't it?  
Exactly, that's why defaultvers is needed...

> Basically, you can't disable that global nfsvers= setting on an individual mount to
> allow it to negotiate -- you can only specify a different specific NFS
> version.  Even setting a vers= on the global, site, or server specific
> option lists has this same behavior, doesn't it?  I posit therefore that
> the only version setting we might want in the config file is the
> defaultvers setting (and not nfsvers: ), and it should be allowed in
> each of the three categories, and on individual mount points.  If/when
> the kernel supports a defaultvers= mount option, we get that behavior
> anyway.
You can disable anything by simply commenting out... but I think you 
are looking as this a bit backwards. The config file is for people 
to *enable* certain option different servers and mount points. So
true the concept of "disabling a global option" does not exist, but 
the ability to change a global option will know exist.
 
> 
> What prevents config_defaults() from removing a vers= or proto= option
> specified by he user for that individual mount, when the default version
> bit is set?  That would be undesirable behavior, yes?  It doesn't seem
> to deal with the synonyms of vers= (nfsvers= and v2 and v3), either.
See http://marc.info/?l=linux-nfs&m=125578663226042&w=2

> 
> The patches ignore the way I designed the switch statement in
> nfs_try_mount().  It's already able to do the correct thing, looking at
> mi->version: if it's 0, it does v4/v3/v2; if it's 2 or 3, it does
> v3/v2.  If there is a vers= option in the option string,
> nfs_validate_options() already sets mi->version properly.  There is no
> need to add any extra logic here whatever for changing the version
> negotiation strategy.  I designed this specifically to support switching
> strategies correctly with minimal fuss, and without making additional
> copies of the options list.  See below to read why I think the default
> version setting has different requirements than the default protocol
> setting.
See http://marc.info/?l=linux-nfs&m=125578663226042&w=2

> 
> If you're passing these global default bit settings around, why can't
> you pass the actual default version and protocol settings instead?  If
> you have the default version setting in a global variable, all you have
> to do is check the value of the global variable in
> nfs_validate_option(), and set mi->version as needed.  That takes care
> of defaultvers= in about 4 lines of very straightforward code (after
> you've added support in configfile.c to set the global variables).
See http://marc.info/?l=linux-nfs&m=125578663226042&w=2

> 
> ----
> 
> We also need to nail down exactly what the "default protocol" setting
> does.  I can easily see how a default version setting in the config file
> would be good, but I'm not clear on why a default protocol setting is
> needed.  Do you have any examples of how this setting might be useful?
Means what it always has meant... which protocol to try first... 

> 
> Does this setting specify a protocol name, or a netid (see mre's feature
> request, RH bz 479350)?  If it's a netid, it could also affect the way
> DNS resolution is done for all mounts on that client -- all servers
> would be contacted by IPv4 or all by IPv6.  If it's not a netid, there's
> no way to specify udp6 or tcp6 for the default value of proto=.
The setting of a protocol will be changing... and when those changes
happen, they will also change in the config file...

> 
> How will NFSv4 mounts, which do not negotiate the transport protocol at
> all, use this setting?  Today, in fact, NFSv4 on Linux uses TCP almost
> to the exclusion of any other transport.  Should mount.nfs append the
> default setting as a proto= option and be done with it, or just ignore
> it?  
The protocol setting is not looked at on v4 mounts, so there is 
absolutely no effect whatsoever...
 
> What happens if the default setting is "rdma" which NFSv4 may not
> support?
We will cross that bridge when we get there. If this is the only
problem people have that are doing rdma mount, that will be a good thing.
 
> 
> Does the default protocol setting also affect the behavior of
> mountproto=?  
Nope. 

> Recall that if neither proto= nor mountproto= is specified, mount.nfs will 
> choose UDP for the MNT protocol, and TCP for the NFS protocol.  
> If the proto= option is specified, that sets both the MNT transport and 
> the NFS transport to the same value.  The user can't
> get that nice default negotiation behavior back once they've set the
> global Proto: setting (just like the nfsvers: setting).
People will *always* have the option of *not* using the config file
it does not meet their needs...

> 
> The correct place to handle a default protocol setting, therefore, is in
> the version-specific negotiation logic ie., _under_ nfs_try_mount(), not
> as part of nfs_try_mount(), since this setting will almost certainly
> behave differently for NFSv4 and NFSv2/v3.  I think default version
> strategy switching is different enough than default protocol strategy
> switching that we are much better off keeping the two entirely
> separate.  Doing otherwise feels a lot like a layering violation to me.
See http://marc.info/?l=linux-nfs&m=125578663226042&w=2

> 
> Consider looking at nfs_probe_bothports() as the place to use a default
> protocol setting to its fullest effectiveness.
> 
> Some potential pitfalls with this implementation:
> 
> ESPIPE is returned in some cases when the server can't be reached....
> this is exactly the case where having config_defaults() remove "proto="
> from the master option list when a default bit is set can result in
> unexpected results subsequent later iterations of the fg/bg loop.
See http://marc.info/?l=linux-nfs&m=125578663226042&w=2

> 
> What happens if there's a "udp" or "tcp" option specified by the user
> (ie. not "proto=tcp", but "tcp")?  config_defaults() seems to ignore
> that possibility.
See http://marc.info/?l=linux-nfs&m=125578663226042&w=2

> 
> How does the retry logic in config_defaults() behave if the server isn't
> responding?  Since it doesn't look at mount.nfs's retry timeout timer
> between tries, wouldn't this introduce longer mount hangs (especially in
> the NFSv4 case)?  Another reason why this probably would be better
> handled in version-specific logic well below nfs_try_mount().
See http://marc.info/?l=linux-nfs&m=125578663226042&w=2

steved.

Re: [PATCH] nfsmount.conf: New variables that explicitly set default

[Chuck Lever]

On Oct 19, 2009, at 9:38 PM, Steve Dickson wrote:
> On 10/15/2009 11:01 AM, Chuck Lever wrote:
>> I think that having both defaultvers: and nfsvers: is really  
>> confusing.
>> What happens when a user uncomments both of them?
> nfsvers has precedence...
>
>
>> It would be better, just as an example, to combine nfsvers and  
>> defaultvers
>> into a single setting, since a user would want exactly one of these  
>> behaviors:
>> only v2, only v3, only v4, negotiate v2/v3, or negotiate v2/v3/v4.
> No. Setting nfsvers on either the command line or in the config file
> means the same thing... No negotiation. When the server does not
> support the given version, the mount will fail. With defaultvers
> there will be a negotiation.
>
>>
>> Having a global nfsvers= setting basically means that no NFS mount on
>> that client can ever negotiate the version, doesn't it?
> Exactly, that's why defaultvers is needed...
>
>> Basically, you can't disable that global nfsvers= setting on an  
>> individual mount to
>> allow it to negotiate -- you can only specify a different specific  
>> NFS
>> version.  Even setting a vers= on the global, site, or server  
>> specific
>> option lists has this same behavior, doesn't it?  I posit therefore  
>> that
>> the only version setting we might want in the config file is the
>> defaultvers setting (and not nfsvers: ), and it should be allowed in
>> each of the three categories, and on individual mount points.  If/ 
>> when
>> the kernel supports a defaultvers= mount option, we get that behavior
>> anyway.
> You can disable anything by simply commenting out... but I think you
> are looking as this a bit backwards. The config file is for people
> to *enable* certain option different servers and mount points. So
> true the concept of "disabling a global option" does not exist, but
> the ability to change a global option will know exist.

Let's consider specifically for a moment admins who want to make use  
of the config file facility, not folks who want to ignore it.

As I understand the proposed implementation, if an admin sets  
"defaultvers" in the config file, she alters the mount command's  
version negotiation strategy.  That strategy can be overridden only  
somewhat for specific mount points by setting a specific version.   
However, she can't allow some mounts to negotiate v4/v3/v2, and others  
negotiate v3/v2 only.  The latter would be useful, I think.

If she sets "nfsvers" in the config file, she mandates a particular  
version for _all_ mounts on that client, and while specific mounts can  
specify a different version, there's no way to allow specific mount  
points to negotiate the version _at_ _all_, when this option is set.   
I think that's needlessly inflexible.

IMO admins who use the config file would find it more useful if they  
could allow a new negotiation strategy or a specific NFS version to be  
set _anywhere_ in the config file.  For example: "all mounts on this  
server should negotiate v2/v3; all mounts on this other newer server  
can negotiate v2/v3/v4; all mounts on this third server can negotiate  
v2/v3/v4, except these two, which are for a database, and they need v3  
always; for this fourth server, I want to negotiate v2/v3 except for  
the v4 test mount, which can negotiate all three versions".

Or, simpler: "I want v2/v3 negotiation everywhere, but this test  
server here should be able to negotiate v2/v3/v4."

A better solution, as I've suggested before, is to have a defaultvers=  
mount option instead.  This provides the flexibility to set a default  
version at _any_ level in the config file, and it even allows previous  
default settings to be overridden on specific mount points.  In  
addition, folks who choose not to use the config file at all can also  
get the ability to specify the default behavior on each mount.  That  
seems like a win-win for everyone.

In order to get the ability to override config file settings, though,  
you would need to use the "rightmost wins" rule for defaultvers= -- it  
should be able to override NFS version settings earlier in the option  
list.  That should be simple to implement by having nfs_nfs_version  
leave *version set to zero if the defaultvers= option is rightmost,  
and then have the new logic you added in nfs_validate_options()  
additionally strip off defaultvers= before going ahead with the mount  
attempts.

Does that make sense?

I can shut up about mount options if you have a stronger argument than  
"aw, not another mount option..." :-)  Otherwise, please convince me  
that a single global default setting is better than allowing  
negotiation to be enabled and disabled anywhere.

>> ----
>>
>> We also need to nail down exactly what the "default protocol" setting
>> does.  I can easily see how a default version setting in the config  
>> file
>> would be good, but I'm not clear on why a default protocol setting is
>> needed.  Do you have any examples of how this setting might be  
>> useful?
> Means what it always has meant... which protocol to try first...

The default protocol setting is new, so "what it always has meant" is  
meaningless.  Unless you mean "I'm adding absolutely no new  
functionality here," in which case, why are we bothering to add  
defaultproto: ?

But I think you miss my point here.  Do you have any specific real  
world examples that demonstrate a problem that can be solved using  
defaultproto= that we haven't been able to solve before now?   
Providing such examples helps us understand how this feature should  
behave and be implemented.  Basically, it sharpens our implementation  
to have a target.

It would help our discussion immensely to see how you intend this to  
be used.  I still can't see what benefit it would have over simply  
specifying proto= where needed.  The benefit of being able to specify  
a default version is clear.  But unlike defaultvers: , the most common  
case, I think, would be simply to disable protocol negotiation, as you  
can already do with proto=.  Otherwise, less hand waving please and  
more concrete examples.

>> Does this setting specify a protocol name, or a netid (see mre's  
>> feature
>> request, RH bz 479350)?  If it's a netid, it could also affect the  
>> way
>> DNS resolution is done for all mounts on that client -- all servers
>> would be contacted by IPv4 or all by IPv6.  If it's not a netid,  
>> there's
>> no way to specify udp6 or tcp6 for the default value of proto=.
> The setting of a protocol will be changing... and when those changes
> happen, they will also change in the config file...

The value of defaultvers: is obviously a number, but the value of  
defaultproto: is clearly a string.  We know for certain that we will  
need that value to be a string and not a number in the lower parts of  
the mount code in the near future, so making this a string now would  
be simple to do, and help us out in the long run.

>> How will NFSv4 mounts, which do not negotiate the transport  
>> protocol at
>> all, use this setting?  Today, in fact, NFSv4 on Linux uses TCP  
>> almost
>> to the exclusion of any other transport.  Should mount.nfs append the
>> default setting as a proto= option and be done with it, or just  
>> ignore
>> it?
> The protocol setting is not looked at on v4 mounts, so there is
> absolutely no effect whatsoever...

I'm not sure what you mean by that, especially given the previous  
implementation of this patch set which seemed to remove a proto=  
option in logic _above_ the version-specific code in nfs_try_mount().

In addition, proto= does have a meaning for v4, and it has a default  
value: tcp.  It is not negotiated, as it is with v2/v3, true enough.   
But defaultproto: can have a reasonable meaning for v4: when proto= is  
not specified on v4 mounts, always use the specified protocol  
setting.  (I'm not necessarily arguing in favor of or against this  
particular behavior, but I am trying to get my head around how you  
want this thing to behave).

>> What happens if the default setting is "rdma" which NFSv4 may not
>> support?
> We will cross that bridge when we get there. If this is the only
> problem people have that are doing rdma mount, that will be a good  
> thing.

Again, saving a string and not a number right now makes this a lot  
easier down the road.... it would allow us to support rdma here  
immediately, and this would not need to be changed later to get it to  
work.  There's no reason not to do it right the first time.

>> Does the default protocol setting also affect the behavior of
>> mountproto=?
> Nope.

That's different than the meaning of "proto=" then, as I have  
described below.  So, why do we need defaultproto: but not  
defaultmountproto: ?  Having an example or two of real world usage  
would help me understand the distinction.

>> Recall that if neither proto= nor mountproto= is specified,  
>> mount.nfs will
>> choose UDP for the MNT protocol, and TCP for the NFS protocol.
>> If the proto= option is specified, that sets both the MNT transport  
>> and
>> the NFS transport to the same value.  The user can't
>> get that nice default negotiation behavior back once they've set the
>> global Proto: setting (just like the nfsvers: setting).
> People will *always* have the option of *not* using the config file
> it does not meet their needs...

Understood; I'm not arguing against using the config file at all.

But why would an admin who does choose to use the config file expect  
or want different semantics for proto= and defaultproto: ?

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

Re: [PATCH] nfsmount.conf: New variables that explicitly set default

[Steve Dickson]

On 10/21/2009 01:37 PM, Chuck Lever wrote:
> On Oct 19, 2009, at 9:38 PM, Steve Dickson wrote:
>> You can disable anything by simply commenting out... but I think you
>> are looking as this a bit backwards. The config file is for people
>> to *enable* certain option different servers and mount points. So
>> true the concept of "disabling a global option" does not exist, but
>> the ability to change a global option will know exist.
> 
> Let's consider specifically for a moment admins who want to make use of
> the config file facility, not folks who want to ignore it.
> 
> As I understand the proposed implementation, if an admin sets
> "defaultvers" in the config file, she alters the mount command's version
> negotiation strategy.  That strategy can be overridden only somewhat for
> specific mount points by setting a specific version.  However, she can't
> allow some mounts to negotiate v4/v3/v2, and others negotiate v3/v2
> only.  The latter would be useful, I think.
No, that functionality does not exist. 

> 
> If she sets "nfsvers" in the config file, she mandates a particular
> version for _all_ mounts on that client, and while specific mounts can
> specify a different version, there's no way to allow specific mount
> points to negotiate the version _at_ _all_, when this option is set.  I
> think that's needlessly inflexible.
Yes, If "nfsvers" is set in the global section, that would turn
off all negotiations, But, if "nfsvers" set in the mount point section 
or server section (which is the expected use), then only those mounts point
defined in the mnt point section or mount to that particular server would 
be effected (i.e. negotiations turned off). All other mounts would not be effect,
meaning version negotiation would happen as expected.

> 
> IMO admins who use the config file would find it more useful if they
> could allow a new negotiation strategy or a specific NFS version to be
> set _anywhere_ in the config file.  For example: "all mounts on this
> server should negotiate v2/v3; all mounts on this other newer server can
> negotiate v2/v3/v4; all mounts on this third server can negotiate
> v2/v3/v4, except these two, which are for a database, and they need v3
> always; for this fourth server, I want to negotiate v2/v3 except for the
> v4 test mount, which can negotiate all three versions".
> 
> Or, simpler: "I want v2/v3 negotiation everywhere, but this test server
> here should be able to negotiate v2/v3/v4."
Or lets let the people that used this config file tell us what they want... 

I would rather not speculate on whether an additional  feature like this 
would be useful. I would rather just give them the config file (as is) 
and the them tell us what is or is not needed...

> 
> A better solution, as I've suggested before, is to have a defaultvers=
> mount option instead.  This provides the flexibility to set a default
> version at _any_ level in the config file, and it even allows previous
> default settings to be overridden on specific mount points.  In
> addition, folks who choose not to use the config file at all can also
> get the ability to specify the default behavior on each mount.  That
> seems like a win-win for everyone.
When the kernel supports a defaultvers mount option, the appropriate changes
will be made...

> 
> In order to get the ability to override config file settings, though,
> you would need to use the "rightmost wins" rule for defaultvers= -- it
> should be able to override NFS version settings earlier in the option
> list.  
I'm not sure what you mean by "it" but the default version can be
overwritten by version being set earlier... 

> That should be simple to implement by having nfs_nfs_version
> leave *version set to zero if the defaultvers= option is rightmost, and
> then have the new logic you added in nfs_validate_options() additionally
> strip off defaultvers= before going ahead with the mount attempts.
> 
> Does that make sense?
Yes. but you are assuming defaultvers is a valid mount option and it is
not. So when/if it does, we can have this discussion then... Plus
what we have today works just fine and if we want to do tweaks like this 
later on, so be it... 

> 
> I can shut up about mount options if you have a stronger argument than
> "aw, not another mount option..." :-)  Otherwise, please convince me
> that a single global default setting is better than allowing negotiation
> to be enabled and disabled anywhere.
As I said, setting the version in the global section of the config file 
probably will not be done. Setting the version/protocol in the mount
point and/or Server section will be done. Mostly to either maintain 
legacy servers or to try new technology like V4, V4.1 and pNFS.

Having the mount point and server section do negotiations, yeah,
that may or may not be a useful thing, only time will tell... 
But I don't think this type of speculation should not stand in 
the way of progress...
 
> 
>>> ----
>>>
>>> We also need to nail down exactly what the "default protocol" setting
>>> does.  I can easily see how a default version setting in the config file
>>> would be good, but I'm not clear on why a default protocol setting is
>>> needed.  Do you have any examples of how this setting might be useful?
>> Means what it always has meant... which protocol to try first...
> 
> The default protocol setting is new, so "what it always has meant" is
> meaningless.  Unless you mean "I'm adding absolutely no new
> functionality here," in which case, why are we bothering to add
> defaultproto: ?
The only new part about is it being exposed... As I'm sure you remember
when the default transport moved from udp to tcp there were a number
of issues. So this does is gives people a way to start with UDP first 
and then try TCP. Will this be norm, no. Will it be recommend, no. 
But will this be useful (and wanted) feature in a very small percentage 
of the world, probably... 

> 
> But I think you miss my point here.  Do you have any specific real world
> examples that demonstrate a problem that can be solved using
> defaultproto= that we haven't been able to solve before now?  Providing
> such examples helps us understand how this feature should behave and be
> implemented.  Basically, it sharpens our implementation to have a target.
No. But I do know people still use UDP instead of TCP so in those 
instances this would a welcome feature... 

> 
> It would help our discussion immensely to see how you intend this to be
> used.  I still can't see what benefit it would have over simply
> specifying proto= where needed.  The benefit of being able to specify a
> default version is clear.  But unlike defaultvers: , the most common
> case, I think, would be simply to disable protocol negotiation, as you
> can already do with proto=.  Otherwise, less hand waving please and more
> concrete examples.
> 
>>> Does this setting specify a protocol name, or a netid (see mre's feature
>>> request, RH bz 479350)?  If it's a netid, it could also affect the way
>>> DNS resolution is done for all mounts on that client -- all servers
>>> would be contacted by IPv4 or all by IPv6.  If it's not a netid, there's
>>> no way to specify udp6 or tcp6 for the default value of proto=.
>> The setting of a protocol will be changing... and when those changes
>> happen, they will also change in the config file...
> 
> The value of defaultvers: is obviously a number, but the value of
> defaultproto: is clearly a string.  We know for certain that we will
> need that value to be a string and not a number in the lower parts of
> the mount code in the near future, so making this a string now would be
> simple to do, and help us out in the long run.
I would rather the config file stay compatible with the command line.
Meaning when the command line option changes, the config file will...

> 
>>> How will NFSv4 mounts, which do not negotiate the transport protocol at
>>> all, use this setting?  Today, in fact, NFSv4 on Linux uses TCP almost
>>> to the exclusion of any other transport.  Should mount.nfs append the
>>> default setting as a proto= option and be done with it, or just ignore
>>> it?
>> The protocol setting is not looked at on v4 mounts, so there is
>> absolutely no effect whatsoever...
> 
> I'm not sure what you mean by that, especially given the previous
> implementation of this patch set which seemed to remove a proto= option
> in logic _above_ the version-specific code in nfs_try_mount().
Keywords being "previous implementation", that is not loner done.
The point is, there is no network protocol negation that goes on
with v4 mounts, so defaultproto does not come into play.

> 
> In addition, proto= does have a meaning for v4, and it has a default
> value: tcp.  It is not negotiated, as it is with v2/v3, true enough. 
> But defaultproto: can have a reasonable meaning for v4: when proto= is
> not specified on v4 mounts, always use the specified protocol setting. 
> (I'm not necessarily arguing in favor of or against this particular
> behavior, but I am trying to get my head around how you want this thing
> to behave).
> 
>>> What happens if the default setting is "rdma" which NFSv4 may not
>>> support?
>> We will cross that bridge when we get there. If this is the only
>> problem people have that are doing rdma mount, that will be a good thing.
> 
> Again, saving a string and not a number right now makes this a lot
> easier down the road.... it would allow us to support rdma here
> immediately, and this would not need to be changed later to get it to
> work.  There's no reason not to do it right the first time.
Again, when the command line changes the configuration file will change.

> 
>>> Does the default protocol setting also affect the behavior of
>>> mountproto=?
>> Nope.
> 
> That's different than the meaning of "proto=" then, as I have described
> below.  So, why do we need defaultproto: but not defaultmountproto: ? 
> Having an example or two of real world usage would help me understand
> the distinction.
If a defaultmountproto is needed down the road, we will put it in...
At this point I don't think there is a needed for it...

> 
>>> Recall that if neither proto= nor mountproto= is specified, mount.nfs
>>> will
>>> choose UDP for the MNT protocol, and TCP for the NFS protocol.
>>> If the proto= option is specified, that sets both the MNT transport and
>>> the NFS transport to the same value.  The user can't
>>> get that nice default negotiation behavior back once they've set the
>>> global Proto: setting (just like the nfsvers: setting).
>> People will *always* have the option of *not* using the config file
>> it does not meet their needs...
> 
> Understood; I'm not arguing against using the config file at all.
> 
> But why would an admin who does choose to use the config file expect or
> want different semantics for proto= and defaultproto: ?
Because they want to switch the order in which the network protocols are tried.
 
Ok... since it appears there are no strictly technical issues (just philosophical 
ones), I think I'm going to move on and commit the patch which will 
give us some building block on which to build from...

Thanks for you input, its greatly appreciated! 

steved.

Re: [PATCH] nfsmount.conf: New variables that explicitly set default

[J. Bruce Fields]

On Wed, Oct 21, 2009 at 04:31:30PM -0400, Steve Dickson wrote:
> On 10/21/2009 01:37 PM, Chuck Lever wrote:
> > That should be simple to implement by having nfs_nfs_version
> > leave *version set to zero if the defaultvers= option is rightmost, and
> > then have the new logic you added in nfs_validate_options() additionally
> > strip off defaultvers= before going ahead with the mount attempts.
> > 
> > Does that make sense?
> Yes. but you are assuming defaultvers is a valid mount option and it is
> not. So when/if it does, we can have this discussion then... Plus
> what we have today works just fine and if we want to do tweaks like this 
> later on, so be it... 

It may be hard to change the meaning of "defaultvers" (or whatever) once
it's in use.

--b.

Re: [PATCH] nfsmount.conf: New variables that explicitly set default

[Chuck Lever]

On Oct 23, 2009, at 10:20 AM, J. Bruce Fields wrote:

> On Wed, Oct 21, 2009 at 04:31:30PM -0400, Steve Dickson wrote:
>> On 10/21/2009 01:37 PM, Chuck Lever wrote:
>>> That should be simple to implement by having nfs_nfs_version
>>> leave *version set to zero if the defaultvers= option is  
>>> rightmost, and
>>> then have the new logic you added in nfs_validate_options()  
>>> additionally
>>> strip off defaultvers= before going ahead with the mount attempts.
>>>
>>> Does that make sense?
>> Yes. but you are assuming defaultvers is a valid mount option and  
>> it is
>> not. So when/if it does, we can have this discussion then... Plus
>> what we have today works just fine and if we want to do tweaks like  
>> this
>> later on, so be it...
>
> It may be hard to change the meaning of "defaultvers" (or whatever)  
> once
> it's in use.

I agree.  I can have a mount.nfs implementation of defaultvers= for  
you very quickly, FYI.

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