From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff Layton Subject: Re: [PATCH] CIFS: New read cache mechanism Date: Sat, 18 Sep 2010 07:32:14 -0400 Message-ID: <20100918073214.4104b01e@corrin.poochiereds.net> References: Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Pavel Shilovsky Return-path: In-Reply-To: Sender: linux-cifs-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: On Sat, 18 Sep 2010 10:32:03 +0400 Pavel Shilovsky 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 > --- > 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