If try_module_get failed, xpt_bc_xprt should be set to NULL,
because xprt will be free.
Don't needed using xprt_put to free xprt, because it is always new.
Signed-off-by: Kinglong Mee <[email protected]>
---
net/sunrpc/xprtsock.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 4fcdf74..5ed124f 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2986,10 +2986,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;
- xprt_put(xprt);
+
+ args->bc_xprt->xpt_bc_xprt = NULL;
ret = ERR_PTR(-EINVAL);
out_err:
xs_xprt_free(xprt);
--
1.8.4.2
On 01/10/2014 10:43 AM, Kinglong Mee wrote:
> On 01/10/2014 12:34 AM, Dr Fields James Bruce wrote:
>> On Tue, Jan 07, 2014 at 03:26:04PM +0800, Kinglong Mee wrote:
>>> On 01/07/2014 01:01 PM, Trond Myklebust wrote:
>>>>
>>>> On Jan 6, 2014, at 23:41, Kinglong Mee <[email protected]> wrote:
>>>>
>>>>> If try_module_get failed, xpt_bc_xprt should be set to NULL,
>>>>> because xprt will be free.
>>>>>
>>>>> Don't needed using xprt_put to free xprt, because it is always new.
>>>>>
>>>>> Signed-off-by: Kinglong Mee <[email protected]>
>>>>> ---
>>>>> net/sunrpc/xprtsock.c | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>>>>> index 4fcdf74..5ed124f 100644
>>>>> --- a/net/sunrpc/xprtsock.c
>>>>> +++ b/net/sunrpc/xprtsock.c
>>>>> @@ -2986,10 +2986,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;
>>>>> - xprt_put(xprt);
>>>>> +
>>>>> + args->bc_xprt->xpt_bc_xprt = NULL;
>>>>> ret = ERR_PTR(-EINVAL);
>>>>> out_err:
>>>>> xs_xprt_free(xprt);
>>>>
>>>> Just move the 'args->bc_xprt->xpt_bc_xprt = xprt’ line into the caller (see earlier discussion about xs_setup_bc_tcp()). Quite frankly, I don’t see why the client code should be modifying this variable in the first place. An svc_xprt is a server construct.
>>>
>>> Hi Bruce,
>>>
>>> When reviewing those codes for bc_xprt, I found commit d75faea330dbd1873c9094e9926ae306590c0998
>>> 'rpc: move sk_bc_xprt to svc_xprt'. I'd like recording bc_xprt in svc_sock.
>>
>> OK, I think it would be alright to revert that commit.
>
> I will try to revert it later.
I had try to revert it, but, I don't revert it.
I'm sorry for my above thinking.
I think it is not needed to move bc_xprt from one place to another
without fix some problem, because knfsd running correctly with that.
Just let bc_xprt in svc_xprt until some problem appears.
Thanks,
Kinglong Mee
On 01/10/2014 12:34 AM, Dr Fields James Bruce wrote:
> On Tue, Jan 07, 2014 at 03:26:04PM +0800, Kinglong Mee wrote:
>> On 01/07/2014 01:01 PM, Trond Myklebust wrote:
>>>
>>> On Jan 6, 2014, at 23:41, Kinglong Mee <[email protected]> wrote:
>>>
>>>> If try_module_get failed, xpt_bc_xprt should be set to NULL,
>>>> because xprt will be free.
>>>>
>>>> Don't needed using xprt_put to free xprt, because it is always new.
>>>>
>>>> Signed-off-by: Kinglong Mee <[email protected]>
>>>> ---
>>>> net/sunrpc/xprtsock.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>>>> index 4fcdf74..5ed124f 100644
>>>> --- a/net/sunrpc/xprtsock.c
>>>> +++ b/net/sunrpc/xprtsock.c
>>>> @@ -2986,10 +2986,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;
>>>> - xprt_put(xprt);
>>>> +
>>>> + args->bc_xprt->xpt_bc_xprt = NULL;
>>>> ret = ERR_PTR(-EINVAL);
>>>> out_err:
>>>> xs_xprt_free(xprt);
>>>
>>> Just move the 'args->bc_xprt->xpt_bc_xprt = xprt’ line into the caller (see earlier discussion about xs_setup_bc_tcp()). Quite frankly, I don’t see why the client code should be modifying this variable in the first place. An svc_xprt is a server construct.
>>
>> Hi Bruce,
>>
>> When reviewing those codes for bc_xprt, I found commit d75faea330dbd1873c9094e9926ae306590c0998
>> 'rpc: move sk_bc_xprt to svc_xprt'. I'd like recording bc_xprt in svc_sock.
>
> OK, I think it would be alright to revert that commit.
I will try to revert it later.
Thanks,
Kinglong Mee
On 01/07/2014 01:01 PM, Trond Myklebust wrote:
>
> On Jan 6, 2014, at 23:41, Kinglong Mee <[email protected]> wrote:
>
>> If try_module_get failed, xpt_bc_xprt should be set to NULL,
>> because xprt will be free.
>>
>> Don't needed using xprt_put to free xprt, because it is always new.
>>
>> Signed-off-by: Kinglong Mee <[email protected]>
>> ---
>> net/sunrpc/xprtsock.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>> index 4fcdf74..5ed124f 100644
>> --- a/net/sunrpc/xprtsock.c
>> +++ b/net/sunrpc/xprtsock.c
>> @@ -2986,10 +2986,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;
>> - xprt_put(xprt);
>> +
>> + args->bc_xprt->xpt_bc_xprt = NULL;
>> ret = ERR_PTR(-EINVAL);
>> out_err:
>> xs_xprt_free(xprt);
>
> Just move the 'args->bc_xprt->xpt_bc_xprt = xprt? line into the caller (see earlier discussion about xs_setup_bc_tcp()). Quite frankly, I don?t see why the client code should be modifying this variable in the first place. An svc_xprt is a server construct.
Hi Bruce,
When reviewing those codes for bc_xprt, I found commit d75faea330dbd1873c9094e9926ae306590c0998
'rpc: move sk_bc_xprt to svc_xprt'. I'd like recording bc_xprt in svc_sock.
thanks,
Kinglong Mee
On 01/07/2014 01:01 PM, Trond Myklebust wrote:
>
> On Jan 6, 2014, at 23:41, Kinglong Mee <[email protected]> wrote:
>
>> If try_module_get failed, xpt_bc_xprt should be set to NULL,
>> because xprt will be free.
>>
>> Don't needed using xprt_put to free xprt, because it is always new.
>>
>> Signed-off-by: Kinglong Mee <[email protected]>
>> ---
>> net/sunrpc/xprtsock.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>> index 4fcdf74..5ed124f 100644
>> --- a/net/sunrpc/xprtsock.c
>> +++ b/net/sunrpc/xprtsock.c
>> @@ -2986,10 +2986,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;
>> - xprt_put(xprt);
>> +
>> + args->bc_xprt->xpt_bc_xprt = NULL;
>> ret = ERR_PTR(-EINVAL);
>> out_err:
>> xs_xprt_free(xprt);
>
>
> Just move the 'args->bc_xprt->xpt_bc_xprt = xprt? line into the caller
> (see earlier discussion about xs_setup_bc_tcp()).
> Quite frankly, I don?t see why the client code should be modifying this variable in the first place.
> An svc_xprt is a server construct.
That's really better.
I will check those codes for NFSv4.1 backchannel in sunrpc,
maybe I will cleanup the client code from svc_xprt.
thanks,
Kinglong Mee
On Jan 6, 2014, at 23:41, Kinglong Mee <[email protected]> wrote:
> If try_module_get failed, xpt_bc_xprt should be set to NULL,
> because xprt will be free.
>
> Don't needed using xprt_put to free xprt, because it is always new.
>
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> net/sunrpc/xprtsock.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 4fcdf74..5ed124f 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -2986,10 +2986,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;
> - xprt_put(xprt);
> +
> + args->bc_xprt->xpt_bc_xprt = NULL;
> ret = ERR_PTR(-EINVAL);
> out_err:
> xs_xprt_free(xprt);
Just move the 'args->bc_xprt->xpt_bc_xprt = xprt? line into the caller (see earlier discussion about xs_setup_bc_tcp()). Quite frankly, I don?t see why the client code should be modifying this variable in the first place. An svc_xprt is a server construct.
Trond
On Tue, Jan 07, 2014 at 03:26:04PM +0800, Kinglong Mee wrote:
> On 01/07/2014 01:01 PM, Trond Myklebust wrote:
> >
> > On Jan 6, 2014, at 23:41, Kinglong Mee <[email protected]> wrote:
> >
> >> If try_module_get failed, xpt_bc_xprt should be set to NULL,
> >> because xprt will be free.
> >>
> >> Don't needed using xprt_put to free xprt, because it is always new.
> >>
> >> Signed-off-by: Kinglong Mee <[email protected]>
> >> ---
> >> net/sunrpc/xprtsock.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> >> index 4fcdf74..5ed124f 100644
> >> --- a/net/sunrpc/xprtsock.c
> >> +++ b/net/sunrpc/xprtsock.c
> >> @@ -2986,10 +2986,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;
> >> - xprt_put(xprt);
> >> +
> >> + args->bc_xprt->xpt_bc_xprt = NULL;
> >> ret = ERR_PTR(-EINVAL);
> >> out_err:
> >> xs_xprt_free(xprt);
> >
> > Just move the 'args->bc_xprt->xpt_bc_xprt = xprt’ line into the caller (see earlier discussion about xs_setup_bc_tcp()). Quite frankly, I don’t see why the client code should be modifying this variable in the first place. An svc_xprt is a server construct.
>
> Hi Bruce,
>
> When reviewing those codes for bc_xprt, I found commit d75faea330dbd1873c9094e9926ae306590c0998
> 'rpc: move sk_bc_xprt to svc_xprt'. I'd like recording bc_xprt in svc_sock.
OK, I think it would be alright to revert that commit.
--b.