NODELAY is often set on NFS, FTP and Samba.
On Mon, May 7, 2012 at 8:34 AM, Steve French <smfrench@xxxxxxxxx> wrote:
> It is going to be a small effect normally but should be measurable -
> for all smb requests
> but write we issue them as one tcp send, and in all cases except write they
> are small so can get delayed.
>
> I remember asking Linux networking guys many years ago and they said
> we should be setting it (unless something strange changed not sure
> why the logic for all but writes would be different on no delay).
>
> On Mon, May 7, 2012 at 8:32 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>> On Mon, 7 May 2012 07:52:03 -0500
>> Steve French <smfrench@xxxxxxxxx> wrote:
>>
>>> On Mon, May 7, 2012 at 6:36 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>>> > On Sun, 6 May 2012 22:32:54 -0500
>>> > Steve French <smfrench@xxxxxxxxx> wrote:
>>> >
>>> >> 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)
>>> >>
>>> >
>>> > It is more complicated to give a choice, but why should we choose to
>>> > use TCP_NODELAY at all? Do we have any benchmarks or anything that show
>>> > that it really helps performance or makes any difference at all?
>>>
>>> We did have benchmarks that showed it was faster (it got requests
>>> on the wire faster) but that was years ago. Generally for request/response
>>> protocols why wouldn't you always set TCP_NODELAY?
>>
>> Because it's less efficient to send a request in two frames than one?
>> You're basically trading extra bandwidth for lowered latency. That's
>> not necessarily a win...
>>
>> Consider a write request, for instance. If we have to do it in small
>> pieces we could end up sending more TCP frames than necessary which
>> have their own overhead. The data may get to the server faster in some
>> cases, but until the whole request is there it can't really do anything
>> with it anyway. You're better off having waited until it was all
>> buffered up.
>>
>> I don't know one way or the other whether using TCP_NODELAY is faster
>> on some workloads. Before we make any assumptions however, we ought to
>> come up with a way to test that theory, and to assess its impact on
>> bandwidth utilization.
>>
>> --
>> Jeff Layton <jlayton@xxxxxxxxxx>
>
>
>
> --
> Thanks,
>
> Steve
--
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
