fcntl method for file_operations

fcntl method for file_operations

[Steve French]

> the NFS developers have submitted a patch which adds an fcntl() method
> to the file_operations structure.

This patch should be helpful to the cifs vfs for a different reason
(than the original NFS conflict between O_DIRECT/O_APPEND) , it will
allow a way to get directory change notification F_NOTIFY (and file open
notification, F_SETLEASE, perhaps) sent across the network (currenty
many fcntl calls are meaningful for local filesystems only).

Re: fcntl method for file_operations

[Matthew Wilcox]

On Thu, Mar 25, 2004 at 12:04:54PM -0600, Steve French wrote:
> > the NFS developers have submitted a patch which adds an fcntl() method
> > to the file_operations structure.
> 
> This patch should be helpful to the cifs vfs for a different reason
> (than the original NFS conflict between O_DIRECT/O_APPEND) , it will
> allow a way to get directory change notification F_NOTIFY (and file open
> notification, F_SETLEASE, perhaps) sent across the network (currenty
> many fcntl calls are meaningful for local filesystems only).

I don't think we should have an fcntl() method.  I'd rather see a typed
set of method calls for whatever set of routines are actually useful.
Something along the lines of ethtool_ops, say ;-)

perhaps the right thing to do is just to add the following ops to
file_operations:

	int (*fcntl_setfl)(int fd, struct file *filp, unsigned long arg);
	int (*fcntl_getlease)(struct file *filp);
	int (*fcntl_setlease)(int fd, struct file *filp, unsigned long arg);
	int (*fcntl_dirnotify(int fd, struct file *filp, unsigned long arg);

comments?

-- 
"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: fcntl method for file_operations

[Jamie Lokier]

Matthew Wilcox wrote:
> perhaps the right thing to do is just to add the following ops to
> file_operations:
> 
> 	int (*fcntl_setfl)(int fd, struct file *filp, unsigned long arg);
> 	int (*fcntl_getlease)(struct file *filp);
> 	int (*fcntl_setlease)(int fd, struct file *filp, unsigned long arg);
> 	int (*fcntl_dirnotify(int fd, struct file *filp, unsigned long arg);
> 
> comments?

I agree that a generic fcntl() doesn't make a lot of sense.

However, rather than your suggestion, I'd rather
getlease/setlease/dirnotify have more appropriate structure for those
operations, be called from a generic lease/dnotify core, and be added
as and when they are actually used by a filesystem.

Otherwise I can easily see NFS dirnotify being implemented, and
dnotify.c being enhanced to support some feature like non-signal
results (DN_NOSIGNAL), ->poll() waiting, last writer close, or
whatever, and those new features fine *except* on NFS filesystems
which has it's own signal delivery code etc. copied from dnotify.c.

In other words, for dnotify, the generic dnotify code should always be
used so that the API is consistent among filesystems, but it should
call the filesystem to do the filesystem-specific part namely
registering a callback function to be called when a non-local change
is detected.

Similarly for leases, and any other wonders that are added in future
like mount notifications.

-- Jamie

Re: fcntl method for file_operations

[Trond Myklebust]

På to , 25/03/2004 klokka 14:35, skreiv Matthew Wilcox:

> I don't think we should have an fcntl() method.  I'd rather see a typed
> set of method calls for whatever set of routines are actually useful.
> Something along the lines of ethtool_ops, say ;-)
> 
> perhaps the right thing to do is just to add the following ops to
> file_operations:
> 
> 	int (*fcntl_setfl)(int fd, struct file *filp, unsigned long arg);
> 	int (*fcntl_getlease)(struct file *filp);
> 	int (*fcntl_setlease)(int fd, struct file *filp, unsigned long arg);
> 	int (*fcntl_dirnotify(int fd, struct file *filp, unsigned long arg);
> 
> comments?

This was what we first proposed (actually, just the fcntl_setfl() call),
but akpm prefers the fcntl() method.

I can see his point: fcntl() is second only to ioctl() when it comes to
interface design. If you want to provide methods/callbacks for each and
every overloaded subcase, then your "struct file_operations" can quickly
turn ugly.

Cheers,
  Trond
-
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

Re: fcntl method for file_operations

[viro]

On Thu, Mar 25, 2004 at 03:15:58PM -0500, Trond Myklebust wrote:
> I can see his point: fcntl() is second only to ioctl() when it comes to
> interface design. If you want to provide methods/callbacks for each and
> every overloaded subcase, then your "struct file_operations" can quickly
> turn ugly.

And for that very reason we don't _want_ to make it easy to add new
piles of vomit into fcntl().  ioctl() is bad enough; until now fcntl()
was somewhat limited, but typeless method will immediately open the
floodgates.

IOW, my vote is strongly against ->fcntl() - the last thing we need
is yet another multiplexor available to Joe Random Driver-Monkey.

Re: fcntl method for file_operations

[Trond Myklebust]

På to , 25/03/2004 klokka 15:31, skreiv
viro@parcelfarce.linux.theplanet.co.uk:

> And for that very reason we don't _want_ to make it easy to add new
> piles of vomit into fcntl().  ioctl() is bad enough; until now fcntl()
> was somewhat limited, but typeless method will immediately open the
> floodgates.

If/when it breaks, it will be their problem...

POSIX/SUS both allow you to play the arbitrary interface game with
ioctl(), but nowhere do they sanction it for fcntl()...

> IOW, my vote is strongly against ->fcntl() - the last thing we need
> is yet another multiplexor available to Joe Random Driver-Monkey.

Fair enough. Then I'll put that to akpm, and we'll resend our original
proposal...

Cheers,
  Trond
-
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

Re: fcntl method for file_operations

[Steve French]

On Thu, 2004-03-25 at 13:35, Matthew Wilcox wrote:
> On Thu, Mar 25, 2004 at 12:04:54PM -0600, Steve French wrote:
> > > the NFS developers have submitted a patch which adds an fcntl() method
> > > to the file_operations structure.
> > 
> > This patch should be helpful to the cifs vfs for a different reason
> > (than the original NFS conflict between O_DIRECT/O_APPEND) , it will
> > allow a way to get directory change notification F_NOTIFY (and file open
> > notification, F_SETLEASE, perhaps) sent across the network (currenty
> > many fcntl calls are meaningful for local filesystems only).
> 
> I don't think we should have an fcntl() method.  I'd rather see a typed
> set of method calls for whatever set of routines are actually useful.
> Something along the lines of ethtool_ops, say ;-)
> 
> perhaps the right thing to do is just to add the following ops to
> file_operations:
> 
> 	int (*fcntl_setfl)(int fd, struct file *filp, unsigned long arg);
> 	int (*fcntl_getlease)(struct file *filp);
> 	int (*fcntl_setlease)(int fd, struct file *filp, unsigned long arg);
> 	int (*fcntl_dirnotify(int fd, struct file *filp, unsigned long arg);
> 
> comments?

Sounds ok to me, the only minor drawback is that it would mean a few
more routines to be exported by the fs layer (generic_fcntl_dirnotify,
generic_fcntl_setlease etc.) rather than a single generic_fcntl.  I
don't know if there is a case in which filesystems would ever need to
get non-standard fcntls (or ones that fs/fcntl.c does not implement
yet).  The NFS guys suggestion would allow nfs to handled new fcntls by
filtering them before fcntl.c got to them.  Although I can't imagine any
reason why someone would invent a new fcntl call that I would need the
cifs vfs to filter, but perhaps it would allow other filesystems to
simplify fs tools that port to multiple operating system by adding
filesystem dependent fcntls (sounds like a bad idea though).

But I slightly prefer Jamie's suggestion of modifying the code farther
downstream (e.g. in dnotify to call out to the affected filesystem)
which is similar to what I had suggested a few months ago.  

There is another obvious hole in the vfs layer - to allow quotas to be
passed to network filesystems and filesystems without true block
devices.   Current sys_quotactl does	
	bdev = lookup_bdev(name)
then
	get_super(bdev)

rather than looking up the superblock directly from the name (using
user_path_walk as sys_statfs and other places in the kernel do) so that
small section of quota.c needs to be modified for non-local and
deviceless filesystems.

On Thu, 2004-03-25 at 13:55, Jamie Lokier wrote:
> In other words, for dnotify, the generic dnotify code should always be
> used so that the API is consistent among filesystems, but it should
> call the filesystem to do the filesystem-specific part namely
> registering a callback function to be called when a non-local change
> is detected.

> Similarly for leases, and any other wonders that are added in future
> like mount notifications

Re: fcntl method for file_operations

[viro]

On Thu, Mar 25, 2004 at 02:25:58PM -0600, Steve French wrote:
> passed to network filesystems and filesystems without true block
> devices.   Current sys_quotactl does	
> 	bdev = lookup_bdev(name)
> then
> 	get_super(bdev)
> 
> rather than looking up the superblock directly from the name (using
> user_path_walk as sys_statfs and other places in the kernel do) so that
> small section of quota.c needs to be modified for non-local and
> deviceless filesystems.

That's not a VFS problem - it's a bad syscall API.  Note that statfs(2)
uses pathname of mountpoint (anything on fs, actually).  quotactl(2)
uses pathname of _device_.  Which is, indeed, a bad idea, but we are
tied by user-visible API here.