On Thu, 09 Jan 2025, Jeff Layton wrote:
> On Wed, 2025-01-08 at 17:54 -0500, Scott Mayhew wrote:
> > The section for the 'nfsdctl version' subcommand on the man page states
> > that the minorversion is optional, and if omitted it will cause all
> > minorversions to be enabled/disabled, but it currently doesn't work that
> > way.
> >
> > Make it work that way, with one exception. If v4.0 is disabled, then
> > 'nfsdctl version +4' will not re-enable it; instead it must be
> > explicitly re-enabled via 'nfsdctl version +4.0'. This mirrors the way
> > /proc/fs/nfsd/versions works.
> >
>
> The question is: do we want to mirror that particular quirk in the
> interface? I'm not sure if there was a logical reason for making +4
> work that way in the /proc interface, so it's not clear to me that we
> want to replicate that here.
Digging thru my email archive it seems that the reason for that was to
deal with really old kernels that understood +4/-4 but not +4.0/-4.0.
Since old kernels obviously won't have a netlink interface for nfsd it
seems we can drop this quirk at least.
>
> Honestly, it may be better to just require explicit minorversions in
> this interface and not worry about trying to interpret what +4
> means. You'd need to specify "+4.1 +4.2" instead of just saying "+4",
> but that doesn't seem too onerous.
>
> Thoughts?
It seems to me main alternatives are
1. Drop the special handling for v4.0 but allow +4/-4 to always
enable/disable all minor versions, including v4.0. So a small
adjustment to this patch.
2. Require the minor versions to be explicitly specified for v4. So allow
+2/-2/+3/-3, but emit an error if +4/-4 is specified. Maybe emit an error
if +2.0/-2.0/+3.0/-3.0 is specified.
3. Leave the code alone (so +4/-4 would be interpreted as +4.0/-4.1) and
just update the man page.
4. Make the 'nfsdctl version' behavior consistent with the nfs.conf option
handling in 'nfsdctl autostart'. Currently thats:
* vers4=y turns on all minor versions. vers4.x=n on top of that
will turn off those specific minor versions.
* vers4=n turns off all minor versions. vers4.x=y on top of
that has no effect.
I don't really have a strong preference as long as the behavior matches
whatever the man page says it is. If I had to vote I'd just say go with
option 3, but if it's important for the behaviors of the different
subcommands to be consistent, then we really don't have a choice and
have to go with option 4.
>
> > Link: https://issues.redhat.com/browse/RHEL-72477
> > Signed-off-by: Scott Mayhew <smayhew@xxxxxxxxxx>
> > ---
> > utils/nfsdctl/nfsdctl.8 | 9 ++++--
> > utils/nfsdctl/nfsdctl.adoc | 5 +++-
> > utils/nfsdctl/nfsdctl.c | 58 +++++++++++++++++++++++++++++++++++---
> > 3 files changed, 64 insertions(+), 8 deletions(-)
> >
> > diff --git a/utils/nfsdctl/nfsdctl.8 b/utils/nfsdctl/nfsdctl.8
> > index b08fe803..835d60b4 100644
> > --- a/utils/nfsdctl/nfsdctl.8
> > +++ b/utils/nfsdctl/nfsdctl.8
> > @@ -2,12 +2,12 @@
> > .\" Title: nfsdctl
> > .\" Author: Jeff Layton
> > .\" Generator: Asciidoctor 2.0.20
> > -.\" Date: 2024-12-30
> > +.\" Date: 2025-01-08
> > .\" Manual: \ \&
> > .\" Source: \ \&
> > .\" Language: English
> > .\"
> > -.TH "NFSDCTL" "8" "2024-12-30" "\ \&" "\ \&"
> > +.TH "NFSDCTL" "8" "2025-01-08" "\ \&" "\ \&"
> > .ie \n(.g .ds Aq \(aq
> > .el .ds Aq '
> > .ss \n[.ss] 0
> > @@ -172,7 +172,10 @@ MINOR: the minor version integer value
> > .nf
> > .fam C
> > The minorversion field is optional. If not given, it will disable or enable
> > -all minorversions for that major version.
> > +all minorversions for that major version. Note however that if NFSv4.0 was
> > +previously disabled, it can only be re\-enabled by explicitly specifying the
> > +minorversion (this mirrors the behavior of the /proc/fs/nfsd/versions
> > +interface).
> > .fam
> > .fi
> > .if n .RE
> > diff --git a/utils/nfsdctl/nfsdctl.adoc b/utils/nfsdctl/nfsdctl.adoc
> > index c5921458..20e9bf8e 100644
> > --- a/utils/nfsdctl/nfsdctl.adoc
> > +++ b/utils/nfsdctl/nfsdctl.adoc
> > @@ -91,7 +91,10 @@ Each subcommand can also accept its own set of options and arguments. The
> > MINOR: the minor version integer value
> >
> > The minorversion field is optional. If not given, it will disable or enable
> > - all minorversions for that major version.
> > + all minorversions for that major version. Note however that if NFSv4.0 was
> > + previously disabled, it can only be re-enabled by explicitly specifying the
> > + minorversion (this mirrors the behavior of the /proc/fs/nfsd/versions
> > + interface).
> >
> > Note that versions can only be set when there are no nfsd threads running.
> >
> > diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
> > index 722bf4a0..d86ff80e 100644
> > --- a/utils/nfsdctl/nfsdctl.c
> > +++ b/utils/nfsdctl/nfsdctl.c
> > @@ -761,6 +761,32 @@ static int update_nfsd_version(int major, int minor, bool enabled)
> > return -EINVAL;
> > }
> >
> > +static bool v40_is_disabled(void)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < MAX_NFS_VERSIONS; ++i) {
> > + if (nfsd_versions[i].major == 0)
> > + break;
> > + if (nfsd_versions[i].major == 4 && nfsd_versions[i].minor == 0)
> > + return !nfsd_versions[i].enabled;
> > + }
> > + return false;
> > +}
> > +
> > +static int get_max_minorversion(void)
> > +{
> > + int i, max = 0;
> > +
> > + for (i = 0; i < MAX_NFS_VERSIONS; ++i) {
> > + if (nfsd_versions[i].major == 0)
> > + break;
> > + if (nfsd_versions[i].major == 4 && nfsd_versions[i].minor > max)
> > + max = nfsd_versions[i].minor;
> > + }
> > + return max;
> > +}
> > +
> > static void version_usage(void)
> > {
> > printf("Usage: %s version { {+,-}major.minor } ...\n", taskname);
> > @@ -778,7 +804,8 @@ static void version_usage(void)
> >
> > static int version_func(struct nl_sock *sock, int argc, char ** argv)
> > {
> > - int ret, i;
> > + int ret, i, j, max_minor;
> > + bool v40_disabled;
> >
> > /* help is only valid as first argument after command */
> > if (argc > 1 &&
> > @@ -792,6 +819,9 @@ static int version_func(struct nl_sock *sock, int argc, char ** argv)
> > return ret;
> >
> > if (argc > 1) {
> > + v40_disabled = v40_is_disabled();
> > + max_minor = get_max_minorversion();
> > +
> > for (i = 1; i < argc; ++i) {
> > int ret, major, minor = 0;
> > char sign = '\0', *str = argv[i];
> > @@ -815,9 +845,29 @@ static int version_func(struct nl_sock *sock, int argc, char ** argv)
> > return -EINVAL;
> > }
> >
> > - ret = update_nfsd_version(major, minor, enabled);
> > - if (ret)
> > - return ret;
> > + /*
> > + * The minorversion field is optional. If omitted, it should
> > + * cause all the minor versions for that major version to be
> > + * enabled/disabled.
> > + *
> > + * HOWEVER, we do not enable v4.0 in this manner if it was
> > + * previously disabled - it has to be explicitly enabled
> > + * instead. This is to retain the behavior of the old
> > + * /proc/fs/nfsd/versions interface.
> > + */
> > + if (major == 4 && ret == 2) {
> > + for (j = 0; j <= max_minor; ++j) {
> > + if (j == 0 && enabled && v40_disabled)
> > + continue;
> > + ret = update_nfsd_version(major, j, enabled);
> > + if (ret)
> > + return ret;
> > + }
> > + } else {
> > + ret = update_nfsd_version(major, minor, enabled);
> > + if (ret)
> > + return ret;
> > + }
> > }
> > return set_nfsd_versions(sock);
> > }
>
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
>
