[PATCH] Remove fcntl f_op

[PATCH] Remove fcntl f_op

[Matthew Wilcox]

The newly introduced ->fcntl file_operation is badly thought out,
not to mention undocumented.  This patch replaces it with two better
defined operations -- check_flags and dir_notify.  Any other fcntl()s
that filesystems are interested in can have their own properly typed
f_op method when they need it.

diff -urpNX build-tools/dontdiff linux-2.6/Documentation/filesystems/Locking flock-2.6/Documentation/filesystems/Locking
--- linux-2.6/Documentation/filesystems/Locking	2004-06-21 22:50:31.000000000 -0400
+++ flock-2.6/Documentation/filesystems/Locking	2004-08-10 11:49:34.000000000 -0400
@@ -349,6 +349,8 @@ prototypes:
 			loff_t *, int);
 	unsigned long (*get_unmapped_area)(struct file *, unsigned long,
 			unsigned long, unsigned long, unsigned long);
+	int (*check_flags)(int);
+	int (*dir_notify)(struct file *, unsigned long);
 };
 
 locking rules:
@@ -375,6 +377,8 @@ writev:			no
 sendfile:		no
 sendpage:		no
 get_unmapped_area:	no
+check_flags:		no
+dir_notify:		no
 
 ->llseek() locking has moved from llseek to the individual llseek
 implementations.  If your fs is not using generic_file_llseek, you
diff -urpNX build-tools/dontdiff linux-2.6/fs/cifs/cifsfs.c flock-2.6/fs/cifs/cifsfs.c
--- linux-2.6/fs/cifs/cifsfs.c	2004-07-22 13:26:40.000000000 -0400
+++ flock-2.6/fs/cifs/cifsfs.c	2004-08-09 15:07:23.000000000 -0400
@@ -540,18 +540,14 @@ struct file_operations cifs_file_ops = {
 	.flush = cifs_flush,
 	.mmap  = cifs_file_mmap,
 	.sendfile = generic_file_sendfile,
-#ifdef CONFIG_CIFS_FCNTL
-	.fcntl = cifs_fcntl,
-#endif
+	.dir_notify = cifs_dir_notify,
 };
 
 struct file_operations cifs_dir_ops = {
 	.readdir = cifs_readdir,
 	.release = cifs_closedir,
 	.read    = generic_read_dir,
-#ifdef CONFIG_CIFS_FCNTL
-	.fcntl   = cifs_fcntl,
-#endif
+	.dir_notify = cifs_dir_notify,
 };
 
 static void
diff -urpNX build-tools/dontdiff linux-2.6/fs/cifs/cifsfs.h flock-2.6/fs/cifs/cifsfs.h
--- linux-2.6/fs/cifs/cifsfs.h	2004-07-22 13:26:40.000000000 -0400
+++ flock-2.6/fs/cifs/cifsfs.h	2004-08-09 21:43:59.000000000 -0400
@@ -78,7 +78,7 @@ extern int cifs_file_mmap(struct file * 
 extern 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);
-extern long cifs_fcntl(int, unsigned int, unsigned long, struct file *);
+extern int cifs_dir_notify(struct file *, unsigned long arg);
 
 /* Functions related to dir entries */
 extern struct dentry_operations cifs_dentry_ops;
diff -urpNX build-tools/dontdiff linux-2.6/fs/cifs/fcntl.c flock-2.6/fs/cifs/fcntl.c
--- linux-2.6/fs/cifs/fcntl.c	2004-06-21 22:50:48.000000000 -0400
+++ flock-2.6/fs/cifs/fcntl.c	2004-08-09 15:11:30.000000000 -0400
@@ -28,7 +28,7 @@
 #include "cifs_unicode.h"
 #include "cifs_debug.h"
 
-int cifs_directory_notify(unsigned long arg, struct file * file)
+int cifs_dir_notify(struct file * file, unsigned long arg)
 {
 	int xid;
 	int rc = -EINVAL;
@@ -70,53 +70,3 @@ int cifs_directory_notify(unsigned long 
 	FreeXid(xid);
 	return rc;
 }
-
-
-long cifs_fcntl(int file_desc, unsigned int command, unsigned long arg,
-				struct file * file)
-{
-	/* Few few file control functions need to be specially mapped. So far
-	only:
-		F_NOTIFY (for directory change notification)
-	And eventually:
-		F_GETLEASE
-		F_SETLEASE 
-	need to be mapped here. The others either already are mapped downstream
-	or do not need to go to the server (client only sideeffects):
-		F_DUPFD:
-		F_GETFD:
-		F_SETFD:
-		F_GETFL:
-		F_SETFL:
-		F_GETLK:
-		F_SETLK:
-		F_SETLKW:
-		F_GETOWN:
-		F_SETOWN:
-		F_GETSIG:
-		F_SETSIG:
-	*/
-	long rc = 0;
-
-	cFYI(1,("cifs_fcntl: command %d with arg %lx",command,arg)); /* BB removeme BB */
-
-	switch (command) {
-	case F_NOTIFY:
-		/* let the local call have a chance to fail first */
-		rc = generic_file_fcntl(file_desc,command,arg,file);
-		if(rc)
-			return rc;
-		else {
-			/* local call succeeded try to do remote notify to
-			pick up changes from other clients to server file */
-			cifs_directory_notify(arg, file);
-			/* BB add case to long and return rc from above */
-			return rc;
-		}
-		break;
-	default:
-		break;
-	}
-	return generic_file_fcntl(file_desc,command,arg,file);
-}
-                
diff -urpNX build-tools/dontdiff linux-2.6/fs/dnotify.c flock-2.6/fs/dnotify.c
--- linux-2.6/fs/dnotify.c	2004-07-22 13:26:39.000000000 -0400
+++ flock-2.6/fs/dnotify.c	2004-08-09 21:45:22.000000000 -0400
@@ -104,6 +104,9 @@ int fcntl_dirnotify(int fd, struct file 
 	dn->dn_next = inode->i_dnotify;
 	inode->i_dnotify = dn;
 	spin_unlock(&inode->i_lock);
+
+	if (filp->f_op && filp->f_op->dir_notify)
+		return filp->f_op->dir_notify(filp, arg);
 	return 0;
 
 out_free:
diff -urpNX build-tools/dontdiff linux-2.6/fs/fcntl.c flock-2.6/fs/fcntl.c
--- linux-2.6/fs/fcntl.c	2004-07-22 13:26:40.000000000 -0400
+++ flock-2.6/fs/fcntl.c	2004-08-09 15:00:39.000000000 -0400
@@ -239,6 +239,11 @@ static int setfl(int fd, struct file * f
 				return -EINVAL;
 	}
 
+	if (filp->f_op && filp->f_op->check_flags)
+		error = filp->f_op->check_flags(arg);
+	if (error)
+		return error;
+
 	lock_kernel();
 	if ((arg ^ filp->f_flags) & FASYNC) {
 		if (filp->f_op && filp->f_op->fasync) {
@@ -287,8 +292,8 @@ void f_delown(struct file *filp)
 
 EXPORT_SYMBOL(f_delown);
 
-long generic_file_fcntl(int fd, unsigned int cmd,
-			unsigned long arg, struct file *filp)
+static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
+		struct file *filp)
 {
 	long err = -EINVAL;
 
@@ -356,15 +359,6 @@ long generic_file_fcntl(int fd, unsigned
 	}
 	return err;
 }
-EXPORT_SYMBOL(generic_file_fcntl);
-
-static long do_fcntl(int fd, unsigned int cmd,
-			unsigned long arg, struct file *filp)
-{
-	if (filp->f_op && filp->f_op->fcntl)
-		return filp->f_op->fcntl(fd, cmd, arg, filp);
-	return generic_file_fcntl(fd, cmd, arg, filp);
-}
 
 asmlinkage long sys_fcntl(int fd, unsigned int cmd, unsigned long arg)
 {	
diff -urpNX build-tools/dontdiff linux-2.6/fs/nfs/file.c flock-2.6/fs/nfs/file.c
--- linux-2.6/fs/nfs/file.c	2004-07-22 13:26:43.000000000 -0400
+++ flock-2.6/fs/nfs/file.c	2004-08-09 14:58:00.000000000 -0400
@@ -33,8 +33,6 @@
 
 #define NFSDBG_FACILITY		NFSDBG_FILE
 
-static long nfs_file_fcntl(int fd, unsigned int cmd,
-			unsigned long arg, struct file *filp);
 static int nfs_file_open(struct inode *, struct file *);
 static int nfs_file_release(struct inode *, struct file *);
 static int  nfs_file_mmap(struct file *, struct vm_area_struct *);
@@ -43,6 +41,7 @@ static ssize_t nfs_file_read(struct kioc
 static ssize_t nfs_file_write(struct kiocb *, const char __user *, size_t, loff_t);
 static int  nfs_file_flush(struct file *);
 static int  nfs_fsync(struct file *, struct dentry *dentry, int datasync);
+static int nfs_check_flags(int flags);
 
 struct file_operations nfs_file_operations = {
 	.llseek		= remote_llseek,
@@ -57,7 +56,7 @@ struct file_operations nfs_file_operatio
 	.fsync		= nfs_fsync,
 	.lock		= nfs_lock,
 	.sendfile	= nfs_file_sendfile,
-	.fcntl		= nfs_file_fcntl,
+	.check_flags	= nfs_check_flags,
 };
 
 struct inode_operations nfs_file_inode_operations = {
@@ -71,26 +70,12 @@ struct inode_operations nfs_file_inode_o
 # define IS_SWAPFILE(inode)	(0)
 #endif
 
-#define nfs_invalid_flags	(O_APPEND | O_DIRECT)
-
-/*
- * Check for special cases that NFS doesn't support, and
- * pass the rest to the generic fcntl function.
- */
-static long
-nfs_file_fcntl(int fd, unsigned int cmd,
-		unsigned long arg, struct file *filp)
-{
-	switch (cmd) {
-	case F_SETFL:
-		if ((filp->f_flags & nfs_invalid_flags) == nfs_invalid_flags)
-			return -EINVAL;
-		break;
-	default:
-		break;
-	}
+static int nfs_check_flags(int flags)
+{
+	if (flags & (O_APPEND | O_DIRECT))
+		return -EINVAL;
 
-	return generic_file_fcntl(fd, cmd, arg, filp);
+	return 0;
 }
 
 /*
@@ -101,10 +86,11 @@ nfs_file_open(struct inode *inode, struc
 {
 	struct nfs_server *server = NFS_SERVER(inode);
 	int (*open)(struct inode *, struct file *);
-	int res = 0;
+	int res;
 
-	if ((filp->f_flags & nfs_invalid_flags) == nfs_invalid_flags)
-		return -EINVAL;
+	res = nfs_check_flags(filp->f_flags);
+	if (!res)
+		return res;
 
 	lock_kernel();
 	/* Do NFSv4 open() call */
diff -urpNX build-tools/dontdiff linux-2.6/include/linux/fs.h flock-2.6/include/linux/fs.h
--- linux-2.6/include/linux/fs.h	2004-07-22 13:26:58.000000000 -0400
+++ flock-2.6/include/linux/fs.h	2004-08-09 15:05:17.000000000 -0400
@@ -659,9 +679,6 @@ extern struct list_head file_lock_list;
 
 #include <linux/fcntl.h>
 
-extern long generic_file_fcntl(int fd, unsigned int cmd,
-				unsigned long arg, struct file *filp);
-
 extern int fcntl_getlk(struct file *, struct flock __user *);
 extern int fcntl_setlk(struct file *, unsigned int, struct flock __user *);
 
@@ -889,8 +907,8 @@ 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);
-	long (*fcntl)(int fd, unsigned int cmd,
-			unsigned long arg, struct file *filp);
+	int (*check_flags)(int);
+	int (*dir_notify)(struct file *filp, unsigned long arg);
 };
 
 struct inode_operations {

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] Remove fcntl f_op

[viro]

On Thu, Aug 12, 2004 at 11:12:29PM +0100, Matthew Wilcox wrote:
> 
> The newly introduced ->fcntl file_operation is badly thought out,
> not to mention undocumented.  This patch replaces it with two better
> defined operations -- check_flags and dir_notify.  Any other fcntl()s
> that filesystems are interested in can have their own properly typed
> f_op method when they need it.

ACK

Re: [PATCH] Remove fcntl f_op

[Andrew Morton]

Matthew Wilcox <willy@debian.org> wrote:
>
> The newly introduced ->fcntl file_operation is badly thought out,

Not really - it's very simple.

>  not to mention undocumented.

That's because it's simple.

>  This patch replaces it with two better
>  defined operations -- check_flags and dir_notify.

Except these _do_ need to be documented, and you didn't do that.


Seems like a fairly pointless patch to me but whatever - it's a trivial
matter.  Please send an updated patch which documents the new API.

Re: [PATCH] Remove fcntl f_op

[Matthew Wilcox]

On Sat, Aug 14, 2004 at 11:39:06PM -0700, Andrew Morton wrote:
> Matthew Wilcox <willy@debian.org> wrote:
> > The newly introduced ->fcntl file_operation is badly thought out,
> 
> Not really - it's very simple.

Mmm.  It's typeless, passes the fd number down to the filesystem,
and pretty much requires a switch() in every filesystem that wants
to implement it.  It's also a bad API in that it allows filesystems to
introduce private fcntls or do something completely different in response
to a standard fcntl.

> >  This patch replaces it with two better
> >  defined operations -- check_flags and dir_notify.
> 
> Except these _do_ need to be documented, and you didn't do that.
> 
> Seems like a fairly pointless patch to me but whatever - it's a trivial
> matter.  Please send an updated patch which documents the new API.

I documented them where the other file_operations are
documented, in Documentation/filesystems/Locking.

If you wanted Documentation/filesystems/vfs.txt to mention them, well,
that needs to be updated to something from this decade first -- I can't
believe anyone's actually reading that.  Just from file_operations,
it's missing aio_read, aio_write, aio_fsync, readv, writev, sendfile,
sendpage and get_unmapped_area.  It also claims check_media_change and
revalidate are file_operations, when they aren't.

If there's somewhere else to document them, I'd be only too happy to.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] Remove fcntl f_op

[Andrew Morton]

Matthew Wilcox <willy@debian.org> wrote:
>
>  I documented them where the other file_operations are
>  documented, in Documentation/filesystems/Locking.

Documentation/filesystems/Locking does document address_space_ops.  But
not, it seems, file_operations.

>  If you wanted Documentation/filesystems/vfs.txt to mention them, well,
>  that needs to be updated to something from this decade first -- I can't
>  believe anyone's actually reading that.  Just from file_operations,
>  it's missing aio_read, aio_write, aio_fsync, readv, writev, sendfile,
>  sendpage and get_unmapped_area.  It also claims check_media_change and
>  revalidate are file_operations, when they aren't.
> 
>  If there's somewhere else to document them, I'd be only too happy to.

Documentation in Documentation/ goes out of date - we've seen this again
and again and again.  Documentation in code comments gets a lot more
eyeball time, and people spot errors and omissions.

For this reason, I'd suggest that we prefer to document the code inside the
code.  External interfaces and things which are distributed all over the
tree (proc.txt, kernel-parameters.txt, etc) are more appropriately handled
in Documentation/.