xs_setup_bc_tcp may return an existing xprt with non-NULL servername.
xprt_create_transport should not kstrdup servername for it.
Otherwise, those memory for servername will be leaked.
Signed-off-by: Kinglong Mee <[email protected]>
---
net/sunrpc/xprt.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index ddd198e..6fa966f 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -1339,7 +1339,11 @@ found:
xprt_destroy(xprt);
return ERR_PTR(-EINVAL);
}
- xprt->servername = kstrdup(args->servername, GFP_KERNEL);
+
+ /* servername may not be NULL for tcp NFSv4.1 backchannel */
+ if (xprt->servername == NULL)
+ xprt->servername = kstrdup(args->servername, GFP_KERNEL);
+
if (xprt->servername == NULL) {
xprt_destroy(xprt);
return ERR_PTR(-ENOMEM);
--
1.8.4.2
On Jan 6, 2014, at 17:53, Dr Fields James Bruce <[email protected]> wrote:
> On Mon, Jan 06, 2014 at 05:40:03PM -0500, Trond Myklebust wrote:
>>
>> On Jan 6, 2014, at 13:49, J. Bruce Fields <[email protected]> wrote:
>>
>>> On Mon, Jan 06, 2014 at 05:33:22PM +0800, Kinglong Mee wrote:
>>>> xs_setup_bc_tcp may return an existing xprt with non-NULL servername.
>>>> xprt_create_transport should not kstrdup servername for it.
>>>> Otherwise, those memory for servername will be leaked.
>>>
>>> OK. Applying to my tree if Trond has no objection.
>>
>> Actually. Why do we go through all this code at all if xs_setup_bc_tcp() returns args->bc_xprt->xpt_bc_xprt? I?m assuming that is the only case where xprt->servername != NULL, right?
>>
>> For instance, won?t calling INIT_WORK() be a source of problems?
>
> Huh. Looking at the history.... There used to be a
>
> if (test_and_set_bit(XPRT_INITIALIZED, &xprt->state))
> /* ->setup returned a pre-initialized xprt: */
> return xprt;
>
> here, but it got removed by 21de0a955f3af29fa1100d96f66e6adade89e77a
> "SUNRPC: Clean up the slot table allocation", which looks otherwise
> unrelated. Was that just some kind of rebasing mistake, or was there a
> reason for that?
I probably misunderstood that bc_xprt sends a fully initialized struct rpc_xprt.
The obvious question if that is the case, is why we are calling xprt_create_transport() at all? If .bc_xprt->xpt_bc_xprt contains a fully initialized struct rpc_xprt, then just have rpc_create() do the honors.
Better yet, create a svc_create_backchannel_client() helper that calls rpc_new_client() with the correct parameters. I really don?t like those rpc_create_args hacks that introduce fields that are completely private to nfsd.
Trond
On Jan 9, 2014, at 11:26, Dr Fields James Bruce <[email protected]> wrote:
> On Thu, Jan 09, 2014 at 06:33:10PM +0800, Kinglong Mee wrote:
>> Besides checking rpc_xprt out of xs_setup_bc_tcp,
>> increase it's reference (it's important).
>
> This sounds wrong to me: the presence of a backchannel can't prevent the
> client's connection from going away. Instead, when the connection dies,
> any associated backchannels should be immediately destroyed.
Hi Bruce,
The right way to deal with this is to have knfsd shut down the rpc_client when it detects the TCP disconnection event. The xprt->count shouldn?t be an issue here: it has nothing to do with the socket connection state.
Cheers
Trond
On 01/10/2014 01:27 AM, Trond Myklebust wrote:
>
> On Jan 9, 2014, at 11:26, Dr Fields James Bruce <[email protected]> wrote:
>
>> On Thu, Jan 09, 2014 at 06:33:10PM +0800, Kinglong Mee wrote:
>>> Besides checking rpc_xprt out of xs_setup_bc_tcp,
>>> increase it's reference (it's important).
>>
>> This sounds wrong to me: the presence of a backchannel can't prevent the
>> client's connection from going away. Instead, when the connection dies,
>> any associated backchannels should be immediately destroyed.
>
> Hi Bruce,
>
> The right way to deal with this is to have knfsd shut down the rpc_client
> when it detects the TCP disconnection event.
Yes, that's right.
Knfsd has do it as you said.
When getting xprt's status of XPT_CLOSE in svc_recv/svc_handle_xprt,
knfsd will delete the xprt by svc_delete_xprt.
In svc_delete_xprt, knfsd calls call_xpt_users to notify those users of the xprt.
So, nfsd4_conn_lost will be called to free the connection.
After freeing connection, nfsd4_probe_callback will update the callback for the client.
And, the clp->cl_cb_client will be shutdown in nfsd4_process_cb_update.
At last, all using of the xprt will be released.
I have test it, that's OK.
> The xprt->count shouldn?t be an issue here:
> it has nothing to do with the socket connection state.
The xprt of backchannel, there are two references,
1. rpc_clnt
2. svc_xprt
For rpc_clnt, initialize the xprt->count in xs_setup_bc_tcp,
or increase it in create_backchannel_client, and, decrease in rpc_free_client.
For svc_xprt, increase it in xs_setup_bc_tcp, and decrease in svc_xprt_free.
thanks,
Kinglong Mee
On Mon, Jan 06, 2014 at 05:33:22PM +0800, Kinglong Mee wrote:
> xs_setup_bc_tcp may return an existing xprt with non-NULL servername.
> xprt_create_transport should not kstrdup servername for it.
> Otherwise, those memory for servername will be leaked.
OK. Applying to my tree if Trond has no objection.
--b.
>
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> net/sunrpc/xprt.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> index ddd198e..6fa966f 100644
> --- a/net/sunrpc/xprt.c
> +++ b/net/sunrpc/xprt.c
> @@ -1339,7 +1339,11 @@ found:
> xprt_destroy(xprt);
> return ERR_PTR(-EINVAL);
> }
> - xprt->servername = kstrdup(args->servername, GFP_KERNEL);
> +
> + /* servername may not be NULL for tcp NFSv4.1 backchannel */
> + if (xprt->servername == NULL)
> + xprt->servername = kstrdup(args->servername, GFP_KERNEL);
> +
> if (xprt->servername == NULL) {
> xprt_destroy(xprt);
> return ERR_PTR(-ENOMEM);
> --
> 1.8.4.2
On 01/07/2014 07:28 AM, Trond Myklebust wrote:
>
> On Jan 6, 2014, at 17:53, Dr Fields James Bruce <[email protected]> wrote:
>
>> On Mon, Jan 06, 2014 at 05:40:03PM -0500, Trond Myklebust wrote:
>>>
>>> On Jan 6, 2014, at 13:49, J. Bruce Fields <[email protected]> wrote:
>>>
>>>> On Mon, Jan 06, 2014 at 05:33:22PM +0800, Kinglong Mee wrote:
>>>>> xs_setup_bc_tcp may return an existing xprt with non-NULL servername.
>>>>> xprt_create_transport should not kstrdup servername for it.
>>>>> Otherwise, those memory for servername will be leaked.
>>>>
>>>> OK. Applying to my tree if Trond has no objection.
>>>
>>> Actually. Why do we go through all this code at all if xs_setup_bc_tcp() returns args->bc_xprt->xpt_bc_xprt? I?m assuming that is the only case where xprt->servername != NULL, right?
>>>
>>> For instance, won?t calling INIT_WORK() be a source of problems?
I will have a check for INIT_WORK which I have missing here.
>>
>> Huh. Looking at the history.... There used to be a
>>
>> if (test_and_set_bit(XPRT_INITIALIZED, &xprt->state))
>> /* ->setup returned a pre-initialized xprt: */
>> return xprt;
>>
>> here, but it got removed by 21de0a955f3af29fa1100d96f66e6adade89e77a
>> "SUNRPC: Clean up the slot table allocation", which looks otherwise
>> unrelated. Was that just some kind of rebasing mistake, or was there a
>> reason for that?
>
> I probably misunderstood that bc_xprt sends a fully initialized struct rpc_xprt.
>
> The obvious question if that is the case, is why we are calling xprt_create_transport() at all? If .bc_xprt->xpt_bc_xprt contains a fully initialized struct rpc_xprt, then just have rpc_create() do the honors.
> Better yet, create a svc_create_backchannel_client() helper that calls rpc_new_client() with the correct parameters. I really don?t like those rpc_create_args hacks that introduce fields that are completely private to nfsd.
Maybe should redesign the xs_setup_bc_tcp, because I have meet a new bug.
the xprt for backchannel don't be freed at all, and those memory is leaked,
as,
--> free_conn
--> svc_xprt_put
--> svc_xprt_free
--> xprt_put
--> xprt_destroy
--> xprt->ops->destroy(xprt);
--> bc_destory
but, bc_destroy is empty as,
2541 /*
2542 * The xprt destroy routine. Again, because this connection is client
2543 * initiated, we do nothing
2544 */
2545
2546 static void bc_destroy(struct rpc_xprt *xprt)
2547 {
2548 }
so, after that, the memory of xprt for back channel is leaked.
thanks,
Kinglong Mee
Signed-off-by: Kinglong Mee <[email protected]>
---
include/linux/sunrpc/clnt.h | 2 ++
net/sunrpc/clnt.c | 58 ++++++++++++++++++++++++++-------------------
2 files changed, 35 insertions(+), 25 deletions(-)
diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index 8af2804..bb9a1fa 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -130,6 +130,8 @@ struct rpc_create_args {
#define RPC_CLNT_CREATE_NO_RETRANS_TIMEOUT (1UL << 9)
struct rpc_clnt *rpc_create(struct rpc_create_args *args);
+struct rpc_clnt *rpc_create_xprt(struct rpc_create_args *args,
+ struct rpc_xprt *xprt);
struct rpc_clnt *rpc_bind_new_program(struct rpc_clnt *,
const struct rpc_program *, u32);
void rpc_task_reset_client(struct rpc_task *task, struct rpc_clnt *clnt);
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index b9276a6..c70da0e 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -438,6 +438,38 @@ out_no_rpciod:
return ERR_PTR(err);
}
+struct rpc_clnt *rpc_create_xprt(struct rpc_create_args *args,
+ struct rpc_xprt *xprt)
+{
+ struct rpc_clnt *clnt;
+
+ clnt = rpc_new_client(args, xprt, NULL);
+ if (IS_ERR(clnt))
+ return clnt;
+
+ if (!(args->flags & RPC_CLNT_CREATE_NOPING)) {
+ int err = rpc_ping(clnt);
+ if (err != 0) {
+ rpc_shutdown_client(clnt);
+ return ERR_PTR(err);
+ }
+ }
+
+ clnt->cl_softrtry = 1;
+ if (args->flags & RPC_CLNT_CREATE_HARDRTRY)
+ clnt->cl_softrtry = 0;
+
+ if (args->flags & RPC_CLNT_CREATE_AUTOBIND)
+ clnt->cl_autobind = 1;
+ if (args->flags & RPC_CLNT_CREATE_DISCRTRY)
+ clnt->cl_discrtry = 1;
+ if (!(args->flags & RPC_CLNT_CREATE_QUIET))
+ clnt->cl_chatty = 1;
+
+ return clnt;
+}
+EXPORT_SYMBOL_GPL(rpc_create_xprt);
+
/**
* rpc_create - create an RPC client and transport with one call
* @args: rpc_clnt create argument structure
@@ -451,7 +483,6 @@ out_no_rpciod:
struct rpc_clnt *rpc_create(struct rpc_create_args *args)
{
struct rpc_xprt *xprt;
- struct rpc_clnt *clnt;
struct xprt_create xprtargs = {
.net = args->net,
.ident = args->protocol,
@@ -515,30 +546,7 @@ struct rpc_clnt *rpc_create(struct rpc_create_args *args)
if (args->flags & RPC_CLNT_CREATE_NONPRIVPORT)
xprt->resvport = 0;
- clnt = rpc_new_client(args, xprt, NULL);
- if (IS_ERR(clnt))
- return clnt;
-
- if (!(args->flags & RPC_CLNT_CREATE_NOPING)) {
- int err = rpc_ping(clnt);
- if (err != 0) {
- rpc_shutdown_client(clnt);
- return ERR_PTR(err);
- }
- }
-
- clnt->cl_softrtry = 1;
- if (args->flags & RPC_CLNT_CREATE_HARDRTRY)
- clnt->cl_softrtry = 0;
-
- if (args->flags & RPC_CLNT_CREATE_AUTOBIND)
- clnt->cl_autobind = 1;
- if (args->flags & RPC_CLNT_CREATE_DISCRTRY)
- clnt->cl_discrtry = 1;
- if (!(args->flags & RPC_CLNT_CREATE_QUIET))
- clnt->cl_chatty = 1;
-
- return clnt;
+ return rpc_create_xprt(args, xprt);
}
EXPORT_SYMBOL_GPL(rpc_create);
--
1.8.4.2
On Thu, Jan 09, 2014 at 06:33:10PM +0800, Kinglong Mee wrote:
> Besides checking rpc_xprt out of xs_setup_bc_tcp,
> increase it's reference (it's important).
This sounds wrong to me: the presence of a backchannel can't prevent the
client's connection from going away. Instead, when the connection dies,
any associated backchannels should be immediately destroyed.
--b.
>
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> fs/nfsd/nfs4callback.c | 19 ++++++++++++++++++-
> include/linux/sunrpc/xprt.h | 13 ++++++++++++-
> net/sunrpc/xprt.c | 12 ------------
> net/sunrpc/xprtsock.c | 9 ---------
> 4 files changed, 30 insertions(+), 23 deletions(-)
>
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index 7f05cd1..39c8ef8 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -32,6 +32,7 @@
> */
>
> #include <linux/sunrpc/clnt.h>
> +#include <linux/sunrpc/xprt.h>
> #include <linux/sunrpc/svc_xprt.h>
> #include <linux/slab.h>
> #include "nfsd.h"
> @@ -635,6 +636,22 @@ static struct rpc_cred *get_backchannel_cred(struct nfs4_client *clp, struct rpc
> }
> }
>
> +static struct rpc_clnt *create_backchannel_client(struct rpc_create_args *args)
> +{
> + struct rpc_xprt *xprt;
> +
> + if (args->protocol != XPRT_TRANSPORT_BC_TCP)
> + return rpc_create(args);
> +
> + xprt = args->bc_xprt->xpt_bc_xprt;
> + if (xprt) {
> + xprt_get(xprt);
> + return rpc_create_xprt(args, xprt);
> + }
> +
> + return rpc_create(args);
> +}
> +
> static int setup_callback_client(struct nfs4_client *clp, struct nfs4_cb_conn *conn, struct nfsd4_session *ses)
> {
> struct rpc_timeout timeparms = {
> @@ -674,7 +691,7 @@ static int setup_callback_client(struct nfs4_client *clp, struct nfs4_cb_conn *c
> args.authflavor = ses->se_cb_sec.flavor;
> }
> /* Create RPC client */
> - client = rpc_create(&args);
> + client = create_backchannel_client(&args);
> if (IS_ERR(client)) {
> dprintk("NFSD: couldn't create callback client: %ld\n",
> PTR_ERR(client));
> diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
> index 8097b9d..3e5efb2 100644
> --- a/include/linux/sunrpc/xprt.h
> +++ b/include/linux/sunrpc/xprt.h
> @@ -295,13 +295,24 @@ int xprt_adjust_timeout(struct rpc_rqst *req);
> void xprt_release_xprt(struct rpc_xprt *xprt, struct rpc_task *task);
> void xprt_release_xprt_cong(struct rpc_xprt *xprt, struct rpc_task *task);
> void xprt_release(struct rpc_task *task);
> -struct rpc_xprt * xprt_get(struct rpc_xprt *xprt);
> void xprt_put(struct rpc_xprt *xprt);
> struct rpc_xprt * xprt_alloc(struct net *net, size_t size,
> unsigned int num_prealloc,
> unsigned int max_req);
> void xprt_free(struct rpc_xprt *);
>
> +/**
> + * xprt_get - return a reference to an RPC transport.
> + * @xprt: pointer to the transport
> + *
> + */
> +static inline struct rpc_xprt *xprt_get(struct rpc_xprt *xprt)
> +{
> + if (atomic_inc_not_zero(&xprt->count))
> + return xprt;
> + return NULL;
> +}
> +
> static inline __be32 *xprt_skip_transport_header(struct rpc_xprt *xprt, __be32 *p)
> {
> return p + xprt->tsh_size;
> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> index ddd198e..b0a1bbb 100644
> --- a/net/sunrpc/xprt.c
> +++ b/net/sunrpc/xprt.c
> @@ -1383,15 +1383,3 @@ void xprt_put(struct rpc_xprt *xprt)
> if (atomic_dec_and_test(&xprt->count))
> xprt_destroy(xprt);
> }
> -
> -/**
> - * xprt_get - return a reference to an RPC transport.
> - * @xprt: pointer to the transport
> - *
> - */
> -struct rpc_xprt *xprt_get(struct rpc_xprt *xprt)
> -{
> - if (atomic_inc_not_zero(&xprt->count))
> - return xprt;
> - return NULL;
> -}
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 7289e3c..37ea15a 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -2923,15 +2923,6 @@ static struct rpc_xprt *xs_setup_bc_tcp(struct xprt_create *args)
> struct svc_sock *bc_sock;
> struct rpc_xprt *ret;
>
> - if (args->bc_xprt->xpt_bc_xprt) {
> - /*
> - * This server connection already has a backchannel
> - * transport; we can't create a new one, as we wouldn't
> - * be able to match replies based on xid any more. So,
> - * reuse the already-existing one:
> - */
> - return args->bc_xprt->xpt_bc_xprt;
> - }
> xprt = xs_setup_xprt(args, xprt_tcp_slot_table_entries,
> xprt_tcp_slot_table_entries);
> if (IS_ERR(xprt))
> --
> 1.8.4.2
When using pynfs test-site, I get some memory leak for the backchannel.
1,2,4,5 are bugfix, 3 adds a helper function for 4.
Kinglong Mee (5):
NFSD: Using free_conn free connection
NFSD: Free backchannel xprt in bc_destroy
SUNRPC: New helper for creating client with rpc_xprt
NFSD/SUNRPC: Check rpc_xprt out of xs_setup_bc_tcp
SUNRPC: Clear xpt_bc_xprt if xs_setup_bc_tcp failed
fs/nfsd/nfs4callback.c | 19 ++++++++++++++-
fs/nfsd/nfs4state.c | 3 ++-
include/linux/sunrpc/clnt.h | 2 ++
include/linux/sunrpc/xprt.h | 13 +++++++++-
net/sunrpc/clnt.c | 58 ++++++++++++++++++++++++++-------------------
net/sunrpc/xprt.c | 12 ----------
net/sunrpc/xprtsock.c | 16 +++++--------
7 files changed, 73 insertions(+), 50 deletions(-)
--
1.8.4.2
Besides checking rpc_xprt out of xs_setup_bc_tcp,
increase it's reference (it's important).
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4callback.c | 19 ++++++++++++++++++-
include/linux/sunrpc/xprt.h | 13 ++++++++++++-
net/sunrpc/xprt.c | 12 ------------
net/sunrpc/xprtsock.c | 9 ---------
4 files changed, 30 insertions(+), 23 deletions(-)
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 7f05cd1..39c8ef8 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -32,6 +32,7 @@
*/
#include <linux/sunrpc/clnt.h>
+#include <linux/sunrpc/xprt.h>
#include <linux/sunrpc/svc_xprt.h>
#include <linux/slab.h>
#include "nfsd.h"
@@ -635,6 +636,22 @@ static struct rpc_cred *get_backchannel_cred(struct nfs4_client *clp, struct rpc
}
}
+static struct rpc_clnt *create_backchannel_client(struct rpc_create_args *args)
+{
+ struct rpc_xprt *xprt;
+
+ if (args->protocol != XPRT_TRANSPORT_BC_TCP)
+ return rpc_create(args);
+
+ xprt = args->bc_xprt->xpt_bc_xprt;
+ if (xprt) {
+ xprt_get(xprt);
+ return rpc_create_xprt(args, xprt);
+ }
+
+ return rpc_create(args);
+}
+
static int setup_callback_client(struct nfs4_client *clp, struct nfs4_cb_conn *conn, struct nfsd4_session *ses)
{
struct rpc_timeout timeparms = {
@@ -674,7 +691,7 @@ static int setup_callback_client(struct nfs4_client *clp, struct nfs4_cb_conn *c
args.authflavor = ses->se_cb_sec.flavor;
}
/* Create RPC client */
- client = rpc_create(&args);
+ client = create_backchannel_client(&args);
if (IS_ERR(client)) {
dprintk("NFSD: couldn't create callback client: %ld\n",
PTR_ERR(client));
diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
index 8097b9d..3e5efb2 100644
--- a/include/linux/sunrpc/xprt.h
+++ b/include/linux/sunrpc/xprt.h
@@ -295,13 +295,24 @@ int xprt_adjust_timeout(struct rpc_rqst *req);
void xprt_release_xprt(struct rpc_xprt *xprt, struct rpc_task *task);
void xprt_release_xprt_cong(struct rpc_xprt *xprt, struct rpc_task *task);
void xprt_release(struct rpc_task *task);
-struct rpc_xprt * xprt_get(struct rpc_xprt *xprt);
void xprt_put(struct rpc_xprt *xprt);
struct rpc_xprt * xprt_alloc(struct net *net, size_t size,
unsigned int num_prealloc,
unsigned int max_req);
void xprt_free(struct rpc_xprt *);
+/**
+ * xprt_get - return a reference to an RPC transport.
+ * @xprt: pointer to the transport
+ *
+ */
+static inline struct rpc_xprt *xprt_get(struct rpc_xprt *xprt)
+{
+ if (atomic_inc_not_zero(&xprt->count))
+ return xprt;
+ return NULL;
+}
+
static inline __be32 *xprt_skip_transport_header(struct rpc_xprt *xprt, __be32 *p)
{
return p + xprt->tsh_size;
diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index ddd198e..b0a1bbb 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -1383,15 +1383,3 @@ void xprt_put(struct rpc_xprt *xprt)
if (atomic_dec_and_test(&xprt->count))
xprt_destroy(xprt);
}
-
-/**
- * xprt_get - return a reference to an RPC transport.
- * @xprt: pointer to the transport
- *
- */
-struct rpc_xprt *xprt_get(struct rpc_xprt *xprt)
-{
- if (atomic_inc_not_zero(&xprt->count))
- return xprt;
- return NULL;
-}
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 7289e3c..37ea15a 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2923,15 +2923,6 @@ static struct rpc_xprt *xs_setup_bc_tcp(struct xprt_create *args)
struct svc_sock *bc_sock;
struct rpc_xprt *ret;
- if (args->bc_xprt->xpt_bc_xprt) {
- /*
- * This server connection already has a backchannel
- * transport; we can't create a new one, as we wouldn't
- * be able to match replies based on xid any more. So,
- * reuse the already-existing one:
- */
- return args->bc_xprt->xpt_bc_xprt;
- }
xprt = xs_setup_xprt(args, xprt_tcp_slot_table_entries,
xprt_tcp_slot_table_entries);
if (IS_ERR(xprt))
--
1.8.4.2
On Mon, Jan 06, 2014 at 06:28:38PM -0500, Trond Myklebust wrote:
>
> On Jan 6, 2014, at 17:53, Dr Fields James Bruce <[email protected]>
> wrote:
>
> > On Mon, Jan 06, 2014 at 05:40:03PM -0500, Trond Myklebust wrote:
> >>
> >> On Jan 6, 2014, at 13:49, J. Bruce Fields <[email protected]>
> >> wrote:
> >>
> >>> On Mon, Jan 06, 2014 at 05:33:22PM +0800, Kinglong Mee wrote:
> >>>> xs_setup_bc_tcp may return an existing xprt with non-NULL
> >>>> servername. xprt_create_transport should not kstrdup servername
> >>>> for it. Otherwise, those memory for servername will be leaked.
> >>>
> >>> OK. Applying to my tree if Trond has no objection.
> >>
> >> Actually. Why do we go through all this code at all if
> >> xs_setup_bc_tcp() returns args->bc_xprt->xpt_bc_xprt? I’m assuming
> >> that is the only case where xprt->servername != NULL, right?
> >>
> >> For instance, won’t calling INIT_WORK() be a source of problems?
> >
> > Huh. Looking at the history.... There used to be a
> >
> > if (test_and_set_bit(XPRT_INITIALIZED, &xprt->state)) /* ->setup
> > returned a pre-initialized xprt: */ return xprt;
> >
> > here, but it got removed by 21de0a955f3af29fa1100d96f66e6adade89e77a
> > "SUNRPC: Clean up the slot table allocation", which looks otherwise
> > unrelated. Was that just some kind of rebasing mistake, or was
> > there a reason for that?
>
> I probably misunderstood that bc_xprt sends a fully initialized struct
> rpc_xprt.
>
> The obvious question if that is the case, is why we are calling
> xprt_create_transport() at all? If .bc_xprt->xpt_bc_xprt contains a
> fully initialized struct rpc_xprt, then just have rpc_create() do the
> honors. Better yet, create a svc_create_backchannel_client() helper
> that calls rpc_new_client() with the correct parameters.
Yes, that sounds great. Looking at rpc_create, it could be that
everything there outside the rpc_new_client call is useless.
--b.
> I really
> don’t like those rpc_create_args hacks that introduce fields that are
> completely private to nfsd.
Apologies, I completely dropped this and now I've lost the thread of the
discussion.
What is your most recent patchset?
--b.
On Fri, Jan 10, 2014 at 10:41:24AM +0800, Kinglong Mee wrote:
> On 01/10/2014 01:27 AM, Trond Myklebust wrote:
> >
> > On Jan 9, 2014, at 11:26, Dr Fields James Bruce <[email protected]> wrote:
> >
> >> On Thu, Jan 09, 2014 at 06:33:10PM +0800, Kinglong Mee wrote:
> >>> Besides checking rpc_xprt out of xs_setup_bc_tcp,
> >>> increase it's reference (it's important).
> >>
> >> This sounds wrong to me: the presence of a backchannel can't prevent the
> >> client's connection from going away. Instead, when the connection dies,
> >> any associated backchannels should be immediately destroyed.
> >
> > Hi Bruce,
> >
> > The right way to deal with this is to have knfsd shut down the rpc_client
> > when it detects the TCP disconnection event.
>
> Yes, that's right.
> Knfsd has do it as you said.
>
> When getting xprt's status of XPT_CLOSE in svc_recv/svc_handle_xprt,
> knfsd will delete the xprt by svc_delete_xprt.
>
> In svc_delete_xprt, knfsd calls call_xpt_users to notify those users of the xprt.
> So, nfsd4_conn_lost will be called to free the connection.
> After freeing connection, nfsd4_probe_callback will update the callback for the client.
> And, the clp->cl_cb_client will be shutdown in nfsd4_process_cb_update.
>
> At last, all using of the xprt will be released.
> I have test it, that's OK.
>
> > The xprt->count shouldn’t be an issue here:
> > it has nothing to do with the socket connection state.
>
> The xprt of backchannel, there are two references,
> 1. rpc_clnt
> 2. svc_xprt
>
> For rpc_clnt, initialize the xprt->count in xs_setup_bc_tcp,
> or increase it in create_backchannel_client, and, decrease in rpc_free_client.
>
> For svc_xprt, increase it in xs_setup_bc_tcp, and decrease in svc_xprt_free.
>
> thanks,
> Kinglong Mee
>
Don't move the assign of args->bc_xprt->xpt_bc_xprt out of xs_setup_bc_tcp,
because rpc_ping (which is in rpc_create) will using it.
Signed-off-by: Kinglong Mee <[email protected]>
---
net/sunrpc/xprtsock.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 37ea15a..c996ace 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2981,9 +2981,10 @@ static struct rpc_xprt *xs_setup_bc_tcp(struct xprt_create *args)
*/
xprt_set_connected(xprt);
-
if (try_module_get(THIS_MODULE))
return xprt;
+
+ args->bc_xprt->xpt_bc_xprt = NULL;
xprt_put(xprt);
ret = ERR_PTR(-EINVAL);
out_err:
--
1.8.4.2
On Mon, Jan 06, 2014 at 05:40:03PM -0500, Trond Myklebust wrote:
>
> On Jan 6, 2014, at 13:49, J. Bruce Fields <[email protected]> wrote:
>
> > On Mon, Jan 06, 2014 at 05:33:22PM +0800, Kinglong Mee wrote:
> >> xs_setup_bc_tcp may return an existing xprt with non-NULL servername.
> >> xprt_create_transport should not kstrdup servername for it.
> >> Otherwise, those memory for servername will be leaked.
> >
> > OK. Applying to my tree if Trond has no objection.
>
> Actually. Why do we go through all this code at all if xs_setup_bc_tcp() returns args->bc_xprt->xpt_bc_xprt? I’m assuming that is the only case where xprt->servername != NULL, right?
>
> For instance, won’t calling INIT_WORK() be a source of problems?
Huh. Looking at the history.... There used to be a
if (test_and_set_bit(XPRT_INITIALIZED, &xprt->state))
/* ->setup returned a pre-initialized xprt: */
return xprt;
here, but it got removed by 21de0a955f3af29fa1100d96f66e6adade89e77a
"SUNRPC: Clean up the slot table allocation", which looks otherwise
unrelated. Was that just some kind of rebasing mistake, or was there a
reason for that?
--b.
>
> >
> > --b.
> >
> >>
> >> Signed-off-by: Kinglong Mee <[email protected]>
> >> ---
> >> net/sunrpc/xprt.c | 6 +++++-
> >> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> >> index ddd198e..6fa966f 100644
> >> --- a/net/sunrpc/xprt.c
> >> +++ b/net/sunrpc/xprt.c
> >> @@ -1339,7 +1339,11 @@ found:
> >> xprt_destroy(xprt);
> >> return ERR_PTR(-EINVAL);
> >> }
> >> - xprt->servername = kstrdup(args->servername, GFP_KERNEL);
> >> +
> >> + /* servername may not be NULL for tcp NFSv4.1 backchannel */
> >> + if (xprt->servername == NULL)
> >> + xprt->servername = kstrdup(args->servername, GFP_KERNEL);
> >> +
> >> if (xprt->servername == NULL) {
> >> xprt_destroy(xprt);
> >> return ERR_PTR(-ENOMEM);
> >> --
> >> 1.8.4.2
>
Backchannel xprt isnot freed right now.
Free it in bc_destroy, and put the reference of THIS_MODULE.
Signed-off-by: Kinglong Mee <[email protected]>
---
net/sunrpc/xprtsock.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 4fcdf74..7289e3c 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2545,6 +2545,10 @@ static void bc_close(struct rpc_xprt *xprt)
static void bc_destroy(struct rpc_xprt *xprt)
{
+ dprintk("RPC: bc_destroy xprt %p\n", xprt);
+
+ xs_xprt_free(xprt);
+ module_put(THIS_MODULE);
}
static struct rpc_xprt_ops xs_local_ops = {
--
1.8.4.2
Connection from alloc_conn must be freed through free_conn,
otherwise, the reference of svc_xprt will never be put.
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4state.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 7d613a7..f89f1f8 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2282,7 +2282,8 @@ out:
if (!list_empty(&clp->cl_revoked))
seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED;
out_no_session:
- kfree(conn);
+ if (conn)
+ free_conn(conn);
spin_unlock(&nn->client_lock);
return status;
out_put_session:
--
1.8.4.2
On Jan 6, 2014, at 13:49, J. Bruce Fields <[email protected]> wrote:
> On Mon, Jan 06, 2014 at 05:33:22PM +0800, Kinglong Mee wrote:
>> xs_setup_bc_tcp may return an existing xprt with non-NULL servername.
>> xprt_create_transport should not kstrdup servername for it.
>> Otherwise, those memory for servername will be leaked.
>
> OK. Applying to my tree if Trond has no objection.
Actually. Why do we go through all this code at all if xs_setup_bc_tcp() returns args->bc_xprt->xpt_bc_xprt? I?m assuming that is the only case where xprt->servername != NULL, right?
For instance, won?t calling INIT_WORK() be a source of problems?
>
> --b.
>
>>
>> Signed-off-by: Kinglong Mee <[email protected]>
>> ---
>> net/sunrpc/xprt.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
>> index ddd198e..6fa966f 100644
>> --- a/net/sunrpc/xprt.c
>> +++ b/net/sunrpc/xprt.c
>> @@ -1339,7 +1339,11 @@ found:
>> xprt_destroy(xprt);
>> return ERR_PTR(-EINVAL);
>> }
>> - xprt->servername = kstrdup(args->servername, GFP_KERNEL);
>> +
>> + /* servername may not be NULL for tcp NFSv4.1 backchannel */
>> + if (xprt->servername == NULL)
>> + xprt->servername = kstrdup(args->servername, GFP_KERNEL);
>> +
>> if (xprt->servername == NULL) {
>> xprt_destroy(xprt);
>> return ERR_PTR(-ENOMEM);
>> --
>> 1.8.4.2
Dear Bruce,
I am sorry for replying so late.
I will resend those patchs again.
thanks,
Kinglong Mee
On Tue, Jan 28, 2014 at 7:08 AM, Dr Fields James Bruce
<[email protected]> wrote:
> Apologies, I completely dropped this and now I've lost the thread of the
> discussion.
>
> What is your most recent patchset?
>
> --b.
>
> On Fri, Jan 10, 2014 at 10:41:24AM +0800, Kinglong Mee wrote:
>> On 01/10/2014 01:27 AM, Trond Myklebust wrote:
>> >
>> > On Jan 9, 2014, at 11:26, Dr Fields James Bruce <[email protected]> wrote:
>> >
>> >> On Thu, Jan 09, 2014 at 06:33:10PM +0800, Kinglong Mee wrote:
>> >>> Besides checking rpc_xprt out of xs_setup_bc_tcp,
>> >>> increase it's reference (it's important).
>> >>
>> >> This sounds wrong to me: the presence of a backchannel can't prevent the
>> >> client's connection from going away. Instead, when the connection dies,
>> >> any associated backchannels should be immediately destroyed.
>> >
>> > Hi Bruce,
>> >
>> > The right way to deal with this is to have knfsd shut down the rpc_client
>> > when it detects the TCP disconnection event.
>>
>> Yes, that's right.
>> Knfsd has do it as you said.
>>
>> When getting xprt's status of XPT_CLOSE in svc_recv/svc_handle_xprt,
>> knfsd will delete the xprt by svc_delete_xprt.
>>
>> In svc_delete_xprt, knfsd calls call_xpt_users to notify those users of the xprt.
>> So, nfsd4_conn_lost will be called to free the connection.
>> After freeing connection, nfsd4_probe_callback will update the callback for the client.
>> And, the clp->cl_cb_client will be shutdown in nfsd4_process_cb_update.
>>
>> At last, all using of the xprt will be released.
>> I have test it, that's OK.
>>
>> > The xprt->count shouldn't be an issue here:
>> > it has nothing to do with the socket connection state.
>>
>> The xprt of backchannel, there are two references,
>> 1. rpc_clnt
>> 2. svc_xprt
>>
>> For rpc_clnt, initialize the xprt->count in xs_setup_bc_tcp,
>> or increase it in create_backchannel_client, and, decrease in rpc_free_client.
>>
>> For svc_xprt, increase it in xs_setup_bc_tcp, and decrease in svc_xprt_free.
>>
>> thanks,
>> Kinglong Mee
>>