diff mbox

[Version,7,6/8] SUNRPC add an RPC null call to test session trunking connection

Message ID 1469645000-19791-7-git-send-email-andros@netapp.com (mailing list archive)
State Changes Requested
Delegated to: Trond Myklebust
Headers show

Commit Message

Andy Adamson July 27, 2016, 6:43 p.m. UTC 
From: Andy Adamson <andros@netapp.com>

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 include/linux/sunrpc/clnt.h |  2 ++
 net/sunrpc/clnt.c           | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+)

Comments

Schumaker, Anna Aug. 4, 2016, 7:26 p.m. UTC | #1 
Hi Andy,

On 07/27/2016 02:43 PM, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
> 
> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  include/linux/sunrpc/clnt.h |  2 ++
>  net/sunrpc/clnt.c           | 18 ++++++++++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> index 99410bb..ebc83df 100644
> --- a/include/linux/sunrpc/clnt.h
> +++ b/include/linux/sunrpc/clnt.h
> @@ -189,6 +189,8 @@ int 		rpc_clnt_test_and_add_xprt(struct rpc_clnt *clnt,
>  			struct rpc_xprt_switch *xps,
>  			struct rpc_xprt *xprt,
>  			void *dummy);
> +int		rpc_clnt_test_xprt(struct rpc_clnt *clnt,
> +			struct rpc_xprt *xprt);
>  int		rpc_clnt_add_xprt(struct rpc_clnt *, struct xprt_create *,
>  			int (*setup)(struct rpc_clnt *,
>  				struct rpc_xprt_switch *,
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 459f9b1..822060f 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -2614,6 +2614,24 @@ int rpc_clnt_test_and_add_xprt(struct rpc_clnt *clnt,
>  }
>  EXPORT_SYMBOL_GPL(rpc_clnt_test_and_add_xprt);
>  
> +int rpc_clnt_test_xprt(struct rpc_clnt *clnt, struct rpc_xprt *xprt)

Looks like rpc_clnt_test_and_add_xprt() runs the same testing code.  Can you swap the order of these functions in clnt.c and then have test_and_add_xprt() call test_xprt()?

Thanks,
Anna

> +{
> +	struct rpc_cred *cred;
> +	struct rpc_task *task;
> +	int status;
> +
> +	cred = authnull_ops.lookup_cred(NULL, NULL, 0);
> +	task = rpc_call_null_helper(clnt, xprt, cred,
> +				RPC_TASK_SOFT | RPC_TASK_SOFTCONN, NULL, NULL);
> +	put_rpccred(cred);
> +	if (IS_ERR(task))
> +		return PTR_ERR(task);
> +	status = task->tk_status;
> +	rpc_put_task(task);
> +	return status;
> +}
> +EXPORT_SYMBOL_GPL(rpc_clnt_test_xprt);
> +
>  /**
>   * rpc_clnt_add_xprt - Add a new transport to a rpc_clnt
>   * @clnt: pointer to struct rpc_clnt
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Adamson, Andy Aug. 5, 2016, 4:41 p.m. UTC | #2 
> On Aug 4, 2016, at 3:26 PM, Schumaker, Anna <Anna.Schumaker@netapp.com> wrote:

> 

> Hi Andy,

> 

> On 07/27/2016 02:43 PM, andros@netapp.com wrote:

>> From: Andy Adamson <andros@netapp.com>

>> 

>> Signed-off-by: Andy Adamson <andros@netapp.com>

>> ---

>> include/linux/sunrpc/clnt.h |  2 ++

>> net/sunrpc/clnt.c           | 18 ++++++++++++++++++

>> 2 files changed, 20 insertions(+)

>> 

>> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h

>> index 99410bb..ebc83df 100644

>> --- a/include/linux/sunrpc/clnt.h

>> +++ b/include/linux/sunrpc/clnt.h

>> @@ -189,6 +189,8 @@ int 		rpc_clnt_test_and_add_xprt(struct rpc_clnt *clnt,

>> 			struct rpc_xprt_switch *xps,

>> 			struct rpc_xprt *xprt,

>> 			void *dummy);

>> +int		rpc_clnt_test_xprt(struct rpc_clnt *clnt,

>> +			struct rpc_xprt *xprt);

>> int		rpc_clnt_add_xprt(struct rpc_clnt *, struct xprt_create *,

>> 			int (*setup)(struct rpc_clnt *,

>> 				struct rpc_xprt_switch *,

>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c

>> index 459f9b1..822060f 100644

>> --- a/net/sunrpc/clnt.c

>> +++ b/net/sunrpc/clnt.c

>> @@ -2614,6 +2614,24 @@ int rpc_clnt_test_and_add_xprt(struct rpc_clnt *clnt,

>> }

>> EXPORT_SYMBOL_GPL(rpc_clnt_test_and_add_xprt);

>> 

>> +int rpc_clnt_test_xprt(struct rpc_clnt *clnt, struct rpc_xprt *xprt)

> 

> Looks like rpc_clnt_test_and_add_xprt() runs the same testing code.  Can you swap the order of these functions in clnt.c and then have test_and_add_xprt() call test_xprt()?


rpc_clnt_test_xprt uses a SYNC null call while test_and_add_xprt uses an ASYNC call. Futhermore, while both test_xprt and test_and_add_xprt return an error if the rpc_call_null_helper task cannot be allocated (ERR_PTR(task) in rpc_new_task), test_and_add_xprt ignores the tk_status and returns 1 while test_xprt returns the tk_status.

For these reasons I think it is cleaner and easier to read to keep them separate functions.

—>Andy
:
> 

> Thanks,

> Anna

> 

>> +{

>> +	struct rpc_cred *cred;

>> +	struct rpc_task *task;

>> +	int status;

>> +

>> +	cred = authnull_ops.lookup_cred(NULL, NULL, 0);

>> +	task = rpc_call_null_helper(clnt, xprt, cred,

>> +				RPC_TASK_SOFT | RPC_TASK_SOFTCONN, NULL, NULL);

>> +	put_rpccred(cred);

>> +	if (IS_ERR(task))

>> +		return PTR_ERR(task);

>> +	status = task->tk_status;

>> +	rpc_put_task(task);

>> +	return status;

>> +}

>> +EXPORT_SYMBOL_GPL(rpc_clnt_test_xprt);

>> +

>> /**

>>  * rpc_clnt_add_xprt - Add a new transport to a rpc_clnt

>>  * @clnt: pointer to struct rpc_clnt
Schumaker, Anna Aug. 5, 2016, 6:08 p.m. UTC | #3 
On 08/05/2016 12:41 PM, Adamson, Andy wrote:
> 
>> On Aug 4, 2016, at 3:26 PM, Schumaker, Anna <Anna.Schumaker@netapp.com> wrote:
>>
>> Hi Andy,
>>
>> On 07/27/2016 02:43 PM, andros@netapp.com wrote:
>>> From: Andy Adamson <andros@netapp.com>
>>>
>>> Signed-off-by: Andy Adamson <andros@netapp.com>
>>> ---
>>> include/linux/sunrpc/clnt.h |  2 ++
>>> net/sunrpc/clnt.c           | 18 ++++++++++++++++++
>>> 2 files changed, 20 insertions(+)
>>>
>>> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
>>> index 99410bb..ebc83df 100644
>>> --- a/include/linux/sunrpc/clnt.h
>>> +++ b/include/linux/sunrpc/clnt.h
>>> @@ -189,6 +189,8 @@ int 		rpc_clnt_test_and_add_xprt(struct rpc_clnt *clnt,
>>> 			struct rpc_xprt_switch *xps,
>>> 			struct rpc_xprt *xprt,
>>> 			void *dummy);
>>> +int		rpc_clnt_test_xprt(struct rpc_clnt *clnt,
>>> +			struct rpc_xprt *xprt);
>>> int		rpc_clnt_add_xprt(struct rpc_clnt *, struct xprt_create *,
>>> 			int (*setup)(struct rpc_clnt *,
>>> 				struct rpc_xprt_switch *,
>>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>>> index 459f9b1..822060f 100644
>>> --- a/net/sunrpc/clnt.c
>>> +++ b/net/sunrpc/clnt.c
>>> @@ -2614,6 +2614,24 @@ int rpc_clnt_test_and_add_xprt(struct rpc_clnt *clnt,
>>> }
>>> EXPORT_SYMBOL_GPL(rpc_clnt_test_and_add_xprt);
>>>
>>> +int rpc_clnt_test_xprt(struct rpc_clnt *clnt, struct rpc_xprt *xprt)
>>
>> Looks like rpc_clnt_test_and_add_xprt() runs the same testing code.  Can you swap the order of these functions in clnt.c and then have test_and_add_xprt() call test_xprt()?
> 
> rpc_clnt_test_xprt uses a SYNC null call while test_and_add_xprt uses an ASYNC call. Futhermore, while both test_xprt and test_and_add_xprt return an error if the rpc_call_null_helper task cannot be allocated (ERR_PTR(task) in rpc_new_task), test_and_add_xprt ignores the tk_status and returns 1 while test_xprt returns the tk_status.

Ah, I missed that one is sync and the other isn't.  Thanks for pointing that out!  I still wish there was a common function they both could tall to handle the cred lookup, but maybe that's not as easy as it seems.

Anna

> 
> For these reasons I think it is cleaner and easier to read to keep them separate functions.
> 
> —>Andy
> :
>>
>> Thanks,
>> Anna
>>
>>> +{
>>> +	struct rpc_cred *cred;
>>> +	struct rpc_task *task;
>>> +	int status;
>>> +
>>> +	cred = authnull_ops.lookup_cred(NULL, NULL, 0);
>>> +	task = rpc_call_null_helper(clnt, xprt, cred,
>>> +				RPC_TASK_SOFT | RPC_TASK_SOFTCONN, NULL, NULL);
>>> +	put_rpccred(cred);
>>> +	if (IS_ERR(task))
>>> +		return PTR_ERR(task);
>>> +	status = task->tk_status;
>>> +	rpc_put_task(task);
>>> +	return status;
>>> +}
>>> +EXPORT_SYMBOL_GPL(rpc_clnt_test_xprt);
>>> +
>>> /**
>>>  * rpc_clnt_add_xprt - Add a new transport to a rpc_clnt
>>>  * @clnt: pointer to struct rpc_clnt
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index 99410bb..ebc83df 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -189,6 +189,8 @@  int 		rpc_clnt_test_and_add_xprt(struct rpc_clnt *clnt,
 			struct rpc_xprt_switch *xps,
 			struct rpc_xprt *xprt,
 			void *dummy);
+int		rpc_clnt_test_xprt(struct rpc_clnt *clnt,
+			struct rpc_xprt *xprt);
 int		rpc_clnt_add_xprt(struct rpc_clnt *, struct xprt_create *,
 			int (*setup)(struct rpc_clnt *,
 				struct rpc_xprt_switch *,
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 459f9b1..822060f 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -2614,6 +2614,24 @@  int rpc_clnt_test_and_add_xprt(struct rpc_clnt *clnt,
 }
 EXPORT_SYMBOL_GPL(rpc_clnt_test_and_add_xprt);
 
+int rpc_clnt_test_xprt(struct rpc_clnt *clnt, struct rpc_xprt *xprt)
+{
+	struct rpc_cred *cred;
+	struct rpc_task *task;
+	int status;
+
+	cred = authnull_ops.lookup_cred(NULL, NULL, 0);
+	task = rpc_call_null_helper(clnt, xprt, cred,
+				RPC_TASK_SOFT | RPC_TASK_SOFTCONN, NULL, NULL);
+	put_rpccred(cred);
+	if (IS_ERR(task))
+		return PTR_ERR(task);
+	status = task->tk_status;
+	rpc_put_task(task);
+	return status;
+}
+EXPORT_SYMBOL_GPL(rpc_clnt_test_xprt);
+
 /**
  * rpc_clnt_add_xprt - Add a new transport to a rpc_clnt
  * @clnt: pointer to struct rpc_clnt