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@xxxxxxxxxx> wrote: > On Wed, 9 Nov 2011 14:55:36 -0600 > Steve French <smfrench@xxxxxxxxx> 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@xxxxxxxxxx> wrote: >> > On Wed, 9 Nov 2011 13:37:23 -0500 >> > Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxxx> >> >> Reported-by: Nick Davis <phireph0x@xxxxxxxxx> >> >> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx> >> >> --- >> >> 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@xxxxxxxxxx> >> > >> >> >> > > > -- > Jeff Layton <jlayton@xxxxxxxxxx> > -- 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
