[PATCH 1/3] fs: fcntl add set_flags wrapper

[PATCH 1/3] fs: fcntl add set_flags wrapper

[Dmitry Monakhov]

fcntl(F_SETFL) performs direct manipulation f_flags, which may be not be appropriate
for some filesytems (mostly stack-fs like FUSE, ecryptfs, unionfs, etc)
For example O_DIRECT toggling may require extra 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.

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/bad_inode.c     |    4 ++--
 fs/fcntl.c         |   18 ++++++++----------
 fs/nfs/dir.c       |    5 ++---
 fs/nfs/file.c      |   12 ++++++++----
 fs/nfs/internal.h  |    2 +-
 fs/nfs/nfs4file.c  |    2 +-
 include/linux/fs.h |    3 ++-
 7 files changed, 24 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..71ce217 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,15 @@ 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 {
+		spin_lock(&filp->f_lock);
+		filp->f_flags = (arg & SETFL_MASK) |
+			(filp->f_flags & ~SETFL_MASK);
+		spin_unlock(&filp->f_lock);
+	}
  out:
 	return error;
 }
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 36d921f..a112d53 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1485,9 +1485,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 9418772..bbf40a5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -944,6 +944,7 @@ struct file_lock {
 #endif
 
 #include <linux/fcntl.h>
+#define SETFL_MASK (O_APPEND | O_NONBLOCK | O_NDELAY | O_DIRECT | O_NOATIME)
 
 extern void send_sigio(struct fown_struct *fown, int fd, int band);
 
@@ -1502,7 +1503,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 *, int);
 	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/3] 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 bbf40a5..8c4c323 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1511,6 +1511,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 {
@@ -2073,6 +2074,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/3] 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 |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 67 insertions(+), 0 deletions(-)

diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
index db0fad3..637469a 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,68 @@ 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, int 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);
+		if (rc)
+			return rc;
+	} else {
+		spin_lock(&lower_file->f_lock);
+		lower_file->f_flags = (flags & ECRYPTFS_FL_MASK) |
+			(lower_file->f_flags & ~ECRYPTFS_FL_MASK);
+		spin_unlock(&lower_file->f_lock);
+	}
+	spin_lock(&file->f_lock);
+	file->f_flags = (flags & ECRYPTFS_FL_MASK) |
+		(file->f_flags & ~ECRYPTFS_FL_MASK);
+	spin_unlock(&file->f_lock);
+out:
+	return rc;
+}
 const struct file_operations ecryptfs_dir_fops = {
 	.iterate = ecryptfs_readdir,
 	.read = generic_read_dir,
@@ -347,6 +410,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 +432,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

Re: [PATCH 1/3] fs: fcntl add set_flags wrapper

[Christoph Hellwig]

> +	else {
> +		spin_lock(&filp->f_lock);
> +		filp->f_flags = (arg & SETFL_MASK) |
> +			(filp->f_flags & ~SETFL_MASK);
> +		spin_unlock(&filp->f_lock);

Please move this into an exported generic_file_set_flags helper, that
the filesystems can use in their implementations instead of duplicating
it.

Also a more conceptual question:  Basically any check the filesystems
may perform needs to be duplicated in open and ->set_flags.  Any chance
to have the open path call into ->set_flags?

Re: [PATCH 1/3] fs: fcntl add set_flags wrapper

[Dmitry Monakhov]

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

Christoph Hellwig <hch@infradead.org> writes:

>> +	else {
>> +		spin_lock(&filp->f_lock);
>> +		filp->f_flags = (arg & SETFL_MASK) |
>> +			(filp->f_flags & ~SETFL_MASK);
>> +		spin_unlock(&filp->f_lock);
>
> Please move this into an exported generic_file_set_flags helper, that
> the filesystems can use in their implementations instead of duplicating
> it.
Ok. Will do
>
> Also a more conceptual question:  Basically any check the filesystems
> may perform needs to be duplicated in open and ->set_flags.  Any chance
> to have the open path call into ->set_flags?
After your point I've checked various ->open callbacks and found
that most filesystem makes important decisions such as:
cifs_open:
        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;
                        }
         }
And I am completely agree that it is reasonable to move such
functionality to ->set_flags and let ->open call ->set_flags internally.
 ->set_flags can determine that it is called from open by condition:
(filp->f_flags & SETFL_MASK) == (arg & SETFL_MASK)
I'll fix setfl to prevent ->set_flags if args are the same

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