[linux-cifs-client] [PATCH] Fix page reading error with mandatory locking style
Pavel Shilovsky
piastry at etersoft.ru
Mon Apr 5 14:09:31 MDT 2010
В сообщении от 5 апреля 2010 16:20:54 вы написали:
> On Mon, 5 Apr 2010 10:01:19 +0400
>
> piastry at etersoft.ru wrote:
> > From: Pavel Shilovsky <piastryyy at gmail.com>
> >
> > [CIFS] Fix page reading error with mandatory locking style
> >
> > If we lock file from one process from 1 to 2 and then try to read it
> > from another from 0 to 1 without direct mount options, reading fails.
> > That's why vfs tries to read whole page and fails due the lock from
> > the first process.
> >
> > Signed-off-by: Pavel Shilovsky <piastryyy at gmail.com>
> > ---
> >
> > fs/cifs/cifsfs.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
> > 1 files changed, 46 insertions(+), 1 deletions(-)
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index ded66be..94a3b1f 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -616,6 +616,51 @@ 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)
> > +{
> > + int retval, read, posix_locking = 0;
> > + struct file_lock pfLock;
> > + struct cifsInodeInfo *cifsInode;
> > + struct cifs_sb_info *cifs_sb;
> > + struct cifsTconInfo *tcon;
> > +
> > + cifs_sb = CIFS_SB(filp->f_path.dentry->d_sb);
> > + tcon = cifs_sb->tcon;
> > + if ((tcon->ses->capabilities & CAP_UNIX) &&
> > + (CIFS_UNIX_FCNTL_CAP & le64_to_cpu(tcon->fsUnixInfo.Capability)) &&
> > + ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOPOSIXBRL) == 0))
> > + posix_locking = 1;
> > +
> > + retval = cifs_revalidate_file(filp);
> > + if (retval < 0)
> > + return (ssize_t)retval;
> > +
>
> I'd prefer to see the above call in the aio_read function instead.
> Steve's not too crazy about the idea though as he thinks it'll hurt
> performance. My argument is that it doesn't matter how fast you get a
> return from the syscall if the answer is wrong. Shrug.
>
> > + memset(&pfLock, 0, sizeof(pfLock));
> > + pfLock.fl_type = F_RDLCK;
> > + pfLock.fl_start = *ppos;
> > + pfLock.fl_end = *ppos+len;
> > + cifsInode = CIFS_I(filp->f_path.dentry->d_inode);
> > + if (cifsInode == NULL)
> > + return -ENOENT;
> > +
> > + if (!CIFS_I(filp->f_path.dentry->d_inode)->clientCanCacheRead &&
> > + !posix_locking) {
> > + retval = cifs_lock(filp, F_GETLK, &pfLock);
> > + if (retval < 0)
> > + return (ssize_t)retval;
> > + if (pfLock.fl_type == F_UNLCK)
> > + read = do_sync_read(filp, buf, len, ppos);
> > + else
> > + return -EACCES;
>
> This looks really inefficient. A read call on the wire has suddenly
> turned into multiple lock/unlock calls and then a read. Maybe it would
> be better to attempt the read first and then just fall back to the slow
> path if that fails with an error that indicates that part of that page
> is locked?
That won't work. This is the situation:
1. The first process has valid pages.
2. The second process sets a lock from 1 to 2.
3. The first process try to read from 1 to 2:
It reads page and call returns successfully because client dosn't read from server - it has valid cache pages! That's wrong!
That's why we have to check if there are no locks preventing us and then we can read from cache.
>
> > + } else
> > + read = do_sync_read(filp, buf, len, ppos);
> > +
> > + if (read == -EACCES && !posix_locking)
> > + 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)
> >
> > {
> >
> > @@ -737,7 +782,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,
--
Best regards,
Pavel Shilovsky.
More information about the linux-cifs-client
mailing list