Re: [NFS] [PATCH 1/7] SUNRPC: Fix a race in xs_tcp_state_change()

[Chuck Lever <chuck.lever@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 2857 bytes --]

Trond Myklebust wrote:
> On Wed, 2007-11-07 at 17:21 -0500, Chuck Lever wrote:
>> Trond Myklebust wrote:
>>> From: Trond Myklebust <Trond.Myklebust@netapp.com>
>>>
>>> When scheduling the autoclose RPC call, we want to ensure that we don't
>>> race against the test_bit() call in xprt_clear_locked().
>>>
>>> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
>>> ---
>>>
>>>  include/linux/sunrpc/xprt.h |    1 +
>>>  net/sunrpc/xprt.c           |   20 ++++++++++++++++++++
>>>  net/sunrpc/xprtsock.c       |    5 +----
>>>  3 files changed, 22 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
>>> index 30b17b3..6f524a9 100644
>>> --- a/include/linux/sunrpc/xprt.h
>>> +++ b/include/linux/sunrpc/xprt.h
>>> @@ -246,6 +246,7 @@ struct rpc_rqst *	xprt_lookup_rqst(struct rpc_xprt *xprt, __be32 xid);
>>>  void			xprt_complete_rqst(struct rpc_task *task, int copied);
>>>  void			xprt_release_rqst_cong(struct rpc_task *task);
>>>  void			xprt_disconnect(struct rpc_xprt *xprt);
>>> +void			xprt_force_disconnect(struct rpc_xprt *xprt);
>>>  
>>>  /*
>>>   * Reserved bit positions in xprt->state
>>> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
>>> index 282a9a2..48c5a8b 100644
>>> --- a/net/sunrpc/xprt.c
>>> +++ b/net/sunrpc/xprt.c
>>> @@ -570,6 +570,7 @@ static void xprt_autoclose(struct work_struct *work)
>>>  
>>>  	xprt_disconnect(xprt);
>>>  	xprt->ops->close(xprt);
>>> +	clear_bit(XPRT_CLOSE_WAIT, &xprt->state);
>> xs_close already clears the CLOSE_WAIT bit, and so does xs_tcp_shutdown 
>> (added in a later patch).  So this hunk appears to be unnecessary.
>>
>> But xprt_rdma_close doesn't clear CLOSE_WAIT, it appears.  Do we want to 
>> copy (some of) the logic from the end of xs_close into xprt_rdma_close?
> 
> Nah. I'd like to move the XPRT_CLOSE_WAIT stuff into xprt.c in order to
> convert it into a generic way of telling the transport layer that we
> want it to shut down. It is already in the xprt_clear_locked() logic, so
> this is really just a cleanup of the current hack.

That's fine.  But you still clear XPRT_CLOSE_WAIT twice in a row in 
xprt_autoclose, at least for the socket transport method.  Either clear 
it in xprt_autoclose, or clear it in the tranport close methods.  Not 
both.  It doesn't break anything, but it is confusing to human readers.

I'm still a little confused about which state flags are supposed to be 
managed entirely by the transports, and which by the generic logic.  Can 
you perhaps add a block comment in xprt.h where the flags are defined 
that describes how these flags are supposed to be used?  Actually, even 
better would be to have a separate state word for the transports, 
defined in their private transport structure.  The 
non-connection-oriented transports wouldn't need to manipulate the 
specific flags.

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 259 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
NFSv4 mailing list
NFSv4@linux-nfs.org
http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4