| Message ID | 200904010009.17556.piastry@etersoft.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 1 Apr 2009 00:09:17 +0400 Pavel Shilovsky <piastry@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. > + > 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, >
On Tue, 31 Mar 2009 17:16:20 -0400 Jeff Layton <jlayton@redhat.com> wrote: > On Wed, 1 Apr 2009 00:09:17 +0400 > Pavel Shilovsky <piastry@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, > > > >
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; +} + + 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,