[nfs-utils] sm-notify: ending the grace period early should be configurable
diff mbox

Message ID 1489181959-2905-1-git-send-email-smayhew@redhat.com
State New
Headers show

Commit Message

Scott Mayhew March 10, 2017, 9:39 p.m. UTC
sm-notify's early ending of the grace period when it has no hosts to
notify can cause problems in some high availability configurations,
which may be running one sm-notify per floating IP address in the
cluster.  This commit makes that behavior configurable via the
nfs.conf (I don't think having a corresponding command line option
would be particularly useful, hence none was added).

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 nfs.conf                  |  1 +
 utils/statd/sm-notify.c   |  6 +++++-
 utils/statd/sm-notify.man | 18 ++++++++++++++++++
 3 files changed, 24 insertions(+), 1 deletion(-)

Comments

J. Bruce Fields March 13, 2017, 8:19 p.m. UTC | #1
On Fri, Mar 10, 2017 at 04:39:19PM -0500, Scott Mayhew wrote:
> sm-notify's early ending of the grace period when it has no hosts to
> notify can cause problems in some high availability configurations,
> which may be running one sm-notify per floating IP address in the
> cluster.

So, there may be no hosts associated with this floating IP, but there
may be with another?

> This commit makes that behavior configurable via the
> nfs.conf (I don't think having a corresponding command line option
> would be particularly useful, hence none was added).

And, I see that you're defaulting it to on to match existing behavior.

Makes sense to me, ACK.--b.

> 
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---
>  nfs.conf                  |  1 +
>  utils/statd/sm-notify.c   |  6 +++++-
>  utils/statd/sm-notify.man | 18 ++++++++++++++++++
>  3 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/nfs.conf b/nfs.conf
> index 81ece06..690645c 100644
> --- a/nfs.conf
> +++ b/nfs.conf
> @@ -65,6 +65,7 @@
>  # retry-time=900
>  # outgoing-port=
>  # outgoing-addr=
> +# lift-grace=y
>  #
>  #[svcgssd]
>  # principal=
> diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
> index 623213e..0c6766f 100644
> --- a/utils/statd/sm-notify.c
> +++ b/utils/statd/sm-notify.c
> @@ -45,6 +45,8 @@
>  
>  #define NLM_END_GRACE_FILE	"/proc/fs/lockd/nlm_end_grace"
>  
> +int lift_grace = 1;
> +
>  struct nsm_host {
>  	struct nsm_host *	next;
>  	char *			name;
> @@ -494,6 +496,7 @@ main(int argc, char **argv)
>  	opt_max_retry = conf_get_num("sm-notify", "retry-time", opt_max_retry / 60) * 60;
>  	opt_srcport = conf_get_str("sm-notify", "outgoing-port");
>  	opt_srcaddr = conf_get_str("sm-notify", "outgoing-addr");
> +	lift_grace = conf_get_bool("sm-notify", "lift-grace", lift_grace);
>  	s = conf_get_str("statd", "state-directory-path");
>  	if (s && !nsm_setup_pathnames(argv[0], s))
>  		exit(1);
> @@ -570,7 +573,8 @@ usage:		fprintf(stderr,
>  	(void)nsm_retire_monitored_hosts();
>  	if (nsm_load_notify_list(smn_get_host) == 0) {
>  		xlog(D_GENERAL, "No hosts to notify; exiting");
> -		nsm_lift_grace_period();
> +		if (lift_grace)
> +			nsm_lift_grace_period();
>  		return 0;
>  	}
>  
> diff --git a/utils/statd/sm-notify.man b/utils/statd/sm-notify.man
> index bb7f6e0..cfe1e4b 100644
> --- a/utils/statd/sm-notify.man
> +++ b/utils/statd/sm-notify.man
> @@ -241,6 +241,24 @@ These have the same effect as the command line options
>  .B v
>  respectively.
>  
> +An additional value recognized in the
> +.B [sm-notify]
> +section is
> +.BR lift-grace .
> +By default,
> +.B sm-notify
> +will lift lockd's grace period early if it has no hosts to notify.
> +Some high availability configurations will run one
> +.B sm-notify
> +per floating IP address.  In these configurations, lifting the
> +grace period early may prevent clients from reclaiming locks.
> +.RB "Setting " lift-grace " to " n
> +will prevent
> +.B sm-notify
> +from ending the grace period early.
> +.B lift-grace
> +has no corresponding command line option.
> +
>  The value recognized in the
>  .B [statd]
>  section is
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Mayhew March 13, 2017, 9:10 p.m. UTC | #2
On Mon, 13 Mar 2017, J. Bruce Fields wrote:

> On Fri, Mar 10, 2017 at 04:39:19PM -0500, Scott Mayhew wrote:
> > sm-notify's early ending of the grace period when it has no hosts to
> > notify can cause problems in some high availability configurations,
> > which may be running one sm-notify per floating IP address in the
> > cluster.
> 
> So, there may be no hosts associated with this floating IP, but there
> may be with another?

Exactly.  What was happening in my testing was that clients would
receive and respond to a NOTIFY, and would immediately send a LOCK with
reclaim=yes but would get NFSERR_NO_GRACE in return.

BTW I had a cluster with this change running at Connectathon.

-Scott
> 
> > This commit makes that behavior configurable via the
> > nfs.conf (I don't think having a corresponding command line option
> > would be particularly useful, hence none was added).
> 
> And, I see that you're defaulting it to on to match existing behavior.
> 
> Makes sense to me, ACK.--b.
> 
> > 
> > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > ---
> >  nfs.conf                  |  1 +
> >  utils/statd/sm-notify.c   |  6 +++++-
> >  utils/statd/sm-notify.man | 18 ++++++++++++++++++
> >  3 files changed, 24 insertions(+), 1 deletion(-)
> > 
> > diff --git a/nfs.conf b/nfs.conf
> > index 81ece06..690645c 100644
> > --- a/nfs.conf
> > +++ b/nfs.conf
> > @@ -65,6 +65,7 @@
> >  # retry-time=900
> >  # outgoing-port=
> >  # outgoing-addr=
> > +# lift-grace=y
> >  #
> >  #[svcgssd]
> >  # principal=
> > diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
> > index 623213e..0c6766f 100644
> > --- a/utils/statd/sm-notify.c
> > +++ b/utils/statd/sm-notify.c
> > @@ -45,6 +45,8 @@
> >  
> >  #define NLM_END_GRACE_FILE	"/proc/fs/lockd/nlm_end_grace"
> >  
> > +int lift_grace = 1;
> > +
> >  struct nsm_host {
> >  	struct nsm_host *	next;
> >  	char *			name;
> > @@ -494,6 +496,7 @@ main(int argc, char **argv)
> >  	opt_max_retry = conf_get_num("sm-notify", "retry-time", opt_max_retry / 60) * 60;
> >  	opt_srcport = conf_get_str("sm-notify", "outgoing-port");
> >  	opt_srcaddr = conf_get_str("sm-notify", "outgoing-addr");
> > +	lift_grace = conf_get_bool("sm-notify", "lift-grace", lift_grace);
> >  	s = conf_get_str("statd", "state-directory-path");
> >  	if (s && !nsm_setup_pathnames(argv[0], s))
> >  		exit(1);
> > @@ -570,7 +573,8 @@ usage:		fprintf(stderr,
> >  	(void)nsm_retire_monitored_hosts();
> >  	if (nsm_load_notify_list(smn_get_host) == 0) {
> >  		xlog(D_GENERAL, "No hosts to notify; exiting");
> > -		nsm_lift_grace_period();
> > +		if (lift_grace)
> > +			nsm_lift_grace_period();
> >  		return 0;
> >  	}
> >  
> > diff --git a/utils/statd/sm-notify.man b/utils/statd/sm-notify.man
> > index bb7f6e0..cfe1e4b 100644
> > --- a/utils/statd/sm-notify.man
> > +++ b/utils/statd/sm-notify.man
> > @@ -241,6 +241,24 @@ These have the same effect as the command line options
> >  .B v
> >  respectively.
> >  
> > +An additional value recognized in the
> > +.B [sm-notify]
> > +section is
> > +.BR lift-grace .
> > +By default,
> > +.B sm-notify
> > +will lift lockd's grace period early if it has no hosts to notify.
> > +Some high availability configurations will run one
> > +.B sm-notify
> > +per floating IP address.  In these configurations, lifting the
> > +grace period early may prevent clients from reclaiming locks.
> > +.RB "Setting " lift-grace " to " n
> > +will prevent
> > +.B sm-notify
> > +from ending the grace period early.
> > +.B lift-grace
> > +has no corresponding command line option.
> > +
> >  The value recognized in the
> >  .B [statd]
> >  section is
> > -- 
> > 2.7.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
J. Bruce Fields March 13, 2017, 9:21 p.m. UTC | #3
On Mon, Mar 13, 2017 at 05:10:26PM -0400, Scott Mayhew wrote:
> On Mon, 13 Mar 2017, J. Bruce Fields wrote:
> 
> > On Fri, Mar 10, 2017 at 04:39:19PM -0500, Scott Mayhew wrote:
> > > sm-notify's early ending of the grace period when it has no hosts to
> > > notify can cause problems in some high availability configurations,
> > > which may be running one sm-notify per floating IP address in the
> > > cluster.
> > 
> > So, there may be no hosts associated with this floating IP, but there
> > may be with another?
> 
> Exactly.  What was happening in my testing was that clients would
> receive and respond to a NOTIFY, and would immediately send a LOCK with
> reclaim=yes but would get NFSERR_NO_GRACE in return.
> 
> BTW I had a cluster with this change running at Connectathon.

Great, thanks.--b.

> 
> -Scott
> > 
> > > This commit makes that behavior configurable via the
> > > nfs.conf (I don't think having a corresponding command line option
> > > would be particularly useful, hence none was added).
> > 
> > And, I see that you're defaulting it to on to match existing behavior.
> > 
> > Makes sense to me, ACK.--b.
> > 
> > > 
> > > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > > ---
> > >  nfs.conf                  |  1 +
> > >  utils/statd/sm-notify.c   |  6 +++++-
> > >  utils/statd/sm-notify.man | 18 ++++++++++++++++++
> > >  3 files changed, 24 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/nfs.conf b/nfs.conf
> > > index 81ece06..690645c 100644
> > > --- a/nfs.conf
> > > +++ b/nfs.conf
> > > @@ -65,6 +65,7 @@
> > >  # retry-time=900
> > >  # outgoing-port=
> > >  # outgoing-addr=
> > > +# lift-grace=y
> > >  #
> > >  #[svcgssd]
> > >  # principal=
> > > diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
> > > index 623213e..0c6766f 100644
> > > --- a/utils/statd/sm-notify.c
> > > +++ b/utils/statd/sm-notify.c
> > > @@ -45,6 +45,8 @@
> > >  
> > >  #define NLM_END_GRACE_FILE	"/proc/fs/lockd/nlm_end_grace"
> > >  
> > > +int lift_grace = 1;
> > > +
> > >  struct nsm_host {
> > >  	struct nsm_host *	next;
> > >  	char *			name;
> > > @@ -494,6 +496,7 @@ main(int argc, char **argv)
> > >  	opt_max_retry = conf_get_num("sm-notify", "retry-time", opt_max_retry / 60) * 60;
> > >  	opt_srcport = conf_get_str("sm-notify", "outgoing-port");
> > >  	opt_srcaddr = conf_get_str("sm-notify", "outgoing-addr");
> > > +	lift_grace = conf_get_bool("sm-notify", "lift-grace", lift_grace);
> > >  	s = conf_get_str("statd", "state-directory-path");
> > >  	if (s && !nsm_setup_pathnames(argv[0], s))
> > >  		exit(1);
> > > @@ -570,7 +573,8 @@ usage:		fprintf(stderr,
> > >  	(void)nsm_retire_monitored_hosts();
> > >  	if (nsm_load_notify_list(smn_get_host) == 0) {
> > >  		xlog(D_GENERAL, "No hosts to notify; exiting");
> > > -		nsm_lift_grace_period();
> > > +		if (lift_grace)
> > > +			nsm_lift_grace_period();
> > >  		return 0;
> > >  	}
> > >  
> > > diff --git a/utils/statd/sm-notify.man b/utils/statd/sm-notify.man
> > > index bb7f6e0..cfe1e4b 100644
> > > --- a/utils/statd/sm-notify.man
> > > +++ b/utils/statd/sm-notify.man
> > > @@ -241,6 +241,24 @@ These have the same effect as the command line options
> > >  .B v
> > >  respectively.
> > >  
> > > +An additional value recognized in the
> > > +.B [sm-notify]
> > > +section is
> > > +.BR lift-grace .
> > > +By default,
> > > +.B sm-notify
> > > +will lift lockd's grace period early if it has no hosts to notify.
> > > +Some high availability configurations will run one
> > > +.B sm-notify
> > > +per floating IP address.  In these configurations, lifting the
> > > +grace period early may prevent clients from reclaiming locks.
> > > +.RB "Setting " lift-grace " to " n
> > > +will prevent
> > > +.B sm-notify
> > > +from ending the grace period early.
> > > +.B lift-grace
> > > +has no corresponding command line option.
> > > +
> > >  The value recognized in the
> > >  .B [statd]
> > >  section is
> > > -- 
> > > 2.7.4
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steve Dickson April 4, 2017, 5:59 p.m. UTC | #4
On 03/10/2017 04:39 PM, Scott Mayhew wrote:
> sm-notify's early ending of the grace period when it has no hosts to
> notify can cause problems in some high availability configurations,
> which may be running one sm-notify per floating IP address in the
> cluster.  This commit makes that behavior configurable via the
> nfs.conf (I don't think having a corresponding command line option
> would be particularly useful, hence none was added).
> 
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
Committed... 

steved.
> ---
>  nfs.conf                  |  1 +
>  utils/statd/sm-notify.c   |  6 +++++-
>  utils/statd/sm-notify.man | 18 ++++++++++++++++++
>  3 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/nfs.conf b/nfs.conf
> index 81ece06..690645c 100644
> --- a/nfs.conf
> +++ b/nfs.conf
> @@ -65,6 +65,7 @@
>  # retry-time=900
>  # outgoing-port=
>  # outgoing-addr=
> +# lift-grace=y
>  #
>  #[svcgssd]
>  # principal=
> diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
> index 623213e..0c6766f 100644
> --- a/utils/statd/sm-notify.c
> +++ b/utils/statd/sm-notify.c
> @@ -45,6 +45,8 @@
>  
>  #define NLM_END_GRACE_FILE	"/proc/fs/lockd/nlm_end_grace"
>  
> +int lift_grace = 1;
> +
>  struct nsm_host {
>  	struct nsm_host *	next;
>  	char *			name;
> @@ -494,6 +496,7 @@ main(int argc, char **argv)
>  	opt_max_retry = conf_get_num("sm-notify", "retry-time", opt_max_retry / 60) * 60;
>  	opt_srcport = conf_get_str("sm-notify", "outgoing-port");
>  	opt_srcaddr = conf_get_str("sm-notify", "outgoing-addr");
> +	lift_grace = conf_get_bool("sm-notify", "lift-grace", lift_grace);
>  	s = conf_get_str("statd", "state-directory-path");
>  	if (s && !nsm_setup_pathnames(argv[0], s))
>  		exit(1);
> @@ -570,7 +573,8 @@ usage:		fprintf(stderr,
>  	(void)nsm_retire_monitored_hosts();
>  	if (nsm_load_notify_list(smn_get_host) == 0) {
>  		xlog(D_GENERAL, "No hosts to notify; exiting");
> -		nsm_lift_grace_period();
> +		if (lift_grace)
> +			nsm_lift_grace_period();
>  		return 0;
>  	}
>  
> diff --git a/utils/statd/sm-notify.man b/utils/statd/sm-notify.man
> index bb7f6e0..cfe1e4b 100644
> --- a/utils/statd/sm-notify.man
> +++ b/utils/statd/sm-notify.man
> @@ -241,6 +241,24 @@ These have the same effect as the command line options
>  .B v
>  respectively.
>  
> +An additional value recognized in the
> +.B [sm-notify]
> +section is
> +.BR lift-grace .
> +By default,
> +.B sm-notify
> +will lift lockd's grace period early if it has no hosts to notify.
> +Some high availability configurations will run one
> +.B sm-notify
> +per floating IP address.  In these configurations, lifting the
> +grace period early may prevent clients from reclaiming locks.
> +.RB "Setting " lift-grace " to " n
> +will prevent
> +.B sm-notify
> +from ending the grace period early.
> +.B lift-grace
> +has no corresponding command line option.
> +
>  The value recognized in the
>  .B [statd]
>  section is
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/nfs.conf b/nfs.conf
index 81ece06..690645c 100644
--- a/nfs.conf
+++ b/nfs.conf
@@ -65,6 +65,7 @@ 
 # retry-time=900
 # outgoing-port=
 # outgoing-addr=
+# lift-grace=y
 #
 #[svcgssd]
 # principal=
diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
index 623213e..0c6766f 100644
--- a/utils/statd/sm-notify.c
+++ b/utils/statd/sm-notify.c
@@ -45,6 +45,8 @@ 
 
 #define NLM_END_GRACE_FILE	"/proc/fs/lockd/nlm_end_grace"
 
+int lift_grace = 1;
+
 struct nsm_host {
 	struct nsm_host *	next;
 	char *			name;
@@ -494,6 +496,7 @@  main(int argc, char **argv)
 	opt_max_retry = conf_get_num("sm-notify", "retry-time", opt_max_retry / 60) * 60;
 	opt_srcport = conf_get_str("sm-notify", "outgoing-port");
 	opt_srcaddr = conf_get_str("sm-notify", "outgoing-addr");
+	lift_grace = conf_get_bool("sm-notify", "lift-grace", lift_grace);
 	s = conf_get_str("statd", "state-directory-path");
 	if (s && !nsm_setup_pathnames(argv[0], s))
 		exit(1);
@@ -570,7 +573,8 @@  usage:		fprintf(stderr,
 	(void)nsm_retire_monitored_hosts();
 	if (nsm_load_notify_list(smn_get_host) == 0) {
 		xlog(D_GENERAL, "No hosts to notify; exiting");
-		nsm_lift_grace_period();
+		if (lift_grace)
+			nsm_lift_grace_period();
 		return 0;
 	}
 
diff --git a/utils/statd/sm-notify.man b/utils/statd/sm-notify.man
index bb7f6e0..cfe1e4b 100644
--- a/utils/statd/sm-notify.man
+++ b/utils/statd/sm-notify.man
@@ -241,6 +241,24 @@  These have the same effect as the command line options
 .B v
 respectively.
 
+An additional value recognized in the
+.B [sm-notify]
+section is
+.BR lift-grace .
+By default,
+.B sm-notify
+will lift lockd's grace period early if it has no hosts to notify.
+Some high availability configurations will run one
+.B sm-notify
+per floating IP address.  In these configurations, lifting the
+grace period early may prevent clients from reclaiming locks.
+.RB "Setting " lift-grace " to " n
+will prevent
+.B sm-notify
+from ending the grace period early.
+.B lift-grace
+has no corresponding command line option.
+
 The value recognized in the
 .B [statd]
 section is