On 01/07/2014 07:28 AM, Trond Myklebust wrote:
>
> On Jan 6, 2014, at 17:53, Dr Fields James Bruce <bfields@xxxxxxxxxxxx> wrote:
>
>> On Mon, Jan 06, 2014 at 05:40:03PM -0500, Trond Myklebust wrote:
>>>
>>> On Jan 6, 2014, at 13:49, J. Bruce Fields <bfields@xxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
