Re: [PATCH 6/6] nfs: disable leases over NFS
On Thu, Jul 05, 2007 at 11:41:00AM -0400, J. Bruce Fields wrote: 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 :) - 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. 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. - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] nfs: disable leases over NFS
On Wed, Jul 11, 2007 at 11:20:18AM +0100, Christoph Hellwig wrote:
On Thu, Jul 05, 2007 at 11:41:00AM -0400, J. Bruce Fields wrote:
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).
- 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.
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.
Signed-off-by: J. Bruce Fields [EMAIL PROTECTED]
---
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(-)
diff --git a/fs/adfs/file.c b/fs/adfs/file.c
index f544a28..4e99861 100644
--- a/fs/adfs/file.c
+++ b/fs/adfs/file.c
@@ -34,6 +34,7 @@ const struct file_operations adfs_file_operations = {
.write = do_sync_write,
.aio_write =
Re: [PATCH 6/6] nfs: disable leases over NFS
On Sat, Jun 30, 2007 at 10:25:16AM +0100, Christoph Hellwig wrote: On Fri, Jun 29, 2007 at 03:21:30PM -0400, J. Bruce Fields wrote: 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. 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. - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] nfs: disable leases over NFS
On Sat, Jun 30, 2007 at 10:25:16AM +0100, Christoph Hellwig wrote: On Fri, Jun 29, 2007 at 03:21:30PM -0400, J. Bruce Fields wrote: From: J. Bruce Fields [EMAIL PROTECTED] As Peter Staubach says elsewhere (http://marc.info/?l=linux-kernelm=118113649526444w=2): 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. 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. 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. - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] nfs: disable leases over NFS
On Fri, Jun 29, 2007 at 03:21:30PM -0400, J. Bruce Fields wrote: From: J. Bruce Fields [EMAIL PROTECTED] As Peter Staubach says elsewhere (http://marc.info/?l=linux-kernelm=118113649526444w=2): 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. 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. - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] nfs: disable leases over NFS
J. Bruce Fields wrote:
From: J. Bruce Fields [EMAIL PROTECTED]
As Peter Staubach says elsewhere
(http://marc.info/?l=linux-kernelm=118113649526444w=2):
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(-)
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 9eb8eb4..97c1a3d 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -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;
+}
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
-
To unsubscribe from this list: send the line unsubscribe linux-fsdevel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] nfs: disable leases over NFS
On Fri, Jun 29, 2007 at 05:16:19PM -0400, Peter Staubach wrote: 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 Are you planning on removing it? I'd rather do that, yes. Any objection? 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-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. - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] nfs: disable leases over NFS
Hi, On Fri, 2007-06-29 at 17:39 -0400, J. Bruce Fields wrote: On Fri, Jun 29, 2007 at 05:16:19PM -0400, Peter Staubach wrote: 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 Are you planning on removing it? I'd rather do that, yes. Any objection? 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-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. EINVAL is fine by me, just so long as its not EAGAIN then it gets my blessing :-) Steve. - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] nfs: disable leases over NFS
On Fri, Jun 29, 2007 at 11:30:27PM +0100, Steven Whitehouse wrote:
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.
diff --git a/fs/gfs2/ops_file.c b/fs/gfs2/ops_file.c
index e34d9bd..29a86fe 100644
--- a/fs/gfs2/ops_file.c
+++ b/fs/gfs2/ops_file.c
@@ -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;
}
/**
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 97c1a3d..d92a383 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -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;
}
-
To unsubscribe from this list: send the line unsubscribe linux-fsdevel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
