[PATCH] cifs: remove the sockopt= mount option

[PATCH] cifs: remove the sockopt= mount option

[Jeff Layton]

...as promised for 3.9.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index d997737..8609c42 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -97,7 +97,7 @@ enum {
 	Opt_user, Opt_pass, Opt_ip,
 	Opt_unc, Opt_domain,
 	Opt_srcaddr, Opt_prefixpath,
-	Opt_iocharset, Opt_sockopt,
+	Opt_iocharset,
 	Opt_netbiosname, Opt_servern,
 	Opt_ver, Opt_vers, Opt_sec, Opt_cache,
 
@@ -202,7 +202,6 @@ static const match_table_t cifs_mount_option_tokens = {
 	{ Opt_srcaddr, "srcaddr=%s" },
 	{ Opt_prefixpath, "prefixpath=%s" },
 	{ Opt_iocharset, "iocharset=%s" },
-	{ Opt_sockopt, "sockopt=%s" },
 	{ Opt_netbiosname, "netbiosname=%s" },
 	{ Opt_servern, "servern=%s" },
 	{ Opt_ver, "ver=%s" },
@@ -1722,19 +1721,6 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
 			 */
 			cFYI(1, "iocharset set to %s", string);
 			break;
-		case Opt_sockopt:
-			string = match_strdup(args);
-			if (string == NULL)
-				goto out_nomem;
-
-			if (strnicmp(string, "TCP_NODELAY", 11) == 0) {
-				printk(KERN_WARNING "CIFS: the "
-					"sockopt=TCP_NODELAY option has been "
-					"deprecated and will be removed "
-					"in 3.9\n");
-				vol->sockopt_tcp_nodelay = 1;
-			}
-			break;
 		case Opt_netbiosname:
 			string = match_strdup(args);
 			if (string == NULL)
-- 
1.7.11.7

Re: [PATCH] cifs: remove the sockopt= mount option

[Scott Lovenberg]

On Thu, Feb 21, 2013 at 6:32 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> ...as promised for 3.9.
>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/connect.c | 16 +---------------
>  1 file changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index d997737..8609c42 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -97,7 +97,7 @@ enum {
>         Opt_user, Opt_pass, Opt_ip,
>         Opt_unc, Opt_domain,
>         Opt_srcaddr, Opt_prefixpath,
> -       Opt_iocharset, Opt_sockopt,
> +       Opt_iocharset,
>         Opt_netbiosname, Opt_servern,
>         Opt_ver, Opt_vers, Opt_sec, Opt_cache,
>
> @@ -202,7 +202,6 @@ static const match_table_t cifs_mount_option_tokens =
> {
>         { Opt_srcaddr, "srcaddr=%s" },
>         { Opt_prefixpath, "prefixpath=%s" },
>         { Opt_iocharset, "iocharset=%s" },
> -       { Opt_sockopt, "sockopt=%s" },
>         { Opt_netbiosname, "netbiosname=%s" },
>         { Opt_servern, "servern=%s" },
>         { Opt_ver, "ver=%s" },
> @@ -1722,19 +1721,6 @@ cifs_parse_mount_options(const char *mountdata,
> const char *devname,
>                          */
>                         cFYI(1, "iocharset set to %s", string);
>                         break;
> -               case Opt_sockopt:
> -                       string = match_strdup(args);
> -                       if (string == NULL)
> -                               goto out_nomem;
> -
> -                       if (strnicmp(string, "TCP_NODELAY", 11) == 0) {
> -                               printk(KERN_WARNING "CIFS: the "
> -                                       "sockopt=TCP_NODELAY option has
> been "
> -                                       "deprecated and will be removed "
> -                                       "in 3.9\n");
> -                               vol->sockopt_tcp_nodelay = 1;
> -                       }
> -                       break;
>                 case Opt_netbiosname:
>                         string = match_strdup(args);
>                         if (string == NULL)

Gar.  Why was TCP_NODELAY pulled?  I only ask because I'm updating the
socket options section of the smb.conf man page.  Supposedly NODELAY
can have great results in some network configurations[citation
needed].

--
Peace and Blessings,
-Scott.

Re: [PATCH] cifs: remove the sockopt= mount option

[Steve French]

We should do some quick performance runs to verify this - but with
corking and uncorking the socket explicitly presumably TCP_NODELAY
should not be needed.

On Thu, Feb 21, 2013 at 9:37 AM, Scott Lovenberg
<scott.lovenberg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, Feb 21, 2013 at 6:32 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>
>> ...as promised for 3.9.
>>
>> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  fs/cifs/connect.c | 16 +---------------
>>  1 file changed, 1 insertion(+), 15 deletions(-)
>>
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index d997737..8609c42 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -97,7 +97,7 @@ enum {
>>         Opt_user, Opt_pass, Opt_ip,
>>         Opt_unc, Opt_domain,
>>         Opt_srcaddr, Opt_prefixpath,
>> -       Opt_iocharset, Opt_sockopt,
>> +       Opt_iocharset,
>>         Opt_netbiosname, Opt_servern,
>>         Opt_ver, Opt_vers, Opt_sec, Opt_cache,
>>
>> @@ -202,7 +202,6 @@ static const match_table_t cifs_mount_option_tokens =
>> {
>>         { Opt_srcaddr, "srcaddr=%s" },
>>         { Opt_prefixpath, "prefixpath=%s" },
>>         { Opt_iocharset, "iocharset=%s" },
>> -       { Opt_sockopt, "sockopt=%s" },
>>         { Opt_netbiosname, "netbiosname=%s" },
>>         { Opt_servern, "servern=%s" },
>>         { Opt_ver, "ver=%s" },
>> @@ -1722,19 +1721,6 @@ cifs_parse_mount_options(const char *mountdata,
>> const char *devname,
>>                          */
>>                         cFYI(1, "iocharset set to %s", string);
>>                         break;
>> -               case Opt_sockopt:
>> -                       string = match_strdup(args);
>> -                       if (string == NULL)
>> -                               goto out_nomem;
>> -
>> -                       if (strnicmp(string, "TCP_NODELAY", 11) == 0) {
>> -                               printk(KERN_WARNING "CIFS: the "
>> -                                       "sockopt=TCP_NODELAY option has
>> been "
>> -                                       "deprecated and will be removed "
>> -                                       "in 3.9\n");
>> -                               vol->sockopt_tcp_nodelay = 1;
>> -                       }
>> -                       break;
>>                 case Opt_netbiosname:
>>                         string = match_strdup(args);
>>                         if (string == NULL)
>
> Gar.  Why was TCP_NODELAY pulled?  I only ask because I'm updating the
> socket options section of the smb.conf man page.  Supposedly NODELAY
> can have great results in some network configurations[citation
> needed].
>
> --
> Peace and Blessings,
> -Scott.



-- 
Thanks,

Steve

Re: [PATCH] cifs: remove the sockopt= mount option

[Jeff Layton]

On Thu, 21 Feb 2013 10:00:07 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> We should do some quick performance runs to verify this - but with
> corking and uncorking the socket explicitly presumably TCP_NODELAY
> should not be needed.
> 

I would think time for that sort of investigation was before you merged
the patch that said we were going to remove it. I assumed that since
you took that patch, you were OK with it.

In any case, I'll turn this question around...

TCP_NODELAY says: Send frames as soon as possible after a sendmsg (or
kernel_sendmsg in this case)

TCP_CORK says: hold sending until I uncork the socket

If the socket is CORK'ed then that overrides TCP_NODELAY (at least in
my reading of the code). Since we always cork/uncork the socket now,
what possible benefit could there be to keeping the sockopt=TCP_NODELAY
option around?

> On Thu, Feb 21, 2013 at 9:37 AM, Scott Lovenberg
> <scott.lovenberg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Thu, Feb 21, 2013 at 6:32 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >>
> >> ...as promised for 3.9.
> >>
> >> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> ---
> >>  fs/cifs/connect.c | 16 +---------------
> >>  1 file changed, 1 insertion(+), 15 deletions(-)
> >>
> >> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >> index d997737..8609c42 100644
> >> --- a/fs/cifs/connect.c
> >> +++ b/fs/cifs/connect.c
> >> @@ -97,7 +97,7 @@ enum {
> >>         Opt_user, Opt_pass, Opt_ip,
> >>         Opt_unc, Opt_domain,
> >>         Opt_srcaddr, Opt_prefixpath,
> >> -       Opt_iocharset, Opt_sockopt,
> >> +       Opt_iocharset,
> >>         Opt_netbiosname, Opt_servern,
> >>         Opt_ver, Opt_vers, Opt_sec, Opt_cache,
> >>
> >> @@ -202,7 +202,6 @@ static const match_table_t cifs_mount_option_tokens =
> >> {
> >>         { Opt_srcaddr, "srcaddr=%s" },
> >>         { Opt_prefixpath, "prefixpath=%s" },
> >>         { Opt_iocharset, "iocharset=%s" },
> >> -       { Opt_sockopt, "sockopt=%s" },
> >>         { Opt_netbiosname, "netbiosname=%s" },
> >>         { Opt_servern, "servern=%s" },
> >>         { Opt_ver, "ver=%s" },
> >> @@ -1722,19 +1721,6 @@ cifs_parse_mount_options(const char *mountdata,
> >> const char *devname,
> >>                          */
> >>                         cFYI(1, "iocharset set to %s", string);
> >>                         break;
> >> -               case Opt_sockopt:
> >> -                       string = match_strdup(args);
> >> -                       if (string == NULL)
> >> -                               goto out_nomem;
> >> -
> >> -                       if (strnicmp(string, "TCP_NODELAY", 11) == 0) {
> >> -                               printk(KERN_WARNING "CIFS: the "
> >> -                                       "sockopt=TCP_NODELAY option has
> >> been "
> >> -                                       "deprecated and will be removed "
> >> -                                       "in 3.9\n");
> >> -                               vol->sockopt_tcp_nodelay = 1;
> >> -                       }
> >> -                       break;
> >>                 case Opt_netbiosname:
> >>                         string = match_strdup(args);
> >>                         if (string == NULL)
> >
> > Gar.  Why was TCP_NODELAY pulled?  I only ask because I'm updating the
> > socket options section of the smb.conf man page.  Supposedly NODELAY
> > can have great results in some network configurations[citation
> > needed].
> >
> > --
> > Peace and Blessings,
> > -Scott.
> 
> 
> 


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

Re: [PATCH] cifs: remove the sockopt= mount option

[Steve French]

On Thu, Feb 21, 2013 at 10:27 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Thu, 21 Feb 2013 10:00:07 -0600
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> We should do some quick performance runs to verify this - but with
>> corking and uncorking the socket explicitly presumably TCP_NODELAY
>> should not be needed.
>>
>
> I would think time for that sort of investigation was before you merged
> the patch that said we were going to remove it. I assumed that since
> you took that patch, you were OK with it.

Yes - I am ok with it and it didn't seem to hurt performance.
Corking/uncorking explicitly makes more sense if the complex network
layers work as expected - but the problem is that they change so best
to be safe and at least try some quick tests.

> In any case, I'll turn this question around...
>
> TCP_NODELAY says: Send frames as soon as possible after a sendmsg (or
> kernel_sendmsg in this case)
>
> TCP_CORK says: hold sending until I uncork the socket
>
> If the socket is CORK'ed then that overrides TCP_NODELAY (at least in
> my reading of the code). Since we always cork/uncork the socket now,
> what possible benefit could there be to keeping the sockopt=TCP_NODELAY
> option around?

None if corking/uncorking works as one would intuitively think - but
the tcp network layers under us are very complicated.

(I am not objecting to the patch to remove sockopt setting - just want
to do the simple checks to make sure we don't miss something)



-- 
Thanks,

Steve

Re: [PATCH] cifs: remove the sockopt= mount option

[Jeff Layton]

On Thu, 21 Feb 2013 10:31:24 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Thu, Feb 21, 2013 at 10:27 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Thu, 21 Feb 2013 10:00:07 -0600
> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> We should do some quick performance runs to verify this - but with
> >> corking and uncorking the socket explicitly presumably TCP_NODELAY
> >> should not be needed.
> >>
> >
> > I would think time for that sort of investigation was before you merged
> > the patch that said we were going to remove it. I assumed that since
> > you took that patch, you were OK with it.
> 
> Yes - I am ok with it and it didn't seem to hurt performance.
> Corking/uncorking explicitly makes more sense if the complex network
> layers work as expected - but the problem is that they change so best
> to be safe and at least try some quick tests.
> 
> > In any case, I'll turn this question around...
> >
> > TCP_NODELAY says: Send frames as soon as possible after a sendmsg (or
> > kernel_sendmsg in this case)
> >
> > TCP_CORK says: hold sending until I uncork the socket
> >
> > If the socket is CORK'ed then that overrides TCP_NODELAY (at least in
> > my reading of the code). Since we always cork/uncork the socket now,
> > what possible benefit could there be to keeping the sockopt=TCP_NODELAY
> > option around?
> 
> None if corking/uncorking works as one would intuitively think - but
> the tcp network layers under us are very complicated.
> 
> (I am not objecting to the patch to remove sockopt setting - just want
> to do the simple checks to make sure we don't miss something)
> 

Fair enough, let me know what you find.

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

Re: [PATCH] cifs: remove the sockopt= mount option

[Scott Lovenberg]

On 2/21/2013 11:00 AM, Steve French wrote:
> We should do some quick performance runs to verify this - but with
> corking and uncorking the socket explicitly presumably TCP_NODELAY
> should not be needed.
I got that info from the old Samba HOWTO and Reference Guide, but it's 
very, very old info.  I'm guessing that when matched with an older 
kernel and possibly older TCP/IP congestion algorithm, it might have 
made more of a difference than it does now.

Re: [PATCH] cifs: remove the sockopt= mount option

[Jeff Layton]

On Thu, 21 Feb 2013 10:37:21 -0500
Scott Lovenberg <scott.lovenberg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Thu, Feb 21, 2013 at 6:32 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >
> > ...as promised for 3.9.
> >
> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/cifs/connect.c | 16 +---------------
> >  1 file changed, 1 insertion(+), 15 deletions(-)
> >
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index d997737..8609c42 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -97,7 +97,7 @@ enum {
> >         Opt_user, Opt_pass, Opt_ip,
> >         Opt_unc, Opt_domain,
> >         Opt_srcaddr, Opt_prefixpath,
> > -       Opt_iocharset, Opt_sockopt,
> > +       Opt_iocharset,
> >         Opt_netbiosname, Opt_servern,
> >         Opt_ver, Opt_vers, Opt_sec, Opt_cache,
> >
> > @@ -202,7 +202,6 @@ static const match_table_t cifs_mount_option_tokens =
> > {
> >         { Opt_srcaddr, "srcaddr=%s" },
> >         { Opt_prefixpath, "prefixpath=%s" },
> >         { Opt_iocharset, "iocharset=%s" },
> > -       { Opt_sockopt, "sockopt=%s" },
> >         { Opt_netbiosname, "netbiosname=%s" },
> >         { Opt_servern, "servern=%s" },
> >         { Opt_ver, "ver=%s" },
> > @@ -1722,19 +1721,6 @@ cifs_parse_mount_options(const char *mountdata,
> > const char *devname,
> >                          */
> >                         cFYI(1, "iocharset set to %s", string);
> >                         break;
> > -               case Opt_sockopt:
> > -                       string = match_strdup(args);
> > -                       if (string == NULL)
> > -                               goto out_nomem;
> > -
> > -                       if (strnicmp(string, "TCP_NODELAY", 11) == 0) {
> > -                               printk(KERN_WARNING "CIFS: the "
> > -                                       "sockopt=TCP_NODELAY option has
> > been "
> > -                                       "deprecated and will be removed "
> > -                                       "in 3.9\n");
> > -                               vol->sockopt_tcp_nodelay = 1;
> > -                       }
> > -                       break;
> >                 case Opt_netbiosname:
> >                         string = match_strdup(args);
> >                         if (string == NULL)
> 
> Gar.  Why was TCP_NODELAY pulled?  I only ask because I'm updating the
> socket options section of the smb.conf man page.  Supposedly NODELAY
> can have great results in some network configurations[citation
> needed].
>

commit b8eed28375a43e1c9aaa9d15af2a052aae0d0725
Author: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Date:   Tue Sep 18 16:20:35 2012 -0700

    cifs: cork the socket before a send and uncork it afterward
    
    We want to send SMBs as "atomically" as possible. Prior to sending any
    data on the socket, cork it to make sure that no non-full frames go
    out. Afterward, uncork it to make sure all of the data gets pushed out
    to the wire.
    
    Note that this more or less renders the socket=TCP_NODELAY mount option
    obsolete. When TCP_CORK and TCP_NODELAY are used on the same socket,
    TCP_NODELAY is essentially ignored.
    
    Acked-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
    Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    Signed-off-by: Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

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

Re: [PATCH] cifs: remove the sockopt= mount option

[Steve French]

Resending patch to a slightly broader list for last minute check if
anyone objects.  Although setting this particular socket option
(TCP_NODELAY) may not be as useful when corking/uncorking explicitly,
I want to doublecheck before removing them because there has been some
utility to the server allowing override of various sockopt options.
Samba server has long supported at least the following set of settable
socket options (although I don't know if the defaults are frequently
overridden now, by setting sockopts in smb.conf as used to be common
for the server).

SO_KEEPALIVE
SO_REUSEADDR
SO_BROADCAST
TCP_NODELAY
IPTOS_LOWDELAY
IPTOS_THROUGHPUT
SO_SNDBUF *
SO_RCVBUF *
SO_SNDLOWAT *
SO_RCVLOWAT *

* takes an integer argument rather than a boolean on/off

Any objections to removing the ability to set socket options
explicitly for the cifs network file system client?

On Thu, Feb 21, 2013 at 5:32 AM, Jeff Layton <jlayton@redhat.com> wrote:
>
> ...as promised for 3.9.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/cifs/connect.c | 16 +---------------
>  1 file changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index d997737..8609c42 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -97,7 +97,7 @@ enum {
>         Opt_user, Opt_pass, Opt_ip,
>         Opt_unc, Opt_domain,
>         Opt_srcaddr, Opt_prefixpath,
> -       Opt_iocharset, Opt_sockopt,
> +       Opt_iocharset,
>         Opt_netbiosname, Opt_servern,
>         Opt_ver, Opt_vers, Opt_sec, Opt_cache,
>
> @@ -202,7 +202,6 @@ static const match_table_t cifs_mount_option_tokens =
> {
>         { Opt_srcaddr, "srcaddr=%s" },
>         { Opt_prefixpath, "prefixpath=%s" },
>         { Opt_iocharset, "iocharset=%s" },
> -       { Opt_sockopt, "sockopt=%s" },
>         { Opt_netbiosname, "netbiosname=%s" },
>         { Opt_servern, "servern=%s" },
>         { Opt_ver, "ver=%s" },
> @@ -1722,19 +1721,6 @@ cifs_parse_mount_options(const char *mountdata,
> const char *devname,
>                          */
>                         cFYI(1, "iocharset set to %s", string);
>                         break;
> -               case Opt_sockopt:
> -                       string = match_strdup(args);
> -                       if (string == NULL)
> -                               goto out_nomem;
> -
> -                       if (strnicmp(string, "TCP_NODELAY", 11) == 0) {
> -                               printk(KERN_WARNING "CIFS: the "
> -                                       "sockopt=TCP_NODELAY option has
> been "
> -                                       "deprecated and will be removed "
> -                                       "in 3.9\n");
> -                               vol->sockopt_tcp_nodelay = 1;
> -                       }
> -                       break;
>                 case Opt_netbiosname:
>                         string = match_strdup(args);
>                         if (string == NULL)
> --
> 1.7.11.7
>



--
Thanks,

Steve

Re: [PATCH] cifs: remove the sockopt= mount option

[Jeff Layton]

On Mon, 4 Mar 2013 16:08:30 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Resending patch to a slightly broader list for last minute check if
> anyone objects.  Although setting this particular socket option
> (TCP_NODELAY) may not be as useful when corking/uncorking explicitly,
> I want to doublecheck before removing them because there has been some
> utility to the server allowing override of various sockopt options.
> Samba server has long supported at least the following set of settable
> socket options (although I don't know if the defaults are frequently
> overridden now, by setting sockopts in smb.conf as used to be common
> for the server).
> 
> SO_KEEPALIVE
> SO_REUSEADDR
> SO_BROADCAST
> TCP_NODELAY
> IPTOS_LOWDELAY
> IPTOS_THROUGHPUT
> SO_SNDBUF *
> SO_RCVBUF *
> SO_SNDLOWAT *
> SO_RCVLOWAT *
> 
> * takes an integer argument rather than a boolean on/off
> 
> Any objections to removing the ability to set socket options
> explicitly for the cifs network file system client?
> 

A couple of points...

The sockopt= option was never documented in the mount.cifs manpage and
the only value it ever accepted was TCP_NODELAY. Now that we're
explicitly corking the socket, TCP_NODELAY has no effect. I don't think
there's any value in leaving in a "placeholder" socket= option.

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

Re: [PATCH] cifs: remove the sockopt= mount option

[Jeff Layton]

On Mon, 4 Mar 2013 16:08:30 -0600
Steve French <smfrench@gmail.com> wrote:

> Resending patch to a slightly broader list for last minute check if
> anyone objects.  Although setting this particular socket option
> (TCP_NODELAY) may not be as useful when corking/uncorking explicitly,
> I want to doublecheck before removing them because there has been some
> utility to the server allowing override of various sockopt options.
> Samba server has long supported at least the following set of settable
> socket options (although I don't know if the defaults are frequently
> overridden now, by setting sockopts in smb.conf as used to be common
> for the server).
> 
> SO_KEEPALIVE
> SO_REUSEADDR
> SO_BROADCAST
> TCP_NODELAY
> IPTOS_LOWDELAY
> IPTOS_THROUGHPUT
> SO_SNDBUF *
> SO_RCVBUF *
> SO_SNDLOWAT *
> SO_RCVLOWAT *
> 
> * takes an integer argument rather than a boolean on/off
> 
> Any objections to removing the ability to set socket options
> explicitly for the cifs network file system client?
> 

A couple of points...

The sockopt= option was never documented in the mount.cifs manpage and
the only value it ever accepted was TCP_NODELAY. Now that we're
explicitly corking the socket, TCP_NODELAY has no effect. I don't think
there's any value in leaving in a "placeholder" socket= option.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] cifs: remove the sockopt= mount option

[Scott Lovenberg]

On Wed, Mar 6, 2013 at 8:40 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Mon, 4 Mar 2013 16:08:30 -0600
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> Resending patch to a slightly broader list for last minute check if
>> anyone objects.  Although setting this particular socket option
>> (TCP_NODELAY) may not be as useful when corking/uncorking explicitly,
>> I want to doublecheck before removing them because there has been some
>> utility to the server allowing override of various sockopt options.
>> Samba server has long supported at least the following set of settable
>> socket options (although I don't know if the defaults are frequently
>> overridden now, by setting sockopts in smb.conf as used to be common
>> for the server).
>>
>> SO_KEEPALIVE
>> SO_REUSEADDR
>> SO_BROADCAST
>> TCP_NODELAY
>> IPTOS_LOWDELAY
>> IPTOS_THROUGHPUT
>> SO_SNDBUF *
>> SO_RCVBUF *
>> SO_SNDLOWAT *
>> SO_RCVLOWAT *
>>
>> * takes an integer argument rather than a boolean on/off
>>
>> Any objections to removing the ability to set socket options
>> explicitly for the cifs network file system client?
>>
>
> A couple of points...
>
> The sockopt= option was never documented in the mount.cifs manpage and
> the only value it ever accepted was TCP_NODELAY. Now that we're
> explicitly corking the socket, TCP_NODELAY has no effect. I don't think
> there's any value in leaving in a "placeholder" socket= option.
>

After doing some research, I agree.  I've been updating the socket
options section of the Samba smb.conf man page and the performance
section of the Samba HOWTO.  The more research I do the more convinced
that there is almost no reason to set most socket options on a modern
Linux kernel or in Samba (server or client side).

One thing pointed out to me in another thread is that the smbd server
sets TCP_NODELAY by default.  AIUI this was because the TCP_NODELAY
option used to almost double the speed of SMB in some cases due to the
way Microsoft's old TCP/IP stack sent ACKs.

This is all to say that I think the "sockopt" option can go altogether
since it's unlikely that any socket options will be needed in the
future and the only one that was supported is now useless.
-- 
Peace and Blessings,
-Scott.

Re: [PATCH] cifs: remove the sockopt= mount option

[Scott Lovenberg]

On Wed, Mar 6, 2013 at 8:40 AM, Jeff Layton <jlayton@redhat.com> wrote:
> On Mon, 4 Mar 2013 16:08:30 -0600
> Steve French <smfrench@gmail.com> wrote:
>
>> Resending patch to a slightly broader list for last minute check if
>> anyone objects.  Although setting this particular socket option
>> (TCP_NODELAY) may not be as useful when corking/uncorking explicitly,
>> I want to doublecheck before removing them because there has been some
>> utility to the server allowing override of various sockopt options.
>> Samba server has long supported at least the following set of settable
>> socket options (although I don't know if the defaults are frequently
>> overridden now, by setting sockopts in smb.conf as used to be common
>> for the server).
>>
>> SO_KEEPALIVE
>> SO_REUSEADDR
>> SO_BROADCAST
>> TCP_NODELAY
>> IPTOS_LOWDELAY
>> IPTOS_THROUGHPUT
>> SO_SNDBUF *
>> SO_RCVBUF *
>> SO_SNDLOWAT *
>> SO_RCVLOWAT *
>>
>> * takes an integer argument rather than a boolean on/off
>>
>> Any objections to removing the ability to set socket options
>> explicitly for the cifs network file system client?
>>
>
> A couple of points...
>
> The sockopt= option was never documented in the mount.cifs manpage and
> the only value it ever accepted was TCP_NODELAY. Now that we're
> explicitly corking the socket, TCP_NODELAY has no effect. I don't think
> there's any value in leaving in a "placeholder" socket= option.
>

After doing some research, I agree.  I've been updating the socket
options section of the Samba smb.conf man page and the performance
section of the Samba HOWTO.  The more research I do the more convinced
that there is almost no reason to set most socket options on a modern
Linux kernel or in Samba (server or client side).

One thing pointed out to me in another thread is that the smbd server
sets TCP_NODELAY by default.  AIUI this was because the TCP_NODELAY
option used to almost double the speed of SMB in some cases due to the
way Microsoft's old TCP/IP stack sent ACKs.

This is all to say that I think the "sockopt" option can go altogether
since it's unlikely that any socket options will be needed in the
future and the only one that was supported is now useless.
-- 
Peace and Blessings,
-Scott.