[PATCH 0/3] cifs: fix a few fscache issues

[PATCH 0/3] cifs: fix a few fscache issues

[Suresh Jayaraman]

This patchset fixes a couple of issues with fscache found by Jeff Layton. I
think these are straight-forward fixes and can be considered for next -rc.

Suresh Jayaraman (3):
   cifs: allow fsc mount option only if CONFIG_CIFS_FSCACHE is set
   cifs: enable fscache iff fsc mount option is used explicitly
   cifs: display fsc in /proc/mounts


 cifsfs.c  |    2 ++
 connect.c |    5 +++++
 fscache.c |   12 ++++++------
 3 files changed, 13 insertions(+), 6 deletions(-)

[PATCH 1/3] cifs: allow fsc mount option only if CONFIG_CIFS_FSCACHE is set

[Suresh Jayaraman]

Currently, it is possible to specify 'fsc' mount option even if
CONFIG_CIFS_FSCACHE has not been set. The option is being ignored silently
while the user fscache functionality to work. Fix this by raising error when
the CONFIG option is not set.


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

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 251a17c..32fa4d9 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1352,6 +1352,11 @@ cifs_parse_mount_options(char *options, const char *devname,
 				"supported. Instead set "
 				"/proc/fs/cifs/LookupCacheEnabled to 0\n");
 		} else if (strnicmp(data, "fsc", 3) == 0) {
+#ifndef CONFIG_CIFS_FSCACHE
+			cERROR(1, "FS-Cache support needs CONFIG_CIFS_FSCACHE"
+				  "kernel config option set");
+			return 1;
+#endif
 			vol->fsc = true;
 		} else if (strnicmp(data, "mfsymlinks", 10) == 0) {
 			vol->mfsymlinks = true;

[PATCH 2/3] cifs: enable fscache iff fsc mount option is used explicitly

[Suresh Jayaraman]

Currently, if CONFIG_CIFS_FSCACHE is set, fscache is enabled on files opened
as read-only irrespective of the 'fsc' mount option. Fix this by enabling
fscache only if 'fsc' mount option is specified explicitly.

Remove an extraneous cFYI debug message and fix a typo while at it.

Reported-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
---
 fs/cifs/fscache.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/fscache.c b/fs/cifs/fscache.c
index a2ad94e..297a43d 100644
--- a/fs/cifs/fscache.c
+++ b/fs/cifs/fscache.c
@@ -2,7 +2,7 @@
  *   fs/cifs/fscache.c - CIFS filesystem cache interface
  *
  *   Copyright (c) 2010 Novell, Inc.
- *   Author(s): Suresh Jayaraman (sjayaraman-l3A5Bk7waGM@public.gmane.org>
+ *   Author(s): Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
  *
  *   This library is free software; you can redistribute it and/or modify
  *   it under the terms of the GNU Lesser General Public License as published
@@ -67,10 +67,12 @@ static void cifs_fscache_enable_inode_cookie(struct inode *inode)
 	if (cifsi->fscache)
 		return;
 
-	cifsi->fscache = fscache_acquire_cookie(tcon->fscache,
+	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_FSCACHE) {
+		cifsi->fscache = fscache_acquire_cookie(tcon->fscache,
 				&cifs_fscache_inode_object_def, cifsi);
-	cFYI(1, "CIFS: got FH cookie (0x%p/0x%p)", tcon->fscache,
+		cFYI(1, "CIFS: got FH cookie (0x%p/0x%p)", tcon->fscache,
 				cifsi->fscache);
+	}
 }
 
 void cifs_fscache_release_inode_cookie(struct inode *inode)
@@ -101,10 +103,8 @@ void cifs_fscache_set_inode_cookie(struct inode *inode, struct file *filp)
 {
 	if ((filp->f_flags & O_ACCMODE) != O_RDONLY)
 		cifs_fscache_disable_inode_cookie(inode);
-	else {
+	else
 		cifs_fscache_enable_inode_cookie(inode);
-		cFYI(1, "CIFS: fscache inode cookie set");
-	}
 }
 
 void cifs_fscache_reset_inode_cookie(struct inode *inode)

[PATCH 3/3] cifs: display fsc in /proc/mounts

[Suresh Jayaraman]

Signed-off-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
---
 fs/cifs/cifsfs.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 9c37897..76c8a90 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -458,6 +458,8 @@ cifs_show_options(struct seq_file *s, struct vfsmount *m)
 		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");
 
 	seq_printf(s, ",rsize=%d", cifs_sb->rsize);
 	seq_printf(s, ",wsize=%d", cifs_sb->wsize);

Re: [PATCH 1/3] cifs: allow fsc mount option only if CONFIG_CIFS_FSCACHE is set

[Jeff Layton]

On Wed, 24 Nov 2010 17:49:05 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> Currently, it is possible to specify 'fsc' mount option even if
> CONFIG_CIFS_FSCACHE has not been set. The option is being ignored silently
> while the user fscache functionality to work. Fix this by raising error when
> the CONFIG option is not set.
> 
> 
> Reported-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
> ---
>  fs/cifs/connect.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 251a17c..32fa4d9 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -1352,6 +1352,11 @@ cifs_parse_mount_options(char *options, const char *devname,
>  				"supported. Instead set "
>  				"/proc/fs/cifs/LookupCacheEnabled to 0\n");
>  		} else if (strnicmp(data, "fsc", 3) == 0) {
> +#ifndef CONFIG_CIFS_FSCACHE
> +			cERROR(1, "FS-Cache support needs CONFIG_CIFS_FSCACHE"
> +				  "kernel config option set");
> +			return 1;
> +#endif
>  			vol->fsc = true;
>  		} else if (strnicmp(data, "mfsymlinks", 10) == 0) {
>  			vol->mfsymlinks = true;
> --
> 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

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

Re: [PATCH 2/3] cifs: enable fscache iff fsc mount option is used explicitly

[Jeff Layton]

On Wed, 24 Nov 2010 17:49:06 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> Currently, if CONFIG_CIFS_FSCACHE is set, fscache is enabled on files opened
> as read-only irrespective of the 'fsc' mount option. Fix this by enabling
> fscache only if 'fsc' mount option is specified explicitly.
> 
> Remove an extraneous cFYI debug message and fix a typo while at it.
> 
> Reported-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
> ---
>  fs/cifs/fscache.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/cifs/fscache.c b/fs/cifs/fscache.c
> index a2ad94e..297a43d 100644
> --- a/fs/cifs/fscache.c
> +++ b/fs/cifs/fscache.c
> @@ -2,7 +2,7 @@
>   *   fs/cifs/fscache.c - CIFS filesystem cache interface
>   *
>   *   Copyright (c) 2010 Novell, Inc.
> - *   Author(s): Suresh Jayaraman (sjayaraman-l3A5Bk7waGM@public.gmane.org>
> + *   Author(s): Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
>   *
>   *   This library is free software; you can redistribute it and/or modify
>   *   it under the terms of the GNU Lesser General Public License as published
> @@ -67,10 +67,12 @@ static void cifs_fscache_enable_inode_cookie(struct inode *inode)
>  	if (cifsi->fscache)
>  		return;
>  
> -	cifsi->fscache = fscache_acquire_cookie(tcon->fscache,
> +	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_FSCACHE) {
> +		cifsi->fscache = fscache_acquire_cookie(tcon->fscache,
>  				&cifs_fscache_inode_object_def, cifsi);
> -	cFYI(1, "CIFS: got FH cookie (0x%p/0x%p)", tcon->fscache,
> +		cFYI(1, "CIFS: got FH cookie (0x%p/0x%p)", tcon->fscache,
>  				cifsi->fscache);
> +	}
>  }
>  
>  void cifs_fscache_release_inode_cookie(struct inode *inode)
> @@ -101,10 +103,8 @@ void cifs_fscache_set_inode_cookie(struct inode *inode, struct file *filp)
>  {
>  	if ((filp->f_flags & O_ACCMODE) != O_RDONLY)
>  		cifs_fscache_disable_inode_cookie(inode);
> -	else {
> +	else
>  		cifs_fscache_enable_inode_cookie(inode);
> -		cFYI(1, "CIFS: fscache inode cookie set");
> -	}
>  }
>  
>  void cifs_fscache_reset_inode_cookie(struct inode *inode)
> --
> 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


Ok, so we're disabling fscache by just not setting the cookie on the
inode, but the server->fscache and tcon->fscache pointers get set
unconditionally. I suppose that makes sense since we could have someone
mount a tcon without fsc set and then later mount the same one with it
set.

Looks reasonable, I guess...

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

Re: [PATCH 3/3] cifs: display fsc in /proc/mounts

[Jeff Layton]

On Wed, 24 Nov 2010 17:49:07 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> 
> Signed-off-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
> ---
>  fs/cifs/cifsfs.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 9c37897..76c8a90 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -458,6 +458,8 @@ cifs_show_options(struct seq_file *s, struct vfsmount *m)
>  		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");
>  
>  	seq_printf(s, ",rsize=%d", cifs_sb->rsize);
>  	seq_printf(s, ",wsize=%d", cifs_sb->wsize);
> --
> 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

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

Re: [PATCH 1/3] cifs: allow fsc mount option only if CONFIG_CIFS_FSCACHE is set

[Suresh Jayaraman]

On 11/27/2010 09:21 AM, Steve French wrote:
> 
> 
> On Fri, Nov 26, 2010 at 9:50 PM, Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
> <mailto:smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>> wrote:
> 
>     As an alternative - if fsc is stable and no defects and doesn't
>     increase the code size appreciably ... what are the drawbacks of
>     turning it on unconditionally?
> 
> 
> ie always configure it, we still only enable it on a mount when fsc is
> specified on mount
> �

I'm not aware of any open issues w.r.t cifs FS-Cache (except the
problems this patchset addresses) but I'm not sure how much testing it
has undergone really (except for me running all my tests with fsc on for
sometime and a few openSUSE users). The code size is not really much of
a problem.

The strict cache patches accounting for fscache might also provide more
testing ground. So, I think keeping it as non-default atleast for one
more release would be good before we make it as a config default.

Thanks,

>     On Wed, Nov 24, 2010 at 6:19 AM, Suresh Jayaraman
>     <sjayaraman-l3A5Bk7waGM@public.gmane.org <mailto:sjayaraman-l3A5Bk7waGM@public.gmane.org>> wrote:
> 
>         Currently, it is possible to specify 'fsc' mount option even if
>         CONFIG_CIFS_FSCACHE has not been set. The option is being
>         ignored silently
>         while the user fscache functionality to work. Fix this by
>         raising error when
>         the CONFIG option is not set.
> 
> 
>         Reported-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
>         <mailto:jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>>
>         Signed-off-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org
>         <mailto:sjayaraman-l3A5Bk7waGM@public.gmane.org>>
>         ---
>         �fs/cifs/connect.c | � �5 +++++
>         �1 files changed, 5 insertions(+), 0 deletions(-)
> 
>         diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>         index 251a17c..32fa4d9 100644
>         --- a/fs/cifs/connect.c
>         +++ b/fs/cifs/connect.c
>         @@ -1352,6 +1352,11 @@ cifs_parse_mount_options(char *options,
>         const char *devname,
>         � � � � � � � � � � � � � � � �"supported. Instead set "
>         � � � � � � � � � � � � � � � �"/proc/fs/cifs/LookupCacheEnabled
>         to 0\n");
>         � � � � � � � �} else if (strnicmp(data, "fsc", 3) == 0) {
>         +#ifndef CONFIG_CIFS_FSCACHE
>         + � � � � � � � � � � � cERROR(1, "FS-Cache support needs
>         CONFIG_CIFS_FSCACHE"
>         + � � � � � � � � � � � � � � � � "kernel config option set");
>         + � � � � � � � � � � � return 1;
>         +#endif
>         � � � � � � � � � � � �vol->fsc = true;
>         � � � � � � � �} else if (strnicmp(data, "mfsymlinks", 10) == 0) {
>         � � � � � � � � � � � �vol->mfsymlinks = true;
> 
> 
> 
> 
>     -- 
>     Thanks,
> 
>     Steve
> 
> 
> 
> 
> -- 
> Thanks,
> 
> Steve


-- 
Suresh Jayaraman