[PATCH 0/4] fs: fcntl/fadvice fixes v2

[PATCH 0/4] fs: fcntl/fadvice fixes v2

[Dmitry Monakhov]

fcntl(F_SETFL) and fadvise performs direct manipulation with file's internals.
w/o notifying to fs layer. This behavior be not be suitable for some filesystems
(mostly stack-fs like ecryptfs, unionfs, etc). Let's introduce new ->set_flags()
callback for that purpose. This callback is responsible for flags check so
->check_flags() no longer required.

TOC:
 fs: fcntl add set_flags wrapper -v2
 fs: add fadvise file_operation
 ecryptfs: add fadvise/set_flags calbacks
 cifs: add set_flag callback

*OPEN ISSUE REMAINS*
This series does not fix all issues related with set_flags.
Race between fcntl(toggling O_DIRECT) vs write() is still possible
Usually O_DIRECT checked twice during call chain:
 ->xxx_file_write_iter
 --->__generic_file_write_iter
So we may end-up up with two different values. Some filesystems (btrfs/xfs)
avoid this issue by copy-pasting __generic_file_write_iter.
One of possible way to fix this issue it to save flags in kiocb->ki_flags
as we already do with ->ki_pos. And fixup all places accordingly.
I've calculated numbers of direct access to ->f_flags it is close to 150,
half of that number is ->open() methods. So patch would not be gigantic.
And finally here is my question to AlViro and Christoph and other VFS-people:
*Are you agree with that approach?* Please say your word.

[PATCH 1/4] fs: fcntl add set_flags wrapper -v2

[Dmitry Monakhov]

fcntl(F_SETFL) performs direct f_flags manipulation which may be not be suitable
for some filesytems (mostly stack-fs like ecryptfs, unionfs, etc)
For example O_DIRECT toggling may require some actions (page cache flush)
Let's introduce new ->set_flags() callback for that purpose. This callback
is responsible for flags check so ->check_flags() no longer needed.

changes from v1:
 - add generic_file_set_flags helper

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/bad_inode.c     |    4 ++--
 fs/fcntl.c         |   14 ++++----------
 fs/nfs/dir.c       |    5 ++---
 fs/nfs/file.c      |   12 ++++++++----
 fs/nfs/internal.h  |    2 +-
 fs/nfs/nfs4file.c  |    2 +-
 include/linux/fs.h |   10 +++++++++-
 7 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/fs/bad_inode.c b/fs/bad_inode.c
index afd2b44..1977f10 100644
--- a/fs/bad_inode.c
+++ b/fs/bad_inode.c
@@ -121,7 +121,7 @@ static unsigned long bad_file_get_unmapped_area(struct file *file,
 	return -EIO;
 }
 
-static int bad_file_check_flags(int flags)
+static int bad_file_set_flags(struct file *file, int flags)
 {
 	return -EIO;
 }
@@ -166,7 +166,7 @@ static const struct file_operations bad_file_ops =
 	.lock		= bad_file_lock,
 	.sendpage	= bad_file_sendpage,
 	.get_unmapped_area = bad_file_get_unmapped_area,
-	.check_flags	= bad_file_check_flags,
+	.set_flags	= bad_file_set_flags,
 	.flock		= bad_file_flock,
 	.splice_write	= bad_file_splice_write,
 	.splice_read	= bad_file_splice_read,
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 22d1c3d..2c1b3d7 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -27,8 +27,6 @@
 #include <asm/siginfo.h>
 #include <asm/uaccess.h>
 
-#define SETFL_MASK (O_APPEND | O_NONBLOCK | O_NDELAY | O_DIRECT | O_NOATIME)
-
 static int setfl(int fd, struct file * filp, unsigned long arg)
 {
 	struct inode * inode = file_inode(filp);
@@ -57,11 +55,6 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
 				return -EINVAL;
 	}
 
-	if (filp->f_op->check_flags)
-		error = filp->f_op->check_flags(arg);
-	if (error)
-		return error;
-
 	/*
 	 * ->fasync() is responsible for setting the FASYNC bit.
 	 */
@@ -72,10 +65,11 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
 		if (error > 0)
 			error = 0;
 	}
-	spin_lock(&filp->f_lock);
-	filp->f_flags = (arg & SETFL_MASK) | (filp->f_flags & ~SETFL_MASK);
-	spin_unlock(&filp->f_lock);
 
+	if (filp->f_op && filp->f_op->set_flags)
+		error = filp->f_op->set_flags(filp, arg);
+	else
+		generic_file_set_flags(filp, arg);
  out:
 	return error;
 }
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 06e8cfc..8b5d9dc 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1480,9 +1480,8 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
 	dfprintk(VFS, "NFS: atomic_open(%s/%lu), %pd\n",
 			dir->i_sb->s_id, dir->i_ino, dentry);
 
-	err = nfs_check_flags(open_flags);
-	if (err)
-		return err;
+	if ((open_flags & (O_APPEND | O_DIRECT)) == (O_APPEND | O_DIRECT))
+		return -EINVAL;
 
 	/* NFS only supports OPEN on regular files */
 	if ((open_flags & O_DIRECTORY)) {
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 524dd80..b68d272 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -48,14 +48,18 @@ static const struct vm_operations_struct nfs_file_vm_ops;
 # define IS_SWAPFILE(inode)	(0)
 #endif
 
-int nfs_check_flags(int flags)
+#define NFS_FL_MASK (O_NONBLOCK | O_NDELAY | O_NOATIME)
+int nfs_set_flags(struct file *filp, int flags)
 {
 	if ((flags & (O_APPEND | O_DIRECT)) == (O_APPEND | O_DIRECT))
 		return -EINVAL;
 
+	spin_lock(&filp->f_lock);
+	filp->f_flags = (flags & NFS_FL_MASK) | (filp->f_flags & ~NFS_FL_MASK);
+	spin_unlock(&filp->f_lock);
 	return 0;
 }
-EXPORT_SYMBOL_GPL(nfs_check_flags);
+EXPORT_SYMBOL_GPL(nfs_set_flags);
 
 /*
  * Open file
@@ -68,7 +72,7 @@ nfs_file_open(struct inode *inode, struct file *filp)
 	dprintk("NFS: open file(%pD2)\n", filp);
 
 	nfs_inc_stats(inode, NFSIOS_VFSOPEN);
-	res = nfs_check_flags(filp->f_flags);
+	res = nfs_set_flags(filp, filp->f_flags);
 	if (res)
 		return res;
 
@@ -917,7 +921,7 @@ const struct file_operations nfs_file_operations = {
 	.flock		= nfs_flock,
 	.splice_read	= nfs_file_splice_read,
 	.splice_write	= iter_file_splice_write,
-	.check_flags	= nfs_check_flags,
+	.set_flags	= nfs_set_flags,
 	.setlease	= nfs_setlease,
 };
 EXPORT_SYMBOL_GPL(nfs_file_operations);
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 9056622..00cf588 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -345,7 +345,7 @@ ssize_t nfs_file_write(struct kiocb *, struct iov_iter *);
 int nfs_file_release(struct inode *, struct file *);
 int nfs_lock(struct file *, int, struct file_lock *);
 int nfs_flock(struct file *, int, struct file_lock *);
-int nfs_check_flags(int);
+int nfs_set_flags(struct file *file, int flags);
 int nfs_setlease(struct file *, long, struct file_lock **);
 
 /* inode.c */
diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index a816f06..cc192f4 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -130,6 +130,6 @@ const struct file_operations nfs4_file_operations = {
 	.flock		= nfs_flock,
 	.splice_read	= nfs_file_splice_read,
 	.splice_write	= iter_file_splice_write,
-	.check_flags	= nfs_check_flags,
+	.set_flags	= nfs_set_flags,
 	.setlease	= nfs_setlease,
 };
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 9214836..4ce1414 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -944,6 +944,14 @@ struct file_lock {
 #endif
 
 #include <linux/fcntl.h>
+#define SETFL_MASK (O_APPEND | O_NONBLOCK | O_NDELAY | O_DIRECT | O_NOATIME)
+
+static inline void generic_file_set_flags(struct file *filp, unsigned arg)
+{
+	spin_lock(&filp->f_lock);
+	filp->f_flags = (arg & SETFL_MASK) | (filp->f_flags & ~SETFL_MASK);
+	spin_unlock(&filp->f_lock);
+}
 
 extern void send_sigio(struct fown_struct *fown, int fd, int band);
 
@@ -1502,7 +1510,7 @@ struct file_operations {
 	int (*lock) (struct file *, int, struct file_lock *);
 	ssize_t (*sendpage) (struct file *, struct page *, int, size_t, loff_t *, int);
 	unsigned long (*get_unmapped_area)(struct file *, unsigned long, unsigned long, unsigned long, unsigned long);
-	int (*check_flags)(int);
+	int (*set_flags)(struct file *, unsigned);
 	int (*flock) (struct file *, int, struct file_lock *);
 	ssize_t (*splice_write)(struct pipe_inode_info *, struct file *, loff_t *, size_t, unsigned int);
 	ssize_t (*splice_read)(struct file *, loff_t *, struct pipe_inode_info *, size_t, unsigned int);
-- 
1.7.1

[PATCH 2/4] fs: add fadvise file_operation

[Dmitry Monakhov]

sys_fadvise result in direct f_mode modification, which  may be not
suitable for some unusual filesytems where file mode invariant is more
complex. In order to support such filesystems we have to delegate fadvise
logic to filesystem layer.

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 include/linux/fs.h |    4 ++
 mm/fadvise.c       |   81 ++++++++++++++++++++++++++++++++-------------------
 2 files changed, 55 insertions(+), 30 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 4ce1414..0fe06f5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1518,6 +1518,7 @@ struct file_operations {
 	long (*fallocate)(struct file *file, int mode, loff_t offset,
 			  loff_t len);
 	int (*show_fdinfo)(struct seq_file *m, struct file *f);
+	int (*fadvise)(struct file *file, loff_t off, loff_t len, int advice);
 };
 
 struct inode_operations {
@@ -2081,6 +2082,9 @@ extern int finish_open(struct file *file, struct dentry *dentry,
 			int *opened);
 extern int finish_no_open(struct file *file, struct dentry *dentry);
 
+/* fs/fadvise.c */
+extern int generic_fadvise(struct file *file, loff_t off, loff_t len, int adv);
+
 /* fs/ioctl.c */
 
 extern int ioctl_preallocate(struct file *filp, void __user *argp);
diff --git a/mm/fadvise.c b/mm/fadvise.c
index 3bcfd81..a568ba6 100644
--- a/mm/fadvise.c
+++ b/mm/fadvise.c
@@ -7,6 +7,7 @@
  *		Initial version.
  */
 
+#include <linux/export.h>
 #include <linux/kernel.h>
 #include <linux/file.h>
 #include <linux/fs.h>
@@ -25,10 +26,9 @@
  * POSIX_FADV_WILLNEED could set PG_Referenced, and POSIX_FADV_NOREUSE could
  * deactivate the pages and clear PG_Referenced.
  */
-SYSCALL_DEFINE4(fadvise64_64, int, fd, loff_t, offset, loff_t, len, int, advice)
+int generic_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
 {
-	struct fd f = fdget(fd);
-	struct address_space *mapping;
+	struct address_space *mapping = file->f_mapping;
 	struct backing_dev_info *bdi;
 	loff_t endbyte;			/* inclusive */
 	pgoff_t start_index;
@@ -36,20 +36,6 @@ SYSCALL_DEFINE4(fadvise64_64, int, fd, loff_t, offset, loff_t, len, int, advice)
 	unsigned long nrpages;
 	int ret = 0;
 
-	if (!f.file)
-		return -EBADF;
-
-	if (S_ISFIFO(file_inode(f.file)->i_mode)) {
-		ret = -ESPIPE;
-		goto out;
-	}
-
-	mapping = f.file->f_mapping;
-	if (!mapping || len < 0) {
-		ret = -EINVAL;
-		goto out;
-	}
-
 	if (mapping->a_ops->get_xip_mem) {
 		switch (advice) {
 		case POSIX_FADV_NORMAL:
@@ -77,21 +63,21 @@ SYSCALL_DEFINE4(fadvise64_64, int, fd, loff_t, offset, loff_t, len, int, advice)
 
 	switch (advice) {
 	case POSIX_FADV_NORMAL:
-		f.file->f_ra.ra_pages = bdi->ra_pages;
-		spin_lock(&f.file->f_lock);
-		f.file->f_mode &= ~FMODE_RANDOM;
-		spin_unlock(&f.file->f_lock);
+		file->f_ra.ra_pages = bdi->ra_pages;
+		spin_lock(&file->f_lock);
+		file->f_mode &= ~FMODE_RANDOM;
+		spin_unlock(&file->f_lock);
 		break;
 	case POSIX_FADV_RANDOM:
-		spin_lock(&f.file->f_lock);
-		f.file->f_mode |= FMODE_RANDOM;
-		spin_unlock(&f.file->f_lock);
+		spin_lock(&file->f_lock);
+		file->f_mode |= FMODE_RANDOM;
+		spin_unlock(&file->f_lock);
 		break;
 	case POSIX_FADV_SEQUENTIAL:
-		f.file->f_ra.ra_pages = bdi->ra_pages * 2;
-		spin_lock(&f.file->f_lock);
-		f.file->f_mode &= ~FMODE_RANDOM;
-		spin_unlock(&f.file->f_lock);
+		file->f_ra.ra_pages = bdi->ra_pages * 2;
+		spin_lock(&file->f_lock);
+		file->f_mode &= ~FMODE_RANDOM;
+		spin_unlock(&file->f_lock);
 		break;
 	case POSIX_FADV_WILLNEED:
 		/* First and last PARTIAL page! */
@@ -107,7 +93,7 @@ SYSCALL_DEFINE4(fadvise64_64, int, fd, loff_t, offset, loff_t, len, int, advice)
 		 * Ignore return value because fadvise() shall return
 		 * success even if filesystem can't retrieve a hint,
 		 */
-		force_page_cache_readahead(mapping, f.file, start_index,
+		force_page_cache_readahead(mapping, file, start_index,
 					   nrpages);
 		break;
 	case POSIX_FADV_NOREUSE:
@@ -142,15 +128,50 @@ SYSCALL_DEFINE4(fadvise64_64, int, fd, loff_t, offset, loff_t, len, int, advice)
 		ret = -EINVAL;
 	}
 out:
+	return ret;
+}
+EXPORT_SYMBOL(generic_fadvise);
+
+static int do_fadvise(int fd, loff_t offset, loff_t len, int advice)
+{
+	struct fd f = fdget(fd);
+	int (*fadvise)(struct file *, loff_t, loff_t, int) = generic_fadvise;
+	int ret = 0;
+
+	if (!f.file)
+		return -EBADF;
+
+	if (S_ISFIFO(file_inode(f.file)->i_mode)) {
+		ret = -ESPIPE;
+		goto out;
+	}
+	if (!f.file->f_mapping || len < 0) {
+		ret = -EINVAL;
+		goto out;
+	}
+	if (!f.file->f_mapping || len < 0) {
+		ret = -EINVAL;
+		goto out;
+	}
+	if (f.file->f_op && f.file->f_op->fadvise)
+		fadvise = f.file->f_op->fadvise;
+
+	ret = fadvise(f.file, offset, len, advice);
+out:
 	fdput(f);
 	return ret;
 }
 
+SYSCALL_DEFINE4(fadvise64_64, int, fd, loff_t, offset, loff_t, len, int, advice)
+{
+	return do_fadvise(fd, offset, len, advice);
+}
+
 #ifdef __ARCH_WANT_SYS_FADVISE64
 
 SYSCALL_DEFINE4(fadvise64, int, fd, loff_t, offset, size_t, len, int, advice)
 {
-	return sys_fadvise64_64(fd, offset, len, advice);
+	return do_fadvise(fd, offset, len, advice);
 }
 
 #endif
-- 
1.7.1

[PATCH 3/4] ecryptfs: add fadvise/set_flags calbacks

[Dmitry Monakhov]

CC: tyhicks@canonical.com
CC: ecryptfs@vger.kernel.org
Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ecryptfs/file.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 62 insertions(+), 0 deletions(-)

diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
index 4ffa35e..c84df35 100644
--- a/fs/ecryptfs/file.c
+++ b/fs/ecryptfs/file.c
@@ -31,6 +31,7 @@
 #include <linux/security.h>
 #include <linux/compat.h>
 #include <linux/fs_stack.h>
+#include <linux/fadvise.h>
 #include <linux/aio.h>
 #include "ecryptfs_kernel.h"
 
@@ -333,6 +334,63 @@ ecryptfs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 }
 #endif
 
+static int
+ecryptfs_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
+{
+	struct file *lower_file = NULL;
+	long rc = 0;
+
+	if (ecryptfs_file_to_private(file))
+		lower_file = ecryptfs_file_to_lower(file);
+
+	if (!lower_file || !lower_file->f_op)
+		return rc;
+
+	if (lower_file->f_op && lower_file->f_op->fadvise)
+		rc = lower_file->f_op->fadvise(lower_file, offset, len, advice);
+	else
+		rc = generic_fadvise(lower_file, offset, len, advice);
+	if (!rc)
+		generic_fadvise(file, offset, len, advice);
+
+	return rc;
+}
+
+#define ECRYPTFS_FL_MASK (O_NONBLOCK | O_NDELAY | O_NOATIME)
+static int ecryptfs_set_flags(struct file *file, unsigned flags)
+{
+	struct ecryptfs_mount_crypt_stat *mount_crypt_stat;
+	struct dentry *ecryptfs_dentry = file->f_path.dentry;
+	struct file *lower_file = NULL;
+	int rc = 0;
+
+	mount_crypt_stat = &ecryptfs_superblock_to_private(
+		ecryptfs_dentry->d_sb)->mount_crypt_stat;
+	if ((mount_crypt_stat->flags & ECRYPTFS_ENCRYPTED_VIEW_ENABLED)
+	    && (flags & O_APPEND)) {
+		printk(KERN_WARNING "Mount has encrypted view enabled; "
+		       "files may only be read\n");
+		rc = -EPERM;
+		goto out;
+	}
+
+	if (ecryptfs_file_to_private(file))
+		lower_file = ecryptfs_file_to_lower(file);
+	if (!lower_file)
+		goto out;
+
+	if (lower_file->f_op && lower_file->f_op->set_flags) {
+		rc = lower_file->f_op->set_flags(lower_file,
+						 flags & ECRYPTFS_FL_MASK);
+		if (rc)
+			return rc;
+	} else
+		generic_file_set_flags(file, flags & ECRYPTFS_FL_MASK);
+
+	generic_file_set_flags(file, flags & ECRYPTFS_FL_MASK);
+out:
+	return rc;
+}
 const struct file_operations ecryptfs_dir_fops = {
 	.iterate = ecryptfs_readdir,
 	.read = generic_read_dir,
@@ -347,6 +405,8 @@ const struct file_operations ecryptfs_dir_fops = {
 	.fasync = ecryptfs_fasync,
 	.splice_read = generic_file_splice_read,
 	.llseek = default_llseek,
+	.set_flags = ecryptfs_set_flags,
+	.fadvise = ecryptfs_fadvise,
 };
 
 const struct file_operations ecryptfs_main_fops = {
@@ -367,4 +427,6 @@ const struct file_operations ecryptfs_main_fops = {
 	.fsync = ecryptfs_fsync,
 	.fasync = ecryptfs_fasync,
 	.splice_read = generic_file_splice_read,
+	.set_flags = ecryptfs_set_flags,
+	.fadvise = ecryptfs_fadvise,
 };
-- 
1.7.1

[PATCH 4/4] cifs: add set_flag callback

[Dmitry Monakhov]

Add set_flag callback which is called from fcntl(F_SETFL)
Share common logic for cifs_open and cifs_set_flags
I'm not cifs expert, but it is looks like toggling O_DIRECT on file is
unsafe operation so disable it temporally.

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/cifs/cifsfs.c |    6 ++++++
 fs/cifs/cifsfs.h |    1 +
 fs/cifs/file.c   |   37 ++++++++++++++++++++++++++++++-------
 3 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 889b984..18ad412 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -918,6 +918,7 @@ const struct file_operations cifs_file_ops = {
 	.mmap  = cifs_file_mmap,
 	.splice_read = generic_file_splice_read,
 	.llseek = cifs_llseek,
+	.set_flags = cifs_set_flags,
 #ifdef CONFIG_CIFS_POSIX
 	.unlocked_ioctl	= cifs_ioctl,
 #endif /* CONFIG_CIFS_POSIX */
@@ -938,6 +939,7 @@ const struct file_operations cifs_file_strict_ops = {
 	.mmap = cifs_file_strict_mmap,
 	.splice_read = generic_file_splice_read,
 	.llseek = cifs_llseek,
+	.set_flags = cifs_set_flags,
 #ifdef CONFIG_CIFS_POSIX
 	.unlocked_ioctl	= cifs_ioctl,
 #endif /* CONFIG_CIFS_POSIX */
@@ -958,6 +960,7 @@ const struct file_operations cifs_file_direct_ops = {
 	.flush = cifs_flush,
 	.mmap = cifs_file_mmap,
 	.splice_read = generic_file_splice_read,
+	.set_flags = cifs_set_flags,
 #ifdef CONFIG_CIFS_POSIX
 	.unlocked_ioctl  = cifs_ioctl,
 #endif /* CONFIG_CIFS_POSIX */
@@ -978,6 +981,7 @@ const struct file_operations cifs_file_nobrl_ops = {
 	.mmap  = cifs_file_mmap,
 	.splice_read = generic_file_splice_read,
 	.llseek = cifs_llseek,
+	.set_flags = cifs_set_flags,
 #ifdef CONFIG_CIFS_POSIX
 	.unlocked_ioctl	= cifs_ioctl,
 #endif /* CONFIG_CIFS_POSIX */
@@ -997,6 +1001,7 @@ const struct file_operations cifs_file_strict_nobrl_ops = {
 	.mmap = cifs_file_strict_mmap,
 	.splice_read = generic_file_splice_read,
 	.llseek = cifs_llseek,
+	.set_flags = cifs_set_flags,
 #ifdef CONFIG_CIFS_POSIX
 	.unlocked_ioctl	= cifs_ioctl,
 #endif /* CONFIG_CIFS_POSIX */
@@ -1016,6 +1021,7 @@ const struct file_operations cifs_file_direct_nobrl_ops = {
 	.flush = cifs_flush,
 	.mmap = cifs_file_mmap,
 	.splice_read = generic_file_splice_read,
+	.set_flags = cifs_set_flags,
 #ifdef CONFIG_CIFS_POSIX
 	.unlocked_ioctl  = cifs_ioctl,
 #endif /* CONFIG_CIFS_POSIX */
diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index 002e0c1..353190d 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -92,6 +92,7 @@ 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_set_flags(struct file *filp, unsigned arg);
 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);
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index dc3c7e6..5167ecb 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -431,6 +431,33 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 	kfree(cifs_file);
 }
 
+int cifs_set_flags(struct file *file, unsigned arg)
+{
+	struct cifs_sb_info *cifs_sb = CIFS_SB(file->f_path.dentry->d_sb);
+	int rc = 0;
+
+	spin_lock(&file->f_lock);
+	if ((file->f_flags ^ arg) & O_DIRECT) {
+		/*
+		 * TODO: Toggling O_DIRECT is not obvious task,
+		 * temproraly disabled for safity reason
+		 */
+		rc = -EINVAL;
+		goto out;
+	}
+	file->f_flags = (arg & SETFL_MASK) | (file->f_flags & ~SETFL_MASK);
+	if (file->f_flags & O_DIRECT &&
+	    cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) {
+		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
+			file->f_op = &cifs_file_direct_nobrl_ops;
+		else
+			file->f_op = &cifs_file_direct_ops;
+	}
+out:
+	spin_unlock(&file->f_lock);
+	return rc;
+}
+
 int cifs_open(struct inode *inode, struct file *file)
 
 {
@@ -467,13 +494,9 @@ int cifs_open(struct inode *inode, struct file *file)
 	cifs_dbg(FYI, "inode = 0x%p file flags are 0x%x for %s\n",
 		 inode, file->f_flags, full_path);
 
-	if (file->f_flags & O_DIRECT &&
-	    cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) {
-		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
-			file->f_op = &cifs_file_direct_nobrl_ops;
-		else
-			file->f_op = &cifs_file_direct_ops;
-	}
+	rc = cifs_set_flags(file, file->f_flags);
+	if (rc)
+		goto out;
 
 	if (server->oplocks)
 		oplock = REQ_OPLOCK;
-- 
1.7.1

Re: [PATCH 3/4] ecryptfs: add fadvise/set_flags calbacks

[Christoph Hellwig]

On Sat, Oct 18, 2014 at 07:21:27PM +0400, Dmitry Monakhov wrote:
> +	if (ecryptfs_file_to_private(file))
> +		lower_file = ecryptfs_file_to_lower(file);
> +
> +	if (!lower_file || !lower_file->f_op)
> +		return rc;

At least a file without f->f_op should never happen.  How could ecryptfs
not have a lower file here?

>
> +
> +	if (lower_file->f_op && lower_file->f_op->fadvise)
> +		rc = lower_file->f_op->fadvise(lower_file, offset, len, advice);
> +	else
> +		rc = generic_fadvise(lower_file, offset, len, advice);

Seems like this should be in a vfs_fadvice helper.

> +	if (!rc)
> +		generic_fadvise(file, offset, len, advice);

Setting the advice on both files seems odd.  Which one do we actually
need them on?

> +	if (lower_file->f_op && lower_file->f_op->set_flags) {
> +		rc = lower_file->f_op->set_flags(lower_file,
> +						 flags & ECRYPTFS_FL_MASK);
> +		if (rc)
> +			return rc;
> +	} else
> +		generic_file_set_flags(file, flags & ECRYPTFS_FL_MASK);

Seems like you want a vfs_set_flags again.

Re: [PATCH 2/4] fs: add fadvise file_operation

[Christoph Hellwig]

On Sat, Oct 18, 2014 at 07:21:26PM +0400, Dmitry Monakhov wrote:
> sys_fadvise result in direct f_mode modification, which  may be not
> suitable for some unusual filesytems where file mode invariant is more
> complex. In order to support such filesystems we have to delegate fadvise
> logic to filesystem layer.

Is there a real use case for it?  So for it seems mostly about ecryptfs,
and even that use is lacking a proper explanation.

Also fadvice and set_flags seem entirely unrelated, I don't understand
why you're throwing fadvice in thise series.

Re: [PATCH 2/4] fs: add fadvise file_operation

[Dmitry Monakhov]

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

Christoph Hellwig <hch@infradead.org> writes:

> On Sat, Oct 18, 2014 at 07:21:26PM +0400, Dmitry Monakhov wrote:
>> sys_fadvise result in direct f_mode modification, which  may be not
>> suitable for some unusual filesytems where file mode invariant is more
>> complex. In order to support such filesystems we have to delegate fadvise
>> logic to filesystem layer.
>
> Is there a real use case for it?  So for it seems mostly about ecryptfs,
> and even that use is lacking a proper explanation.
Fairly to say original issue was with vzfs (COW stack filesystem for
containers from openvz). As far as I understand direct analog is
unionfs. We asserted  v_file->mode == lower_file->f_mode, but
fadvise (POSIX_FADV_{RANDOM,RANDOM,SEQUENTIAL})changes v_file->mode directly.
>
> Also fadvice and set_flags seem entirely unrelated, I don't understand
> why you're throwing fadvice in thise series.
It has semantic relation. Both methods manipulate filp->XXX internals directly
No problem I can split this to separate patch set.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Type: application/pgp-signature, Size: 818 bytes --]