Re: [PATCH 1/2] nfsd: Allow the caller to turn off NFSv4.0 without turning off NFSv4.x

[Steve Dickson <SteveD@RedHat.com>]

Hey Trond,

My apologies for taking so long to address this... 

On 02/23/2017 07:33 PM, Trond Myklebust wrote:
> The new semantic is that '-N4' turns off all NFSv4 minor versions, while
> '-V4' turns them all on. In order to turn off just minor version x (x >= 0),
> use -N4.x, and to turn it back on. '-V4.x'.
> 
> Note that on older kernels, attempting to use -N4.0 and -V4.0 is
> equivalent to specifying -N4 or -V4.
doing a 

nfsd -d -N4.0 -V4.1 -V4.2 
nfsd: Writing version string to kernel: -2 +3 -4 +4.1 +4.2

does the right thing but when I do a

nfsd -d -N4.0 
nfsd: Writing version string to kernel: -2 +3 -4

It brings down all of the v4 minor versions, Is that
intentional? It seems to me doing a -N4.0 should only
stop 4.0 from coming up not v4.1 or v4.2

steved.
 
> 
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
>  support/include/nfs/nfs.h |  7 +++--
>  utils/nfsd/nfsd.c         | 39 +++++++++++++++---------
>  utils/nfsd/nfsd.man       |  4 +--
>  utils/nfsd/nfssvc.c       | 76 ++++++++++++++++++++++++++++++++++++-----------
>  utils/nfsd/nfssvc.h       |  1 +
>  5 files changed, 92 insertions(+), 35 deletions(-)
> 
> diff --git a/support/include/nfs/nfs.h b/support/include/nfs/nfs.h
> index 15ecc6bfc485..5860343f78b7 100644
> --- a/support/include/nfs/nfs.h
> +++ b/support/include/nfs/nfs.h
> @@ -16,8 +16,8 @@
>  #define NFSD_MINVERS 2
>  #define NFSD_MAXVERS 4
>  
> -#define NFS4_MINMINOR 1
> -#define NFS4_MAXMINOR WORD_BIT
> +#define NFS4_MINMINOR 0
> +#define NFS4_MAXMINOR (WORD_BIT-1)
>  
>  struct nfs_fh_len {
>  	int		fh_size;
> @@ -29,15 +29,18 @@ struct nfs_fh_len {
>  #define NFSCTL_TCPBIT		      (1 << (18 - 1))
>  
>  #define NFSCTL_VERUNSET(_cltbits, _v) ((_cltbits) &= ~(1 << ((_v) - 1))) 
> +#define NFSCTL_MINORUNSET(_cltbits, _v) ((_cltbits) &= ~(1 << (_v)))
>  #define NFSCTL_UDPUNSET(_cltbits)     ((_cltbits) &= ~NFSCTL_UDPBIT) 
>  #define NFSCTL_TCPUNSET(_cltbits)     ((_cltbits) &= ~NFSCTL_TCPBIT) 
>  
>  #define NFSCTL_VERISSET(_cltbits, _v) ((_cltbits) & (1 << ((_v) - 1))) 
> +#define NFSCTL_MINORISSET(_cltbits, _v) ((_cltbits) & (1 << (_v)))
>  #define NFSCTL_UDPISSET(_cltbits)     ((_cltbits) & NFSCTL_UDPBIT) 
>  #define NFSCTL_TCPISSET(_cltbits)     ((_cltbits) & NFSCTL_TCPBIT) 
>  
>  #define NFSCTL_VERDEFAULT (0xc)       /* versions 3 and 4 */
>  #define NFSCTL_VERSET(_cltbits, _v)   ((_cltbits) |= (1 << ((_v) - 1))) 
> +#define NFSCTL_MINORSET(_cltbits, _v)   ((_cltbits) |= (1 << (_v)))
>  #define NFSCTL_UDPSET(_cltbits)       ((_cltbits) |= NFSCTL_UDPBIT)
>  #define NFSCTL_TCPSET(_cltbits)       ((_cltbits) |= NFSCTL_TCPBIT)
>  
> diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
> index 20f4b7952203..1708521ab286 100644
> --- a/utils/nfsd/nfsd.c
> +++ b/utils/nfsd/nfsd.c
> @@ -67,6 +67,7 @@ main(int argc, char **argv)
>  	int	socket_up = 0;
>  	unsigned int minorvers = 0;
>  	unsigned int minorversset = 0;
> +	unsigned int minormask = 0;
>  	unsigned int versbits = NFSCTL_VERDEFAULT;
>  	unsigned int protobits = NFSCTL_ALLBITS;
>  	int grace = -1;
> @@ -104,10 +105,16 @@ main(int argc, char **argv)
>  		else
>  			NFSCTL_VERUNSET(versbits, i);
>  	}
> +
> +	nfssvc_get_minormask(&minormask);
>  	/* We assume the kernel will default all minor versions to 'on',
>  	 * and allow the config file to disable some.
>  	 */
> -	for (i = NFS4_MINMINOR; i <= NFS4_MAXMINOR; i++) {
> +	if (NFSCTL_VERISSET(versbits, 4)) {
> +		NFSCTL_MINORSET(minorversset, 0);
> +		NFSCTL_MINORSET(minorvers, 0);
> +	}
> +	for (i = 1; i <= NFS4_MAXMINOR; i++) {
>  		char tag[20];
>  		sprintf(tag, "vers4.%d", i);
>  		/* The default for minor version support is to let the
> @@ -119,12 +126,12 @@ main(int argc, char **argv)
>  		 * (i.e. don't set the bit in minorversset).
>  		 */
>  		if (!conf_get_bool("nfsd", tag, 1)) {
> -			NFSCTL_VERSET(minorversset, i);
> -			NFSCTL_VERUNSET(minorvers, i);
> +			NFSCTL_MINORSET(minorversset, i);
> +			NFSCTL_MINORUNSET(minorvers, i);
>  		}
>  		if (conf_get_bool("nfsd", tag, 0)) {
> -			NFSCTL_VERSET(minorversset, i);
> -			NFSCTL_VERSET(minorvers, i);
> +			NFSCTL_MINORSET(minorversset, i);
> +			NFSCTL_MINORSET(minorvers, i);
>  		}
>  	}
>  
> @@ -179,13 +186,17 @@ main(int argc, char **argv)
>  			case 4:
>  				if (*p == '.') {
>  					int i = atoi(p+1);
> -					if (i < NFS4_MINMINOR || i > NFS4_MAXMINOR) {
> +					if (i < 0 || i > NFS4_MAXMINOR) {
>  						fprintf(stderr, "%s: unsupported minor version\n", optarg);
>  						exit(1);
>  					}
> -					NFSCTL_VERSET(minorversset, i);
> -					NFSCTL_VERUNSET(minorvers, i);
> -					break;
> +					NFSCTL_MINORSET(minorversset, i);
> +					NFSCTL_MINORUNSET(minorvers, i);
> +					if (minorvers != 0)
> +						break;
> +				} else {
> +					minorvers = 0;
> +					minorversset = minormask;
>  				}
>  			case 3:
>  			case 2:
> @@ -201,14 +212,14 @@ main(int argc, char **argv)
>  			case 4:
>  				if (*p == '.') {
>  					int i = atoi(p+1);
> -					if (i < NFS4_MINMINOR || i > NFS4_MAXMINOR) {
> +					if (i < 0 || i > NFS4_MAXMINOR) {
>  						fprintf(stderr, "%s: unsupported minor version\n", optarg);
>  						exit(1);
>  					}
> -					NFSCTL_VERSET(minorversset, i);
> -					NFSCTL_VERSET(minorvers, i);
> -					break;
> -				}
> +					NFSCTL_MINORSET(minorversset, i);
> +					NFSCTL_MINORSET(minorvers, i);
> +				} else
> +					minorvers = minorversset = minormask;
>  			case 3:
>  			case 2:
>  				NFSCTL_VERSET(versbits, c);
> diff --git a/utils/nfsd/nfsd.man b/utils/nfsd/nfsd.man
> index 8901fb6c6872..0d797fd2ec8d 100644
> --- a/utils/nfsd/nfsd.man
> +++ b/utils/nfsd/nfsd.man
> @@ -57,7 +57,7 @@ This option can be used to request that
>  .B rpc.nfsd
>  does not offer certain versions of NFS. The current version of
>  .B rpc.nfsd
> -can support major NFS versions 2,3,4 and the minor versions 4.1 and 4.2.
> +can support major NFS versions 2,3,4 and the minor versions 4.0, 4.1 and 4.2.
>  .TP
>  .B \-s " or " \-\-syslog
>  By default,
> @@ -82,7 +82,7 @@ This option can be used to request that
>  .B rpc.nfsd
>  offer certain versions of NFS. The current version of
>  .B rpc.nfsd
> -can support major NFS versions 2,3,4 and the minor versions 4.1 and 4.2.
> +can support major NFS versions 2,3,4 and the minor versions 4.0, 4.1 and 4.2.
>  .TP
>  .B \-L " or " \-\-lease-time seconds
>  Set the lease-time used for NFSv4.  This corresponds to how often
> diff --git a/utils/nfsd/nfssvc.c b/utils/nfsd/nfssvc.c
> index 07f6ff1204d1..e8609c15b8e5 100644
> --- a/utils/nfsd/nfssvc.c
> +++ b/utils/nfsd/nfssvc.c
> @@ -330,36 +330,78 @@ nfssvc_set_time(const char *type, const int seconds)
>  }
>  
>  void
> +nfssvc_get_minormask(unsigned int *mask)
> +{
> +	int fd;
> +	char *ptr = buf;
> +	ssize_t size;
> +
> +	fd = open(NFSD_VERS_FILE, O_RDONLY);
> +	if (fd < 0)
> +		return;
> +
> +	size = read(fd, buf, sizeof(buf));
> +	if (size < 0) {
> +		xlog(L_ERROR, "Getting versions failed: errno %d (%m)", errno);
> +		goto out;
> +	}
> +	ptr[size] = '\0';
> +	for (;;) {
> +		unsigned vers, minor = 0;
> +		char *token = strtok(ptr, " ");
> +
> +		if (!token)
> +			break;
> +		ptr = NULL;
> +		if (*token != '+' && *token != '-')
> +			continue;
> +		if (sscanf(++token, "%u.%u", &vers, &minor) > 0 &&
> +		    vers == 4 && minor <= NFS4_MAXMINOR)
> +			NFSCTL_MINORSET(*mask, minor);
> +	}
> +out:
> +	close(fd);
> +	return;
> +}
> +
> +static int
> +nfssvc_print_vers(char *ptr, unsigned size, unsigned vers, unsigned minorvers,
> +		int isset)
> +{
> +	char sign = isset ? '+' : '-';
> +	if (minorvers == 0)
> +		return snprintf(ptr, size, "%c%u ", sign, vers);
> +	return snprintf(ptr, size, "%c%u.%u ", sign, vers, minorvers);
> +}
> +
> +void
>  nfssvc_setvers(unsigned int ctlbits, unsigned int minorvers, unsigned int minorversset)
>  {
>  	int fd, n, off;
> -	char *ptr;
>  
> -	ptr = buf;
>  	off = 0;
>  	fd = open(NFSD_VERS_FILE, O_WRONLY);
>  	if (fd < 0)
>  		return;
>  
> -	for (n = NFS4_MINMINOR; n <= NFS4_MAXMINOR; n++) {
> -		if (NFSCTL_VERISSET(minorversset, n)) {
> -			if (NFSCTL_VERISSET(minorvers, n))
> -				off += snprintf(ptr+off, sizeof(buf) - off, "+4.%d ", n);
> -			else
> -				off += snprintf(ptr+off, sizeof(buf) - off, "-4.%d ", n);
> -		}
> -	}
> -	for (n = NFSD_MINVERS; n <= NFSD_MAXVERS; n++) {
> -		if (NFSCTL_VERISSET(ctlbits, n))
> -		    off += snprintf(ptr+off, sizeof(buf) - off, "+%d ", n);
> -		else
> -		    off += snprintf(ptr+off, sizeof(buf) - off, "-%d ", n);
> +	for (n = NFSD_MINVERS; n <= ((NFSD_MAXVERS < 3) ? NFSD_MAXVERS : 3); n++)
> +		off += nfssvc_print_vers(&buf[off], sizeof(buf) - off,
> +				n, 0, NFSCTL_VERISSET(ctlbits, n));
> +
> +	for (n = 0; n <= NFS4_MAXMINOR; n++) {
> +		if (!NFSCTL_MINORISSET(minorversset, n))
> +			continue;
> +		off += nfssvc_print_vers(&buf[off], sizeof(buf) - off,
> +				4, n, NFSCTL_MINORISSET(minorvers, n));
>  	}
> +	if (!off--)
> +		goto out;
> +	buf[off] = '\0';
>  	xlog(D_GENERAL, "Writing version string to kernel: %s", buf);
> -	snprintf(ptr+off, sizeof(buf) - off, "\n");
> +	snprintf(&buf[off], sizeof(buf) - off, "\n");
>  	if (write(fd, buf, strlen(buf)) != (ssize_t)strlen(buf))
>  		xlog(L_ERROR, "Setting version failed: errno %d (%m)", errno);
> -
> +out:
>  	close(fd);
>  
>  	return;
> diff --git a/utils/nfsd/nfssvc.h b/utils/nfsd/nfssvc.h
> index cd5a7e84dc7e..39ebf37a90fe 100644
> --- a/utils/nfsd/nfssvc.h
> +++ b/utils/nfsd/nfssvc.h
> @@ -28,3 +28,4 @@ void	nfssvc_set_time(const char *type, const int seconds);
>  int	nfssvc_set_rdmaport(const char *port);
>  void	nfssvc_setvers(unsigned int ctlbits, unsigned int minorvers4, unsigned int minorvers4set);
>  int	nfssvc_threads(int nrservs);
> +void	nfssvc_get_minormask(unsigned int *mask);
>