Re: [PATCH 03/16] CIFS: Check for SMB2 vs CIFS in find_tcp_session

[Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Mon, 7 May 2012 08:36:13 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> NODELAY is often set on NFS, FTP and Samba.
> 

I don't see where the Linux NFS client sets TCP_NODELAY. I've also
never noticed the client delaying small requests, but I haven't really
looked for it either...
 
You may be correct here and it is beneficial, but I think we ought to
have some way to measure that gain and the impact on the overall
bandwidth utilization. After all, you don't get any speedup from this
"for free". It costs you in overall network utilization. That cost may
be marginal, but it won't be zero.

> On Mon, May 7, 2012 at 8:34 AM, Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >> On Mon, 7 May 2012 07:52:03 -0500
> >> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>
> >>> On Mon, May 7, 2012 at 6:36 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >>> > On Sun, 6 May 2012 22:32:54 -0500
> >>> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>> >
> >>> >> On Sun, May 6, 2012 at 9:01 AM, Jeff Layton <jlayton-H+wXaHxf7aJhl2p70BpVqQ@public.gmane.orgm> wrote:
> >>> >> > On Mon, 26 Mar 2012 13:21:30 +0400
> >>> >> > Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> >>> >> >
> >>> >> >> Signed-off-by: Steve French <sfrench-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> >>> >> >> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> >>> >> >> ---
> >>> >> >>  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-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org)
> >>> >> >>   *
> >>> >> >>   *   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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
> 
> 
> 


-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>