[PATCH] CIFS: New read cache mechanism

[PATCH] CIFS: New read cache mechanism

[Pavel Shilovsky]

Add cifs_sync_read call to provide reading from the cache if we have at least
Level II oplock and otherwise - reading from the server.

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

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index b7431af..ba70048 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -533,6 +533,25 @@ cifs_get_sb(struct file_system_type *fs_type,
 	return 0;
 }

+static ssize_t cifs_sync_read(struct file *filp, char __user *buf,
+			      size_t len, loff_t *ppos)
+{
+	ssize_t read;
+	struct cifsInodeInfo *cinode;
+
+	if (filp && filp->f_path.dentry && filp->f_path.dentry->d_inode)
+		cinode = CIFS_I(filp->f_path.dentry->d_inode);
+	else
+		return -ENOENT;
+
+	if (cinode->clientCanCacheRead)
+		read = do_sync_read(filp, buf, len, ppos);
+	else
+		read = cifs_user_read(filp, buf, len, ppos);
+
+	return read;
+}
+
 static ssize_t cifs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
 				   unsigned long nr_segs, loff_t pos)
 {
@@ -652,7 +671,7 @@ const struct inode_operations cifs_symlink_inode_ops = {
 };

 const struct file_operations cifs_file_ops = {
-	.read = do_sync_read,
+	.read = cifs_sync_read,
 	.write = do_sync_write,
 	.aio_read = generic_file_aio_read,
 	.aio_write = cifs_file_aio_write,
@@ -689,7 +708,7 @@ const struct file_operations cifs_file_direct_ops = {
 	.setlease = cifs_setlease,
 };
 const struct file_operations cifs_file_nobrl_ops = {
-	.read = do_sync_read,
+	.read = cifs_sync_read,
 	.write = do_sync_write,
 	.aio_read = generic_file_aio_read,
 	.aio_write = cifs_file_aio_write,
-- 
1.7.2.1

Re: [PATCH] CIFS: New read cache mechanism

[Jeff Layton]

On Sat, 18 Sep 2010 10:32:03 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Add cifs_sync_read call to provide reading from the cache if we have at least
> Level II oplock and otherwise - reading from the server.
> 
> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/cifs/cifsfs.c |   23 +++++++++++++++++++++--
>  1 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index b7431af..ba70048 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -533,6 +533,25 @@ cifs_get_sb(struct file_system_type *fs_type,
>  	return 0;
>  }
> 
> +static ssize_t cifs_sync_read(struct file *filp, char __user *buf,
> +			      size_t len, loff_t *ppos)
> +{
> +	ssize_t read;
> +	struct cifsInodeInfo *cinode;
> +
> +	if (filp && filp->f_path.dentry && filp->f_path.dentry->d_inode)

A NULL filp or one with a negative dentry would be a serious VFS bug.
I'd get rid of the NULL pointer checks above. I don't think they're
necessary.

> +		cinode = CIFS_I(filp->f_path.dentry->d_inode);
> +	else
> +		return -ENOENT;
> +
> +	if (cinode->clientCanCacheRead)
> +		read = do_sync_read(filp, buf, len, ppos);
> +	else
> +		read = cifs_user_read(filp, buf, len, ppos);
> +
> +	return read;
> +}
> +
>  static ssize_t cifs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
>  				   unsigned long nr_segs, loff_t pos)
>  {
> @@ -652,7 +671,7 @@ const struct inode_operations cifs_symlink_inode_ops = {
>  };
> 
>  const struct file_operations cifs_file_ops = {
> -	.read = do_sync_read,
> +	.read = cifs_sync_read,
>  	.write = do_sync_write,
>  	.aio_read = generic_file_aio_read,
>  	.aio_write = cifs_file_aio_write,
> @@ -689,7 +708,7 @@ const struct file_operations cifs_file_direct_ops = {
>  	.setlease = cifs_setlease,
>  };
>  const struct file_operations cifs_file_nobrl_ops = {
> -	.read = do_sync_read,
> +	.read = cifs_sync_read,
>  	.write = do_sync_write,
>  	.aio_read = generic_file_aio_read,
>  	.aio_write = cifs_file_aio_write,

Looks reasonable, but it would be good to know what sort of performance
impact to expect from this. Have you done any benchmarking pre and post
patch?

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

Re: [PATCH] CIFS: New read cache mechanism

[Christoph Hellwig]

> +	struct cifsInodeInfo *cinode;
> +
> +	if (filp && filp->f_path.dentry && filp->f_path.dentry->d_inode)
> +		cinode = CIFS_I(filp->f_path.dentry->d_inode);
> +	else
> +		return -ENOENT;

Totally pointless check.  Could you guys please stop putting in all
these utterly pointless cargo cult checks in cifs?

> +
> +	if (cinode->clientCanCacheRead)
> +		read = do_sync_read(filp, buf, len, ppos);
> +	else
> +		read = cifs_user_read(filp, buf, len, ppos);
> +
> +	return read;

And how do you enforce your caching semantics for aio or vectored reads?

Re: [PATCH] CIFS: New read cache mechanism

[Steve French]

On Sat, Sep 18, 2010 at 1:32 AM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Add cifs_sync_read call to provide reading from the cache if we have at least
> Level II oplock and otherwise - reading from the server.

This is a stricter guarantee than NFS and will slow performance
dramatically.  If you are going to prevent all ability to reread a
page - this should probably be optional.   This affects cases even
when there are no 2nd clients opening the same file and cases where a
2nd client opened a file but never wrote to it.  Note that the common
case of multiply open files from the same client will break oplock too
even though there is no caching issue there (much as we would like to
change the protocol for that - we have to wait until SMB2 until we can
reaquire or upgrade oplocks).

Currently we write through all writes when we don't have oplock.   We
should also invalidate any cached pages when opening a non-cached file
that has changed since we last opened it - so we will reread these
pages although they will stay in the page cache.   We should
invalidate these cached pages whenever mtime changes (even if some
Windows server defer mtime updates - it will be correct file close or
sync - which is the only place we have guarantees that data is written
anyway).  We check mtime updates in revalidate (and this can be
configured in /proc/fs/cifs to always be sent - rather than every
second).


-- 
Thanks,

Steve

Re: [PATCH] CIFS: New read cache mechanism

[Pavel Shilovsky]

> Looks reasonable, but it would be good to know what sort of performance
> impact to expect from this. Have you done any benchmarking pre and post
> patch?

I don't run any performance benchmarking on it but of course I did
checkings with for logic and tried it with the real Wine application,
that has network mode provided by CIFS.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH] CIFS: New read cache mechanism

[Jeff Layton]

On Sun, 19 Sep 2010 09:52:10 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Sat, Sep 18, 2010 at 1:32 AM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > Add cifs_sync_read call to provide reading from the cache if we have at least
> > Level II oplock and otherwise - reading from the server.
> 
> This is a stricter guarantee than NFS and will slow performance
> dramatically.  If you are going to prevent all ability to reread a
> page - this should probably be optional.   This affects cases even
> when there are no 2nd clients opening the same file and cases where a
> 2nd client opened a file but never wrote to it.  Note that the common
> case of multiply open files from the same client will break oplock too
> even though there is no caching issue there (much as we would like to
> change the protocol for that - we have to wait until SMB2 until we can
> reaquire or upgrade oplocks).
> 
> Currently we write through all writes when we don't have oplock.   We
> should also invalidate any cached pages when opening a non-cached file
> that has changed since we last opened it - so we will reread these
> pages although they will stay in the page cache.   We should
> invalidate these cached pages whenever mtime changes (even if some
> Windows server defer mtime updates - it will be correct file close or
> sync - which is the only place we have guarantees that data is written
> anyway).  We check mtime updates in revalidate (and this can be
> configured in /proc/fs/cifs to always be sent - rather than every
> second).
> 
> 

I think, before any of this goes in, someone (Pavel?) needs to step back,
and write up (in English) what the caching semantics should be when we
have oplocks of various levels, including none at all. The
documentation could be fairly informal -- maybe just a few paragraphs
in the README, but it needs to be written before we can do any serious
discussion of patches.

As Christoph pointed out, async I/O isn't covered by that patch. We
also need to settle on semantics for mmap. The patches are fine as a
point of reference for discussion, but please don't merge any of this
until we have a clear, consistent goal in mind.

That'll also help us to ensure that we don't cause regressions, and to
make sure that we understand what the performance impacts of these
changes will be.

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