Re: [PATCH] Fix page reading error with mandatory locking style

On Mon, 5 Apr 2010 10:01:19 +0400
[email protected] wrote:

> From: Pavel Shilovsky <[email protected]>
> 
> [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 <[email protected]>
> ---
>  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?

> +	} 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,


-- 
Jeff Layton <[email protected]>