[PATCH] nfsd: use nfs client rpc callback program

[PATCH] nfsd: use nfs client rpc callback program

[Benny Halevy]

From: Benny Halevy <bhalevy@panasas.com>

since commit ff7d9756b501744540be65e172d27ee321d86103
"nfsd: use static memory for callback program and stats"
do_probe_callback uses a static callback program
(NFS4_CALLBACK) rather than the one set in clp->cl_callback.cb_prog
as passed in by the client in setclientid (4.0)
or create_session (4.1).

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4callback.c |   31 ++++++++++++++++---------------
 1 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 30d3130..f555178 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -342,21 +342,6 @@ static struct rpc_version *	nfs_cb_version[] = {
 	&nfs_cb_version4,
 };
 
-static struct rpc_program cb_program;
-
-static struct rpc_stat cb_stats = {
-		.program	= &cb_program
-};
-
-#define NFS4_CALLBACK 0x40000000
-static struct rpc_program cb_program = {
-		.name 		= "nfs4_cb",
-		.number		= NFS4_CALLBACK,
-		.nrvers		= ARRAY_SIZE(nfs_cb_version),
-		.version	= nfs_cb_version,
-		.stats		= &cb_stats,
-};
-
 /* Reference counting, callback cleanup, etc., all look racy as heck.
  * And why is cb_set an atomic? */
 
@@ -371,6 +356,8 @@ static int do_probe_callback(void *data)
 		.to_maxval	= (NFSD_LEASE_TIME/2) * HZ,
 		.to_exponential	= 1,
 	};
+	static struct rpc_stat	cb_stats;
+	struct rpc_program	cb_program;
 	struct rpc_create_args args = {
 		.protocol	= IPPROTO_TCP,
 		.address	= (struct sockaddr *)&addr,
@@ -394,6 +381,20 @@ static int do_probe_callback(void *data)
 	addr.sin_port = htons(cb->cb_port);
 	addr.sin_addr.s_addr = htonl(cb->cb_addr);
 
+	/* Initialize rpc_program */
+	memset(&cb_program, 0, sizeof(cb_program));
+	cb_program.name = "nfs4_cb";
+	cb_program.number = clp->cl_callback.cb_prog;
+	cb_program.nrvers = ARRAY_SIZE(nfs_cb_version);
+	cb_program.version = nfs_cb_version;
+	cb_program.stats = &cb_stats;
+	memset(&cb_stats, 0, sizeof(cb_stats));
+	cb_stats.program = &cb_program;
+
+	dprintk("%s: program %s 0x%x nrvers %u version %u\n",
+		__func__, args.program->name, args.program->number,
+		args.program->nrvers, args.version);
+
 	/* Initialize rpc_stat */
 	memset(args.program->stats, 0, sizeof(struct rpc_stat));
 
-- 
1.6.0.1

Re: [PATCH] nfsd: use nfs client rpc callback program

[J. Bruce Fields]

On Wed, Sep 17, 2008 at 02:43:44PM -0500, Benny Halevy wrote:
> From: Benny Halevy <bhalevy@panasas.com>
> 
> since commit ff7d9756b501744540be65e172d27ee321d86103
> "nfsd: use static memory for callback program and stats"
> do_probe_callback uses a static callback program
> (NFS4_CALLBACK) rather than the one set in clp->cl_callback.cb_prog
> as passed in by the client in setclientid (4.0)
> or create_session (4.1).

Ugh, yes, sorry about that.  (I wonder why pynfs testing didn't catch
this?  Oh, I guess it's because NFS4_CALLBACK is the program number our
client always gives us.)

> @@ -371,6 +356,8 @@ static int do_probe_callback(void *data)
>  		.to_maxval	= (NFSD_LEASE_TIME/2) * HZ,
>  		.to_exponential	= 1,
>  	};
> +	static struct rpc_stat	cb_stats;
> +	struct rpc_program	cb_program;
>  	struct rpc_create_args args = {
>  		.protocol	= IPPROTO_TCP,
>  		.address	= (struct sockaddr *)&addr,
> @@ -394,6 +381,20 @@ static int do_probe_callback(void *data)
>  	addr.sin_port = htons(cb->cb_port);
>  	addr.sin_addr.s_addr = htonl(cb->cb_addr);
>  
> +	/* Initialize rpc_program */
> +	memset(&cb_program, 0, sizeof(cb_program));
> +	cb_program.name = "nfs4_cb";
> +	cb_program.number = clp->cl_callback.cb_prog;
> +	cb_program.nrvers = ARRAY_SIZE(nfs_cb_version);
> +	cb_program.version = nfs_cb_version;
> +	cb_program.stats = &cb_stats;
> +	memset(&cb_stats, 0, sizeof(cb_stats));
> +	cb_stats.program = &cb_program;

You don't want a pointer to data on the stack here, do you?

--b.

Re: [PATCH] nfsd: use nfs client rpc callback program

[Benny Halevy]

On Sep. 17, 2008, 18:10 -0500, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> On Wed, Sep 17, 2008 at 02:43:44PM -0500, Benny Halevy wrote:
>> From: Benny Halevy <bhalevy@panasas.com>
>>
>> since commit ff7d9756b501744540be65e172d27ee321d86103
>> "nfsd: use static memory for callback program and stats"
>> do_probe_callback uses a static callback program
>> (NFS4_CALLBACK) rather than the one set in clp->cl_callback.cb_prog
>> as passed in by the client in setclientid (4.0)
>> or create_session (4.1).
> 
> Ugh, yes, sorry about that.  (I wonder why pynfs testing didn't catch
> this?  Oh, I guess it's because NFS4_CALLBACK is the program number our
> client always gives us.)

Well, Fred (thanks!) added a test today which uses a non-default
callback program and he sees a corresponding callback coming back.

(Note that this test is not absolutely generic as the server is
not required to probe the callback immediately, or at all, after
setclientid or create_session.)

> 
>> @@ -371,6 +356,8 @@ static int do_probe_callback(void *data)
>>  		.to_maxval	= (NFSD_LEASE_TIME/2) * HZ,
>>  		.to_exponential	= 1,
>>  	};
>> +	static struct rpc_stat	cb_stats;
>> +	struct rpc_program	cb_program;
>>  	struct rpc_create_args args = {
>>  		.protocol	= IPPROTO_TCP,
>>  		.address	= (struct sockaddr *)&addr,
>> @@ -394,6 +381,20 @@ static int do_probe_callback(void *data)
>>  	addr.sin_port = htons(cb->cb_port);
>>  	addr.sin_addr.s_addr = htonl(cb->cb_addr);
>>  
>> +	/* Initialize rpc_program */
>> +	memset(&cb_program, 0, sizeof(cb_program));
>> +	cb_program.name = "nfs4_cb";
>> +	cb_program.number = clp->cl_callback.cb_prog;
>> +	cb_program.nrvers = ARRAY_SIZE(nfs_cb_version);
>> +	cb_program.version = nfs_cb_version;
>> +	cb_program.stats = &cb_stats;
>> +	memset(&cb_stats, 0, sizeof(cb_stats));
>> +	cb_stats.program = &cb_program;
> 
> You don't want a pointer to data on the stack here, do you?

Hmm, you're right...
I went back and forth whether this should be allocated statically,
dynamically, or on the stack.  I was mislead by the fact we're doing
a sync rpc call, but indeed this needs to live until the nfs client
is destroyed.  I'm trying to fully understand what Olga saw
before coming up with a new proposal... maybe putting the cb_program
back into struct nfs4_callback and just make cb_stats static would
provide a solution of the problem Olga witnessed and keep everybody
happy.

Benny

> 
> --b.

Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

[Benny Halevy]

On Sep. 17, 2008, 18:34 -0500, Benny Halevy <bhalevy@panasas.com> wrote:
> On Sep. 17, 2008, 18:10 -0500, "J. Bruce Fields" <bfields@fieldses.org> wrote:
>> On Wed, Sep 17, 2008 at 02:43:44PM -0500, Benny Halevy wrote:
>>> From: Benny Halevy <bhalevy@panasas.com>
>>>
>>> since commit ff7d9756b501744540be65e172d27ee321d86103
>>> "nfsd: use static memory for callback program and stats"
>>> do_probe_callback uses a static callback program
>>> (NFS4_CALLBACK) rather than the one set in clp->cl_callback.cb_prog
>>> as passed in by the client in setclientid (4.0)
>>> or create_session (4.1).
>> Ugh, yes, sorry about that.  (I wonder why pynfs testing didn't catch
>> this?  Oh, I guess it's because NFS4_CALLBACK is the program number our
>> client always gives us.)
> 
> Well, Fred (thanks!) added a test today which uses a non-default
> callback program and he sees a corresponding callback coming back.
> 
> (Note that this test is not absolutely generic as the server is
> not required to probe the callback immediately, or at all, after
> setclientid or create_session.)
> 
>>> @@ -371,6 +356,8 @@ static int do_probe_callback(void *data)
>>>  		.to_maxval	= (NFSD_LEASE_TIME/2) * HZ,
>>>  		.to_exponential	= 1,
>>>  	};
>>> +	static struct rpc_stat	cb_stats;
>>> +	struct rpc_program	cb_program;
>>>  	struct rpc_create_args args = {
>>>  		.protocol	= IPPROTO_TCP,
>>>  		.address	= (struct sockaddr *)&addr,
>>> @@ -394,6 +381,20 @@ static int do_probe_callback(void *data)
>>>  	addr.sin_port = htons(cb->cb_port);
>>>  	addr.sin_addr.s_addr = htonl(cb->cb_addr);
>>>  
>>> +	/* Initialize rpc_program */
>>> +	memset(&cb_program, 0, sizeof(cb_program));
>>> +	cb_program.name = "nfs4_cb";
>>> +	cb_program.number = clp->cl_callback.cb_prog;
>>> +	cb_program.nrvers = ARRAY_SIZE(nfs_cb_version);
>>> +	cb_program.version = nfs_cb_version;
>>> +	cb_program.stats = &cb_stats;
>>> +	memset(&cb_stats, 0, sizeof(cb_stats));
>>> +	cb_stats.program = &cb_program;
>> You don't want a pointer to data on the stack here, do you?
> 
> Hmm, you're right...
> I went back and forth whether this should be allocated statically,
> dynamically, or on the stack.  I was mislead by the fact we're doing
> a sync rpc call, but indeed this needs to live until the nfs client
> is destroyed.  I'm trying to fully understand what Olga saw
> before coming up with a new proposal... maybe putting the cb_program
> back into struct nfs4_callback and just make cb_stats static would
> provide a solution of the problem Olga witnessed and keep everybody
> happy.

It looks like the gss_cred references the rpc_client and that is used
in gss_destroying_context.  However, I couldn't find a call to 
rpc_release_client from the gss_cred destruction path.

Shouldn't gss_create formally kref the clnt and gss_free_cred
call rpc_release_client(gss_cred->client)?

If so, I think we could teach the rpc_client to allocate the
program and stats dynamically and let rpc_free_client() to free them
along with the rpc_client.

Benny

> 
> Benny
> 
>> --b.
> 
> _______________________________________________
> pNFS mailing list
> pNFS@linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs

Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

[Benny Halevy]

From: Benny Halevy <bhalevy@panasas.com>

since commit ff7d9756b501744540be65e172d27ee321d86103
"nfsd: use static memory for callback program and stats"
do_probe_callback uses a static callback program
(NFS4_CALLBACK) rather than the one set in clp->cl_callback.cb_prog
as passed in by the client in setclientid (4.0)
or create_session (4.1).

This patches allows allocating cb_program (and cb_stats) dynamically
and setting a free_rpc_program function pointer to be
called when the rpc_clnt structure is destroyed.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---

Bruce, how about the following patch instead.
It should solve the callback program number as well as Olga
gss_auth issue.

I'm still not sure about the gss_auth reference accounting
for the rpc_clnt it references, but that's somewhat orthogonal
to the solution in this patch.

Benny

 fs/nfsd/nfs4callback.c      |   47 ++++++++++++++++++++++++++++--------------
 include/linux/sunrpc/clnt.h |    1 +
 net/sunrpc/clnt.c           |    2 +
 3 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 30d3130..6599b1e 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -342,20 +342,11 @@ static struct rpc_version *	nfs_cb_version[] = {
 	&nfs_cb_version4,
 };
 
-static struct rpc_program cb_program;
-
-static struct rpc_stat cb_stats = {
-		.program	= &cb_program
-};
-
-#define NFS4_CALLBACK 0x40000000
-static struct rpc_program cb_program = {
-		.name 		= "nfs4_cb",
-		.number		= NFS4_CALLBACK,
-		.nrvers		= ARRAY_SIZE(nfs_cb_version),
-		.version	= nfs_cb_version,
-		.stats		= &cb_stats,
-};
+static void free_rpc_program(struct rpc_program *cb_program)
+{
+	dprintk("%s: freeing callback program 0x%p\n", __func__, cb_program);
+	kfree(cb_program);
+}
 
 /* Reference counting, callback cleanup, etc., all look racy as heck.
  * And why is cb_set an atomic? */
@@ -371,12 +362,13 @@ static int do_probe_callback(void *data)
 		.to_maxval	= (NFSD_LEASE_TIME/2) * HZ,
 		.to_exponential	= 1,
 	};
+	struct rpc_stat		*cb_stats;
+	struct rpc_program	*cb_program;
 	struct rpc_create_args args = {
 		.protocol	= IPPROTO_TCP,
 		.address	= (struct sockaddr *)&addr,
 		.addrsize	= sizeof(addr),
 		.timeout	= &timeparms,
-		.program	= &cb_program,
 		.version	= nfs_cb_version[1]->number,
 		.authflavor	= RPC_AUTH_UNIX, /* XXX: need AUTH_GSS... */
 		.flags		= (RPC_CLNT_CREATE_NOPING | RPC_CLNT_CREATE_QUIET),
@@ -394,8 +386,31 @@ static int do_probe_callback(void *data)
 	addr.sin_port = htons(cb->cb_port);
 	addr.sin_addr.s_addr = htonl(cb->cb_addr);
 
+	/* Initialize rpc_program */
+	cb_program = kzalloc(sizeof(struct rpc_program) +
+			     sizeof(struct rpc_stat), GFP_KERNEL);
+	if (!cb_program) {
+		dprintk("NFSD: %s: couldn't allocate callback program\n",
+			__func__);
+		status = -ENOMEM;
+		goto out_err;
+	}
+
+	cb_program->name = "nfs4_cb";
+	cb_program->number = clp->cl_callback.cb_prog;
+	cb_program->nrvers = ARRAY_SIZE(nfs_cb_version);
+	cb_program->version = nfs_cb_version;
+	cb_program->free_rpc_program = free_rpc_program;
+	args.program = cb_program;
+
+	dprintk("%s: program %s 0x%x nrvers %u version %u\n",
+		__func__, args.program->name, args.program->number,
+		args.program->nrvers, args.version);
+
 	/* Initialize rpc_stat */
-	memset(args.program->stats, 0, sizeof(struct rpc_stat));
+	cb_stats = (struct rpc_stat *)(cb_program + 1);
+	cb_stats->program = cb_program;
+	cb_program->stats = cb_stats;
 
 	/* Create RPC client */
 	client = rpc_create(&args);
diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index e5bfe01..d342374 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -71,6 +71,7 @@ struct rpc_program {
 	struct rpc_version **	version;	/* version array */
 	struct rpc_stat *	stats;		/* statistics */
 	char *			pipe_dir_name;	/* path to rpc_pipefs dir */
+	void			(*free_rpc_program)(struct rpc_program *);
 };
 
 struct rpc_version {
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 76739e9..cfb21c0 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -415,6 +415,8 @@ rpc_free_client(struct kref *kref)
 	if (clnt->cl_server != clnt->cl_inline_name)
 		kfree(clnt->cl_server);
 out_free:
+	if (clnt->cl_program && clnt->cl_program->free_rpc_program)
+		clnt->cl_program->free_rpc_program(clnt->cl_program);
 	rpc_unregister_client(clnt);
 	rpc_free_iostats(clnt->cl_metrics);
 	clnt->cl_metrics = NULL;
-- 
1.6.0.1

Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

[Peter Staubach]

Benny Halevy wrote:
> From: Benny Halevy <bhalevy@panasas.com>
>
> since commit ff7d9756b501744540be65e172d27ee321d86103
> "nfsd: use static memory for callback program and stats"
> do_probe_callback uses a static callback program
> (NFS4_CALLBACK) rather than the one set in clp->cl_callback.cb_prog
> as passed in by the client in setclientid (4.0)
> or create_session (4.1).
>
> This patches allows allocating cb_program (and cb_stats) dynamically
> and setting a free_rpc_program function pointer to be
> called when the rpc_clnt structure is destroyed.
>
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
>
> Bruce, how about the following patch instead.
> It should solve the callback program number as well as Olga
> gss_auth issue.
>
> I'm still not sure about the gss_auth reference accounting
> for the rpc_clnt it references, but that's somewhat orthogonal
> to the solution in this patch.
>
> Benny
>
>  fs/nfsd/nfs4callback.c      |   47 ++++++++++++++++++++++++++++--------------
>  include/linux/sunrpc/clnt.h |    1 +
>  net/sunrpc/clnt.c           |    2 +
>  3 files changed, 34 insertions(+), 16 deletions(-)
>
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index 30d3130..6599b1e 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -342,20 +342,11 @@ static struct rpc_version *	nfs_cb_version[] = {
>  	&nfs_cb_version4,
>  };
>  
> -static struct rpc_program cb_program;
> -
> -static struct rpc_stat cb_stats = {
> -		.program	= &cb_program
> -};
> -
> -#define NFS4_CALLBACK 0x40000000
> -static struct rpc_program cb_program = {
> -		.name 		= "nfs4_cb",
> -		.number		= NFS4_CALLBACK,
> -		.nrvers		= ARRAY_SIZE(nfs_cb_version),
> -		.version	= nfs_cb_version,
> -		.stats		= &cb_stats,
> -};
> +static void free_rpc_program(struct rpc_program *cb_program)
> +{
> +	dprintk("%s: freeing callback program 0x%p\n", __func__, cb_program);
> +	kfree(cb_program);
> +}
>  
>  /* Reference counting, callback cleanup, etc., all look racy as heck.
>   * And why is cb_set an atomic? */
> @@ -371,12 +362,13 @@ static int do_probe_callback(void *data)
>  		.to_maxval	= (NFSD_LEASE_TIME/2) * HZ,
>  		.to_exponential	= 1,
>  	};
> +	struct rpc_stat		*cb_stats;
> +	struct rpc_program	*cb_program;
>  	struct rpc_create_args args = {
>  		.protocol	= IPPROTO_TCP,
>  		.address	= (struct sockaddr *)&addr,
>  		.addrsize	= sizeof(addr),
>  		.timeout	= &timeparms,
> -		.program	= &cb_program,
>  		.version	= nfs_cb_version[1]->number,
>  		.authflavor	= RPC_AUTH_UNIX, /* XXX: need AUTH_GSS... */
>  		.flags		= (RPC_CLNT_CREATE_NOPING | RPC_CLNT_CREATE_QUIET),
> @@ -394,8 +386,31 @@ static int do_probe_callback(void *data)
>  	addr.sin_port = htons(cb->cb_port);
>  	addr.sin_addr.s_addr = htonl(cb->cb_addr);
>  
> +	/* Initialize rpc_program */
> +	cb_program = kzalloc(sizeof(struct rpc_program) +
> +			     sizeof(struct rpc_stat), GFP_KERNEL);
>   

Ugg.  What about defining a struct which contains an rpc_program
followed by an rpc_stat and then allocate and free that?  This
seems a little, unclean, to me.

       ps

> +	if (!cb_program) {
> +		dprintk("NFSD: %s: couldn't allocate callback program\n",
> +			__func__);
> +		status = -ENOMEM;
> +		goto out_err;
> +	}
> +
> +	cb_program->name = "nfs4_cb";
> +	cb_program->number = clp->cl_callback.cb_prog;
> +	cb_program->nrvers = ARRAY_SIZE(nfs_cb_version);
> +	cb_program->version = nfs_cb_version;
> +	cb_program->free_rpc_program = free_rpc_program;
> +	args.program = cb_program;
> +
> +	dprintk("%s: program %s 0x%x nrvers %u version %u\n",
> +		__func__, args.program->name, args.program->number,
> +		args.program->nrvers, args.version);
> +
>  	/* Initialize rpc_stat */
> -	memset(args.program->stats, 0, sizeof(struct rpc_stat));
> +	cb_stats = (struct rpc_stat *)(cb_program + 1);
> +	cb_stats->program = cb_program;
> +	cb_program->stats = cb_stats;
>  
>  	/* Create RPC client */
>  	client = rpc_create(&args);
> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> index e5bfe01..d342374 100644
> --- a/include/linux/sunrpc/clnt.h
> +++ b/include/linux/sunrpc/clnt.h
> @@ -71,6 +71,7 @@ struct rpc_program {
>  	struct rpc_version **	version;	/* version array */
>  	struct rpc_stat *	stats;		/* statistics */
>  	char *			pipe_dir_name;	/* path to rpc_pipefs dir */
> +	void			(*free_rpc_program)(struct rpc_program *);
>  };
>  
>  struct rpc_version {
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 76739e9..cfb21c0 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -415,6 +415,8 @@ rpc_free_client(struct kref *kref)
>  	if (clnt->cl_server != clnt->cl_inline_name)
>  		kfree(clnt->cl_server);
>  out_free:
> +	if (clnt->cl_program && clnt->cl_program->free_rpc_program)
> +		clnt->cl_program->free_rpc_program(clnt->cl_program);
>  	rpc_unregister_client(clnt);
>  	rpc_free_iostats(clnt->cl_metrics);
>  	clnt->cl_metrics = NULL;
>   

Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

[Benny Halevy]

On Sep. 18, 2008, 14:43 -0500, Peter Staubach <staubach@redhat.com> wrote:
[snip]
>> +	cb_program = kzalloc(sizeof(struct rpc_program) +
>> +			     sizeof(struct rpc_stat), GFP_KERNEL);
>>   
> 
> Ugg.  What about defining a struct which contains an rpc_program
> followed by an rpc_stat and then allocate and free that?  This
> seems a little, unclean, to me.
> 
>        ps
> 

Sure. No problem.
Thanks for reviewing this.

Benny

Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

[Benny Halevy]

On Sep. 18, 2008, 15:05 -0500, Benny Halevy <bhalevy@panasas.com> wrote:
> On Sep. 18, 2008, 14:43 -0500, Peter Staubach <staubach@redhat.com> wrote:
> [snip]
>>> +	cb_program = kzalloc(sizeof(struct rpc_program) +
>>> +			     sizeof(struct rpc_stat), GFP_KERNEL);
>>>   
>> Ugg.  What about defining a struct which contains an rpc_program
>> followed by an rpc_stat and then allocate and free that?  This
>> seems a little, unclean, to me.
>>
>>        ps
>>
> 
> Sure. No problem.
> Thanks for reviewing this.
> 
> Benny

Here's the diff from the patch I sent.
I'll send the modified patch once I get Bruce's Ack
and possibly other comments.
Tested here in the Austin BAT.

Benny

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 6599b1e..0f13d75 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -342,10 +342,19 @@ static struct rpc_version *	nfs_cb_version[] = {
 	&nfs_cb_version4,
 };
 
-static void free_rpc_program(struct rpc_program *cb_program)
+/* used internally for dynamically allocating the callback rpc_program
+ * and rpc_stat */
+struct __rpc_prog_stats {
+	struct rpc_program	program;
+	struct rpc_stat		stats;
+};
+
+static void free_rpc_program(struct rpc_program *p)
 {
-	dprintk("%s: freeing callback program 0x%p\n", __func__, cb_program);
-	kfree(cb_program);
+	struct __rpc_prog_stats *ps = container_of(p, struct __rpc_prog_stats,
+						   program);
+	dprintk("%s: freeing callback prog_stats 0x%p\n", __func__, ps);
+	kfree(ps);
 }
 
 /* Reference counting, callback cleanup, etc., all look racy as heck.
@@ -362,8 +371,7 @@ static int do_probe_callback(void *data)
 		.to_maxval	= (NFSD_LEASE_TIME/2) * HZ,
 		.to_exponential	= 1,
 	};
-	struct rpc_stat		*cb_stats;
-	struct rpc_program	*cb_program;
+	struct __rpc_prog_stats	*cb_prog_stats;
 	struct rpc_create_args args = {
 		.protocol	= IPPROTO_TCP,
 		.address	= (struct sockaddr *)&addr,
@@ -387,30 +395,28 @@ static int do_probe_callback(void *data)
 	addr.sin_addr.s_addr = htonl(cb->cb_addr);
 
 	/* Initialize rpc_program */
-	cb_program = kzalloc(sizeof(struct rpc_program) +
-			     sizeof(struct rpc_stat), GFP_KERNEL);
-	if (!cb_program) {
+	cb_prog_stats = kzalloc(sizeof(*cb_prog_stats), GFP_KERNEL);
+	if (!cb_prog_stats) {
 		dprintk("NFSD: %s: couldn't allocate callback program\n",
 			__func__);
 		status = -ENOMEM;
 		goto out_err;
 	}
 
-	cb_program->name = "nfs4_cb";
-	cb_program->number = clp->cl_callback.cb_prog;
-	cb_program->nrvers = ARRAY_SIZE(nfs_cb_version);
-	cb_program->version = nfs_cb_version;
-	cb_program->free_rpc_program = free_rpc_program;
-	args.program = cb_program;
+	args.program = &cb_prog_stats->program;
+	args.program->name = "nfs4_cb";
+	args.program->number = clp->cl_callback.cb_prog;
+	args.program->nrvers = ARRAY_SIZE(nfs_cb_version);
+	args.program->version = nfs_cb_version;
+	args.program->free_rpc_program = free_rpc_program;
 
 	dprintk("%s: program %s 0x%x nrvers %u version %u\n",
 		__func__, args.program->name, args.program->number,
 		args.program->nrvers, args.version);
 
 	/* Initialize rpc_stat */
-	cb_stats = (struct rpc_stat *)(cb_program + 1);
-	cb_stats->program = cb_program;
-	cb_program->stats = cb_stats;
+	args.program->stats = &cb_prog_stats->stats;
+	args.program->stats->program = args.program;
 
 	/* Create RPC client */
 	client = rpc_create(&args);

Re: [PATCH] nfsd: use nfs client rpc callback program

[Olga Kornievskaia]

Benny Halevy wrote:
> On Sep. 17, 2008, 18:10 -0500, "J. Bruce Fields" <bfields@fieldses.org> wrote:
>   
>> On Wed, Sep 17, 2008 at 02:43:44PM -0500, Benny Halevy wrote:
>>     
>>> From: Benny Halevy <bhalevy@panasas.com>
>>>
>>> since commit ff7d9756b501744540be65e172d27ee321d86103
>>> "nfsd: use static memory for callback program and stats"
>>> do_probe_callback uses a static callback program
>>> (NFS4_CALLBACK) rather than the one set in clp->cl_callback.cb_prog
>>> as passed in by the client in setclientid (4.0)
>>> or create_session (4.1).
>>>       
>> Ugh, yes, sorry about that.  (I wonder why pynfs testing didn't catch
>> this?  Oh, I guess it's because NFS4_CALLBACK is the program number our
>> client always gives us.)
>>     
>
> Well, Fred (thanks!) added a test today which uses a non-default
> callback program and he sees a corresponding callback coming back.
>
> (Note that this test is not absolutely generic as the server is
> not required to probe the callback immediately, or at all, after
> setclientid or create_session.)
>
>   
>>> @@ -371,6 +356,8 @@ static int do_probe_callback(void *data)
>>>  		.to_maxval	= (NFSD_LEASE_TIME/2) * HZ,
>>>  		.to_exponential	= 1,
>>>  	};
>>> +	static struct rpc_stat	cb_stats;
>>> +	struct rpc_program	cb_program;
>>>  	struct rpc_create_args args = {
>>>  		.protocol	= IPPROTO_TCP,
>>>  		.address	= (struct sockaddr *)&addr,
>>> @@ -394,6 +381,20 @@ static int do_probe_callback(void *data)
>>>  	addr.sin_port = htons(cb->cb_port);
>>>  	addr.sin_addr.s_addr = htonl(cb->cb_addr);
>>>  
>>> +	/* Initialize rpc_program */
>>> +	memset(&cb_program, 0, sizeof(cb_program));
>>> +	cb_program.name = "nfs4_cb";
>>> +	cb_program.number = clp->cl_callback.cb_prog;
>>> +	cb_program.nrvers = ARRAY_SIZE(nfs_cb_version);
>>> +	cb_program.version = nfs_cb_version;
>>> +	cb_program.stats = &cb_stats;
>>> +	memset(&cb_stats, 0, sizeof(cb_stats));
>>> +	cb_stats.program = &cb_program;
>>>       
>> You don't want a pointer to data on the stack here, do you?
>>     
>
> Hmm, you're right...
> I went back and forth whether this should be allocated statically,
> dynamically, or on the stack.  I was mislead by the fact we're doing
> a sync rpc call, but indeed this needs to live until the nfs client
> is destroyed.  I'm trying to fully understand what Olga saw
> before coming up with a new proposal... maybe putting the cb_program
> back into struct nfs4_callback and just make cb_stats static would
> provide a solution of the problem Olga witnessed and keep everybody
> happy.
>   
I'm trying really hard to remember what was the issue of not using the 
structure and instead using static memory. From what I remember the 
issue was that the memory (clp->cl_callback.cb_prog) was going away.

Re: [PATCH] nfsd: use nfs client rpc callback program

[Benny Halevy]

On Sep. 19, 2008, 14:51 -0500, Olga Kornievskaia <aglo@citi.umich.edu> wrote:
> 
> Benny Halevy wrote:
>> On Sep. 17, 2008, 18:10 -0500, "J. Bruce Fields" <bfields@fieldses.org> wrote:
>>   
>>> On Wed, Sep 17, 2008 at 02:43:44PM -0500, Benny Halevy wrote:
>>>     
>>>> From: Benny Halevy <bhalevy@panasas.com>
>>>>
>>>> since commit ff7d9756b501744540be65e172d27ee321d86103
>>>> "nfsd: use static memory for callback program and stats"
>>>> do_probe_callback uses a static callback program
>>>> (NFS4_CALLBACK) rather than the one set in clp->cl_callback.cb_prog
>>>> as passed in by the client in setclientid (4.0)
>>>> or create_session (4.1).
>>>>       
>>> Ugh, yes, sorry about that.  (I wonder why pynfs testing didn't catch
>>> this?  Oh, I guess it's because NFS4_CALLBACK is the program number our
>>> client always gives us.)
>>>     
>> Well, Fred (thanks!) added a test today which uses a non-default
>> callback program and he sees a corresponding callback coming back.
>>
>> (Note that this test is not absolutely generic as the server is
>> not required to probe the callback immediately, or at all, after
>> setclientid or create_session.)
>>
>>   
>>>> @@ -371,6 +356,8 @@ static int do_probe_callback(void *data)
>>>>  		.to_maxval	= (NFSD_LEASE_TIME/2) * HZ,
>>>>  		.to_exponential	= 1,
>>>>  	};
>>>> +	static struct rpc_stat	cb_stats;
>>>> +	struct rpc_program	cb_program;
>>>>  	struct rpc_create_args args = {
>>>>  		.protocol	= IPPROTO_TCP,
>>>>  		.address	= (struct sockaddr *)&addr,
>>>> @@ -394,6 +381,20 @@ static int do_probe_callback(void *data)
>>>>  	addr.sin_port = htons(cb->cb_port);
>>>>  	addr.sin_addr.s_addr = htonl(cb->cb_addr);
>>>>  
>>>> +	/* Initialize rpc_program */
>>>> +	memset(&cb_program, 0, sizeof(cb_program));
>>>> +	cb_program.name = "nfs4_cb";
>>>> +	cb_program.number = clp->cl_callback.cb_prog;
>>>> +	cb_program.nrvers = ARRAY_SIZE(nfs_cb_version);
>>>> +	cb_program.version = nfs_cb_version;
>>>> +	cb_program.stats = &cb_stats;
>>>> +	memset(&cb_stats, 0, sizeof(cb_stats));
>>>> +	cb_stats.program = &cb_program;
>>>>       
>>> You don't want a pointer to data on the stack here, do you?
>>>     
>> Hmm, you're right...
>> I went back and forth whether this should be allocated statically,
>> dynamically, or on the stack.  I was mislead by the fact we're doing
>> a sync rpc call, but indeed this needs to live until the nfs client
>> is destroyed.  I'm trying to fully understand what Olga saw
>> before coming up with a new proposal... maybe putting the cb_program
>> back into struct nfs4_callback and just make cb_stats static would
>> provide a solution of the problem Olga witnessed and keep everybody
>> happy.
>>   
> I'm trying really hard to remember what was the issue of not using the 
> structure and instead using static memory. From what I remember the 
> issue was that the memory (clp->cl_callback.cb_prog) was going away.
> 

Yeah... pls see my reply on this thread.  From reading the code it
seems like the root cause is that gss_auth is holding a reference
on the rpc_clnt (for which I haven't seen a kref taken and released
which looks quite worrisome) and in gss_destroying_context() it's
doing rpc_call_null(gss_auth->client ... RPC_TASK_ASYNC) and therfore it needs
the program and stats.  This probably being called via:

nfs_free_client -> put_rpccred -> gss_destroy_cred (via
cred->cr_ops->crdestroy) -> gss_destroying_context.

since gss_destroying_context issues async null rpc
control returns to nfs_free_client which frees up the nfs_client
(and with it it used to free the rpc_program and rpc_stat)

Benny

Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

[J. Bruce Fields]

On Thu, Sep 18, 2008 at 02:24:39PM -0500, Benny Halevy wrote:
> From: Benny Halevy <bhalevy@panasas.com>
> 
> since commit ff7d9756b501744540be65e172d27ee321d86103
> "nfsd: use static memory for callback program and stats"
> do_probe_callback uses a static callback program
> (NFS4_CALLBACK) rather than the one set in clp->cl_callback.cb_prog
> as passed in by the client in setclientid (4.0)
> or create_session (4.1).
> 
> This patches allows allocating cb_program (and cb_stats) dynamically
> and setting a free_rpc_program function pointer to be
> called when the rpc_clnt structure is destroyed.

So that means we handle two cases:

	- free_rpc_program = NULL: We assume the program and stats are
	  in static memory (or module memory--which might be a problem
	  if we shut down the server and remove the nfsd module in quick
	  succession.  I assume there's a similar (probably very
	  hard-to-hit) bug on the client too, but haven't looked
	  carefully.).
	- free_rpc_program != NULL: We assume this rpc_client is the
	  last user of the program.

It seems a little ad hoc, but I can't see why it wouldn't solve the
problem.

I'd want Trond's ack.

--b.

> 
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
> 
> Bruce, how about the following patch instead.
> It should solve the callback program number as well as Olga
> gss_auth issue.
> 
> I'm still not sure about the gss_auth reference accounting
> for the rpc_clnt it references, but that's somewhat orthogonal
> to the solution in this patch.
> 
> Benny
> 
>  fs/nfsd/nfs4callback.c      |   47 ++++++++++++++++++++++++++++--------------
>  include/linux/sunrpc/clnt.h |    1 +
>  net/sunrpc/clnt.c           |    2 +
>  3 files changed, 34 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index 30d3130..6599b1e 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -342,20 +342,11 @@ static struct rpc_version *	nfs_cb_version[] = {
>  	&nfs_cb_version4,
>  };
>  
> -static struct rpc_program cb_program;
> -
> -static struct rpc_stat cb_stats = {
> -		.program	= &cb_program
> -};
> -
> -#define NFS4_CALLBACK 0x40000000
> -static struct rpc_program cb_program = {
> -		.name 		= "nfs4_cb",
> -		.number		= NFS4_CALLBACK,
> -		.nrvers		= ARRAY_SIZE(nfs_cb_version),
> -		.version	= nfs_cb_version,
> -		.stats		= &cb_stats,
> -};
> +static void free_rpc_program(struct rpc_program *cb_program)
> +{
> +	dprintk("%s: freeing callback program 0x%p\n", __func__, cb_program);
> +	kfree(cb_program);
> +}
>  
>  /* Reference counting, callback cleanup, etc., all look racy as heck.
>   * And why is cb_set an atomic? */
> @@ -371,12 +362,13 @@ static int do_probe_callback(void *data)
>  		.to_maxval	= (NFSD_LEASE_TIME/2) * HZ,
>  		.to_exponential	= 1,
>  	};
> +	struct rpc_stat		*cb_stats;
> +	struct rpc_program	*cb_program;
>  	struct rpc_create_args args = {
>  		.protocol	= IPPROTO_TCP,
>  		.address	= (struct sockaddr *)&addr,
>  		.addrsize	= sizeof(addr),
>  		.timeout	= &timeparms,
> -		.program	= &cb_program,
>  		.version	= nfs_cb_version[1]->number,
>  		.authflavor	= RPC_AUTH_UNIX, /* XXX: need AUTH_GSS... */
>  		.flags		= (RPC_CLNT_CREATE_NOPING | RPC_CLNT_CREATE_QUIET),
> @@ -394,8 +386,31 @@ static int do_probe_callback(void *data)
>  	addr.sin_port = htons(cb->cb_port);
>  	addr.sin_addr.s_addr = htonl(cb->cb_addr);
>  
> +	/* Initialize rpc_program */
> +	cb_program = kzalloc(sizeof(struct rpc_program) +
> +			     sizeof(struct rpc_stat), GFP_KERNEL);
> +	if (!cb_program) {
> +		dprintk("NFSD: %s: couldn't allocate callback program\n",
> +			__func__);
> +		status = -ENOMEM;
> +		goto out_err;
> +	}
> +
> +	cb_program->name = "nfs4_cb";
> +	cb_program->number = clp->cl_callback.cb_prog;
> +	cb_program->nrvers = ARRAY_SIZE(nfs_cb_version);
> +	cb_program->version = nfs_cb_version;
> +	cb_program->free_rpc_program = free_rpc_program;
> +	args.program = cb_program;
> +
> +	dprintk("%s: program %s 0x%x nrvers %u version %u\n",
> +		__func__, args.program->name, args.program->number,
> +		args.program->nrvers, args.version);
> +
>  	/* Initialize rpc_stat */
> -	memset(args.program->stats, 0, sizeof(struct rpc_stat));
> +	cb_stats = (struct rpc_stat *)(cb_program + 1);
> +	cb_stats->program = cb_program;
> +	cb_program->stats = cb_stats;
>  
>  	/* Create RPC client */
>  	client = rpc_create(&args);
> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> index e5bfe01..d342374 100644
> --- a/include/linux/sunrpc/clnt.h
> +++ b/include/linux/sunrpc/clnt.h
> @@ -71,6 +71,7 @@ struct rpc_program {
>  	struct rpc_version **	version;	/* version array */
>  	struct rpc_stat *	stats;		/* statistics */
>  	char *			pipe_dir_name;	/* path to rpc_pipefs dir */
> +	void			(*free_rpc_program)(struct rpc_program *);
>  };
>  
>  struct rpc_version {
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 76739e9..cfb21c0 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -415,6 +415,8 @@ rpc_free_client(struct kref *kref)
>  	if (clnt->cl_server != clnt->cl_inline_name)
>  		kfree(clnt->cl_server);
>  out_free:
> +	if (clnt->cl_program && clnt->cl_program->free_rpc_program)
> +		clnt->cl_program->free_rpc_program(clnt->cl_program);
>  	rpc_unregister_client(clnt);
>  	rpc_free_iostats(clnt->cl_metrics);
>  	clnt->cl_metrics = NULL;
> -- 
> 1.6.0.1
> 

Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

[Trond Myklebust]

On Wed, 2008-09-24 at 12:35 -0400, J. Bruce Fields wrote:
> On Thu, Sep 18, 2008 at 02:24:39PM -0500, Benny Halevy wrote:
> > From: Benny Halevy <bhalevy@panasas.com>
> > 
> > since commit ff7d9756b501744540be65e172d27ee321d86103
> > "nfsd: use static memory for callback program and stats"
> > do_probe_callback uses a static callback program
> > (NFS4_CALLBACK) rather than the one set in clp->cl_callback.cb_prog
> > as passed in by the client in setclientid (4.0)
> > or create_session (4.1).
> > 
> > This patches allows allocating cb_program (and cb_stats) dynamically
> > and setting a free_rpc_program function pointer to be
> > called when the rpc_clnt structure is destroyed.
> 
> So that means we handle two cases:
> 
> 	- free_rpc_program = NULL: We assume the program and stats are
> 	  in static memory (or module memory--which might be a problem
> 	  if we shut down the server and remove the nfsd module in quick
> 	  succession.  I assume there's a similar (probably very
> 	  hard-to-hit) bug on the client too, but haven't looked
> 	  carefully.).
> 	- free_rpc_program != NULL: We assume this rpc_client is the
> 	  last user of the program.
> 
> It seems a little ad hoc, but I can't see why it wouldn't solve the
> problem.
> 
> I'd want Trond's ack.

Well the current implementation is certainly broken. Look at what
happens if I clone the rpc_clnt...

  Trond

Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

[J. Bruce Fields]

On Wed, Sep 24, 2008 at 12:59:42PM -0400, Trond Myklebust wrote:
> On Wed, 2008-09-24 at 12:35 -0400, J. Bruce Fields wrote:
> > On Thu, Sep 18, 2008 at 02:24:39PM -0500, Benny Halevy wrote:
> > > From: Benny Halevy <bhalevy@panasas.com>
> > > 
> > > since commit ff7d9756b501744540be65e172d27ee321d86103
> > > "nfsd: use static memory for callback program and stats"
> > > do_probe_callback uses a static callback program
> > > (NFS4_CALLBACK) rather than the one set in clp->cl_callback.cb_prog
> > > as passed in by the client in setclientid (4.0)
> > > or create_session (4.1).
> > > 
> > > This patches allows allocating cb_program (and cb_stats) dynamically
> > > and setting a free_rpc_program function pointer to be
> > > called when the rpc_clnt structure is destroyed.
> > 
> > So that means we handle two cases:
> > 
> > 	- free_rpc_program = NULL: We assume the program and stats are
> > 	  in static memory (or module memory--which might be a problem
> > 	  if we shut down the server and remove the nfsd module in quick
> > 	  succession.  I assume there's a similar (probably very
> > 	  hard-to-hit) bug on the client too, but haven't looked
> > 	  carefully.).
> > 	- free_rpc_program != NULL: We assume this rpc_client is the
> > 	  last user of the program.
> > 
> > It seems a little ad hoc, but I can't see why it wouldn't solve the
> > problem.
> > 
> > I'd want Trond's ack.
> 
> Well the current implementation is certainly broken.  Look at what
> happens if I clone the rpc_clnt...

Hence the comment that "we assume this rpc_client is the last user of
the program."  I believe that assumption is correct in the case of nfsd
callbacks, so Benny's patch is at least not broken--just a little
ad-hoc.

So the question is whether the above solution, which addresses only this
particular case, is sufficient, or whether we'd like something more
general, like adding a reference count to the program along with a
free_program callback called only on the final put.

--b.

Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

[Trond Myklebust]

On Wed, 2008-09-24 at 13:21 -0400, J. Bruce Fields wrote:
> > Well the current implementation is certainly broken.  Look at what
> > happens if I clone the rpc_clnt...
> 
> Hence the comment that "we assume this rpc_client is the last user of
> the program."  I believe that assumption is correct in the case of nfsd
> callbacks, so Benny's patch is at least not broken--just a little
> ad-hoc.
> 
> So the question is whether the above solution, which addresses only this
> particular case, is sufficient, or whether we'd like something more
> general, like adding a reference count to the program along with a
> free_program callback called only on the final put.

It's broken...

Trond

Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

[J. Bruce Fields]

On Wed, Sep 24, 2008 at 01:26:08PM -0400, Trond Myklebust wrote:
> On Wed, 2008-09-24 at 13:21 -0400, J. Bruce Fields wrote:
> > > Well the current implementation is certainly broken.  Look at what
> > > happens if I clone the rpc_clnt...
> > 
> > Hence the comment that "we assume this rpc_client is the last user of
> > the program."  I believe that assumption is correct in the case of nfsd
> > callbacks, so Benny's patch is at least not broken--just a little
> > ad-hoc.
> > 
> > So the question is whether the above solution, which addresses only this
> > particular case, is sufficient, or whether we'd like something more
> > general, like adding a reference count to the program along with a
> > free_program callback called only on the final put.
> 
> It's broken...

If by "broken" you mean, "introduces a new kernel bug", I don't see it.

The new free_rpc_program callback is set only in
fs/nfsd/nfs4callback.c:do_probe_callback().  For all other programs it
is NULL, thus there's no change of behavior.

And rpc_clone_client() is never called on the client created in
do_probe_callback().

If your argument that it's ugly to introduce a rule like that requires
you never to call rpc_clone_client on a cllient with certain properties,
then I can agree.  (In that case, does a reference count on the program
look like an acceptable solution?)

If there's some other bug, then I'd like to understand.

--b.

Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

[Trond Myklebust]

On Wed, 2008-09-24 at 13:42 -0400, J. Bruce Fields wrote:

> If by "broken" you mean, "introduces a new kernel bug", I don't see it.

I mean it introduces utterly unnecessary complications that may return
to bite us in the arse at a future time.
Remember how we once said that it would never make sense for child
clones to call the portmapper, and so we added the BUG_ON() in
rpcb_getport_async; well guess what, we currently have a bug to fix...

One pretty obvious fix is to simply move the release method so that it
doesn't occur when you release a child. The disadvantage is that a child
may then not change its program to one that requires a release method
(do we need that?).

Another fix would be to add a refcount to the rpc_program structure...

Trond

Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

[J. Bruce Fields]

On Wed, Sep 24, 2008 at 02:42:25PM -0400, Trond Myklebust wrote:
> On Wed, 2008-09-24 at 13:42 -0400, J. Bruce Fields wrote:
> 
> > If by "broken" you mean, "introduces a new kernel bug", I don't see it.
> 
> I mean it introduces utterly unnecessary complications that may return
> to bite us in the arse at a future time.

OK, that's the answer I was looking for, thanks.

> Remember how we once said that it would never make sense for child
> clones to call the portmapper, and so we added the BUG_ON() in
> rpcb_getport_async; well guess what, we currently have a bug to fix...

No, I don't remember that.  But yes, I can see how in general this sort
of thing could make the code harder to maintain.

> One pretty obvious fix is to simply move the release method so that it
> doesn't occur when you release a child. The disadvantage is that a child
> may then not change its program to one that requires a release method
> (do we need that?).

I doubt we need that, but...

> Another fix would be to add a refcount to the rpc_program structure...

... a refcount seems more straightforward.  Benny, what do you think?

--b.

Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

[Benny Halevy]

On 09/24/2008 9:49:34 pm +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> On Wed, Sep 24, 2008 at 02:42:25PM -0400, Trond Myklebust wrote:
>> On Wed, 2008-09-24 at 13:42 -0400, J. Bruce Fields wrote:
>>
>>> If by "broken" you mean, "introduces a new kernel bug", I don't see it.
>> I mean it introduces utterly unnecessary complications that may return
>> to bite us in the arse at a future time.
> 
> OK, that's the answer I was looking for, thanks.
> 
>> Remember how we once said that it would never make sense for child
>> clones to call the portmapper, and so we added the BUG_ON() in
>> rpcb_getport_async; well guess what, we currently have a bug to fix...
> 
> No, I don't remember that.  But yes, I can see how in general this sort
> of thing could make the code harder to maintain.
> 
>> One pretty obvious fix is to simply move the release method so that it
>> doesn't occur when you release a child. The disadvantage is that a child
>> may then not change its program to one that requires a release method
>> (do we need that?).
> 
> I doubt we need that, but...
> 
>> Another fix would be to add a refcount to the rpc_program structure...
> 
> ... a refcount seems more straightforward.  Benny, what do you think?

I agree.  I'll send a patch hopefully tomorrow.
Would you like that combined with the one I sent or as a separate one?
(I'm inclined towards the latter).

One more thing that seems to need fixing is rpc_bind_new_program
which now uses the passed-in program->cl_stats but doesn't point
to the passed-in program, but rather it only extracts its
name, number, and stats.
Since program->stats may possibly go away with the program
in the refcounted world I think we should get a reference on the
program here too.

That observed, it may also be a good idea to get rid of clnt->cl_stats
altogether and use clnt->program->stats instead to prevent any
discrepancy.

Benny

> 
> --b.

Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

[J. Bruce Fields]

On Thu, Sep 25, 2008 at 10:30:33PM +0300, Benny Halevy wrote:
> On 09/24/2008 9:49:34 pm +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > On Wed, Sep 24, 2008 at 02:42:25PM -0400, Trond Myklebust wrote:
> >> On Wed, 2008-09-24 at 13:42 -0400, J. Bruce Fields wrote:
> >>
> >>> If by "broken" you mean, "introduces a new kernel bug", I don't see it.
> >> I mean it introduces utterly unnecessary complications that may return
> >> to bite us in the arse at a future time.
> > 
> > OK, that's the answer I was looking for, thanks.
> > 
> >> Remember how we once said that it would never make sense for child
> >> clones to call the portmapper, and so we added the BUG_ON() in
> >> rpcb_getport_async; well guess what, we currently have a bug to fix...
> > 
> > No, I don't remember that.  But yes, I can see how in general this sort
> > of thing could make the code harder to maintain.
> > 
> >> One pretty obvious fix is to simply move the release method so that it
> >> doesn't occur when you release a child. The disadvantage is that a child
> >> may then not change its program to one that requires a release method
> >> (do we need that?).
> > 
> > I doubt we need that, but...
> > 
> >> Another fix would be to add a refcount to the rpc_program structure...
> > 
> > ... a refcount seems more straightforward.  Benny, what do you think?
> 
> I agree.  I'll send a patch hopefully tomorrow.
> Would you like that combined with the one I sent or as a separate one?
> (I'm inclined towards the latter).

That'd be fine.

> One more thing that seems to need fixing is rpc_bind_new_program
> which now uses the passed-in program->cl_stats but doesn't point
> to the passed-in program, but rather it only extracts its
> name, number, and stats.
> Since program->stats may possibly go away with the program
> in the refcounted world I think we should get a reference on the
> program here too.
> 
> That observed, it may also be a good idea to get rid of clnt->cl_stats
> altogether and use clnt->program->stats instead to prevent any
> discrepancy.

Sounds good to me.

--b.

Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

[Trond Myklebust]

On Thu, 2008-09-25 at 22:30 +0300, Benny Halevy wrote:
> One more thing that seems to need fixing is rpc_bind_new_program
> which now uses the passed-in program->cl_stats but doesn't point
> to the passed-in program, but rather it only extracts its
> name, number, and stats.
> Since program->stats may possibly go away with the program
> in the refcounted world I think we should get a reference on the
> program here too.
> 
> That observed, it may also be a good idea to get rid of clnt->cl_stats
> altogether and use clnt->program->stats instead to prevent any
> discrepancy.

No. cl_stats are _per_client_ stats. program->stats are per protocol.
You can't just substitute one for the other.

Trond

Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

[Trond Myklebust]

On Thu, 2008-09-25 at 16:00 -0400, J. Bruce Fields wrote:
> > >> Another fix would be to add a refcount to the rpc_program structure...
> > > 
> > > ... a refcount seems more straightforward.  Benny, what do you think?
> > 
> > I agree.  I'll send a patch hopefully tomorrow.
> > Would you like that combined with the one I sent or as a separate one?
> > (I'm inclined towards the latter).
> 
> That'd be fine.

So, looking at what you're trying to do, I'm still having trouble
figuring out why you think you need a dynamically allocated rpc_program
in the first place.

If the only thing you are trying to support is dynamically allocated
program numbers, then note that rpc_encode_header() doesn't use
program->number at all. Instead, it uses clnt->cl_prog and
clnt->cl_vers. Nothing stops you from setting those values explicitly...

Trond

Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

[J. Bruce Fields]

On Thu, Sep 25, 2008 at 04:08:58PM -0400, Trond Myklebust wrote:
> On Thu, 2008-09-25 at 22:30 +0300, Benny Halevy wrote:
> > One more thing that seems to need fixing is rpc_bind_new_program
> > which now uses the passed-in program->cl_stats but doesn't point
> > to the passed-in program, but rather it only extracts its
> > name, number, and stats.
> > Since program->stats may possibly go away with the program
> > in the refcounted world I think we should get a reference on the
> > program here too.
> > 
> > That observed, it may also be a good idea to get rid of clnt->cl_stats
> > altogether and use clnt->program->stats instead to prevent any
> > discrepancy.
> 
> No. cl_stats are _per_client_ stats. program->stats are per protocol.
> You can't just substitute one for the other.

A "git grep cl_stats" only finds me two places where it's assigned to,
both of the form

	clnt->cl_stats    = program->stats;

So it looks per-program; am I missing something?

--b.

Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

[J. Bruce Fields]

On Thu, Sep 25, 2008 at 04:27:01PM -0400, Trond Myklebust wrote:
> On Thu, 2008-09-25 at 16:00 -0400, J. Bruce Fields wrote:
> > > >> Another fix would be to add a refcount to the rpc_program structure...
> > > > 
> > > > ... a refcount seems more straightforward.  Benny, what do you think?
> > > 
> > > I agree.  I'll send a patch hopefully tomorrow.
> > > Would you like that combined with the one I sent or as a separate one?
> > > (I'm inclined towards the latter).
> > 
> > That'd be fine.
> 
> So, looking at what you're trying to do, I'm still having trouble
> figuring out why you think you need a dynamically allocated rpc_program
> in the first place.
> 
> If the only thing you are trying to support is dynamically allocated
> program numbers, then note that rpc_encode_header() doesn't use
> program->number at all. Instead, it uses clnt->cl_prog and
> clnt->cl_vers. Nothing stops you from setting those values explicitly...

Oh, sure, that sounds like an excellent plan--thanks!

There's still, as far as I can tell, the small risk of a race on module
unload.  I don't think we've seen it, and I don't know if it's worth
much effort at this point.

--b.

Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

[Benny Halevy]

J. Bruce Fields wrote:
> On Thu, Sep 25, 2008 at 04:27:01PM -0400, Trond Myklebust wrote:
>> On Thu, 2008-09-25 at 16:00 -0400, J. Bruce Fields wrote:
>>>>>> Another fix would be to add a refcount to the rpc_program structure...
>>>>> ... a refcount seems more straightforward.  Benny, what do you think?
>>>> I agree.  I'll send a patch hopefully tomorrow.
>>>> Would you like that combined with the one I sent or as a separate one?
>>>> (I'm inclined towards the latter).
>>> That'd be fine.
>> So, looking at what you're trying to do, I'm still having trouble
>> figuring out why you think you need a dynamically allocated rpc_program
>> in the first place.
>>
>> If the only thing you are trying to support is dynamically allocated
>> program numbers, then note that rpc_encode_header() doesn't use
>> program->number at all. Instead, it uses clnt->cl_prog and
>> clnt->cl_vers. Nothing stops you from setting those values explicitly...
> 
> Oh, sure, that sounds like an excellent plan--thanks!
> 
Yeah, much simpler.
Though having nfs4_probe_callback directly assign to clnt->cl_prog
would work, it seems like a layering violation.
How about allowing this officially via struct rpc_create_args?

Benny

diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index e5bfe01..4ba84e8 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -104,6 +104,7 @@ struct  {
 	const struct rpc_timeout *timeout;
 	char			*servername;
 	struct rpc_program	*program;
+	u32			prognumber;	/* overrides program->number */
 	u32			version;
 	rpc_authflavor_t	authflavor;
 	unsigned long		flags;
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 76739e9..da0789f 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -174,7 +174,7 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args, stru
 	clnt->cl_procinfo = version->procs;
 	clnt->cl_maxproc  = version->nrprocs;
 	clnt->cl_protname = program->name;
-	clnt->cl_prog     = program->number;
+	clnt->cl_prog     = args->prognumber ? : program->number;
 	clnt->cl_vers     = version->number;
 	clnt->cl_stats    = program->stats;
 	clnt->cl_metrics  = rpc_alloc_iostats(clnt);
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 30d3130..5e95909 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -377,6 +377,7 @@ static int do_probe_callback(void *data)
 		.addrsize	= sizeof(addr),
 		.timeout	= &timeparms,
 		.program	= &cb_program,
+		.prognumber	= cb->cb_prog,
 		.version	= nfs_cb_version[1]->number,
 		.authflavor	= RPC_AUTH_UNIX, /* XXX: need AUTH_GSS... */
 		.flags		= (RPC_CLNT_CREATE_NOPING | RPC_CLNT_CREATE_QUIET),

Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

[J. Bruce Fields]

On Fri, Sep 26, 2008 at 02:52:10PM +0300, Benny Halevy wrote:
> J. Bruce Fields wrote:
> > On Thu, Sep 25, 2008 at 04:27:01PM -0400, Trond Myklebust wrote:
> >> On Thu, 2008-09-25 at 16:00 -0400, J. Bruce Fields wrote:
> >>>>>> Another fix would be to add a refcount to the rpc_program structure...
> >>>>> ... a refcount seems more straightforward.  Benny, what do you think?
> >>>> I agree.  I'll send a patch hopefully tomorrow.
> >>>> Would you like that combined with the one I sent or as a separate one?
> >>>> (I'm inclined towards the latter).
> >>> That'd be fine.
> >> So, looking at what you're trying to do, I'm still having trouble
> >> figuring out why you think you need a dynamically allocated rpc_program
> >> in the first place.
> >>
> >> If the only thing you are trying to support is dynamically allocated
> >> program numbers, then note that rpc_encode_header() doesn't use
> >> program->number at all. Instead, it uses clnt->cl_prog and
> >> clnt->cl_vers. Nothing stops you from setting those values explicitly...
> > 
> > Oh, sure, that sounds like an excellent plan--thanks!
> > 
> Yeah, much simpler.
> Though having nfs4_probe_callback directly assign to clnt->cl_prog
> would work, it seems like a layering violation.
> How about allowing this officially via struct rpc_create_args?

Looks good to me; if you could resend with the usual changelog and
signed-off-by, I'll apply it to my tree, assuming no objection from
Trond.

--b.

> 
> Benny
> 
> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> index e5bfe01..4ba84e8 100644
> --- a/include/linux/sunrpc/clnt.h
> +++ b/include/linux/sunrpc/clnt.h
> @@ -104,6 +104,7 @@ struct  {
>  	const struct rpc_timeout *timeout;
>  	char			*servername;
>  	struct rpc_program	*program;
> +	u32			prognumber;	/* overrides program->number */
>  	u32			version;
>  	rpc_authflavor_t	authflavor;
>  	unsigned long		flags;
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 76739e9..da0789f 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -174,7 +174,7 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args, stru
>  	clnt->cl_procinfo = version->procs;
>  	clnt->cl_maxproc  = version->nrprocs;
>  	clnt->cl_protname = program->name;
> -	clnt->cl_prog     = program->number;
> +	clnt->cl_prog     = args->prognumber ? : program->number;
>  	clnt->cl_vers     = version->number;
>  	clnt->cl_stats    = program->stats;
>  	clnt->cl_metrics  = rpc_alloc_iostats(clnt);
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index 30d3130..5e95909 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -377,6 +377,7 @@ static int do_probe_callback(void *data)
>  		.addrsize	= sizeof(addr),
>  		.timeout	= &timeparms,
>  		.program	= &cb_program,
> +		.prognumber	= cb->cb_prog,
>  		.version	= nfs_cb_version[1]->number,
>  		.authflavor	= RPC_AUTH_UNIX, /* XXX: need AUTH_GSS... */
>  		.flags		= (RPC_CLNT_CREATE_NOPING | RPC_CLNT_CREATE_QUIET),

[PATCH v4] nfsd: use nfs client rpc callback program

[Benny Halevy]

From: Benny Halevy <bhalevy@panasas.com>

since commit ff7d9756b501744540be65e172d27ee321d86103
"nfsd: use static memory for callback program and stats"
do_probe_callback uses a static callback program
(NFS4_CALLBACK) rather than the one set in clp->cl_callback.cb_prog
as passed in by the client in setclientid (4.0)
or create_session (4.1).

This patches introduces rpc_create_args.prognumber that allows
overriding program->number when creating rpc_clnt.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4callback.c      |    1 +
 include/linux/sunrpc/clnt.h |    1 +
 net/sunrpc/clnt.c           |    2 +-
 3 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 3073ccb..e198ead 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -377,6 +377,7 @@ static int do_probe_callback(void *data)
 		.addrsize	= sizeof(addr),
 		.timeout	= &timeparms,
 		.program	= &cb_program,
+		.prognumber	= cb->cb_prog,
 		.version	= nfs_cb_version[1]->number,
 		.authflavor	= RPC_AUTH_UNIX, /* XXX: need AUTH_GSS... */
 		.flags		= (RPC_CLNT_CREATE_NOPING | RPC_CLNT_CREATE_QUIET),
diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index e5bfe01..4ba84e8 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -104,6 +104,7 @@ struct rpc_create_args {
 	const struct rpc_timeout *timeout;
 	char			*servername;
 	struct rpc_program	*program;
+	u32			prognumber;	/* overrides program->number */
 	u32			version;
 	rpc_authflavor_t	authflavor;
 	unsigned long		flags;
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 76739e9..da0789f 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -174,7 +174,7 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args, stru
 	clnt->cl_procinfo = version->procs;
 	clnt->cl_maxproc  = version->nrprocs;
 	clnt->cl_protname = program->name;
-	clnt->cl_prog     = program->number;
+	clnt->cl_prog     = args->prognumber ? : program->number;
 	clnt->cl_vers     = version->number;
 	clnt->cl_stats    = program->stats;
 	clnt->cl_metrics  = rpc_alloc_iostats(clnt);
-- 
1.6.0.2

Re: [PATCH v4] nfsd: use nfs client rpc callback program

[J. Bruce Fields]

On Sun, Sep 28, 2008 at 09:21:26AM +0300, Benny Halevy wrote:
> From: Benny Halevy <bhalevy@panasas.com>
> 
> since commit ff7d9756b501744540be65e172d27ee321d86103
> "nfsd: use static memory for callback program and stats"
> do_probe_callback uses a static callback program
> (NFS4_CALLBACK) rather than the one set in clp->cl_callback.cb_prog
> as passed in by the client in setclientid (4.0)
> or create_session (4.1).
> 
> This patches introduces rpc_create_args.prognumber that allows
> overriding program->number when creating rpc_clnt.

Applied--thanks!--b.

> 
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
>  fs/nfsd/nfs4callback.c      |    1 +
>  include/linux/sunrpc/clnt.h |    1 +
>  net/sunrpc/clnt.c           |    2 +-
>  3 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index 3073ccb..e198ead 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -377,6 +377,7 @@ static int do_probe_callback(void *data)
>  		.addrsize	= sizeof(addr),
>  		.timeout	= &timeparms,
>  		.program	= &cb_program,
> +		.prognumber	= cb->cb_prog,
>  		.version	= nfs_cb_version[1]->number,
>  		.authflavor	= RPC_AUTH_UNIX, /* XXX: need AUTH_GSS... */
>  		.flags		= (RPC_CLNT_CREATE_NOPING | RPC_CLNT_CREATE_QUIET),
> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> index e5bfe01..4ba84e8 100644
> --- a/include/linux/sunrpc/clnt.h
> +++ b/include/linux/sunrpc/clnt.h
> @@ -104,6 +104,7 @@ struct rpc_create_args {
>  	const struct rpc_timeout *timeout;
>  	char			*servername;
>  	struct rpc_program	*program;
> +	u32			prognumber;	/* overrides program->number */
>  	u32			version;
>  	rpc_authflavor_t	authflavor;
>  	unsigned long		flags;
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 76739e9..da0789f 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -174,7 +174,7 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args, stru
>  	clnt->cl_procinfo = version->procs;
>  	clnt->cl_maxproc  = version->nrprocs;
>  	clnt->cl_protname = program->name;
> -	clnt->cl_prog     = program->number;
> +	clnt->cl_prog     = args->prognumber ? : program->number;
>  	clnt->cl_vers     = version->number;
>  	clnt->cl_stats    = program->stats;
>  	clnt->cl_metrics  = rpc_alloc_iostats(clnt);
> -- 
> 1.6.0.2
> 
> 
> --
> 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