[PATCH 1/6] CIFS: Make cifsFileInfo_put work with strict cache mode

[PATCH 1/6] CIFS: Make cifsFileInfo_put work with strict cache mode

[Pavel Shilovsky]

On strict cache mode when we close the last file handle of the inode we
should set invalid_mapping flag on this inode to prevent data coherency
problem when we open it again but it has been modified by other clients.

Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifs_fs_sb.h |    1 +
 fs/cifs/file.c       |    8 ++++++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
index 7852cd6..ac51cd2 100644
--- a/fs/cifs/cifs_fs_sb.h
+++ b/fs/cifs/cifs_fs_sb.h
@@ -40,6 +40,7 @@
 #define CIFS_MOUNT_FSCACHE	0x8000 /* local caching enabled */
 #define CIFS_MOUNT_MF_SYMLINKS	0x10000 /* Minshall+French Symlinks enabled */
 #define CIFS_MOUNT_MULTIUSER	0x20000 /* multiuser mount */
+#define CIFS_MOUNT_STRICT_IO	0x40000 /* strict cache mode */
 
 struct cifs_sb_info {
 	struct rb_root tlink_tree;
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 5a28660..60c3f84 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -264,6 +264,7 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 	struct inode *inode = cifs_file->dentry->d_inode;
 	struct cifsTconInfo *tcon = tlink_tcon(cifs_file->tlink);
 	struct cifsInodeInfo *cifsi = CIFS_I(inode);
+	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
 	struct cifsLockInfo *li, *tmp;
 
 	spin_lock(&cifs_file_list_lock);
@@ -279,6 +280,13 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 	if (list_empty(&cifsi->openFileList)) {
 		cFYI(1, "closing last open instance for inode %p",
 			cifs_file->dentry->d_inode);
+
+		/* in strict cache mode we need invalidate mapping on the last
+		   close  because it may cause a error when we open this file
+		   again and get at least level II oplock */
+		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO)
+			CIFS_I(inode)->invalid_mapping = true;
+
 		cifs_set_oplock_level(cifsi, 0);
 	}
 	spin_unlock(&cifs_file_list_lock);
-- 
1.7.3.2

[PATCH 2/6] CIFS: Implement cifs_strict_fsync

[Pavel Shilovsky]

Invalidate inode mapping if we don't have at least Level II oplock in
cifs_strict_fsync. Also remove filemap_write_and_wait call from cifs_fsync
because it is previously called from vfs_fsync_range. Add file operations'
structures for strict cache mode.

Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/cifs/cifsfs.c |   38 ++++++++++++++++++++++++++++++++++++++
 fs/cifs/cifsfs.h |    8 ++++++--
 fs/cifs/file.c   |   36 ++++++++++++++++++++++++++++--------
 fs/cifs/inode.c  |    8 ++++++--
 4 files changed, 78 insertions(+), 12 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 3936aa7..7660133 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -710,6 +710,25 @@ const struct file_operations cifs_file_ops = {
 	.setlease = cifs_setlease,
 };
 
+const struct file_operations cifs_file_strict_ops = {
+	.read = do_sync_read,
+	.write = do_sync_write,
+	.aio_read = generic_file_aio_read,
+	.aio_write = cifs_file_aio_write,
+	.open = cifs_open,
+	.release = cifs_close,
+	.lock = cifs_lock,
+	.fsync = cifs_strict_fsync,
+	.flush = cifs_flush,
+	.mmap = cifs_file_mmap,
+	.splice_read = generic_file_splice_read,
+	.llseek = cifs_llseek,
+#ifdef CONFIG_CIFS_POSIX
+	.unlocked_ioctl	= cifs_ioctl,
+#endif /* CONFIG_CIFS_POSIX */
+	.setlease = cifs_setlease,
+};
+
 const struct file_operations cifs_file_direct_ops = {
 	/* no aio, no readv -
 	   BB reevaluate whether they can be done with directio, no cache */
@@ -728,6 +747,7 @@ const struct file_operations cifs_file_direct_ops = {
 	.llseek = cifs_llseek,
 	.setlease = cifs_setlease,
 };
+
 const struct file_operations cifs_file_nobrl_ops = {
 	.read = do_sync_read,
 	.write = do_sync_write,
@@ -746,6 +766,24 @@ const struct file_operations cifs_file_nobrl_ops = {
 	.setlease = cifs_setlease,
 };
 
+const struct file_operations cifs_file_strict_nobrl_ops = {
+	.read = do_sync_read,
+	.write = do_sync_write,
+	.aio_read = generic_file_aio_read,
+	.aio_write = cifs_file_aio_write,
+	.open = cifs_open,
+	.release = cifs_close,
+	.fsync = cifs_strict_fsync,
+	.flush = cifs_flush,
+	.mmap = cifs_file_mmap,
+	.splice_read = generic_file_splice_read,
+	.llseek = cifs_llseek,
+#ifdef CONFIG_CIFS_POSIX
+	.unlocked_ioctl	= cifs_ioctl,
+#endif /* CONFIG_CIFS_POSIX */
+	.setlease = cifs_setlease,
+};
+
 const struct file_operations cifs_file_direct_nobrl_ops = {
 	/* no mmap, no aio, no readv -
 	   BB reevaluate whether they can be done with directio, no cache */
diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index 897b2b2..8584829 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -61,6 +61,7 @@ extern int cifs_rename(struct inode *, struct dentry *, struct inode *,
 		       struct dentry *);
 extern int cifs_revalidate_file(struct file *filp);
 extern int cifs_revalidate_dentry(struct dentry *);
+extern void cifs_invalidate_mapping(struct inode *inode);
 extern int cifs_getattr(struct vfsmount *, struct dentry *, struct kstat *);
 extern int cifs_setattr(struct dentry *, struct iattr *);
 
@@ -72,8 +73,10 @@ extern const struct inode_operations cifs_dfs_referral_inode_operations;
 /* Functions related to files and directories */
 extern const struct file_operations cifs_file_ops;
 extern const struct file_operations cifs_file_direct_ops; /* if directio mnt */
-extern const struct file_operations cifs_file_nobrl_ops;
-extern const struct file_operations cifs_file_direct_nobrl_ops; /* no brlocks */
+extern const struct file_operations cifs_file_strict_ops; /* if strictio mnt */
+extern const struct file_operations cifs_file_nobrl_ops; /* no brlocks */
+extern const struct file_operations cifs_file_direct_nobrl_ops;
+extern const struct file_operations cifs_file_strict_nobrl_ops;
 extern int cifs_open(struct inode *inode, struct file *file);
 extern int cifs_close(struct inode *inode, struct file *file);
 extern int cifs_closedir(struct inode *inode, struct file *file);
@@ -83,6 +86,7 @@ extern ssize_t cifs_user_write(struct file *file, const char __user *write_data,
 			 size_t write_size, loff_t *poffset);
 extern int cifs_lock(struct file *, int, struct file_lock *);
 extern int cifs_fsync(struct file *, int);
+extern int cifs_strict_fsync(struct file *, int);
 extern int cifs_flush(struct file *, fl_owner_t id);
 extern int cifs_file_mmap(struct file * , struct vm_area_struct *);
 extern const struct file_operations cifs_dir_ops;
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 60c3f84..4e37cb8 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -1586,27 +1586,47 @@ static int cifs_write_end(struct file *file, struct address_space *mapping,
 	return rc;
 }
 
-int cifs_fsync(struct file *file, int datasync)
+int cifs_strict_fsync(struct file *file, int datasync)
 {
 	int xid;
 	int rc = 0;
 	struct cifsTconInfo *tcon;
 	struct cifsFileInfo *smbfile = file->private_data;
 	struct inode *inode = file->f_path.dentry->d_inode;
+	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
 
 	xid = GetXid();
 
 	cFYI(1, "Sync file - name: %s datasync: 0x%x",
 		file->f_path.dentry->d_name.name, datasync);
 
-	rc = filemap_write_and_wait(inode->i_mapping);
-	if (rc == 0) {
-		struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
+	if (!CIFS_I(inode)->clientCanCacheRead)
+		cifs_invalidate_mapping(inode);
 
-		tcon = tlink_tcon(smbfile->tlink);
-		if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
-			rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
-	}
+	tcon = tlink_tcon(smbfile->tlink);
+	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
+		rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
+
+	FreeXid(xid);
+	return rc;
+}
+
+int cifs_fsync(struct file *file, int datasync)
+{
+	int xid;
+	int rc = 0;
+	struct cifsTconInfo *tcon;
+	struct cifsFileInfo *smbfile = file->private_data;
+	struct cifs_sb_info *cifs_sb = CIFS_SB(file->f_path.dentry->d_sb);
+
+	xid = GetXid();
+
+	cFYI(1, "Sync file - name: %s datasync: 0x%x",
+		file->f_path.dentry->d_name.name, datasync);
+
+	tcon = tlink_tcon(smbfile->tlink);
+	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
+		rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
 
 	FreeXid(xid);
 	return rc;
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 589f3e3..a8f7cf3 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -44,13 +44,17 @@ static void cifs_set_ops(struct inode *inode, const bool is_dfs_referral)
 				inode->i_fop = &cifs_file_direct_nobrl_ops;
 			else
 				inode->i_fop = &cifs_file_direct_ops;
+		} else if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) {
+			if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
+				inode->i_fop = &cifs_file_strict_nobrl_ops;
+			else
+				inode->i_fop = &cifs_file_strict_ops;
 		} else if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
 			inode->i_fop = &cifs_file_nobrl_ops;
 		else { /* not direct, send byte range locks */
 			inode->i_fop = &cifs_file_ops;
 		}
 
-
 		/* check if server can support readpages */
 		if (cifs_sb_master_tcon(cifs_sb)->ses->server->maxBuf <
 				PAGE_CACHE_SIZE + MAX_CIFS_HDR_SIZE)
@@ -1679,7 +1683,7 @@ cifs_inode_needs_reval(struct inode *inode)
 /*
  * Zap the cache. Called when invalid_mapping flag is set.
  */
-static void
+void
 cifs_invalidate_mapping(struct inode *inode)
 {
 	int rc;
-- 
1.7.3.2

[PATCH 3/6] CIFS: Implement cifs_file_strict_mmap

[Pavel Shilovsky]

Invalidate inode mapping if we don't have at least Level II oplock.

Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/cifs/cifsfs.c |    4 ++--
 fs/cifs/cifsfs.h |    1 +
 fs/cifs/file.c   |   17 +++++++++++++++++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 7660133..a218afe 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -720,7 +720,7 @@ const struct file_operations cifs_file_strict_ops = {
 	.lock = cifs_lock,
 	.fsync = cifs_strict_fsync,
 	.flush = cifs_flush,
-	.mmap = cifs_file_mmap,
+	.mmap = cifs_file_strict_mmap,
 	.splice_read = generic_file_splice_read,
 	.llseek = cifs_llseek,
 #ifdef CONFIG_CIFS_POSIX
@@ -775,7 +775,7 @@ const struct file_operations cifs_file_strict_nobrl_ops = {
 	.release = cifs_close,
 	.fsync = cifs_strict_fsync,
 	.flush = cifs_flush,
-	.mmap = cifs_file_mmap,
+	.mmap = cifs_file_strict_mmap,
 	.splice_read = generic_file_splice_read,
 	.llseek = cifs_llseek,
 #ifdef CONFIG_CIFS_POSIX
diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index 8584829..657738f 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -89,6 +89,7 @@ extern int cifs_fsync(struct file *, int);
 extern int cifs_strict_fsync(struct file *, int);
 extern int cifs_flush(struct file *, fl_owner_t id);
 extern int cifs_file_mmap(struct file * , struct vm_area_struct *);
+extern int cifs_file_strict_mmap(struct file * , struct vm_area_struct *);
 extern const struct file_operations cifs_dir_ops;
 extern int cifs_dir_open(struct inode *inode, struct file *file);
 extern int cifs_readdir(struct file *file, void *direntry, filldir_t filldir);
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 4e37cb8..dd1e59a 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -1827,17 +1827,34 @@ static ssize_t cifs_read(struct file *file, char *read_data, size_t read_size,
 	return total_read;
 }
 
+int cifs_file_strict_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	int rc, xid;
+	struct inode *inode = file->f_path.dentry->d_inode;
+
+	xid = GetXid();
+
+	if (!CIFS_I(inode)->clientCanCacheRead)
+		cifs_invalidate_mapping(inode);
+
+	rc = generic_file_mmap(file, vma);
+	FreeXid(xid);
+	return rc;
+}
+
 int cifs_file_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	int rc, xid;
 
 	xid = GetXid();
+
 	rc = cifs_revalidate_file(file);
 	if (rc) {
 		cFYI(1, "Validation prior to mmap failed, error=%d", rc);
 		FreeXid(xid);
 		return rc;
 	}
+
 	rc = generic_file_mmap(file, vma);
 	FreeXid(xid);
 	return rc;
-- 
1.7.3.2

[PATCH 4/6] CIFS: Implement cifs_strict_write

[Pavel Shilovsky]

If we don't have Exclusive oplock we write a data to the server through
cifs_write. Also set invalidate_mapping flag on the inode if we wrote
something to the server.

Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/cifs/cifsfs.c    |   74 ++++++++++++++++++++++++++++++++++++++++++++++++---
 fs/cifs/cifsproto.h |    3 ++
 fs/cifs/file.c      |    5 +--
 3 files changed, 75 insertions(+), 7 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index a218afe..5df0503 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -572,15 +572,81 @@ cifs_do_mount(struct file_system_type *fs_type,
 	return dget(sb->s_root);
 }
 
+static ssize_t cifs_strict_write(struct kiocb *iocb, const struct iovec *iov,
+				 unsigned long nr_segs, loff_t pos)
+{
+	struct inode *inode;
+	struct cifs_sb_info *cifs_sb;
+	ssize_t written = 0;
+	unsigned long i, len = 0;
+	char *buf, *offset;
+
+	inode = iocb->ki_filp->f_path.dentry->d_inode;
+
+	if (CIFS_I(inode)->clientCanCacheAll)
+		return generic_file_aio_write(iocb, iov, nr_segs, pos);
+
+	cifs_sb = CIFS_SB(iocb->ki_filp->f_path.dentry->d_sb);
+
+	/*
+	 * In strict cache mode we need to write the data to the server exactly
+	 * from the pos to pos+len-1 rather than flush all affected pages
+	 * because it may cause a error with mandatory locks on these pages but
+	 * not on the region from pos to ppos+len-1.
+	 */
+
+	for (i = 0; i < nr_segs; i++)
+		len += iov[i].iov_len;
+
+	/*
+	 * BB - optimize the way when signing is disabled. We can drop this
+	 * extra memory-to-memory copying and use iovec buffers for constructing
+	 * write request.
+	 */
+
+	buf = kmalloc(len, GFP_KERNEL);
+
+	for (i = 0, offset = buf; i < nr_segs; i++) {
+		if (copy_from_user(offset, iov[i].iov_base, iov[i].iov_len)) {
+			kfree(buf);
+			return -EFAULT;
+		}
+
+		offset += iov[i].iov_len;
+	}
+
+	written = cifs_write((struct cifsFileInfo *)iocb->ki_filp->private_data,
+			     buf, len, &pos);
+	if (written < 0) {
+		kfree(buf);
+		return written;
+	}
+
+	iocb->ki_pos = pos;
+
+	if (written > 0)
+		CIFS_I(inode)->invalid_mapping = true;
+
+	kfree(buf);
+	return written;
+}
+
 static ssize_t cifs_file_aio_write(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 written;
+	int rc;
 
 	written = generic_file_aio_write(iocb, iov, nr_segs, pos);
-	if (!CIFS_I(inode)->clientCanCacheAll)
-		filemap_fdatawrite(inode->i_mapping);
+
+	if (CIFS_I(inode)->clientCanCacheAll)
+		return written;
+
+	rc = filemap_fdatawrite(inode->i_mapping);
+	if (rc)
+		cFYI(1, "cifs_file_aio_write: %d rc on %p inode", rc, inode);
+
 	return written;
 }
 
@@ -714,7 +780,7 @@ const struct file_operations cifs_file_strict_ops = {
 	.read = do_sync_read,
 	.write = do_sync_write,
 	.aio_read = generic_file_aio_read,
-	.aio_write = cifs_file_aio_write,
+	.aio_write = cifs_strict_write,
 	.open = cifs_open,
 	.release = cifs_close,
 	.lock = cifs_lock,
@@ -770,7 +836,7 @@ const struct file_operations cifs_file_strict_nobrl_ops = {
 	.read = do_sync_read,
 	.write = do_sync_write,
 	.aio_read = generic_file_aio_read,
-	.aio_write = cifs_file_aio_write,
+	.aio_write = cifs_strict_write,
 	.open = cifs_open,
 	.release = cifs_close,
 	.fsync = cifs_strict_fsync,
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index e6d1481..3a337a9 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -79,6 +79,9 @@ extern int checkSMB(struct smb_hdr *smb, __u16 mid, unsigned int length);
 extern bool is_valid_oplock_break(struct smb_hdr *smb,
 				  struct TCP_Server_Info *);
 extern bool is_size_safe_to_change(struct cifsInodeInfo *, __u64 eof);
+extern ssize_t cifs_write(struct cifsFileInfo *open_file,
+			  const char *write_data, size_t write_size,
+			  loff_t *poffset);
 extern struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *, bool);
 extern struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *, bool);
 extern unsigned int smbCalcSize(struct smb_hdr *ptr);
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index dd1e59a..2c848ef 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -1026,9 +1026,8 @@ ssize_t cifs_user_write(struct file *file, const char __user *write_data,
 	return total_written;
 }
 
-static ssize_t cifs_write(struct cifsFileInfo *open_file,
-			  const char *write_data, size_t write_size,
-			  loff_t *poffset)
+ssize_t cifs_write(struct cifsFileInfo *open_file, const char *write_data,
+		   size_t write_size, loff_t *poffset)
 {
 	int rc = 0;
 	unsigned int bytes_written = 0;
-- 
1.7.3.2

[PATCH 5/6] CIFS: Implement cifs_strict_read

[Pavel Shilovsky]

Read from the cache if we have at least Level II oplock - otherwise
read from the server. Add cifs_user_readv to let the client read into
several buffers.

Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/cifs/cifsfs.c |   43 +++++++++++++++++++++++++++++-
 fs/cifs/cifsfs.h |    5 +++-
 fs/cifs/file.c   |   76 ++++++++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 111 insertions(+), 13 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 5df0503..a47b8af 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -572,6 +572,45 @@ cifs_do_mount(struct file_system_type *fs_type,
 	return dget(sb->s_root);
 }
 
+static ssize_t cifs_strict_read(struct kiocb *iocb, const struct iovec *iov,
+				unsigned long nr_segs, loff_t pos)
+{
+	struct inode *inode;
+	struct cifs_sb_info *cifs_sb;
+	ssize_t read = 0;
+	unsigned long i, len = 0;
+
+	if (!nr_segs)
+		return read;
+
+	inode = iocb->ki_filp->f_path.dentry->d_inode;
+	cifs_sb = CIFS_SB(iocb->ki_filp->f_path.dentry->d_sb);
+
+	if (CIFS_I(inode)->clientCanCacheRead)
+		return generic_file_aio_read(iocb, iov, nr_segs, pos);
+
+	/*
+	 * In strict cache mode we need to read from the server all the time
+	 * if we don't have level II oplock because the server can delay mtime
+	 * change - so we can't make a decision about inode invalidating.
+	 * And we can also fail with pagereading if there are mandatory locks
+	 * on pages affected by this read but not on the region from pos to
+	 * pos+len-1.
+	 */
+
+	for (i = 0; i < nr_segs; i++)
+		len += iov[i].iov_len;
+
+
+	read = cifs_user_readv(iocb->ki_filp, iov, nr_segs, len, &pos);
+	if (read < 0)
+		return read;
+
+	iocb->ki_pos = pos;
+
+	return read;
+}
+
 static ssize_t cifs_strict_write(struct kiocb *iocb, const struct iovec *iov,
 				 unsigned long nr_segs, loff_t pos)
 {
@@ -779,7 +818,7 @@ const struct file_operations cifs_file_ops = {
 const struct file_operations cifs_file_strict_ops = {
 	.read = do_sync_read,
 	.write = do_sync_write,
-	.aio_read = generic_file_aio_read,
+	.aio_read = cifs_strict_read,
 	.aio_write = cifs_strict_write,
 	.open = cifs_open,
 	.release = cifs_close,
@@ -835,7 +874,7 @@ const struct file_operations cifs_file_nobrl_ops = {
 const struct file_operations cifs_file_strict_nobrl_ops = {
 	.read = do_sync_read,
 	.write = do_sync_write,
-	.aio_read = generic_file_aio_read,
+	.aio_read = cifs_strict_read,
 	.aio_write = cifs_strict_write,
 	.open = cifs_open,
 	.release = cifs_close,
diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index 657738f..a6bea06 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -81,7 +81,10 @@ extern int cifs_open(struct inode *inode, struct file *file);
 extern int cifs_close(struct inode *inode, struct file *file);
 extern int cifs_closedir(struct inode *inode, struct file *file);
 extern ssize_t cifs_user_read(struct file *file, char __user *read_data,
-			 size_t read_size, loff_t *poffset);
+			      size_t read_size, loff_t *poffset);
+extern ssize_t cifs_user_readv(struct file *file, const struct iovec *iov,
+			       unsigned long nr_segs, size_t len,
+			       loff_t *poffset);
 extern ssize_t cifs_user_write(struct file *file, const char __user *write_data,
 			 size_t write_size, loff_t *poffset);
 extern int cifs_lock(struct file *, int, struct file_lock *);
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 2c848ef..9238a2f 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -1676,8 +1676,40 @@ int cifs_flush(struct file *file, fl_owner_t id)
 	return rc;
 }
 
-ssize_t cifs_user_read(struct file *file, char __user *read_data,
-	size_t read_size, loff_t *poffset)
+static int
+cifs_fill_iov(const struct iovec *iov, unsigned long nr_segs,
+	      unsigned long *last_len, unsigned long *last_ind, char *data,
+	      size_t datalen)
+{
+	const struct iovec *ciov;
+	unsigned long cur_len;
+
+	while (datalen) {
+		cur_len = min_t(unsigned long, datalen,
+					      *last_len);
+		ciov = &iov[*last_ind];
+
+		if (copy_to_user(ciov->iov_base + (ciov->iov_len - *last_len),
+				 data, cur_len))
+			return -EFAULT;
+
+		datalen -= cur_len;
+		data += cur_len;
+
+		*last_len -= cur_len;
+		if (*last_len == 0) {
+			(*last_ind)++;
+			*last_len = iov[*last_ind].iov_len;
+		}
+	}
+
+	return 0;
+}
+
+static ssize_t
+cifs_generic_user_read(struct file *file, char __user *read_data,
+		       size_t read_size, loff_t *poffset,
+		       const struct iovec *iov, unsigned long nr_segs)
 {
 	int rc = -EACCES;
 	unsigned int bytes_read = 0;
@@ -1690,6 +1722,11 @@ ssize_t cifs_user_read(struct file *file, char __user *read_data,
 	char *smb_read_data;
 	char __user *current_offset;
 	struct smb_com_read_rsp *pSMBr;
+	unsigned long last_len = 0, last_ind = 0;
+
+	/* if vectored - set last_len value */
+	if (!read_data && nr_segs > 0)
+		last_len = iov->iov_len;
 
 	xid = GetXid();
 	cifs_sb = CIFS_SB(file->f_path.dentry->d_sb);
@@ -1707,7 +1744,8 @@ ssize_t cifs_user_read(struct file *file, char __user *read_data,
 
 	for (total_read = 0, current_offset = read_data;
 	     read_size > total_read;
-	     total_read += bytes_read, current_offset += bytes_read) {
+	     total_read += bytes_read,
+	     current_offset += read_data ? bytes_read : 0) {
 		current_read_size = min_t(const int, read_size - total_read,
 					  cifs_sb->rsize);
 		rc = -EAGAIN;
@@ -1726,12 +1764,18 @@ ssize_t cifs_user_read(struct file *file, char __user *read_data,
 					 &buf_type);
 			pSMBr = (struct smb_com_read_rsp *)smb_read_data;
 			if (smb_read_data) {
-				if (copy_to_user(current_offset,
-						smb_read_data +
-						4 /* RFC1001 length field */ +
-						le16_to_cpu(pSMBr->DataOffset),
-						bytes_read))
-					rc = -EFAULT;
+				char *data_offset = smb_read_data + 4 +
+						le16_to_cpu(pSMBr->DataOffset);
+				if (read_data) {
+					if (copy_to_user(current_offset,
+						data_offset, bytes_read))
+						rc = -EFAULT;
+				} else {
+					if (cifs_fill_iov(iov, nr_segs,
+						&last_len, &last_ind,
+						data_offset, bytes_read))
+						rc = -EFAULT;
+				}
 
 				if (buf_type == CIFS_SMALL_BUFFER)
 					cifs_small_buf_release(smb_read_data);
@@ -1756,9 +1800,21 @@ ssize_t cifs_user_read(struct file *file, char __user *read_data,
 	return total_read;
 }
 
+ssize_t cifs_user_read(struct file *file, char __user *read_data,
+		       size_t read_size, loff_t *poffset)
+{
+	return cifs_generic_user_read(file, read_data, read_size, poffset, NULL,
+				      0);
+}
+
+ssize_t cifs_user_readv(struct file *file, const struct iovec *iov,
+			unsigned long nr_segs, size_t len, loff_t *poffset)
+{
+	return cifs_generic_user_read(file, NULL, len, poffset, iov, nr_segs);
+}
 
 static ssize_t cifs_read(struct file *file, char *read_data, size_t read_size,
-	loff_t *poffset)
+			 loff_t *poffset)
 {
 	int rc = -EACCES;
 	unsigned int bytes_read = 0;
-- 
1.7.3.2

[PATCH 6/6] CIFS: Add strictcache mount option

[Pavel Shilovsky]

Use for switching on strict cache mode. In this mode the
client reads from the cache all the time it has Oplock Level II,
otherwise - read from the server. As for write - the client stores
a data in the cache in Exclusive Oplock case, otherwise - write
directly to the server.

Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/cifs/README    |    5 +++++
 fs/cifs/connect.c |    5 +++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/README b/fs/cifs/README
index 46af99a..fe16835 100644
--- a/fs/cifs/README
+++ b/fs/cifs/README
@@ -452,6 +452,11 @@ A partial list of the supported mount options follows:
 		if oplock (caching token) is granted and held. Note that
 		direct allows write operations larger than page size
 		to be sent to the server.
+  strictcache   Use for switching on strict cache mode. In this mode the
+		client read from the cache all the time it has Oplock Level II,
+		otherwise - read from the server. All written data are stored
+		in the cache, but if the client doesn't have Exclusive Oplock,
+		it writes the data to the server.
   acl   	Allow setfacl and getfacl to manage posix ACLs if server
 		supports them.  (default)
   noacl 	Do not allow setfacl and getfacl calls on this mount
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index cc1a860..877cb9e 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -84,6 +84,7 @@ struct smb_vol {
 	bool no_xattr:1;   /* set if xattr (EA) support should be disabled*/
 	bool server_ino:1; /* use inode numbers from server ie UniqueId */
 	bool direct_io:1;
+	bool strict_io:1; /* strict cache behavior */
 	bool remap:1;      /* set to remap seven reserved chars in filenames */
 	bool posix_paths:1; /* unset to not ask for posix pathnames. */
 	bool no_linux_ext:1;
@@ -1357,6 +1358,8 @@ cifs_parse_mount_options(char *options, const char *devname,
 			vol->direct_io = 1;
 		} else if (strnicmp(data, "forcedirectio", 13) == 0) {
 			vol->direct_io = 1;
+		} else if (strnicmp(data, "strictcache", 11) == 0) {
+			vol->strict_io = 1;
 		} else if (strnicmp(data, "noac", 4) == 0) {
 			printk(KERN_WARNING "CIFS: Mount option noac not "
 				"supported. Instead set "
@@ -2614,6 +2617,8 @@ static void setup_cifs_sb(struct smb_vol *pvolume_info,
 	if (pvolume_info->multiuser)
 		cifs_sb->mnt_cifs_flags |= (CIFS_MOUNT_MULTIUSER |
 					    CIFS_MOUNT_NO_PERM);
+	if (pvolume_info->strict_io)
+		cifs_sb->mnt_cifs_flags |= CIFS_MOUNT_STRICT_IO;
 	if (pvolume_info->direct_io) {
 		cFYI(1, "mounting share using direct i/o");
 		cifs_sb->mnt_cifs_flags |= CIFS_MOUNT_DIRECT_IO;
-- 
1.7.3.2

Re: [PATCH 4/6] CIFS: Implement cifs_strict_write

[Jeff Layton]

On Tue, 14 Dec 2010 14:10:58 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> If we don't have Exclusive oplock we write a data to the server through
> cifs_write. Also set invalidate_mapping flag on the inode if we wrote
> something to the server.
> 
> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/cifs/cifsfs.c    |   74 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  fs/cifs/cifsproto.h |    3 ++
>  fs/cifs/file.c      |    5 +--
>  3 files changed, 75 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index a218afe..5df0503 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -572,15 +572,81 @@ cifs_do_mount(struct file_system_type *fs_type,
>  	return dget(sb->s_root);
>  }
>  
> +static ssize_t cifs_strict_write(struct kiocb *iocb, const struct iovec *iov,
> +				 unsigned long nr_segs, loff_t pos)
> +{
> +	struct inode *inode;
> +	struct cifs_sb_info *cifs_sb;
> +	ssize_t written = 0;
> +	unsigned long i, len = 0;
> +	char *buf, *offset;
> +
> +	inode = iocb->ki_filp->f_path.dentry->d_inode;
> +
> +	if (CIFS_I(inode)->clientCanCacheAll)
> +		return generic_file_aio_write(iocb, iov, nr_segs, pos);
> +
> +	cifs_sb = CIFS_SB(iocb->ki_filp->f_path.dentry->d_sb);
> +
> +	/*
> +	 * In strict cache mode we need to write the data to the server exactly
> +	 * from the pos to pos+len-1 rather than flush all affected pages
> +	 * because it may cause a error with mandatory locks on these pages but
> +	 * not on the region from pos to ppos+len-1.
> +	 */
> +
> +	for (i = 0; i < nr_segs; i++)
> +		len += iov[i].iov_len;
> +
> +	/*
> +	 * BB - optimize the way when signing is disabled. We can drop this
> +	 * extra memory-to-memory copying and use iovec buffers for constructing
> +	 * write request.
> +	 */
> +
> +	buf = kmalloc(len, GFP_KERNEL);
> +

Actually...I thought about another problem here. Suppose an application
does a 100M write. This may fail in that case. It varies by the slab
allocator in use, but there are limits to how large you can kmalloc.
This is especially going to be a problem on 32-bit arches where
GFP_KERNEL memory is more limited.

There are a couple of different solutions, but I think the best one
would be to stitch together individual pages. Walk the iov array first
and figure out how many pages you're going to need, and kmalloc an iov
array that size. Then start allocating pages (preferably using
GFP_HIGHMEM and kmapping them) and then copy the data to them. Stop
when you get to the wsize limit, and issue a CIFSSMBWrite2 call. When
it returns, pick up where you left off and do it again.

> +	for (i = 0, offset = buf; i < nr_segs; i++) {
> +		if (copy_from_user(offset, iov[i].iov_base, iov[i].iov_len)) {
> +			kfree(buf);
> +			return -EFAULT;
> +		}
> +
> +		offset += iov[i].iov_len;
> +	}
> +
> +	written = cifs_write((struct cifsFileInfo *)iocb->ki_filp->private_data,
> +			     buf, len, &pos);
> +	if (written < 0) {
> +		kfree(buf);
> +		return written;
> +	}
> +
> +	iocb->ki_pos = pos;
> +
> +	if (written > 0)
> +		CIFS_I(inode)->invalid_mapping = true;
> +
> +	kfree(buf);
> +	return written;
> +}
> +
>  static ssize_t cifs_file_aio_write(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 written;
> +	int rc;
>  
>  	written = generic_file_aio_write(iocb, iov, nr_segs, pos);
> -	if (!CIFS_I(inode)->clientCanCacheAll)
> -		filemap_fdatawrite(inode->i_mapping);
> +
> +	if (CIFS_I(inode)->clientCanCacheAll)
> +		return written;
> +
> +	rc = filemap_fdatawrite(inode->i_mapping);
> +	if (rc)
> +		cFYI(1, "cifs_file_aio_write: %d rc on %p inode", rc, inode);
> +
>  	return written;
>  }
>  
> @@ -714,7 +780,7 @@ const struct file_operations cifs_file_strict_ops = {
>  	.read = do_sync_read,
>  	.write = do_sync_write,
>  	.aio_read = generic_file_aio_read,
> -	.aio_write = cifs_file_aio_write,
> +	.aio_write = cifs_strict_write,
>  	.open = cifs_open,
>  	.release = cifs_close,
>  	.lock = cifs_lock,
> @@ -770,7 +836,7 @@ const struct file_operations cifs_file_strict_nobrl_ops = {
>  	.read = do_sync_read,
>  	.write = do_sync_write,
>  	.aio_read = generic_file_aio_read,
> -	.aio_write = cifs_file_aio_write,
> +	.aio_write = cifs_strict_write,
>  	.open = cifs_open,
>  	.release = cifs_close,
>  	.fsync = cifs_strict_fsync,
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index e6d1481..3a337a9 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -79,6 +79,9 @@ extern int checkSMB(struct smb_hdr *smb, __u16 mid, unsigned int length);
>  extern bool is_valid_oplock_break(struct smb_hdr *smb,
>  				  struct TCP_Server_Info *);
>  extern bool is_size_safe_to_change(struct cifsInodeInfo *, __u64 eof);
> +extern ssize_t cifs_write(struct cifsFileInfo *open_file,
> +			  const char *write_data, size_t write_size,
> +			  loff_t *poffset);
>  extern struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *, bool);
>  extern struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *, bool);
>  extern unsigned int smbCalcSize(struct smb_hdr *ptr);
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index dd1e59a..2c848ef 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -1026,9 +1026,8 @@ ssize_t cifs_user_write(struct file *file, const char __user *write_data,
>  	return total_written;
>  }
>  
> -static ssize_t cifs_write(struct cifsFileInfo *open_file,
> -			  const char *write_data, size_t write_size,
> -			  loff_t *poffset)
> +ssize_t cifs_write(struct cifsFileInfo *open_file, const char *write_data,
> +		   size_t write_size, loff_t *poffset)
>  {
>  	int rc = 0;
>  	unsigned int bytes_written = 0;


-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 4/6] CIFS: Implement cifs_strict_write

[Pavel Shilovsky]

2010/12/14 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Tue, 14 Dec 2010 14:10:58 +0300
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> If we don't have Exclusive oplock we write a data to the server through
>> cifs_write. Also set invalidate_mapping flag on the inode if we wrote
>> something to the server.
>>
>> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  fs/cifs/cifsfs.c    |   74 ++++++++++++++++++++++++++++++++++++++++++++++++---
>>  fs/cifs/cifsproto.h |    3 ++
>>  fs/cifs/file.c      |    5 +--
>>  3 files changed, 75 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>> index a218afe..5df0503 100644
>> --- a/fs/cifs/cifsfs.c
>> +++ b/fs/cifs/cifsfs.c
>> @@ -572,15 +572,81 @@ cifs_do_mount(struct file_system_type *fs_type,
>>       return dget(sb->s_root);
>>  }
>>
>> +static ssize_t cifs_strict_write(struct kiocb *iocb, const struct iovec *iov,
>> +                              unsigned long nr_segs, loff_t pos)
>> +{
>> +     struct inode *inode;
>> +     struct cifs_sb_info *cifs_sb;
>> +     ssize_t written = 0;
>> +     unsigned long i, len = 0;
>> +     char *buf, *offset;
>> +
>> +     inode = iocb->ki_filp->f_path.dentry->d_inode;
>> +
>> +     if (CIFS_I(inode)->clientCanCacheAll)
>> +             return generic_file_aio_write(iocb, iov, nr_segs, pos);
>> +
>> +     cifs_sb = CIFS_SB(iocb->ki_filp->f_path.dentry->d_sb);
>> +
>> +     /*
>> +      * In strict cache mode we need to write the data to the server exactly
>> +      * from the pos to pos+len-1 rather than flush all affected pages
>> +      * because it may cause a error with mandatory locks on these pages but
>> +      * not on the region from pos to ppos+len-1.
>> +      */
>> +
>> +     for (i = 0; i < nr_segs; i++)
>> +             len += iov[i].iov_len;
>> +
>> +     /*
>> +      * BB - optimize the way when signing is disabled. We can drop this
>> +      * extra memory-to-memory copying and use iovec buffers for constructing
>> +      * write request.
>> +      */
>> +
>> +     buf = kmalloc(len, GFP_KERNEL);
>> +
>
> Actually...I thought about another problem here. Suppose an application
> does a 100M write. This may fail in that case. It varies by the slab
> allocator in use, but there are limits to how large you can kmalloc.
> This is especially going to be a problem on 32-bit arches where
> GFP_KERNEL memory is more limited.
>
> There are a couple of different solutions, but I think the best one
> would be to stitch together individual pages. Walk the iov array first
> and figure out how many pages you're going to need, and kmalloc an iov
> array that size. Then start allocating pages (preferably using
> GFP_HIGHMEM and kmapping them) and then copy the data to them. Stop
> when you get to the wsize limit, and issue a CIFSSMBWrite2 call. When
> it returns, pick up where you left off and do it again.

May be I should allocate buffer of min(wsize, write_len) size, fill it
with a data to write on cycle and issue a cifs_write with this buffer.

1) get write data len into 'len'
2) buf = kmalloc(min(wsize, len), GFP_KERNEL)
3) last_ind = 0, last_len = iov[0].iov_len
4) while (len > 0){
      cur_len = min(len, last_len)
      memcpy(buf, iov[last_ind].iov_base + iov[last_ind].iov_len -
last_len, cur_len)
      len -= cur_len
      last_len -= cur_len
      if (last_len == 0) {
        last_ind++
        last_len = iov[last_ind].iov_len
      }
      cifs_write(buf)
   }

So - it is like the way I do in read part (cifs_fill_iov). Thoughts?

>
>> +     for (i = 0, offset = buf; i < nr_segs; i++) {
>> +             if (copy_from_user(offset, iov[i].iov_base, iov[i].iov_len)) {
>> +                     kfree(buf);
>> +                     return -EFAULT;
>> +             }
>> +
>> +             offset += iov[i].iov_len;
>> +     }
>> +
>> +     written = cifs_write((struct cifsFileInfo *)iocb->ki_filp->private_data,
>> +                          buf, len, &pos);
>> +     if (written < 0) {
>> +             kfree(buf);
>> +             return written;
>> +     }
>> +
>> +     iocb->ki_pos = pos;
>> +
>> +     if (written > 0)
>> +             CIFS_I(inode)->invalid_mapping = true;
>> +
>> +     kfree(buf);
>> +     return written;
>> +}
>> +
>>  static ssize_t cifs_file_aio_write(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 written;
>> +     int rc;
>>
>>       written = generic_file_aio_write(iocb, iov, nr_segs, pos);
>> -     if (!CIFS_I(inode)->clientCanCacheAll)
>> -             filemap_fdatawrite(inode->i_mapping);
>> +
>> +     if (CIFS_I(inode)->clientCanCacheAll)
>> +             return written;
>> +
>> +     rc = filemap_fdatawrite(inode->i_mapping);
>> +     if (rc)
>> +             cFYI(1, "cifs_file_aio_write: %d rc on %p inode", rc, inode);
>> +
>>       return written;
>>  }
>>
>> @@ -714,7 +780,7 @@ const struct file_operations cifs_file_strict_ops = {
>>       .read = do_sync_read,
>>       .write = do_sync_write,
>>       .aio_read = generic_file_aio_read,
>> -     .aio_write = cifs_file_aio_write,
>> +     .aio_write = cifs_strict_write,
>>       .open = cifs_open,
>>       .release = cifs_close,
>>       .lock = cifs_lock,
>> @@ -770,7 +836,7 @@ const struct file_operations cifs_file_strict_nobrl_ops = {
>>       .read = do_sync_read,
>>       .write = do_sync_write,
>>       .aio_read = generic_file_aio_read,
>> -     .aio_write = cifs_file_aio_write,
>> +     .aio_write = cifs_strict_write,
>>       .open = cifs_open,
>>       .release = cifs_close,
>>       .fsync = cifs_strict_fsync,
>> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
>> index e6d1481..3a337a9 100644
>> --- a/fs/cifs/cifsproto.h
>> +++ b/fs/cifs/cifsproto.h
>> @@ -79,6 +79,9 @@ extern int checkSMB(struct smb_hdr *smb, __u16 mid, unsigned int length);
>>  extern bool is_valid_oplock_break(struct smb_hdr *smb,
>>                                 struct TCP_Server_Info *);
>>  extern bool is_size_safe_to_change(struct cifsInodeInfo *, __u64 eof);
>> +extern ssize_t cifs_write(struct cifsFileInfo *open_file,
>> +                       const char *write_data, size_t write_size,
>> +                       loff_t *poffset);
>>  extern struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *, bool);
>>  extern struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *, bool);
>>  extern unsigned int smbCalcSize(struct smb_hdr *ptr);
>> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>> index dd1e59a..2c848ef 100644
>> --- a/fs/cifs/file.c
>> +++ b/fs/cifs/file.c
>> @@ -1026,9 +1026,8 @@ ssize_t cifs_user_write(struct file *file, const char __user *write_data,
>>       return total_written;
>>  }
>>
>> -static ssize_t cifs_write(struct cifsFileInfo *open_file,
>> -                       const char *write_data, size_t write_size,
>> -                       loff_t *poffset)
>> +ssize_t cifs_write(struct cifsFileInfo *open_file, const char *write_data,
>> +                size_t write_size, loff_t *poffset)
>>  {
>>       int rc = 0;
>>       unsigned int bytes_written = 0;
>
>
> --
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>



-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 4/6] CIFS: Implement cifs_strict_write

[Jeff Layton]

On Tue, 14 Dec 2010 16:56:03 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2010/12/14 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> > On Tue, 14 Dec 2010 14:10:58 +0300
> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> > > If we don't have Exclusive oplock we write a data to the server through
> > > cifs_write. Also set invalidate_mapping flag on the inode if we wrote
> > > something to the server.
> > >
> > > Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > ---
> > >  fs/cifs/cifsfs.c    |   74
> > ++++++++++++++++++++++++++++++++++++++++++++++++---
> > >  fs/cifs/cifsproto.h |    3 ++
> > >  fs/cifs/file.c      |    5 +--
> > >  3 files changed, 75 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > > index a218afe..5df0503 100644
> > > --- a/fs/cifs/cifsfs.c
> > > +++ b/fs/cifs/cifsfs.c
> > > @@ -572,15 +572,81 @@ cifs_do_mount(struct file_system_type *fs_type,
> > >       return dget(sb->s_root);
> > >  }
> > >
> > > +static ssize_t cifs_strict_write(struct kiocb *iocb, const struct iovec
> > *iov,
> > > +                              unsigned long nr_segs, loff_t pos)
> > > +{
> > > +     struct inode *inode;
> > > +     struct cifs_sb_info *cifs_sb;
> > > +     ssize_t written = 0;
> > > +     unsigned long i, len = 0;
> > > +     char *buf, *offset;
> > > +
> > > +     inode = iocb->ki_filp->f_path.dentry->d_inode;
> > > +
> > > +     if (CIFS_I(inode)->clientCanCacheAll)
> > > +             return generic_file_aio_write(iocb, iov, nr_segs, pos);
> > > +
> > > +     cifs_sb = CIFS_SB(iocb->ki_filp->f_path.dentry->d_sb);
> > > +
> > > +     /*
> > > +      * In strict cache mode we need to write the data to the server
> > exactly
> > > +      * from the pos to pos+len-1 rather than flush all affected pages
> > > +      * because it may cause a error with mandatory locks on these pages
> > but
> > > +      * not on the region from pos to ppos+len-1.
> > > +      */
> > > +
> > > +     for (i = 0; i < nr_segs; i++)
> > > +             len += iov[i].iov_len;
> > > +
> > > +     /*
> > > +      * BB - optimize the way when signing is disabled. We can drop this
> > > +      * extra memory-to-memory copying and use iovec buffers for
> > constructing
> > > +      * write request.
> > > +      */
> > > +
> > > +     buf = kmalloc(len, GFP_KERNEL);
> > > +
> >
> > Actually...I thought about another problem here. Suppose an application
> > does a 100M write. This may fail in that case. It varies by the slab
> > allocator in use, but there are limits to how large you can kmalloc.
> > This is especially going to be a problem on 32-bit arches where
> > GFP_KERNEL memory is more limited.
> >
> > There are a couple of different solutions, but I think the best one
> > would be to stitch together individual pages. Walk the iov array first
> > and figure out how many pages you're going to need, and kmalloc an iov
> > array that size. Then start allocating pages (preferably using
> > GFP_HIGHMEM and kmapping them) and then copy the data to them. Stop
> > when you get to the wsize limit, and issue a CIFSSMBWrite2 call. When
> > it returns, pick up where you left off and do it again.
> >
> 
> May be I should allocate buffer of min(wsize, write_len) size, fill it with
> a data to write on cycle and issue a cifs_write with this buffer.
> 
> 1) get write data len into 'len'
> 2) buf = kmalloc(min(wsize, len), GFP_KERNEL)
> 3) last_ind = 0, last_len = iov[0].iov_len
> 4) while (len > 0){
>       cur_len = min(len, last_len)
>       memcpy(buf, iov[last_ind].iov_base + iov[last_ind].iov_len - last_len,
> cur_len)
>       len -= cur_len
>       last_len -= cur_len
>       if (last_len == 0) {
>         last_ind++
>         last_len = iov[last_ind].iov_len
>       }
>       cifs_write(buf)
>    }
> 
> So - it is like the way I do in read part (cifs_fill_iov). Thoughts?
> 

That will work, assuming that you can kmalloc a buffer that size. It's
less of a problem than it used to be, but when you kmalloc you're
asking for contiguous pages. In order to satisfy that, the VM may have
to flush other pages if there is memory pressure.

Large memory allocations in the write codepath are generally a bad
idea. Individual page allocations (particularly from GFP_HIGHMEM) are
almost always able to be satisfied without needing to do things like
flush other stuff out of pagecache.

Also by doing it the way I'm suggesting, you'll eliminate an unneeded
copy of the data too.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 5/6] CIFS: Implement cifs_strict_read

[Jeff Layton]

On Tue, 14 Dec 2010 14:10:59 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Read from the cache if we have at least Level II oplock - otherwise
> read from the server. Add cifs_user_readv to let the client read into
> several buffers.
> 
> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/cifs/cifsfs.c |   43 +++++++++++++++++++++++++++++-
>  fs/cifs/cifsfs.h |    5 +++-
>  fs/cifs/file.c   |   76 ++++++++++++++++++++++++++++++++++++++++++++++-------
>  3 files changed, 111 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 5df0503..a47b8af 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -572,6 +572,45 @@ cifs_do_mount(struct file_system_type *fs_type,
>  	return dget(sb->s_root);
>  }
>  
> +static ssize_t cifs_strict_read(struct kiocb *iocb, const struct iovec *iov,
> +				unsigned long nr_segs, loff_t pos)
> +{
> +	struct inode *inode;
> +	struct cifs_sb_info *cifs_sb;
> +	ssize_t read = 0;
> +	unsigned long i, len = 0;
> +
> +	if (!nr_segs)
> +		return read;
> +
> +	inode = iocb->ki_filp->f_path.dentry->d_inode;
> +	cifs_sb = CIFS_SB(iocb->ki_filp->f_path.dentry->d_sb);
> +
> +	if (CIFS_I(inode)->clientCanCacheRead)
> +		return generic_file_aio_read(iocb, iov, nr_segs, pos);
> +
> +	/*
> +	 * In strict cache mode we need to read from the server all the time
> +	 * if we don't have level II oplock because the server can delay mtime
> +	 * change - so we can't make a decision about inode invalidating.
> +	 * And we can also fail with pagereading if there are mandatory locks
> +	 * on pages affected by this read but not on the region from pos to
> +	 * pos+len-1.
> +	 */
> +
> +	for (i = 0; i < nr_segs; i++)
> +		len += iov[i].iov_len;
> +
> +
> +	read = cifs_user_readv(iocb->ki_filp, iov, nr_segs, len, &pos);
> +	if (read < 0)
> +		return read;
> +
> +	iocb->ki_pos = pos;
> +
> +	return read;
> +}
> +
>  static ssize_t cifs_strict_write(struct kiocb *iocb, const struct iovec *iov,
>  				 unsigned long nr_segs, loff_t pos)
>  {
> @@ -779,7 +818,7 @@ const struct file_operations cifs_file_ops = {
>  const struct file_operations cifs_file_strict_ops = {
>  	.read = do_sync_read,
>  	.write = do_sync_write,
> -	.aio_read = generic_file_aio_read,
> +	.aio_read = cifs_strict_read,
>  	.aio_write = cifs_strict_write,
>  	.open = cifs_open,
>  	.release = cifs_close,
> @@ -835,7 +874,7 @@ const struct file_operations cifs_file_nobrl_ops = {
>  const struct file_operations cifs_file_strict_nobrl_ops = {
>  	.read = do_sync_read,
>  	.write = do_sync_write,
> -	.aio_read = generic_file_aio_read,
> +	.aio_read = cifs_strict_read,
>  	.aio_write = cifs_strict_write,
>  	.open = cifs_open,
>  	.release = cifs_close,
> diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
> index 657738f..a6bea06 100644
> --- a/fs/cifs/cifsfs.h
> +++ b/fs/cifs/cifsfs.h
> @@ -81,7 +81,10 @@ extern int cifs_open(struct inode *inode, struct file *file);
>  extern int cifs_close(struct inode *inode, struct file *file);
>  extern int cifs_closedir(struct inode *inode, struct file *file);
>  extern ssize_t cifs_user_read(struct file *file, char __user *read_data,
> -			 size_t read_size, loff_t *poffset);
> +			      size_t read_size, loff_t *poffset);
> +extern ssize_t cifs_user_readv(struct file *file, const struct iovec *iov,
> +			       unsigned long nr_segs, size_t len,
> +			       loff_t *poffset);
>  extern ssize_t cifs_user_write(struct file *file, const char __user *write_data,
>  			 size_t write_size, loff_t *poffset);
>  extern int cifs_lock(struct file *, int, struct file_lock *);
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 2c848ef..9238a2f 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -1676,8 +1676,40 @@ int cifs_flush(struct file *file, fl_owner_t id)
>  	return rc;
>  }
>  
> -ssize_t cifs_user_read(struct file *file, char __user *read_data,
> -	size_t read_size, loff_t *poffset)
> +static int
> +cifs_fill_iov(const struct iovec *iov, unsigned long nr_segs,
> +	      unsigned long *last_len, unsigned long *last_ind, char *data,
> +	      size_t datalen)
> +{
> +	const struct iovec *ciov;
> +	unsigned long cur_len;
> +
> +	while (datalen) {
> +		cur_len = min_t(unsigned long, datalen,
> +					      *last_len);
> +		ciov = &iov[*last_ind];
> +
> +		if (copy_to_user(ciov->iov_base + (ciov->iov_len - *last_len),
> +				 data, cur_len))
> +			return -EFAULT;
> +
> +		datalen -= cur_len;
> +		data += cur_len;
> +
> +		*last_len -= cur_len;
> +		if (*last_len == 0) {
> +			(*last_ind)++;
> +			*last_len = iov[*last_ind].iov_len;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t
> +cifs_generic_user_read(struct file *file, char __user *read_data,
> +		       size_t read_size, loff_t *poffset,
> +		       const struct iovec *iov, unsigned long nr_segs)
>  {
>  	int rc = -EACCES;
>  	unsigned int bytes_read = 0;
> @@ -1690,6 +1722,11 @@ ssize_t cifs_user_read(struct file *file, char __user *read_data,
>  	char *smb_read_data;
>  	char __user *current_offset;
>  	struct smb_com_read_rsp *pSMBr;
> +	unsigned long last_len = 0, last_ind = 0;
> +
> +	/* if vectored - set last_len value */
> +	if (!read_data && nr_segs > 0)
> +		last_len = iov->iov_len;
>  
	^^^^
cifs_user_read can just be the trivial case of cifs_user_readv. It just
has a nr_segs of 1.

Rather than do this, it would probably be cleaner to just drop the
"read_data" and "read_size" parms from this function. Then make
cifs_user_read a wrapper around the cifs_user_readv that declares a
single iovec and passes in nr_segs of 1.

See smb_send and smb_sendv for an example of what I mean.


>  	xid = GetXid();
>  	cifs_sb = CIFS_SB(file->f_path.dentry->d_sb);
> @@ -1707,7 +1744,8 @@ ssize_t cifs_user_read(struct file *file, char __user *read_data,
>  
>  	for (total_read = 0, current_offset = read_data;
>  	     read_size > total_read;
> -	     total_read += bytes_read, current_offset += bytes_read) {
> +	     total_read += bytes_read,
> +	     current_offset += read_data ? bytes_read : 0) {
>  		current_read_size = min_t(const int, read_size - total_read,
>  					  cifs_sb->rsize);
>  		rc = -EAGAIN;
> @@ -1726,12 +1764,18 @@ ssize_t cifs_user_read(struct file *file, char __user *read_data,
>  					 &buf_type);
>  			pSMBr = (struct smb_com_read_rsp *)smb_read_data;
>  			if (smb_read_data) {
> -				if (copy_to_user(current_offset,
> -						smb_read_data +
> -						4 /* RFC1001 length field */ +
> -						le16_to_cpu(pSMBr->DataOffset),
> -						bytes_read))
> -					rc = -EFAULT;
> +				char *data_offset = smb_read_data + 4 +
> +						le16_to_cpu(pSMBr->DataOffset);
> +				if (read_data) {
> +					if (copy_to_user(current_offset,
> +						data_offset, bytes_read))
> +						rc = -EFAULT;
> +				} else {
> +					if (cifs_fill_iov(iov, nr_segs,
> +						&last_len, &last_ind,
> +						data_offset, bytes_read))
> +						rc = -EFAULT;
> +				}
>  
>  				if (buf_type == CIFS_SMALL_BUFFER)
>  					cifs_small_buf_release(smb_read_data);
> @@ -1756,9 +1800,21 @@ ssize_t cifs_user_read(struct file *file, char __user *read_data,
>  	return total_read;
>  }
>  
> +ssize_t cifs_user_read(struct file *file, char __user *read_data,
> +		       size_t read_size, loff_t *poffset)
> +{
> +	return cifs_generic_user_read(file, read_data, read_size, poffset, NULL,
> +				      0);
> +}
> +
> +ssize_t cifs_user_readv(struct file *file, const struct iovec *iov,
> +			unsigned long nr_segs, size_t len, loff_t *poffset)
> +{
> +	return cifs_generic_user_read(file, NULL, len, poffset, iov, nr_segs);
> +}
>  
>  static ssize_t cifs_read(struct file *file, char *read_data, size_t read_size,
> -	loff_t *poffset)
> +			 loff_t *poffset)
>  {
>  	int rc = -EACCES;
>  	unsigned int bytes_read = 0;


-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 3/6] CIFS: Implement cifs_file_strict_mmap

[Jeff Layton]

On Tue, 14 Dec 2010 14:10:57 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Invalidate inode mapping if we don't have at least Level II oplock.
> 
> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/cifs/cifsfs.c |    4 ++--
>  fs/cifs/cifsfs.h |    1 +
>  fs/cifs/file.c   |   17 +++++++++++++++++
>  3 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 7660133..a218afe 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -720,7 +720,7 @@ const struct file_operations cifs_file_strict_ops = {
>  	.lock = cifs_lock,
>  	.fsync = cifs_strict_fsync,
>  	.flush = cifs_flush,
> -	.mmap = cifs_file_mmap,
> +	.mmap = cifs_file_strict_mmap,
>  	.splice_read = generic_file_splice_read,
>  	.llseek = cifs_llseek,
>  #ifdef CONFIG_CIFS_POSIX
> @@ -775,7 +775,7 @@ const struct file_operations cifs_file_strict_nobrl_ops = {
>  	.release = cifs_close,
>  	.fsync = cifs_strict_fsync,
>  	.flush = cifs_flush,
> -	.mmap = cifs_file_mmap,
> +	.mmap = cifs_file_strict_mmap,
>  	.splice_read = generic_file_splice_read,
>  	.llseek = cifs_llseek,
>  #ifdef CONFIG_CIFS_POSIX
> diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
> index 8584829..657738f 100644
> --- a/fs/cifs/cifsfs.h
> +++ b/fs/cifs/cifsfs.h
> @@ -89,6 +89,7 @@ extern int cifs_fsync(struct file *, int);
>  extern int cifs_strict_fsync(struct file *, int);
>  extern int cifs_flush(struct file *, fl_owner_t id);
>  extern int cifs_file_mmap(struct file * , struct vm_area_struct *);
> +extern int cifs_file_strict_mmap(struct file * , struct vm_area_struct *);
>  extern const struct file_operations cifs_dir_ops;
>  extern int cifs_dir_open(struct inode *inode, struct file *file);
>  extern int cifs_readdir(struct file *file, void *direntry, filldir_t filldir);
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 4e37cb8..dd1e59a 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -1827,17 +1827,34 @@ static ssize_t cifs_read(struct file *file, char *read_data, size_t read_size,
>  	return total_read;
>  }
>  
> +int cifs_file_strict_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +	int rc, xid;
> +	struct inode *inode = file->f_path.dentry->d_inode;
> +
> +	xid = GetXid();
> +
> +	if (!CIFS_I(inode)->clientCanCacheRead)
> +		cifs_invalidate_mapping(inode);
> +
> +	rc = generic_file_mmap(file, vma);
> +	FreeXid(xid);
> +	return rc;
> +}
> +
>  int cifs_file_mmap(struct file *file, struct vm_area_struct *vma)
>  {
>  	int rc, xid;
>  
>  	xid = GetXid();
> +
>  	rc = cifs_revalidate_file(file);
>  	if (rc) {
>  		cFYI(1, "Validation prior to mmap failed, error=%d", rc);
>  		FreeXid(xid);
>  		return rc;
>  	}
> +
	^^^^
Slip of the return key? I don't think we need these extra newlines.

>  	rc = generic_file_mmap(file, vma);
>  	FreeXid(xid);
>  	return rc;


-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 2/6] CIFS: Implement cifs_strict_fsync

[Jeff Layton]

On Tue, 14 Dec 2010 14:10:56 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Invalidate inode mapping if we don't have at least Level II oplock in
> cifs_strict_fsync. Also remove filemap_write_and_wait call from cifs_fsync
> because it is previously called from vfs_fsync_range. Add file operations'
> structures for strict cache mode.
> 
> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/cifs/cifsfs.c |   38 ++++++++++++++++++++++++++++++++++++++
>  fs/cifs/cifsfs.h |    8 ++++++--
>  fs/cifs/file.c   |   36 ++++++++++++++++++++++++++++--------
>  fs/cifs/inode.c  |    8 ++++++--
>  4 files changed, 78 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 3936aa7..7660133 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -710,6 +710,25 @@ const struct file_operations cifs_file_ops = {
>  	.setlease = cifs_setlease,
>  };
>  
> +const struct file_operations cifs_file_strict_ops = {
> +	.read = do_sync_read,
> +	.write = do_sync_write,
> +	.aio_read = generic_file_aio_read,
> +	.aio_write = cifs_file_aio_write,
> +	.open = cifs_open,
> +	.release = cifs_close,
> +	.lock = cifs_lock,
> +	.fsync = cifs_strict_fsync,
> +	.flush = cifs_flush,
> +	.mmap = cifs_file_mmap,
> +	.splice_read = generic_file_splice_read,
> +	.llseek = cifs_llseek,
> +#ifdef CONFIG_CIFS_POSIX
> +	.unlocked_ioctl	= cifs_ioctl,
> +#endif /* CONFIG_CIFS_POSIX */
> +	.setlease = cifs_setlease,
> +};
> +
>  const struct file_operations cifs_file_direct_ops = {
>  	/* no aio, no readv -
>  	   BB reevaluate whether they can be done with directio, no cache */
> @@ -728,6 +747,7 @@ const struct file_operations cifs_file_direct_ops = {
>  	.llseek = cifs_llseek,
>  	.setlease = cifs_setlease,
>  };
> +
>  const struct file_operations cifs_file_nobrl_ops = {
>  	.read = do_sync_read,
>  	.write = do_sync_write,
> @@ -746,6 +766,24 @@ const struct file_operations cifs_file_nobrl_ops = {
>  	.setlease = cifs_setlease,
>  };
>  
> +const struct file_operations cifs_file_strict_nobrl_ops = {
> +	.read = do_sync_read,
> +	.write = do_sync_write,
> +	.aio_read = generic_file_aio_read,
> +	.aio_write = cifs_file_aio_write,
> +	.open = cifs_open,
> +	.release = cifs_close,
> +	.fsync = cifs_strict_fsync,
> +	.flush = cifs_flush,
> +	.mmap = cifs_file_mmap,
> +	.splice_read = generic_file_splice_read,
> +	.llseek = cifs_llseek,
> +#ifdef CONFIG_CIFS_POSIX
> +	.unlocked_ioctl	= cifs_ioctl,
> +#endif /* CONFIG_CIFS_POSIX */
> +	.setlease = cifs_setlease,
> +};
> +
>  const struct file_operations cifs_file_direct_nobrl_ops = {
>  	/* no mmap, no aio, no readv -
>  	   BB reevaluate whether they can be done with directio, no cache */
> diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
> index 897b2b2..8584829 100644
> --- a/fs/cifs/cifsfs.h
> +++ b/fs/cifs/cifsfs.h
> @@ -61,6 +61,7 @@ extern int cifs_rename(struct inode *, struct dentry *, struct inode *,
>  		       struct dentry *);
>  extern int cifs_revalidate_file(struct file *filp);
>  extern int cifs_revalidate_dentry(struct dentry *);
> +extern void cifs_invalidate_mapping(struct inode *inode);
>  extern int cifs_getattr(struct vfsmount *, struct dentry *, struct kstat *);
>  extern int cifs_setattr(struct dentry *, struct iattr *);
>  
> @@ -72,8 +73,10 @@ extern const struct inode_operations cifs_dfs_referral_inode_operations;
>  /* Functions related to files and directories */
>  extern const struct file_operations cifs_file_ops;
>  extern const struct file_operations cifs_file_direct_ops; /* if directio mnt */
> -extern const struct file_operations cifs_file_nobrl_ops;
> -extern const struct file_operations cifs_file_direct_nobrl_ops; /* no brlocks */
> +extern const struct file_operations cifs_file_strict_ops; /* if strictio mnt */
> +extern const struct file_operations cifs_file_nobrl_ops; /* no brlocks */
> +extern const struct file_operations cifs_file_direct_nobrl_ops;
> +extern const struct file_operations cifs_file_strict_nobrl_ops;
>  extern int cifs_open(struct inode *inode, struct file *file);
>  extern int cifs_close(struct inode *inode, struct file *file);
>  extern int cifs_closedir(struct inode *inode, struct file *file);
> @@ -83,6 +86,7 @@ extern ssize_t cifs_user_write(struct file *file, const char __user *write_data,
>  			 size_t write_size, loff_t *poffset);
>  extern int cifs_lock(struct file *, int, struct file_lock *);
>  extern int cifs_fsync(struct file *, int);
> +extern int cifs_strict_fsync(struct file *, int);
>  extern int cifs_flush(struct file *, fl_owner_t id);
>  extern int cifs_file_mmap(struct file * , struct vm_area_struct *);
>  extern const struct file_operations cifs_dir_ops;
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 60c3f84..4e37cb8 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -1586,27 +1586,47 @@ static int cifs_write_end(struct file *file, struct address_space *mapping,
>  	return rc;
>  }
>  
> -int cifs_fsync(struct file *file, int datasync)
> +int cifs_strict_fsync(struct file *file, int datasync)
>  {
>  	int xid;
>  	int rc = 0;
>  	struct cifsTconInfo *tcon;
>  	struct cifsFileInfo *smbfile = file->private_data;
>  	struct inode *inode = file->f_path.dentry->d_inode;
> +	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
>  
>  	xid = GetXid();
>  
>  	cFYI(1, "Sync file - name: %s datasync: 0x%x",
>  		file->f_path.dentry->d_name.name, datasync);
>  
> -	rc = filemap_write_and_wait(inode->i_mapping);
> -	if (rc == 0) {
> -		struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
> +	if (!CIFS_I(inode)->clientCanCacheRead)
> +		cifs_invalidate_mapping(inode);
>  
> -		tcon = tlink_tcon(smbfile->tlink);
> -		if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
> -			rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
> -	}
> +	tcon = tlink_tcon(smbfile->tlink);
> +	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
> +		rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
> +
> +	FreeXid(xid);
> +	return rc;
> +}
> +
> +int cifs_fsync(struct file *file, int datasync)
> +{
> +	int xid;
> +	int rc = 0;
> +	struct cifsTconInfo *tcon;
> +	struct cifsFileInfo *smbfile = file->private_data;
> +	struct cifs_sb_info *cifs_sb = CIFS_SB(file->f_path.dentry->d_sb);
> +
> +	xid = GetXid();
> +
> +	cFYI(1, "Sync file - name: %s datasync: 0x%x",
> +		file->f_path.dentry->d_name.name, datasync);
> +
> +	tcon = tlink_tcon(smbfile->tlink);
> +	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
> +		rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
>  
>  	FreeXid(xid);
>  	return rc;
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 589f3e3..a8f7cf3 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -44,13 +44,17 @@ static void cifs_set_ops(struct inode *inode, const bool is_dfs_referral)
>  				inode->i_fop = &cifs_file_direct_nobrl_ops;
>  			else
>  				inode->i_fop = &cifs_file_direct_ops;
> +		} else if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) {
> +			if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
> +				inode->i_fop = &cifs_file_strict_nobrl_ops;
> +			else
> +				inode->i_fop = &cifs_file_strict_ops;
>  		} else if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
>  			inode->i_fop = &cifs_file_nobrl_ops;
>  		else { /* not direct, send byte range locks */
>  			inode->i_fop = &cifs_file_ops;
>  		}
>  
> -
>  		/* check if server can support readpages */
>  		if (cifs_sb_master_tcon(cifs_sb)->ses->server->maxBuf <
>  				PAGE_CACHE_SIZE + MAX_CIFS_HDR_SIZE)
> @@ -1679,7 +1683,7 @@ cifs_inode_needs_reval(struct inode *inode)
>  /*
>   * Zap the cache. Called when invalid_mapping flag is set.
>   */
> -static void
> +void
>  cifs_invalidate_mapping(struct inode *inode)
>  {
>  	int rc;

Looks good to me.

Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 6/6] CIFS: Add strictcache mount option

[Jeff Layton]

On Tue, 14 Dec 2010 14:11:00 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Use for switching on strict cache mode. In this mode the
> client reads from the cache all the time it has Oplock Level II,
> otherwise - read from the server. As for write - the client stores
> a data in the cache in Exclusive Oplock case, otherwise - write
> directly to the server.
> 
> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/cifs/README    |    5 +++++
>  fs/cifs/connect.c |    5 +++++
>  2 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/cifs/README b/fs/cifs/README
> index 46af99a..fe16835 100644
> --- a/fs/cifs/README
> +++ b/fs/cifs/README
> @@ -452,6 +452,11 @@ A partial list of the supported mount options follows:
>  		if oplock (caching token) is granted and held. Note that
>  		direct allows write operations larger than page size
>  		to be sent to the server.
> +  strictcache   Use for switching on strict cache mode. In this mode the
> +		client read from the cache all the time it has Oplock Level II,
> +		otherwise - read from the server. All written data are stored
> +		in the cache, but if the client doesn't have Exclusive Oplock,
> +		it writes the data to the server.
>    acl   	Allow setfacl and getfacl to manage posix ACLs if server
>  		supports them.  (default)
>    noacl 	Do not allow setfacl and getfacl calls on this mount
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index cc1a860..877cb9e 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -84,6 +84,7 @@ struct smb_vol {
>  	bool no_xattr:1;   /* set if xattr (EA) support should be disabled*/
>  	bool server_ino:1; /* use inode numbers from server ie UniqueId */
>  	bool direct_io:1;
> +	bool strict_io:1; /* strict cache behavior */
>  	bool remap:1;      /* set to remap seven reserved chars in filenames */
>  	bool posix_paths:1; /* unset to not ask for posix pathnames. */
>  	bool no_linux_ext:1;
> @@ -1357,6 +1358,8 @@ cifs_parse_mount_options(char *options, const char *devname,
>  			vol->direct_io = 1;
>  		} else if (strnicmp(data, "forcedirectio", 13) == 0) {
>  			vol->direct_io = 1;
> +		} else if (strnicmp(data, "strictcache", 11) == 0) {
> +			vol->strict_io = 1;
>  		} else if (strnicmp(data, "noac", 4) == 0) {
>  			printk(KERN_WARNING "CIFS: Mount option noac not "
>  				"supported. Instead set "
> @@ -2614,6 +2617,8 @@ static void setup_cifs_sb(struct smb_vol *pvolume_info,
>  	if (pvolume_info->multiuser)
>  		cifs_sb->mnt_cifs_flags |= (CIFS_MOUNT_MULTIUSER |
>  					    CIFS_MOUNT_NO_PERM);
> +	if (pvolume_info->strict_io)
> +		cifs_sb->mnt_cifs_flags |= CIFS_MOUNT_STRICT_IO;
>  	if (pvolume_info->direct_io) {
>  		cFYI(1, "mounting share using direct i/o");
>  		cifs_sb->mnt_cifs_flags |= CIFS_MOUNT_DIRECT_IO;

Looks reasonable, but shouldn't be merged until the previous patches
have been.

Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 1/6] CIFS: Make cifsFileInfo_put work with strict cache mode

[Pavel Shilovsky]

2010/11/19 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Fri, 19 Nov 2010 14:55:57 +0300
> I don't think we should apply any patches to fix a problem that we
> don't really understand. If it mostly goes away with cifsFYI turned on
> then it sounds like a race condition of some sort. It looks like
> invalidate_inode_pages2 invalidates locked pages whereas
> invalidate_remote_inode doesn't, so that may be a hint, or it could
> just change the timing of the race.
>
> You'll need to ascertain the nature of the race to ensure that the fix
> is really fixing the problem and not just papering over the bug.
>

I checked all the lock/unlock calls for pages in CIFS code and didn't
find any problem places. I added debug message that shows how many
pages invalidate_remote_inode invalidates and for wrong case it is 0:

 /home/piastry/git/cifs-2.6/fs/cifs/file.c: CIFS VFS: in cifs_close as
Xid: 1922 with uid: 500
 /home/piastry/git/cifs-2.6/fs/cifs/cifssmb.c: In CIFSSMBClose
 /home/piastry/git/cifs-2.6/fs/cifs/transport.c: For smb_command 4
 /home/piastry/git/cifs-2.6/fs/cifs/transport.c: Sending smb:  total_len 45
 /home/piastry/git/cifs-2.6/fs/cifs/connect.c: rfc1002 length 0x27
 /home/piastry/git/cifs-2.6/fs/cifs/file.c: closing last open instance
for inode e2402828
 /home/piastry/git/cifs-2.6/fs/cifs/file.c: CIFS VFS: leaving
cifs_close (xid = 1922) rc = 0
 /home/piastry/git/cifs-2.6/fs/cifs/inode.c: CIFS VFS: in
cifs_revalidate as Xid: 1923 with uid: 500
 /home/piastry/git/cifs-2.6/fs/cifs/inode.c: Revalidate: /_test4321_
inode 0xe2402690 count 1 dentry: 0xf6f0b550 d_time 1962130 jiffies
2313421
 /home/piastry/git/cifs-2.6/fs/cifs/file.c: CIFS VFS: in
cifs_writepages as Xid: 1924 with uid: 500
 /home/piastry/git/cifs-2.6/fs/cifs/file.c: CIFS VFS: leaving
cifs_writepages (xid = 1924) rc = 0
 /home/piastry/git/cifs-2.6/fs/cifs/inode.c: invalidating remote inode
since open detected it changed
 /home/piastry/git/cifs-2.6/fs/cifs/inode.c: invalidate returned: 0
 ^^^
 /home/piastry/git/cifs-2.6/fs/cifs/inode.c: CIFS VFS: leaving
cifs_revalidate (xid = 1923) rc = 0
 /home/piastry/git/cifs-2.6/fs/cifs/file.c: CIFS VFS: in cifs_open as
Xid: 1925 with uid: 500
 /home/piastry/git/cifs-2.6/fs/cifs/file.c: inode = 0xe2402690 file
flags are 0x8000 for /_test4321_

For right cases it is "invalidate returned: 1". This message was taken
from dmesg after a run of my python script I posted before.

So, the page can be locked and then successfully unlocked by VFS
(because invalidate_inode_pages2 locks a page before processing it).
Anyway I think we shouldn't use invalidate_remote_inode at all - we
can miss pages that are under some VFS process. In this case
invalidate_inode_pages2 is ok - I don't think it can break any things.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 1/6] CIFS: Make cifsFileInfo_put work with strict cache mode

[Jeff Layton]

On Fri, 19 Nov 2010 14:55:57 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2010/11/19 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> > 2010/11/19 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> >> 2010/11/15 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> >>>
> >>> Not directly related to your patch, but what exactly is the purpose of
> >>> the "open_inode_helper"? It looks like "pile o' random junk that we do
> >>
> >> I agree that we can replace it with simply calling get_query_info from
> >> cifs_open.
> >>
> >>> for non-posix opens". Maybe it would be best to eliminate that function
> >>> altogether and further unify the posix and non-posix open code. Steve,
> >>> care to comment?
> >>>
> >>> Now that I'm done ranting, I don't think the invalidate mapping call
> >>> here is unnecessary. We will likely have already invalidated the cache
> >>> during the d_revalidate phase, and you're going to do it again below in
> >>> cifs_new_fileinfo.
> >>>
> >>
> >> I investigated it - you was right about unnecessary calling
> >> cifs_invalidate_mapping from open_inode_helper and cifs_new_fileinfo.
> >> But I found the following problem:
> >>
> >> When we execute script close2_problem.py (see in attachment) several
> >> times (e.x. for i in `seq 1 20`; do ./close2_problem.py; done) with
> >> (!) cifsFYI switched off it shows 'a' instead of 'x' (about 1-2 times
> >> for 20 runs). If we switch cifsFYI on it is almost always 'x' (true) -
> >> I have caught 'a' in this case one or two times for many-many runs.
> >> What do you think about it?
> >
> > I continued the investigation and discovered that if we use
> > invalidate_inode_pages2 instead of invalidate_remote_inode - the
> > problem disappears! So, this is the patch we should apply to solve it
> > (I don't post it as a separate patch? because I think it needs more
> > discussing):
> >
> > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> > index 53cce8c..f20f200 100644
> > --- a/fs/cifs/inode.c
> > +++ b/fs/cifs/inode.c
> > @@ -1591,7 +1591,10 @@ cifs_invalidate_mapping(struct inode *inode)
> >                if (rc)
> >                        cifs_i->write_behind_rc = rc;
> >        }
> > -       invalidate_remote_inode(inode);
> > +
> > +       if (inode->i_mapping)
> > +               invalidate_inode_pages2(inode->i_mapping);
> > +
> >        cifs_fscache_reset_inode_cookie(inode);
> >  }
> >
> 
> Sorry, "invalidate" patch above was based on v2.6.36 git tree. Here
> are the version from cifs master branch:
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index ff7d299..66d3ba3 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1679,12 +1679,16 @@ cifs_invalidate_mapping(struct inode *inode)
> 
>         cifs_i->invalid_mapping = false;
> 
> -       /* write back any cached data */
> -       if (inode->i_mapping && inode->i_mapping->nrpages != 0) {
> -               rc = filemap_write_and_wait(inode->i_mapping);
> -               mapping_set_error(inode->i_mapping, rc);
> +       if (inode->i_mapping) {
> +               /* write back any cached data */
> +               if (inode->i_mapping->nrpages != 0) {
> +                       rc = filemap_write_and_wait(inode->i_mapping);
> +                       mapping_set_error(inode->i_mapping, rc);
> +               }
> +
> +               invalidate_inode_pages2(inode->i_mapping);
>         }
> -       invalidate_remote_inode(inode);
> +
>         cifs_fscache_reset_inode_cookie(inode);
>  }
> 
> 
> If everything is ok with it I will post it as a separate patch in right format.
> 

I don't think we should apply any patches to fix a problem that we
don't really understand. If it mostly goes away with cifsFYI turned on
then it sounds like a race condition of some sort. It looks like
invalidate_inode_pages2 invalidates locked pages whereas
invalidate_remote_inode doesn't, so that may be a hint, or it could
just change the timing of the race.

You'll need to ascertain the nature of the race to ensure that the fix
is really fixing the problem and not just papering over the bug.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 1/6] CIFS: Make cifsFileInfo_put work with strict cache mode

[Pavel Shilovsky]

2010/11/19 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> 2010/11/19 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>> 2010/11/15 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>>>
>>> Not directly related to your patch, but what exactly is the purpose of
>>> the "open_inode_helper"? It looks like "pile o' random junk that we do
>>
>> I agree that we can replace it with simply calling get_query_info from
>> cifs_open.
>>
>>> for non-posix opens". Maybe it would be best to eliminate that function
>>> altogether and further unify the posix and non-posix open code. Steve,
>>> care to comment?
>>>
>>> Now that I'm done ranting, I don't think the invalidate mapping call
>>> here is unnecessary. We will likely have already invalidated the cache
>>> during the d_revalidate phase, and you're going to do it again below in
>>> cifs_new_fileinfo.
>>>
>>
>> I investigated it - you was right about unnecessary calling
>> cifs_invalidate_mapping from open_inode_helper and cifs_new_fileinfo.
>> But I found the following problem:
>>
>> When we execute script close2_problem.py (see in attachment) several
>> times (e.x. for i in `seq 1 20`; do ./close2_problem.py; done) with
>> (!) cifsFYI switched off it shows 'a' instead of 'x' (about 1-2 times
>> for 20 runs). If we switch cifsFYI on it is almost always 'x' (true) -
>> I have caught 'a' in this case one or two times for many-many runs.
>> What do you think about it?
>
> I continued the investigation and discovered that if we use
> invalidate_inode_pages2 instead of invalidate_remote_inode - the
> problem disappears! So, this is the patch we should apply to solve it
> (I don't post it as a separate patch? because I think it needs more
> discussing):
>
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 53cce8c..f20f200 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1591,7 +1591,10 @@ cifs_invalidate_mapping(struct inode *inode)
>                if (rc)
>                        cifs_i->write_behind_rc = rc;
>        }
> -       invalidate_remote_inode(inode);
> +
> +       if (inode->i_mapping)
> +               invalidate_inode_pages2(inode->i_mapping);
> +
>        cifs_fscache_reset_inode_cookie(inode);
>  }
>

Sorry, "invalidate" patch above was based on v2.6.36 git tree. Here
are the version from cifs master branch:
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index ff7d299..66d3ba3 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1679,12 +1679,16 @@ cifs_invalidate_mapping(struct inode *inode)

        cifs_i->invalid_mapping = false;

-       /* write back any cached data */
-       if (inode->i_mapping && inode->i_mapping->nrpages != 0) {
-               rc = filemap_write_and_wait(inode->i_mapping);
-               mapping_set_error(inode->i_mapping, rc);
+       if (inode->i_mapping) {
+               /* write back any cached data */
+               if (inode->i_mapping->nrpages != 0) {
+                       rc = filemap_write_and_wait(inode->i_mapping);
+                       mapping_set_error(inode->i_mapping, rc);
+               }
+
+               invalidate_inode_pages2(inode->i_mapping);
        }
-       invalidate_remote_inode(inode);
+
        cifs_fscache_reset_inode_cookie(inode);
 }


If everything is ok with it I will post it as a separate patch in right format.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 1/6] CIFS: Make cifsFileInfo_put work with strict cache mode

[Pavel Shilovsky]

2010/11/19 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> 2010/11/19 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>> 2010/11/15 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>>>
>>> Not directly related to your patch, but what exactly is the purpose of
>>> the "open_inode_helper"? It looks like "pile o' random junk that we do
>>
>> I agree that we can replace it with simply calling get_query_info from
>> cifs_open.
>>
>>> for non-posix opens". Maybe it would be best to eliminate that function
>>> altogether and further unify the posix and non-posix open code. Steve,
>>> care to comment?
>>>
>>> Now that I'm done ranting, I don't think the invalidate mapping call
>>> here is unnecessary. We will likely have already invalidated the cache
>>> during the d_revalidate phase, and you're going to do it again below in
>>> cifs_new_fileinfo.
>>>
>>
>> I investigated it - you was right about unnecessary calling
>> cifs_invalidate_mapping from open_inode_helper and cifs_new_fileinfo.
>> But I found the following problem:
>>
>> When we execute script close2_problem.py (see in attachment) several
>> times (e.x. for i in `seq 1 20`; do ./close2_problem.py; done) with
>> (!) cifsFYI switched off it shows 'a' instead of 'x' (about 1-2 times
>> for 20 runs). If we switch cifsFYI on it is almost always 'x' (true) -
>> I have caught 'a' in this case one or two times for many-many runs.
>> What do you think about it?
>
> I continued the investigation and discovered that if we use
> invalidate_inode_pages2 instead of invalidate_remote_inode - the
> problem disappears! So, this is the patch we should apply to solve it
> (I don't post it as a separate patch? because I think it needs more
> discussing):
>
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 53cce8c..f20f200 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1591,7 +1591,10 @@ cifs_invalidate_mapping(struct inode *inode)
>                if (rc)
>                        cifs_i->write_behind_rc = rc;
>        }
> -       invalidate_remote_inode(inode);
> +
> +       if (inode->i_mapping)
> +               invalidate_inode_pages2(inode->i_mapping);
> +
>        cifs_fscache_reset_inode_cookie(inode);
>  }
>

I have just noticed that Linux kernel NFS client also uses
invalidate_inode_pages2 in nfs_revalidate_mapping:
http://git.kernel.org/?p=linux/kernel/git/sfrench/cifs-2.6.git;a=blob;f=fs/nfs/inode.c;h=314f57164602eda0762c0a226db44aa19be461f5;hb=362d31297fafb150676f4d564ecc7f7f3e3b7fd4#l839

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 1/6] CIFS: Make cifsFileInfo_put work with strict cache mode

[Pavel Shilovsky]

2010/11/19 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> 2010/11/15 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>>
>> Not directly related to your patch, but what exactly is the purpose of
>> the "open_inode_helper"? It looks like "pile o' random junk that we do
>
> I agree that we can replace it with simply calling get_query_info from
> cifs_open.
>
>> for non-posix opens". Maybe it would be best to eliminate that function
>> altogether and further unify the posix and non-posix open code. Steve,
>> care to comment?
>>
>> Now that I'm done ranting, I don't think the invalidate mapping call
>> here is unnecessary. We will likely have already invalidated the cache
>> during the d_revalidate phase, and you're going to do it again below in
>> cifs_new_fileinfo.
>>
>
> I investigated it - you was right about unnecessary calling
> cifs_invalidate_mapping from open_inode_helper and cifs_new_fileinfo.
> But I found the following problem:
>
> When we execute script close2_problem.py (see in attachment) several
> times (e.x. for i in `seq 1 20`; do ./close2_problem.py; done) with
> (!) cifsFYI switched off it shows 'a' instead of 'x' (about 1-2 times
> for 20 runs). If we switch cifsFYI on it is almost always 'x' (true) -
> I have caught 'a' in this case one or two times for many-many runs.
> What do you think about it?

I continued the investigation and discovered that if we use
invalidate_inode_pages2 instead of invalidate_remote_inode - the
problem disappears! So, this is the patch we should apply to solve it
(I don't post it as a separate patch? because I think it needs more
discussing):

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 53cce8c..f20f200 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1591,7 +1591,10 @@ cifs_invalidate_mapping(struct inode *inode)
                if (rc)
                        cifs_i->write_behind_rc = rc;
        }
-       invalidate_remote_inode(inode);
+
+       if (inode->i_mapping)
+               invalidate_inode_pages2(inode->i_mapping);
+
        cifs_fscache_reset_inode_cookie(inode);
 }

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 1/6] CIFS: Make cifsFileInfo_put work with strict cache mode

[Pavel Shilovsky]

[-- Attachment #1: Type: text/plain, Size: 1292 bytes --]

2010/11/15 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>
> Not directly related to your patch, but what exactly is the purpose of
> the "open_inode_helper"? It looks like "pile o' random junk that we do

I agree that we can replace it with simply calling get_query_info from
cifs_open.

> for non-posix opens". Maybe it would be best to eliminate that function
> altogether and further unify the posix and non-posix open code. Steve,
> care to comment?
>
> Now that I'm done ranting, I don't think the invalidate mapping call
> here is unnecessary. We will likely have already invalidated the cache
> during the d_revalidate phase, and you're going to do it again below in
> cifs_new_fileinfo.
>

I investigated it - you was right about unnecessary calling
cifs_invalidate_mapping from open_inode_helper and cifs_new_fileinfo.
But I found the following problem:

When we execute script close2_problem.py (see in attachment) several
times (e.x. for i in `seq 1 20`; do ./close2_problem.py; done) with
(!) cifsFYI switched off it shows 'a' instead of 'x' (about 1-2 times
for 20 runs). If we switch cifsFYI on it is almost always 'x' (true) -
I have caught 'a' in this case one or two times for many-many runs.
What do you think about it?

-- 
Best regards,
Pavel Shilovsky.

[-- Attachment #2: close2_problem.py --]
[-- Type: text/x-python, Size: 542 bytes --]

#!/bin/env python
#
# We have to mount the same share to test, test1, test2 directories that locate in the directory we
# execute this script from.

from os import open, close, O_RDWR, O_CREAT, write, read, O_RDONLY, O_WRONLY, O_TRUNC, lseek, SEEK_END, SEEK_SET

f = open('test/_test4321_', O_RDWR | O_CREAT | O_TRUNC)
close(f)

f1 = open('test1/_test4321_', O_RDWR)
write(f1, 'a')
close(f1)

f2 = open('test2/_test4321_', O_WRONLY)
write(f2, 'x')
close(f2)

f3 = open('test1/_test4321_', O_RDONLY)
print 'must be x:', read(f3, 1)

close(f3)

Re: [PATCH 1/6] CIFS: Make cifsFileInfo_put work with strict cache mode

[Jeff Layton]

On Thu, 11 Nov 2010 10:07:02 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On strict cache mode when we close the last file handle of the inode we
> should set invalidate flag on the inode and check it during open to prevent
> data coherency problem when we open it again but it has been modified by
> other clients.
> 
> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>


When I send multiple versions of the same patch, I usually add "(try
#X)" to the subject. That makes it easier for reviewers to tell which
patch is the latest version.

> ---
>  fs/cifs/cifs_fs_sb.h |    1 +
>  fs/cifs/cifsfs.h     |    1 +
>  fs/cifs/file.c       |   27 +++++++++++++++------------
>  fs/cifs/inode.c      |    3 +--
>  4 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
> index e9a393c..be7b159 100644
> --- a/fs/cifs/cifs_fs_sb.h
> +++ b/fs/cifs/cifs_fs_sb.h
> @@ -40,6 +40,7 @@
>  #define CIFS_MOUNT_FSCACHE	0x8000 /* local caching enabled */
>  #define CIFS_MOUNT_MF_SYMLINKS	0x10000 /* Minshall+French Symlinks enabled */
>  #define CIFS_MOUNT_MULTIUSER	0x20000 /* multiuser mount */
> +#define CIFS_MOUNT_STRICT_IO	0x40000 /* strict cache mode */
>  
>  struct cifs_sb_info {
>  	struct rb_root tlink_tree;
> diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
> index 897b2b2..b8d783c 100644
> --- a/fs/cifs/cifsfs.h
> +++ b/fs/cifs/cifsfs.h
> @@ -61,6 +61,7 @@ extern int cifs_rename(struct inode *, struct dentry *, struct inode *,
>  		       struct dentry *);
>  extern int cifs_revalidate_file(struct file *filp);
>  extern int cifs_revalidate_dentry(struct dentry *);
> +extern void cifs_invalidate_mapping(struct inode *inode);
>  extern int cifs_getattr(struct vfsmount *, struct dentry *, struct kstat *);
>  extern int cifs_setattr(struct dentry *, struct iattr *);
>  
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 06c3e83..d604e09 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -124,19 +124,11 @@ static inline int cifs_open_inode_helper(struct inode *inode,
>  	temp = cifs_NTtimeToUnix(buf->LastWriteTime);
>  	if (timespec_equal(&inode->i_mtime, &temp) &&
>  			   (inode->i_size ==
> -			    (loff_t)le64_to_cpu(buf->EndOfFile))) {
> +			    (loff_t)le64_to_cpu(buf->EndOfFile)) &&
> +			    !pCifsInode->invalid_mapping) {
>  		cFYI(1, "inode unchanged on server");
> -	} else {
> -		if (inode->i_mapping) {
> -			/* BB no need to lock inode until after invalidate
> -			since namei code should already have it locked? */
> -			rc = filemap_write_and_wait(inode->i_mapping);
> -			mapping_set_error(inode->i_mapping, rc);
> -		}
> -		cFYI(1, "invalidating remote inode since open detected it "
> -			 "changed");
> -		invalidate_remote_inode(inode);
> -	}
> +	} else
> +		cifs_invalidate_mapping(inode);
>  

Not directly related to your patch, but what exactly is the purpose of
the "open_inode_helper"? It looks like "pile o' random junk that we do
for non-posix opens". Maybe it would be best to eliminate that function
altogether and further unify the posix and non-posix open code. Steve,
care to comment?

Now that I'm done ranting, I don't think the invalidate mapping call
here is unnecessary. We will likely have already invalidated the cache
during the d_revalidate phase, and you're going to do it again below in
cifs_new_fileinfo.

>  client_can_cache:
>  	if (pTcon->unix_ext)
> @@ -250,6 +242,9 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
>  
>  	cifs_set_oplock_level(pCifsInode, oplock);
>  
> +	if (pCifsInode->invalid_mapping)
> +		cifs_invalidate_mapping(inode);
> +
>  	file->private_data = pCifsFile;
>  	return pCifsFile;
>  }
> @@ -264,6 +259,7 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>  	struct inode *inode = cifs_file->dentry->d_inode;
>  	struct cifsTconInfo *tcon = tlink_tcon(cifs_file->tlink);
>  	struct cifsInodeInfo *cifsi = CIFS_I(inode);
> +	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
>  	struct cifsLockInfo *li, *tmp;
>  
>  	spin_lock(&cifs_file_list_lock);
> @@ -279,6 +275,13 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>  	if (list_empty(&cifsi->openFileList)) {
>  		cFYI(1, "closing last open instance for inode %p",
>  			cifs_file->dentry->d_inode);
> +
> +		/* in strict cache mode we need invalidate mapping on the last
> +		   close  because it may cause a error when we open this file
> +		   again and get at least level II oplock */
> +		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO)
> +			CIFS_I(inode)->invalid_mapping = true;
> +
>  		cifs_set_oplock_level(cifsi, 0);
>  	}
>  	spin_unlock(&cifs_file_list_lock);
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index ef3a55b..518514e 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1671,8 +1671,7 @@ cifs_inode_needs_reval(struct inode *inode)
>  }
>  
>  /* check invalid_mapping flag and zap the cache if it's set */
> -static void
> -cifs_invalidate_mapping(struct inode *inode)
> +void cifs_invalidate_mapping(struct inode *inode)
>  {
>  	int rc;
>  	struct cifsInodeInfo *cifs_i = CIFS_I(inode);


-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 1/6] CIFS: Make cifsFileInfo_put work with strict cache mode

[Pavel Shilovsky]

2010/11/13 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>
> I don't see why the above is necessary. cifs_revalidate_dentry should
> have taken care of invalidating the mapping when it revalidated the
> dentry.
>
> Also, you're leaving invalid_mapping set so you're almost certainly
> going to end up invalidating the mapping unnecessarily again.
>
> Finally, you're not considering the fscache case here. I think you need
> to look closely at how dentry/inode revalidation is handled -- in
> particular cifs_revalidate_dentry and cifs_invalidate_mapping.
>

Jeff, you are right about leaving invalid_mapping set - I noticed it
after some time I posted this patch. So - I reposted it in the list
again. Let's consider this version as invalid.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 1/6] CIFS: Make cifsFileInfo_put work with strict cache mode

[Jeff Layton]

On Wed, 10 Nov 2010 18:41:46 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On strict cache mode when we close the last file handle of the inode we
> should set invalidate flag on the inode and check it during open to prevent
> data coherency problem when we open it again but it has been modified by
> other clients.
> 
> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/cifs/cifs_fs_sb.h |    1 +
>  fs/cifs/file.c       |   14 ++++++++++++++
>  2 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
> index e9a393c..be7b159 100644
> --- a/fs/cifs/cifs_fs_sb.h
> +++ b/fs/cifs/cifs_fs_sb.h
> @@ -40,6 +40,7 @@
>  #define CIFS_MOUNT_FSCACHE	0x8000 /* local caching enabled */
>  #define CIFS_MOUNT_MF_SYMLINKS	0x10000 /* Minshall+French Symlinks enabled */
>  #define CIFS_MOUNT_MULTIUSER	0x20000 /* multiuser mount */
> +#define CIFS_MOUNT_STRICT_IO	0x40000 /* strict cache mode */
>  
>  struct cifs_sb_info {
>  	struct rb_root tlink_tree;
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 06c3e83..88bb366 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -148,6 +148,9 @@ client_can_cache:
>  
>  	cifs_set_oplock_level(pCifsInode, oplock);
>  
> +	if (pCifsInode->invalid_mapping)
> +		invalidate_remote_inode(inode);
> +
>  	return rc;
>  }
>  
> @@ -250,6 +253,9 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
>  
>  	cifs_set_oplock_level(pCifsInode, oplock);
>  
> +	if (pCifsInode->invalid_mapping)
> +		invalidate_remote_inode(inode);
> +
>  	file->private_data = pCifsFile;
>  	return pCifsFile;
>  }

I don't see why the above is necessary. cifs_revalidate_dentry should
have taken care of invalidating the mapping when it revalidated the
dentry.

Also, you're leaving invalid_mapping set so you're almost certainly
going to end up invalidating the mapping unnecessarily again.

Finally, you're not considering the fscache case here. I think you need
to look closely at how dentry/inode revalidation is handled -- in
particular cifs_revalidate_dentry and cifs_invalidate_mapping.

> @@ -264,6 +270,7 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>  	struct inode *inode = cifs_file->dentry->d_inode;
>  	struct cifsTconInfo *tcon = tlink_tcon(cifs_file->tlink);
>  	struct cifsInodeInfo *cifsi = CIFS_I(inode);
> +	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
>  	struct cifsLockInfo *li, *tmp;
>  
>  	spin_lock(&cifs_file_list_lock);
> @@ -279,6 +286,13 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>  	if (list_empty(&cifsi->openFileList)) {
>  		cFYI(1, "closing last open instance for inode %p",
>  			cifs_file->dentry->d_inode);
> +
> +		/* in strict cache mode we need invalidate mapping on the last
> +		   close  because it may cause a error when we open this file
> +		   again and get at least level II oplock */
> +		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO)
> +			CIFS_I(inode)->invalid_mapping = true;
> +
>  		cifs_set_oplock_level(cifsi, 0);
>  	}
>  	spin_unlock(&cifs_file_list_lock);


-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

[PATCH 1/6] CIFS: Make cifsFileInfo_put work with strict cache mode

[Pavel Shilovsky]

On strict cache mode when we close the last file handle of the inode we
should set invalidate flag on the inode and check it during open to prevent
data coherency problem when we open it again but it has been modified by
other clients.

Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/cifs/cifs_fs_sb.h |    1 +
 fs/cifs/cifsfs.h     |    1 +
 fs/cifs/file.c       |   27 +++++++++++++++------------
 fs/cifs/inode.c      |    3 +--
 4 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
index e9a393c..be7b159 100644
--- a/fs/cifs/cifs_fs_sb.h
+++ b/fs/cifs/cifs_fs_sb.h
@@ -40,6 +40,7 @@
 #define CIFS_MOUNT_FSCACHE	0x8000 /* local caching enabled */
 #define CIFS_MOUNT_MF_SYMLINKS	0x10000 /* Minshall+French Symlinks enabled */
 #define CIFS_MOUNT_MULTIUSER	0x20000 /* multiuser mount */
+#define CIFS_MOUNT_STRICT_IO	0x40000 /* strict cache mode */
 
 struct cifs_sb_info {
 	struct rb_root tlink_tree;
diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index 897b2b2..b8d783c 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -61,6 +61,7 @@ extern int cifs_rename(struct inode *, struct dentry *, struct inode *,
 		       struct dentry *);
 extern int cifs_revalidate_file(struct file *filp);
 extern int cifs_revalidate_dentry(struct dentry *);
+extern void cifs_invalidate_mapping(struct inode *inode);
 extern int cifs_getattr(struct vfsmount *, struct dentry *, struct kstat *);
 extern int cifs_setattr(struct dentry *, struct iattr *);
 
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 06c3e83..d604e09 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -124,19 +124,11 @@ static inline int cifs_open_inode_helper(struct inode *inode,
 	temp = cifs_NTtimeToUnix(buf->LastWriteTime);
 	if (timespec_equal(&inode->i_mtime, &temp) &&
 			   (inode->i_size ==
-			    (loff_t)le64_to_cpu(buf->EndOfFile))) {
+			    (loff_t)le64_to_cpu(buf->EndOfFile)) &&
+			    !pCifsInode->invalid_mapping) {
 		cFYI(1, "inode unchanged on server");
-	} else {
-		if (inode->i_mapping) {
-			/* BB no need to lock inode until after invalidate
-			since namei code should already have it locked? */
-			rc = filemap_write_and_wait(inode->i_mapping);
-			mapping_set_error(inode->i_mapping, rc);
-		}
-		cFYI(1, "invalidating remote inode since open detected it "
-			 "changed");
-		invalidate_remote_inode(inode);
-	}
+	} else
+		cifs_invalidate_mapping(inode);
 
 client_can_cache:
 	if (pTcon->unix_ext)
@@ -250,6 +242,9 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
 
 	cifs_set_oplock_level(pCifsInode, oplock);
 
+	if (pCifsInode->invalid_mapping)
+		cifs_invalidate_mapping(inode);
+
 	file->private_data = pCifsFile;
 	return pCifsFile;
 }
@@ -264,6 +259,7 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 	struct inode *inode = cifs_file->dentry->d_inode;
 	struct cifsTconInfo *tcon = tlink_tcon(cifs_file->tlink);
 	struct cifsInodeInfo *cifsi = CIFS_I(inode);
+	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
 	struct cifsLockInfo *li, *tmp;
 
 	spin_lock(&cifs_file_list_lock);
@@ -279,6 +275,13 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 	if (list_empty(&cifsi->openFileList)) {
 		cFYI(1, "closing last open instance for inode %p",
 			cifs_file->dentry->d_inode);
+
+		/* in strict cache mode we need invalidate mapping on the last
+		   close  because it may cause a error when we open this file
+		   again and get at least level II oplock */
+		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO)
+			CIFS_I(inode)->invalid_mapping = true;
+
 		cifs_set_oplock_level(cifsi, 0);
 	}
 	spin_unlock(&cifs_file_list_lock);
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index ef3a55b..518514e 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1671,8 +1671,7 @@ cifs_inode_needs_reval(struct inode *inode)
 }
 
 /* check invalid_mapping flag and zap the cache if it's set */
-static void
-cifs_invalidate_mapping(struct inode *inode)
+void cifs_invalidate_mapping(struct inode *inode)
 {
 	int rc;
 	struct cifsInodeInfo *cifs_i = CIFS_I(inode);
-- 
1.7.2.3

[PATCH 1/6] CIFS: Make cifsFileInfo_put work with strict cache mode

[Pavel Shilovsky]

On strict cache mode when we close the last file handle of the inode we
should set invalidate flag on the inode and check it during open to prevent
data coherency problem when we open it again but it has been modified by
other clients.

Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/cifs/cifs_fs_sb.h |    1 +
 fs/cifs/file.c       |   14 ++++++++++++++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
index e9a393c..be7b159 100644
--- a/fs/cifs/cifs_fs_sb.h
+++ b/fs/cifs/cifs_fs_sb.h
@@ -40,6 +40,7 @@
 #define CIFS_MOUNT_FSCACHE	0x8000 /* local caching enabled */
 #define CIFS_MOUNT_MF_SYMLINKS	0x10000 /* Minshall+French Symlinks enabled */
 #define CIFS_MOUNT_MULTIUSER	0x20000 /* multiuser mount */
+#define CIFS_MOUNT_STRICT_IO	0x40000 /* strict cache mode */
 
 struct cifs_sb_info {
 	struct rb_root tlink_tree;
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 06c3e83..88bb366 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -148,6 +148,9 @@ client_can_cache:
 
 	cifs_set_oplock_level(pCifsInode, oplock);
 
+	if (pCifsInode->invalid_mapping)
+		invalidate_remote_inode(inode);
+
 	return rc;
 }
 
@@ -250,6 +253,9 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
 
 	cifs_set_oplock_level(pCifsInode, oplock);
 
+	if (pCifsInode->invalid_mapping)
+		invalidate_remote_inode(inode);
+
 	file->private_data = pCifsFile;
 	return pCifsFile;
 }
@@ -264,6 +270,7 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 	struct inode *inode = cifs_file->dentry->d_inode;
 	struct cifsTconInfo *tcon = tlink_tcon(cifs_file->tlink);
 	struct cifsInodeInfo *cifsi = CIFS_I(inode);
+	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
 	struct cifsLockInfo *li, *tmp;
 
 	spin_lock(&cifs_file_list_lock);
@@ -279,6 +286,13 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 	if (list_empty(&cifsi->openFileList)) {
 		cFYI(1, "closing last open instance for inode %p",
 			cifs_file->dentry->d_inode);
+
+		/* in strict cache mode we need invalidate mapping on the last
+		   close  because it may cause a error when we open this file
+		   again and get at least level II oplock */
+		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO)
+			CIFS_I(inode)->invalid_mapping = true;
+
 		cifs_set_oplock_level(cifsi, 0);
 	}
 	spin_unlock(&cifs_file_list_lock);
-- 
1.7.2.3

Re: [PATCH 1/6] CIFS: Make cifsFileInfo_put work with strict cache mode

[Jeff Layton]

On Tue, 9 Nov 2010 21:02:19 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2010/11/9 Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>:
> > On Fri,  5 Nov 2010 11:29:32 +0300
> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> On strict cache mode when we close the last file handle of the inode we
> >> should invalidate it to prevent data coherency problem when we open it
> >> again but it has been modified by other clients.
> >>
> >> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >>  fs/cifs/cifs_fs_sb.h |    1 +
> >>  fs/cifs/file.c       |    6 ++++++
> >>  2 files changed, 7 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
> >> index e9a393c..be7b159 100644
> >> --- a/fs/cifs/cifs_fs_sb.h
> >> +++ b/fs/cifs/cifs_fs_sb.h
> >> @@ -40,6 +40,7 @@
> >>  #define CIFS_MOUNT_FSCACHE   0x8000 /* local caching enabled */
> >>  #define CIFS_MOUNT_MF_SYMLINKS       0x10000 /* Minshall+French Symlinks enabled */
> >>  #define CIFS_MOUNT_MULTIUSER 0x20000 /* multiuser mount */
> >> +#define CIFS_MOUNT_STRICT_IO 0x40000 /* strict cache mode */
> >>
> >>  struct cifs_sb_info {
> >>       struct rb_root tlink_tree;
> >> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> >> index 777e7f4..b36de2e 100644
> >> --- a/fs/cifs/file.c
> >> +++ b/fs/cifs/file.c
> >> @@ -264,7 +264,9 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
> >>       struct inode *inode = cifs_file->dentry->d_inode;
> >>       struct cifsTconInfo *tcon = tlink_tcon(cifs_file->tlink);
> >>       struct cifsInodeInfo *cifsi = CIFS_I(inode);
> >> +     struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
> >>       struct cifsLockInfo *li, *tmp;
> >> +     bool last_open_file = false;
> >>
> >>       spin_lock(&cifs_file_list_lock);
> >>       if (--cifs_file->count > 0) {
> >> @@ -279,10 +281,14 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
> >>       if (list_empty(&cifsi->openFileList)) {
> >>               cFYI(1, "closing last open instance for inode %p",
> >>                       cifs_file->dentry->d_inode);
> >> +             last_open_file = true;
> >>               cifs_set_oplock_level(inode, 0);
> >>       }
> >>       spin_unlock(&cifs_file_list_lock);
> >>
> >> +     if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) && last_open_file)
> >> +             invalidate_remote_inode(inode);
> >> +
> >
> > Now that I think about it...this looks racy. Suppose someone races in
> > after you set last_open_file, opens the file and does a write....
> 
> Can you explain what bad happens in this case?
> 

It looks like you'll be invalidating potentially up-to-date data.
Either way...I've been steadily trying to move the code to a more
well-defined codepath for invalidating dirty data. Zapping the cached
pages here is unnecessary. It would be better to just mark the inode as
having an invalid mapping and move on, unless you see some other reason
why we must zap all of the pages here.

> >
> > I think you'd be better served by setting the invalid_mapping flag on
> > the cifs_i instead and letting the normal revalidation codepath handle
> > this. The cache will then be invalidated the next time
> > cifs_revalidate_file/dentry is run.
> >
> >>       if (!tcon->need_reconnect && !cifs_file->invalidHandle) {
> >>               int xid, rc;
> >>
> >
> 
> 
> 


-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 1/6] CIFS: Make cifsFileInfo_put work with strict cache mode

[Pavel Shilovsky]

2010/11/9 Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>:
> On Fri,  5 Nov 2010 11:29:32 +0300
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On strict cache mode when we close the last file handle of the inode we
>> should invalidate it to prevent data coherency problem when we open it
>> again but it has been modified by other clients.
>>
>> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  fs/cifs/cifs_fs_sb.h |    1 +
>>  fs/cifs/file.c       |    6 ++++++
>>  2 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
>> index e9a393c..be7b159 100644
>> --- a/fs/cifs/cifs_fs_sb.h
>> +++ b/fs/cifs/cifs_fs_sb.h
>> @@ -40,6 +40,7 @@
>>  #define CIFS_MOUNT_FSCACHE   0x8000 /* local caching enabled */
>>  #define CIFS_MOUNT_MF_SYMLINKS       0x10000 /* Minshall+French Symlinks enabled */
>>  #define CIFS_MOUNT_MULTIUSER 0x20000 /* multiuser mount */
>> +#define CIFS_MOUNT_STRICT_IO 0x40000 /* strict cache mode */
>>
>>  struct cifs_sb_info {
>>       struct rb_root tlink_tree;
>> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>> index 777e7f4..b36de2e 100644
>> --- a/fs/cifs/file.c
>> +++ b/fs/cifs/file.c
>> @@ -264,7 +264,9 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>>       struct inode *inode = cifs_file->dentry->d_inode;
>>       struct cifsTconInfo *tcon = tlink_tcon(cifs_file->tlink);
>>       struct cifsInodeInfo *cifsi = CIFS_I(inode);
>> +     struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
>>       struct cifsLockInfo *li, *tmp;
>> +     bool last_open_file = false;
>>
>>       spin_lock(&cifs_file_list_lock);
>>       if (--cifs_file->count > 0) {
>> @@ -279,10 +281,14 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>>       if (list_empty(&cifsi->openFileList)) {
>>               cFYI(1, "closing last open instance for inode %p",
>>                       cifs_file->dentry->d_inode);
>> +             last_open_file = true;
>>               cifs_set_oplock_level(inode, 0);
>>       }
>>       spin_unlock(&cifs_file_list_lock);
>>
>> +     if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) && last_open_file)
>> +             invalidate_remote_inode(inode);
>> +
>
> Now that I think about it...this looks racy. Suppose someone races in
> after you set last_open_file, opens the file and does a write....

Can you explain what bad happens in this case?

>
> I think you'd be better served by setting the invalid_mapping flag on
> the cifs_i instead and letting the normal revalidation codepath handle
> this. The cache will then be invalidated the next time
> cifs_revalidate_file/dentry is run.
>
>>       if (!tcon->need_reconnect && !cifs_file->invalidHandle) {
>>               int xid, rc;
>>
>



-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 1/6] CIFS: Make cifsFileInfo_put work with strict cache mode

[Jeff Layton]

On Fri,  5 Nov 2010 11:29:32 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On strict cache mode when we close the last file handle of the inode we
> should invalidate it to prevent data coherency problem when we open it
> again but it has been modified by other clients.
> 
> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/cifs/cifs_fs_sb.h |    1 +
>  fs/cifs/file.c       |    6 ++++++
>  2 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
> index e9a393c..be7b159 100644
> --- a/fs/cifs/cifs_fs_sb.h
> +++ b/fs/cifs/cifs_fs_sb.h
> @@ -40,6 +40,7 @@
>  #define CIFS_MOUNT_FSCACHE	0x8000 /* local caching enabled */
>  #define CIFS_MOUNT_MF_SYMLINKS	0x10000 /* Minshall+French Symlinks enabled */
>  #define CIFS_MOUNT_MULTIUSER	0x20000 /* multiuser mount */
> +#define CIFS_MOUNT_STRICT_IO	0x40000 /* strict cache mode */
>  
>  struct cifs_sb_info {
>  	struct rb_root tlink_tree;
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 777e7f4..b36de2e 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -264,7 +264,9 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>  	struct inode *inode = cifs_file->dentry->d_inode;
>  	struct cifsTconInfo *tcon = tlink_tcon(cifs_file->tlink);
>  	struct cifsInodeInfo *cifsi = CIFS_I(inode);
> +	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
>  	struct cifsLockInfo *li, *tmp;
> +	bool last_open_file = false;
>  
>  	spin_lock(&cifs_file_list_lock);
>  	if (--cifs_file->count > 0) {
> @@ -279,10 +281,14 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>  	if (list_empty(&cifsi->openFileList)) {
>  		cFYI(1, "closing last open instance for inode %p",
>  			cifs_file->dentry->d_inode);
> +		last_open_file = true;
>  		cifs_set_oplock_level(inode, 0);
>  	}
>  	spin_unlock(&cifs_file_list_lock);
>  
> +	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) && last_open_file)
> +		invalidate_remote_inode(inode);
> +

Now that I think about it...this looks racy. Suppose someone races in
after you set last_open_file, opens the file and does a write....

I think you'd be better served by setting the invalid_mapping flag on
the cifs_i instead and letting the normal revalidation codepath handle
this. The cache will then be invalidated the next time
cifs_revalidate_file/dentry is run.

>  	if (!tcon->need_reconnect && !cifs_file->invalidHandle) {
>  		int xid, rc;
>  

[PATCH 1/6] CIFS: Make cifsFileInfo_put work with strict cache mode

[Pavel Shilovsky]

On strict cache mode when we close the last file handle of the inode we
should invalidate it to prevent data coherency problem when we open it
again but it has been modified by other clients.

Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/cifs/cifs_fs_sb.h |    1 +
 fs/cifs/file.c       |    6 ++++++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
index e9a393c..be7b159 100644
--- a/fs/cifs/cifs_fs_sb.h
+++ b/fs/cifs/cifs_fs_sb.h
@@ -40,6 +40,7 @@
 #define CIFS_MOUNT_FSCACHE	0x8000 /* local caching enabled */
 #define CIFS_MOUNT_MF_SYMLINKS	0x10000 /* Minshall+French Symlinks enabled */
 #define CIFS_MOUNT_MULTIUSER	0x20000 /* multiuser mount */
+#define CIFS_MOUNT_STRICT_IO	0x40000 /* strict cache mode */
 
 struct cifs_sb_info {
 	struct rb_root tlink_tree;
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 777e7f4..b36de2e 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -264,7 +264,9 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 	struct inode *inode = cifs_file->dentry->d_inode;
 	struct cifsTconInfo *tcon = tlink_tcon(cifs_file->tlink);
 	struct cifsInodeInfo *cifsi = CIFS_I(inode);
+	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
 	struct cifsLockInfo *li, *tmp;
+	bool last_open_file = false;
 
 	spin_lock(&cifs_file_list_lock);
 	if (--cifs_file->count > 0) {
@@ -279,10 +281,14 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 	if (list_empty(&cifsi->openFileList)) {
 		cFYI(1, "closing last open instance for inode %p",
 			cifs_file->dentry->d_inode);
+		last_open_file = true;
 		cifs_set_oplock_level(inode, 0);
 	}
 	spin_unlock(&cifs_file_list_lock);
 
+	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) && last_open_file)
+		invalidate_remote_inode(inode);
+
 	if (!tcon->need_reconnect && !cifs_file->invalidHandle) {
 		int xid, rc;
 
-- 
1.7.2.3