[linux-cifs-client] [PATCH] non-direct reading

[Jeff Layton]

On Tue, 31 Mar 2009 17:16:20 -0400
Jeff Layton <jlayton at redhat.com> wrote:

> On Wed, 1 Apr 2009 00:09:17 +0400
> Pavel Shilovsky <piastry at etersoft.ru> wrote:
> 
> > I found bug in my patch, Here is right variant.
> > 
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 3ae62fb..6525661 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -622,6 +622,23 @@ static ssize_t cifs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
> >         return written;
> >  }
> > 
> > +static ssize_t cifs_file_aio_read(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;
> > +       ssize_t read;
> > +
> > +       if (CIFS_I(inode)->clientCanCacheRead)
> > +               read = generic_file_aio_read(iocb, iov, nr_segs, pos);
> > +       else {
> > +               read = cifs_user_read(iocb->ki_filp, iov->iov_base,
> > +                               iov->iov_len, &pos);
> > +               iocb->ki_pos = pos;
> > +       }
> > +       return read;
> > +}
> > +
> 
> I don't think this is what you want at all. Won't this make it so that
> all reads will hit the server unless we have an oplock?
> 
> If you're having problems with cache consistency, then what you really
> want to do is to fix it so that the inode is properly revalidated and
> then invalidate the cache only if it looks like the file has changed.
> 

Actually...you may be correct here.

I had a long talk with JRA about this while we were at SambaXP. The
problem with trying to only invalidate the cache when we detect that
the file has changed is that we can't necessarily detect when a file
has changed on the server. Apparently windows will allow someone to to
change the LastWriteTime on the file, and then subsequent writes to
that fd won't change any metadata (ugh!).

I think you may be correct. We should flip to unbuffered reads
and writes when we don't have an oplock. That'll really make
performance suck on files that are open on multiple clients though.

Because of that, we probably need to consider having the client
occasionally try to reclaim the oplock (via closing and reopening the
file). We'll have to come up with some heuristic for that, and we won't
be able to do it if the client is holding a file lock (since it might
be lost).

> > +
> >  static loff_t cifs_llseek(struct file *file, loff_t offset, int origin)
> >  {
> >         /* origin == SEEK_END => we must revalidate the cached file length */
> > @@ -733,7 +750,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,
> > 
> 
> 


-- 
Jeff Layton <jlayton at redhat.com>