Re: SMB2 implementation

1) Agreed

2) agreed

3) I was waiting to look at this until we start looking at batch
oplock, assuming that that code would have to change a lot to handle
batch oplock

4) I think the general idea of the change to the "need_reconnect" to a
"serial number" (a glorified counter) which you discussed at
Connectathon makes sense.

5) It has to have a pointer to the smb ses info to do the signing

On Sun, Feb 28, 2010 at 6:20 AM, Jeff Layton <[email protected]> wrote:
> 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 <[email protected]>
>



-- 
Thanks,

Steve