[PATCH 0/6] Add strict cache mode (try #4)

[PATCH 0/6] Add strict cache mode (try #4)

[Pavel Shilovsky]

Re-posting the whole set of strict cache patches.

Pavel Shilovsky (6):
  CIFS: Make cifsFileInfo_put work with strict cache mode (try #4)
  CIFS: Make read call work with strict cache mode (try #2)
  CIFS: Make write call work with strict cache mode (try #2)
  CIFS: Make cifs_fsync work with strict cache mode (try #2)
  CIFS: Make cifs_file_map work with strict cache mode (try #2)
  CIFS: Add strictcache mount option (try #2)

 fs/cifs/README       |    5 +++
 fs/cifs/cifs_fs_sb.h |    1 +
 fs/cifs/cifsfs.c     |   71 +++++++++++++++++++++++++++++++++++++++++++++----
 fs/cifs/connect.c    |    5 +++
 fs/cifs/file.c       |   42 +++++++++++++++++++++--------
 5 files changed, 106 insertions(+), 18 deletions(-)

-- 
1.7.3.2

[PATCH 1/6] CIFS: Make cifsFileInfo_put work with strict cache mode (try #4)

[Pavel Shilovsky]

On strict cache mode when we close the last file handle of the inode we
should set invalid_mapping flag on this inode to prevent data coherency
problem when we open it again but it has been modified by other clients.

Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/cifs/cifs_fs_sb.h |    1 +
 fs/cifs/file.c       |    8 ++++++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
index e9a393c..be7b159 100644
--- a/fs/cifs/cifs_fs_sb.h
+++ b/fs/cifs/cifs_fs_sb.h
@@ -40,6 +40,7 @@
 #define CIFS_MOUNT_FSCACHE	0x8000 /* local caching enabled */
 #define CIFS_MOUNT_MF_SYMLINKS	0x10000 /* Minshall+French Symlinks enabled */
 #define CIFS_MOUNT_MULTIUSER	0x20000 /* multiuser mount */
+#define CIFS_MOUNT_STRICT_IO	0x40000 /* strict cache mode */
 
 struct cifs_sb_info {
 	struct rb_root tlink_tree;
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index b857ce5..f853bf0 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -264,6 +264,7 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 	struct inode *inode = cifs_file->dentry->d_inode;
 	struct cifsTconInfo *tcon = tlink_tcon(cifs_file->tlink);
 	struct cifsInodeInfo *cifsi = CIFS_I(inode);
+	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
 	struct cifsLockInfo *li, *tmp;
 
 	spin_lock(&cifs_file_list_lock);
@@ -279,6 +280,13 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 	if (list_empty(&cifsi->openFileList)) {
 		cFYI(1, "closing last open instance for inode %p",
 			cifs_file->dentry->d_inode);
+
+		/* in strict cache mode we need invalidate mapping on the last
+		   close  because it may cause a error when we open this file
+		   again and get at least level II oplock */
+		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO)
+			CIFS_I(inode)->invalid_mapping = true;
+
 		cifs_set_oplock_level(cifsi, 0);
 	}
 	spin_unlock(&cifs_file_list_lock);
-- 
1.7.3.2

Re: [PATCH 1/6] CIFS: Make cifsFileInfo_put work with strict cache mode (try #4)

[Jeff Layton]

On Sun, 28 Nov 2010 11:12:47 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On strict cache mode when we close the last file handle of the inode we
> should set invalid_mapping flag on this inode to prevent data coherency
> problem when we open it again but it has been modified by other clients.
> 
> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/cifs/cifs_fs_sb.h |    1 +
>  fs/cifs/file.c       |    8 ++++++++
>  2 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
> index e9a393c..be7b159 100644
> --- a/fs/cifs/cifs_fs_sb.h
> +++ b/fs/cifs/cifs_fs_sb.h
> @@ -40,6 +40,7 @@
>  #define CIFS_MOUNT_FSCACHE	0x8000 /* local caching enabled */
>  #define CIFS_MOUNT_MF_SYMLINKS	0x10000 /* Minshall+French Symlinks enabled */
>  #define CIFS_MOUNT_MULTIUSER	0x20000 /* multiuser mount */
> +#define CIFS_MOUNT_STRICT_IO	0x40000 /* strict cache mode */
>  
>  struct cifs_sb_info {
>  	struct rb_root tlink_tree;
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index b857ce5..f853bf0 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -264,6 +264,7 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>  	struct inode *inode = cifs_file->dentry->d_inode;
>  	struct cifsTconInfo *tcon = tlink_tcon(cifs_file->tlink);
>  	struct cifsInodeInfo *cifsi = CIFS_I(inode);
> +	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
>  	struct cifsLockInfo *li, *tmp;
>  
>  	spin_lock(&cifs_file_list_lock);
> @@ -279,6 +280,13 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>  	if (list_empty(&cifsi->openFileList)) {
>  		cFYI(1, "closing last open instance for inode %p",
>  			cifs_file->dentry->d_inode);
> +
> +		/* in strict cache mode we need invalidate mapping on the last
> +		   close  because it may cause a error when we open this file
> +		   again and get at least level II oplock */
> +		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO)
> +			CIFS_I(inode)->invalid_mapping = true;
> +
>  		cifs_set_oplock_level(cifsi, 0);
>  	}
>  	spin_unlock(&cifs_file_list_lock);

Looks reasonable.

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

[PATCH 2/6] CIFS: Make read call work with strict cache mode (try #2)

[Pavel Shilovsky]

Add cifs_file_aio_read where we read from the cache if we have at least
Level II oplock - otherwise read from the server.

Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/cifs/cifsfs.c |   31 +++++++++++++++++++++++++++++--
 1 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 9c37897..bbb5294 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -568,6 +568,33 @@ cifs_do_mount(struct file_system_type *fs_type,
 	return dget(sb->s_root);
 }
 
+static ssize_t cifs_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
+				  unsigned long nr_segs, loff_t pos)
+{
+	struct inode *inode;
+	struct cifs_sb_info *cifs_sb;
+	ssize_t read;
+
+	inode = iocb->ki_filp->f_path.dentry->d_inode;
+	cifs_sb = CIFS_SB(iocb->ki_filp->f_path.dentry->d_sb);
+
+	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) == 0 ||
+	    CIFS_I(inode)->clientCanCacheRead)
+		return generic_file_aio_read(iocb, iov, nr_segs, pos);
+
+	/* in strict cache mode we need to read from the server all the time
+	   if we don't have level II oplock because the server can delay mtime
+	   change - so we can't make a decision about inode invalidating.
+	   And we can also fail with pagereading if there are mandatory locks
+	   on pages affected by this read but not on the region from pos to
+	   pos+len-1 */
+	read = cifs_user_read(iocb->ki_filp, iov->iov_base,
+			      iov->iov_len, &pos);
+	iocb->ki_pos = pos;
+
+	return read;
+}
+
 static ssize_t cifs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
 				   unsigned long nr_segs, loff_t pos)
 {
@@ -690,7 +717,7 @@ const struct inode_operations cifs_symlink_inode_ops = {
 const struct file_operations cifs_file_ops = {
 	.read = do_sync_read,
 	.write = do_sync_write,
-	.aio_read = generic_file_aio_read,
+	.aio_read = cifs_file_aio_read,
 	.aio_write = cifs_file_aio_write,
 	.open = cifs_open,
 	.release = cifs_close,
@@ -727,7 +754,7 @@ const struct file_operations cifs_file_direct_ops = {
 const struct file_operations cifs_file_nobrl_ops = {
 	.read = do_sync_read,
 	.write = do_sync_write,
-	.aio_read = generic_file_aio_read,
+	.aio_read = cifs_file_aio_read,
 	.aio_write = cifs_file_aio_write,
 	.open = cifs_open,
 	.release = cifs_close,
-- 
1.7.3.2

Re: [PATCH 2/6] CIFS: Make read call work with strict cache mode (try #2)

[Jeff Layton]

On Sun, 28 Nov 2010 11:12:48 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Add cifs_file_aio_read where we read from the cache if we have at least
> Level II oplock - otherwise read from the server.
> 
> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/cifs/cifsfs.c |   31 +++++++++++++++++++++++++++++--
>  1 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 9c37897..bbb5294 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -568,6 +568,33 @@ cifs_do_mount(struct file_system_type *fs_type,
>  	return dget(sb->s_root);
>  }
>  
> +static ssize_t cifs_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
> +				  unsigned long nr_segs, loff_t pos)
> +{
> +	struct inode *inode;
> +	struct cifs_sb_info *cifs_sb;
> +	ssize_t read;
> +
> +	inode = iocb->ki_filp->f_path.dentry->d_inode;
> +	cifs_sb = CIFS_SB(iocb->ki_filp->f_path.dentry->d_sb);
> +
> +	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) == 0 ||
> +	    CIFS_I(inode)->clientCanCacheRead)
> +		return generic_file_aio_read(iocb, iov, nr_segs, pos);
> +
> +	/* in strict cache mode we need to read from the server all the time
> +	   if we don't have level II oplock because the server can delay mtime
> +	   change - so we can't make a decision about inode invalidating.
> +	   And we can also fail with pagereading if there are mandatory locks
> +	   on pages affected by this read but not on the region from pos to
> +	   pos+len-1 */
	^^^^^^^^^^^
	Nit: while much of the existing cifs code doesn't, it's best to
	follow kernel coding style when you're adding code. The above
	comment doesn't do this however. You may want to have a look at
	Documentation/CodingStyle in the kernel sources.

> +	read = cifs_user_read(iocb->ki_filp, iov->iov_base,
> +			      iov->iov_len, &pos);
> +	iocb->ki_pos = pos;
> +
> +	return read;
> +}
> +
>  static ssize_t cifs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
>  				   unsigned long nr_segs, loff_t pos)
>  {
> @@ -690,7 +717,7 @@ const struct inode_operations cifs_symlink_inode_ops = {
>  const struct file_operations cifs_file_ops = {
>  	.read = do_sync_read,
>  	.write = do_sync_write,
> -	.aio_read = generic_file_aio_read,
> +	.aio_read = cifs_file_aio_read,
>  	.aio_write = cifs_file_aio_write,
>  	.open = cifs_open,
>  	.release = cifs_close,
> @@ -727,7 +754,7 @@ const struct file_operations cifs_file_direct_ops = {
>  const struct file_operations cifs_file_nobrl_ops = {
>  	.read = do_sync_read,
>  	.write = do_sync_write,
> -	.aio_read = generic_file_aio_read,
> +	.aio_read = cifs_file_aio_read,
>  	.aio_write = cifs_file_aio_write,
>  	.open = cifs_open,
>  	.release = cifs_close,

Looks reasonable:

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

Re: [PATCH 2/6] CIFS: Make read call work with strict cache mode (try #2)

[Christoph Hellwig]

Your patch arbitrarily shortens the read to the first vector.  While
short reads for regular files are fine with Posix, it will break a lot
of userspace applications.

Re: [PATCH 2/6] CIFS: Make read call work with strict cache mode (try #2)

[Pavel Shilovsky]

2010/11/29 Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>:
>
> Your patch arbitrarily shortens the read to the first vector.  While
> short reads for regular files are fine with Posix, it will break a lot
> of userspace applications.
>

cifs_user_read returns the number of bytes we read from the server and
it, of course, can be different with requested number. Could you
explain carefully what do you mean, please? Where is a problem here?


-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 2/6] CIFS: Make read call work with strict cache mode (try #2)

[Pavel Shilovsky]

2010/11/29 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> 2010/11/29 Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>:
>>
>> Your patch arbitrarily shortens the read to the first vector.  While
>> short reads for regular files are fine with Posix, it will break a lot
>> of userspace applications.
>>
>
> cifs_user_read returns the number of bytes we read from the server and
> it, of course, can be different with requested number. Could you
> explain carefully what do you mean, please? Where is a problem here?
>

It seems to me that I understand what you meant. Of course, it is a
bug to process only one segment while nr_segs can be more than one
here. Thanks!

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 2/6] CIFS: Make read call work with strict cache mode (try #2)

[Jeff Layton]

On Mon, 29 Nov 2010 13:41:13 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2010/11/29 Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>:
> >
> > Your patch arbitrarily shortens the read to the first vector.  While
> > short reads for regular files are fine with Posix, it will break a lot
> > of userspace applications.
> >
> 
> cifs_user_read returns the number of bytes we read from the server and
> it, of course, can be different with requested number. Could you
> explain carefully what do you mean, please? Where is a problem here?
> 
> 

Gah...good catch, Christoph. I'll need to rescind my ack for this patch
too.

Pavel, note that the aio_read prototype has a "nr_segs" argument. Your
patch ignores that argument in the strictcache case. If someone passes
in an array of iovecs, then you're ignoring all but the first.

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

[PATCH 3/6] CIFS: Make write call work with strict cache mode (try #2)

[Pavel Shilovsky]

On strict cache mode if we don't have Exclusive oplock we write a data to
the server through cifs_user_write. Then if we Level II oplock store it in
the cache, otherwise - invalidate inode pages affected by this writing.

Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/cifs/cifsfs.c |   40 ++++++++++++++++++++++++++++++++++++----
 1 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index bbb5294..901c82b 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -598,12 +598,44 @@ static ssize_t cifs_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
 static ssize_t cifs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
 				   unsigned long nr_segs, loff_t pos)
 {
-	struct inode *inode = iocb->ki_filp->f_path.dentry->d_inode;
+	struct inode *inode;
+	struct cifs_sb_info *cifs_sb;
 	ssize_t written;
 
-	written = generic_file_aio_write(iocb, iov, nr_segs, pos);
-	if (!CIFS_I(inode)->clientCanCacheAll)
-		filemap_fdatawrite(inode->i_mapping);
+	inode = iocb->ki_filp->f_path.dentry->d_inode;
+
+	if (CIFS_I(inode)->clientCanCacheAll)
+		return generic_file_aio_write(iocb, iov, nr_segs, pos);
+
+	cifs_sb = CIFS_SB(iocb->ki_filp->f_path.dentry->d_sb);
+
+	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) == 0) {
+		int rc;
+
+		written = generic_file_aio_write(iocb, iov, nr_segs, pos);
+
+		rc = filemap_fdatawrite(inode->i_mapping);
+		if (rc)
+			cFYI(1, "cifs_file_aio_write: %d rc on %p inode",
+			     rc, inode);
+		return written;
+	}
+
+	/* in strict cache mode we need to write the data to the server exactly
+	   from the pos to pos+len-1 rather than flush all affected pages
+	   because it may cause a error with mandatory locks on these pages but
+	   not on the region from pos to ppos+len-1 */
+	written = cifs_user_write(iocb->ki_filp, iov->iov_base,
+				  iov->iov_len, &pos);
+
+	iocb->ki_pos = pos;
+
+	/* if we were successful - invalidate inode pages the write affected */
+	if (written > 0)
+		invalidate_mapping_pages(inode->i_mapping,
+					      (pos-written) >> PAGE_CACHE_SHIFT,
+					      (pos-1) >> PAGE_CACHE_SHIFT);
+
 	return written;
 }
 
-- 
1.7.3.2

Re: [PATCH 3/6] CIFS: Make write call work with strict cache mode (try #2)

[Jeff Layton]

On Sun, 28 Nov 2010 11:12:49 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On strict cache mode if we don't have Exclusive oplock we write a data to
> the server through cifs_user_write. Then if we Level II oplock store it in
> the cache, otherwise - invalidate inode pages affected by this writing.
> 
> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/cifs/cifsfs.c |   40 ++++++++++++++++++++++++++++++++++++----
>  1 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index bbb5294..901c82b 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -598,12 +598,44 @@ static ssize_t cifs_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
>  static ssize_t cifs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
>  				   unsigned long nr_segs, loff_t pos)
>  {
> -	struct inode *inode = iocb->ki_filp->f_path.dentry->d_inode;
> +	struct inode *inode;
> +	struct cifs_sb_info *cifs_sb;
>  	ssize_t written;
>  
> -	written = generic_file_aio_write(iocb, iov, nr_segs, pos);
> -	if (!CIFS_I(inode)->clientCanCacheAll)
> -		filemap_fdatawrite(inode->i_mapping);
> +	inode = iocb->ki_filp->f_path.dentry->d_inode;
> +
> +	if (CIFS_I(inode)->clientCanCacheAll)
> +		return generic_file_aio_write(iocb, iov, nr_segs, pos);
> +
> +	cifs_sb = CIFS_SB(iocb->ki_filp->f_path.dentry->d_sb);
> +
> +	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) == 0) {
> +		int rc;
> +
> +		written = generic_file_aio_write(iocb, iov, nr_segs, pos);
> +
> +		rc = filemap_fdatawrite(inode->i_mapping);
> +		if (rc)
> +			cFYI(1, "cifs_file_aio_write: %d rc on %p inode",
> +			     rc, inode);
> +		return written;
> +	}
> +
> +	/* in strict cache mode we need to write the data to the server exactly
> +	   from the pos to pos+len-1 rather than flush all affected pages
> +	   because it may cause a error with mandatory locks on these pages but
> +	   not on the region from pos to ppos+len-1 */

	Again, please fix the comment style. Here: ^^^^

> +	written = cifs_user_write(iocb->ki_filp, iov->iov_base,
> +				  iov->iov_len, &pos);
> +
> +	iocb->ki_pos = pos;
> +
> +	/* if we were successful - invalidate inode pages the write affected */
> +	if (written > 0)
> +		invalidate_mapping_pages(inode->i_mapping,
> +					      (pos-written) >> PAGE_CACHE_SHIFT,
> +					      (pos-1) >> PAGE_CACHE_SHIFT);
> +
>  	return written;
>  }
>  

May god have mercy on anyone who tries to mix strictcache and mmap.

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

Re: [PATCH 3/6] CIFS: Make write call work with strict cache mode (try #2)

[Jeff Layton]

On Sun, 28 Nov 2010 06:36:04 -0500
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> On Sun, 28 Nov 2010 11:12:49 +0300
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > On strict cache mode if we don't have Exclusive oplock we write a data to
> > the server through cifs_user_write. Then if we Level II oplock store it in
> > the cache, otherwise - invalidate inode pages affected by this writing.
> > 
> > Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> >  fs/cifs/cifsfs.c |   40 ++++++++++++++++++++++++++++++++++++----
> >  1 files changed, 36 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index bbb5294..901c82b 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -598,12 +598,44 @@ static ssize_t cifs_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
> >  static ssize_t cifs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
> >  				   unsigned long nr_segs, loff_t pos)
> >  {
> > -	struct inode *inode = iocb->ki_filp->f_path.dentry->d_inode;
> > +	struct inode *inode;
> > +	struct cifs_sb_info *cifs_sb;
> >  	ssize_t written;
> >  
> > -	written = generic_file_aio_write(iocb, iov, nr_segs, pos);
> > -	if (!CIFS_I(inode)->clientCanCacheAll)
> > -		filemap_fdatawrite(inode->i_mapping);
> > +	inode = iocb->ki_filp->f_path.dentry->d_inode;
> > +
> > +	if (CIFS_I(inode)->clientCanCacheAll)
> > +		return generic_file_aio_write(iocb, iov, nr_segs, pos);
> > +
> > +	cifs_sb = CIFS_SB(iocb->ki_filp->f_path.dentry->d_sb);
> > +
> > +	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) == 0) {
> > +		int rc;
> > +
> > +		written = generic_file_aio_write(iocb, iov, nr_segs, pos);
> > +
> > +		rc = filemap_fdatawrite(inode->i_mapping);
> > +		if (rc)
> > +			cFYI(1, "cifs_file_aio_write: %d rc on %p inode",
> > +			     rc, inode);
> > +		return written;
> > +	}
> > +
> > +	/* in strict cache mode we need to write the data to the server exactly
> > +	   from the pos to pos+len-1 rather than flush all affected pages
> > +	   because it may cause a error with mandatory locks on these pages but
> > +	   not on the region from pos to ppos+len-1 */
> 
> 	Again, please fix the comment style. Here: ^^^^
> 
> > +	written = cifs_user_write(iocb->ki_filp, iov->iov_base,
> > +				  iov->iov_len, &pos);
> > +
> > +	iocb->ki_pos = pos;
> > +
> > +	/* if we were successful - invalidate inode pages the write affected */
> > +	if (written > 0)
> > +		invalidate_mapping_pages(inode->i_mapping,
> > +					      (pos-written) >> PAGE_CACHE_SHIFT,
> > +					      (pos-1) >> PAGE_CACHE_SHIFT);
> > +
> >  	return written;
> >  }
> >  
> 
> May god have mercy on anyone who tries to mix strictcache and mmap.
> 
> Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

(cc'ing Suresh so he can comment)

Actually...I'm going to withdraw my Reviewed-by tag here for now. This
bare invalidate_mapping_pages doesn't deal with fscache.

I think I need to understand what's intended when someone specifies
strictcache and fsc before I can ack this. The simple answer would be
that they are mutually exclusive, but if that's the case then the patch
that adds the mount option needs to deal with that appropriately.

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

Re: [PATCH 3/6] CIFS: Make write call work with strict cache mode (try #2)

[Pavel Shilovsky]

2010/11/28 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Sun, 28 Nov 2010 06:36:04 -0500
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
>> On Sun, 28 Nov 2010 11:12:49 +0300
>> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>> > On strict cache mode if we don't have Exclusive oplock we write a data to
>> > the server through cifs_user_write. Then if we Level II oplock store it in
>> > the cache, otherwise - invalidate inode pages affected by this writing.
>> >
>> > Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> > ---
>> >  fs/cifs/cifsfs.c |   40 ++++++++++++++++++++++++++++++++++++----
>> >  1 files changed, 36 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>> > index bbb5294..901c82b 100644
>> > --- a/fs/cifs/cifsfs.c
>> > +++ b/fs/cifs/cifsfs.c
>> > @@ -598,12 +598,44 @@ static ssize_t cifs_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
>> >  static ssize_t cifs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
>> >                                unsigned long nr_segs, loff_t pos)
>> >  {
>> > -   struct inode *inode = iocb->ki_filp->f_path.dentry->d_inode;
>> > +   struct inode *inode;
>> > +   struct cifs_sb_info *cifs_sb;
>> >     ssize_t written;
>> >
>> > -   written = generic_file_aio_write(iocb, iov, nr_segs, pos);
>> > -   if (!CIFS_I(inode)->clientCanCacheAll)
>> > -           filemap_fdatawrite(inode->i_mapping);
>> > +   inode = iocb->ki_filp->f_path.dentry->d_inode;
>> > +
>> > +   if (CIFS_I(inode)->clientCanCacheAll)
>> > +           return generic_file_aio_write(iocb, iov, nr_segs, pos);
>> > +
>> > +   cifs_sb = CIFS_SB(iocb->ki_filp->f_path.dentry->d_sb);
>> > +
>> > +   if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) == 0) {
>> > +           int rc;
>> > +
>> > +           written = generic_file_aio_write(iocb, iov, nr_segs, pos);
>> > +
>> > +           rc = filemap_fdatawrite(inode->i_mapping);
>> > +           if (rc)
>> > +                   cFYI(1, "cifs_file_aio_write: %d rc on %p inode",
>> > +                        rc, inode);
>> > +           return written;
>> > +   }
>> > +
>> > +   /* in strict cache mode we need to write the data to the server exactly
>> > +      from the pos to pos+len-1 rather than flush all affected pages
>> > +      because it may cause a error with mandatory locks on these pages but
>> > +      not on the region from pos to ppos+len-1 */
>>
>>       Again, please fix the comment style. Here: ^^^^
>>
>> > +   written = cifs_user_write(iocb->ki_filp, iov->iov_base,
>> > +                             iov->iov_len, &pos);
>> > +
>> > +   iocb->ki_pos = pos;
>> > +
>> > +   /* if we were successful - invalidate inode pages the write affected */
>> > +   if (written > 0)
>> > +           invalidate_mapping_pages(inode->i_mapping,
>> > +                                         (pos-written) >> PAGE_CACHE_SHIFT,
>> > +                                         (pos-1) >> PAGE_CACHE_SHIFT);
>> > +
>> >     return written;
>> >  }
>> >
>>
>> May god have mercy on anyone who tries to mix strictcache and mmap.
>>
>> Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> (cc'ing Suresh so he can comment)
>
> Actually...I'm going to withdraw my Reviewed-by tag here for now. This
> bare invalidate_mapping_pages doesn't deal with fscache.
>
> I think I need to understand what's intended when someone specifies
> strictcache and fsc before I can ack this. The simple answer would be
> that they are mutually exclusive, but if that's the case then the patch
> that adds the mount option needs to deal with that appropriately.


I don't think they can live together. I think we should do smth like a
following in mount options parsing:

...
if (opt == fscache) {
   vol->fscahe = 1;
   vol->strictcache = 0;
}
...
if (opt == strictcache) {
  vol->strictcache = 1;
  vol->fscache = 0;
}

So, if user specify both only the last will affect the client
behavior. Also we should add this information into cifs manpage.
Thoughts?

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 3/6] CIFS: Make write call work with strict cache mode (try #2)

[Jeff Layton]

On Mon, 29 Nov 2010 13:58:05 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2010/11/28 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > On Sun, 28 Nov 2010 06:36:04 -0500
> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >
> >> On Sun, 28 Nov 2010 11:12:49 +0300
> >> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>
> >> > On strict cache mode if we don't have Exclusive oplock we write a data to
> >> > the server through cifs_user_write. Then if we Level II oplock store it in
> >> > the cache, otherwise - invalidate inode pages affected by this writing.
> >> >
> >> > Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> > ---
> >> >  fs/cifs/cifsfs.c |   40 ++++++++++++++++++++++++++++++++++++----
> >> >  1 files changed, 36 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> >> > index bbb5294..901c82b 100644
> >> > --- a/fs/cifs/cifsfs.c
> >> > +++ b/fs/cifs/cifsfs.c
> >> > @@ -598,12 +598,44 @@ static ssize_t cifs_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
> >> >  static ssize_t cifs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
> >> >                                unsigned long nr_segs, loff_t pos)
> >> >  {
> >> > -   struct inode *inode = iocb->ki_filp->f_path.dentry->d_inode;
> >> > +   struct inode *inode;
> >> > +   struct cifs_sb_info *cifs_sb;
> >> >     ssize_t written;
> >> >
> >> > -   written = generic_file_aio_write(iocb, iov, nr_segs, pos);
> >> > -   if (!CIFS_I(inode)->clientCanCacheAll)
> >> > -           filemap_fdatawrite(inode->i_mapping);
> >> > +   inode = iocb->ki_filp->f_path.dentry->d_inode;
> >> > +
> >> > +   if (CIFS_I(inode)->clientCanCacheAll)
> >> > +           return generic_file_aio_write(iocb, iov, nr_segs, pos);
> >> > +
> >> > +   cifs_sb = CIFS_SB(iocb->ki_filp->f_path.dentry->d_sb);
> >> > +
> >> > +   if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) == 0) {
> >> > +           int rc;
> >> > +
> >> > +           written = generic_file_aio_write(iocb, iov, nr_segs, pos);
> >> > +
> >> > +           rc = filemap_fdatawrite(inode->i_mapping);
> >> > +           if (rc)
> >> > +                   cFYI(1, "cifs_file_aio_write: %d rc on %p inode",
> >> > +                        rc, inode);
> >> > +           return written;
> >> > +   }
> >> > +
> >> > +   /* in strict cache mode we need to write the data to the server exactly
> >> > +      from the pos to pos+len-1 rather than flush all affected pages
> >> > +      because it may cause a error with mandatory locks on these pages but
> >> > +      not on the region from pos to ppos+len-1 */
> >>
> >>       Again, please fix the comment style. Here: ^^^^
> >>
> >> > +   written = cifs_user_write(iocb->ki_filp, iov->iov_base,
> >> > +                             iov->iov_len, &pos);
> >> > +
> >> > +   iocb->ki_pos = pos;
> >> > +
> >> > +   /* if we were successful - invalidate inode pages the write affected */
> >> > +   if (written > 0)
> >> > +           invalidate_mapping_pages(inode->i_mapping,
> >> > +                                         (pos-written) >> PAGE_CACHE_SHIFT,
> >> > +                                         (pos-1) >> PAGE_CACHE_SHIFT);
> >> > +
> >> >     return written;
> >> >  }
> >> >
> >>
> >> May god have mercy on anyone who tries to mix strictcache and mmap.
> >>
> >> Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >
> > (cc'ing Suresh so he can comment)
> >
> > Actually...I'm going to withdraw my Reviewed-by tag here for now. This
> > bare invalidate_mapping_pages doesn't deal with fscache.
> >
> > I think I need to understand what's intended when someone specifies
> > strictcache and fsc before I can ack this. The simple answer would be
> > that they are mutually exclusive, but if that's the case then the patch
> > that adds the mount option needs to deal with that appropriately.
> 
> 
> I don't think they can live together. I think we should do smth like a
> following in mount options parsing:
> 
> ...
> if (opt == fscache) {
>    vol->fscahe = 1;
>    vol->strictcache = 0;
> }
> ...
> if (opt == strictcache) {
>   vol->strictcache = 1;
>   vol->fscache = 0;
> }
> 
> So, if user specify both only the last will affect the client
> behavior. Also we should add this information into cifs manpage.
> Thoughts?
> 

That would one way to deal with it.

On the other hand though...fscache allows you to keep more data cached
than you have RAM. This could be useful in a strictcache situation as
well. Consider the case of an application on a client that has a lot of
large files open for read. The server may grant oplocks on all of them.
fscache would allow for fewer round trips to the server in such a case.

So, another way to deal with it would be to simply invalidate the
fscache whenever you'd invalidate the in-ram cache. I'm not sure what
to do for the cifs_file_aio_write case where you're invalidating just a
small range however.

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

Re: [PATCH 3/6] CIFS: Make write call work with strict cache mode (try #2)

[Pavel Shilovsky]

2010/11/29 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Mon, 29 Nov 2010 13:58:05 +0300
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> 2010/11/28 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> > On Sun, 28 Nov 2010 06:36:04 -0500
>> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> >
>> >> On Sun, 28 Nov 2010 11:12:49 +0300
>> >> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >>
>> >> > On strict cache mode if we don't have Exclusive oplock we write a data to
>> >> > the server through cifs_user_write. Then if we Level II oplock store it in
>> >> > the cache, otherwise - invalidate inode pages affected by this writing.
>> >> >
>> >> > Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >> > ---
>> >> >  fs/cifs/cifsfs.c |   40 ++++++++++++++++++++++++++++++++++++----
>> >> >  1 files changed, 36 insertions(+), 4 deletions(-)
>> >> >
>> >> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>> >> > index bbb5294..901c82b 100644
>> >> > --- a/fs/cifs/cifsfs.c
>> >> > +++ b/fs/cifs/cifsfs.c
>> >> > @@ -598,12 +598,44 @@ static ssize_t cifs_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
>> >> >  static ssize_t cifs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
>> >> >                                unsigned long nr_segs, loff_t pos)
>> >> >  {
>> >> > -   struct inode *inode = iocb->ki_filp->f_path.dentry->d_inode;
>> >> > +   struct inode *inode;
>> >> > +   struct cifs_sb_info *cifs_sb;
>> >> >     ssize_t written;
>> >> >
>> >> > -   written = generic_file_aio_write(iocb, iov, nr_segs, pos);
>> >> > -   if (!CIFS_I(inode)->clientCanCacheAll)
>> >> > -           filemap_fdatawrite(inode->i_mapping);
>> >> > +   inode = iocb->ki_filp->f_path.dentry->d_inode;
>> >> > +
>> >> > +   if (CIFS_I(inode)->clientCanCacheAll)
>> >> > +           return generic_file_aio_write(iocb, iov, nr_segs, pos);
>> >> > +
>> >> > +   cifs_sb = CIFS_SB(iocb->ki_filp->f_path.dentry->d_sb);
>> >> > +
>> >> > +   if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) == 0) {
>> >> > +           int rc;
>> >> > +
>> >> > +           written = generic_file_aio_write(iocb, iov, nr_segs, pos);
>> >> > +
>> >> > +           rc = filemap_fdatawrite(inode->i_mapping);
>> >> > +           if (rc)
>> >> > +                   cFYI(1, "cifs_file_aio_write: %d rc on %p inode",
>> >> > +                        rc, inode);
>> >> > +           return written;
>> >> > +   }
>> >> > +
>> >> > +   /* in strict cache mode we need to write the data to the server exactly
>> >> > +      from the pos to pos+len-1 rather than flush all affected pages
>> >> > +      because it may cause a error with mandatory locks on these pages but
>> >> > +      not on the region from pos to ppos+len-1 */
>> >>
>> >>       Again, please fix the comment style. Here: ^^^^
>> >>
>> >> > +   written = cifs_user_write(iocb->ki_filp, iov->iov_base,
>> >> > +                             iov->iov_len, &pos);
>> >> > +
>> >> > +   iocb->ki_pos = pos;
>> >> > +
>> >> > +   /* if we were successful - invalidate inode pages the write affected */
>> >> > +   if (written > 0)
>> >> > +           invalidate_mapping_pages(inode->i_mapping,
>> >> > +                                         (pos-written) >> PAGE_CACHE_SHIFT,
>> >> > +                                         (pos-1) >> PAGE_CACHE_SHIFT);
>> >> > +
>> >> >     return written;
>> >> >  }
>> >> >
>> >>
>> >> May god have mercy on anyone who tries to mix strictcache and mmap.
>> >>
>> >> Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> >
>> > (cc'ing Suresh so he can comment)
>> >
>> > Actually...I'm going to withdraw my Reviewed-by tag here for now. This
>> > bare invalidate_mapping_pages doesn't deal with fscache.
>> >
>> > I think I need to understand what's intended when someone specifies
>> > strictcache and fsc before I can ack this. The simple answer would be
>> > that they are mutually exclusive, but if that's the case then the patch
>> > that adds the mount option needs to deal with that appropriately.
>>
>>
>> I don't think they can live together. I think we should do smth like a
>> following in mount options parsing:
>>
>> ...
>> if (opt == fscache) {
>>    vol->fscahe = 1;
>>    vol->strictcache = 0;
>> }
>> ...
>> if (opt == strictcache) {
>>   vol->strictcache = 1;
>>   vol->fscache = 0;
>> }
>>
>> So, if user specify both only the last will affect the client
>> behavior. Also we should add this information into cifs manpage.
>> Thoughts?
>>
>
> That would one way to deal with it.
>
> On the other hand though...fscache allows you to keep more data cached
> than you have RAM. This could be useful in a strictcache situation as
> well. Consider the case of an application on a client that has a lot of
> large files open for read. The server may grant oplocks on all of them.
> fscache would allow for fewer round trips to the server in such a case.
>
> So, another way to deal with it would be to simply invalidate the
> fscache whenever you'd invalidate the in-ram cache. I'm not sure what
> to do for the cifs_file_aio_write case where you're invalidating just a
> small range however.
>

According to the idea that we can't keep read oplock and writing to
the server in the same time, I think we can simply call
cifs_invalidate_mapping or even set invalidate_mapping flag on inode.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 3/6] CIFS: Make write call work with strict cache mode (try #2)

[Suresh Jayaraman]

On 11/29/2010 05:07 PM, Jeff Layton wrote:
> On Mon, 29 Nov 2010 13:58:05 +0300
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
>> 2010/11/28 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>>> On Sun, 28 Nov 2010 06:36:04 -0500
>>> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>
>>>> On Sun, 28 Nov 2010 11:12:49 +0300
>>>> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>
>>>>> On strict cache mode if we don't have Exclusive oplock we write a data to
>>>>> the server through cifs_user_write. Then if we Level II oplock store it in
>>>>> the cache, otherwise - invalidate inode pages affected by this writing.
>>>>>
>>>>> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>> ---
>>>>> �fs/cifs/cifsfs.c | � 40 ++++++++++++++++++++++++++++++++++++----
>>>>> �1 files changed, 36 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>>>>> index bbb5294..901c82b 100644
>>>>> --- a/fs/cifs/cifsfs.c
>>>>> +++ b/fs/cifs/cifsfs.c
>>>>> @@ -598,12 +598,44 @@ static ssize_t cifs_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
>>>>> �static ssize_t cifs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
>>>>> � � � � � � � � � � � � � � � �unsigned long nr_segs, loff_t pos)
>>>>> �{
>>>>> - � struct inode *inode = iocb->ki_filp->f_path.dentry->d_inode;
>>>>> + � struct inode *inode;
>>>>> + � struct cifs_sb_info *cifs_sb;
>>>>> � � ssize_t written;
>>>>>
>>>>> - � written = generic_file_aio_write(iocb, iov, nr_segs, pos);
>>>>> - � if (!CIFS_I(inode)->clientCanCacheAll)
>>>>> - � � � � � filemap_fdatawrite(inode->i_mapping);
>>>>> + � inode = iocb->ki_filp->f_path.dentry->d_inode;
>>>>> +
>>>>> + � if (CIFS_I(inode)->clientCanCacheAll)
>>>>> + � � � � � return generic_file_aio_write(iocb, iov, nr_segs, pos);
>>>>> +
>>>>> + � cifs_sb = CIFS_SB(iocb->ki_filp->f_path.dentry->d_sb);
>>>>> +
>>>>> + � if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) == 0) {
>>>>> + � � � � � int rc;
>>>>> +
>>>>> + � � � � � written = generic_file_aio_write(iocb, iov, nr_segs, pos);
>>>>> +
>>>>> + � � � � � rc = filemap_fdatawrite(inode->i_mapping);
>>>>> + � � � � � if (rc)
>>>>> + � � � � � � � � � cFYI(1, "cifs_file_aio_write: %d rc on %p inode",
>>>>> + � � � � � � � � � � � �rc, inode);
>>>>> + � � � � � return written;
>>>>> + � }
>>>>> +
>>>>> + � /* in strict cache mode we need to write the data to the server exactly
>>>>> + � � �from the pos to pos+len-1 rather than flush all affected pages
>>>>> + � � �because it may cause a error with mandatory locks on these pages but
>>>>> + � � �not on the region from pos to ppos+len-1 */
>>>>
>>>> � � � Again, please fix the comment style. Here: ^^^^
>>>>
>>>>> + � written = cifs_user_write(iocb->ki_filp, iov->iov_base,
>>>>> + � � � � � � � � � � � � � � iov->iov_len, &pos);
>>>>> +
>>>>> + � iocb->ki_pos = pos;
>>>>> +
>>>>> + � /* if we were successful - invalidate inode pages the write affected */
>>>>> + � if (written > 0)
>>>>> + � � � � � invalidate_mapping_pages(inode->i_mapping,
>>>>> + � � � � � � � � � � � � � � � � � � � � (pos-written) >> PAGE_CACHE_SHIFT,
>>>>> + � � � � � � � � � � � � � � � � � � � � (pos-1) >> PAGE_CACHE_SHIFT);
>>>>> +
>>>>> � � return written;
>>>>> �}
>>>>>
>>>>
>>>> May god have mercy on anyone who tries to mix strictcache and mmap.
>>>>
>>>> Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>
>>> (cc'ing Suresh so he can comment)
>>>
>>> Actually...I'm going to withdraw my Reviewed-by tag here for now. This
>>> bare invalidate_mapping_pages doesn't deal with fscache.
>>>
>>> I think I need to understand what's intended when someone specifies
>>> strictcache and fsc before I can ack this. The simple answer would be
>>> that they are mutually exclusive, but if that's the case then the patch
>>> that adds the mount option needs to deal with that appropriately.
>>
>>
>> I don't think they can live together. I think we should do smth like a
>> following in mount options parsing:
>>
>> ...
>> if (opt == fscache) {
>>    vol->fscahe = 1;
>>    vol->strictcache = 0;
>> }
>> ...
>> if (opt == strictcache) {
>>   vol->strictcache = 1;
>>   vol->fscache = 0;
>> }
>>
>> So, if user specify both only the last will affect the client
>> behavior. Also we should add this information into cifs manpage.
>> Thoughts?
>>
> 
> That would one way to deal with it.
> 
> On the other hand though...fscache allows you to keep more data cached
> than you have RAM. This could be useful in a strictcache situation as
> well. Consider the case of an application on a client that has a lot of
> large files open for read. The server may grant oplocks on all of them.
> fscache would allow for fewer round trips to the server in such a case.
> 
> So, another way to deal with it would be to simply invalidate the
> fscache whenever you'd invalidate the in-ram cache. I'm not sure what
> to do for the cifs_file_aio_write case where you're invalidating just a
> small range however.
> 

Yes, we seem to have those two options while the later has the advantage
that Jeff mentioned. I think we could do the later without much of a
problme. We just need to retire the cookies and get new ones
(relinquishing with retire set to 1) i.e. by calling
cifs_fscache_reset_inode_cookie(). FS-Cache does not provide data
invalidation by itself. For the cifs_file_aio_write case too we could
set invalid_mapping and get fresh cookies.


Thanks,


-- 
Suresh Jayaraman

Re: [PATCH 3/6] CIFS: Make write call work with strict cache mode (try #2)

[Pavel Shilovsky]

2010/11/29 Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>:
> On 11/29/2010 05:07 PM, Jeff Layton wrote:
>> On Mon, 29 Nov 2010 13:58:05 +0300
>> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>>> 2010/11/28 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>>>> On Sun, 28 Nov 2010 06:36:04 -0500
>>>> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>>
>>>>> On Sun, 28 Nov 2010 11:12:49 +0300
>>>>> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>
>>>>>> On strict cache mode if we don't have Exclusive oplock we write a data to
>>>>>> the server through cifs_user_write. Then if we Level II oplock store it in
>>>>>> the cache, otherwise - invalidate inode pages affected by this writing.
>>>>>>
>>>>>> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>>> ---
>>>>>> �fs/cifs/cifsfs.c | � 40 ++++++++++++++++++++++++++++++++++++----
>>>>>> �1 files changed, 36 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>>>>>> index bbb5294..901c82b 100644
>>>>>> --- a/fs/cifs/cifsfs.c
>>>>>> +++ b/fs/cifs/cifsfs.c
>>>>>> @@ -598,12 +598,44 @@ static ssize_t cifs_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
>>>>>> �static ssize_t cifs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
>>>>>> � � � � � � � � � � � � � � � �unsigned long nr_segs, loff_t pos)
>>>>>> �{
>>>>>> - � struct inode *inode = iocb->ki_filp->f_path.dentry->d_inode;
>>>>>> + � struct inode *inode;
>>>>>> + � struct cifs_sb_info *cifs_sb;
>>>>>> � � ssize_t written;
>>>>>>
>>>>>> - � written = generic_file_aio_write(iocb, iov, nr_segs, pos);
>>>>>> - � if (!CIFS_I(inode)->clientCanCacheAll)
>>>>>> - � � � � � filemap_fdatawrite(inode->i_mapping);
>>>>>> + � inode = iocb->ki_filp->f_path.dentry->d_inode;
>>>>>> +
>>>>>> + � if (CIFS_I(inode)->clientCanCacheAll)
>>>>>> + � � � � � return generic_file_aio_write(iocb, iov, nr_segs, pos);
>>>>>> +
>>>>>> + � cifs_sb = CIFS_SB(iocb->ki_filp->f_path.dentry->d_sb);
>>>>>> +
>>>>>> + � if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) == 0) {
>>>>>> + � � � � � int rc;
>>>>>> +
>>>>>> + � � � � � written = generic_file_aio_write(iocb, iov, nr_segs, pos);
>>>>>> +
>>>>>> + � � � � � rc = filemap_fdatawrite(inode->i_mapping);
>>>>>> + � � � � � if (rc)
>>>>>> + � � � � � � � � � cFYI(1, "cifs_file_aio_write: %d rc on %p inode",
>>>>>> + � � � � � � � � � � � �rc, inode);
>>>>>> + � � � � � return written;
>>>>>> + � }
>>>>>> +
>>>>>> + � /* in strict cache mode we need to write the data to the server exactly
>>>>>> + � � �from the pos to pos+len-1 rather than flush all affected pages
>>>>>> + � � �because it may cause a error with mandatory locks on these pages but
>>>>>> + � � �not on the region from pos to ppos+len-1 */
>>>>>
>>>>> � � � Again, please fix the comment style. Here: ^^^^
>>>>>
>>>>>> + � written = cifs_user_write(iocb->ki_filp, iov->iov_base,
>>>>>> + � � � � � � � � � � � � � � iov->iov_len, &pos);
>>>>>> +
>>>>>> + � iocb->ki_pos = pos;
>>>>>> +
>>>>>> + � /* if we were successful - invalidate inode pages the write affected */
>>>>>> + � if (written > 0)
>>>>>> + � � � � � invalidate_mapping_pages(inode->i_mapping,
>>>>>> + � � � � � � � � � � � � � � � � � � � � (pos-written) >> PAGE_CACHE_SHIFT,
>>>>>> + � � � � � � � � � � � � � � � � � � � � (pos-1) >> PAGE_CACHE_SHIFT);
>>>>>> +
>>>>>> � � return written;
>>>>>> �}
>>>>>>
>>>>>
>>>>> May god have mercy on anyone who tries to mix strictcache and mmap.
>>>>>
>>>>> Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>>
>>>> (cc'ing Suresh so he can comment)
>>>>
>>>> Actually...I'm going to withdraw my Reviewed-by tag here for now. This
>>>> bare invalidate_mapping_pages doesn't deal with fscache.
>>>>
>>>> I think I need to understand what's intended when someone specifies
>>>> strictcache and fsc before I can ack this. The simple answer would be
>>>> that they are mutually exclusive, but if that's the case then the patch
>>>> that adds the mount option needs to deal with that appropriately.
>>>
>>>
>>> I don't think they can live together. I think we should do smth like a
>>> following in mount options parsing:
>>>
>>> ...
>>> if (opt == fscache) {
>>>    vol->fscahe = 1;
>>>    vol->strictcache = 0;
>>> }
>>> ...
>>> if (opt == strictcache) {
>>>   vol->strictcache = 1;
>>>   vol->fscache = 0;
>>> }
>>>
>>> So, if user specify both only the last will affect the client
>>> behavior. Also we should add this information into cifs manpage.
>>> Thoughts?
>>>
>>
>> That would one way to deal with it.
>>
>> On the other hand though...fscache allows you to keep more data cached
>> than you have RAM. This could be useful in a strictcache situation as
>> well. Consider the case of an application on a client that has a lot of
>> large files open for read. The server may grant oplocks on all of them.
>> fscache would allow for fewer round trips to the server in such a case.
>>
>> So, another way to deal with it would be to simply invalidate the
>> fscache whenever you'd invalidate the in-ram cache. I'm not sure what
>> to do for the cifs_file_aio_write case where you're invalidating just a
>> small range however.
>>
>
> Yes, we seem to have those two options while the later has the advantage
> that Jeff mentioned. I think we could do the later without much of a
> problme. We just need to retire the cookies and get new ones
> (relinquishing with retire set to 1) i.e. by calling
> cifs_fscache_reset_inode_cookie(). FS-Cache does not provide data
> invalidation by itself. For the cifs_file_aio_write case too we could
> set invalid_mapping and get fresh cookies.
>

Ok, I am going to provide these changes.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 3/6] CIFS: Make write call work with strict cache mode (try #2)

[Pavel Shilovsky]

2010/11/29 Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>:
> On 11/29/2010 05:07 PM, Jeff Layton wrote:
>> On Mon, 29 Nov 2010 13:58:05 +0300
>> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>>> 2010/11/28 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>>>> On Sun, 28 Nov 2010 06:36:04 -0500
>>>> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>>
>>>>> On Sun, 28 Nov 2010 11:12:49 +0300
>>>>> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>
>>>>>> On strict cache mode if we don't have Exclusive oplock we write a data to
>>>>>> the server through cifs_user_write. Then if we Level II oplock store it in
>>>>>> the cache, otherwise - invalidate inode pages affected by this writing.
>>>>>>
>>>>>> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>>> ---
>>>>>> �fs/cifs/cifsfs.c | � 40 ++++++++++++++++++++++++++++++++++++----
>>>>>> �1 files changed, 36 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>>>>>> index bbb5294..901c82b 100644
>>>>>> --- a/fs/cifs/cifsfs.c
>>>>>> +++ b/fs/cifs/cifsfs.c
>>>>>> @@ -598,12 +598,44 @@ static ssize_t cifs_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
>>>>>> �static ssize_t cifs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
>>>>>> � � � � � � � � � � � � � � � �unsigned long nr_segs, loff_t pos)
>>>>>> �{
>>>>>> - � struct inode *inode = iocb->ki_filp->f_path.dentry->d_inode;
>>>>>> + � struct inode *inode;
>>>>>> + � struct cifs_sb_info *cifs_sb;
>>>>>> � � ssize_t written;
>>>>>>
>>>>>> - � written = generic_file_aio_write(iocb, iov, nr_segs, pos);
>>>>>> - � if (!CIFS_I(inode)->clientCanCacheAll)
>>>>>> - � � � � � filemap_fdatawrite(inode->i_mapping);
>>>>>> + � inode = iocb->ki_filp->f_path.dentry->d_inode;
>>>>>> +
>>>>>> + � if (CIFS_I(inode)->clientCanCacheAll)
>>>>>> + � � � � � return generic_file_aio_write(iocb, iov, nr_segs, pos);
>>>>>> +
>>>>>> + � cifs_sb = CIFS_SB(iocb->ki_filp->f_path.dentry->d_sb);
>>>>>> +
>>>>>> + � if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) == 0) {
>>>>>> + � � � � � int rc;
>>>>>> +
>>>>>> + � � � � � written = generic_file_aio_write(iocb, iov, nr_segs, pos);
>>>>>> +
>>>>>> + � � � � � rc = filemap_fdatawrite(inode->i_mapping);
>>>>>> + � � � � � if (rc)
>>>>>> + � � � � � � � � � cFYI(1, "cifs_file_aio_write: %d rc on %p inode",
>>>>>> + � � � � � � � � � � � �rc, inode);
>>>>>> + � � � � � return written;
>>>>>> + � }
>>>>>> +
>>>>>> + � /* in strict cache mode we need to write the data to the server exactly
>>>>>> + � � �from the pos to pos+len-1 rather than flush all affected pages
>>>>>> + � � �because it may cause a error with mandatory locks on these pages but
>>>>>> + � � �not on the region from pos to ppos+len-1 */
>>>>>
>>>>> � � � Again, please fix the comment style. Here: ^^^^
>>>>>
>>>>>> + � written = cifs_user_write(iocb->ki_filp, iov->iov_base,
>>>>>> + � � � � � � � � � � � � � � iov->iov_len, &pos);
>>>>>> +
>>>>>> + � iocb->ki_pos = pos;
>>>>>> +
>>>>>> + � /* if we were successful - invalidate inode pages the write affected */
>>>>>> + � if (written > 0)
>>>>>> + � � � � � invalidate_mapping_pages(inode->i_mapping,
>>>>>> + � � � � � � � � � � � � � � � � � � � � (pos-written) >> PAGE_CACHE_SHIFT,
>>>>>> + � � � � � � � � � � � � � � � � � � � � (pos-1) >> PAGE_CACHE_SHIFT);
>>>>>> +
>>>>>> � � return written;
>>>>>> �}
>>>>>>
>>>>>
>>>>> May god have mercy on anyone who tries to mix strictcache and mmap.
>>>>>
>>>>> Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>>
>>>> (cc'ing Suresh so he can comment)
>>>>
>>>> Actually...I'm going to withdraw my Reviewed-by tag here for now. This
>>>> bare invalidate_mapping_pages doesn't deal with fscache.
>>>>
>>>> I think I need to understand what's intended when someone specifies
>>>> strictcache and fsc before I can ack this. The simple answer would be
>>>> that they are mutually exclusive, but if that's the case then the patch
>>>> that adds the mount option needs to deal with that appropriately.
>>>
>>>
>>> I don't think they can live together. I think we should do smth like a
>>> following in mount options parsing:
>>>
>>> ...
>>> if (opt == fscache) {
>>>    vol->fscahe = 1;
>>>    vol->strictcache = 0;
>>> }
>>> ...
>>> if (opt == strictcache) {
>>>   vol->strictcache = 1;
>>>   vol->fscache = 0;
>>> }
>>>
>>> So, if user specify both only the last will affect the client
>>> behavior. Also we should add this information into cifs manpage.
>>> Thoughts?
>>>
>>
>> That would one way to deal with it.
>>
>> On the other hand though...fscache allows you to keep more data cached
>> than you have RAM. This could be useful in a strictcache situation as
>> well. Consider the case of an application on a client that has a lot of
>> large files open for read. The server may grant oplocks on all of them.
>> fscache would allow for fewer round trips to the server in such a case.
>>
>> So, another way to deal with it would be to simply invalidate the
>> fscache whenever you'd invalidate the in-ram cache. I'm not sure what
>> to do for the cifs_file_aio_write case where you're invalidating just a
>> small range however.
>>
>
> Yes, we seem to have those two options while the later has the advantage
> that Jeff mentioned. I think we could do the later without much of a
> problme. We just need to retire the cookies and get new ones
> (relinquishing with retire set to 1) i.e. by calling
> cifs_fscache_reset_inode_cookie(). FS-Cache does not provide data
> invalidation by itself. For the cifs_file_aio_write case too we could
> set invalid_mapping and get fresh cookies.
>
>

What's about mmap and fsync patch? How do you think they mix with fscache?

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 3/6] CIFS: Make write call work with strict cache mode (try #2)

[Suresh Jayaraman]

On 11/29/2010 05:56 PM, Pavel Shilovsky wrote:
> 2010/11/29 Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>:
>> On 11/29/2010 05:07 PM, Jeff Layton wrote:
>>> On Mon, 29 Nov 2010 13:58:05 +0300
>>> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>
>>>> 2010/11/28 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>>>>> On Sun, 28 Nov 2010 06:36:04 -0500
>>>>> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>>>
>>>>>> On Sun, 28 Nov 2010 11:12:49 +0300
>>>>>> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>>
>>>>>>> On strict cache mode if we don't have Exclusive oplock we write a data to
>>>>>>> the server through cifs_user_write. Then if we Level II oplock store it in
>>>>>>> the cache, otherwise - invalidate inode pages affected by this writing.
>>>>>>>
>>>>>>> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>>>> ---
>>>>>>> �fs/cifs/cifsfs.c | � 40 ++++++++++++++++++++++++++++++++++++----
>>>>>>> �1 files changed, 36 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>>>>>>> index bbb5294..901c82b 100644
>>>>>>> --- a/fs/cifs/cifsfs.c
>>>>>>> +++ b/fs/cifs/cifsfs.c
>>>>>>> @@ -598,12 +598,44 @@ static ssize_t cifs_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
>>>>>>> �static ssize_t cifs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
>>>>>>> � � � � � � � � � � � � � � � �unsigned long nr_segs, loff_t pos)
>>>>>>> �{
>>>>>>> - � struct inode *inode = iocb->ki_filp->f_path.dentry->d_inode;
>>>>>>> + � struct inode *inode;
>>>>>>> + � struct cifs_sb_info *cifs_sb;
>>>>>>> � � ssize_t written;
>>>>>>>
>>>>>>> - � written = generic_file_aio_write(iocb, iov, nr_segs, pos);
>>>>>>> - � if (!CIFS_I(inode)->clientCanCacheAll)
>>>>>>> - � � � � � filemap_fdatawrite(inode->i_mapping);
>>>>>>> + � inode = iocb->ki_filp->f_path.dentry->d_inode;
>>>>>>> +
>>>>>>> + � if (CIFS_I(inode)->clientCanCacheAll)
>>>>>>> + � � � � � return generic_file_aio_write(iocb, iov, nr_segs, pos);
>>>>>>> +
>>>>>>> + � cifs_sb = CIFS_SB(iocb->ki_filp->f_path.dentry->d_sb);
>>>>>>> +
>>>>>>> + � if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) == 0) {
>>>>>>> + � � � � � int rc;
>>>>>>> +
>>>>>>> + � � � � � written = generic_file_aio_write(iocb, iov, nr_segs, pos);
>>>>>>> +
>>>>>>> + � � � � � rc = filemap_fdatawrite(inode->i_mapping);
>>>>>>> + � � � � � if (rc)
>>>>>>> + � � � � � � � � � cFYI(1, "cifs_file_aio_write: %d rc on %p inode",
>>>>>>> + � � � � � � � � � � � �rc, inode);
>>>>>>> + � � � � � return written;
>>>>>>> + � }
>>>>>>> +
>>>>>>> + � /* in strict cache mode we need to write the data to the server exactly
>>>>>>> + � � �from the pos to pos+len-1 rather than flush all affected pages
>>>>>>> + � � �because it may cause a error with mandatory locks on these pages but
>>>>>>> + � � �not on the region from pos to ppos+len-1 */
>>>>>>
>>>>>> � � � Again, please fix the comment style. Here: ^^^^
>>>>>>
>>>>>>> + � written = cifs_user_write(iocb->ki_filp, iov->iov_base,
>>>>>>> + � � � � � � � � � � � � � � iov->iov_len, &pos);
>>>>>>> +
>>>>>>> + � iocb->ki_pos = pos;
>>>>>>> +
>>>>>>> + � /* if we were successful - invalidate inode pages the write affected */
>>>>>>> + � if (written > 0)
>>>>>>> + � � � � � invalidate_mapping_pages(inode->i_mapping,
>>>>>>> + � � � � � � � � � � � � � � � � � � � � (pos-written) >> PAGE_CACHE_SHIFT,
>>>>>>> + � � � � � � � � � � � � � � � � � � � � (pos-1) >> PAGE_CACHE_SHIFT);
>>>>>>> +
>>>>>>> � � return written;
>>>>>>> �}
>>>>>>>
>>>>>>
>>>>>> May god have mercy on anyone who tries to mix strictcache and mmap.
>>>>>>
>>>>>> Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>>>
>>>>> (cc'ing Suresh so he can comment)
>>>>>
>>>>> Actually...I'm going to withdraw my Reviewed-by tag here for now. This
>>>>> bare invalidate_mapping_pages doesn't deal with fscache.
>>>>>
>>>>> I think I need to understand what's intended when someone specifies
>>>>> strictcache and fsc before I can ack this. The simple answer would be
>>>>> that they are mutually exclusive, but if that's the case then the patch
>>>>> that adds the mount option needs to deal with that appropriately.
>>>>
>>>>
>>>> I don't think they can live together. I think we should do smth like a
>>>> following in mount options parsing:
>>>>
>>>> ...
>>>> if (opt == fscache) {
>>>>    vol->fscahe = 1;
>>>>    vol->strictcache = 0;
>>>> }
>>>> ...
>>>> if (opt == strictcache) {
>>>>   vol->strictcache = 1;
>>>>   vol->fscache = 0;
>>>> }
>>>>
>>>> So, if user specify both only the last will affect the client
>>>> behavior. Also we should add this information into cifs manpage.
>>>> Thoughts?
>>>>
>>>
>>> That would one way to deal with it.
>>>
>>> On the other hand though...fscache allows you to keep more data cached
>>> than you have RAM. This could be useful in a strictcache situation as
>>> well. Consider the case of an application on a client that has a lot of
>>> large files open for read. The server may grant oplocks on all of them.
>>> fscache would allow for fewer round trips to the server in such a case.
>>>
>>> So, another way to deal with it would be to simply invalidate the
>>> fscache whenever you'd invalidate the in-ram cache. I'm not sure what
>>> to do for the cifs_file_aio_write case where you're invalidating just a
>>> small range however.
>>>
>>
>> Yes, we seem to have those two options while the later has the advantage
>> that Jeff mentioned. I think we could do the later without much of a
>> problme. We just need to retire the cookies and get new ones
>> (relinquishing with retire set to 1) i.e. by calling
>> cifs_fscache_reset_inode_cookie(). FS-Cache does not provide data
>> invalidation by itself. For the cifs_file_aio_write case too we could
>> set invalid_mapping and get fresh cookies.
>>
>>
> 
> What's about mmap and fsync patch? How do you think they mix with fscache?
> 

Basically, relinquishing cookie when you are invalidating inode should
make them work without issues I think. But, I would also test your
patches (once you post the patchset that includes fscache handling as
well) to be sure.


Thanks,


-- 
Suresh Jayaraman

Re: [PATCH 3/6] CIFS: Make write call work with strict cache mode (try #2)

[Pavel Shilovsky]

2010/11/29 Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>:
> On 11/29/2010 05:56 PM, Pavel Shilovsky wrote:
>>
>> What's about mmap and fsync patch? How do you think they mix with fscache?
>>
>
> Basically, relinquishing cookie when you are invalidating inode should
> make them work without issues I think. But, I would also test your
> patches (once you post the patchset that includes fscache handling as
> well) to be sure.
>
>

I reposted patches from 2 to 5(included). So, uptodate set is

Pavel Shilovsky (6):
 CIFS: Make cifsFileInfo_put work with strict cache mode (try #4)
 CIFS: Make read call work with strict cache mode (try #3)
 CIFS: Make write call work with strict cache mode (try #3)
 CIFS: Make cifs_fsync work with strict cache mode (try #3)
 CIFS: Make cifs_file_map work with strict cache mode (try #3)
 CIFS: Add strictcache mount option (try #2)

please, let me know about results when you finished testing them.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 3/6] CIFS: Make write call work with strict cache mode (try #2)

[Suresh Jayaraman]

On 11/29/2010 10:38 PM, Pavel Shilovsky wrote:
> 2010/11/29 Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>:
>> On 11/29/2010 05:56 PM, Pavel Shilovsky wrote:
>>>
>>> What's about mmap and fsync patch? How do you think they mix with fscache?
>>>
>>
>> Basically, relinquishing cookie when you are invalidating inode should
>> make them work without issues I think. But, I would also test your
>> patches (once you post the patchset that includes fscache handling as
>> well) to be sure.
>>
>>
> 
> I reposted patches from 2 to 5(included). So, uptodate set is
> 
> Pavel Shilovsky (6):
>  CIFS: Make cifsFileInfo_put work with strict cache mode (try #4)
>  CIFS: Make read call work with strict cache mode (try #3)
>  CIFS: Make write call work with strict cache mode (try #3)
>  CIFS: Make cifs_fsync work with strict cache mode (try #3)
>  CIFS: Make cifs_file_map work with strict cache mode (try #3)
>  CIFS: Add strictcache mount option (try #2)
> 

I just applied your patches on my test box, but I see that the fscache
integration is not done, yet. Am I seeing older version of patches?


-- 
Suresh Jayaraman

Re: [PATCH 3/6] CIFS: Make write call work with strict cache mode (try #2)

[Pavel Shilovsky]

2010/12/1 Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>:
>
> I just applied your patches on my test box, but I see that the fscache
> integration is not done, yet. Am I seeing older version of patches?

What do you mean for "fscache integration"? I changed all
invalidate_inode_* calls to cifs_invalidate_mapping or setting
invalid_mapping flag - so, it seems to me that I don't miss places
where I affect cache but don't aware of fscache
(cifs_invalidate_mapping process fscache as well). Of course, it can
be - so, could you point me to such places, please?

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 3/6] CIFS: Make write call work with strict cache mode (try #2)

[Suresh Jayaraman]

On 12/01/2010 10:09 PM, Pavel Shilovsky wrote:
> 2010/12/1 Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>:
>>
>> I just applied your patches on my test box, but I see that the fscache
>> integration is not done, yet. Am I seeing older version of patches?
> 
> What do you mean for "fscache integration"? I changed all
> invalidate_inode_* calls to cifs_invalidate_mapping or setting
> invalid_mapping flag - so, it seems to me that I don't miss places
> where I affect cache but don't aware of fscache
> (cifs_invalidate_mapping process fscache as well). Of course, it can
> be - so, could you point me to such places, please?
> 

Sorry about the delay in getting back. You're right the patches are
taking care of relinquishing the cookies wherever appropriate.
I have done some testing with both 'strictcache' and 'fsc' enabled and
so far they work as expected. The tests involve doing file operations
with fsc enabled always but strict cache enabled/disabled and watching
fscache statistics via /proc/fs/fscache/stats). I didn't encounter any
unexpected results.


Thanks,

-- 
Suresh Jayaraman

Re: [PATCH 3/6] CIFS: Make write call work with strict cache mode (try #2)

[Pavel Shilovsky]

2010/12/6 Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>:
> On 12/01/2010 10:09 PM, Pavel Shilovsky wrote:
>> 2010/12/1 Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>:
>>>
>>> I just applied your patches on my test box, but I see that the fscache
>>> integration is not done, yet. Am I seeing older version of patches?
>>
>> What do you mean for "fscache integration"? I changed all
>> invalidate_inode_* calls to cifs_invalidate_mapping or setting
>> invalid_mapping flag - so, it seems to me that I don't miss places
>> where I affect cache but don't aware of fscache
>> (cifs_invalidate_mapping process fscache as well). Of course, it can
>> be - so, could you point me to such places, please?
>>
>
> Sorry about the delay in getting back. You're right the patches are
> taking care of relinquishing the cookies wherever appropriate.
> I have done some testing with both 'strictcache' and 'fsc' enabled and
> so far they work as expected. The tests involve doing file operations
> with fsc enabled always but strict cache enabled/disabled and watching
> fscache statistics via /proc/fs/fscache/stats). I didn't encounter any
> unexpected results.

Thank you very much for this testing!

Jeff, Suresh, as we have the good results on strict cache + fscache
and reviewed patch number 1 (cifsFileInfo_put part), could you review
other patches: 2) read (try #4), 3) write (try #4), 4) fsync (try #3),
5) mmap (try #3) and 6) mount option (try #2) please?

-- 
Best regards,
Pavel Shilovsky.

[PATCH 4/6] CIFS: Make cifs_fsync work with strict cache mode (try #2)

[Pavel Shilovsky]

Remove filemap_write_and_wait call because it is previously called from
vfs_sync_range and invalidate inode if we don't have at least Level II
oplock and strcit cache mode switched on.

Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/cifs/file.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index f853bf0..22d0421 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -1595,20 +1595,20 @@ int cifs_fsync(struct file *file, int datasync)
 	struct cifsTconInfo *tcon;
 	struct cifsFileInfo *smbfile = file->private_data;
 	struct inode *inode = file->f_path.dentry->d_inode;
+	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
 
 	xid = GetXid();
 
 	cFYI(1, "Sync file - name: %s datasync: 0x%x",
 		file->f_path.dentry->d_name.name, datasync);
 
-	rc = filemap_write_and_wait(inode->i_mapping);
-	if (rc == 0) {
-		struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
+	if (!CIFS_I(inode)->clientCanCacheRead &&
+	    (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO))
+		invalidate_remote_inode(inode);
 
-		tcon = tlink_tcon(smbfile->tlink);
-		if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
-			rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
-	}
+	tcon = tlink_tcon(smbfile->tlink);
+	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
+		rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
 
 	FreeXid(xid);
 	return rc;
-- 
1.7.3.2

Re: [PATCH 4/6] CIFS: Make cifs_fsync work with strict cache mode (try #2)

[Jeff Layton]

On Sun, 28 Nov 2010 11:12:50 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Remove filemap_write_and_wait call because it is previously called from
> vfs_sync_range and invalidate inode if we don't have at least Level II
> oplock and strcit cache mode switched on.
> 

I think you mean vfs_fsync_range?

> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/cifs/file.c |   14 +++++++-------
>  1 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index f853bf0..22d0421 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -1595,20 +1595,20 @@ int cifs_fsync(struct file *file, int datasync)
>  	struct cifsTconInfo *tcon;
>  	struct cifsFileInfo *smbfile = file->private_data;
>  	struct inode *inode = file->f_path.dentry->d_inode;
> +	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
>  
>  	xid = GetXid();
>  
>  	cFYI(1, "Sync file - name: %s datasync: 0x%x",
>  		file->f_path.dentry->d_name.name, datasync);
>  
> -	rc = filemap_write_and_wait(inode->i_mapping);
> -	if (rc == 0) {
> -		struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
> +	if (!CIFS_I(inode)->clientCanCacheRead &&
> +	    (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO))
> +		invalidate_remote_inode(inode);
>  
> -		tcon = tlink_tcon(smbfile->tlink);
> -		if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
> -			rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
> -	}
> +	tcon = tlink_tcon(smbfile->tlink);
> +	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
> +		rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
>  
>  	FreeXid(xid);
>  	return rc;


Looks reasonable. I like the fact that we're pulling out the
unnecessary filemap_write_and_wait here.

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

Re: [PATCH 4/6] CIFS: Make cifs_fsync work with strict cache mode (try #2)

[Jeff Layton]

On Sun, 28 Nov 2010 06:42:19 -0500
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> On Sun, 28 Nov 2010 11:12:50 +0300
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > Remove filemap_write_and_wait call because it is previously called from
> > vfs_sync_range and invalidate inode if we don't have at least Level II
> > oplock and strcit cache mode switched on.
> > 
> 
> I think you mean vfs_fsync_range?
> 
> > Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> >  fs/cifs/file.c |   14 +++++++-------
> >  1 files changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index f853bf0..22d0421 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -1595,20 +1595,20 @@ int cifs_fsync(struct file *file, int datasync)
> >  	struct cifsTconInfo *tcon;
> >  	struct cifsFileInfo *smbfile = file->private_data;
> >  	struct inode *inode = file->f_path.dentry->d_inode;
> > +	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
> >  
> >  	xid = GetXid();
> >  
> >  	cFYI(1, "Sync file - name: %s datasync: 0x%x",
> >  		file->f_path.dentry->d_name.name, datasync);
> >  
> > -	rc = filemap_write_and_wait(inode->i_mapping);
> > -	if (rc == 0) {
> > -		struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
> > +	if (!CIFS_I(inode)->clientCanCacheRead &&
> > +	    (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO))
> > +		invalidate_remote_inode(inode);
> >  
> > -		tcon = tlink_tcon(smbfile->tlink);
> > -		if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
> > -			rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
> > -	}
> > +	tcon = tlink_tcon(smbfile->tlink);
> > +	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
> > +		rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
> >  
> >  	FreeXid(xid);
> >  	return rc;
> 
> 
> Looks reasonable. I like the fact that we're pulling out the
> unnecessary filemap_write_and_wait here.
> 
> Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Actually...I must withdraw this Reviewed-by until I understand how this
is expected to mesh with fscache.

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

[PATCH 5/6] CIFS: Make cifs_file_map work with strict cache mode (try #2)

[Pavel Shilovsky]

Invalidate inode if we don't have at least Level II oplock and strict
cache mode switched on.

Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/cifs/file.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 22d0421..d9dcc98 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -1812,14 +1812,24 @@ static ssize_t cifs_read(struct file *file, char *read_data, size_t read_size,
 int cifs_file_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	int rc, xid;
+	struct inode *inode = file->f_path.dentry->d_inode;
+	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
 
 	xid = GetXid();
-	rc = cifs_revalidate_file(file);
-	if (rc) {
-		cFYI(1, "Validation prior to mmap failed, error=%d", rc);
-		FreeXid(xid);
-		return rc;
+
+	if (!CIFS_I(inode)->clientCanCacheRead &&
+	    (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO))
+		invalidate_remote_inode(inode);
+	else {
+		rc = cifs_revalidate_file(file);
+		if (rc) {
+			cFYI(1, "Validation prior to mmap failed, error=%d",
+			     rc);
+			FreeXid(xid);
+			return rc;
+		}
 	}
+
 	rc = generic_file_mmap(file, vma);
 	FreeXid(xid);
 	return rc;
-- 
1.7.3.2

[PATCH 6/6] CIFS: Add strictcache mount option (try #2)

[Pavel Shilovsky]

Use for switching on strict cache mode. In this mode the
client read from the cache all the time it has Oplock Level II,
otherwise - read from the server. All written data are stored
in the cache, but if the client doesn't have Exclusive Oplock,
it writes the data to the server.

Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/cifs/README    |    5 +++++
 fs/cifs/connect.c |    5 +++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/README b/fs/cifs/README
index ee68d10..8fb6192 100644
--- a/fs/cifs/README
+++ b/fs/cifs/README
@@ -443,6 +443,11 @@ A partial list of the supported mount options follows:
 		if oplock (caching token) is granted and held. Note that
 		direct allows write operations larger than page size
 		to be sent to the server.
+  strictcache   Use for switching on strict cache mode. In this mode the
+		client read from the cache all the time it has Oplock Level II,
+		otherwise - read from the server. All written data are stored
+		in the cache, but if the client doesn't have Exclusive Oplock,
+		it writes the data to the server.
   acl   	Allow setfacl and getfacl to manage posix ACLs if server
 		supports them.  (default)
   noacl 	Do not allow setfacl and getfacl calls on this mount
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 251a17c..97286c7 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -84,6 +84,7 @@ struct smb_vol {
 	bool no_xattr:1;   /* set if xattr (EA) support should be disabled*/
 	bool server_ino:1; /* use inode numbers from server ie UniqueId */
 	bool direct_io:1;
+	bool strict_io:1; /* strict cache behavior */
 	bool remap:1;      /* set to remap seven reserved chars in filenames */
 	bool posix_paths:1; /* unset to not ask for posix pathnames. */
 	bool no_linux_ext:1;
@@ -1347,6 +1348,8 @@ cifs_parse_mount_options(char *options, const char *devname,
 			vol->direct_io = 1;
 		} else if (strnicmp(data, "forcedirectio", 13) == 0) {
 			vol->direct_io = 1;
+		} else if (strnicmp(data, "strictcache", 11) == 0) {
+			vol->strict_io = 1;
 		} else if (strnicmp(data, "noac", 4) == 0) {
 			printk(KERN_WARNING "CIFS: Mount option noac not "
 				"supported. Instead set "
@@ -2597,6 +2600,8 @@ static void setup_cifs_sb(struct smb_vol *pvolume_info,
 	if (pvolume_info->multiuser)
 		cifs_sb->mnt_cifs_flags |= (CIFS_MOUNT_MULTIUSER |
 					    CIFS_MOUNT_NO_PERM);
+	if (pvolume_info->strict_io)
+		cifs_sb->mnt_cifs_flags |= CIFS_MOUNT_STRICT_IO;
 	if (pvolume_info->direct_io) {
 		cFYI(1, "mounting share using direct i/o");
 		cifs_sb->mnt_cifs_flags |= CIFS_MOUNT_DIRECT_IO;
-- 
1.7.3.2

Re: [PATCH 0/6] Add strict cache mode (try #4)

[Suresh Jayaraman]

On 11/28/2010 01:42 PM, Pavel Shilovsky wrote:
> Re-posting the whole set of strict cache patches.

I try to explain here the "strict cache" semantics as I'm not sure
whether I understand it clearly. Please correct me if I'm wrong.

Strict cache semantics

  o provides stricter cache coherency among cifs clients that access
    the same files.
  o The clients will read data from the Server always, except when they
    hold read oplock or Level II oplock on the file.
  o The clients will write data to the Server always, except when they
    hold exclusive oplock on the file.
  o When we close the last filehandle of the inode, file should be
    marked for revalidation as it is possible for the client to access
    stale data from the cache when we open it again with a read
    oplock.
  o On fsync/mmap, invalidate inode if read oplock has not been set.


Is this the semantics being proposed? Did I miss anything?


Thanks,


> Pavel Shilovsky (6):
>   CIFS: Make cifsFileInfo_put work with strict cache mode (try #4)
>   CIFS: Make read call work with strict cache mode (try #2)
>   CIFS: Make write call work with strict cache mode (try #2)
>   CIFS: Make cifs_fsync work with strict cache mode (try #2)
>   CIFS: Make cifs_file_map work with strict cache mode (try #2)
>   CIFS: Add strictcache mount option (try #2)
> 
>  fs/cifs/README       |    5 +++
>  fs/cifs/cifs_fs_sb.h |    1 +
>  fs/cifs/cifsfs.c     |   71 +++++++++++++++++++++++++++++++++++++++++++++----
>  fs/cifs/connect.c    |    5 +++
>  fs/cifs/file.c       |   42 +++++++++++++++++++++--------
>  5 files changed, 106 insertions(+), 18 deletions(-)
> 


-- 
Suresh Jayaraman

Re: [PATCH 0/6] Add strict cache mode (try #4)

[Pavel Shilovsky]

2010/11/29 Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>:
> On 11/28/2010 01:42 PM, Pavel Shilovsky wrote:
>> Re-posting the whole set of strict cache patches.
>
> I try to explain here the "strict cache" semantics as I'm not sure
> whether I understand it clearly. Please correct me if I'm wrong.
>
> Strict cache semantics
>
>  o provides stricter cache coherency among cifs clients that access
>    the same files.
>  o The clients will read data from the Server always, except when they
>    hold read oplock or Level II oplock on the file.
>  o The clients will write data to the Server always, except when they
>    hold exclusive oplock on the file.
>  o When we close the last filehandle of the inode, file should be
>    marked for revalidation as it is possible for the client to access
>    stale data from the cache when we open it again with a read
>    oplock.
>  o On fsync/mmap, invalidate inode if read oplock has not been set.
>
>
> Is this the semantics being proposed? Did I miss anything?

Yes, you are right - it is exactly what I mean.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 0/6] Add strict cache mode (try #4)

[Suresh Jayaraman]

On 11/29/2010 04:30 PM, Pavel Shilovsky wrote:
> 2010/11/29 Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>:
>> On 11/28/2010 01:42 PM, Pavel Shilovsky wrote:
>>> Re-posting the whole set of strict cache patches.
>>
>> I try to explain here the "strict cache" semantics as I'm not sure
>> whether I understand it clearly. Please correct me if I'm wrong.
>>
>> Strict cache semantics
>>
>> �o provides stricter cache coherency among cifs clients that access
>> � �the same files.
>> �o The clients will read data from the Server always, except when they
>> � �hold read oplock or Level II oplock on the file.
>> �o The clients will write data to the Server always, except when they
>> � �hold exclusive oplock on the file.
>> �o When we close the last filehandle of the inode, file should be
>> � �marked for revalidation as it is possible for the client to access
>> � �stale data from the cache when we open it again with a read
>> � �oplock.
>> �o On fsync/mmap, invalidate inode if read oplock has not been set.
>>
>>
>> Is this the semantics being proposed? Did I miss anything?
> 
> Yes, you are right - it is exactly what I mean.
> 

Also, would be good to know what level of testing these patches have
undergone. Could you share the test results as well?


Thanks,

-- 
Suresh Jayaraman

Re: [PATCH 0/6] Add strict cache mode (try #4)

[Pavel Shilovsky]

[-- Attachment #1: Type: text/plain, Size: 1602 bytes --]

2010/11/29 Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>:
> On 11/29/2010 04:30 PM, Pavel Shilovsky wrote:
>> 2010/11/29 Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>:
>>> On 11/28/2010 01:42 PM, Pavel Shilovsky wrote:
>>>> Re-posting the whole set of strict cache patches.
>>>
>>> I try to explain here the "strict cache" semantics as I'm not sure
>>> whether I understand it clearly. Please correct me if I'm wrong.
>>>
>>> Strict cache semantics
>>>
>>> �o provides stricter cache coherency among cifs clients that access
>>> � �the same files.
>>> �o The clients will read data from the Server always, except when they
>>> � �hold read oplock or Level II oplock on the file.
>>> �o The clients will write data to the Server always, except when they
>>> � �hold exclusive oplock on the file.
>>> �o When we close the last filehandle of the inode, file should be
>>> � �marked for revalidation as it is possible for the client to access
>>> � �stale data from the cache when we open it again with a read
>>> � �oplock.
>>> �o On fsync/mmap, invalidate inode if read oplock has not been set.
>>>
>>>
>>> Is this the semantics being proposed? Did I miss anything?
>>
>> Yes, you are right - it is exactly what I mean.
>>
>
> Also, would be good to know what level of testing these patches have
> undergone. Could you share the test results as well?
>

I don't run big stress tests over it but I have tree small tests that
shows the problem this mode is going to fix.

-- 
Best regards,
Pavel Shilovsky.

[-- Attachment #2: close_problem.py --]
[-- Type: text/x-python, Size: 542 bytes --]

#!/bin/env python
#
# We have to mount the same share to test, test1, test2 directories that locate in the directory we
# execute this script from.

from os import open, close, O_RDWR, O_CREAT, write, read, O_RDONLY, O_WRONLY, O_TRUNC, lseek, SEEK_END, SEEK_SET

f = open('test/_test4321_', O_RDWR | O_CREAT | O_TRUNC)
close(f)

f1 = open('test1/_test4321_', O_RDWR)
write(f1, 'a')
close(f1)

f2 = open('test2/_test4321_', O_WRONLY)
write(f2, 'x')
close(f2)

f3 = open('test1/_test4321_', O_RDONLY)
print 'must be x:', read(f3, 1)

close(f3)

[-- Attachment #3: mtime_problem.py --]
[-- Type: text/x-python, Size: 542 bytes --]

#!/bin/env python
#
# We have to mount the same share to test, test1, test2 directories that locate in the directory we
# execute this script from.

from os import open, close, O_RDWR, O_CREAT, write, read, O_RDONLY, O_WRONLY, O_TRUNC, lseek, SEEK_END, SEEK_SET

f = open('test/_test4321_', O_RDWR | O_CREAT | O_TRUNC)
close(f)

f1 = open('test1/_test4321_', O_RDWR)
write(f1, 'a')

f2 = open('test2/_test4321_', O_WRONLY)
write(f2, 'x')

f3 = open('test1/_test4321_', O_RDONLY)
print 'must be x:', read(f3, 1)

close(f1)
close(f2)
close(f3)

[-- Attachment #4: cache_problem.py --]
[-- Type: text/x-python, Size: 717 bytes --]

#!/bin/env python
#
# We have to mount the same share to test, test1, test2 directories that locate in the directory we
# execute this script from.

from os import open, close, O_RDWR, O_CREAT, write, read, O_RDONLY, O_WRONLY, O_TRUNC, lseek

f = open('test/_test4321_', O_RDWR | O_CREAT | O_TRUNC)
write(f, ''.join('a' for _ in range(4096)))
close(f)

f1 = open('test1/_test4321_', O_RDWR)
f2 = open('test2/_test4321_', O_RDWR)

write(f1, 'x')
print 'x is written through f1'
print '%c is read from f2' % read(f2, 1)

write(f1, 'y')
print 'y is written through f1'
print '%c is read from f2' % read(f2, 1)

write(f1, 'z')
print 'z is written through f1'
print '%c is read from f2' % read(f2, 1)

close(f1)
close(f2)