From: Benny Halevy <[email protected]>
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 <[email protected]>
---
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
On 09/24/2008 9:49:34 pm +0300, "J. Bruce Fields" <[email protected]> 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.
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" <[email protected]> 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.
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
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
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.
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.
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),
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),
From: Benny Halevy <[email protected]>
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 <[email protected]>
---
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
On Sun, Sep 28, 2008 at 09:21:26AM +0300, Benny Halevy wrote:
> From: Benny Halevy <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Benny Halevy <[email protected]>
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 <[email protected]>
---
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
Benny Halevy wrote:
> From: Benny Halevy <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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;
>
On Sep. 18, 2008, 14:43 -0500, Peter Staubach <[email protected]> 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
On Sep. 18, 2008, 15:05 -0500, Benny Halevy <[email protected]> wrote:
> On Sep. 18, 2008, 14:43 -0500, Peter Staubach <[email protected]> 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);
Benny Halevy wrote:
> On Sep. 17, 2008, 18:10 -0500, "J. Bruce Fields" <[email protected]> wrote:
>
>> On Wed, Sep 17, 2008 at 02:43:44PM -0500, Benny Halevy wrote:
>>
>>> From: Benny Halevy <[email protected]>
>>>
>>> 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.
On Sep. 19, 2008, 14:51 -0500, Olga Kornievskaia <[email protected]> wrote:
>
> Benny Halevy wrote:
>> On Sep. 17, 2008, 18:10 -0500, "J. Bruce Fields" <[email protected]> wrote:
>>
>>> On Wed, Sep 17, 2008 at 02:43:44PM -0500, Benny Halevy wrote:
>>>
>>>> From: Benny Halevy <[email protected]>
>>>>
>>>> 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
On Thu, Sep 18, 2008 at 02:24:39PM -0500, Benny Halevy wrote:
> From: Benny Halevy <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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
>
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 <[email protected]>
> >
> > 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
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 <[email protected]>
> > >
> > > 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.
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
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.
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
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.
On Wed, Sep 17, 2008 at 02:43:44PM -0500, Benny Halevy wrote:
> From: Benny Halevy <[email protected]>
>
> 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.
On Sep. 17, 2008, 18:10 -0500, "J. Bruce Fields" <[email protected]> wrote:
> On Wed, Sep 17, 2008 at 02:43:44PM -0500, Benny Halevy wrote:
>> From: Benny Halevy <[email protected]>
>>
>> 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.
On Sep. 17, 2008, 18:34 -0500, Benny Halevy <[email protected]> wrote:
> On Sep. 17, 2008, 18:10 -0500, "J. Bruce Fields" <[email protected]> wrote:
>> On Wed, Sep 17, 2008 at 02:43:44PM -0500, Benny Halevy wrote:
>>> From: Benny Halevy <[email protected]>
>>>
>>> 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
> [email protected]
> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs