[PATCH] cifs: lower default wsize when unix extensions are not used

[PATCH] cifs: lower default wsize when unix extensions are not used

[Jeff Layton]

We've had some reports of servers (namely, the Solaris in-kernel CIFS
server) that don't deal properly with writes that are "too large" even
though they set CAP_LARGE_WRITE_ANDX. Change the default to better
mirror what windows clients do.

Cc: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
Reported-by: Nick Davis <phireph0x-/E1597aS9LQAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c |   23 +++++++++++++++++++----
 1 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index d6a972d..bf82f88 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2912,18 +2912,33 @@ void cifs_setup_cifs_sb(struct smb_vol *pvolume_info,
 #define CIFS_DEFAULT_IOSIZE (1024 * 1024)
 
 /*
- * Windows only supports a max of 60k reads. Default to that when posix
- * extensions aren't in force.
+ * Windows only supports a max of 60kb reads and 65535 byte writes. Default to
+ * those values when posix extensions aren't in force. In actuality here, we
+ * use 65536 to allow for a write that is a multiple of 4k. Most servers seem
+ * to be ok with the extra byte even though Windows doesn't send writes that
+ * are that large.
+ *
+ * Citation:
+ *
+ * http://blogs.msdn.com/b/openspecification/archive/2009/04/10/smb-maximum-transmit-buffer-size-and-performance-tuning.aspx
  */
 #define CIFS_DEFAULT_NON_POSIX_RSIZE (60 * 1024)
+#define CIFS_DEFAULT_NON_POSIX_WSIZE (65536)
 
 static unsigned int
 cifs_negotiate_wsize(struct cifs_tcon *tcon, struct smb_vol *pvolume_info)
 {
 	__u64 unix_cap = le64_to_cpu(tcon->fsUnixInfo.Capability);
 	struct TCP_Server_Info *server = tcon->ses->server;
-	unsigned int wsize = pvolume_info->wsize ? pvolume_info->wsize :
-				CIFS_DEFAULT_IOSIZE;
+	unsigned int wsize;
+
+	/* start with specified wsize, or default */
+	if (pvolume_info->wsize)
+		wsize = pvolume_info->wsize;
+	else if (tcon->unix_ext && (unix_cap & CIFS_UNIX_LARGE_WRITE_CAP))
+		wsize = CIFS_DEFAULT_IOSIZE;
+	else
+		wsize = CIFS_DEFAULT_NON_POSIX_WSIZE;
 
 	/* can server support 24-bit write sizes? (via UNIX extensions) */
 	if (!tcon->unix_ext || !(unix_cap & CIFS_UNIX_LARGE_WRITE_CAP))
-- 
1.7.6.4

Re: [PATCH] cifs: lower default wsize when unix extensions are not used

[Jeff Layton]

On Wed,  9 Nov 2011 13:37:23 -0500
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> We've had some reports of servers (namely, the Solaris in-kernel CIFS
> server) that don't deal properly with writes that are "too large" even
> though they set CAP_LARGE_WRITE_ANDX. Change the default to better
> mirror what windows clients do.
> 
> Cc: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> Reported-by: Nick Davis <phireph0x-/E1597aS9LQAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/connect.c |   23 +++++++++++++++++++----
>  1 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index d6a972d..bf82f88 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2912,18 +2912,33 @@ void cifs_setup_cifs_sb(struct smb_vol *pvolume_info,
>  #define CIFS_DEFAULT_IOSIZE (1024 * 1024)
>  
>  /*
> - * Windows only supports a max of 60k reads. Default to that when posix
> - * extensions aren't in force.
> + * Windows only supports a max of 60kb reads and 65535 byte writes. Default to
> + * those values when posix extensions aren't in force. In actuality here, we
> + * use 65536 to allow for a write that is a multiple of 4k. Most servers seem
> + * to be ok with the extra byte even though Windows doesn't send writes that
> + * are that large.
> + *
> + * Citation:
> + *
> + * http://blogs.msdn.com/b/openspecification/archive/2009/04/10/smb-maximum-transmit-buffer-size-and-performance-tuning.aspx
>   */
>  #define CIFS_DEFAULT_NON_POSIX_RSIZE (60 * 1024)
> +#define CIFS_DEFAULT_NON_POSIX_WSIZE (65536)
>  
>  static unsigned int
>  cifs_negotiate_wsize(struct cifs_tcon *tcon, struct smb_vol *pvolume_info)
>  {
>  	__u64 unix_cap = le64_to_cpu(tcon->fsUnixInfo.Capability);
>  	struct TCP_Server_Info *server = tcon->ses->server;
> -	unsigned int wsize = pvolume_info->wsize ? pvolume_info->wsize :
> -				CIFS_DEFAULT_IOSIZE;
> +	unsigned int wsize;
> +
> +	/* start with specified wsize, or default */
> +	if (pvolume_info->wsize)
> +		wsize = pvolume_info->wsize;
> +	else if (tcon->unix_ext && (unix_cap & CIFS_UNIX_LARGE_WRITE_CAP))
> +		wsize = CIFS_DEFAULT_IOSIZE;
> +	else
> +		wsize = CIFS_DEFAULT_NON_POSIX_WSIZE;
>  
>  	/* can server support 24-bit write sizes? (via UNIX extensions) */
>  	if (!tcon->unix_ext || !(unix_cap & CIFS_UNIX_LARGE_WRITE_CAP))


I should also mention that I suspect that this will fix this bug
without needing extra options:

    https://bugzilla.samba.org/show_bug.cgi?id=8559

If it looks acceptable, this might be suitable for stable too.
-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: lower default wsize when unix extensions are not used

[Pavel Shilovsky]

2011/11/9 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> We've had some reports of servers (namely, the Solaris in-kernel CIFS
> server) that don't deal properly with writes that are "too large" even
> though they set CAP_LARGE_WRITE_ANDX. Change the default to better
> mirror what windows clients do.
>
> Cc: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> Reported-by: Nick Davis <phireph0x-/E1597aS9LQAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/connect.c |   23 +++++++++++++++++++----
>  1 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index d6a972d..bf82f88 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2912,18 +2912,33 @@ void cifs_setup_cifs_sb(struct smb_vol *pvolume_info,
>  #define CIFS_DEFAULT_IOSIZE (1024 * 1024)
>
>  /*
> - * Windows only supports a max of 60k reads. Default to that when posix
> - * extensions aren't in force.
> + * Windows only supports a max of 60kb reads and 65535 byte writes. Default to
> + * those values when posix extensions aren't in force. In actuality here, we
> + * use 65536 to allow for a write that is a multiple of 4k. Most servers seem
> + * to be ok with the extra byte even though Windows doesn't send writes that
> + * are that large.
> + *
> + * Citation:
> + *
> + * http://blogs.msdn.com/b/openspecification/archive/2009/04/10/smb-maximum-transmit-buffer-size-and-performance-tuning.aspx
>  */
>  #define CIFS_DEFAULT_NON_POSIX_RSIZE (60 * 1024)
> +#define CIFS_DEFAULT_NON_POSIX_WSIZE (65536)
>
>  static unsigned int
>  cifs_negotiate_wsize(struct cifs_tcon *tcon, struct smb_vol *pvolume_info)
>  {
>        __u64 unix_cap = le64_to_cpu(tcon->fsUnixInfo.Capability);
>        struct TCP_Server_Info *server = tcon->ses->server;
> -       unsigned int wsize = pvolume_info->wsize ? pvolume_info->wsize :
> -                               CIFS_DEFAULT_IOSIZE;
> +       unsigned int wsize;
> +
> +       /* start with specified wsize, or default */
> +       if (pvolume_info->wsize)
> +               wsize = pvolume_info->wsize;
> +       else if (tcon->unix_ext && (unix_cap & CIFS_UNIX_LARGE_WRITE_CAP))
> +               wsize = CIFS_DEFAULT_IOSIZE;
> +       else
> +               wsize = CIFS_DEFAULT_NON_POSIX_WSIZE;
>
>        /* can server support 24-bit write sizes? (via UNIX extensions) */
>        if (!tcon->unix_ext || !(unix_cap & CIFS_UNIX_LARGE_WRITE_CAP))
> --
> 1.7.6.4

Reviewed-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH] cifs: lower default wsize when unix extensions are not used

[Steve French]

How much of a performance hit does this make?  I would expect that
dropping the writesize by more than 50% to most non-Samba servers to
work around a bug in Solaris seems extreme.  If this hurts our
performance measurably, it may be more pragmatic to limit the wsize
change to a subset of these (e.g. based on server type) - seems more
fair to not punish other servers for a Solaris bug.

On Wed, Nov 9, 2011 at 12:41 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Wed,  9 Nov 2011 13:37:23 -0500
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
>> We've had some reports of servers (namely, the Solaris in-kernel CIFS
>> server) that don't deal properly with writes that are "too large" even
>> though they set CAP_LARGE_WRITE_ANDX. Change the default to better
>> mirror what windows clients do.
>>
>> Cc: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
>> Reported-by: Nick Davis <phireph0x-/E1597aS9LQAvxtiuMwx3w@public.gmane.org>
>> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  fs/cifs/connect.c |   23 +++++++++++++++++++----
>>  1 files changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index d6a972d..bf82f88 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -2912,18 +2912,33 @@ void cifs_setup_cifs_sb(struct smb_vol *pvolume_info,
>>  #define CIFS_DEFAULT_IOSIZE (1024 * 1024)
>>
>>  /*
>> - * Windows only supports a max of 60k reads. Default to that when posix
>> - * extensions aren't in force.
>> + * Windows only supports a max of 60kb reads and 65535 byte writes. Default to
>> + * those values when posix extensions aren't in force. In actuality here, we
>> + * use 65536 to allow for a write that is a multiple of 4k. Most servers seem
>> + * to be ok with the extra byte even though Windows doesn't send writes that
>> + * are that large.
>> + *
>> + * Citation:
>> + *
>> + * http://blogs.msdn.com/b/openspecification/archive/2009/04/10/smb-maximum-transmit-buffer-size-and-performance-tuning.aspx
>>   */
>>  #define CIFS_DEFAULT_NON_POSIX_RSIZE (60 * 1024)
>> +#define CIFS_DEFAULT_NON_POSIX_WSIZE (65536)
>>
>>  static unsigned int
>>  cifs_negotiate_wsize(struct cifs_tcon *tcon, struct smb_vol *pvolume_info)
>>  {
>>       __u64 unix_cap = le64_to_cpu(tcon->fsUnixInfo.Capability);
>>       struct TCP_Server_Info *server = tcon->ses->server;
>> -     unsigned int wsize = pvolume_info->wsize ? pvolume_info->wsize :
>> -                             CIFS_DEFAULT_IOSIZE;
>> +     unsigned int wsize;
>> +
>> +     /* start with specified wsize, or default */
>> +     if (pvolume_info->wsize)
>> +             wsize = pvolume_info->wsize;
>> +     else if (tcon->unix_ext && (unix_cap & CIFS_UNIX_LARGE_WRITE_CAP))
>> +             wsize = CIFS_DEFAULT_IOSIZE;
>> +     else
>> +             wsize = CIFS_DEFAULT_NON_POSIX_WSIZE;
>>
>>       /* can server support 24-bit write sizes? (via UNIX extensions) */
>>       if (!tcon->unix_ext || !(unix_cap & CIFS_UNIX_LARGE_WRITE_CAP))
>
>
> I should also mention that I suspect that this will fix this bug
> without needing extra options:
>
>    https://bugzilla.samba.org/show_bug.cgi?id=8559
>
> If it looks acceptable, this might be suitable for stable too.
> --
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>



-- 
Thanks,

Steve

Re: [PATCH] cifs: lower default wsize when unix extensions are not used

[Jeff Layton]

On Wed, 9 Nov 2011 14:55:36 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> How much of a performance hit does this make?  I would expect that
> dropping the writesize by more than 50% to most non-Samba servers to
> work around a bug in Solaris seems extreme.  If this hurts our
> performance measurably, it may be more pragmatic to limit the wsize
> change to a subset of these (e.g. based on server type) - seems more
> fair to not punish other servers for a Solaris bug.
> 

I'm not sure how big a hit it will be, but it won't be 0. As always, it
depends on workload. The problem is, I'm not sure we can reliably
detect when the server can't handle larger writes like this.

Most servers are fine with larger writes, but when it's not then the
result is failed writeback. Given that a lot of userspace code doesn't
check the return code on close(), then this manifests itself as
"silent" data corruption.

So, while I don't like it, I think we're basically of constrained by
Windows' behavior here. We really have to ensure that the defaults work
for everyone, even if they're not what we'd consider ideal.

Note too that this just sets the default wsize to 65536 in this
situation. There's nothing that prevents someone from setting it higher
if they have a server that can handle larger writes.

> On Wed, Nov 9, 2011 at 12:41 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Wed,  9 Nov 2011 13:37:23 -0500
> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >
> >> We've had some reports of servers (namely, the Solaris in-kernel CIFS
> >> server) that don't deal properly with writes that are "too large" even
> >> though they set CAP_LARGE_WRITE_ANDX. Change the default to better
> >> mirror what windows clients do.
> >>
> >> Cc: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> >> Reported-by: Nick Davis <phireph0x-/E1597aS9LQAvxtiuMwx3w@public.gmane.org>
> >> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> ---
> >>  fs/cifs/connect.c |   23 +++++++++++++++++++----
> >>  1 files changed, 19 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >> index d6a972d..bf82f88 100644
> >> --- a/fs/cifs/connect.c
> >> +++ b/fs/cifs/connect.c
> >> @@ -2912,18 +2912,33 @@ void cifs_setup_cifs_sb(struct smb_vol *pvolume_info,
> >>  #define CIFS_DEFAULT_IOSIZE (1024 * 1024)
> >>
> >>  /*
> >> - * Windows only supports a max of 60k reads. Default to that when posix
> >> - * extensions aren't in force.
> >> + * Windows only supports a max of 60kb reads and 65535 byte writes. Default to
> >> + * those values when posix extensions aren't in force. In actuality here, we
> >> + * use 65536 to allow for a write that is a multiple of 4k. Most servers seem
> >> + * to be ok with the extra byte even though Windows doesn't send writes that
> >> + * are that large.
> >> + *
> >> + * Citation:
> >> + *
> >> + * http://blogs.msdn.com/b/openspecification/archive/2009/04/10/smb-maximum-transmit-buffer-size-and-performance-tuning.aspx
> >>   */
> >>  #define CIFS_DEFAULT_NON_POSIX_RSIZE (60 * 1024)
> >> +#define CIFS_DEFAULT_NON_POSIX_WSIZE (65536)
> >>
> >>  static unsigned int
> >>  cifs_negotiate_wsize(struct cifs_tcon *tcon, struct smb_vol *pvolume_info)
> >>  {
> >>       __u64 unix_cap = le64_to_cpu(tcon->fsUnixInfo.Capability);
> >>       struct TCP_Server_Info *server = tcon->ses->server;
> >> -     unsigned int wsize = pvolume_info->wsize ? pvolume_info->wsize :
> >> -                             CIFS_DEFAULT_IOSIZE;
> >> +     unsigned int wsize;
> >> +
> >> +     /* start with specified wsize, or default */
> >> +     if (pvolume_info->wsize)
> >> +             wsize = pvolume_info->wsize;
> >> +     else if (tcon->unix_ext && (unix_cap & CIFS_UNIX_LARGE_WRITE_CAP))
> >> +             wsize = CIFS_DEFAULT_IOSIZE;
> >> +     else
> >> +             wsize = CIFS_DEFAULT_NON_POSIX_WSIZE;
> >>
> >>       /* can server support 24-bit write sizes? (via UNIX extensions) */
> >>       if (!tcon->unix_ext || !(unix_cap & CIFS_UNIX_LARGE_WRITE_CAP))
> >
> >
> > I should also mention that I suspect that this will fix this bug
> > without needing extra options:
> >
> >    https://bugzilla.samba.org/show_bug.cgi?id=8559
> >
> > If it looks acceptable, this might be suitable for stable too.
> > --
> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >
> 
> 
> 


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

Re: [PATCH] cifs: lower default wsize when unix extensions are not used

[Steve French]

Part of the issue is whether we view this as a server "bug" or not.
Reading the documentation it looks to me like a server bug if
it doesn't handle writes up to almost 128K when the flag is on
(even though Windows client doesn't us send requests for over 64K,
or at least rarely sends over 64K).  I don't remember seeing problems
with this to any Windows servers, but clearly if more than one
major server (NetApp or EMC etc., not just Solaris) had this problem,
your argument makes sense.  If this is just a bug in a single server
implementation, then detecting that and working around it probably
makes more sense than punishing NetApp, EMC and Windows
for another vendors bug.  Do we have data on problems to
any other servers with large writes?

On Wed, Nov 9, 2011 at 3:04 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Wed, 9 Nov 2011 14:55:36 -0600
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> How much of a performance hit does this make?  I would expect that
>> dropping the writesize by more than 50% to most non-Samba servers to
>> work around a bug in Solaris seems extreme.  If this hurts our
>> performance measurably, it may be more pragmatic to limit the wsize
>> change to a subset of these (e.g. based on server type) - seems more
>> fair to not punish other servers for a Solaris bug.
>>
>
> I'm not sure how big a hit it will be, but it won't be 0. As always, it
> depends on workload. The problem is, I'm not sure we can reliably
> detect when the server can't handle larger writes like this.
>
> Most servers are fine with larger writes, but when it's not then the
> result is failed writeback. Given that a lot of userspace code doesn't
> check the return code on close(), then this manifests itself as
> "silent" data corruption.
>
> So, while I don't like it, I think we're basically of constrained by
> Windows' behavior here. We really have to ensure that the defaults work
> for everyone, even if they're not what we'd consider ideal.
>
> Note too that this just sets the default wsize to 65536 in this
> situation. There's nothing that prevents someone from setting it higher
> if they have a server that can handle larger writes.
>
>> On Wed, Nov 9, 2011 at 12:41 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > On Wed,  9 Nov 2011 13:37:23 -0500
>> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> >
>> >> We've had some reports of servers (namely, the Solaris in-kernel CIFS
>> >> server) that don't deal properly with writes that are "too large" even
>> >> though they set CAP_LARGE_WRITE_ANDX. Change the default to better
>> >> mirror what windows clients do.
>> >>
>> >> Cc: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
>> >> Reported-by: Nick Davis <phireph0x-/E1597aS9LQAvxtiuMwx3w@public.gmane.org>
>> >> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> >> ---
>> >>  fs/cifs/connect.c |   23 +++++++++++++++++++----
>> >>  1 files changed, 19 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> >> index d6a972d..bf82f88 100644
>> >> --- a/fs/cifs/connect.c
>> >> +++ b/fs/cifs/connect.c
>> >> @@ -2912,18 +2912,33 @@ void cifs_setup_cifs_sb(struct smb_vol *pvolume_info,
>> >>  #define CIFS_DEFAULT_IOSIZE (1024 * 1024)
>> >>
>> >>  /*
>> >> - * Windows only supports a max of 60k reads. Default to that when posix
>> >> - * extensions aren't in force.
>> >> + * Windows only supports a max of 60kb reads and 65535 byte writes. Default to
>> >> + * those values when posix extensions aren't in force. In actuality here, we
>> >> + * use 65536 to allow for a write that is a multiple of 4k. Most servers seem
>> >> + * to be ok with the extra byte even though Windows doesn't send writes that
>> >> + * are that large.
>> >> + *
>> >> + * Citation:
>> >> + *
>> >> + * http://blogs.msdn.com/b/openspecification/archive/2009/04/10/smb-maximum-transmit-buffer-size-and-performance-tuning.aspx
>> >>   */
>> >>  #define CIFS_DEFAULT_NON_POSIX_RSIZE (60 * 1024)
>> >> +#define CIFS_DEFAULT_NON_POSIX_WSIZE (65536)
>> >>
>> >>  static unsigned int
>> >>  cifs_negotiate_wsize(struct cifs_tcon *tcon, struct smb_vol *pvolume_info)
>> >>  {
>> >>       __u64 unix_cap = le64_to_cpu(tcon->fsUnixInfo.Capability);
>> >>       struct TCP_Server_Info *server = tcon->ses->server;
>> >> -     unsigned int wsize = pvolume_info->wsize ? pvolume_info->wsize :
>> >> -                             CIFS_DEFAULT_IOSIZE;
>> >> +     unsigned int wsize;
>> >> +
>> >> +     /* start with specified wsize, or default */
>> >> +     if (pvolume_info->wsize)
>> >> +             wsize = pvolume_info->wsize;
>> >> +     else if (tcon->unix_ext && (unix_cap & CIFS_UNIX_LARGE_WRITE_CAP))
>> >> +             wsize = CIFS_DEFAULT_IOSIZE;
>> >> +     else
>> >> +             wsize = CIFS_DEFAULT_NON_POSIX_WSIZE;
>> >>
>> >>       /* can server support 24-bit write sizes? (via UNIX extensions) */
>> >>       if (!tcon->unix_ext || !(unix_cap & CIFS_UNIX_LARGE_WRITE_CAP))
>> >
>> >
>> > I should also mention that I suspect that this will fix this bug
>> > without needing extra options:
>> >
>> >    https://bugzilla.samba.org/show_bug.cgi?id=8559
>> >
>> > If it looks acceptable, this might be suitable for stable too.
>> > --
>> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> >
>>
>>
>>
>
>
> --
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>



-- 
Thanks,

Steve

Re: [PATCH] cifs: lower default wsize when unix extensions are not used

[Jeff Layton]

On Wed, 9 Nov 2011 15:14:30 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Part of the issue is whether we view this as a server "bug" or not.
> Reading the documentation it looks to me like a server bug if
> it doesn't handle writes up to almost 128K when the flag is on
> (even though Windows client doesn't us send requests for over 64K,
> or at least rarely sends over 64K).  I don't remember seeing problems
> with this to any Windows servers, but clearly if more than one
> major server (NetApp or EMC etc., not just Solaris) had this problem,
> your argument makes sense.  If this is just a bug in a single server
> implementation, then detecting that and working around it probably
> makes more sense than punishing NetApp, EMC and Windows
> for another vendors bug.  Do we have data on problems to
> any other servers with large writes?
> 

I'm not sure and that's a problem. For every user like Nick that takes
the time to report these sorts of issues, we may have dozens that
just give up and walk away when they hit them.

I don't see anywhere in the spec that mentions the (128k - 1) limit on
writes. The description of the CAP_LARGE_WRITE_ANDX flag just says that
if it's set then you can exceed the MaxBufferSize on a WRITE_ANDX.

What it doesn't say (at least not anywhere that I could find) is how
much larger it's allowed to be. Clearly (128k - 1) is a practical limit
since you're not supposed to exceed the RFC1002 size, but it's not a
clear-cut bug if the server doesn't allow up to that limit.

Again, I think we need to be very conservative with the defaults. We
already set the rsize to the Windows default. We should do the same
with the wsize to ensure the default settings just work, regardless of
the server.

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

Re: [PATCH] cifs: lower default wsize when unix extensions are not used

[Jeff Layton]

On Wed, 9 Nov 2011 15:36:04 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Wed, Nov 9, 2011 at 3:34 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > On Wed, 9 Nov 2011 15:14:30 -0600
> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> > > Part of the issue is whether we view this as a server "bug" or not.
> > > Reading the documentation it looks to me like a server bug if
> > > it doesn't handle writes up to almost 128K when the flag is on
> > > (even though Windows client doesn't us send requests for over 64K,
> > > or at least rarely sends over 64K).  I don't remember seeing problems
> > > with this to any Windows servers, but clearly if more than one
> > > major server (NetApp or EMC etc., not just Solaris) had this problem,
> > > your argument makes sense.  If this is just a bug in a single server
> > > implementation, then detecting that and working around it probably
> > > makes more sense than punishing NetApp, EMC and Windows
> > > for another vendors bug.  Do we have data on problems to
> > > any other servers with large writes?
> > >
> >
> > I'm not sure and that's a problem. For every user like Nick that takes
> > the time to report these sorts of issues, we may have dozens that
> > just give up and walk away when they hit them.
> >
> >
> Perhaps - but I think we would have hit these with fsx or connectathon test
> suite at the last (SNIA) test event if it were widespread.
> 

Again, we don't know. This is 100% dependent on what the server
supports. There is no way to test every possible cifs server that's in
the field. All we can do is follow the spec when there is one.

When the spec is unclear (as it is in this situation), then we really
have little choice but to look to the reference implementation for
guidance (aka Windows).

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

Re: [PATCH] cifs: lower default wsize when unix extensions are not used

[Shirish Pargaonkar]

On Wed, Nov 9, 2011 at 12:37 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> We've had some reports of servers (namely, the Solaris in-kernel CIFS
> server) that don't deal properly with writes that are "too large" even
> though they set CAP_LARGE_WRITE_ANDX. Change the default to better
> mirror what windows clients do.
>
> Cc: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> Reported-by: Nick Davis <phireph0x-/E1597aS9LQAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/connect.c |   23 +++++++++++++++++++----
>  1 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index d6a972d..bf82f88 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2912,18 +2912,33 @@ void cifs_setup_cifs_sb(struct smb_vol *pvolume_info,
>  #define CIFS_DEFAULT_IOSIZE (1024 * 1024)
>
>  /*
> - * Windows only supports a max of 60k reads. Default to that when posix
> - * extensions aren't in force.
> + * Windows only supports a max of 60kb reads and 65535 byte writes. Default to
> + * those values when posix extensions aren't in force. In actuality here, we
> + * use 65536 to allow for a write that is a multiple of 4k. Most servers seem
> + * to be ok with the extra byte even though Windows doesn't send writes that
> + * are that large.
> + *
> + * Citation:
> + *
> + * http://blogs.msdn.com/b/openspecification/archive/2009/04/10/smb-maximum-transmit-buffer-size-and-performance-tuning.aspx
>  */
>  #define CIFS_DEFAULT_NON_POSIX_RSIZE (60 * 1024)
> +#define CIFS_DEFAULT_NON_POSIX_WSIZE (65536)
>
>  static unsigned int
>  cifs_negotiate_wsize(struct cifs_tcon *tcon, struct smb_vol *pvolume_info)
>  {
>        __u64 unix_cap = le64_to_cpu(tcon->fsUnixInfo.Capability);
>        struct TCP_Server_Info *server = tcon->ses->server;
> -       unsigned int wsize = pvolume_info->wsize ? pvolume_info->wsize :
> -                               CIFS_DEFAULT_IOSIZE;
> +       unsigned int wsize;
> +
> +       /* start with specified wsize, or default */
> +       if (pvolume_info->wsize)
> +               wsize = pvolume_info->wsize;
> +       else if (tcon->unix_ext && (unix_cap & CIFS_UNIX_LARGE_WRITE_CAP))
> +               wsize = CIFS_DEFAULT_IOSIZE;
> +       else
> +               wsize = CIFS_DEFAULT_NON_POSIX_WSIZE;
>
>        /* can server support 24-bit write sizes? (via UNIX extensions) */
>        if (!tcon->unix_ext || !(unix_cap & CIFS_UNIX_LARGE_WRITE_CAP))
> --
> 1.7.6.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Does this change warrant any change to mount.cifs man page as well?

Re: [PATCH] cifs: lower default wsize when unix extensions are not used

[Jeff Layton]

On Wed, 9 Nov 2011 17:10:48 -0600
Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Wed, Nov 9, 2011 at 12:37 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > We've had some reports of servers (namely, the Solaris in-kernel CIFS
> > server) that don't deal properly with writes that are "too large" even
> > though they set CAP_LARGE_WRITE_ANDX. Change the default to better
> > mirror what windows clients do.
> >
> > Cc: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> > Reported-by: Nick Davis <phireph0x-/E1597aS9LQAvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/cifs/connect.c |   23 +++++++++++++++++++----
> >  1 files changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index d6a972d..bf82f88 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -2912,18 +2912,33 @@ void cifs_setup_cifs_sb(struct smb_vol *pvolume_info,
> >  #define CIFS_DEFAULT_IOSIZE (1024 * 1024)
> >
> >  /*
> > - * Windows only supports a max of 60k reads. Default to that when posix
> > - * extensions aren't in force.
> > + * Windows only supports a max of 60kb reads and 65535 byte writes. Default to
> > + * those values when posix extensions aren't in force. In actuality here, we
> > + * use 65536 to allow for a write that is a multiple of 4k. Most servers seem
> > + * to be ok with the extra byte even though Windows doesn't send writes that
> > + * are that large.
> > + *
> > + * Citation:
> > + *
> > + * http://blogs.msdn.com/b/openspecification/archive/2009/04/10/smb-maximum-transmit-buffer-size-and-performance-tuning.aspx
> >  */
> >  #define CIFS_DEFAULT_NON_POSIX_RSIZE (60 * 1024)
> > +#define CIFS_DEFAULT_NON_POSIX_WSIZE (65536)
> >
> >  static unsigned int
> >  cifs_negotiate_wsize(struct cifs_tcon *tcon, struct smb_vol *pvolume_info)
> >  {
> >        __u64 unix_cap = le64_to_cpu(tcon->fsUnixInfo.Capability);
> >        struct TCP_Server_Info *server = tcon->ses->server;
> > -       unsigned int wsize = pvolume_info->wsize ? pvolume_info->wsize :
> > -                               CIFS_DEFAULT_IOSIZE;
> > +       unsigned int wsize;
> > +
> > +       /* start with specified wsize, or default */
> > +       if (pvolume_info->wsize)
> > +               wsize = pvolume_info->wsize;
> > +       else if (tcon->unix_ext && (unix_cap & CIFS_UNIX_LARGE_WRITE_CAP))
> > +               wsize = CIFS_DEFAULT_IOSIZE;
> > +       else
> > +               wsize = CIFS_DEFAULT_NON_POSIX_WSIZE;
> >
> >        /* can server support 24-bit write sizes? (via UNIX extensions) */
> >        if (!tcon->unix_ext || !(unix_cap & CIFS_UNIX_LARGE_WRITE_CAP))
> > --
> > 1.7.6.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> 
> Does this change warrant any change to mount.cifs man page as well?

Yes, it does. I want to confirm that Steve plans to merge it first, but
I plan to do a manpage update to document it.

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

Re: [PATCH] cifs: lower default wsize when unix extensions are not used

[Björn JACKE]

On 2011-11-09 at 16:04 -0500 Jeff Layton sent off:
> On Wed, 9 Nov 2011 14:55:36 -0600
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > How much of a performance hit does this make?  I would expect that
> > dropping the writesize by more than 50% to most non-Samba servers to
> > work around a bug in Solaris seems extreme.  If this hurts our
> > performance measurably, it may be more pragmatic to limit the wsize
> > change to a subset of these (e.g. based on server type) - seems more
> > fair to not punish other servers for a Solaris bug.
> > 
> 
> I'm not sure how big a hit it will be, but it won't be 0. As always, it
> depends on workload. The problem is, I'm not sure we can reliably
> detect when the server can't handle larger writes like this.

I've seen a performance drop from 60MB/s to 50MB/s on a 1GB switched network
when unix extensions were disabled just because the rsize was limited from 128k
to 64k. That was tested with Darwin's smbfs but the dropdown in performance of
cifs vfs will be similar. If this can be seen as a server bug I'd also like to
see Oracle fix their server and keep cifs vfs be able to use 128k.

Björn

Re: [PATCH] cifs: lower default wsize when unix extensions are not used

[Jeff Layton]

On Wed, 23 Nov 2011 19:34:05 +0100
Björn JACKE <bj-3ekOc4rQMZmzQB+pC5nmwQ@public.gmane.org> wrote:

> On 2011-11-09 at 16:04 -0500 Jeff Layton sent off:
> > On Wed, 9 Nov 2011 14:55:36 -0600
> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > 
> > > How much of a performance hit does this make?  I would expect that
> > > dropping the writesize by more than 50% to most non-Samba servers to
> > > work around a bug in Solaris seems extreme.  If this hurts our
> > > performance measurably, it may be more pragmatic to limit the wsize
> > > change to a subset of these (e.g. based on server type) - seems more
> > > fair to not punish other servers for a Solaris bug.
> > > 
> > 
> > I'm not sure how big a hit it will be, but it won't be 0. As always, it
> > depends on workload. The problem is, I'm not sure we can reliably
> > detect when the server can't handle larger writes like this.
> 
> I've seen a performance drop from 60MB/s to 50MB/s on a 1GB switched network
> when unix extensions were disabled just because the rsize was limited from 128k
> to 64k. That was tested with Darwin's smbfs but the dropdown in performance of
> cifs vfs will be similar. If this can be seen as a server bug I'd also like to
> see Oracle fix their server and keep cifs vfs be able to use 128k.
> 

That 64k is just a default. There's nothing that stops you from setting
the rsize or wsize higher in this situation. You can go up to ~128k for
the rsize too if you know your server can handle it.

The Solaris/Illumos problem is apparently a kernel bug, and Gordon Ross
has apparently fixed it for Illumos. My concern though is that we don't
know if there are other servers with similar limitations out there in
the field. Windows never sends anything larger than 64k. There are
potentially a lot of servers out there that can't handle a larger wsize.

Even if it's *just* Solaris kernels that are broken this way, there are
still a number of those servers in the field. By defaulting larger
we're putting those users' data at risk.

Ultimately, this is Steve's call, even if I think his logic is flawed.
I would however appreciate a clear NAK on this patch if he's not going
to take it as I have to decide what to do for RHEL and Fedora.

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

Re: [PATCH] cifs: lower default wsize when unix extensions are not used

[Jeff Layton]

On Wed, 23 Nov 2011 23:26:08 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> We are waiting a dochelp response from ms. In addition more data on the
> perf penalty of cutting  the wsize in half would help. In any case thecmore
> serious problem is ignoring the write error not the wsize default

We know there'll be a perf penalty, and I've done some rough numbers
that show about a 30% decrease in some tests. The question is -- is it
a good idea to default to mount options that give better performance
at the expense of interoperability? My vote is no -- the defaults
should be as safe as possible, but we should allow people to set the
wsize higher if they choose. That's what the proposed patch does.

Also, most applications do not ignore write() errors, but a lot of
applications ignore errors on close(). Even the ones that do check for
errors on close() can't usually do much about it other than to log
the error or crash. It's therefore advantageous to avoid that situation
entirely if we can.

The dochelp response won't mean much, IMO. We know there are servers
with this limitation in the field, but they work with Windows. The
casual user will see that Windows works against those servers and Linux
corrupts data, and will conclude that Linux blows.

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

Re: [PATCH] cifs: lower default wsize when unix extensions are not used

[Steve French]

On Thu, Nov 24, 2011 at 6:20 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Wed, 23 Nov 2011 23:26:08 -0600
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> We are waiting a dochelp response from ms. In addition more data on the
>> perf penalty of cutting  the wsize in half would help. In any case thecmore
>> serious problem is ignoring the write error not the wsize default
>
> We know there'll be a perf penalty, and I've done some rough numbers
> that show about a 30% decrease in some tests. The question is -- is it
> a good idea to default to mount options that give better performance
> at the expense of interoperability? My vote is no -- the defaults

It depends ... one of the less common servers has a bug (which
apparently is being fixed) and it apparently hurts write
performance significantly to special case the wsize for this one server.
Implied in your answer is a worry that other servers incorrectly handle
the large write size but as we have seen there is a precedent for
writes over 64K (although apparently Windows clients do not send them
often - we will know more when Microsoft responds).   I am more
open to dropping the wsize to the value that fixes the problem to
Solaris (100K apparently) and has the least performance penalty
unless we get information back from Microsoft indicating a more
serious issue.  In any case there is no interoperability issue
if we handle the error on write (although the code path is tricky,
it is a fairly obvious example of "server tells us, late, that it
doesn't support this"
and if it were codeable to fall back wsize after the write error, there would be
no issue to solaris).

This is a bad week to get response from Microsoft - but assuming
they respond the following week we should have plenty of time to
determine if we need to drop the wsize - but I feel uncomfortable
ignoring the retry on fairly obvious write error (even though the code
path is hard) because that can cause loss of data.


-- 
Thanks,

Steve