[PATCH 1/2] vfs: pass 'struct file *' as parameter to ->check_flags() methods

[PATCH 1/2] vfs: pass 'struct file *' as parameter to ->check_flags() methods

[Anand Avati]

Along with corresponding changes in -

- Documentation/
- nfs
- bad_inodes.c
- fcntl.c
---
 Documentation/filesystems/Locking |    2 +-
 Documentation/filesystems/vfs.txt |    2 +-
 fs/bad_inode.c                    |    2 +-
 fs/fcntl.c                        |    2 +-
 fs/nfs/file.c                     |    6 +++---
 include/linux/fs.h                |    2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking
index 57d827d..9619841 100644
--- a/Documentation/filesystems/Locking
+++ b/Documentation/filesystems/Locking
@@ -426,7 +426,7 @@ prototypes:
 			loff_t *, int);
 	unsigned long (*get_unmapped_area)(struct file *, unsigned long,
 			unsigned long, unsigned long, unsigned long);
-	int (*check_flags)(int);
+	int (*check_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);
diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index 88b9f55..442aefb 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -764,7 +764,7 @@ struct file_operations {
 	ssize_t (*sendfile) (struct file *, loff_t *, size_t, read_actor_t, void *);
 	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 (*check_flags)(struct file *, int);
 	int (*flock) (struct file *, int, struct file_lock *);
 	ssize_t (*splice_write)(struct pipe_inode_info *, struct file *, size_t, unsigned int);
 	ssize_t (*splice_read)(struct file *, struct pipe_inode_info *, size_t, unsigned int);
diff --git a/fs/bad_inode.c b/fs/bad_inode.c
index bfcb18f..c7eef18 100644
--- a/fs/bad_inode.c
+++ b/fs/bad_inode.c
@@ -120,7 +120,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_check_flags(struct file *filp, int flags)
 {
 	return -EIO;
 }
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 22764c7..1a2a6d3 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -174,7 +174,7 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
 	}
 
 	if (filp->f_op && filp->f_op->check_flags)
-		error = filp->f_op->check_flags(arg);
+		error = filp->f_op->check_flags(filp, arg);
 	if (error)
 		return error;
 
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 2f093ed..9f96a8b 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -56,7 +56,7 @@ static ssize_t nfs_file_write(struct kiocb *, const struct iovec *iov,
 				unsigned long nr_segs, loff_t pos);
 static int  nfs_file_flush(struct file *, fl_owner_t id);
 static int  nfs_file_fsync(struct file *, int datasync);
-static int nfs_check_flags(int flags);
+static int nfs_check_flags(struct file *, int flags);
 static int nfs_lock(struct file *filp, int cmd, struct file_lock *fl);
 static int nfs_flock(struct file *filp, int cmd, struct file_lock *fl);
 static int nfs_setlease(struct file *file, long arg, struct file_lock **fl);
@@ -105,7 +105,7 @@ const struct inode_operations nfs3_file_inode_operations = {
 # define IS_SWAPFILE(inode)	(0)
 #endif
 
-static int nfs_check_flags(int flags)
+static int nfs_check_flags(struct file *filp, int flags)
 {
 	if ((flags & (O_APPEND | O_DIRECT)) == (O_APPEND | O_DIRECT))
 		return -EINVAL;
@@ -126,7 +126,7 @@ nfs_file_open(struct inode *inode, struct file *filp)
 			filp->f_path.dentry->d_name.name);
 
 	nfs_inc_stats(inode, NFSIOS_VFSOPEN);
-	res = nfs_check_flags(filp->f_flags);
+	res = nfs_check_flags(filp, filp->f_flags);
 	if (res)
 		return res;
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b5b9792..98ce7c7 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1564,7 +1564,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 (*check_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.4.4

[PATCH 2/2] fuse: permit O_DIRECT flag in open()

[Anand Avati]

but do not permit setting of O_DIRECT flag via fcntl() (or unsetting)
---
 fs/fuse/file.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 82a6646..f30a7c6 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -154,6 +154,18 @@ int fuse_do_open(struct fuse_conn *fc, u64 nodeid, struct file *file,
 		return err;
 	}
 
+        if (file->f_flags & O_DIRECT) {
+                /* set fuse_direct_io_file_operations as fops in
+                   fuse_finish_open as though the FS enforced direct_io
+                */
+                outarg.open_flags |= FOPEN_DIRECT_IO;
+
+                /* make VFS believe we don't support O_DIRECT till we
+                   implement a_ops->direct_IO
+                */
+                file->f_flags &= ~O_DIRECT;
+        }
+
 	if (isdir)
 		outarg.open_flags &= ~FOPEN_DIRECT_IO;
 
@@ -193,10 +205,6 @@ int fuse_open_common(struct inode *inode, struct file *file, bool isdir)
 	struct fuse_conn *fc = get_fuse_conn(inode);
 	int err;
 
-	/* VFS checks this, but only _after_ ->open() */
-	if (file->f_flags & O_DIRECT)
-		return -EINVAL;
-
 	err = generic_file_open(inode, file);
 	if (err)
 		return err;
@@ -2132,6 +2140,15 @@ int fuse_notify_poll_wakeup(struct fuse_conn *fc,
 	return 0;
 }
 
+
+static int fuse_check_flags(struct file *filp, int flags)
+{
+        if ((filp->f_flags ^ flags) & O_DIRECT)
+                return -EINVAL;
+	return 0;
+}
+
+
 static const struct file_operations fuse_file_operations = {
 	.llseek		= fuse_file_llseek,
 	.read		= do_sync_read,
@@ -2149,6 +2166,7 @@ static const struct file_operations fuse_file_operations = {
 	.unlocked_ioctl	= fuse_file_ioctl,
 	.compat_ioctl	= fuse_file_compat_ioctl,
 	.poll		= fuse_file_poll,
+        .check_flags    = fuse_check_flags,
 };
 
 static const struct file_operations fuse_direct_io_file_operations = {
@@ -2165,6 +2183,7 @@ static const struct file_operations fuse_direct_io_file_operations = {
 	.unlocked_ioctl	= fuse_file_ioctl,
 	.compat_ioctl	= fuse_file_compat_ioctl,
 	.poll		= fuse_file_poll,
+        .check_flags    = fuse_check_flags,
 	/* no splice_read */
 };
 
-- 
1.7.4.4

Re: [PATCH 1/2] vfs: pass 'struct file *' as parameter to ->check_flags() methods

[Miklos Szeredi]

Anand Avati <avati@gluster.com> writes:

> Along with corresponding changes in -
>
> - Documentation/
> - nfs
> - bad_inodes.c
> - fcntl.c


Patch looks good to me.

You should add some better description to the patch.  Listing the
changed files isn't needed, it's apparent from the diffstat below.
Rather you should describe why this is needed.

You should also add a "Signed-off-by:" line.

Thanks,
Miklos

> ---
>  Documentation/filesystems/Locking |    2 +-
>  Documentation/filesystems/vfs.txt |    2 +-
>  fs/bad_inode.c                    |    2 +-
>  fs/fcntl.c                        |    2 +-
>  fs/nfs/file.c                     |    6 +++---
>  include/linux/fs.h                |    2 +-
>  6 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking
> index 57d827d..9619841 100644
> --- a/Documentation/filesystems/Locking
> +++ b/Documentation/filesystems/Locking
> @@ -426,7 +426,7 @@ prototypes:
>  			loff_t *, int);
>  	unsigned long (*get_unmapped_area)(struct file *, unsigned long,
>  			unsigned long, unsigned long, unsigned long);
> -	int (*check_flags)(int);
> +	int (*check_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);
> diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
> index 88b9f55..442aefb 100644
> --- a/Documentation/filesystems/vfs.txt
> +++ b/Documentation/filesystems/vfs.txt
> @@ -764,7 +764,7 @@ struct file_operations {
>  	ssize_t (*sendfile) (struct file *, loff_t *, size_t, read_actor_t, void *);
>  	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 (*check_flags)(struct file *, int);
>  	int (*flock) (struct file *, int, struct file_lock *);
>  	ssize_t (*splice_write)(struct pipe_inode_info *, struct file *, size_t, unsigned int);
>  	ssize_t (*splice_read)(struct file *, struct pipe_inode_info *, size_t, unsigned int);
> diff --git a/fs/bad_inode.c b/fs/bad_inode.c
> index bfcb18f..c7eef18 100644
> --- a/fs/bad_inode.c
> +++ b/fs/bad_inode.c
> @@ -120,7 +120,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_check_flags(struct file *filp, int flags)
>  {
>  	return -EIO;
>  }
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 22764c7..1a2a6d3 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -174,7 +174,7 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
>  	}
>  
>  	if (filp->f_op && filp->f_op->check_flags)
> -		error = filp->f_op->check_flags(arg);
> +		error = filp->f_op->check_flags(filp, arg);
>  	if (error)
>  		return error;
>  
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index 2f093ed..9f96a8b 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -56,7 +56,7 @@ static ssize_t nfs_file_write(struct kiocb *, const struct iovec *iov,
>  				unsigned long nr_segs, loff_t pos);
>  static int  nfs_file_flush(struct file *, fl_owner_t id);
>  static int  nfs_file_fsync(struct file *, int datasync);
> -static int nfs_check_flags(int flags);
> +static int nfs_check_flags(struct file *, int flags);
>  static int nfs_lock(struct file *filp, int cmd, struct file_lock *fl);
>  static int nfs_flock(struct file *filp, int cmd, struct file_lock *fl);
>  static int nfs_setlease(struct file *file, long arg, struct file_lock **fl);
> @@ -105,7 +105,7 @@ const struct inode_operations nfs3_file_inode_operations = {
>  # define IS_SWAPFILE(inode)	(0)
>  #endif
>  
> -static int nfs_check_flags(int flags)
> +static int nfs_check_flags(struct file *filp, int flags)
>  {
>  	if ((flags & (O_APPEND | O_DIRECT)) == (O_APPEND | O_DIRECT))
>  		return -EINVAL;
> @@ -126,7 +126,7 @@ nfs_file_open(struct inode *inode, struct file *filp)
>  			filp->f_path.dentry->d_name.name);
>  
>  	nfs_inc_stats(inode, NFSIOS_VFSOPEN);
> -	res = nfs_check_flags(filp->f_flags);
> +	res = nfs_check_flags(filp, filp->f_flags);
>  	if (res)
>  		return res;
>  
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index b5b9792..98ce7c7 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1564,7 +1564,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 (*check_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);

Re: [PATCH 2/2] fuse: permit O_DIRECT flag in open()

[Miklos Szeredi]

Anand Avati <avati@gluster.com> writes:

> but do not permit setting of O_DIRECT flag via fcntl() (or unsetting)

Again, some more verbose description would be good, as well as a
Signed-off-by.

> ---
>  fs/fuse/file.c |   27 +++++++++++++++++++++++----
>  1 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 82a6646..f30a7c6 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -154,6 +154,18 @@ int fuse_do_open(struct fuse_conn *fc, u64 nodeid, struct file *file,
>  		return err;
>  	}
>  
> +        if (file->f_flags & O_DIRECT) {

Please use tabs, not spaces.

Before submission you should run your patch through
scripts/checkpatch.pl to filter out such problems.

> +                /* set fuse_direct_io_file_operations as fops in
> +                   fuse_finish_open as though the FS enforced direct_io
> +                */
> +                outarg.open_flags |= FOPEN_DIRECT_IO;

As I said earlier, I don't like this.  Rather the userspace filesystem
should set FOPEN_DIRECT_IO on O_DIRECT if it wants to.

And if the userspace filesystem has not set FOPEN_DIRECT_IO and O_DIRECT
is set then we should return -EINVAL.

Thanks,
Miklos

> +
> +                /* make VFS believe we don't support O_DIRECT till we
> +                   implement a_ops->direct_IO
> +                */
> +                file->f_flags &= ~O_DIRECT;
> +        }
> +
>  	if (isdir)
>  		outarg.open_flags &= ~FOPEN_DIRECT_IO;
>  
> @@ -193,10 +205,6 @@ int fuse_open_common(struct inode *inode, struct file *file, bool isdir)
>  	struct fuse_conn *fc = get_fuse_conn(inode);
>  	int err;
>  
> -	/* VFS checks this, but only _after_ ->open() */
> -	if (file->f_flags & O_DIRECT)
> -		return -EINVAL;
> -
>  	err = generic_file_open(inode, file);
>  	if (err)
>  		return err;
> @@ -2132,6 +2140,15 @@ int fuse_notify_poll_wakeup(struct fuse_conn *fc,
>  	return 0;
>  }
>  
> +
> +static int fuse_check_flags(struct file *filp, int flags)
> +{
> +        if ((filp->f_flags ^ flags) & O_DIRECT)
> +                return -EINVAL;
> +	return 0;
> +}
> +
> +
>  static const struct file_operations fuse_file_operations = {
>  	.llseek		= fuse_file_llseek,
>  	.read		= do_sync_read,
> @@ -2149,6 +2166,7 @@ static const struct file_operations fuse_file_operations = {
>  	.unlocked_ioctl	= fuse_file_ioctl,
>  	.compat_ioctl	= fuse_file_compat_ioctl,
>  	.poll		= fuse_file_poll,
> +        .check_flags    = fuse_check_flags,
>  };
>  
>  static const struct file_operations fuse_direct_io_file_operations = {
> @@ -2165,6 +2183,7 @@ static const struct file_operations fuse_direct_io_file_operations = {
>  	.unlocked_ioctl	= fuse_file_ioctl,
>  	.compat_ioctl	= fuse_file_compat_ioctl,
>  	.poll		= fuse_file_poll,
> +        .check_flags    = fuse_check_flags,
>  	/* no splice_read */
>  };

Re: [PATCH 2/2] fuse: permit O_DIRECT flag in open()

[Stef Bon]

I do not like it. Setting O_DIRECT to the flags is an option which
should be set in the filesystem, not in the general library.

I'm working on a overlay fs with various backends, like a normal (part
of) partition on a ATA harddisk, a USB stick, a Cdrom, a mounted SMB
share.... and it is able to set the O_DIRECT flag, but always per
backend.

Stef

2011/7/22 Miklos Szeredi <miklos@szeredi.hu>:
> Anand Avati <avati@gluster.com> writes:
>
>> but do not permit setting of O_DIRECT flag via fcntl() (or unsetting)
>
> Again, some more verbose description would be good, as well as a