vfs lease api
Subsystems (17)
Eventually we want to be able to support NFSv4 delegations for cluster filesystem exports. We implement NFSv4 delegations using leases. So to make this work, we need leases to be passed down to the filesystem, so that a cluster filesystem can enforce leases correctly across all nodes.
The following patches do some minor cleanup of the lease code, and then add a setlease() file method.
For now the only implementations of setlease() we include (for NFS and GFS2) are used only to turn off leases selectively. To complete this work we'll need to implement proper lease support for GFS2 or OCFS2, and probably do some more work on lease breaking.
But we're requesting this be included in -mm now in the theory that the lease-disabling behavior is useful now anyway, and in hopes that this will the required further work.
I'm also keeping this in the "for-mm" branch at:
git://linux-nfs.org/~bfields/linux.git for-mm
(And maybe you could also remove the server-cluster-locking-api branch from your list of git fetches, if it's still there?--I don't intend to use it any more.)
--b.
PATCH 1/6 locks: share more common lease code
From: J. Bruce Fields <[email protected]>
Share more code between setlease (used by nfsd) and fcntl.
Also some minor cleanup.
--- fs/locks.c | 30 ++++++++++-------------------- 1 files changed, 10 insertions(+), 20 deletions(-)
fs/locks.c +10 -20
@@ -1346,6 +1346,14 @@ static int __setlease(struct file *filp, long arg, struct file_lock **flp)struct inode *inode = dentry->d_inode;int error, rdlease_count =0 , wrlease_count =0 ; +if ((current->fsuid != inode->i_uid) && !capable (CAP_LEASE )) +return -EACCES ; +if (!S_ISREG (inode->i_mode)) +return -EINVAL ; + error =security_file_lock (filp, arg); +if (error) +return error; +time_out_leases (inode); error = -EINVAL ;
@@ -1431,18 +1439,8 @@ out:int setlease (struct file *filp,long arg,struct file_lock **lease) { -struct dentry *dentry = filp->f_path.dentry; -struct inode *inode = dentry->d_inode;int error; -if ((current->fsuid != inode->i_uid) && !capable (CAP_LEASE )) -return -EACCES ; -if (!S_ISREG (inode->i_mode)) -return -EINVAL ; - error =security_file_lock (filp, arg); -if (error) -return error; -lock_kernel (); error =__setlease (filp, arg, lease);unlock_kernel ();
@@ -1469,14 +1467,6 @@ int fcntl_setlease(unsigned int fd, struct file *filp, long arg)struct inode *inode = dentry->d_inode;int error; -if ((current->fsuid != inode->i_uid) && !capable (CAP_LEASE )) -return -EACCES ; -if (!S_ISREG (inode->i_mode)) -return -EINVAL ; - error =security_file_lock (filp, arg); -if (error) -return error; -locks_init_lock (&fl); error =lease_init (filp, arg, &fl);if (error)
@@ -1490,9 +1480,9 @@ int fcntl_setlease(unsigned int fd, struct file *filp, long arg) error =fasync_helper (fd, filp,1 , &flp->fl_fasync);if (error <0 ) { -/* remove lease just inserted by __setlease */ +/* remove lease just inserted by setlease */ flp->fl_type =F_UNLCK |F_INPROGRESS ; - flp->fl_break_time = jiffies-10 ; + flp->fl_break_time = jiffies -10 ;time_out_leases (inode); goto out_unlock; }
Signature · 1 line
1.5.2.58.g98ee
PATCH 2/6 locks: provide a file lease method enabling cluster-coherent leases
From: J. Bruce Fields <[email protected]>
Currently leases are only kept locally, so there's no way for a distributed filesystem to enforce them against multiple clients. We're particularly interested in the case of nfsd exporting a cluster filesystem, in which case nfsd needs cluster-coherent leases in order to implement delegations correctly.
--- fs/locks.c | 7 +++++-- include/linux/fs.h | 1 + 2 files changed, 6 insertions(+), 2 deletions(-)
fs/locks.c +5 -2
@@ -1442,7 +1442,10 @@ int setlease(struct file *filp, long arg, struct file_lock **lease)int error;lock_kernel (); - error =__setlease (filp, arg, lease); +if (filp->f_op && filp->f_op->setlease) + error = filp->f_op->setlease (filp, arg, lease); +else + error =__setlease (filp, arg, lease);unlock_kernel ();return error;
@@ -1474,7 +1477,7 @@ int fcntl_setlease(unsigned int fd, struct file *filp, long arg)lock_kernel (); - error =__setlease (filp, arg, &flp); + error =setlease (filp, arg, &flp);if (error || arg ==F_UNLCK ) goto out_unlock;
include/linux/fs.h +1 -0
@@ -1113,6 +1113,7 @@ struct file_operations {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 ); +int (*setlease)(struct file *,long ,struct file_lock **); };struct inode_operations {
Signature · 1 line
1.5.2.58.g98ee
PATCH 3/6 locks: rename lease functions to reflect locks.c conventions
From: J. Bruce Fields <[email protected]>
We've been using the convention that vfs_foo is the function that calls a filesystem-specific foo method if it exists, or falls back on a generic method if it doesn't.
So rename setlease to vfs_setlease, and __setlease to setlease. Keep setlease exported to allow filesystems to use the generic method in addition to doing their own bookkeeping.
--- fs/locks.c | 14 +++++++------- fs/nfsd/nfs4state.c | 10 +++++----- include/linux/fs.h | 1 + 3 files changed, 13 insertions(+), 12 deletions(-)
fs/locks.c +7 -7
@@ -1329,7 +1329,7 @@ int fcntl_getlease(struct file *filp) }/** -* __setlease - sets a lease on an open file +* setlease - sets a lease on an open file * @filp: file pointer * @arg: type of lease to obtain * @flp: input - file_lock to use, output - file_lock inserted
@@ -1339,7 +1339,7 @@ int fcntl_getlease(struct file *filp)* * Called with kernel lock held. */ -static int __setlease (struct file *filp,long arg,struct file_lock **flp) +int setlease (struct file *filp,long arg,struct file_lock **flp) {struct file_lock *fl, **before, **my_before =NULL , *lease;struct dentry *dentry = filp->f_path.dentry;
@@ -1428,7 +1428,7 @@ out: }/** -* setlease - sets a lease on an open file +* vfs_setlease - sets a lease on an open file * @filp: file pointer * @arg: type of lease to obtain * @lease: file_lock to use
@@ -1437,7 +1437,7 @@ out:* The fl_lmops fl_break function is required by break_lease */ -int setlease (struct file *filp,long arg,struct file_lock **lease) +int vfs_setlease (struct file *filp,long arg,struct file_lock **lease) {int error;
@@ -1445,13 +1445,13 @@ int setlease(struct file *filp, long arg, struct file_lock **lease)if (filp->f_op && filp->f_op->setlease) error = filp->f_op->setlease (filp, arg, lease);else - error =__setlease (filp, arg, lease); + error =setlease (filp, arg, lease);unlock_kernel ();return error; } -EXPORT_SYMBOL (setlease); +EXPORT_SYMBOL (vfs_setlease);/** * fcntl_setlease - sets a lease on an open file
@@ -1477,7 +1477,7 @@ int fcntl_setlease(unsigned int fd, struct file *filp, long arg)lock_kernel (); - error =setlease (filp, arg, &flp); + error =vfs_setlease (filp, arg, &flp);if (error || arg ==F_UNLCK ) goto out_unlock;
fs/nfsd/nfs4state.c +5 -5
@@ -251,7 +251,7 @@ nfs4_close_delegation(struct nfs4_delegation *dp)/* The following nfsd_close may not actually close the file, * but we want to remove the lease in any case. */ if (dp->dl_flock) -setlease (filp,F_UNLCK , &dp->dl_flock); +vfs_setlease (filp,F_UNLCK , &dp->dl_flock);nfsd_close (filp); }
@@ -1396,7 +1396,7 @@ void nfsd_release_deleg_cb(struct file_lock *fl)/* * Set the delegation file_lock back pointer. * -* Called from __setlease() with lock_kernel() held. +* Called from setlease() with lock_kernel() held. */ static void nfsd_copy_lock_deleg_cb (struct file_lock *new,struct file_lock *fl)
@@ -1410,7 +1410,7 @@ void nfsd_copy_lock_deleg_cb(struct file_lock *new, struct file_lock *fl) }/* -* Called from __setlease() with lock_kernel() held +* Called from setlease() with lock_kernel() held */ static int nfsd_same_client_deleg_cb (struct file_lock *onlist,struct file_lock *try)
@@ -1710,10 +1710,10 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open, struct nfs4_sta fl.fl_file = stp->st_vfs_file; fl.fl_pid = current->tgid; -/* setlease checks to see if delegation should be handed out. +/* vfs_setlease checks to see if delegation should be handed out. * the lock_manager callbacks fl_mylease and fl_change are used */ -if ((status =setlease (stp->st_vfs_file, +if ((status =vfs_setlease (stp->st_vfs_file, flag ==NFS4_OPEN_DELEGATE_READ ?F_RDLCK :F_WRLCK , &flp))) {dprintk ("NFSD: setlease failed [%d], no delegation\n" , status);unhash_delegation (dp);
include/linux/fs.h +1 -0
@@ -866,6 +866,7 @@ extern int flock_lock_file_wait(struct file *filp, struct file_lock *fl);extern int __break_lease (struct inode *inode,unsigned int flags);extern void lease_get_mtime (struct inode *,struct timespec *time);extern int setlease (struct file *,long ,struct file_lock **); +extern int vfs_setlease (struct file *,long ,struct file_lock **);extern int lease_modify (struct file_lock **,int );extern int lock_may_read (struct inode *,loff_t start,unsigned long count);extern int lock_may_write (struct inode *,loff_t start,unsigned long count);
Signature · 1 line
1.5.2.58.g98ee
PATCH 4/6 locks: fix locks.c lease symbol exports
From: J. Bruce Fields <[email protected]>
Bring lease exports into line with conventions for posix locks:
setlease() should be exported so filesystems can use it to implement
their lease methods.
vfs_setlease() need only be GPL-exported since only nfsd
needs it.
--- fs/locks.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
fs/locks.c +3 -1
@@ -1427,6 +1427,8 @@ out:return error; } +EXPORT_SYMBOL (setlease); +/** * vfs_setlease - sets a lease on an open file * @filp: file pointer
@@ -1451,7 +1453,7 @@ int vfs_setlease(struct file *filp, long arg, struct file_lock **lease)return error; } -EXPORT_SYMBOL (vfs_setlease); +EXPORT_SYMBOL_GPL (vfs_setlease);/** * fcntl_setlease - sets a lease on an open file
Signature · 1 line
1.5.2.58.g98ee
PATCH 5/6 gfs2: stop giving out non-cluster-coherent leases
From: Marc Eshel <[email protected]>
Since gfs2 can't prevent conflicting opens or leases on other nodes, we probably shouldn't allow it to give out leases at all.
Put the newly defined lease operation into use in gfs2 by turning off lease, unless we're using the "nolock' locking module (in which case all locking is local anyway).
--- fs/gfs2/ops_file.c | 26 ++++++++++++++++++++++++++ 1 files changed, 26 insertions(+), 0 deletions(-)
fs/gfs2/ops_file.c +26 -0
@@ -489,6 +489,31 @@ static int gfs2_fsync(struct file *file, struct dentry *dentry, int datasync) }/** +* gfs2_setlease - acquire/release a file lease +* @file: the file pointer +* @arg: lease type +* @fl: file lock +* +* Returns: errno +*/ + +static int gfs2_setlease (struct file *file,long arg,struct file_lock **fl) +{ +struct gfs2_sbd *sdp =GFS2_SB (file->f_mapping->host); +int ret = -EOPNOTSUPP ; + +if (sdp->sd_args.ar_localflocks) { +return setlease (file, arg, fl); + } + +/* For now fail the delegation request. Cluster file system can not +allow any node in the cluster to get a local lease until it can +be managed centrally by the cluster file system. +*/ +return ret; +} + +/** * gfs2_lock - acquire/release a posix lock on a file * @file: the file pointer * @cmd: either modify or retrieve lock state, possibly wait
@@ -639,6 +664,7 @@ const struct file_operations gfs2_file_fops = { .flock = gfs2_flock, .splice_read = generic_file_splice_read, .splice_write = generic_file_splice_write, + .setlease = gfs2_setlease, };const struct file_operations gfs2_dir_fops = {
Signature · 1 line
1.5.2.58.g98ee
PATCH 6/6 nfs: disable leases over NFS
From: J. Bruce Fields <[email protected]>
As Peter Staubach says elsewhere
(http://marc.info/?l=linux-kernel&m=118113649526444&w=2):
Quoted 9 lines
The problem is that some file system such as NFSv2 and NFSv3 do not have sufficient support to be able to support leases correctly. In particular for these two file systems, there is no over the wire protocol support.
Currently, these two file systems fail the
fcntl(F_SETLEASE)call accidentally, due to a reference counting difference. These file systems should fail more consciously, with a proper error to indicate that the call is invalid for them.
Define an nfs setlease method that just returns -EOPNOTSUPP.
If someone can demonstrate a real need, perhaps we could reenable them in the presence of the "nolock" mount option.
--- fs/nfs/file.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
fs/nfs/file.c +7 -0
@@ -51,6 +51,7 @@ static int nfs_fsync(struct file *, struct dentry *dentry, int datasync);static int nfs_check_flags (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);const struct file_operations nfs_file_operations = { .llseek = nfs_file_llseek,
@@ -67,6 +68,7 @@ const struct file_operations nfs_file_operations = { .flock = nfs_flock, .sendfile = nfs_file_sendfile, .check_flags = nfs_check_flags, + .setlease = nfs_setlease, };const struct inode_operations nfs_file_inode_operations = {
@@ -555,3 +557,8 @@ static int nfs_flock(struct file *filp, int cmd, struct file_lock *fl)return do_unlk (filp, cmd, fl);return do_setlk (filp, cmd, fl); } + +static int nfs_setlease (struct file *file,long arg,struct file_lock **fl) +{ +return -EOPNOTSUPP ; +}
Signature · 1 line
1.5.2.58.g98ee
Re: [PATCH 6/6] nfs: disable leases over NFS
Quoted 51 lines · J. Bruce Fields
From: J. Bruce Fields <[email protected]>
As Peter Staubach says elsewhere
(http://marc.info/?l=linux-kernel&m=118113649526444&w=2):Quoted 10 lines
The problem is that some file system such as NFSv2 and NFSv3 do not have sufficient support to be able to support leases correctly. In particular for these two file systems, there is no over the wire protocol support.
Currently, these two file systems fail the
fcntl(F_SETLEASE)call accidentally, due to a reference counting difference. These file systems should fail more consciously, with a proper error to indicate that the call is invalid for them.Define an nfs setlease method that just returns
-EOPNOTSUPP.If someone can demonstrate a real need, perhaps we could reenable them in the presence of the "nolock" mount option.
Signed-off-by "J. Bruce Fields" <[email protected]>Cc Peter Staubach <[email protected]>Cc Trond Myklebust <[email protected]>--- fs/nfs/file.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)fs/nfs/file.c +2
-0@@ -51,6 +51,7 @@ static int nfs_fsync(struct file *, struct dentry *dentry, int datasync);static int nfs_check_flags (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);const struct file_operations nfs_file_operations = { .llseek = nfs_file_llseek,@@ -67,6 +68,7 @@ const struct file_operations nfs_file_operations = { .flock = nfs_flock, .sendfile = nfs_file_sendfile, .check_flags = nfs_check_flags, + .setlease = nfs_setlease, };const struct inode_operations nfs_file_inode_operations = {
fs/nfs/file.c static int nfs_flock(struct file *filp, int cmd, struct file_lock *fl) · line 564
Quoted 8 lines
fs/nfs/file.c +5
-0@@ -555,3 +557,8 @@ static int nfs_flock(struct file *filp, int cmd, struct file_lock *fl)return do_unlk (filp, cmd, fl);return do_setlk (filp, cmd, fl); } + +static int nfs_setlease (struct file *file,long arg,struct file_lock **fl) +{ +return -EOPNOTSUPP ; +}
A couple of things --
First, there is already some support to disable leases for NFS mounted file systems in -mm, I think. Are you planning on removing it?
Second, it seems to me that EINVAL would be a better error to return than EOPNOTSUPP. This is an invalid operation to apply to this file and might match POSIX style specs better.
Thanx...
ps
Re: [PATCH 6/6] nfs: disable leases over NFS
Quoted 2 lines · Peter Staubach
First, there is already some support to disable leases for NFS mounted file systems in -mm, I think.
Oops, sorry; my fault for not checking -mm before sending....
Quoted 1 line
Are you planning on removing it?
I'd rather do that, yes. Any objection?
Quoted 3 lines
Second, it seems to me that EINVAL would be a better error to return than EOPNOTSUPP. This is an invalid operation to apply to this file and might match POSIX style specs better.
I'm not sure what you mean by "might match POSIX style specs better"?
Quoted 1 line
From a quick check, other reasons we'd get EINVAL in this case:
- attempt to get a lease on something other than a regular file.
- leases disabled with /proc/sys/fs/leases-enable
So if the application calling fcntl knows it was calling it on a regular file, then with your proposal an EINVAL return would mean leases were disabled for one reason or another, and it could take that as a sign to fall back on some other behavior. And I can't see any reason it would need to distinguish between those two remaining cases (filesystem doesn't support leases, or leases are disabled by the sysctl). So, OK, EINVAL sounds fine to me.
But I don't have a really strong opinion. I think the suggestion of EOPNOTSUPP was from Steven Whitehouse; Steven, do you care?
--b.
Re: [PATCH 6/6] nfs: disable leases over NFS
Hi,
Quoted 29 lines · J. Bruce Fields
Quoted 2 lines · Peter Staubach
First, there is already some support to disable leases for NFS mounted file systems in -mm, I think.
Oops, sorry; my fault for not checking -mm before sending....
Quoted 1 line
Are you planning on removing it?
I'd rather do that, yes. Any objection?
Quoted 3 lines
Second, it seems to me that EINVAL would be a better error to return than EOPNOTSUPP. This is an invalid operation to apply to this file and might match POSIX style specs better.
I'm not sure what you mean by "might match POSIX style specs better"?
From a quick check, other reasons we'd get EINVAL in this case:
- attempt to get a lease on something other than a regular file.
- leases disabled with/proc/sys/fs/leases-enableSo if the application calling fcntl knows it was calling it on a regular file, then with your proposal an EINVAL return would mean leases were disabled for one reason or another, and it could take that as a sign to fall back on some other behavior. And I can't see any reason it would need to distinguish between those two remaining cases (filesystem doesn't support leases, or leases are disabled by the sysctl). So, OK, EINVAL sounds fine to me.
Quoted 4 lines
But I don't have a really strong opinion. I think the suggestion of EOPNOTSUPP was from Steven Whitehouse; Steven, do you care?
--b.
EINVAL is fine by me, just so long as its not EAGAIN then it gets my blessing :-)
Steve.
Re: [PATCH 6/6] nfs: disable leases over NFS
Quoted 2 lines · Steven Whitehouse
EINVAL is fine by me, just so long as its not EAGAIN then it gets my blessing :-)
OK. I've changed the error return, in both the NFS and GFS2 cases, did some minor cleanup and commenting while I was at it, and pushed the results out to "for-mm":
git://linux-nfs.org/~bfields/linux.git for-mm
--b.
fs/gfs2/ops_file.c +7 -9
@@ -500,17 +500,15 @@ static int gfs2_fsync(struct file *file, struct dentry *dentry, int datasync)static int gfs2_setlease (struct file *file,long arg,struct file_lock **fl) {struct gfs2_sbd *sdp =GFS2_SB (file->f_mapping->host); -int ret = -EOPNOTSUPP ; -if (sdp->sd_args.ar_localflocks) { +/* +* We don't currently have a way to enforce a lease across the whole +* cluster; until we do, disable leases (by just returning -EINVAL), +* unless the administrator has requested purely local locking. +*/ +if (sdp->sd_args.ar_localflocks)return setlease (file, arg, fl); - } - -/* For now fail the delegation request. Cluster file system can not -allow any node in the cluster to get a local lease until it can -be managed centrally by the cluster file system. -*/ -return ret; +return -EINVAL ; }/**
fs/nfs/file.c +6 -1
@@ -560,5 +560,10 @@ 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) { -return -EOPNOTSUPP ; +/* +* There is no protocol support for leases, so we have no way +* to implement them correctly in the face of opens by other +* clients. +*/ +return -EINVAL ; }
A message that is not in the archive (sent off-list or not yet received): 6e0beaf3e950494a6903571f0b5c9b61fc7bf650.1183143819.git.bfields@citi.umich.edu
Re: [PATCH 1/6] locks: share more common lease code
Quoted 5 lines · J. Bruce Fields
From: J. Bruce Fields <[email protected]>
Share more code between setlease (used by nfsd) and fcntl.
Also some minor cleanup.
Looks good. Fine for mainline just after 2.6.23 opens.
Re: [PATCH 1/6] locks: share more common lease code
Quoted 8 lines · Christoph Hellwig
Quoted 5 lines · J. Bruce Fields
From: J. Bruce Fields <[email protected]>
Share more code between setlease (used by nfsd) and fcntl.
Also some minor cleanup.
Looks good. Fine for mainline just after 2.6.23 opens.
Thanks. (And, by the way, would it be helpful for me to translate this kind of statement into an "acked-by: Christoph..." on the eventual patch?)
--b.
Re: [PATCH 1/6] locks: share more common lease code
Quoted 6 lines · J. Bruce Fields
Quoted 5 lines · Christoph Hellwig
Quoted 4 lines · J. Bruce Fields
From: J. Bruce Fields <[email protected]>
Share more code between setlease (used by nfsd) and fcntl.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Also some minor cleanup.
Looks good. Fine for mainline just after 2.6.23 opens.
Thanks. (And, by the way, would it be helpful for me to translate this kind of statement into an "acked-by: Christoph..." on the eventual patch?)
Fine with me.
A message that is not in the archive (sent off-list or not yet received): dc828b771d2a4b78d59bdbe3c583b81887205cab.1183143820.git.bfields@citi.umich.edu
Re: [PATCH 3/6] locks: rename lease functions to reflect locks.c conventions
Quoted 9 lines · J. Bruce Fields
From: J. Bruce Fields <[email protected]>
We've been using the convention that
vfs_foois the function that calls a filesystem-specific foo method if it exists, or falls back on a generic method if it doesn't.So rename setlease to
vfs_setlease, and__setleaseto setlease. Keep setlease exported to allow filesystems to use the generic method in addition to doing their own bookkeeping.
This should be moved before patch 2 and can probably go in once 2.6.23 opens aswell.
fs/locks.c int fcntl_getlease(struct file *filp) · line 1342
Quoted 6 lines
fs/locks.c +1
-1@@ -1339,7 +1339,7 @@ int fcntl_getlease(struct file *filp)* * Called with kernel lock held. */ -static int __setlease (struct file *filp,long arg,struct file_lock **flp) +int setlease (struct file *filp,long arg,struct file_lock **flp)
Except this shouldn't be made non-static in this patch.
include/linux/fs.h extern int flock_lock_file_wait(struct file *filp, struct file_lock *fl); · line 868
Quoted 1 line
include/linux/fs.h +0
-0extern int setlease (struct file *,long ,struct file_lock **);
And this prototype shouldn't be kept. If you want to use it somewhere later in the patch series make it non-static and export it in a separate patch. If it's ever intended to be export it should probably get a better name aswell, e.g. generic_setlease.
Re: [PATCH 3/6] locks: rename lease functions to reflect locks.c conventions
Quoted 23 lines · Christoph Hellwig
Quoted 9 lines · J. Bruce Fields
From: J. Bruce Fields <[email protected]>
We've been using the convention that
vfs_foois the function that calls a filesystem-specific foo method if it exists, or falls back on a generic method if it doesn't.So rename setlease to
vfs_setlease, and__setleaseto setlease. Keep setlease exported to allow filesystems to use the generic method in addition to doing their own bookkeeping.This should be moved before patch 2 and can probably go in once 2.6.23 opens aswell.
fs/locks.c int fcntl_getlease(struct file *filp) · line 1342
Quoted 6 lines
fs/locks.c +1
-1@@ -1339,7 +1339,7 @@ int fcntl_getlease(struct file *filp)* * Called with kernel lock held. */ -static int __setlease (struct file *filp,long arg,struct file_lock **flp) +int setlease (struct file *filp,long arg,struct file_lock **flp)Except this shouldn't be made non-static in this patch.
Quoted 6 lines
include/linux/fs.h extern int flock_lock_file_wait(struct file *filp, struct file_lock *fl); · line 868
Quoted 1 line
include/linux/fs.h +0
-0extern int setlease (struct file *,long ,struct file_lock **);And this prototype shouldn't be kept. If you want to use it somewhere later in the patch series make it non-static and export it in a separate patch. If it's ever intended to be export it should probably get a better name aswell, e.g.
generic_setlease.
OK, done.--b.
A message that is not in the archive (sent off-list or not yet received): b057dca3f8acb125eccdce3a3b84ff04713fea7c.1183143820.git.bfields@citi.umich.edu
Re: [PATCH 4/6] locks: fix locks.c lease symbol exports
Quoted 7 lines · J. Bruce Fields
From: J. Bruce Fields <[email protected]>
Bring lease exports into line with conventions for posix locks:
setlease()should be exported so filesystems can use it to implement
their lease methods.
vfs_setlease()need only be GPL-exported since only nfsd
needs it.
The latter should be merged in the previous patch, the first should be a patch with making it non-static and exporting it. (after the rename of it is merged into the patch that introduces the vfs_setlease name)
Re: [PATCH 4/6] locks: fix locks.c lease symbol exports
Quoted 9 lines · Christoph Hellwig
Quoted 7 lines · J. Bruce Fields
From: J. Bruce Fields <[email protected]>
Bring lease exports into line with conventions for posix locks:
setlease()should be exported so filesystems can use it to implement
their lease methods.
vfs_setlease()need only be GPL-exported since only nfsd
needs it.
Quoted 4 lines
The latter should be merged in the previous patch, the first should be a patch with making it non-static and exporting it. (after the rename of it is merged into the patch that introduces the
vfs_setleasename)
Done.--b.
A message that is not in the archive (sent off-list or not yet received): ce5021881e67029f0e3d6f24109cf2953a0edcd1.1183143820.git.bfields@citi.umich.edu
Re: [PATCH 6/6] nfs: disable leases over NFS
Quoted 15 lines · J. Bruce Fields
From: J. Bruce Fields <[email protected]>
As Peter Staubach says elsewhere
(http://marc.info/?l=linux-kernel&m=118113649526444&w=2):Quoted 9 lines
The problem is that some file system such as NFSv2 and NFSv3 do not have sufficient support to be able to support leases correctly. In particular for these two file systems, there is no over the wire protocol support.
Currently, these two file systems fail the
fcntl(F_SETLEASE)call accidentally, due to a reference counting difference. These file systems should fail more consciously, with a proper error to indicate that the call is invalid for them.
Quoted 4 lines
Define an nfs setlease method that just returns
-EOPNOTSUPP.If someone can demonstrate a real need, perhaps we could reenable them in the presence of the "nolock" mount option.
I'm not a big fan of default methods that do the wrong thing instead of just missing functionality. Would you mind just returning -EOPNOTSUPP if ->setlease is not implemented and add it to all the local filesystems while all the network/distributed filesystems should not have it, not just nfs.
Re: [PATCH 6/6] nfs: disable leases over NFS
Quoted 18 lines · Christoph Hellwig
Quoted 17 lines · J. Bruce Fields
From: J. Bruce Fields <[email protected]>
As Peter Staubach says elsewhere
(http://marc.info/?l=linux-kernel&m=118113649526444&w=2):Quoted 9 lines
The problem is that some file system such as NFSv2 and NFSv3 do not have sufficient support to be able to support leases correctly. In particular for these two file systems, there is no over the wire protocol support.
Currently, these two file systems fail the
fcntl(F_SETLEASE)call accidentally, due to a reference counting difference. These file systems should fail more consciously, with a proper error to indicate that the call is invalid for them.Define an nfs setlease method that just returns
-EOPNOTSUPP.
Quoted 8 lines
Quoted 2 lines
If someone can demonstrate a real need, perhaps we could reenable them in the presence of the "nolock" mount option.
I'm not a big fan of default methods that do the wrong thing instead of just missing functionality. Would you mind just returning
-EOPNOTSUPPif ->setlease is not implemented and add it to all the local filesystems while all the network/distributed filesystems should not have it, not just nfs.
OK, I think that may make sense, but... wow does linux ever have a lot of obscure filesystems. This might take me a little longer.
--b.
Re: [PATCH 6/6] nfs: disable leases over NFS
In reply to Christoph HellwigQuoted 11 lines · Christoph Hellwig
Quoted 4 lines · J. Bruce Fields
Define an nfs setlease method that just returns
-EOPNOTSUPP.If someone can demonstrate a real need, perhaps we could reenable them in the presence of the "nolock" mount option.
I'm not a big fan of default methods that do the wrong thing instead of just missing functionality. Would you mind just returning
-EOPNOTSUPPif ->setlease is not implemented and add it to all the local filesystems while all the network/distributed filesystems should not have it, not just nfs.
OK, after looking at this a little more, I'm less happy about the idea of erroring out by default:
- There are a ton of filesystems that probably should allow leases, and only a few (network filesystems) that shouldn't, so leaving leases on by default seems simpler.
- We already fall back on the local method by default in the case of locks, and I don't see a reason to treat leases differently.
- The patch to add
.setlease = setlease,
to all the file_operations is going to be a big patch that changes behavior in a way that might be easy to miss (because it changes behavior exactly on those filesystems it *doesn't* touch.) I think it'll be easier to get better review on the patch that adds a method just to those filesystems that we're disabling leases for.
I agree about dealing with the other network filesystems, though, and not just NFS and GFS2; I'll look into that.
--b.
Re: [PATCH 6/6] nfs: disable leases over NFS
Quoted 6 lines · J. Bruce Fields
OK, after looking at this a little more, I'm less happy about the idea of erroring out by default:
- There are a ton of filesystems that probably should allow leases, and only a few (network filesystems) that shouldn't, so leaving leases on by default seems simpler.
But it gets you possible wrong behaviour by default. I'm not a big fan of non-trivial default methods as you see :)
Quoted 10 lines
- We already fall back on the local method by default in the case of locks, and I don't see a reason to treat leases differently.
- The patch to add
.setlease = setlease,
to all thefile_operationsis going to be a big patch that changes behavior in a way that might be easy to miss (because it changes behavior exactly on those filesystems it *doesn't* touch.) I think it'll be easier to get better review on the patch that adds a method just to those filesystems that we're disabling leases for.
Anyway, feel free to go ahead with the simpler version for now, I'll do the switchover for locks and leases when I get some time.
Re: [PATCH 6/6] nfs: disable leases over NFS
Quoted 10 lines · Christoph Hellwig
Quoted 6 lines · J. Bruce Fields
OK, after looking at this a little more, I'm less happy about the idea of erroring out by default:
- There are a ton of filesystems that probably should allow leases, and only a few (network filesystems) that shouldn't, so leaving leases on by default seems simpler.
But it gets you possible wrong behaviour by default. I'm not a big fan of non-trivial default methods as you see :)
Yeah, I do understand the attraction of doing it your way. With some quick grepping, what I found was:
- about 28 on-disk filesystems, all of which I assume should support leases.
- about 12 filesystems that either are network filesystems (9p, afs, cifs, coda, ncpfs, nfs, nfs4, ocfs2, smbfs) or that don't have control over all opens/data modifications for whatever reason (ecryptfs, fuse, hostfs), so shouldn't be giving out leases by default.
- A bunch of synthetic filesystems (proc, sysfs...).
The latter category being the strongest argument for your approach, since it's sort of ludicrous to allow leases on those filesystems, but currently we do just out of laziness. (Or, in any case, I don't see any reason the current code wouldn't allow them; I haven't actually tested).
Quoted 11 lines
Quoted 10 lines
- We already fall back on the local method by default in the case of locks, and I don't see a reason to treat leases differently.
- The patch to add
.setlease = setlease,
to all thefile_operationsis going to be a big patch that changes behavior in a way that might be easy to miss (because it changes behavior exactly on those filesystems it *doesn't* touch.) I think it'll be easier to get better review on the patch that adds a method just to those filesystems that we're disabling leases for.
Quoted 2 lines
Anyway, feel free to go ahead with the simpler version for now, I'll do the switchover for locks and leases when I get some time.
That would be great. For now I think I'll also add another simple EINVAL-returning setlease() at least to cifs (partly just as an attempt to goad Steve French into following up on a promise at OLS to look into proper lease support for cifs....)
But I've appended my first attempt at your suggestion for leases, in case it's of use. (Untested.)
--b.
From: J. Bruce Fields <[email protected]>
Subject: [PATCH] Turn off support for fcntl leases by default
A lease enforces mutual exclusion with conflicting opens. On filesystems such as network filesystems where not all opens happen under the control of this kernel, the default setlease() operation, which can only check for local conflicts, is incorrect. So in most cases the correct thing is probably to disable leases for those filesystems until they can implement something more sophisticated.
The only users of leases that I'm aware of (samba and nfsd) are actually using them primarly to get synchronous notification of changes to files so that they can update their caches. So, more generally, we should disable leases for any filesystem which might allow file contents to change without a local open for write occuring. That includes most of the synthetic filesystems (like proc), which the file servers probably don't want to export anyway.
Previously we explicitly disabled leases for some network filesystems, but with this patch we disable by default and add an explicit setlease method for those filesystems on which leases will be allowed.
--- fs/adfs/file.c | 1 + fs/affs/file.c | 1 + fs/bfs/file.c | 1 + fs/ext2/file.c | 2 ++ fs/ext3/file.c | 1 + fs/ext4/file.c | 1 + fs/fat/file.c | 1 + fs/hfs/inode.c | 1 + fs/hfsplus/inode.c | 1 + fs/hppfs/hppfs_kern.c | 1 + fs/jffs2/file.c | 1 + fs/jfs/file.c | 1 + fs/locks.c | 8 ++++---- fs/minix/file.c | 1 + fs/nfs/file.c | 12 ------------ fs/ntfs/file.c | 1 + fs/qnx4/file.c | 1 + fs/ramfs/file-mmu.c | 1 + fs/ramfs/file-nommu.c | 1 + fs/read_write.c | 1 + fs/reiserfs/file.c | 1 + fs/sysv/file.c | 1 + fs/udf/file.c | 1 + fs/ufs/file.c | 1 + fs/xfs/linux-2.6/xfs_file.c | 2 ++ 25 files changed, 29 insertions(+), 16 deletions(-)
fs/adfs/file.c +1 -0
@@ -34,6 +34,7 @@ const struct file_operations adfs_file_operations = { .write = do_sync_write, .aio_write = generic_file_aio_write, .sendfile = generic_file_sendfile, + .setlease = setlease, };const struct inode_operations adfs_file_inode_operations = {
fs/affs/file.c +1 -0
@@ -36,6 +36,7 @@ const struct file_operations affs_file_operations = { .release = affs_file_release, .fsync = file_fsync, .sendfile = generic_file_sendfile, + .setlease = setlease, };const struct inode_operations affs_file_inode_operations = {
fs/bfs/file.c +1 -0
@@ -25,6 +25,7 @@ const struct file_operations bfs_file_operations = { .aio_write = generic_file_aio_write, .mmap = generic_file_mmap, .sendfile = generic_file_sendfile, + .setlease = setlease, };static int bfs_move_block (unsigned long from,unsigned long to,struct super_block *sb)
fs/ext2/file.c +2 -0
@@ -56,6 +56,7 @@ const struct file_operations ext2_file_operations = { .sendfile = generic_file_sendfile, .splice_read = generic_file_splice_read, .splice_write = generic_file_splice_write, + .setlease = setlease, };#ifdef CONFIG_EXT2_FS_XIP
@@ -72,6 +73,7 @@ const struct file_operations ext2_xip_file_operations = { .release = ext2_release_file, .fsync = ext2_sync_file, .sendfile = xip_file_sendfile, + .setlease = setlease, };#endif
fs/ext3/file.c +1 -0
@@ -123,6 +123,7 @@ const struct file_operations ext3_file_operations = { .sendfile = generic_file_sendfile, .splice_read = generic_file_splice_read, .splice_write = generic_file_splice_write, + .setlease = setlease, };const struct inode_operations ext3_file_inode_operations = {
fs/ext4/file.c +1 -0
@@ -123,6 +123,7 @@ const struct file_operations ext4_file_operations = { .sendfile = generic_file_sendfile, .splice_read = generic_file_splice_read, .splice_write = generic_file_splice_write, + .setlease = setlease, };const struct inode_operations ext4_file_inode_operations = {
fs/fat/file.c +1 -0
@@ -135,6 +135,7 @@ const struct file_operations fat_file_operations = { .ioctl = fat_generic_ioctl, .fsync = file_fsync, .sendfile = generic_file_sendfile, + .setlease = setlease, };static int fat_cont_expand (struct inode *inode,loff_t size)
fs/hfs/inode.c +1 -0
@@ -611,6 +611,7 @@ static const struct file_operations hfs_file_operations = { .fsync = file_fsync, .open = hfs_file_open, .release = hfs_file_release, + .setlease = setlease, };static const struct inode_operations hfs_file_inode_operations = {
fs/hfsplus/inode.c +1 -0
@@ -293,6 +293,7 @@ static const struct file_operations hfsplus_file_operations = { .open = hfsplus_file_open, .release = hfsplus_file_release, .ioctl = hfsplus_ioctl, + .setlease = setlease, };struct inode *hfsplus_new_inode (struct super_block *sb,int mode)
fs/hppfs/hppfs_kern.c +1 -0
@@ -563,6 +563,7 @@ static const struct file_operations hppfs_file_fops = { .read = hppfs_read, .write = hppfs_write, .open = hppfs_open, + .setlease = setlease, };struct hppfs_dirent {
fs/jffs2/file.c +1 -0
@@ -48,6 +48,7 @@ const struct file_operations jffs2_file_operations = .mmap = generic_file_readonly_mmap, .fsync = jffs2_fsync, .sendfile = generic_file_sendfile + .setlease = setlease, };/* jffs2_file_inode_operations */
fs/jfs/file.c +1 -0
@@ -114,4 +114,5 @@ const struct file_operations jfs_file_operations = { .fsync = jfs_fsync, .release = jfs_release, .ioctl = jfs_ioctl, + .setlease = setlease, };
fs/locks.c +4 -4
@@ -1455,11 +1455,11 @@ int vfs_setlease(struct file *filp, long arg, struct file_lock **lease) {int error; +if (!filp->f_op || !filp->f_op->setlease) +return -EINVAL ; +lock_kernel (); -if (filp->f_op && filp->f_op->setlease) - error = filp->f_op->setlease (filp, arg, lease); -else - error =setlease (filp, arg, lease); + error = filp->f_op->setlease (filp, arg, lease);unlock_kernel ();return error;
fs/minix/file.c +1 -0
@@ -24,6 +24,7 @@ const struct file_operations minix_file_operations = { .mmap = generic_file_mmap, .fsync = minix_sync_file, .sendfile = generic_file_sendfile, + .setlease = setlease, };const struct inode_operations minix_file_inode_operations = {
fs/nfs/file.c +0 -12
@@ -51,7 +51,6 @@ static int nfs_fsync(struct file *, struct dentry *dentry, int datasync);static int nfs_check_flags (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);const struct file_operations nfs_file_operations = { .llseek = nfs_file_llseek,
@@ -68,7 +67,6 @@ const struct file_operations nfs_file_operations = { .flock = nfs_flock, .sendfile = nfs_file_sendfile, .check_flags = nfs_check_flags, - .setlease = nfs_setlease, };const struct inode_operations nfs_file_inode_operations = {
@@ -557,13 +555,3 @@ static int nfs_flock(struct file *filp, int cmd, struct file_lock *fl)return do_unlk (filp, cmd, fl);return do_setlk (filp, cmd, fl); } - -static int nfs_setlease (struct file *file,long arg,struct file_lock **fl) -{ -/* -* There is no protocol support for leases, so we have no way -* to implement them correctly in the face of opens by other -* clients. -*/ -return -EINVAL ; -}
fs/ntfs/file.c +1 -0
@@ -2286,6 +2286,7 @@ const struct file_operations ntfs_file_ops = {on the ntfs partition. We do not need to care about the data source. */ + .setlease = setlease, };const struct inode_operations ntfs_file_inode_ops = {
fs/qnx4/file.c +1 -0
@@ -31,6 +31,7 @@ const struct file_operations qnx4_file_operations = .aio_write = generic_file_aio_write, .fsync = qnx4_sync_file, #endif + .setlease = setlease, }; const struct inode_operations qnx4_file_inode_operations =
fs/ramfs/file-mmu.c +1 -0
@@ -43,6 +43,7 @@ const struct file_operations ramfs_file_operations = { .fsync = simple_sync_file, .sendfile = generic_file_sendfile, .llseek = generic_file_llseek, + .setlease = setlease, };const struct inode_operations ramfs_file_inode_operations = {
fs/ramfs/file-nommu.c +1 -0
@@ -44,6 +44,7 @@ const struct file_operations ramfs_file_operations = { .fsync = simple_sync_file, .sendfile = generic_file_sendfile, .llseek = generic_file_llseek, + .setlease = setlease, };const struct inode_operations ramfs_file_inode_operations = {
fs/read_write.c +1 -0
@@ -26,6 +26,7 @@ const struct file_operations generic_ro_fops = { .aio_read = generic_file_aio_read, .mmap = generic_file_readonly_mmap, .sendfile = generic_file_sendfile, + .setlease = setlease, };EXPORT_SYMBOL (generic_ro_fops);
fs/reiserfs/file.c +1 -0
@@ -1536,6 +1536,7 @@ const struct file_operations reiserfs_file_operations = { .aio_write = generic_file_aio_write, .splice_read = generic_file_splice_read, .splice_write = generic_file_splice_write, + .setlease = setlease, };const struct inode_operations reiserfs_file_inode_operations = {
fs/sysv/file.c +1 -0
@@ -28,6 +28,7 @@ const struct file_operations sysv_file_operations = { .mmap = generic_file_mmap, .fsync = sysv_sync_file, .sendfile = generic_file_sendfile, + .setlease = setlease, };const struct inode_operations sysv_file_inode_operations = {
fs/udf/file.c +1 -0
@@ -262,6 +262,7 @@ const struct file_operations udf_file_operations = { .release = udf_release_file, .fsync = udf_fsync_file, .sendfile = generic_file_sendfile, + .setlease = setlease, };const struct inode_operations udf_file_inode_operations = {
fs/ufs/file.c +1 -0
@@ -61,4 +61,5 @@ const struct file_operations ufs_file_operations = { .open = generic_file_open, .fsync = ufs_sync_file, .sendfile = generic_file_sendfile, + .setlease = setlease, };
fs/xfs/linux-2.6/xfs_file.c +2 -0
@@ -467,6 +467,7 @@ const struct file_operations xfs_file_operations = { #ifdef HAVE_FOP_OPEN_EXEC .open_exec = xfs_file_open_exec, #endif + .setlease = setlease, }; const struct file_operations xfs_invis_file_operations = {
@@ -487,6 +488,7 @@ const struct file_operations xfs_invis_file_operations = { .flush = xfs_file_close, .release = xfs_file_release, .fsync = xfs_file_fsync, + .setlease = setlease, };
Signature · 1 line
1.5.3.rc0.63.gc956
A message that is not in the archive (sent off-list or not yet received): 59343fe9a0b0bdb9c39ed217185b9c0d6c7d8dae.1183143819.git.bfields@citi.umich.edu
Re: [PATCH 2/6] locks: provide a file lease method enabling cluster-coherent leases
Quoted 7 lines · J. Bruce Fields
From: J. Bruce Fields <[email protected]>
Currently leases are only kept locally, so there's no way for a distributed filesystem to enforce them against multiple clients. We're particularly interested in the case of nfsd exporting a cluster filesystem, in which case nfsd needs cluster-coherent leases in order to implement delegations correctly.
Looks good but should be moved second to last in the patch series, with the last one beeing implementing setleast for gfs. Also see my comment to the nfs patch about the default semantics for not implementing the method.
A message that is not in the archive (sent off-list or not yet received): 1bdef6b017f0ccb94ed76dbdd2b4cc676e5ef312.1183143820.git.bfields@citi.umich.edu
Re: [PATCH 5/6] gfs2: stop giving out non-cluster-coherent leases
Quoted 5 lines · J. Bruce Fields
(quoted diff) +5
-0+static int gfs2_setlease(struct file *file, long arg, struct file_lock **fl) +{ + struct gfs2_sbd *sdp = GFS2_SB(file->f_mapping->host); + int ret = -EOPNOTSUPP; +
Review
Quoted 10 lines
(quoted diff) +10
-0+ if (sdp->sd_args.ar_localflocks) { + return setlease(file, arg, fl); + } + + /* For now fail the delegation request. Cluster file system can not + allow any node in the cluster to get a local lease until it can + be managed centrally by the cluster file system. + */ + return ret; +}
Very odd way to write this function. It should look more like:
static int gfs2_setlease(struct file *file, long arg, struct file_lock **fl)
{
struct gfs2_sbd *sdp = GFS2_SB(file->f_mapping->host);
int ret = -EOPNOTSUPP;
/*
* For now fail the delegation request. Cluster file system can not
* allow any node in the cluster to get a local lease until it can
* be managed centrally by the cluster file system.
*/
if (!sdp->sd_args.ar_localflocks)
return -EOPNOTSUPP;
return setlease(file, arg, fl);
}Re: [PATCH 5/6] gfs2: stop giving out non-cluster-coherent leases
Quoted 10 lines · Christoph Hellwig
Quoted 9 lines · J. Bruce Fields
(quoted diff) +9
-0+static int gfs2_setlease(struct file *file, long arg, struct file_lock **fl) +{ + struct gfs2_sbd *sdp = GFS2_SB(file->f_mapping->host); + int ret = -EOPNOTSUPP; + + if (sdp->sd_args.ar_localflocks) { + return setlease(file, arg, fl); + } +
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ /* For now fail the delegation request. Cluster file system can not + allow any node in the cluster to get a local lease until it can + be managed centrally by the cluster file system. + */ + return ret; +}Very odd way to write this function. It should look more like:
Thanks for reading these through! I'm travelling tomorrow, so I'll work through your comments Monday afternoon or Tuesday....
--b.
Re: [PATCH 5/6] gfs2: stop giving out non-cluster-coherent leases
In reply to Christoph HellwigQuoted 10 lines · Christoph Hellwig
Quoted 9 lines · J. Bruce Fields
(quoted diff) +9
-0+static int gfs2_setlease(struct file *file, long arg, struct file_lock **fl) +{ + struct gfs2_sbd *sdp = GFS2_SB(file->f_mapping->host); + int ret = -EOPNOTSUPP; + + if (sdp->sd_args.ar_localflocks) { + return setlease(file, arg, fl); + } +
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ /* For now fail the delegation request. Cluster file system can not + allow any node in the cluster to get a local lease until it can + be managed centrally by the cluster file system. + */ + return ret; +}Very odd way to write this function. It should look more like:
Agreed; I've reversed the sense of the test, as suggested, and got rid of the superfluous local "ret".
--b.
Quoted 16 lines
static int
gfs2_setlease(struct file *file, long arg, struct file_lock **fl){ struct gfs2_sbd *sdp = GFS2_SB(file->f_mapping->host); int ret = -EOPNOTSUPP; /* * For now fail the delegation request. Cluster file system can not * allow any node in the cluster to get a local lease until it can * be managed centrally by the cluster file system. */ if (!sdp->sd_args.ar_localflocks) return -EOPNOTSUPP; return setlease(file, arg, fl); }