[RFC,SMB3] Allow share to be mounted with "cache=ro" if immutable share
diff mbox series

Message ID CAH2r5mu_koRUCV9snYu_A6at8r+kJ85DgFszG4=seEEn+qb0LQ@mail.gmail.com
State New
Headers show
Series
  • [RFC,SMB3] Allow share to be mounted with "cache=ro" if immutable share
Related show

Commit Message

Steve French Aug. 28, 2019, 5:02 a.m. UTC
Increases performance a lot in cases where we know that the share is
not changing

Comments

ronnie sahlberg Aug. 28, 2019, 6:11 a.m. UTC | #1
On Wed, Aug 28, 2019 at 3:04 PM Steve French <smfrench@gmail.com> wrote:
>
> Increases performance a lot in cases where we know that the share is
> not changing

Good idea but we can do it without adding a new mount option.
Instead of "cache=ro" I think we should look at the WRITE bit in the
tree connect response access mask since this should tell us if the share is
read-only server-side or not.

If the share is thus read-only we should then have that reflected in the
"ro" mount option.

I.e.
* use TreeConnect/AccessMaks/WRITE flag to enable this more aggressive caching
and not a mount option.
* Probably also fail the mount if the WRITE flag is clear and the user
did not specify "ro"
as a mount option.

ronnie

>
>
>
> --
> Thanks,
>
> Steve
Aurelien Aptel Aug. 28, 2019, 10:20 a.m. UTC | #2
"Steve French" <smfrench@gmail.com> writes:
> Increases performance a lot in cases where we know that the share is
> not changing

This is just adding the parsing of the option but it sounds like a good idea.

> +#define CIFS_MOUNT_RO_CACHE	0x20000000  /* assumes share will not change */

This flag probably needs to be added to CIFS_MOUNT_MASK: if one cifs sb
is using that flag, it should only be reused for new mounts that also
require that flag.

Cheers,
ronnie sahlberg Aug. 28, 2019, 10:27 a.m. UTC | #3
On Wed, Aug 28, 2019 at 8:21 PM Aurélien Aptel <aaptel@suse.com> wrote:
>
> "Steve French" <smfrench@gmail.com> writes:
> > Increases performance a lot in cases where we know that the share is
> > not changing
>
> This is just adding the parsing of the option but it sounds like a good idea.

You also have the magic happening in:
-#define CIFS_CACHE_READ(cinode) (cinode->oplock & CIFS_CACHE_READ_FLG)
+#define CIFS_CACHE_READ(cinode) ((cinode->oplock &
CIFS_CACHE_READ_FLG) ||
(CIFS_SB(cinode->vfs_inode.i_sb)->mnt_cifs_flags &
CIFS_MOUNT_RO_CACHE))

which makes things work. Still I would want this to be driven by
whether the server returns "this share is WRITEABLE or  not" flag
instead of a mount option.
A mount option only invites people to "I use this because it makes
thing faster"  then "why do my files look corrupted sometimes" ?

>
> > +#define CIFS_MOUNT_RO_CACHE  0x20000000  /* assumes share will not change */
>
> This flag probably needs to be added to CIFS_MOUNT_MASK: if one cifs sb
> is using that flag, it should only be reused for new mounts that also
> require that flag.
>
> Cheers,
>
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
Aurelien Aptel Aug. 28, 2019, 10:54 a.m. UTC | #4
"ronnie sahlberg" <ronniesahlberg@gmail.com> writes:
> You also have the magic happening in:
> -#define CIFS_CACHE_READ(cinode) (cinode->oplock & CIFS_CACHE_READ_FLG)
> +#define CIFS_CACHE_READ(cinode) ((cinode->oplock &
> CIFS_CACHE_READ_FLG) ||
> (CIFS_SB(cinode->vfs_inode.i_sb)->mnt_cifs_flags &
> CIFS_MOUNT_RO_CACHE))

Ah thanks, I didn't notice that part.

> which makes things work. Still I would want this to be driven by
> whether the server returns "this share is WRITEABLE or  not" flag
> instead of a mount option.
> A mount option only invites people to "I use this because it makes
> thing faster"  then "why do my files look corrupted sometimes" ?

That's true but there are situations where it would be handy. Imagine
mounting //host/non_ro_share/sub/path/ro_folder i.e. mounting a folder
that you know won't change within a share that can change.

I guess this hints at a more granular cache mode.

Cheers,
Steve French Aug. 28, 2019, 10:17 p.m. UTC | #5
Since there isn't really a share flag which says "this share is
writable" (the access mask returned in tcon is only for that user -
not for any user), we could do the reverse ie forbid it if the server
forbids it, but otherwise just warn on it:

See e.g. SMB2_SHAREFLAG_NO_CACHING Offline caching MUST NOT occur.

On Wed, Aug 28, 2019 at 5:27 AM ronnie sahlberg
<ronniesahlberg@gmail.com> wrote:
>
> On Wed, Aug 28, 2019 at 8:21 PM Aurélien Aptel <aaptel@suse.com> wrote:
> >
> > "Steve French" <smfrench@gmail.com> writes:
> > > Increases performance a lot in cases where we know that the share is
> > > not changing
> >
> > This is just adding the parsing of the option but it sounds like a good idea.
>
> You also have the magic happening in:
> -#define CIFS_CACHE_READ(cinode) (cinode->oplock & CIFS_CACHE_READ_FLG)
> +#define CIFS_CACHE_READ(cinode) ((cinode->oplock &
> CIFS_CACHE_READ_FLG) ||
> (CIFS_SB(cinode->vfs_inode.i_sb)->mnt_cifs_flags &
> CIFS_MOUNT_RO_CACHE))
>
> which makes things work. Still I would want this to be driven by
> whether the server returns "this share is WRITEABLE or  not" flag
> instead of a mount option.
> A mount option only invites people to "I use this because it makes
> thing faster"  then "why do my files look corrupted sometimes" ?
>
> >
> > > +#define CIFS_MOUNT_RO_CACHE  0x20000000  /* assumes share will not change */
> >
> > This flag probably needs to be added to CIFS_MOUNT_MASK: if one cifs sb
> > is using that flag, it should only be reused for new mounts that also
> > require that flag.
> >
> > Cheers,
> >
> > --
> > Aurélien Aptel / SUSE Labs Samba Team
> > GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Patch
diff mbox series

From a2c56fa07718ec7de30866919c7ae442a72f8a5f Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Tue, 27 Aug 2019 23:58:54 -0500
Subject: [PATCH] smb3: add mount option to allow forced caching of read only
 share

If a share is immutable (at least for the period that it will
be mounted) it would be helpful to not have to revalidate
dentries repeatedly that we know can not be changed remotely.

Add "cache=" option (cache=ro) for mounting read only shares
in order to improve performance in cases in which we know that
the share will not be changing while it is in use.

Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/cifs_fs_sb.h |  1 +
 fs/cifs/cifsfs.c     |  2 ++
 fs/cifs/cifsglob.h   |  3 ++-
 fs/cifs/connect.c    | 14 ++++++++++++++
 4 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
index b326d2ca3765..286a104c4761 100644
--- a/fs/cifs/cifs_fs_sb.h
+++ b/fs/cifs/cifs_fs_sb.h
@@ -53,6 +53,7 @@ 
 #define CIFS_MOUNT_NO_HANDLE_CACHE 0x4000000 /* disable caching dir handles */
 #define CIFS_MOUNT_NO_DFS 0x8000000 /* disable DFS resolving */
 #define CIFS_MOUNT_MODE_FROM_SID 0x10000000 /* retrieve mode from special ACE */
+#define CIFS_MOUNT_RO_CACHE	0x20000000  /* assumes share will not change */
 
 struct cifs_sb_info {
 	struct rb_root tlink_tree;
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 3289b566463f..970251bc0661 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -400,6 +400,8 @@  cifs_show_cache_flavor(struct seq_file *s, struct cifs_sb_info *cifs_sb)
 		seq_puts(s, "strict");
 	else if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_DIRECT_IO)
 		seq_puts(s, "none");
+	else if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_RO_CACHE)
+		seq_puts(s, "ro"); /* read only caching assumed */
 	else
 		seq_puts(s, "loose");
 }
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index fe610e7e3670..f2ee201ec98a 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -559,6 +559,7 @@  struct smb_vol {
 	bool server_ino:1; /* use inode numbers from server ie UniqueId */
 	bool direct_io:1;
 	bool strict_io:1; /* strict cache behavior */
+	bool cache_ro:1;
 	bool remap:1;      /* set to remap seven reserved chars in filenames */
 	bool sfu_remap:1;  /* remap seven reserved chars ala SFU */
 	bool posix_paths:1; /* unset to not ask for posix pathnames. */
@@ -1366,7 +1367,7 @@  void cifsFileInfo_put(struct cifsFileInfo *cifs_file);
 #define CIFS_CACHE_RW_FLG	(CIFS_CACHE_READ_FLG | CIFS_CACHE_WRITE_FLG)
 #define CIFS_CACHE_RHW_FLG	(CIFS_CACHE_RW_FLG | CIFS_CACHE_HANDLE_FLG)
 
-#define CIFS_CACHE_READ(cinode) (cinode->oplock & CIFS_CACHE_READ_FLG)
+#define CIFS_CACHE_READ(cinode) ((cinode->oplock & CIFS_CACHE_READ_FLG) || (CIFS_SB(cinode->vfs_inode.i_sb)->mnt_cifs_flags & CIFS_MOUNT_RO_CACHE))
 #define CIFS_CACHE_HANDLE(cinode) (cinode->oplock & CIFS_CACHE_HANDLE_FLG)
 #define CIFS_CACHE_WRITE(cinode) (cinode->oplock & CIFS_CACHE_WRITE_FLG)
 
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 4fe559821aff..81cebf4c2269 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -298,6 +298,7 @@  enum {
 	Opt_cache_loose,
 	Opt_cache_strict,
 	Opt_cache_none,
+	Opt_cache_ro,
 	Opt_cache_err
 };
 
@@ -305,6 +306,7 @@  static const match_table_t cifs_cacheflavor_tokens = {
 	{ Opt_cache_loose, "loose" },
 	{ Opt_cache_strict, "strict" },
 	{ Opt_cache_none, "none" },
+	{ Opt_cache_ro, "ro" },
 	{ Opt_cache_err, NULL }
 };
 
@@ -1418,14 +1420,22 @@  cifs_parse_cache_flavor(char *value, struct smb_vol *vol)
 	case Opt_cache_loose:
 		vol->direct_io = false;
 		vol->strict_io = false;
+		vol->cache_ro = false;
 		break;
 	case Opt_cache_strict:
 		vol->direct_io = false;
 		vol->strict_io = true;
+		vol->cache_ro = false;
 		break;
 	case Opt_cache_none:
 		vol->direct_io = true;
 		vol->strict_io = false;
+		vol->cache_ro = false;
+		break;
+	case Opt_cache_ro:
+		vol->direct_io = false;
+		vol->strict_io = false;
+		vol->cache_ro = true;
 		break;
 	default:
 		cifs_dbg(VFS, "bad cache= option: %s\n", value);
@@ -4040,6 +4050,10 @@  int cifs_setup_cifs_sb(struct smb_vol *pvolume_info,
 		cifs_dbg(FYI, "mounting share using direct i/o\n");
 		cifs_sb->mnt_cifs_flags |= CIFS_MOUNT_DIRECT_IO;
 	}
+	if (pvolume_info->cache_ro) {
+		cifs_dbg(VFS, "mounting share with read only caching. Ensure that the share will not be modified while in use.\n");
+		cifs_sb->mnt_cifs_flags |= CIFS_MOUNT_RO_CACHE;
+	}
 	if (pvolume_info->mfsymlinks) {
 		if (pvolume_info->sfu_emul) {
 			/*
-- 
2.20.1