[PATCH 0/4] cifs: pave way for change to "strictcache" by default

[PATCH 0/4] cifs: pave way for change to "strictcache" by default

[Jeff Layton]

cifs.ko currently does not follow the CIFS protocol as it should by
default. The protocol specifies (paraphrasing) "Thou shalt not cache
file data without an oplock." cifs.ko by default *does* do this however,
opting for a more NFS-like cache coherency scheme. It caches file data
in the pagecache, and invalidate it when the attributes look like the
file has changed.

Unfortunately, this can and does cause cache coherency problems. The
kernel can end up buffering up writes for longer than it should. In
particular, internal testing at RH with Samba/CTDB has shown cache
coherency problems when we don't mount on the clients with
strictcache:

    https://bugzilla.redhat.com/show_bug.cgi?id=742314

Up until now, we've shied away from changing this default primarily for
performance reasons. The code to handle uncached reads and writes was
synchronous, and very slow. With the addition of async uncached writes
in 3.4 and async uncached reads in 3.5, I think performance is good
enough that we ought to change the defaults to be "strictcache" for
better adherence to the protocol.

This patchset begins this transition by cleaning up the mount options
that we use to control the cache behavior, and adding some warnings
about the impending behavior change. I'm proposing that we follow the
kernel convention of warning for 2 releases about user-visible changes
and then we'll make the switch in 3.7.

At the same time, it corrects a problem with the current code. The
different cache behaviors are not considered to be mutually exclusive
even though they should be. If someone (for instance) specifies
"strictcache" and "fsc" the results are poorly defined. The first
patch corrects this.

If this looks acceptable, I'll also plan to spin up a manpage patch
to help document this change.

Jeff Layton (4):
  cifs: add a cache= option to better describe the different cache
    flavors
  cifs: add deprecation warnings to strictcache, forcedirectio, and fsc
  cifs: display cache= option in /proc/mounts
  cifs: add warning about change in default cache semantics in 3.7

 fs/cifs/cifsfs.c  |   22 +++++++++---
 fs/cifs/connect.c |   96 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 109 insertions(+), 9 deletions(-)

-- 
1.7.7.6

[PATCH 1/4] cifs: add a cache= option to better describe the different cache flavors

[Jeff Layton]

Currently, we have several mount options that control cifs' cache
behavior, but those options aren't considered to be mutually exclusive.
The result is poorly-defined when someone specifies more than one of
these options at mont time.

Fix this by adding a new cache= mount option that will supercede "fsc",
"strictcache", and "forcedirectio". That will help make it clear that
these options are mutually exclusive. Also, change the legacy options to
be mutually exclusive too, to ensure that users don't get surprises.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c |   74 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 71 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index f31dc9a..de967dc 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -102,7 +102,7 @@ enum {
 	Opt_srcaddr, Opt_prefixpath,
 	Opt_iocharset, Opt_sockopt,
 	Opt_netbiosname, Opt_servern,
-	Opt_ver, Opt_sec,
+	Opt_ver, Opt_sec, Opt_cache,
 
 	/* Mount options to be ignored */
 	Opt_ignore,
@@ -212,6 +212,7 @@ static const match_table_t cifs_mount_option_tokens = {
 	{ Opt_ver, "vers=%s" },
 	{ Opt_ver, "version=%s" },
 	{ Opt_sec, "sec=%s" },
+	{ Opt_cache, "cache=%s" },
 
 	{ Opt_ignore, "cred" },
 	{ Opt_ignore, "credentials" },
@@ -258,6 +259,23 @@ static const match_table_t cifs_secflavor_tokens = {
 	{ Opt_sec_err, NULL }
 };
 
+/* cache flavors */
+enum {
+	Opt_cache_fsc,
+	Opt_cache_loose,
+	Opt_cache_strict,
+	Opt_cache_none,
+	Opt_cache_err
+};
+
+static const match_table_t cifs_cacheflavor_tokens = {
+	{ Opt_cache_fsc, "fsc" },
+	{ Opt_cache_loose, "loose" },
+	{ Opt_cache_strict, "strict" },
+	{ Opt_cache_none, "none" },
+	{ Opt_cache_err, NULL }
+};
+
 static int ip_connect(struct TCP_Server_Info *server);
 static int generic_ip_connect(struct TCP_Server_Info *server);
 static void tlink_rb_insert(struct rb_root *root, struct tcon_link *new_tlink);
@@ -1183,6 +1201,42 @@ static int cifs_parse_security_flavors(char *value,
 }
 
 static int
+cifs_parse_cache_flavor(char *value, struct smb_vol *vol)
+{
+	substring_t args[MAX_OPT_ARGS];
+
+	switch (match_token(value, cifs_cacheflavor_tokens, args)) {
+	case Opt_cache_fsc:
+#ifndef CONFIG_CIFS_FSCACHE
+		cERROR(1, "cache=fsc support requires that the kernel be "
+			  "built with CONFIG_CIFS_FSCACHE enabled.");
+		return 1;
+#endif
+		vol->direct_io = false;
+		vol->strict_io = false;
+		vol->fsc = true;
+	case Opt_cache_loose:
+		vol->direct_io = false;
+		vol->strict_io = false;
+		vol->fsc = false;
+	case Opt_cache_strict:
+		vol->direct_io = false;
+		vol->strict_io = true;
+		vol->fsc = false;
+		break;
+	case Opt_cache_none:
+		vol->direct_io = true;
+		vol->strict_io = false;
+		vol->fsc = false;
+		break;
+	default:
+		cERROR(1, "bad cache= option: %s", value);
+		return 1;
+	}
+	return 0;
+}
+
+static int
 cifs_parse_mount_options(const char *mountdata, const char *devname,
 			 struct smb_vol *vol)
 {
@@ -1411,10 +1465,14 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
 			vol->seal = 1;
 			break;
 		case Opt_direct:
-			vol->direct_io = 1;
+			vol->fsc = false;
+			vol->direct_io = true;
+			vol->strict_io = false;
 			break;
 		case Opt_strictcache:
-			vol->strict_io = 1;
+			vol->fsc = false;
+			vol->direct_io = false;
+			vol->strict_io = true;
 			break;
 		case Opt_noac:
 			printk(KERN_WARNING "CIFS: Mount option noac not "
@@ -1428,6 +1486,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
 			goto cifs_parse_mount_err;
 #endif
 			vol->fsc = true;
+			vol->direct_io = false;
+			vol->strict_io = false;
 			break;
 		case Opt_mfsymlinks:
 			vol->mfsymlinks = true;
@@ -1835,6 +1895,14 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
 			if (cifs_parse_security_flavors(string, vol) != 0)
 				goto cifs_parse_mount_err;
 			break;
+		case Opt_cache:
+			string = match_strdup(args);
+			if (string == NULL)
+				goto out_nomem;
+
+			if (cifs_parse_cache_flavor(string, vol) != 0)
+				goto cifs_parse_mount_err;
+			break;
 		default:
 			/*
 			 * An option we don't recognize. Save it off for later
-- 
1.7.7.6

[PATCH 2/4] cifs: add deprecation warnings to strictcache, forcedirectio, and fsc

[Jeff Layton]

Leave them in for 2 releases and remove for 3.7.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index de967dc..604814d 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1468,11 +1468,17 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
 			vol->fsc = false;
 			vol->direct_io = true;
 			vol->strict_io = false;
+			cERROR(1, "The \"forcedirectio\" option will be "
+				"removed in 3.7. Please move to the cache=none "
+				"option.");
 			break;
 		case Opt_strictcache:
 			vol->fsc = false;
 			vol->direct_io = false;
 			vol->strict_io = true;
+			cERROR(1, "The \"strictcache\" option will be removed "
+				"in 3.7. Please move to the cache=strict "
+				"option.");
 			break;
 		case Opt_noac:
 			printk(KERN_WARNING "CIFS: Mount option noac not "
@@ -1488,6 +1494,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
 			vol->fsc = true;
 			vol->direct_io = false;
 			vol->strict_io = false;
+			cERROR(1, "The \"fsc\" option will be removed in "
+				"3.7. Please move to the cache=fsc option.");
 			break;
 		case Opt_mfsymlinks:
 			vol->mfsymlinks = true;
-- 
1.7.7.6

[PATCH 3/4] cifs: display cache= option in /proc/mounts

[Jeff Layton]

...and deprecate the display of strictcache, forcedirectio, and fsc
as separate options.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifsfs.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index d342128..646699e 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -329,6 +329,21 @@ cifs_show_security(struct seq_file *s, struct TCP_Server_Info *server)
 		seq_printf(s, "i");
 }
 
+static void
+cifs_show_cache_flavor(struct seq_file *s, struct cifs_sb_info *cifs_sb)
+{
+	seq_printf(s, ",cache=");
+
+	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_FSCACHE)
+		seq_printf(s, "fsc");
+	else if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO)
+		seq_printf(s, "strict");
+	else if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_DIRECT_IO)
+		seq_printf(s, "none");
+	else
+		seq_printf(s, "loose");
+}
+
 /*
  * cifs_show_options() is for displaying mount options in /proc/mounts.
  * Not all settable options are displayed but most of the important
@@ -343,6 +358,7 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
 	srcaddr = (struct sockaddr *)&tcon->ses->server->srcaddr;
 
 	cifs_show_security(s, tcon->ses->server);
+	cifs_show_cache_flavor(s, cifs_sb);
 
 	seq_printf(s, ",unc=%s", tcon->treeName);
 
@@ -408,8 +424,6 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
 		seq_printf(s, ",rwpidforward");
 	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOPOSIXBRL)
 		seq_printf(s, ",forcemand");
-	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_DIRECT_IO)
-		seq_printf(s, ",directio");
 	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_XATTR)
 		seq_printf(s, ",nouser_xattr");
 	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR)
@@ -426,14 +440,10 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
 		seq_printf(s, ",acl");
 	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MF_SYMLINKS)
 		seq_printf(s, ",mfsymlinks");
-	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_FSCACHE)
-		seq_printf(s, ",fsc");
 	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC)
 		seq_printf(s, ",nostrictsync");
 	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_PERM)
 		seq_printf(s, ",noperm");
-	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO)
-		seq_printf(s, ",strictcache");
 
 	seq_printf(s, ",rsize=%d", cifs_sb->rsize);
 	seq_printf(s, ",wsize=%d", cifs_sb->wsize);
-- 
1.7.7.6

[PATCH 4/4] cifs: add warning about change in default cache semantics in 3.7

[Jeff Layton]

Add a warning that will be displayed when there is no cache= option
specified. We want to ensure that users are aware of the change in
defaults coming in 3.7.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 604814d..38119e4 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1254,6 +1254,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
 	char *string = NULL;
 	char *tmp_end, *value;
 	char delim;
+	bool cache_specified = false;
+	static bool cache_warned = false;
 
 	separator[0] = ',';
 	separator[1] = 0;
@@ -1465,6 +1467,7 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
 			vol->seal = 1;
 			break;
 		case Opt_direct:
+			cache_specified = true;
 			vol->fsc = false;
 			vol->direct_io = true;
 			vol->strict_io = false;
@@ -1473,6 +1476,7 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
 				"option.");
 			break;
 		case Opt_strictcache:
+			cache_specified = true;
 			vol->fsc = false;
 			vol->direct_io = false;
 			vol->strict_io = true;
@@ -1491,6 +1495,7 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
 				  "kernel config option set");
 			goto cifs_parse_mount_err;
 #endif
+			cache_specified = true;
 			vol->fsc = true;
 			vol->direct_io = false;
 			vol->strict_io = false;
@@ -1904,6 +1909,7 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
 				goto cifs_parse_mount_err;
 			break;
 		case Opt_cache:
+			cache_specified = true;
 			string = match_strdup(args);
 			if (string == NULL)
 				goto out_nomem;
@@ -1954,6 +1960,14 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
 		printk(KERN_NOTICE "CIFS: ignoring forcegid mount option "
 				   "specified with no gid= option.\n");
 
+	/* FIXME: remove this block in 3.7 */
+	if (!cache_specified && !cache_warned) {
+		cache_warned = true;
+		printk(KERN_NOTICE "CIFS: no cache= option specified, using "
+				   "\"cache=loose\". This default will change "
+				   "to \"cache=strict\" in 3.7.\n");
+	}
+
 	kfree(mountdata_copy);
 	return 0;
 
-- 
1.7.7.6

Re: [PATCH 0/4] cifs: pave way for change to "strictcache" by default

[Pavel Shilovsky]

27 апреля 2012 г. 14:56 пользователь Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> написал:
> cifs.ko currently does not follow the CIFS protocol as it should by
> default. The protocol specifies (paraphrasing) "Thou shalt not cache
> file data without an oplock." cifs.ko by default *does* do this however,
> opting for a more NFS-like cache coherency scheme. It caches file data
> in the pagecache, and invalidate it when the attributes look like the
> file has changed.
>
> Unfortunately, this can and does cause cache coherency problems. The
> kernel can end up buffering up writes for longer than it should. In
> particular, internal testing at RH with Samba/CTDB has shown cache
> coherency problems when we don't mount on the clients with
> strictcache:
>
>    https://bugzilla.redhat.com/show_bug.cgi?id=742314
>
> Up until now, we've shied away from changing this default primarily for
> performance reasons. The code to handle uncached reads and writes was
> synchronous, and very slow. With the addition of async uncached writes
> in 3.4 and async uncached reads in 3.5, I think performance is good
> enough that we ought to change the defaults to be "strictcache" for
> better adherence to the protocol.
>
> This patchset begins this transition by cleaning up the mount options
> that we use to control the cache behavior, and adding some warnings
> about the impending behavior change. I'm proposing that we follow the
> kernel convention of warning for 2 releases about user-visible changes
> and then we'll make the switch in 3.7.
>
> At the same time, it corrects a problem with the current code. The
> different cache behaviors are not considered to be mutually exclusive
> even though they should be. If someone (for instance) specifies
> "strictcache" and "fsc" the results are poorly defined. The first
> patch corrects this.
>
> If this looks acceptable, I'll also plan to spin up a manpage patch
> to help document this change.
>
> Jeff Layton (4):
>  cifs: add a cache= option to better describe the different cache
>    flavors
>  cifs: add deprecation warnings to strictcache, forcedirectio, and fsc
>  cifs: display cache= option in /proc/mounts
>  cifs: add warning about change in default cache semantics in 3.7
>
>  fs/cifs/cifsfs.c  |   22 +++++++++---
>  fs/cifs/connect.c |   96 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 109 insertions(+), 9 deletions(-)
>
> --
> 1.7.7.6
>
> --
> 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

The patchset looks good and resonable.

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

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 0/4] cifs: pave way for change to "strictcache" by default

[Jeff Layton]

On Fri, 27 Apr 2012 06:56:15 -0400
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> cifs.ko currently does not follow the CIFS protocol as it should by
> default. The protocol specifies (paraphrasing) "Thou shalt not cache
> file data without an oplock." cifs.ko by default *does* do this however,
> opting for a more NFS-like cache coherency scheme. It caches file data
> in the pagecache, and invalidate it when the attributes look like the
> file has changed.
> 
> Unfortunately, this can and does cause cache coherency problems. The
> kernel can end up buffering up writes for longer than it should. In
> particular, internal testing at RH with Samba/CTDB has shown cache
> coherency problems when we don't mount on the clients with
> strictcache:
> 
>     https://bugzilla.redhat.com/show_bug.cgi?id=742314
> 
> Up until now, we've shied away from changing this default primarily for
> performance reasons. The code to handle uncached reads and writes was
> synchronous, and very slow. With the addition of async uncached writes
> in 3.4 and async uncached reads in 3.5, I think performance is good
> enough that we ought to change the defaults to be "strictcache" for
> better adherence to the protocol.
> 
> This patchset begins this transition by cleaning up the mount options
> that we use to control the cache behavior, and adding some warnings
> about the impending behavior change. I'm proposing that we follow the
> kernel convention of warning for 2 releases about user-visible changes
> and then we'll make the switch in 3.7.
> 
> At the same time, it corrects a problem with the current code. The
> different cache behaviors are not considered to be mutually exclusive
> even though they should be. If someone (for instance) specifies
> "strictcache" and "fsc" the results are poorly defined. The first
> patch corrects this.
> 
> If this looks acceptable, I'll also plan to spin up a manpage patch
> to help document this change.
> 

(cc'ing samba-technical too for those that don't follow linux-cifs...)

I discussed this some via IM with Steve last week and he has his doubts
about whether we should do this. I think we ought to bring this
discussion to the lists though so we can try to settle this in time for
the next kernel merge window.

The (valid) point that he brings up is that this will harm performance
when an application is doing small I/Os sizes and does not have an
oplock. This is true -- reads and writes will be synchronous and will
match the sizes that come from userspace. An application doing small
I/Os will suffer performance-wise when it does not have an oplock on an
open file.

He also claims that Windows will still buffer up writes when it doesn't
have an oplock in an effort to do larger I/Os on the wire. I've got a
dochelp request into MS to clarify this point and what the protocol
mandates, but I don't think the answer there will have much bearing on
what we should do. There are a number of problems with keeping
"cache=loose" the default...

When we do I/O out of the pagecache, then we more or less do it along
page boundaries. If another client has locked a region in the middle of
a page, then that request will generally fail. We have no way to know
what region is locked, so we have no way to handle this case.

We also have quite a few documented cases where using "strictcache" has
fixed cache coherency problems. Here's one, for instance, but there are
others:

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

I think it's important that cifs.ko is safe to use by default, even if
it's a little slower. We can always tell people "try cache=loose" if
their workload is slow with "cache=strict", but having defaults that
can munch your data is really not acceptable. 

I guess the question we have to answer is:

"Is it worth risking data corruption in order to eke out better
performance when a user is doing small I/Os and does not have an oplock?"

To me, the answer is an unqualified "no". In most cases, you don't have
an oplock because another client has the file open. In those cases,
cache coherency generally trumps performance. In cases where it
doesn't, a user has the freedom to mount with "cache=loose" with the
understanding of all that that entails.

Thoughts?
-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH 0/4] cifs: pave way for change to "strictcache" by default

[Steve French]

There is a related question of how deferring writes slightly (e.g. to
buffer a full page or a full smb buffer) could corrupt data?

AFAIK - there isn't an ordering guarantee in posix when writes from
two different applications overlap (thus the need for byte range locks
or other synchronization mechanisms, or flush/fsync).

On Mon, Apr 30, 2012 at 6:28 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Fri, 27 Apr 2012 06:56:15 -0400
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
>> cifs.ko currently does not follow the CIFS protocol as it should by
>> default. The protocol specifies (paraphrasing) "Thou shalt not cache
>> file data without an oplock." cifs.ko by default *does* do this however,
>> opting for a more NFS-like cache coherency scheme. It caches file data
>> in the pagecache, and invalidate it when the attributes look like the
>> file has changed.
>>
>> Unfortunately, this can and does cause cache coherency problems. The
>> kernel can end up buffering up writes for longer than it should. In
>> particular, internal testing at RH with Samba/CTDB has shown cache
>> coherency problems when we don't mount on the clients with
>> strictcache:
>>
>>     https://bugzilla.redhat.com/show_bug.cgi?id=742314
>>
>> Up until now, we've shied away from changing this default primarily for
>> performance reasons. The code to handle uncached reads and writes was
>> synchronous, and very slow. With the addition of async uncached writes
>> in 3.4 and async uncached reads in 3.5, I think performance is good
>> enough that we ought to change the defaults to be "strictcache" for
>> better adherence to the protocol.
>>
>> This patchset begins this transition by cleaning up the mount options
>> that we use to control the cache behavior, and adding some warnings
>> about the impending behavior change. I'm proposing that we follow the
>> kernel convention of warning for 2 releases about user-visible changes
>> and then we'll make the switch in 3.7.
>>
>> At the same time, it corrects a problem with the current code. The
>> different cache behaviors are not considered to be mutually exclusive
>> even though they should be. If someone (for instance) specifies
>> "strictcache" and "fsc" the results are poorly defined. The first
>> patch corrects this.
>>
>> If this looks acceptable, I'll also plan to spin up a manpage patch
>> to help document this change.
>>
>
> (cc'ing samba-technical too for those that don't follow linux-cifs...)
>
> I discussed this some via IM with Steve last week and he has his doubts
> about whether we should do this. I think we ought to bring this
> discussion to the lists though so we can try to settle this in time for
> the next kernel merge window.
>
> The (valid) point that he brings up is that this will harm performance
> when an application is doing small I/Os sizes and does not have an
> oplock. This is true -- reads and writes will be synchronous and will
> match the sizes that come from userspace. An application doing small
> I/Os will suffer performance-wise when it does not have an oplock on an
> open file.
>
> He also claims that Windows will still buffer up writes when it doesn't
> have an oplock in an effort to do larger I/Os on the wire. I've got a
> dochelp request into MS to clarify this point and what the protocol
> mandates, but I don't think the answer there will have much bearing on
> what we should do. There are a number of problems with keeping
> "cache=loose" the default...
>
> When we do I/O out of the pagecache, then we more or less do it along
> page boundaries. If another client has locked a region in the middle of
> a page, then that request will generally fail. We have no way to know
> what region is locked, so we have no way to handle this case.
>
> We also have quite a few documented cases where using "strictcache" has
> fixed cache coherency problems. Here's one, for instance, but there are
> others:
>
>    https://bugzilla.samba.org/show_bug.cgi?id=6174
>
> I think it's important that cifs.ko is safe to use by default, even if
> it's a little slower. We can always tell people "try cache=loose" if
> their workload is slow with "cache=strict", but having defaults that
> can munch your data is really not acceptable.
>
> I guess the question we have to answer is:
>
> "Is it worth risking data corruption in order to eke out better
> performance when a user is doing small I/Os and does not have an oplock?"
>
> To me, the answer is an unqualified "no". In most cases, you don't have
> an oplock because another client has the file open. In those cases,
> cache coherency generally trumps performance. In cases where it
> doesn't, a user has the freedom to mount with "cache=loose" with the
> understanding of all that that entails.
>
> Thoughts?
> --
> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>



-- 
Thanks,

Steve

Re: [PATCH 0/4] cifs: pave way for change to "strictcache" by default

[Jeff Layton]

On Mon, 30 Apr 2012 10:11:43 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> There is a related question of how deferring writes slightly (e.g. to
> buffer a full page or a full smb buffer) could corrupt data?
> 

You're returning from write(2) without actually sending it to the
backing store, but a read(2) will go to that backing store. So,
applications that expect the results of that write to be visible to
other clients (or even on the same client) won't see it.

> AFAIK - there isn't an ordering guarantee in posix when writes from
> two different applications overlap (thus the need for byte range locks
> or other synchronization mechanisms, or flush/fsync).
> 

Correct, there isn't, but even locking doesn't really help with cifs.ko
since it doesn't treat locks as cache coherency points like the nfs
client does. Even if it did though, the NFS cache coherency model is not
necessarily something we want to emulate. It's pretty fragile itself.
Coarse-grained timestamps on the underlying fs can easily break it for
instance.

To compound the problem, windows servers tend to be quite lazy about
mtime updates. So even if you do proper locking, there's no guarantee
that the "loose" cache coherency algorithm in cifs.ko will detect that
the file has changed. Another client might do some writes but the mtime
doesn't necessarily change as a result.

I'm certainly aware that this might slow down performance in some
cases. Those people however can always switch to the less-safe
cache=loose if they wish. By default however, we should attempt to
follow the letter of the protocol as best we can...

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH 0/4] cifs: pave way for change to "strictcache" by default

[Steve French]

On Mon, Apr 30, 2012 at 10:45 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Mon, 30 Apr 2012 10:11:43 -0500
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> There is a related question of how deferring writes slightly (e.g. to
>> buffer a full page or a full smb buffer) could corrupt data?
>>
>
> You're returning from write(2) without actually sending it to the
> backing store, but a read(2) will go to that backing store. So,
> applications that expect the results of that write to be visible to
> other clients (or even on the same client) won't see it.

well the results of the write MUST be visible to others reading
on the same client.  In what case would they not be
(other than perhaps some odd private memory mapping case)


-- 
Thanks,

Steve

Re: [PATCH 0/4] cifs: pave way for change to "strictcache" by default

[Jeff Layton]

On Mon, 30 Apr 2012 10:50:25 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Mon, Apr 30, 2012 at 10:45 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> > On Mon, 30 Apr 2012 10:11:43 -0500
> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> There is a related question of how deferring writes slightly (e.g. to
> >> buffer a full page or a full smb buffer) could corrupt data?
> >>
> >
> > You're returning from write(2) without actually sending it to the
> > backing store, but a read(2) will go to that backing store. So,
> > applications that expect the results of that write to be visible to
> > other clients (or even on the same client) won't see it.
> 
> well the results of the write MUST be visible to others reading
> on the same client.  In what case would they not be
> (other than perhaps some odd private memory mapping case)
> 
> 

If you're delaying the write, how are you going to satisfy that read?

Where will the results of that write be stored such that the read can
then see it? Typically the answer would be "the pagecache" but you
can't use it in this case.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH 1/4] cifs: add a cache= option to better describe the different cache flavors

[Steve French]

Why is "fsc" (in particular caching of reads, to file, for oplocked
files) mutually exclusive with "strictcache" (write through of
non-oplocked files).

On Fri, Apr 27, 2012 at 5:56 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Currently, we have several mount options that control cifs' cache
> behavior, but those options aren't considered to be mutually exclusive.
> The result is poorly-defined when someone specifies more than one of
> these options at mont time.
>
> Fix this by adding a new cache= mount option that will supercede "fsc",
> "strictcache", and "forcedirectio". That will help make it clear that
> these options are mutually exclusive. Also, change the legacy options to
> be mutually exclusive too, to ensure that users don't get surprises.
>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/connect.c |   74 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 71 insertions(+), 3 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index f31dc9a..de967dc 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -102,7 +102,7 @@ enum {
>        Opt_srcaddr, Opt_prefixpath,
>        Opt_iocharset, Opt_sockopt,
>        Opt_netbiosname, Opt_servern,
> -       Opt_ver, Opt_sec,
> +       Opt_ver, Opt_sec, Opt_cache,
>
>        /* Mount options to be ignored */
>        Opt_ignore,
> @@ -212,6 +212,7 @@ static const match_table_t cifs_mount_option_tokens = {
>        { Opt_ver, "vers=%s" },
>        { Opt_ver, "version=%s" },
>        { Opt_sec, "sec=%s" },
> +       { Opt_cache, "cache=%s" },
>
>        { Opt_ignore, "cred" },
>        { Opt_ignore, "credentials" },
> @@ -258,6 +259,23 @@ static const match_table_t cifs_secflavor_tokens = {
>        { Opt_sec_err, NULL }
>  };
>
> +/* cache flavors */
> +enum {
> +       Opt_cache_fsc,
> +       Opt_cache_loose,
> +       Opt_cache_strict,
> +       Opt_cache_none,
> +       Opt_cache_err
> +};
> +
> +static const match_table_t cifs_cacheflavor_tokens = {
> +       { Opt_cache_fsc, "fsc" },
> +       { Opt_cache_loose, "loose" },
> +       { Opt_cache_strict, "strict" },
> +       { Opt_cache_none, "none" },
> +       { Opt_cache_err, NULL }
> +};
> +
>  static int ip_connect(struct TCP_Server_Info *server);
>  static int generic_ip_connect(struct TCP_Server_Info *server);
>  static void tlink_rb_insert(struct rb_root *root, struct tcon_link *new_tlink);
> @@ -1183,6 +1201,42 @@ static int cifs_parse_security_flavors(char *value,
>  }
>
>  static int
> +cifs_parse_cache_flavor(char *value, struct smb_vol *vol)
> +{
> +       substring_t args[MAX_OPT_ARGS];
> +
> +       switch (match_token(value, cifs_cacheflavor_tokens, args)) {
> +       case Opt_cache_fsc:
> +#ifndef CONFIG_CIFS_FSCACHE
> +               cERROR(1, "cache=fsc support requires that the kernel be "
> +                         "built with CONFIG_CIFS_FSCACHE enabled.");
> +               return 1;
> +#endif
> +               vol->direct_io = false;
> +               vol->strict_io = false;
> +               vol->fsc = true;
> +       case Opt_cache_loose:
> +               vol->direct_io = false;
> +               vol->strict_io = false;
> +               vol->fsc = false;
> +       case Opt_cache_strict:
> +               vol->direct_io = false;
> +               vol->strict_io = true;
> +               vol->fsc = false;
> +               break;
> +       case Opt_cache_none:
> +               vol->direct_io = true;
> +               vol->strict_io = false;
> +               vol->fsc = false;
> +               break;
> +       default:
> +               cERROR(1, "bad cache= option: %s", value);
> +               return 1;
> +       }
> +       return 0;
> +}
> +
> +static int
>  cifs_parse_mount_options(const char *mountdata, const char *devname,
>                         struct smb_vol *vol)
>  {
> @@ -1411,10 +1465,14 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
>                        vol->seal = 1;
>                        break;
>                case Opt_direct:
> -                       vol->direct_io = 1;
> +                       vol->fsc = false;
> +                       vol->direct_io = true;
> +                       vol->strict_io = false;
>                        break;
>                case Opt_strictcache:
> -                       vol->strict_io = 1;
> +                       vol->fsc = false;
> +                       vol->direct_io = false;
> +                       vol->strict_io = true;
>                        break;
>                case Opt_noac:
>                        printk(KERN_WARNING "CIFS: Mount option noac not "
> @@ -1428,6 +1486,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
>                        goto cifs_parse_mount_err;
>  #endif
>                        vol->fsc = true;
> +                       vol->direct_io = false;
> +                       vol->strict_io = false;
>                        break;
>                case Opt_mfsymlinks:
>                        vol->mfsymlinks = true;
> @@ -1835,6 +1895,14 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
>                        if (cifs_parse_security_flavors(string, vol) != 0)
>                                goto cifs_parse_mount_err;
>                        break;
> +               case Opt_cache:
> +                       string = match_strdup(args);
> +                       if (string == NULL)
> +                               goto out_nomem;
> +
> +                       if (cifs_parse_cache_flavor(string, vol) != 0)
> +                               goto cifs_parse_mount_err;
> +                       break;
>                default:
>                        /*
>                         * An option we don't recognize. Save it off for later
> --
> 1.7.7.6
>



-- 
Thanks,

Steve

Re: [PATCH 1/4] cifs: add a cache= option to better describe the different cache flavors

[Jeff Layton]

On Tue, 15 May 2012 13:34:45 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Why is "fsc" (in particular caching of reads, to file, for oplocked
> files) mutually exclusive with "strictcache" (write through of
> non-oplocked files).
> 

The main purpose of cache=fsc is to allow you to cache file data between
reboots (or technically, between mounts). That is contingent on being
able to tell whether the cache is actually valid after rebooting
however.

cache=strict implies that you're following the cifs cache coherency
protocol to the letter. The CIFS protocol effectively states that you
are only allowed to cache file data while holding an oplock. Since you
can't hold an oplock over a reboot, trusting fsc cached data across one
would invalidate the guarantee that we try to provide with cache=strict.

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

Re: [PATCH 1/4] cifs: add a cache= option to better describe the different cache flavors

[Jeff Layton]

On Tue, 15 May 2012 15:02:56 -0400
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> On Tue, 15 May 2012 13:34:45 -0500
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > Why is "fsc" (in particular caching of reads, to file, for oplocked
> > files) mutually exclusive with "strictcache" (write through of
> > non-oplocked files).
> > 
> 
> The main purpose of cache=fsc is to allow you to cache file data between
> reboots (or technically, between mounts). That is contingent on being
> able to tell whether the cache is actually valid after rebooting
> however.
> 
> cache=strict implies that you're following the cifs cache coherency
> protocol to the letter. The CIFS protocol effectively states that you
> are only allowed to cache file data while holding an oplock. Since you
> can't hold an oplock over a reboot, trusting fsc cached data across one
> would invalidate the guarantee that we try to provide with cache=strict.
> 

...and since we're on the subject, I got this back today from my
dochelp query about the protocol and windows' behavior:

---------------------------[snip]------------------------------------
Hi Jeff:

The SMB protocols do not have any specific requirement as to how much
or how little caching is allowed on the client side. An implementation
could very well "choose to batch writes for a short period of time"
even in the absence of an oplock/lease.  However, then there are no
data consistency guarantees between multiple readers and writers.
Oplocks/leases provide a mechanism for implementers to guarantee better
data consistency.

Windows in general does not do caching in the absence of oplock/lease.
The specific conditions in which caching without oplock/lease may
happen is implementation detail, not protocol.

Please let me know if it answers your question.

Regards,
Obaid Farooqi
Escalation Engineer | Microsoft
---------------------------[snip]------------------------------------

Which means precisely zilch to us of course. The only thing we can do
is try to behave as sanely as possible. I maintain that cache=strict as
a default is the safest course of action since it allows for reasonable
data integrity and locking behavior when multiple clients are
interacting with the same files.

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