On Sun, May 6, 2012 at 9:01 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Mon, 26 Mar 2012 13:21:30 +0400
> Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
>
>> Signed-off-by: Steve French <sfrench@xxxxxxxxxx>
>> Signed-off-by: Pavel Shilovsky <piastry@xxxxxxxxxxx>
>> ---
>> fs/cifs/cifsglob.h | 3 +++
>> fs/cifs/connect.c | 16 +++++++++++++++-
>> 2 files changed, 18 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index 9e28070..fbee8ef 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -267,6 +267,9 @@ struct TCP_Server_Info {
>> char server_GUID[16];
>> char sec_mode;
>> bool session_estab; /* mark when very first sess is established */
>> +#ifdef CONFIG_CIFS_SMB2
>> + bool is_smb2; /* SMB2 not CIFS protocol negotiated */
>> +#endif
>> u16 dialect; /* dialect index that server chose */
>> enum securityEnum secType;
>> bool oplocks:1; /* enable oplocks */
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index 955f0a3..1fbc21f 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -1,7 +1,7 @@
>> /*
>> * fs/cifs/connect.c
>> *
>> - * Copyright (C) International Business Machines Corp., 2002,2009
>> + * Copyright (C) International Business Machines Corp., 2002,2011
>> * Author(s): Steve French (sfrench@xxxxxxxxxx)
>> *
>> * This library is free software; you can redistribute it and/or modify
>> @@ -2093,6 +2093,14 @@ static int match_server(struct TCP_Server_Info *server, struct sockaddr *addr,
>> (struct sockaddr *)&vol->srcaddr))
>> return 0;
>>
>> +#ifdef CONFIG_CIFS_SMB2
>> + if ((server->is_smb2 == true) && (vol->use_smb2 == false))
>> + return 0;
>> +
>> + if ((server->is_smb2 == false) && (vol->use_smb2 == true))
>> + return 0;
>> +#endif /* CONFIG_CIFS_SMB2 */
>> +
>
> Again, booleans for this seem like a less than ideal solution. Better
> to have this based on a version number or maybe a protocol version enum?
>
>> if (!match_port(server, addr))
>> return 0;
>>
>> @@ -2217,6 +2225,7 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
>>
>> tcp_ses->noblocksnd = volume_info->noblocksnd;
>> tcp_ses->noautotune = volume_info->noautotune;
>> + /* BB should we set this unconditionally now, especially for SMB2 */
>> tcp_ses->tcp_nodelay = volume_info->sockopt_tcp_nodelay;
>
> Why is it more important to use TCP_NODELAY for SMB2? The above comment
> doesn't really say...
>
>> tcp_ses->in_flight = 0;
>> tcp_ses->credits = 1;
>> @@ -2261,6 +2270,11 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
>> goto out_err_crypto_release;
>> }
>>
>> +#ifdef CONFIG_CIFS_SMB2
>> + if (volume_info->use_smb2)
>> + tcp_ses->is_smb2 = true;
>> +#endif /* CONFIG_CIFS_SMB2 */
>> +
>
> The above should instead set a pointer to a "struct
> protocol_operations" or something. That struct could have a version
> field within it and it would give you a mechanism to allow for
> protocol-version specific behavior without resorting to sprinkling
> "is_smb2" checks all over the code.
On the tcp-nodelay question:
I think it complicates things to give the user a choice now (it should
be faster to send with nodelay) - should probably be unconditional
(less options for SMB2 is easier since we have a spec, and well
behaved servers)
On the "is_smb2" - mostly it just matters whether it is smb2 - but I
have no problem with a sub-version within smb2, but lets limit this as
much as is reasonably possible now (most servers support cifs - so
lets focus as narrowly as we can to accelerate development safely) I
really don't want to worry about testing smb2.0 vs. 2.1 vs. 2.2 (aka3)
- can't we simply offer smb2.1 and smb3 (if they need to talk to
Windows Vista they can use cifs) and choose between smb2.1 and smb3
via a sub-version field (remember these are MUCH easier in smb2 since
they are just a number not a string).
--
Thanks,
Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
