2014-01-20 07:00:14

by shaobingqing

[permalink] [raw]
Subject: [PATCH] SUNRPC: Allow one callback request to be received from two sk_buff

In current code, there only one struct rpc_rqst is prealloced. If one
callback request is received from two sk_buff, the xprt_alloc_bc_request
would be execute two times with the same transport->xid. The first time
xprt_alloc_bc_request will alloc one struct rpc_rqst and the TCP_RCV_COPY_DATA
bit of transport->tcp_flags will not be cleared. The second time
xprt_alloc_bc_request could not alloc struct rpc_rqst any more and NULL
pointer will be returned, then xprt_force_disconnect occur. I think one
callback request can be allowed to be received from two sk_buff.

Signed-off-by: shaobingqing <[email protected]>
---
net/sunrpc/xprtsock.c | 11 +++++++++--
1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index ee03d35..606950d 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1271,8 +1271,13 @@ static inline int xs_tcp_read_callback(struct rpc_xprt *xprt,
struct sock_xprt *transport =
container_of(xprt, struct sock_xprt, xprt);
struct rpc_rqst *req;
+ static struct rpc_rqst *req_partial;
+
+ if (req_partial == NULL)
+ req = xprt_alloc_bc_request(xprt);
+ else if (req_partial->rq_xid == transport->tcp_xid)
+ req = req_partial;

- req = xprt_alloc_bc_request(xprt);
if (req == NULL) {
printk(KERN_WARNING "Callback slot table overflowed\n");
xprt_force_disconnect(xprt);
@@ -1285,6 +1290,7 @@ static inline int xs_tcp_read_callback(struct rpc_xprt *xprt,

if (!(transport->tcp_flags & TCP_RCV_COPY_DATA)) {
struct svc_serv *bc_serv = xprt->bc_serv;
+ req_partial = NULL;

/*
* Add callback request to callback list. The callback
@@ -1297,7 +1303,8 @@ static inline int xs_tcp_read_callback(struct rpc_xprt *xprt,
list_add(&req->rq_bc_list, &bc_serv->sv_cb_list);
spin_unlock(&bc_serv->sv_cb_lock);
wake_up(&bc_serv->sv_cb_waitq);
- }
+ } else
+ req_partial = req;

req->rq_private_buf.len = transport->tcp_copied;

--
1.7.4.2


2014-01-20 14:27:26

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Allow one callback request to be received from two sk_buff

Hello.

On 20-01-2014 10:59, shaobingqing wrote:

> In current code, there only one struct rpc_rqst is prealloced. If one
> callback request is received from two sk_buff, the xprt_alloc_bc_request
> would be execute two times with the same transport->xid. The first time
> xprt_alloc_bc_request will alloc one struct rpc_rqst and the TCP_RCV_COPY_DATA
> bit of transport->tcp_flags will not be cleared. The second time
> xprt_alloc_bc_request could not alloc struct rpc_rqst any more and NULL
> pointer will be returned, then xprt_force_disconnect occur. I think one
> callback request can be allowed to be received from two sk_buff.

> Signed-off-by: shaobingqing <[email protected]>
> ---
> net/sunrpc/xprtsock.c | 11 +++++++++--
> 1 files changed, 9 insertions(+), 2 deletions(-)

> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index ee03d35..606950d 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
[...]
> @@ -1297,7 +1303,8 @@ static inline int xs_tcp_read_callback(struct rpc_xprt *xprt,
> list_add(&req->rq_bc_list, &bc_serv->sv_cb_list);
> spin_unlock(&bc_serv->sv_cb_lock);
> wake_up(&bc_serv->sv_cb_waitq);
> - }
> + } else
> + req_partial = req;

{} is needed in the *else* branch since it's already used in another
branch of *if* -- see Documentation/CodingStyle.

WBR, Sergei

2014-01-20 23:17:33

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Allow one callback request to be received from two sk_buff

On Mon, 2014-01-20 at 14:59 +0800, shaobingqing wrote:
> In current code, there only one struct rpc_rqst is prealloced. If one
> callback request is received from two sk_buff, the xprt_alloc_bc_request
> would be execute two times with the same transport->xid. The first time
> xprt_alloc_bc_request will alloc one struct rpc_rqst and the TCP_RCV_COPY_DATA
> bit of transport->tcp_flags will not be cleared. The second time
> xprt_alloc_bc_request could not alloc struct rpc_rqst any more and NULL
> pointer will be returned, then xprt_force_disconnect occur. I think one
> callback request can be allowed to be received from two sk_buff.
>
> Signed-off-by: shaobingqing <[email protected]>
> ---
> net/sunrpc/xprtsock.c | 11 +++++++++--
> 1 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index ee03d35..606950d 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -1271,8 +1271,13 @@ static inline int xs_tcp_read_callback(struct rpc_xprt *xprt,
> struct sock_xprt *transport =
> container_of(xprt, struct sock_xprt, xprt);
> struct rpc_rqst *req;
> + static struct rpc_rqst *req_partial;
> +
> + if (req_partial == NULL)
> + req = xprt_alloc_bc_request(xprt);
> + else if (req_partial->rq_xid == transport->tcp_xid)
> + req = req_partial;

What happens here if req_partial->rq_xid != transport->tcp_xid? AFAICS,
req will be undefined. Either way, you cannot use a static variable for
storage here: that isn't re-entrant.

> - req = xprt_alloc_bc_request(xprt);
> if (req == NULL) {
> printk(KERN_WARNING "Callback slot table overflowed\n");
> xprt_force_disconnect(xprt);
> @@ -1285,6 +1290,7 @@ static inline int xs_tcp_read_callback(struct rpc_xprt *xprt,
>
> if (!(transport->tcp_flags & TCP_RCV_COPY_DATA)) {
> struct svc_serv *bc_serv = xprt->bc_serv;
> + req_partial = NULL;
>
> /*
> * Add callback request to callback list. The callback
> @@ -1297,7 +1303,8 @@ static inline int xs_tcp_read_callback(struct rpc_xprt *xprt,
> list_add(&req->rq_bc_list, &bc_serv->sv_cb_list);
> spin_unlock(&bc_serv->sv_cb_lock);
> wake_up(&bc_serv->sv_cb_waitq);
> - }
> + } else
> + req_partial = req;
>
> req->rq_private_buf.len = transport->tcp_copied;
>


--
Trond Myklebust
Linux NFS client maintainer

2014-01-21 10:08:10

by shaobingqing

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Allow one callback request to be received from two sk_buff

2014/1/21 Trond Myklebust <[email protected]>:
> On Mon, 2014-01-20 at 14:59 +0800, shaobingqing wrote:
>> In current code, there only one struct rpc_rqst is prealloced. If one
>> callback request is received from two sk_buff, the xprt_alloc_bc_request
>> would be execute two times with the same transport->xid. The first time
>> xprt_alloc_bc_request will alloc one struct rpc_rqst and the TCP_RCV_COPY_DATA
>> bit of transport->tcp_flags will not be cleared. The second time
>> xprt_alloc_bc_request could not alloc struct rpc_rqst any more and NULL
>> pointer will be returned, then xprt_force_disconnect occur. I think one
>> callback request can be allowed to be received from two sk_buff.
>>
>> Signed-off-by: shaobingqing <[email protected]>
>> ---
>> net/sunrpc/xprtsock.c | 11 +++++++++--
>> 1 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>> index ee03d35..606950d 100644
>> --- a/net/sunrpc/xprtsock.c
>> +++ b/net/sunrpc/xprtsock.c
>> @@ -1271,8 +1271,13 @@ static inline int xs_tcp_read_callback(struct rpc_xprt *xprt,
>> struct sock_xprt *transport =
>> container_of(xprt, struct sock_xprt, xprt);
>> struct rpc_rqst *req;
>> + static struct rpc_rqst *req_partial;
>> +
>> + if (req_partial == NULL)
>> + req = xprt_alloc_bc_request(xprt);
>> + else if (req_partial->rq_xid == transport->tcp_xid)
>> + req = req_partial;
>
> What happens here if req_partial->rq_xid != transport->tcp_xid? AFAICS,
> req will be undefined. Either way, you cannot use a static variable for
> storage here: that isn't re-entrant.

Because metadata sever only have one slot for backchannel request,
req_partial->rq_xid == transport->tcp_xid always happens, if the callback
request just being splited in two sk_buffs. But req_partial->rq_xid !=
transport->tcp_xid may also happens in some special cases, such as
retransmission occurs?
If one callback request is splited in two sk_buffs, xs_tcp_read_callback
will be execute two times. The req_partial should be a static variable,
because the second execution of xs_tcp_read_callback should use
the rpc_rqst allocated for the first execution, which saves information
copies from the first sk_buff.
I think perhaps the code should be modified like bellows:

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 606950d..02dbb82 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1273,10 +1273,14 @@ static inline int xs_tcp_read_callback(struct
rpc_xprt *xprt,
struct rpc_rqst *req;
static struct rpc_rqst *req_partial;

- if (req_partial == NULL)
+ if (req_partial == NULL) {
req = xprt_alloc_bc_request(xprt);
- else if (req_partial->rq_xid == transport->tcp_xid)
+ } else if (req_partial->rq_xid == transport->tcp_xid) {
req = req_partial;
+ } else {
+ xprt_free_bc_request(req_partial);
+ req = xprt_alloc_bc_request(xprt);
+ }

if (req == NULL) {
printk(KERN_WARNING "Callback slot table overflowed\n");
@@ -1303,8 +1307,9 @@ static inline int xs_tcp_read_callback(struct
rpc_xprt *xprt,
list_add(&req->rq_bc_list, &bc_serv->sv_cb_list);
spin_unlock(&bc_serv->sv_cb_lock);
wake_up(&bc_serv->sv_cb_waitq);
- } else
+ } else {
req_partial = req;
+ }

req->rq_private_buf.len = transport->tcp_copied;


>
>> - req = xprt_alloc_bc_request(xprt);
>> if (req == NULL) {
>> printk(KERN_WARNING "Callback slot table overflowed\n");
>> xprt_force_disconnect(xprt);
>> @@ -1285,6 +1290,7 @@ static inline int xs_tcp_read_callback(struct rpc_xprt *xprt,
>>
>> if (!(transport->tcp_flags & TCP_RCV_COPY_DATA)) {
>> struct svc_serv *bc_serv = xprt->bc_serv;
>> + req_partial = NULL;
>>
>> /*
>> * Add callback request to callback list. The callback
>> @@ -1297,7 +1303,8 @@ static inline int xs_tcp_read_callback(struct rpc_xprt *xprt,
>> list_add(&req->rq_bc_list, &bc_serv->sv_cb_list);
>> spin_unlock(&bc_serv->sv_cb_lock);
>> wake_up(&bc_serv->sv_cb_waitq);
>> - }
>> + } else
>> + req_partial = req;
>>
>> req->rq_private_buf.len = transport->tcp_copied;
>>
>
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> --
> 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

2014-01-21 15:35:41

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Allow one callback request to be received from two sk_buff


On Jan 21, 2014, at 3:08, shaobingqing <[email protected]> wrote:

> 2014/1/21 Trond Myklebust <[email protected]>:
>> On Mon, 2014-01-20 at 14:59 +0800, shaobingqing wrote:
>>> In current code, there only one struct rpc_rqst is prealloced. If one
>>> callback request is received from two sk_buff, the xprt_alloc_bc_request
>>> would be execute two times with the same transport->xid. The first time
>>> xprt_alloc_bc_request will alloc one struct rpc_rqst and the TCP_RCV_COPY_DATA
>>> bit of transport->tcp_flags will not be cleared. The second time
>>> xprt_alloc_bc_request could not alloc struct rpc_rqst any more and NULL
>>> pointer will be returned, then xprt_force_disconnect occur. I think one
>>> callback request can be allowed to be received from two sk_buff.
>>>
>>> Signed-off-by: shaobingqing <[email protected]>
>>> ---
>>> net/sunrpc/xprtsock.c | 11 +++++++++--
>>> 1 files changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>>> index ee03d35..606950d 100644
>>> --- a/net/sunrpc/xprtsock.c
>>> +++ b/net/sunrpc/xprtsock.c
>>> @@ -1271,8 +1271,13 @@ static inline int xs_tcp_read_callback(struct rpc_xprt *xprt,
>>> struct sock_xprt *transport =
>>> container_of(xprt, struct sock_xprt, xprt);
>>> struct rpc_rqst *req;
>>> + static struct rpc_rqst *req_partial;
>>> +
>>> + if (req_partial == NULL)
>>> + req = xprt_alloc_bc_request(xprt);
>>> + else if (req_partial->rq_xid == transport->tcp_xid)
>>> + req = req_partial;
>>
>> What happens here if req_partial->rq_xid != transport->tcp_xid? AFAICS,
>> req will be undefined. Either way, you cannot use a static variable for
>> storage here: that isn't re-entrant.
>
> Because metadata sever only have one slot for backchannel request,
> req_partial->rq_xid == transport->tcp_xid always happens, if the callback
> request just being splited in two sk_buffs. But req_partial->rq_xid !=
> transport->tcp_xid may also happens in some special cases, such as
> retransmission occurs?

If the server retransmits, then it is broken. The NFSv4.1 protocol does not allow it to retransmit unless the connection breaks.

> If one callback request is splited in two sk_buffs, xs_tcp_read_callback
> will be execute two times. The req_partial should be a static variable,
> because the second execution of xs_tcp_read_callback should use
> the rpc_rqst allocated for the first execution, which saves information
> copies from the first sk_buff.

No! This is a multi-threaded/process environment which can support multiple connection. It is a bug to use a static variable.

--
Trond Myklebust
Linux NFS client maintainer

2014-01-22 23:52:39

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Allow one callback request to be received from two sk_buff

On Tue, Jan 21, 2014 at 08:35:36AM -0700, Trond Myklebust wrote:
>
> On Jan 21, 2014, at 3:08, shaobingqing <[email protected]> wrote:
>
> > 2014/1/21 Trond Myklebust <[email protected]>:
> >> On Mon, 2014-01-20 at 14:59 +0800, shaobingqing wrote:
> >>> In current code, there only one struct rpc_rqst is prealloced. If one
> >>> callback request is received from two sk_buff, the xprt_alloc_bc_request
> >>> would be execute two times with the same transport->xid. The first time
> >>> xprt_alloc_bc_request will alloc one struct rpc_rqst and the TCP_RCV_COPY_DATA
> >>> bit of transport->tcp_flags will not be cleared. The second time
> >>> xprt_alloc_bc_request could not alloc struct rpc_rqst any more and NULL
> >>> pointer will be returned, then xprt_force_disconnect occur. I think one
> >>> callback request can be allowed to be received from two sk_buff.
> >>>
> >>> Signed-off-by: shaobingqing <[email protected]>
> >>> ---
> >>> net/sunrpc/xprtsock.c | 11 +++++++++--
> >>> 1 files changed, 9 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> >>> index ee03d35..606950d 100644
> >>> --- a/net/sunrpc/xprtsock.c
> >>> +++ b/net/sunrpc/xprtsock.c
> >>> @@ -1271,8 +1271,13 @@ static inline int xs_tcp_read_callback(struct rpc_xprt *xprt,
> >>> struct sock_xprt *transport =
> >>> container_of(xprt, struct sock_xprt, xprt);
> >>> struct rpc_rqst *req;
> >>> + static struct rpc_rqst *req_partial;
> >>> +
> >>> + if (req_partial == NULL)
> >>> + req = xprt_alloc_bc_request(xprt);
> >>> + else if (req_partial->rq_xid == transport->tcp_xid)
> >>> + req = req_partial;
> >>
> >> What happens here if req_partial->rq_xid != transport->tcp_xid? AFAICS,
> >> req will be undefined. Either way, you cannot use a static variable for
> >> storage here: that isn't re-entrant.
> >
> > Because metadata sever only have one slot for backchannel request,
> > req_partial->rq_xid == transport->tcp_xid always happens, if the callback
> > request just being splited in two sk_buffs. But req_partial->rq_xid !=
> > transport->tcp_xid may also happens in some special cases, such as
> > retransmission occurs?
>
> If the server retransmits, then it is broken. The NFSv4.1 protocol does not allow it to retransmit unless the connection breaks.

shaobingqing, are you actually seeing retransmission? (If so, are we
setting up the callback client wrong?)

--b.

>
> > If one callback request is splited in two sk_buffs, xs_tcp_read_callback
> > will be execute two times. The req_partial should be a static variable,
> > because the second execution of xs_tcp_read_callback should use
> > the rpc_rqst allocated for the first execution, which saves information
> > copies from the first sk_buff.
>
> No! This is a multi-threaded/process environment which can support multiple connection. It is a bug to use a static variable.
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>

2014-01-23 01:42:14

by shaobingqing

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Allow one callback request to be received from two sk_buff

2014/1/23 J. Bruce Fields <[email protected]>:
> On Tue, Jan 21, 2014 at 08:35:36AM -0700, Trond Myklebust wrote:
>>
>> On Jan 21, 2014, at 3:08, shaobingqing <[email protected]> wrote:
>>
>> > 2014/1/21 Trond Myklebust <[email protected]>:
>> >> On Mon, 2014-01-20 at 14:59 +0800, shaobingqing wrote:
>> >>> In current code, there only one struct rpc_rqst is prealloced. If one
>> >>> callback request is received from two sk_buff, the xprt_alloc_bc_request
>> >>> would be execute two times with the same transport->xid. The first time
>> >>> xprt_alloc_bc_request will alloc one struct rpc_rqst and the TCP_RCV_COPY_DATA
>> >>> bit of transport->tcp_flags will not be cleared. The second time
>> >>> xprt_alloc_bc_request could not alloc struct rpc_rqst any more and NULL
>> >>> pointer will be returned, then xprt_force_disconnect occur. I think one
>> >>> callback request can be allowed to be received from two sk_buff.
>> >>>
>> >>> Signed-off-by: shaobingqing <[email protected]>
>> >>> ---
>> >>> net/sunrpc/xprtsock.c | 11 +++++++++--
>> >>> 1 files changed, 9 insertions(+), 2 deletions(-)
>> >>>
>> >>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>> >>> index ee03d35..606950d 100644
>> >>> --- a/net/sunrpc/xprtsock.c
>> >>> +++ b/net/sunrpc/xprtsock.c
>> >>> @@ -1271,8 +1271,13 @@ static inline int xs_tcp_read_callback(struct rpc_xprt *xprt,
>> >>> struct sock_xprt *transport =
>> >>> container_of(xprt, struct sock_xprt, xprt);
>> >>> struct rpc_rqst *req;
>> >>> + static struct rpc_rqst *req_partial;
>> >>> +
>> >>> + if (req_partial == NULL)
>> >>> + req = xprt_alloc_bc_request(xprt);
>> >>> + else if (req_partial->rq_xid == transport->tcp_xid)
>> >>> + req = req_partial;
>> >>
>> >> What happens here if req_partial->rq_xid != transport->tcp_xid? AFAICS,
>> >> req will be undefined. Either way, you cannot use a static variable for
>> >> storage here: that isn't re-entrant.
>> >
>> > Because metadata sever only have one slot for backchannel request,
>> > req_partial->rq_xid == transport->tcp_xid always happens, if the callback
>> > request just being splited in two sk_buffs. But req_partial->rq_xid !=
>> > transport->tcp_xid may also happens in some special cases, such as
>> > retransmission occurs?
>>
>> If the server retransmits, then it is broken. The NFSv4.1 protocol does not allow it to retransmit unless the connection breaks.
>
> shaobingqing, are you actually seeing retransmission? (If so, are we
> setting up the callback client wrong?)
No, not actually. Here I just see that one client can receive two
callback requests with the same xid.
>
> --b.
>
>>
>> > If one callback request is splited in two sk_buffs, xs_tcp_read_callback
>> > will be execute two times. The req_partial should be a static variable,
>> > because the second execution of xs_tcp_read_callback should use
>> > the rpc_rqst allocated for the first execution, which saves information
>> > copies from the first sk_buff.
>>
>> No! This is a multi-threaded/process environment which can support multiple connection. It is a bug to use a static variable.
>>
>> --
>> Trond Myklebust
>> Linux NFS client maintainer
>>
> --
> 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

2014-01-23 02:23:43

by shaobingqing

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Allow one callback request to be received from two sk_buff

2014/1/21 Trond Myklebust <[email protected]>:
>
> On Jan 21, 2014, at 3:08, shaobingqing <[email protected]> wrote:
>
>> 2014/1/21 Trond Myklebust <[email protected]>:
>>> On Mon, 2014-01-20 at 14:59 +0800, shaobingqing wrote:
>>>> In current code, there only one struct rpc_rqst is prealloced. If one
>>>> callback request is received from two sk_buff, the xprt_alloc_bc_request
>>>> would be execute two times with the same transport->xid. The first time
>>>> xprt_alloc_bc_request will alloc one struct rpc_rqst and the TCP_RCV_COPY_DATA
>>>> bit of transport->tcp_flags will not be cleared. The second time
>>>> xprt_alloc_bc_request could not alloc struct rpc_rqst any more and NULL
>>>> pointer will be returned, then xprt_force_disconnect occur. I think one
>>>> callback request can be allowed to be received from two sk_buff.
>>>>
>>>> Signed-off-by: shaobingqing <[email protected]>
>>>> ---
>>>> net/sunrpc/xprtsock.c | 11 +++++++++--
>>>> 1 files changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>>>> index ee03d35..606950d 100644
>>>> --- a/net/sunrpc/xprtsock.c
>>>> +++ b/net/sunrpc/xprtsock.c
>>>> @@ -1271,8 +1271,13 @@ static inline int xs_tcp_read_callback(struct rpc_xprt *xprt,
>>>> struct sock_xprt *transport =
>>>> container_of(xprt, struct sock_xprt, xprt);
>>>> struct rpc_rqst *req;
>>>> + static struct rpc_rqst *req_partial;
>>>> +
>>>> + if (req_partial == NULL)
>>>> + req = xprt_alloc_bc_request(xprt);
>>>> + else if (req_partial->rq_xid == transport->tcp_xid)
>>>> + req = req_partial;
>>>
>>> What happens here if req_partial->rq_xid != transport->tcp_xid? AFAICS,
>>> req will be undefined. Either way, you cannot use a static variable for
>>> storage here: that isn't re-entrant.
>>
>> Because metadata sever only have one slot for backchannel request,
>> req_partial->rq_xid == transport->tcp_xid always happens, if the callback
>> request just being splited in two sk_buffs. But req_partial->rq_xid !=
>> transport->tcp_xid may also happens in some special cases, such as
>> retransmission occurs?
>
> If the server retransmits, then it is broken. The NFSv4.1 protocol does not allow it to retransmit unless the connection breaks.

What I am saying above is bogus. As far as I can see, If one callback
request is splitted into two sk_buffs, the function
xs_tcp_read_callback will be called two times with the same rpc_xprt
and the same xid. If between the two calls there is
another call with the same rpc_xprt, but different xid, we consider it
is another callback request from the same server, in
the condition that there is no retransmission in our enviorenment. But
this might not happen because there is only one
callback slot in each server.

>
>> If one callback request is splited in two sk_buffs, xs_tcp_read_callback
>> will be execute two times. The req_partial should be a static variable,
>> because the second execution of xs_tcp_read_callback should use
>> the rpc_rqst allocated for the first execution, which saves information
>> copies from the first sk_buff.
>
> No! This is a multi-threaded/process environment which can support multiple connection. It is a bug to use a static variable.

I think I have misunderstood the question. Here a static variable can
not be used. Perhaps, we should define a variable for each
rpc_client (or rpc xprt).

>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> --
> 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