[linux-cifs-client] SMB2 implementation

Jeff Layton jlayton at redhat.com
Sun Feb 28 05:20:54 MST 2010


I've been looking over the code in your smb2 repo. Here's a few random
things that I think need to be fixed, preferably ASAP since most of
them are infrastructure-level changes that will be harder to fix as new
code is added. BTW, is there a design document or something on all of
this?

1) I see a lot of these constructs:

     smb2_sb->tcon

...this is making a rather large assumption about the relationship
between the superblock and tcon. That is, it's going to make it
extremely difficult to do more than one tcon per superblock. These
should probably be changed to use an accessor function (SMB2_SB_TCON()
or something).

2) SMB2_ses_lock seems to be a lock around the file lists and maybe
some other stuff? What does it really protect and why is it called a
"ses_lock"?

3) SMB2 copies the complex scheme for tracking open files. For instance,
the close_pend flag and smb2_close has this crazy 2s hardcoded delay in
it. This should just use proper refcounting instead. There should be a
lock (global or maybe per-tcon) that protects the list and the
refcount. smb2_close should take the lock, decrement the refcount and
take it off the list (so that it can't be found in later searches).
Then, whoever drops the last reference does the actual on-the-wire
close.

4) it would be nice to get rid of a lot of the existing reconnect
scheme. I'd like to see us move to a scheme where we stamp the
tcp_srv_inf (terrible name for this struct, btw) with a serial number
(just a counter really). The counter will be bumped any time the socket
changes state.

We'll then stamp each "descendant" object (server->ses->tcon->file)
with the serial number of its parent. When we go to send, we record the
serial number of the lowest object used and then encode the request.
When we go to send we send down the recorded S/N. The send routine can
then check to see whether the serial number matches the one on the
socket before attempting the send.

This gives us the opportunity to use socket callbacks (e.g.
sk_state_change) to let us handle reconnects more cleanly and gets rid
of a lot of this state flag changing that occurs on the entire struct
hierarchy whenever the socket reconnects. The locking around all of
that has always been extremely unclear and hard to follow.

5) smb2_sendrcv2 and smb2_sendrcv_norsp should take a tcp_srv_inf arg,
and not a smb2_ses arg. It only takes a ses pointer now because of this
line in allocate_mid:

        if (ses->status != SMB2GOOD) {

...and that should go away with a counter-based reconnect system. That
would allow us to handle NEGOTIATE_PROTOCOL requests as part of the
tcp_srv_info setup rather than as part of the session setup.

-- 
Jeff Layton <jlayton at redhat.com>


More information about the linux-cifs-client mailing list