[PATCH 0/6] First set of patches for 2.6.29

[PATCH 0/6] First set of patches for 2.6.29

[Chuck Lever]

These patches add some IPv6 support to the Linux kernel's NSM upcall
implementation, and address a couple of shortcomings in the existing
NLM/NSM implementation.

I'd like you to consider these for 2.6.29.

There are 40 more patches I have queued for 2.6.29 that finish IPv6
support in the NSM upcall implementation, enable IPv6 for lockd, and
finish and enable IPv6 support in NFSD.

---

Chuck Lever (6):
      NSM: Serialize SM_MON and SM_UNMON upcalls
      NLM: Clean up nsm_monitor() call
      NLM: Clean up nsm_monitor() call
      NSM: Support IPv6 version of mon_name
      NLM: Support IPv6 scope IDs in nlm_display_address()
      NLM: Remove address eye-catcher buffers from nlm_host


 fs/lockd/host.c                |   53 ++++++++++--------
 fs/lockd/mon.c                 |  120 +++++++++++++++++++++-------------------
 include/linux/lockd/lockd.h    |   21 ++++++-
 include/linux/lockd/sm_inter.h |    2 -
 4 files changed, 109 insertions(+), 87 deletions(-)

-- 
Chuck Lever

[PATCH 1/6] NLM: Remove address eye-catcher buffers from nlm_host

[Chuck Lever]

The h_name field in struct nlm_host is a just copy of
h_nsmhandle->sm_name.  Likewise, the contents of the h_addrbuf field
should be identical to the sm_addrbuf field.

The h_srcaddrbuf field is used only in one place for debugging.  We can
live without this until we get %pI formatting for printk().

Currently these buffers are 48 bytes, but we need to support scope IDs
in IPv6 presentation addresses, which means making the buffers even
larger.  Instead, let's find ways to eliminate them to save space.

Finally, AF_UNSPEC support is no longer needed in nlm_display_address()
now that it is not used for the h_srcaddr field.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/host.c             |   14 +++-----------
 include/linux/lockd/lockd.h |    3 ---
 2 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index acc2aa5..68e00c0 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -112,9 +112,6 @@ static void nlm_display_address(const struct sockaddr *sap,
 	const struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)sap;
 
 	switch (sap->sa_family) {
-	case AF_UNSPEC:
-		snprintf(buf, len, "unspecified");
-		break;
 	case AF_INET:
 		snprintf(buf, len, NIPQUAD_FMT, NIPQUAD(sin->sin_addr.s_addr));
 		break;
@@ -178,7 +175,7 @@ static struct nlm_host *nlm_lookup_host(struct nlm_lookup_host_info *ni)
 
 		nlm_get_host(host);
 		dprintk("lockd: nlm_lookup_host found host %s (%s)\n",
-				host->h_name, host->h_addrbuf);
+				host->h_name, nsm->sm_addrbuf);
 		goto out;
 	}
 
@@ -232,11 +229,6 @@ static struct nlm_host *nlm_lookup_host(struct nlm_lookup_host_info *ni)
 
 	nrhosts++;
 
-	nlm_display_address((struct sockaddr *)&host->h_addr,
-				host->h_addrbuf, sizeof(host->h_addrbuf));
-	nlm_display_address((struct sockaddr *)&host->h_srcaddr,
-				host->h_srcaddrbuf, sizeof(host->h_srcaddrbuf));
-
 	dprintk("lockd: nlm_lookup_host created host %s\n",
 			host->h_name);
 
@@ -378,8 +370,8 @@ nlm_bind_host(struct nlm_host *host)
 {
 	struct rpc_clnt	*clnt;
 
-	dprintk("lockd: nlm_bind_host %s (%s), my addr=%s\n",
-			host->h_name, host->h_addrbuf, host->h_srcaddrbuf);
+	dprintk("lockd: nlm_bind_host %s (%s)\n",
+			host->h_name, host->h_nsmhandle->sm_addrbuf);
 
 	/* Lock host handle */
 	mutex_lock(&host->h_mutex);
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 23da3fa..4467b83 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -65,9 +65,6 @@ struct nlm_host {
 	struct list_head	h_granted;	/* Locks in GRANTED state */
 	struct list_head	h_reclaim;	/* Locks in RECLAIM state */
 	struct nsm_handle *	h_nsmhandle;	/* NSM status handle */
-
-	char			h_addrbuf[48],	/* address eyecatchers */
-				h_srcaddrbuf[48];
 };
 
 struct nsm_handle {

Re: [PATCH 1/6] NLM: Remove address eye-catcher buffers from nlm_host

[J. Bruce Fields]

On Mon, Dec 01, 2008 at 01:57:34PM -0500, Chuck Lever wrote:
> The h_name field in struct nlm_host is a just copy of
> h_nsmhandle->sm_name.  Likewise, the contents of the h_addrbuf field
> should be identical to the sm_addrbuf field.
> 
> The h_srcaddrbuf field is used only in one place for debugging.  We can
> live without this until we get %pI formatting for printk().
> 
> Currently these buffers are 48 bytes, but we need to support scope IDs
> in IPv6 presentation addresses, which means making the buffers even
> larger.  Instead, let's find ways to eliminate them to save space.
> 
> Finally, AF_UNSPEC support is no longer needed in nlm_display_address()
> now that it is not used for the h_srcaddr field.

As usual, whenever there's a "finally, ..." or "next, ..." I'd like a
separate patch.

Though this is distinct enough it's not really a problem to read.  Leave
it as is, OK.

> @@ -232,11 +229,6 @@ static struct nlm_host *nlm_lookup_host(struct nlm_lookup_host_info *ni)
>  
>  	nrhosts++;
>  
> -	nlm_display_address((struct sockaddr *)&host->h_addr,
> -				host->h_addrbuf, sizeof(host->h_addrbuf));
> -	nlm_display_address((struct sockaddr *)&host->h_srcaddr,
> -				host->h_srcaddrbuf, sizeof(host->h_srcaddrbuf));
> -
>  	dprintk("lockd: nlm_lookup_host created host %s\n",
>  			host->h_name);
>  
> @@ -378,8 +370,8 @@ nlm_bind_host(struct nlm_host *host)
>  {
>  	struct rpc_clnt	*clnt;
>  
> -	dprintk("lockd: nlm_bind_host %s (%s), my addr=%s\n",
> -			host->h_name, host->h_addrbuf, host->h_srcaddrbuf);
> +	dprintk("lockd: nlm_bind_host %s (%s)\n",
> +			host->h_name, host->h_nsmhandle->sm_addrbuf);

Hm, just checking: so the only place I can see h_nsmhandle set NULL is
in nsm_unmonitor(), which is called only from nlm_destroy_host(),
shortly before a kfree(host), but before a possible
rpc_shutdown_client()--and doesn't look like an rpc task could be
calling rpc_bind_host()?  OK.

And on the creation end, looks like h_nsmhandle is set on host creation.
So that won't be a NULL deference.  OK, looks fine.  Applied.

--b.

>  
>  	/* Lock host handle */
>  	mutex_lock(&host->h_mutex);
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index 23da3fa..4467b83 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -65,9 +65,6 @@ struct nlm_host {
>  	struct list_head	h_granted;	/* Locks in GRANTED state */
>  	struct list_head	h_reclaim;	/* Locks in RECLAIM state */
>  	struct nsm_handle *	h_nsmhandle;	/* NSM status handle */
> -
> -	char			h_addrbuf[48],	/* address eyecatchers */
> -				h_srcaddrbuf[48];
>  };
>  
>  struct nsm_handle {
> 

Re: [PATCH 1/6] NLM: Remove address eye-catcher buffers from nlm_host

[Chuck Lever]

On Dec 3, 2008, at 2:03 PM, J. Bruce Fields wrote:
> On Mon, Dec 01, 2008 at 01:57:34PM -0500, Chuck Lever wrote:
>> The h_name field in struct nlm_host is a just copy of
>> h_nsmhandle->sm_name.  Likewise, the contents of the h_addrbuf field
>> should be identical to the sm_addrbuf field.
>>
>> The h_srcaddrbuf field is used only in one place for debugging.  We  
>> can
>> live without this until we get %pI formatting for printk().
>>
>> Currently these buffers are 48 bytes, but we need to support scope  
>> IDs
>> in IPv6 presentation addresses, which means making the buffers even
>> larger.  Instead, let's find ways to eliminate them to save space.
>>
>> Finally, AF_UNSPEC support is no longer needed in  
>> nlm_display_address()
>> now that it is not used for the h_srcaddr field.
>
> As usual, whenever there's a "finally, ..." or "next, ..." I'd like a
> separate patch.
>
> Though this is distinct enough it's not really a problem to read.   
> Leave
> it as is, OK.

It seemed like a small change (one chunk) and is tightly related to  
what this patch does, so I left it in.

>> @@ -232,11 +229,6 @@ static struct nlm_host *nlm_lookup_host(struct  
>> nlm_lookup_host_info *ni)
>>
>> 	nrhosts++;
>>
>> -	nlm_display_address((struct sockaddr *)&host->h_addr,
>> -				host->h_addrbuf, sizeof(host->h_addrbuf));
>> -	nlm_display_address((struct sockaddr *)&host->h_srcaddr,
>> -				host->h_srcaddrbuf, sizeof(host->h_srcaddrbuf));
>> -
>> 	dprintk("lockd: nlm_lookup_host created host %s\n",
>> 			host->h_name);
>>
>> @@ -378,8 +370,8 @@ nlm_bind_host(struct nlm_host *host)
>> {
>> 	struct rpc_clnt	*clnt;
>>
>> -	dprintk("lockd: nlm_bind_host %s (%s), my addr=%s\n",
>> -			host->h_name, host->h_addrbuf, host->h_srcaddrbuf);
>> +	dprintk("lockd: nlm_bind_host %s (%s)\n",
>> +			host->h_name, host->h_nsmhandle->sm_addrbuf);
>
> Hm, just checking: so the only place I can see h_nsmhandle set NULL is
> in nsm_unmonitor(), which is called only from nlm_destroy_host(),
> shortly before a kfree(host), but before a possible
> rpc_shutdown_client()--and doesn't look like an rpc task could be
> calling rpc_bind_host()?  OK.

nsm_handles live longer than the nlm_hosts that reference them, so  
this shouldn't be a problem.

> And on the creation end, looks like h_nsmhandle is set on host  
> creation.
> So that won't be a NULL deference.  OK, looks fine.  Applied.
>
>
> --b.
>
>>
>> 	/* Lock host handle */
>> 	mutex_lock(&host->h_mutex);
>> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/ 
>> lockd.h
>> index 23da3fa..4467b83 100644
>> --- a/include/linux/lockd/lockd.h
>> +++ b/include/linux/lockd/lockd.h
>> @@ -65,9 +65,6 @@ struct nlm_host {
>> 	struct list_head	h_granted;	/* Locks in GRANTED state */
>> 	struct list_head	h_reclaim;	/* Locks in RECLAIM state */
>> 	struct nsm_handle *	h_nsmhandle;	/* NSM status handle */
>> -
>> -	char			h_addrbuf[48],	/* address eyecatchers */
>> -				h_srcaddrbuf[48];
>> };
>>
>> struct nsm_handle {
>>


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

[PATCH 2/6] NLM: Support IPv6 scope IDs in nlm_display_address()

[Chuck Lever]

Scope ID support is needed since the kernel's NSM implementation is
about to use these displayed addresses as a mon_name in some cases.

If nsm_use_hostnames is zero, NSM will fail to handle peers that
contact us via a link-local address without scope ID support.  Link-
local addresses do not work without an interface ID, which is stored
in the sockaddr's sin6_scope_id field.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/host.c             |   31 +++++++++++++++++++++++--------
 include/linux/lockd/lockd.h |    2 +-
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index 68e00c0..cd7edc3 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -105,22 +105,37 @@ static void nlm_clear_port(struct sockaddr *sap)
 	}
 }
 
-static void nlm_display_address(const struct sockaddr *sap,
-				char *buf, const size_t len)
+static void nlm_display_ipv4_address(const struct sockaddr *sap, char *buf,
+				     const size_t len)
 {
 	const struct sockaddr_in *sin = (struct sockaddr_in *)sap;
+	snprintf(buf, len, NIPQUAD_FMT, NIPQUAD(sin->sin_addr.s_addr));
+}
+
+static void nlm_display_ipv6_address(const struct sockaddr *sap, char *buf,
+				     const size_t len)
+{
 	const struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)sap;
 
+	if (ipv6_addr_v4mapped(&sin6->sin6_addr))
+		snprintf(buf, len, NIPQUAD_FMT,
+				NIPQUAD(sin6->sin6_addr.s6_addr32[3]));
+	else if (sin6->sin6_scope_id != 0)
+		snprintf(buf, len, NIP6_FMT "%%%u", NIP6(sin6->sin6_addr),
+				sin6->sin6_scope_id);
+	else
+		snprintf(buf, len, NIP6_FMT, NIP6(sin6->sin6_addr));
+}
+
+static void nlm_display_address(const struct sockaddr *sap,
+				char *buf, const size_t len)
+{
 	switch (sap->sa_family) {
 	case AF_INET:
-		snprintf(buf, len, NIPQUAD_FMT, NIPQUAD(sin->sin_addr.s_addr));
+		nlm_display_ipv4_address(sap, buf, len);
 		break;
 	case AF_INET6:
-		if (ipv6_addr_v4mapped(&sin6->sin6_addr))
-			snprintf(buf, len, NIPQUAD_FMT,
-				 NIPQUAD(sin6->sin6_addr.s6_addr32[3]));
-		else
-			snprintf(buf, len, NIP6_FMT, NIP6(sin6->sin6_addr));
+		nlm_display_ipv6_address(sap, buf, len);
 		break;
 	default:
 		snprintf(buf, len, "unsupported address family");
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 4467b83..307a8f0 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -75,7 +75,7 @@ struct nsm_handle {
 	size_t			sm_addrlen;
 	unsigned int		sm_monitored : 1,
 				sm_sticky : 1;	/* don't unmonitor */
-	char			sm_addrbuf[48];	/* address eyecatcher */
+	char			sm_addrbuf[63];	/* presentation address */
 };
 
 /*

Re: [PATCH 2/6] NLM: Support IPv6 scope IDs in nlm_display_address()

[J. Bruce Fields]

On Mon, Dec 01, 2008 at 01:57:42PM -0500, Chuck Lever wrote:
> Scope ID support is needed since the kernel's NSM implementation is
> about to use these displayed addresses as a mon_name in some cases.
> 
> If nsm_use_hostnames is zero, NSM will fail to handle peers that
> contact us via a link-local address without scope ID support.  Link-
> local addresses do not work without an interface ID, which is stored
> in the sockaddr's sin6_scope_id field.

As always: what's the latest progress on getting this
ipv6-address-display stuff into the core networking code?

Anyway, looks fine, but looking forward to be able to delete some of
this one day....

Just one more question below:

> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/lockd/host.c             |   31 +++++++++++++++++++++++--------
>  include/linux/lockd/lockd.h |    2 +-
>  2 files changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index 68e00c0..cd7edc3 100644
> --- a/fs/lockd/host.c
> +++ b/fs/lockd/host.c
> @@ -105,22 +105,37 @@ static void nlm_clear_port(struct sockaddr *sap)
>  	}
>  }
>  
> -static void nlm_display_address(const struct sockaddr *sap,
> -				char *buf, const size_t len)
> +static void nlm_display_ipv4_address(const struct sockaddr *sap, char *buf,
> +				     const size_t len)
>  {
>  	const struct sockaddr_in *sin = (struct sockaddr_in *)sap;
> +	snprintf(buf, len, NIPQUAD_FMT, NIPQUAD(sin->sin_addr.s_addr));
> +}
> +
> +static void nlm_display_ipv6_address(const struct sockaddr *sap, char *buf,
> +				     const size_t len)
> +{
>  	const struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)sap;
>  
> +	if (ipv6_addr_v4mapped(&sin6->sin6_addr))
> +		snprintf(buf, len, NIPQUAD_FMT,
> +				NIPQUAD(sin6->sin6_addr.s6_addr32[3]));
> +	else if (sin6->sin6_scope_id != 0)
> +		snprintf(buf, len, NIP6_FMT "%%%u", NIP6(sin6->sin6_addr),
> +				sin6->sin6_scope_id);
> +	else
> +		snprintf(buf, len, NIP6_FMT, NIP6(sin6->sin6_addr));
> +}
> +
> +static void nlm_display_address(const struct sockaddr *sap,
> +				char *buf, const size_t len)
> +{
>  	switch (sap->sa_family) {
>  	case AF_INET:
> -		snprintf(buf, len, NIPQUAD_FMT, NIPQUAD(sin->sin_addr.s_addr));
> +		nlm_display_ipv4_address(sap, buf, len);
>  		break;
>  	case AF_INET6:
> -		if (ipv6_addr_v4mapped(&sin6->sin6_addr))
> -			snprintf(buf, len, NIPQUAD_FMT,
> -				 NIPQUAD(sin6->sin6_addr.s6_addr32[3]));
> -		else
> -			snprintf(buf, len, NIP6_FMT, NIP6(sin6->sin6_addr));
> +		nlm_display_ipv6_address(sap, buf, len);
>  		break;
>  	default:
>  		snprintf(buf, len, "unsupported address family");
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index 4467b83..307a8f0 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -75,7 +75,7 @@ struct nsm_handle {
>  	size_t			sm_addrlen;
>  	unsigned int		sm_monitored : 1,
>  				sm_sticky : 1;	/* don't unmonitor */
> -	char			sm_addrbuf[48];	/* address eyecatcher */
> +	char			sm_addrbuf[63];	/* presentation address */
>  };

Could you add a brief (1-line?) comment justifying the choice of 63?

--b.

Re: [PATCH 2/6] NLM: Support IPv6 scope IDs in nlm_display_address()

[J. Bruce Fields]

On Wed, Dec 03, 2008 at 02:34:04PM -0500, bfields wrote:
> On Mon, Dec 01, 2008 at 01:57:42PM -0500, Chuck Lever wrote:
> > --- a/include/linux/lockd/lockd.h
> > +++ b/include/linux/lockd/lockd.h
> > @@ -75,7 +75,7 @@ struct nsm_handle {
> >  	size_t			sm_addrlen;
> >  	unsigned int		sm_monitored : 1,
> >  				sm_sticky : 1;	/* don't unmonitor */
> > -	char			sm_addrbuf[48];	/* address eyecatcher */
> > +	char			sm_addrbuf[63];	/* presentation address */
> >  };
> 
> Could you add a brief (1-line?) comment justifying the choice of 63?

So something like just:


 				sm_sticky : 1;	/* don't unmonitor */
	unsigned int		sm_monitored : 1,
				sm_sticky : 1;	/* don't unmonitor */
/*
 * This can hold 8 groups of colon-separated 4-hex-digit numbers, a
 * percent sign, and a scope id (at most 32 bits, in decimal, so):
 * 63 == 8*4 + 7 colons + 1 percent sign + at most 10 digits + padding:
 */
	char			sm_addrbuf[63];	/* presentation address */

[PATCH 3/6] NSM: Support IPv6 version of mon_name

[Chuck Lever]

The "mon_name" argument of the NSMPROC_MON and NSMPROC_UNMON upcalls
is a string that contains the hostname or IP address of the remote peer
to be notified when this host has rebooted.  The sm-notify command uses
this identifier to contact the peer when we reboot, so it must be
either a well-qualified DNS hostname or a presentation format IP
address string.

When the "nsm_use_hostnames" sysctl is set to zero, the kernel's NSM
provides a presentation format IP address in the "mon_name" argument.
Otherwise, the "caller_name" argument from NLM requests is used,
which is usually just the DNS hostname of the peer.

To support IPv6 addresses for the mon_name argument, we use the
nsm_handle's address eye-catcher, which already contains an appropriate
presentation format address string.  Using the eye-catcher string
obviates the need to use a large buffer on the stack to form the
presentation address string for the upcall.

This patch also addresses a subtle bug.

An NSMPROC_MON request and the subsequent NSMPROC_UNMON request for the
same peer are required to use the same value for the "mon_name"
argument.  Otherwise, rpc.statd's NSMPROC_UNMON processing cannot
locate the database entry for that peer and remove it.

If the setting of nsm_use_hostnames is changed between the time the
kernel sends an NSMPROC_MON request and the time it sends the
NSMPROC_UNMON request for the same peer, the "mon_name" argument for
these two requests may not be the same.  This is because the value of
"mon_name" is currently chosen at the moment the call is made based on
the setting of nsm_use_hostnames

To ensure both requests pass identical contents in the "mon_name"
argument, we now select which string to use for the argument in the
nsm_monitor() function.  A pointer to this string is saved in the
nsm_handle so it can be used for the subsequent NSMPROC_UNMON upcall.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/mon.c              |   45 +++++++++++++++++--------------------------
 include/linux/lockd/lockd.h |    1 +
 2 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 4e7e958..a606fbb 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -18,8 +18,6 @@
 
 #define NLMDBG_FACILITY		NLMDBG_MONITOR
 
-#define XDR_ADDRBUF_LEN		(20)
-
 static struct rpc_clnt *	nsm_create(void);
 
 static struct rpc_program	nsm_program;
@@ -37,7 +35,13 @@ nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res)
 {
 	struct rpc_clnt	*clnt;
 	int		status;
-	struct nsm_args	args;
+	struct nsm_args args = {
+		.addr		= nsm_addr_in(nsm)->sin_addr.s_addr,
+		.prog		= NLM_PROGRAM,
+		.vers		= 3,
+		.proc		= NLMPROC_NSM_NOTIFY,
+		.mon_name	= nsm->sm_mon_name,
+	};
 	struct rpc_message msg = {
 		.rpc_argp	= &args,
 		.rpc_resp	= res,
@@ -46,22 +50,18 @@ nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res)
 	clnt = nsm_create();
 	if (IS_ERR(clnt)) {
 		status = PTR_ERR(clnt);
+		dprintk("lockd: failed to create NSM upcall transport, "
+				"status=%d\n", status);
 		goto out;
 	}
 
-	memset(&args, 0, sizeof(args));
-	args.mon_name = nsm->sm_name;
-	args.addr = nsm_addr_in(nsm)->sin_addr.s_addr;
-	args.prog = NLM_PROGRAM;
-	args.vers = 3;
-	args.proc = NLMPROC_NSM_NOTIFY;
 	memset(res, 0, sizeof(*res));
 
 	msg.rpc_proc = &clnt->cl_procinfo[proc];
 	status = rpc_call_sync(clnt, &msg, 0);
 	if (status < 0)
-		printk(KERN_DEBUG "nsm_mon_unmon: rpc failed, status=%d\n",
-			status);
+		dprintk("lockd: NSM upcall RPC failed, status=%d\n",
+				status);
 	else
 		status = 0;
 	rpc_shutdown_client(clnt);
@@ -85,6 +85,12 @@ nsm_monitor(struct nlm_host *host)
 	if (nsm->sm_monitored)
 		return 0;
 
+	/*
+	 * Choose whether to record the caller_name or IP address of
+	 * this peer in the local rpc.statd's database.
+	 */
+	nsm->sm_mon_name = nsm_use_hostnames ? nsm->sm_name : nsm->sm_addrbuf;
+
 	status = nsm_mon_unmon(nsm, SM_MON, &res);
 
 	if (status < 0 || res.status != 0)
@@ -165,25 +171,10 @@ static __be32 *xdr_encode_nsm_string(__be32 *p, char *string)
 
 /*
  * "mon_name" specifies the host to be monitored.
- *
- * Linux uses a text version of the IP address of the remote
- * host as the host identifier (the "mon_name" argument).
- *
- * Linux statd always looks up the canonical hostname first for
- * whatever remote hostname it receives, so this works alright.
  */
 static __be32 *xdr_encode_mon_name(__be32 *p, struct nsm_args *argp)
 {
-	char	buffer[XDR_ADDRBUF_LEN + 1];
-	char	*name = argp->mon_name;
-
-	if (!nsm_use_hostnames) {
-		snprintf(buffer, XDR_ADDRBUF_LEN,
-			 NIPQUAD_FMT, NIPQUAD(argp->addr));
-		name = buffer;
-	}
-
-	return xdr_encode_nsm_string(p, name);
+	return xdr_encode_nsm_string(p, argp->mon_name);
 }
 
 /*
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 307a8f0..de9ea7b 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -73,6 +73,7 @@ struct nsm_handle {
 	char *			sm_name;
 	struct sockaddr_storage	sm_addr;
 	size_t			sm_addrlen;
+	char *			sm_mon_name;
 	unsigned int		sm_monitored : 1,
 				sm_sticky : 1;	/* don't unmonitor */
 	char			sm_addrbuf[63];	/* presentation address */

Re: [PATCH 3/6] NSM: Support IPv6 version of mon_name

[J. Bruce Fields]

On Mon, Dec 01, 2008 at 01:57:50PM -0500, Chuck Lever wrote:
> The "mon_name" argument of the NSMPROC_MON and NSMPROC_UNMON upcalls
> is a string that contains the hostname or IP address of the remote peer
> to be notified when this host has rebooted.  The sm-notify command uses
> this identifier to contact the peer when we reboot, so it must be
> either a well-qualified DNS hostname or a presentation format IP
> address string.
> 
> When the "nsm_use_hostnames" sysctl is set to zero, the kernel's NSM
> provides a presentation format IP address in the "mon_name" argument.
> Otherwise, the "caller_name" argument from NLM requests is used,
> which is usually just the DNS hostname of the peer.
> 
> To support IPv6 addresses for the mon_name argument, we use the
> nsm_handle's address eye-catcher, which already contains an appropriate
> presentation format address string.  Using the eye-catcher string
> obviates the need to use a large buffer on the stack to form the
> presentation address string for the upcall.
> 
> This patch also addresses a subtle bug.
> 
> An NSMPROC_MON request and the subsequent NSMPROC_UNMON request for the
> same peer are required to use the same value for the "mon_name"
> argument.  Otherwise, rpc.statd's NSMPROC_UNMON processing cannot
> locate the database entry for that peer and remove it.

At some point we need to grep for each read of nsm_use_hostnames and
think about what would happen if it changed there.

For example, the check of nsm_use_hostnames when searching for a match
in nsm_find() could cause a spurious failure to find a host.  If the
nsm_find() came from nlm_host_rebooted(), we could fail to release locks
from some dead host.

Probably we should just forbid changing nsm_use_hostnames while the
server is running or an nfs filesystem is mounted.  Or, if that's not
possible, allow changing the sysctl at any time, but only actually look
at it (and store it) once on server startup or first mount (whichever
comes first).

As this requires a root user doing something wrong, fixing this bug
probably isn't high priority enough to block the rest of the ipv6
patches, so we could make a note of the problem and move on for now.

--b.

> If the setting of nsm_use_hostnames is changed between the time the
> kernel sends an NSMPROC_MON request and the time it sends the
> NSMPROC_UNMON request for the same peer, the "mon_name" argument for
> these two requests may not be the same.  This is because the value of
> "mon_name" is currently chosen at the moment the call is made based on
> the setting of nsm_use_hostnames
> 
> To ensure both requests pass identical contents in the "mon_name"
> argument, we now select which string to use for the argument in the
> nsm_monitor() function.  A pointer to this string is saved in the
> nsm_handle so it can be used for the subsequent NSMPROC_UNMON upcall.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/lockd/mon.c              |   45 +++++++++++++++++--------------------------
>  include/linux/lockd/lockd.h |    1 +
>  2 files changed, 19 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
> index 4e7e958..a606fbb 100644
> --- a/fs/lockd/mon.c
> +++ b/fs/lockd/mon.c
> @@ -18,8 +18,6 @@
>  
>  #define NLMDBG_FACILITY		NLMDBG_MONITOR
>  
> -#define XDR_ADDRBUF_LEN		(20)
> -
>  static struct rpc_clnt *	nsm_create(void);
>  
>  static struct rpc_program	nsm_program;
> @@ -37,7 +35,13 @@ nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res)
>  {
>  	struct rpc_clnt	*clnt;
>  	int		status;
> -	struct nsm_args	args;
> +	struct nsm_args args = {
> +		.addr		= nsm_addr_in(nsm)->sin_addr.s_addr,
> +		.prog		= NLM_PROGRAM,
> +		.vers		= 3,
> +		.proc		= NLMPROC_NSM_NOTIFY,
> +		.mon_name	= nsm->sm_mon_name,
> +	};
>  	struct rpc_message msg = {
>  		.rpc_argp	= &args,
>  		.rpc_resp	= res,
> @@ -46,22 +50,18 @@ nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res)
>  	clnt = nsm_create();
>  	if (IS_ERR(clnt)) {
>  		status = PTR_ERR(clnt);
> +		dprintk("lockd: failed to create NSM upcall transport, "
> +				"status=%d\n", status);
>  		goto out;
>  	}
>  
> -	memset(&args, 0, sizeof(args));
> -	args.mon_name = nsm->sm_name;
> -	args.addr = nsm_addr_in(nsm)->sin_addr.s_addr;
> -	args.prog = NLM_PROGRAM;
> -	args.vers = 3;
> -	args.proc = NLMPROC_NSM_NOTIFY;
>  	memset(res, 0, sizeof(*res));
>  
>  	msg.rpc_proc = &clnt->cl_procinfo[proc];
>  	status = rpc_call_sync(clnt, &msg, 0);
>  	if (status < 0)
> -		printk(KERN_DEBUG "nsm_mon_unmon: rpc failed, status=%d\n",
> -			status);
> +		dprintk("lockd: NSM upcall RPC failed, status=%d\n",
> +				status);
>  	else
>  		status = 0;
>  	rpc_shutdown_client(clnt);
> @@ -85,6 +85,12 @@ nsm_monitor(struct nlm_host *host)
>  	if (nsm->sm_monitored)
>  		return 0;
>  
> +	/*
> +	 * Choose whether to record the caller_name or IP address of
> +	 * this peer in the local rpc.statd's database.
> +	 */
> +	nsm->sm_mon_name = nsm_use_hostnames ? nsm->sm_name : nsm->sm_addrbuf;
> +
>  	status = nsm_mon_unmon(nsm, SM_MON, &res);
>  
>  	if (status < 0 || res.status != 0)
> @@ -165,25 +171,10 @@ static __be32 *xdr_encode_nsm_string(__be32 *p, char *string)
>  
>  /*
>   * "mon_name" specifies the host to be monitored.
> - *
> - * Linux uses a text version of the IP address of the remote
> - * host as the host identifier (the "mon_name" argument).
> - *
> - * Linux statd always looks up the canonical hostname first for
> - * whatever remote hostname it receives, so this works alright.
>   */
>  static __be32 *xdr_encode_mon_name(__be32 *p, struct nsm_args *argp)
>  {
> -	char	buffer[XDR_ADDRBUF_LEN + 1];
> -	char	*name = argp->mon_name;
> -
> -	if (!nsm_use_hostnames) {
> -		snprintf(buffer, XDR_ADDRBUF_LEN,
> -			 NIPQUAD_FMT, NIPQUAD(argp->addr));
> -		name = buffer;
> -	}
> -
> -	return xdr_encode_nsm_string(p, name);
> +	return xdr_encode_nsm_string(p, argp->mon_name);
>  }
>  
>  /*
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index 307a8f0..de9ea7b 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -73,6 +73,7 @@ struct nsm_handle {
>  	char *			sm_name;
>  	struct sockaddr_storage	sm_addr;
>  	size_t			sm_addrlen;
> +	char *			sm_mon_name;
>  	unsigned int		sm_monitored : 1,
>  				sm_sticky : 1;	/* don't unmonitor */
>  	char			sm_addrbuf[63];	/* presentation address */
> 

Re: [PATCH 3/6] NSM: Support IPv6 version of mon_name

[Chuck Lever]

On Dec 3, 2008, at 4:01 PM, J. Bruce Fields wrote:
> On Mon, Dec 01, 2008 at 01:57:50PM -0500, Chuck Lever wrote:
>> The "mon_name" argument of the NSMPROC_MON and NSMPROC_UNMON upcalls
>> is a string that contains the hostname or IP address of the remote  
>> peer
>> to be notified when this host has rebooted.  The sm-notify command  
>> uses
>> this identifier to contact the peer when we reboot, so it must be
>> either a well-qualified DNS hostname or a presentation format IP
>> address string.
>>
>> When the "nsm_use_hostnames" sysctl is set to zero, the kernel's NSM
>> provides a presentation format IP address in the "mon_name" argument.
>> Otherwise, the "caller_name" argument from NLM requests is used,
>> which is usually just the DNS hostname of the peer.
>>
>> To support IPv6 addresses for the mon_name argument, we use the
>> nsm_handle's address eye-catcher, which already contains an  
>> appropriate
>> presentation format address string.  Using the eye-catcher string
>> obviates the need to use a large buffer on the stack to form the
>> presentation address string for the upcall.
>>
>> This patch also addresses a subtle bug.
>>
>> An NSMPROC_MON request and the subsequent NSMPROC_UNMON request for  
>> the
>> same peer are required to use the same value for the "mon_name"
>> argument.  Otherwise, rpc.statd's NSMPROC_UNMON processing cannot
>> locate the database entry for that peer and remove it.
>
> At some point we need to grep for each read of nsm_use_hostnames and
> think about what would happen if it changed there.
>
> For example, the check of nsm_use_hostnames when searching for a match
> in nsm_find() could cause a spurious failure to find a host.  If the
> nsm_find() came from nlm_host_rebooted(), we could fail to release  
> locks
> from some dead host.

Agreed.  A subsequent patch in this series removes that particular use  
of nsm_use_hostnames.  An audit of the other use(s) is a good future  
work item.

> Probably we should just forbid changing nsm_use_hostnames while the
> server is running or an nfs filesystem is mounted.  Or, if that's not
> possible, allow changing the sysctl at any time, but only actually  
> look
> at it (and store it) once on server startup or first mount (whichever
> comes first).

Actually, "when lockd comes up" might be an appropriate moment to copy  
the value of this variable.

> As this requires a root user doing something wrong,

Or a bug in rpc.statd or sm-notify...

> fixing this bug
> probably isn't high priority enough to block the rest of the ipv6
> patches, so we could make a note of the problem and move on for now.
>
> --b.
>
>> If the setting of nsm_use_hostnames is changed between the time the
>> kernel sends an NSMPROC_MON request and the time it sends the
>> NSMPROC_UNMON request for the same peer, the "mon_name" argument for
>> these two requests may not be the same.  This is because the value of
>> "mon_name" is currently chosen at the moment the call is made based  
>> on
>> the setting of nsm_use_hostnames
>>
>> To ensure both requests pass identical contents in the "mon_name"
>> argument, we now select which string to use for the argument in the
>> nsm_monitor() function.  A pointer to this string is saved in the
>> nsm_handle so it can be used for the subsequent NSMPROC_UNMON upcall.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> fs/lockd/mon.c              |   45 ++++++++++++++++ 
>> +--------------------------
>> include/linux/lockd/lockd.h |    1 +
>> 2 files changed, 19 insertions(+), 27 deletions(-)
>>
>> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
>> index 4e7e958..a606fbb 100644
>> --- a/fs/lockd/mon.c
>> +++ b/fs/lockd/mon.c
>> @@ -18,8 +18,6 @@
>>
>> #define NLMDBG_FACILITY		NLMDBG_MONITOR
>>
>> -#define XDR_ADDRBUF_LEN		(20)
>> -
>> static struct rpc_clnt *	nsm_create(void);
>>
>> static struct rpc_program	nsm_program;
>> @@ -37,7 +35,13 @@ nsm_mon_unmon(struct nsm_handle *nsm, u32 proc,  
>> struct nsm_res *res)
>> {
>> 	struct rpc_clnt	*clnt;
>> 	int		status;
>> -	struct nsm_args	args;
>> +	struct nsm_args args = {
>> +		.addr		= nsm_addr_in(nsm)->sin_addr.s_addr,
>> +		.prog		= NLM_PROGRAM,
>> +		.vers		= 3,
>> +		.proc		= NLMPROC_NSM_NOTIFY,
>> +		.mon_name	= nsm->sm_mon_name,
>> +	};
>> 	struct rpc_message msg = {
>> 		.rpc_argp	= &args,
>> 		.rpc_resp	= res,
>> @@ -46,22 +50,18 @@ nsm_mon_unmon(struct nsm_handle *nsm, u32 proc,  
>> struct nsm_res *res)
>> 	clnt = nsm_create();
>> 	if (IS_ERR(clnt)) {
>> 		status = PTR_ERR(clnt);
>> +		dprintk("lockd: failed to create NSM upcall transport, "
>> +				"status=%d\n", status);
>> 		goto out;
>> 	}
>>
>> -	memset(&args, 0, sizeof(args));
>> -	args.mon_name = nsm->sm_name;
>> -	args.addr = nsm_addr_in(nsm)->sin_addr.s_addr;
>> -	args.prog = NLM_PROGRAM;
>> -	args.vers = 3;
>> -	args.proc = NLMPROC_NSM_NOTIFY;
>> 	memset(res, 0, sizeof(*res));
>>
>> 	msg.rpc_proc = &clnt->cl_procinfo[proc];
>> 	status = rpc_call_sync(clnt, &msg, 0);
>> 	if (status < 0)
>> -		printk(KERN_DEBUG "nsm_mon_unmon: rpc failed, status=%d\n",
>> -			status);
>> +		dprintk("lockd: NSM upcall RPC failed, status=%d\n",
>> +				status);
>> 	else
>> 		status = 0;
>> 	rpc_shutdown_client(clnt);
>> @@ -85,6 +85,12 @@ nsm_monitor(struct nlm_host *host)
>> 	if (nsm->sm_monitored)
>> 		return 0;
>>
>> +	/*
>> +	 * Choose whether to record the caller_name or IP address of
>> +	 * this peer in the local rpc.statd's database.
>> +	 */
>> +	nsm->sm_mon_name = nsm_use_hostnames ? nsm->sm_name : nsm- 
>> >sm_addrbuf;
>> +
>> 	status = nsm_mon_unmon(nsm, SM_MON, &res);
>>
>> 	if (status < 0 || res.status != 0)
>> @@ -165,25 +171,10 @@ static __be32 *xdr_encode_nsm_string(__be32  
>> *p, char *string)
>>
>> /*
>>  * "mon_name" specifies the host to be monitored.
>> - *
>> - * Linux uses a text version of the IP address of the remote
>> - * host as the host identifier (the "mon_name" argument).
>> - *
>> - * Linux statd always looks up the canonical hostname first for
>> - * whatever remote hostname it receives, so this works alright.
>>  */
>> static __be32 *xdr_encode_mon_name(__be32 *p, struct nsm_args *argp)
>> {
>> -	char	buffer[XDR_ADDRBUF_LEN + 1];
>> -	char	*name = argp->mon_name;
>> -
>> -	if (!nsm_use_hostnames) {
>> -		snprintf(buffer, XDR_ADDRBUF_LEN,
>> -			 NIPQUAD_FMT, NIPQUAD(argp->addr));
>> -		name = buffer;
>> -	}
>> -
>> -	return xdr_encode_nsm_string(p, name);
>> +	return xdr_encode_nsm_string(p, argp->mon_name);
>> }
>>
>> /*
>> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/ 
>> lockd.h
>> index 307a8f0..de9ea7b 100644
>> --- a/include/linux/lockd/lockd.h
>> +++ b/include/linux/lockd/lockd.h
>> @@ -73,6 +73,7 @@ struct nsm_handle {
>> 	char *			sm_name;
>> 	struct sockaddr_storage	sm_addr;
>> 	size_t			sm_addrlen;
>> +	char *			sm_mon_name;
>> 	unsigned int		sm_monitored : 1,
>> 				sm_sticky : 1;	/* don't unmonitor */
>> 	char			sm_addrbuf[63];	/* presentation address */
>>

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

Re: [PATCH 3/6] NSM: Support IPv6 version of mon_name

[Chuck Lever]

On Dec 3, 2008, at 4:19 PM, Chuck Lever wrote:

> On Dec 3, 2008, at 4:01 PM, J. Bruce Fields wrote:
>> On Mon, Dec 01, 2008 at 01:57:50PM -0500, Chuck Lever wrote:
>>> The "mon_name" argument of the NSMPROC_MON and NSMPROC_UNMON upcalls
>>> is a string that contains the hostname or IP address of the remote  
>>> peer
>>> to be notified when this host has rebooted.  The sm-notify command  
>>> uses
>>> this identifier to contact the peer when we reboot, so it must be
>>> either a well-qualified DNS hostname or a presentation format IP
>>> address string.
>>>
>>> When the "nsm_use_hostnames" sysctl is set to zero, the kernel's NSM
>>> provides a presentation format IP address in the "mon_name"  
>>> argument.
>>> Otherwise, the "caller_name" argument from NLM requests is used,
>>> which is usually just the DNS hostname of the peer.
>>>
>>> To support IPv6 addresses for the mon_name argument, we use the
>>> nsm_handle's address eye-catcher, which already contains an  
>>> appropriate
>>> presentation format address string.  Using the eye-catcher string
>>> obviates the need to use a large buffer on the stack to form the
>>> presentation address string for the upcall.
>>>
>>> This patch also addresses a subtle bug.
>>>
>>> An NSMPROC_MON request and the subsequent NSMPROC_UNMON request  
>>> for the
>>> same peer are required to use the same value for the "mon_name"
>>> argument.  Otherwise, rpc.statd's NSMPROC_UNMON processing cannot
>>> locate the database entry for that peer and remove it.
>>
>> At some point we need to grep for each read of nsm_use_hostnames and
>> think about what would happen if it changed there.
>>
>> For example, the check of nsm_use_hostnames when searching for a  
>> match
>> in nsm_find() could cause a spurious failure to find a host.  If the
>> nsm_find() came from nlm_host_rebooted(), we could fail to release  
>> locks
>> from some dead host.
>
> Agreed.  A subsequent patch in this series removes that particular  
> use of nsm_use_hostnames.  An audit of the other use(s) is a good  
> future work item.

I'll clarify: a subsequent patch in my for-2.6.29 series that I  
haven't posted yet removes that use of nsm_use_hostnames.

>> Probably we should just forbid changing nsm_use_hostnames while the
>> server is running or an nfs filesystem is mounted.  Or, if that's not
>> possible, allow changing the sysctl at any time, but only actually  
>> look
>> at it (and store it) once on server startup or first mount (whichever
>> comes first).
>
> Actually, "when lockd comes up" might be an appropriate moment to  
> copy the value of this variable.
>
>> As this requires a root user doing something wrong,
>
> Or a bug in rpc.statd or sm-notify...
>
>> fixing this bug
>> probably isn't high priority enough to block the rest of the ipv6
>> patches, so we could make a note of the problem and move on for now.
>>
>> --b.
>>
>>> If the setting of nsm_use_hostnames is changed between the time the
>>> kernel sends an NSMPROC_MON request and the time it sends the
>>> NSMPROC_UNMON request for the same peer, the "mon_name" argument for
>>> these two requests may not be the same.  This is because the value  
>>> of
>>> "mon_name" is currently chosen at the moment the call is made  
>>> based on
>>> the setting of nsm_use_hostnames
>>>
>>> To ensure both requests pass identical contents in the "mon_name"
>>> argument, we now select which string to use for the argument in the
>>> nsm_monitor() function.  A pointer to this string is saved in the
>>> nsm_handle so it can be used for the subsequent NSMPROC_UNMON  
>>> upcall.
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>
>>> fs/lockd/mon.c              |   45 ++++++++++++++++ 
>>> +--------------------------
>>> include/linux/lockd/lockd.h |    1 +
>>> 2 files changed, 19 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
>>> index 4e7e958..a606fbb 100644
>>> --- a/fs/lockd/mon.c
>>> +++ b/fs/lockd/mon.c
>>> @@ -18,8 +18,6 @@
>>>
>>> #define NLMDBG_FACILITY		NLMDBG_MONITOR
>>>
>>> -#define XDR_ADDRBUF_LEN		(20)
>>> -
>>> static struct rpc_clnt *	nsm_create(void);
>>>
>>> static struct rpc_program	nsm_program;
>>> @@ -37,7 +35,13 @@ nsm_mon_unmon(struct nsm_handle *nsm, u32 proc,  
>>> struct nsm_res *res)
>>> {
>>> 	struct rpc_clnt	*clnt;
>>> 	int		status;
>>> -	struct nsm_args	args;
>>> +	struct nsm_args args = {
>>> +		.addr		= nsm_addr_in(nsm)->sin_addr.s_addr,
>>> +		.prog		= NLM_PROGRAM,
>>> +		.vers		= 3,
>>> +		.proc		= NLMPROC_NSM_NOTIFY,
>>> +		.mon_name	= nsm->sm_mon_name,
>>> +	};
>>> 	struct rpc_message msg = {
>>> 		.rpc_argp	= &args,
>>> 		.rpc_resp	= res,
>>> @@ -46,22 +50,18 @@ nsm_mon_unmon(struct nsm_handle *nsm, u32  
>>> proc, struct nsm_res *res)
>>> 	clnt = nsm_create();
>>> 	if (IS_ERR(clnt)) {
>>> 		status = PTR_ERR(clnt);
>>> +		dprintk("lockd: failed to create NSM upcall transport, "
>>> +				"status=%d\n", status);
>>> 		goto out;
>>> 	}
>>>
>>> -	memset(&args, 0, sizeof(args));
>>> -	args.mon_name = nsm->sm_name;
>>> -	args.addr = nsm_addr_in(nsm)->sin_addr.s_addr;
>>> -	args.prog = NLM_PROGRAM;
>>> -	args.vers = 3;
>>> -	args.proc = NLMPROC_NSM_NOTIFY;
>>> 	memset(res, 0, sizeof(*res));
>>>
>>> 	msg.rpc_proc = &clnt->cl_procinfo[proc];
>>> 	status = rpc_call_sync(clnt, &msg, 0);
>>> 	if (status < 0)
>>> -		printk(KERN_DEBUG "nsm_mon_unmon: rpc failed, status=%d\n",
>>> -			status);
>>> +		dprintk("lockd: NSM upcall RPC failed, status=%d\n",
>>> +				status);
>>> 	else
>>> 		status = 0;
>>> 	rpc_shutdown_client(clnt);
>>> @@ -85,6 +85,12 @@ nsm_monitor(struct nlm_host *host)
>>> 	if (nsm->sm_monitored)
>>> 		return 0;
>>>
>>> +	/*
>>> +	 * Choose whether to record the caller_name or IP address of
>>> +	 * this peer in the local rpc.statd's database.
>>> +	 */
>>> +	nsm->sm_mon_name = nsm_use_hostnames ? nsm->sm_name : nsm- 
>>> >sm_addrbuf;
>>> +
>>> 	status = nsm_mon_unmon(nsm, SM_MON, &res);
>>>
>>> 	if (status < 0 || res.status != 0)
>>> @@ -165,25 +171,10 @@ static __be32 *xdr_encode_nsm_string(__be32  
>>> *p, char *string)
>>>
>>> /*
>>> * "mon_name" specifies the host to be monitored.
>>> - *
>>> - * Linux uses a text version of the IP address of the remote
>>> - * host as the host identifier (the "mon_name" argument).
>>> - *
>>> - * Linux statd always looks up the canonical hostname first for
>>> - * whatever remote hostname it receives, so this works alright.
>>> */
>>> static __be32 *xdr_encode_mon_name(__be32 *p, struct nsm_args *argp)
>>> {
>>> -	char	buffer[XDR_ADDRBUF_LEN + 1];
>>> -	char	*name = argp->mon_name;
>>> -
>>> -	if (!nsm_use_hostnames) {
>>> -		snprintf(buffer, XDR_ADDRBUF_LEN,
>>> -			 NIPQUAD_FMT, NIPQUAD(argp->addr));
>>> -		name = buffer;
>>> -	}
>>> -
>>> -	return xdr_encode_nsm_string(p, name);
>>> +	return xdr_encode_nsm_string(p, argp->mon_name);
>>> }
>>>
>>> /*
>>> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/ 
>>> lockd.h
>>> index 307a8f0..de9ea7b 100644
>>> --- a/include/linux/lockd/lockd.h
>>> +++ b/include/linux/lockd/lockd.h
>>> @@ -73,6 +73,7 @@ struct nsm_handle {
>>> 	char *			sm_name;
>>> 	struct sockaddr_storage	sm_addr;
>>> 	size_t			sm_addrlen;
>>> +	char *			sm_mon_name;
>>> 	unsigned int		sm_monitored : 1,
>>> 				sm_sticky : 1;	/* don't unmonitor */
>>> 	char			sm_addrbuf[63];	/* presentation address */
>>>
>
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> --
> 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

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

[PATCH 4/6] NLM: Clean up nsm_monitor() call

[Chuck Lever]

Clean up: A few minor clean-ups for nsm_monitor():

 o  Make sure to return an error if the SM_MON call result is not zero.

 o  Remove the BUG_ON() -- the code will die anyway if nsm is NULL.

 o  Use nsm->sm_name instead of host->h_name to be consistent with
    other functions in fs/lockd/mon.c.

 o  Collect the public declaration of nsm_monitor() in lockd.h with
    other NSM public function declarations (eg. nsm_release).

 o  Add documenting comments.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/mon.c                 |   29 ++++++++++++++++++-----------
 include/linux/lockd/lockd.h    |    4 ++++
 include/linux/lockd/sm_inter.h |    1 -
 3 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index a606fbb..78d5f59 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -69,18 +69,24 @@ nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res)
 	return status;
 }
 
-/*
- * Set up monitoring of a remote host
+/**
+ * nsm_monitor - Notify a peer in case we reboot
+ * @host: pointer to nlm_host of peer to notify
+ *
+ * If this peer is not already monitored, this function sends an
+ * upcall to the local rpc.statd to record the name/address of
+ * the peer to notify in case we reboot.
+ *
+ * Returns zero if the peer is monitored by the local rpc.statd;
+ * otherwise a negative errno value is returned.
  */
-int
-nsm_monitor(struct nlm_host *host)
+int nsm_monitor(const struct nlm_host *host)
 {
 	struct nsm_handle *nsm = host->h_nsmhandle;
-	struct nsm_res	res;
-	int		status;
+	struct nsm_res res;
+	int status;
 
-	dprintk("lockd: nsm_monitor(%s)\n", host->h_name);
-	BUG_ON(nsm == NULL);
+	dprintk("lockd: nsm_monitor(%s)\n", nsm->sm_name);
 
 	if (nsm->sm_monitored)
 		return 0;
@@ -92,9 +98,10 @@ nsm_monitor(struct nlm_host *host)
 	nsm->sm_mon_name = nsm_use_hostnames ? nsm->sm_name : nsm->sm_addrbuf;
 
 	status = nsm_mon_unmon(nsm, SM_MON, &res);
-
-	if (status < 0 || res.status != 0)
-		printk(KERN_NOTICE "lockd: cannot monitor %s\n", host->h_name);
+	if (res.status != 0)
+		status = -EIO;
+	if (status < 0)
+		printk(KERN_NOTICE "lockd: cannot monitor %s\n", nsm->sm_name);
 	else
 		nsm->sm_monitored = 1;
 	return status;
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index de9ea7b..4ca6f39 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -233,6 +233,10 @@ extern void	  nlm_host_rebooted(const struct sockaddr_in *, const char *,
 					unsigned int, u32);
 void		  nsm_release(struct nsm_handle *);
 
+/*
+ * Host monitoring
+ */
+int		  nsm_monitor(const struct nlm_host *host);
 
 /*
  * This is used in garbage collection and resource reclaim
diff --git a/include/linux/lockd/sm_inter.h b/include/linux/lockd/sm_inter.h
index 5a5448b..546b610 100644
--- a/include/linux/lockd/sm_inter.h
+++ b/include/linux/lockd/sm_inter.h
@@ -41,7 +41,6 @@ struct nsm_res {
 	u32		state;
 };
 
-int		nsm_monitor(struct nlm_host *);
 int		nsm_unmonitor(struct nlm_host *);
 extern int	nsm_local_state;
 

Re: [PATCH 4/6] NLM: Clean up nsm_monitor() call

[J. Bruce Fields]

On Mon, Dec 01, 2008 at 01:57:58PM -0500, Chuck Lever wrote:
> Clean up: A few minor clean-ups for nsm_monitor():

All looks fine, thanks, however:

> 
>  o  Make sure to return an error if the SM_MON call result is not zero.

I'd prefer to have a change in behavior split out into a separate patch
from pure cleanup.

--b.

> 
>  o  Remove the BUG_ON() -- the code will die anyway if nsm is NULL.
> 
>  o  Use nsm->sm_name instead of host->h_name to be consistent with
>     other functions in fs/lockd/mon.c.
> 
>  o  Collect the public declaration of nsm_monitor() in lockd.h with
>     other NSM public function declarations (eg. nsm_release).
> 
>  o  Add documenting comments.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/lockd/mon.c                 |   29 ++++++++++++++++++-----------
>  include/linux/lockd/lockd.h    |    4 ++++
>  include/linux/lockd/sm_inter.h |    1 -
>  3 files changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
> index a606fbb..78d5f59 100644
> --- a/fs/lockd/mon.c
> +++ b/fs/lockd/mon.c
> @@ -69,18 +69,24 @@ nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res)
>  	return status;
>  }
>  
> -/*
> - * Set up monitoring of a remote host
> +/**
> + * nsm_monitor - Notify a peer in case we reboot
> + * @host: pointer to nlm_host of peer to notify
> + *
> + * If this peer is not already monitored, this function sends an
> + * upcall to the local rpc.statd to record the name/address of
> + * the peer to notify in case we reboot.
> + *
> + * Returns zero if the peer is monitored by the local rpc.statd;
> + * otherwise a negative errno value is returned.
>   */
> -int
> -nsm_monitor(struct nlm_host *host)
> +int nsm_monitor(const struct nlm_host *host)
>  {
>  	struct nsm_handle *nsm = host->h_nsmhandle;
> -	struct nsm_res	res;
> -	int		status;
> +	struct nsm_res res;
> +	int status;
>  
> -	dprintk("lockd: nsm_monitor(%s)\n", host->h_name);
> -	BUG_ON(nsm == NULL);
> +	dprintk("lockd: nsm_monitor(%s)\n", nsm->sm_name);
>  
>  	if (nsm->sm_monitored)
>  		return 0;
> @@ -92,9 +98,10 @@ nsm_monitor(struct nlm_host *host)
>  	nsm->sm_mon_name = nsm_use_hostnames ? nsm->sm_name : nsm->sm_addrbuf;
>  
>  	status = nsm_mon_unmon(nsm, SM_MON, &res);
> -
> -	if (status < 0 || res.status != 0)
> -		printk(KERN_NOTICE "lockd: cannot monitor %s\n", host->h_name);
> +	if (res.status != 0)
> +		status = -EIO;
> +	if (status < 0)
> +		printk(KERN_NOTICE "lockd: cannot monitor %s\n", nsm->sm_name);
>  	else
>  		nsm->sm_monitored = 1;
>  	return status;
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index de9ea7b..4ca6f39 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -233,6 +233,10 @@ extern void	  nlm_host_rebooted(const struct sockaddr_in *, const char *,
>  					unsigned int, u32);
>  void		  nsm_release(struct nsm_handle *);
>  
> +/*
> + * Host monitoring
> + */
> +int		  nsm_monitor(const struct nlm_host *host);
>  
>  /*
>   * This is used in garbage collection and resource reclaim
> diff --git a/include/linux/lockd/sm_inter.h b/include/linux/lockd/sm_inter.h
> index 5a5448b..546b610 100644
> --- a/include/linux/lockd/sm_inter.h
> +++ b/include/linux/lockd/sm_inter.h
> @@ -41,7 +41,6 @@ struct nsm_res {
>  	u32		state;
>  };
>  
> -int		nsm_monitor(struct nlm_host *);
>  int		nsm_unmonitor(struct nlm_host *);
>  extern int	nsm_local_state;
>  
> 

Re: [PATCH 4/6] NLM: Clean up nsm_monitor() call

[Chuck Lever]

On Dec 3, 2008, at 4:08 PM, J. Bruce Fields wrote:

> On Mon, Dec 01, 2008 at 01:57:58PM -0500, Chuck Lever wrote:
>> Clean up: A few minor clean-ups for nsm_monitor():
>
> All looks fine, thanks, however:
>
>>
>> o  Make sure to return an error if the SM_MON call result is not  
>> zero.
>
> I'd prefer to have a change in behavior split out into a separate  
> patch
> from pure cleanup.

That's fine.  I will probably end up reposting this series with your  
comments addressed, so I will split this one out.

> --b.
>
>>
>> o  Remove the BUG_ON() -- the code will die anyway if nsm is NULL.
>>
>> o  Use nsm->sm_name instead of host->h_name to be consistent with
>>    other functions in fs/lockd/mon.c.
>>
>> o  Collect the public declaration of nsm_monitor() in lockd.h with
>>    other NSM public function declarations (eg. nsm_release).
>>
>> o  Add documenting comments.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> fs/lockd/mon.c                 |   29 ++++++++++++++++++-----------
>> include/linux/lockd/lockd.h    |    4 ++++
>> include/linux/lockd/sm_inter.h |    1 -
>> 3 files changed, 22 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
>> index a606fbb..78d5f59 100644
>> --- a/fs/lockd/mon.c
>> +++ b/fs/lockd/mon.c
>> @@ -69,18 +69,24 @@ nsm_mon_unmon(struct nsm_handle *nsm, u32 proc,  
>> struct nsm_res *res)
>> 	return status;
>> }
>>
>> -/*
>> - * Set up monitoring of a remote host
>> +/**
>> + * nsm_monitor - Notify a peer in case we reboot
>> + * @host: pointer to nlm_host of peer to notify
>> + *
>> + * If this peer is not already monitored, this function sends an
>> + * upcall to the local rpc.statd to record the name/address of
>> + * the peer to notify in case we reboot.
>> + *
>> + * Returns zero if the peer is monitored by the local rpc.statd;
>> + * otherwise a negative errno value is returned.
>>  */
>> -int
>> -nsm_monitor(struct nlm_host *host)
>> +int nsm_monitor(const struct nlm_host *host)
>> {
>> 	struct nsm_handle *nsm = host->h_nsmhandle;
>> -	struct nsm_res	res;
>> -	int		status;
>> +	struct nsm_res res;
>> +	int status;
>>
>> -	dprintk("lockd: nsm_monitor(%s)\n", host->h_name);
>> -	BUG_ON(nsm == NULL);
>> +	dprintk("lockd: nsm_monitor(%s)\n", nsm->sm_name);
>>
>> 	if (nsm->sm_monitored)
>> 		return 0;
>> @@ -92,9 +98,10 @@ nsm_monitor(struct nlm_host *host)
>> 	nsm->sm_mon_name = nsm_use_hostnames ? nsm->sm_name : nsm- 
>> >sm_addrbuf;
>>
>> 	status = nsm_mon_unmon(nsm, SM_MON, &res);
>> -
>> -	if (status < 0 || res.status != 0)
>> -		printk(KERN_NOTICE "lockd: cannot monitor %s\n", host->h_name);
>> +	if (res.status != 0)
>> +		status = -EIO;
>> +	if (status < 0)
>> +		printk(KERN_NOTICE "lockd: cannot monitor %s\n", nsm->sm_name);
>> 	else
>> 		nsm->sm_monitored = 1;
>> 	return status;
>> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/ 
>> lockd.h
>> index de9ea7b..4ca6f39 100644
>> --- a/include/linux/lockd/lockd.h
>> +++ b/include/linux/lockd/lockd.h
>> @@ -233,6 +233,10 @@ extern void	  nlm_host_rebooted(const struct  
>> sockaddr_in *, const char *,
>> 					unsigned int, u32);
>> void		  nsm_release(struct nsm_handle *);
>>
>> +/*
>> + * Host monitoring
>> + */
>> +int		  nsm_monitor(const struct nlm_host *host);
>>
>> /*
>>  * This is used in garbage collection and resource reclaim
>> diff --git a/include/linux/lockd/sm_inter.h b/include/linux/lockd/ 
>> sm_inter.h
>> index 5a5448b..546b610 100644
>> --- a/include/linux/lockd/sm_inter.h
>> +++ b/include/linux/lockd/sm_inter.h
>> @@ -41,7 +41,6 @@ struct nsm_res {
>> 	u32		state;
>> };
>>
>> -int		nsm_monitor(struct nlm_host *);
>> int		nsm_unmonitor(struct nlm_host *);
>> extern int	nsm_local_state;
>>
>>

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

[PATCH 5/6] NLM: Clean up nsm_monitor() call

[Chuck Lever]

Clean up: A few minor clean-ups for nsm_unmonitor():

 o  Arrange dprintk and check of sm_monitored consistently with
    nsm_monitor().

 o  nsm_unmonitor()'s only caller doesn't care about the return code,
    so eliminate it (thanks, Bruce).

 o  The nsm_handle's reference count is bumped in nlm_lookup_host()
    so it should be decremented in nlm_destroy_host() to make it
    easier to see the balance of these two operations.  For the
    moment we leave nsm_release() public.

 o  Use nsm->sm_name instead of host->h_name to be consistent with
    other functions in fs/lockd/mon.c.

 o  Collect the public declaration of nsm_unmonitor() in lockd.h with
    other NSM public function declarations.

 o  Add documenting comments.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/host.c                |    7 +++----
 fs/lockd/mon.c                 |   33 +++++++++++++++------------------
 include/linux/lockd/lockd.h    |    1 +
 include/linux/lockd/sm_inter.h |    1 -
 4 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index cd7edc3..8d26d0c 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -263,10 +263,9 @@ nlm_destroy_host(struct nlm_host *host)
 	BUG_ON(!list_empty(&host->h_lockowners));
 	BUG_ON(atomic_read(&host->h_count));
 
-	/*
-	 * Release NSM handle and unmonitor host.
-	 */
-	nsm_unmonitor(host);
+	nsm_unmonitor(host->h_nsmhandle);
+	nsm_release(host->h_nsmhandle);
+	host->h_nsmhandle = NULL;
 
 	clnt = host->h_rpcclnt;
 	if (clnt != NULL)
diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 78d5f59..b76d9b2 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -107,33 +107,30 @@ int nsm_monitor(const struct nlm_host *host)
 	return status;
 }
 
-/*
- * Cease to monitor remote host
+/**
+ * nsm_unmonitor - Unregister peer notification
+ * @nsm: pointer to nsm_handle of peer to stop monitoring
+ *
+ * If this peer is monitored, this function sends an upcall to
+ * tell the local rpc.statd not to send this peer a notification
+ * when we reboot.
  */
-int
-nsm_unmonitor(struct nlm_host *host)
+void nsm_unmonitor(struct nsm_handle *nsm)
 {
-	struct nsm_handle *nsm = host->h_nsmhandle;
-	struct nsm_res	res;
-	int		status = 0;
+	struct nsm_res res;
 
-	if (nsm == NULL)
-		return 0;
-	host->h_nsmhandle = NULL;
+	dprintk("lockd: nsm_unmonitor(%s)\n", nsm->sm_name);
 
-	if (atomic_read(&nsm->sm_count) == 1
-	 && nsm->sm_monitored && !nsm->sm_sticky) {
-		dprintk("lockd: nsm_unmonitor(%s)\n", host->h_name);
+	if (!nsm->sm_monitored)
+		return;
 
-		status = nsm_mon_unmon(nsm, SM_UNMON, &res);
-		if (status < 0)
+	if (atomic_read(&nsm->sm_count) == 1 && !nsm->sm_sticky) {
+		if (nsm_mon_unmon(nsm, SM_UNMON, &res) < 0)
 			printk(KERN_NOTICE "lockd: cannot unmonitor %s\n",
-					host->h_name);
+					nsm->sm_name);
 		else
 			nsm->sm_monitored = 0;
 	}
-	nsm_release(nsm);
-	return status;
 }
 
 /*
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 4ca6f39..e2fa968 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -237,6 +237,7 @@ void		  nsm_release(struct nsm_handle *);
  * Host monitoring
  */
 int		  nsm_monitor(const struct nlm_host *host);
+void		  nsm_unmonitor(struct nsm_handle *nsm);
 
 /*
  * This is used in garbage collection and resource reclaim
diff --git a/include/linux/lockd/sm_inter.h b/include/linux/lockd/sm_inter.h
index 546b610..896a5e3 100644
--- a/include/linux/lockd/sm_inter.h
+++ b/include/linux/lockd/sm_inter.h
@@ -41,7 +41,6 @@ struct nsm_res {
 	u32		state;
 };
 
-int		nsm_unmonitor(struct nlm_host *);
 extern int	nsm_local_state;
 
 #endif /* LINUX_LOCKD_SM_INTER_H */

Re: [PATCH 5/6] NLM: Clean up nsm_monitor() call

[J. Bruce Fields]

On Mon, Dec 01, 2008 at 01:58:05PM -0500, Chuck Lever wrote:
> Clean up: A few minor clean-ups for nsm_unmonitor():

Looks fine, but might prefer this slightly finer-grained:

> 
>  o  Arrange dprintk and check of sm_monitored consistently with
>     nsm_monitor().
> 
>  o  nsm_unmonitor()'s only caller doesn't care about the return code,
>     so eliminate it (thanks, Bruce).
> 
>  o  The nsm_handle's reference count is bumped in nlm_lookup_host()
>     so it should be decremented in nlm_destroy_host() to make it
>     easier to see the balance of these two operations.  For the
>     moment we leave nsm_release() public.

That might be a candidate to split out as a separate patch.

> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
> index 78d5f59..b76d9b2 100644
> --- a/fs/lockd/mon.c
> +++ b/fs/lockd/mon.c
> @@ -107,33 +107,30 @@ int nsm_monitor(const struct nlm_host *host)
>  	return status;
>  }
>  
> -/*
> - * Cease to monitor remote host
> +/**
> + * nsm_unmonitor - Unregister peer notification
> + * @nsm: pointer to nsm_handle of peer to stop monitoring
> + *
> + * If this peer is monitored, this function sends an upcall to
> + * tell the local rpc.statd not to send this peer a notification
> + * when we reboot.
>   */
> -int
> -nsm_unmonitor(struct nlm_host *host)
> +void nsm_unmonitor(struct nsm_handle *nsm)
>  {
> -	struct nsm_handle *nsm = host->h_nsmhandle;
> -	struct nsm_res	res;
> -	int		status = 0;
> +	struct nsm_res res;
>  
> -	if (nsm == NULL)
> -		return 0;

Hm.  Yeah, can't see how it could ever have been called with NULL--might
be worth a changelog comment, though.

--b.

> -	host->h_nsmhandle = NULL;
> +	dprintk("lockd: nsm_unmonitor(%s)\n", nsm->sm_name);
>  
> -	if (atomic_read(&nsm->sm_count) == 1
> -	 && nsm->sm_monitored && !nsm->sm_sticky) {
> -		dprintk("lockd: nsm_unmonitor(%s)\n", host->h_name);
> +	if (!nsm->sm_monitored)
> +		return;
>  
> -		status = nsm_mon_unmon(nsm, SM_UNMON, &res);
> -		if (status < 0)
> +	if (atomic_read(&nsm->sm_count) == 1 && !nsm->sm_sticky) {
> +		if (nsm_mon_unmon(nsm, SM_UNMON, &res) < 0)
>  			printk(KERN_NOTICE "lockd: cannot unmonitor %s\n",
> -					host->h_name);
> +					nsm->sm_name);
>  		else
>  			nsm->sm_monitored = 0;
>  	}
> -	nsm_release(nsm);
> -	return status;
>  }
>  
>  /*
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index 4ca6f39..e2fa968 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -237,6 +237,7 @@ void		  nsm_release(struct nsm_handle *);
>   * Host monitoring
>   */
>  int		  nsm_monitor(const struct nlm_host *host);
> +void		  nsm_unmonitor(struct nsm_handle *nsm);
>  
>  /*
>   * This is used in garbage collection and resource reclaim
> diff --git a/include/linux/lockd/sm_inter.h b/include/linux/lockd/sm_inter.h
> index 546b610..896a5e3 100644
> --- a/include/linux/lockd/sm_inter.h
> +++ b/include/linux/lockd/sm_inter.h
> @@ -41,7 +41,6 @@ struct nsm_res {
>  	u32		state;
>  };
>  
> -int		nsm_unmonitor(struct nlm_host *);
>  extern int	nsm_local_state;
>  
>  #endif /* LINUX_LOCKD_SM_INTER_H */
> 

[PATCH 6/6] NSM: Serialize SM_MON and SM_UNMON upcalls

[Chuck Lever]

The Linux NSM implementation currently does nothing to prevent
concurrent SM_MON and SM_UNMON upcalls regarding the same peer.  Races
could cause duplicate SM_MON upcalls for the same peer, or locking
activity during server shutdown might cause SM_MON and SM_UNMON upcalls
to occur in an arbitrary order.

To avoid confusing rpc.statd, add a per-nsm_handle mutex to serialize
the upcalls.  To prevent duplicate upcalls, the mutex is held while the
upcall is outstanding, and is not released until we can tell if the
upcall succeeded.

The nsm_monitor() function is called frequently.  The extra inline
function mitigates the overhead of taking the mutex.  It is only taken
if the nsm_handle is not already monitored.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/host.c             |    1 +
 fs/lockd/mon.c              |   23 ++++++++++++++++-------
 include/linux/lockd/lockd.h |   12 +++++++++++-
 3 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index 8d26d0c..bf705d0 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -697,6 +697,7 @@ retry:
 	nsm->sm_name = (char *) (nsm + 1);
 	memcpy(nsm->sm_name, hostname, hostname_len);
 	nsm->sm_name[hostname_len] = '\0';
+	mutex_init(&nsm->sm_mutex);
 	nlm_display_address((struct sockaddr *)&nsm->sm_addr,
 				nsm->sm_addrbuf, sizeof(nsm->sm_addrbuf));
 	atomic_set(&nsm->sm_count, 1);
diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index b76d9b2..301aeb7 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -70,8 +70,8 @@ nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res)
 }
 
 /**
- * nsm_monitor - Notify a peer in case we reboot
- * @host: pointer to nlm_host of peer to notify
+ * __nsm_monitor - Notify a peer in case we reboot
+ * @nsm: pointer to nsm_handle of peer to notify
  *
  * If this peer is not already monitored, this function sends an
  * upcall to the local rpc.statd to record the name/address of
@@ -80,16 +80,17 @@ nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res)
  * Returns zero if the peer is monitored by the local rpc.statd;
  * otherwise a negative errno value is returned.
  */
-int nsm_monitor(const struct nlm_host *host)
+int __nsm_monitor(struct nsm_handle *nsm)
 {
-	struct nsm_handle *nsm = host->h_nsmhandle;
 	struct nsm_res res;
-	int status;
+	int status = 0;
 
 	dprintk("lockd: nsm_monitor(%s)\n", nsm->sm_name);
 
+	mutex_lock(&nsm->sm_mutex);
+
 	if (nsm->sm_monitored)
-		return 0;
+		goto out;
 
 	/*
 	 * Choose whether to record the caller_name or IP address of
@@ -104,6 +105,9 @@ int nsm_monitor(const struct nlm_host *host)
 		printk(KERN_NOTICE "lockd: cannot monitor %s\n", nsm->sm_name);
 	else
 		nsm->sm_monitored = 1;
+
+out:
+	mutex_unlock(&nsm->sm_mutex);
 	return status;
 }
 
@@ -121,8 +125,10 @@ void nsm_unmonitor(struct nsm_handle *nsm)
 
 	dprintk("lockd: nsm_unmonitor(%s)\n", nsm->sm_name);
 
+	mutex_lock(&nsm->sm_mutex);
+
 	if (!nsm->sm_monitored)
-		return;
+		goto out;
 
 	if (atomic_read(&nsm->sm_count) == 1 && !nsm->sm_sticky) {
 		if (nsm_mon_unmon(nsm, SM_UNMON, &res) < 0)
@@ -131,6 +137,9 @@ void nsm_unmonitor(struct nsm_handle *nsm)
 		else
 			nsm->sm_monitored = 0;
 	}
+
+out:
+	mutex_unlock(&nsm->sm_mutex);
 }
 
 /*
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index e2fa968..bf6c1c1 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -76,6 +76,7 @@ struct nsm_handle {
 	char *			sm_mon_name;
 	unsigned int		sm_monitored : 1,
 				sm_sticky : 1;	/* don't unmonitor */
+	struct mutex		sm_mutex;
 	char			sm_addrbuf[63];	/* presentation address */
 };
 
@@ -236,9 +237,18 @@ void		  nsm_release(struct nsm_handle *);
 /*
  * Host monitoring
  */
-int		  nsm_monitor(const struct nlm_host *host);
+int		  __nsm_monitor(struct nsm_handle *nsm);
 void		  nsm_unmonitor(struct nsm_handle *nsm);
 
+static inline int nsm_monitor(const struct nlm_host *host)
+{
+	struct nsm_handle *nsm = host->h_nsmhandle;
+
+	if (likely(nsm->sm_monitored))
+		return 0;
+	return __nsm_monitor(nsm);
+}
+
 /*
  * This is used in garbage collection and resource reclaim
  * A return value != 0 means destroy the lock/block/share

Re: [PATCH 6/6] NSM: Serialize SM_MON and SM_UNMON upcalls

[J. Bruce Fields]

On Mon, Dec 01, 2008 at 01:58:13PM -0500, Chuck Lever wrote:
> The Linux NSM implementation currently does nothing to prevent
> concurrent SM_MON and SM_UNMON upcalls regarding the same peer.  Races
> could cause duplicate SM_MON upcalls for the same peer, or locking
> activity during server shutdown might cause SM_MON and SM_UNMON upcalls
> to occur in an arbitrary order.
> 
> To avoid confusing rpc.statd, add a per-nsm_handle mutex to serialize
> the upcalls.  To prevent duplicate upcalls, the mutex is held while the
> upcall is outstanding, and is not released until we can tell if the
> upcall succeeded.
> 
> The nsm_monitor() function is called frequently.  The extra inline
> function mitigates the overhead of taking the mutex.  It is only taken
> if the nsm_handle is not already monitored.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
...
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index e2fa968..bf6c1c1 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -76,6 +76,7 @@ struct nsm_handle {
>  	char *			sm_mon_name;
>  	unsigned int		sm_monitored : 1,
>  				sm_sticky : 1;	/* don't unmonitor */
> +	struct mutex		sm_mutex;
>  	char			sm_addrbuf[63];	/* presentation address */
>  };
>  
> @@ -236,9 +237,18 @@ void		  nsm_release(struct nsm_handle *);
>  /*
>   * Host monitoring
>   */
> -int		  nsm_monitor(const struct nlm_host *host);
> +int		  __nsm_monitor(struct nsm_handle *nsm);
>  void		  nsm_unmonitor(struct nsm_handle *nsm);
>  
> +static inline int nsm_monitor(const struct nlm_host *host)
> +{
> +	struct nsm_handle *nsm = host->h_nsmhandle;
> +
> +	if (likely(nsm->sm_monitored))
> +		return 0;

We also need to grep for places where sm_monitored is cleared, and
think about what happens in a race between one of those cases and
__nsm_monitor().

--b.

> +	return __nsm_monitor(nsm);
> +}
> +
>  /*
>   * This is used in garbage collection and resource reclaim
>   * A return value != 0 means destroy the lock/block/share
> 

Re: [PATCH 6/6] NSM: Serialize SM_MON and SM_UNMON upcalls

[Chuck Lever]

On Dec 3, 2008, at Dec 3, 2008, 4:41 PM, J. Bruce Fields wrote:
> On Mon, Dec 01, 2008 at 01:58:13PM -0500, Chuck Lever wrote:
>> The Linux NSM implementation currently does nothing to prevent
>> concurrent SM_MON and SM_UNMON upcalls regarding the same peer.   
>> Races
>> could cause duplicate SM_MON upcalls for the same peer, or locking
>> activity during server shutdown might cause SM_MON and SM_UNMON  
>> upcalls
>> to occur in an arbitrary order.
>>
>> To avoid confusing rpc.statd, add a per-nsm_handle mutex to serialize
>> the upcalls.  To prevent duplicate upcalls, the mutex is held while  
>> the
>> upcall is outstanding, and is not released until we can tell if the
>> upcall succeeded.
>>
>> The nsm_monitor() function is called frequently.  The extra inline
>> function mitigates the overhead of taking the mutex.  It is only  
>> taken
>> if the nsm_handle is not already monitored.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ...
>> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/ 
>> lockd.h
>> index e2fa968..bf6c1c1 100644
>> --- a/include/linux/lockd/lockd.h
>> +++ b/include/linux/lockd/lockd.h
>> @@ -76,6 +76,7 @@ struct nsm_handle {
>> 	char *			sm_mon_name;
>> 	unsigned int		sm_monitored : 1,
>> 				sm_sticky : 1;	/* don't unmonitor */
>> +	struct mutex		sm_mutex;
>> 	char			sm_addrbuf[63];	/* presentation address */
>> };
>>
>> @@ -236,9 +237,18 @@ void		  nsm_release(struct nsm_handle *);
>> /*
>>  * Host monitoring
>>  */
>> -int		  nsm_monitor(const struct nlm_host *host);
>> +int		  __nsm_monitor(struct nsm_handle *nsm);
>> void		  nsm_unmonitor(struct nsm_handle *nsm);
>>
>> +static inline int nsm_monitor(const struct nlm_host *host)
>> +{
>> +	struct nsm_handle *nsm = host->h_nsmhandle;
>> +
>> +	if (likely(nsm->sm_monitored))
>> +		return 0;
>
> We also need to grep for places where sm_monitored is cleared, and
> think about what happens in a race between one of those cases and
> __nsm_monitor().

It's cleared in nlm_host_rebooted(), and in nsm_unmonitor().

The nlm_host_mutex already provides much the same serialization that  
is added in this patch, and at least on the server, nlm_lookup_host()  
and nlm_destroy_host() both run under the BKL.

The problem (if there is one) is really in nlm_host_rebooted(), which  
this patch does nothing about.

Let's drop this one for now.  I think there is a way to get rpc.statd  
to report duplicates instead of ignoring them as it does now.  We  
should get an idea of how pervasive this problem really is.

> --b.
>
>> +	return __nsm_monitor(nsm);
>> +}
>> +
>> /*
>>  * This is used in garbage collection and resource reclaim
>>  * A return value != 0 means destroy the lock/block/share
>>

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

Re: [PATCH 6/6] NSM: Serialize SM_MON and SM_UNMON upcalls

[J. Bruce Fields]

On Wed, Dec 03, 2008 at 07:38:07PM -0500, Chuck Lever wrote:
> It's cleared in nlm_host_rebooted(), and in nsm_unmonitor().
>
> The nlm_host_mutex already provides much the same serialization that is 
> added in this patch, and at least on the server, nlm_lookup_host() and 
> nlm_destroy_host() both run under the BKL.
>
> The problem (if there is one) is really in nlm_host_rebooted(), which  
> this patch does nothing about.
>
> Let's drop this one for now.  I think there is a way to get rpc.statd to 
> report duplicates instead of ignoring them as it does now.  We should get 
> an idea of how pervasive this problem really is.

OK, fair enough.

I think it's a good catch, though, and one other motivation is that
*someday* (after fixing a bunch of similar locking problems) it would be
convenient if lockd could be made multithreaded.  And if we haven't seen
this particular problem in practice, part of the reason may be that
lockd is single-threaded and entirely under the BKL.

--b.