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 <bfields@xxxxxxxxxxxxxx>
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" <bfields@xxxxxxxxxxxxxx>
---
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 = generic_file_aio_write,
.sendfile = generic_file_sendfile,
+ .setlease = setlease,
};
const struct inode_operations adfs_file_inode_operations = {
diff --git a/fs/affs/file.c b/fs/affs/file.c
index c879690..5beb510 100644
--- a/fs/affs/file.c
+++ b/fs/affs/file.c
@@ -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 = {
diff --git a/fs/bfs/file.c b/fs/bfs/file.c
index ef4d1fa..055e3c2 100644
--- a/fs/bfs/file.c
+++ b/fs/bfs/file.c
@@ -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)
diff --git a/fs/ext2/file.c b/fs/ext2/file.c
index 566d4e2..d0d9745 100644
--- a/fs/ext2/file.c
+++ b/fs/ext2/file.c
@@ -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
diff --git a/fs/ext3/file.c b/fs/ext3/file.c
index 1e6f138..528e720 100644
--- a/fs/ext3/file.c
+++ b/fs/ext3/file.c
@@ -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 = {
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 3c6c1fd..c4ceb21 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -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 = {
diff --git a/fs/fat/file.c b/fs/fat/file.c
index 55d3c74..9707be9 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -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)
diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index 9a934db..6aaa02d 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -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 = {
diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index 45dab5d..088e7c7 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -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)
diff --git a/fs/hppfs/hppfs_kern.c b/fs/hppfs/hppfs_kern.c
index affb741..edf7081 100644
--- a/fs/hppfs/hppfs_kern.c
+++ b/fs/hppfs/hppfs_kern.c
@@ -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 {
diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
index 9987127..6ccc03e 100644
--- a/fs/jffs2/file.c
+++ b/fs/jffs2/file.c
@@ -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 */
diff --git a/fs/jfs/file.c b/fs/jfs/file.c
index f7f8eff..3ef824b 100644
--- a/fs/jfs/file.c
+++ b/fs/jfs/file.c
@@ -114,4 +114,5 @@ const struct file_operations jfs_file_operations = {
.fsync = jfs_fsync,
.release = jfs_release,
.ioctl = jfs_ioctl,
+ .setlease = setlease,
};
diff --git a/fs/locks.c b/fs/locks.c
index 428fb0a..2ef381e 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -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;
diff --git a/fs/minix/file.c b/fs/minix/file.c
index f92baa1..352352b 100644
--- a/fs/minix/file.c
+++ b/fs/minix/file.c
@@ -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 = {
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index d92a383..9eb8eb4 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -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;
-}
diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
index 7ed5639..569b349 100644
--- a/fs/ntfs/file.c
+++ b/fs/ntfs/file.c
@@ -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 = {
diff --git a/fs/qnx4/file.c b/fs/qnx4/file.c
index 4464998..225a45a 100644
--- a/fs/qnx4/file.c
+++ b/fs/qnx4/file.c
@@ -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 =
diff --git a/fs/ramfs/file-mmu.c b/fs/ramfs/file-mmu.c
index 2f14774..cd1e5b4 100644
--- a/fs/ramfs/file-mmu.c
+++ b/fs/ramfs/file-mmu.c
@@ -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 = {
diff --git a/fs/ramfs/file-nommu.c b/fs/ramfs/file-nommu.c
index 5d258c4..faeb31f 100644
--- a/fs/ramfs/file-nommu.c
+++ b/fs/ramfs/file-nommu.c
@@ -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 = {
diff --git a/fs/read_write.c b/fs/read_write.c
index 4d03008..1e8ea4c 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -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);
diff --git a/fs/reiserfs/file.c b/fs/reiserfs/file.c
index 9e451a6..a95c43c 100644
--- a/fs/reiserfs/file.c
+++ b/fs/reiserfs/file.c
@@ -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 = {
diff --git a/fs/sysv/file.c b/fs/sysv/file.c
index 0732ddb..393487d 100644
--- a/fs/sysv/file.c
+++ b/fs/sysv/file.c
@@ -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 = {
diff --git a/fs/udf/file.c b/fs/udf/file.c
index 51b5764..1941fb9 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -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 = {
diff --git a/fs/ufs/file.c b/fs/ufs/file.c
index 1e09632..c9df8a5 100644
--- a/fs/ufs/file.c
+++ b/fs/ufs/file.c
@@ -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,
};
diff --git a/fs/xfs/linux-2.6/xfs_file.c b/fs/xfs/linux-2.6/xfs_file.c
index cb51dc9..f3a9409 100644
--- a/fs/xfs/linux-2.6/xfs_file.c
+++ b/fs/xfs/linux-2.6/xfs_file.c
@@ -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,
};
--
1.5.3.rc0.63.gc956
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
