[PATCH 00/10] locks/nfsd: internal lease API overhaul

[Jeff Layton]

The internal "API" for handling leases has a lot of problems. The main
one is that on success it can return a pointer to a lease that sits on
the inode's i_flock list. That pointer is only guaranteed to be valid
until the i_lock is dropped, which makes it a bit dangerous to use.

Also, the i_lock is held over much too much of the code, which
precludes any hope of ever adding proper support for leases to
distributed filesystems.

This patchset is a cleanup and overhaul of the internal lease API. It
fixes a number of problems in that code and makes an attempt at making
that API more sane to use.

The only real consumer of that API is knfsd, but this should make it
easier for others to do so, reduce and clarify the spinlocking involved
in handling leases, and get us a step closer toward allowing lease
implementations that can block.

I'm targeting this work for v3.18. Review would be welcome...

Jeff Layton (10):
locks: close potential race in lease_get_mtime
nfsd: fix potential lease memory leak in nfs4_setlease
locks: generic_delete_lease doesn't need a file_lock at all
locks: clean up vfs_setlease kerneldoc comments
nfsd: don't keep a pointer to the lease in nfs4_file
locks: plumb an "aux" pointer into the setlease routines
locks: define a lm_setup handler for leases
locks: move i_lock acquisition into generic_*_lease handlers
locks: move freeing of leases outside of i_lock
locks: update Documentation/filesystems with lease API changes

Documentation/filesystems/Locking | 21 ++--
Documentation/filesystems/vfs.txt | 7 +-
fs/cifs/cifsfs.c | 7 +-
fs/gfs2/file.c | 3 +-
fs/locks.c | 237 +++++++++++++++++++++-----------------
fs/nfs/file.c | 2 +-
fs/nfs/internal.h | 2 +-
fs/nfsd/nfs4state.c | 24 ++--
fs/nfsd/state.h | 1 -
include/linux/fs.h | 18 +--
10 files changed, 172 insertions(+), 150 deletions(-)

[Jeff Layton]

lease_get_mtime is called without the i_lock held, so there's no
guarantee about the stability of the list. Between the time when we
assign "flock" and then dereference it to check whether it's a lease
and for write, the lease could be freed.

Ensure that that doesn't occur by taking the i_lock before trying
to check the lease.

Cc: J. Bruce Fields <bfields-***@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/***@public.gmane.org>
---
fs/locks.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index d7e15a256f8f..58ce8897f2e4 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1456,8 +1456,18 @@ EXPORT_SYMBOL(__break_lease);
*/
void lease_get_mtime(struct inode *inode, struct timespec *time)
{
- struct file_lock *flock = inode->i_flock;
- if (flock && IS_LEASE(flock) && (flock->fl_type == F_WRLCK))
+ bool has_lease = false;
+ struct file_lock *flock;
+
+ if (inode->i_flock) {
+ spin_lock(&inode->i_lock);
+ flock = inode->i_flock;
+ if (flock && IS_LEASE(flock) && (flock->fl_type == F_WRLCK))
+ has_lease = true;
+ spin_unlock(&inode->i_lock);
+ }
+
+ if (has_lease)
*time = current_fs_time(inode->i_sb);
else
*time = inode->i_mtime;

[Christoph Hellwig]

Post by Jeff Layton
lease_get_mtime is called without the i_lock held, so there's no
guarantee about the stability of the list. Between the time when we
assign "flock" and then dereference it to check whether it's a lease
and for write, the lease could be freed.
Ensure that that doesn't occur by taking the i_lock before trying
to check the lease.

Looks good. Also looks way cleaner than before by being just a tad more
verbose..

Reviewed-by: Christoph Hellwig <hch-***@public.gmane.org>

[J. Bruce Fields]

Post by Jeff Layton
lease_get_mtime is called without the i_lock held, so there's no
guarantee about the stability of the list. Between the time when we
assign "flock" and then dereference it to check whether it's a lease
and for write, the lease could be freed.
Ensure that that doesn't occur by taking the i_lock before trying
to check the lease.

ACK.

Though I still wonder whether we shouldn't just axe lease_get_mtime.

What it's doing is telling v2/v3 clients that the mtime of a
write-leased file is always the current time, in order to force
applications such as make to open the file and break the lease, thus
forcing any cached writes to be flushed to the server. Otherwise the
worry was that they'd never find out about writes cached by Samba
clients somewhere.

Talking over NFS write delegation implementation with Trond, he
convinced me that our least-worst option for handling the same problem
there would be just to continue to depend on clients holding write
leases to touch the file at appropriate times (like close and unlock).

I don't know what sort of behavior Samba or its clients has here. Even
if they're not terribly good about keeping the mtime updated the lost of
consistency might be less-worse than this faking up of the mtime.

Looking at the git history, lease_get_mtime appears to have been part of
the lease code from the time it was first merged in 2.4.0-test9pre6, so
it may not have been added in response to an actual practical problem.

Digging around, here's a reference to last time this came up, because
somebody was irritated that make was constantly rebuilding things for no
reason:

https://bugzilla.samba.org/show_bug.cgi?id=5103

Anyway, that's all orthogonal to this patch, ACK to it for now.

--b.

Post by Jeff Layton
---
fs/locks.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index d7e15a256f8f..58ce8897f2e4 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1456,8 +1456,18 @@ EXPORT_SYMBOL(__break_lease);
*/
void lease_get_mtime(struct inode *inode, struct timespec *time)
{
- struct file_lock *flock = inode->i_flock;
- if (flock && IS_LEASE(flock) && (flock->fl_type == F_WRLCK))
+ bool has_lease = false;
+ struct file_lock *flock;
+
+ if (inode->i_flock) {
+ spin_lock(&inode->i_lock);
+ flock = inode->i_flock;
+ if (flock && IS_LEASE(flock) && (flock->fl_type == F_WRLCK))
+ has_lease = true;
+ spin_unlock(&inode->i_lock);
+ }
+
+ if (has_lease)
*time = current_fs_time(inode->i_sb);
else
*time = inode->i_mtime;
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Jeff Layton]

Ensure that it's OK to pass in a NULL file_lock double pointer on
a F_UNLCK request and convert the vfs_setlease F_UNLCK callers to
do just that.

Finally, turn the BUG_ON in generic_setlease into a WARN_ON_ONCE
with an error return. That's a problem we can handle without
crashing the box if it occurs.

Signed-off-by: Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/***@public.gmane.org>
---
fs/locks.c | 25 ++++++++-----------------
fs/nfsd/nfs4state.c | 2 +-
2 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 58ce8897f2e4..bedb817a5cc4 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1637,20 +1637,18 @@ out:
return error;
}

-static int generic_delete_lease(struct file *filp, struct file_lock **flp)
+static int generic_delete_lease(struct file *filp)
{
struct file_lock *fl, **before;
struct dentry *dentry = filp->f_path.dentry;
struct inode *inode = dentry->d_inode;

- trace_generic_delete_lease(inode, *flp);
-
for (before = &inode->i_flock;
((fl = *before) != NULL) && IS_LEASE(fl);
before = &fl->fl_next) {
if (fl->fl_file != filp)
continue;
- return (*flp)->fl_lmops->lm_change(before, F_UNLCK);
+ return fl->fl_lmops->lm_change(before, F_UNLCK);
}
return -EAGAIN;
}
@@ -1682,13 +1680,15 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)

time_out_leases(inode);

- BUG_ON(!(*flp)->fl_lmops->lm_break);
-
switch (arg) {
case F_UNLCK:
- return generic_delete_lease(filp, flp);
+ return generic_delete_lease(filp);
case F_RDLCK:
case F_WRLCK:
+ if (!(*flp)->fl_lmops->lm_break) {
+ WARN_ON_ONCE(1);
+ return -ENOLCK;
+ }
return generic_add_lease(filp, arg, flp);
default:
return -EINVAL;
@@ -1744,15 +1744,6 @@ int vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
}
EXPORT_SYMBOL_GPL(vfs_setlease);

-static int do_fcntl_delete_lease(struct file *filp)
-{
- struct file_lock fl, *flp = &fl;
-
- lease_init(filp, F_UNLCK, flp);
-
- return vfs_setlease(filp, F_UNLCK, &flp);
-}
-
static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
{
struct file_lock *fl, *ret;
@@ -1809,7 +1800,7 @@ out_unlock:
int fcntl_setlease(unsigned int fd, struct file *filp, long arg)
{
if (arg == F_UNLCK)
- return do_fcntl_delete_lease(filp);
+ return vfs_setlease(filp, F_UNLCK, NULL);
return do_fcntl_add_lease(fd, filp, arg);
}

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 29fac18d9102..0cd252916e1a 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -683,7 +683,7 @@ static void nfs4_put_deleg_lease(struct nfs4_file *fp)
if (!fp->fi_lease)
return;
if (atomic_dec_and_test(&fp->fi_delegees)) {
- vfs_setlease(fp->fi_deleg_file, F_UNLCK, &fp->fi_lease);
+ vfs_setlease(fp->fi_deleg_file, F_UNLCK, NULL);
fp->fi_lease = NULL;
fput(fp->fi_deleg_file);
fp->fi_deleg_file = NULL;

[Christoph Hellwig]

Post by Jeff Layton
+static int generic_delete_lease(struct file *filp)
{
struct file_lock *fl, **before;
struct dentry *dentry = filp->f_path.dentry;
struct inode *inode = dentry->d_inode;
- trace_generic_delete_lease(inode, *flp);
-

Can you keep the tracepoint and modify it to not need the file_lock
pointer? It really helped me with some debugging lately.

Otherwise looks fine,

Reviewed-by: Christoph Hellwig <hch-***@public.gmane.org>

[Jeff Layton]

On Sat, 23 Aug 2014 18:27:57 -0700

Post by Christoph Hellwig

Post by Jeff Layton
+static int generic_delete_lease(struct file *filp)
{
struct file_lock *fl, **before;
struct dentry *dentry = filp->f_path.dentry;
struct inode *inode = dentry->d_inode;
- trace_generic_delete_lease(inode, *flp);
-

Can you keep the tracepoint and modify it to not need the file_lock
pointer? It really helped me with some debugging lately.
Otherwise looks fine,

Doh! Yes, I meant to put that back. Will do on the next respin.

Thanks,

[Jeff Layton]

...and call it when setting up a new lease or modifying an existing one.
Add a lm_setup handler for fcntl leases and move the fasync setup into
it.

At the same time, change the semantics of how the file_lock double
pointer is handled. Up until now on a successful lease return, you get
back a pointer to the lock on the inode's i_flock list. This is bad,
since that pointer can't be relied on as valid once the inode->i_lock
has been released. Leases also don't carry a lot of information, so
tracking a pointer to it isn't terribly helpful anyway.

Change the code to instead just zero out the pointer if the lease we
passed in ended up being used. Then the callers can just check to see
if it's NULL after the call and free it if it isn't.

The new aux argument for lm_setup has the same semantics. The lm_setup
function can zero the pointer out to signal to the caller that it should
not be freed after the function returns.

Signed-off-by: Jeff Layton <***@primarydata.com>
---
fs/locks.c | 96 +++++++++++++++++++++++++++++------------------------
fs/nfsd/nfs4state.c | 9 ++---
include/linux/fs.h | 1 +
3 files changed, 56 insertions(+), 50 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 906e09da115a..b35b706c05fe 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -432,9 +432,32 @@ static void lease_break_callback(struct file_lock *fl)
kill_fasync(&fl->fl_fasync, SIGIO, POLL_MSG);
}

+static void
+lease_setup(struct file_lock *fl, void **aux)
+{
+ struct file *filp = fl->fl_file;
+ struct fasync_struct *fa = *aux;
+
+ /*
+ * fasync_insert_entry() returns the old entry if any. If there was no
+ * old entry, then it used "aux" and inserted it into the fasync list.
+ * Clear the pointer to indicate that it shouldn't be freed.
+ */
+ if (!fasync_insert_entry(fa->fa_fd, filp, &fl->fl_fasync, fa))
+ *aux = NULL;
+
+ /*
+ * Despite the fact that it's an int return function, __f_setown never
+ * returns an error. Just ignore any error return here, but spew a
+ * WARN_ON_ONCE in case this ever changes.
+ */
+ WARN_ON_ONCE(__f_setown(filp, task_pid(current), PIDTYPE_PID, 0));
+}
+
static const struct lock_manager_operations lease_manager_ops = {
.lm_break = lease_break_callback,
.lm_change = lease_modify,
+ .lm_setup = lease_setup,
};

/*
@@ -1609,9 +1632,9 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **au
if (my_before != NULL) {
lease = *my_before;
error = lease->fl_lmops->lm_change(my_before, arg);
- if (!error)
- *flp = *my_before;
- goto out;
+ if (error)
+ goto out;
+ goto out_setup;
}

error = -EINVAL;
@@ -1632,9 +1655,15 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **au
error = check_conflicting_open(dentry, arg);
if (error)
goto out_unlink;
+
+out_setup:
+ if (lease->fl_lmops->lm_setup)
+ lease->fl_lmops->lm_setup(lease, aux);
out:
if (is_deleg)
mutex_unlock(&inode->i_mutex);
+ if (!error && !my_before)
+ *flp = NULL;
return error;
out_unlink:
locks_unlink_lock(before);
@@ -1659,10 +1688,11 @@ static int generic_delete_lease(struct file *filp)

/**
* generic_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
- * @aux: auxillary data for lm_setup
+ * @filp: file pointer
+ * @arg: type of lease to obtain
+ * @flp: input - file_lock to use, output - file_lock inserted
+ * @aux: auxillary data for lm_setup (may be NULL if lm_setup
+ * doesn't require it)
*
* The (input) flp->fl_lmops->lm_break function is required
* by break_lease().
@@ -1702,21 +1732,13 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp,
}
EXPORT_SYMBOL(generic_setlease);

-static int
-__vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **aux)
-{
- if (filp->f_op->setlease)
- return filp->f_op->setlease(filp, arg, lease, aux);
- else
- return generic_setlease(filp, arg, lease, aux);
-}
-
/**
* vfs_setlease - sets a lease on an open file
- * @filp: file pointer
- * @arg: type of lease to obtain
- * @lease: file_lock to use when adding a lease
- * @aux: auxillary info for lm_setup when adding a lease
+ * @filp: file pointer
+ * @arg: type of lease to obtain
+ * @lease: file_lock to use when adding a lease
+ * @aux: auxillary info for lm_setup when adding a lease (may be
+ * NULL if lm_setup doesn't require it)
*
* Call this to establish a lease on the file. The "lease" argument is not
* used for F_UNLCK requests and may be NULL. For commands that set or alter
@@ -1730,8 +1752,10 @@ __vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **aux
* where fcntl_getlease() can find it. Since fcntl_getlease() only reports
* whether the current task holds a lease, a cluster filesystem need only do
* this for leases held by processes on this node.
+ *
+ * The "aux" pointer is passed directly to the lm_setup function as-is. It
+ * may be NULL if the lm_setup operation doesn't require it.
*/
-
int
vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **aux)
{
@@ -1739,17 +1763,18 @@ vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **aux)
int error;

spin_lock(&inode->i_lock);
- error = __vfs_setlease(filp, arg, lease, aux);
+ if (filp->f_op->setlease)
+ error = filp->f_op->setlease(filp, arg, lease, aux);
+ else
+ error = generic_setlease(filp, arg, lease, aux);
spin_unlock(&inode->i_lock);
-
return error;
}
EXPORT_SYMBOL_GPL(vfs_setlease);

static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
{
- struct file_lock *fl, *ret;
- struct inode *inode = file_inode(filp);
+ struct file_lock *fl;
struct fasync_struct *new;
int error;

@@ -1762,26 +1787,9 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
locks_free_lock(fl);
return -ENOMEM;
}
- ret = fl;
- spin_lock(&inode->i_lock);
- error = __vfs_setlease(filp, arg, &ret, NULL);
- if (error)
- goto out_unlock;
- if (ret == fl)
- fl = NULL;
+ new->fa_fd = fd;

- /*
- * fasync_insert_entry() returns the old entry if any.
- * If there was no old entry, then it used 'new' and
- * inserted it into the fasync list. Clear new so that
- * we don't release it here.
- */
- if (!fasync_insert_entry(fd, filp, &ret->fl_fasync, new))
- new = NULL;
-
- error = __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
-out_unlock:
- spin_unlock(&inode->i_lock);
+ error = vfs_setlease(filp, arg, &fl, (void **)&new);
if (fl)
locks_free_lock(fl);
if (new)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d964a41c9151..6af5d0890373 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3773,7 +3773,7 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_file *fp, int flag)
static int nfs4_setlease(struct nfs4_delegation *dp)
{
struct nfs4_file *fp = dp->dl_stid.sc_file;
- struct file_lock *fl, *ret;
+ struct file_lock *fl;
struct file *filp;
int status = 0;

@@ -3787,14 +3787,11 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
return -EBADF;
}
fl->fl_file = filp;
- ret = fl;
status = vfs_setlease(filp, fl->fl_type, &fl, NULL);
- if (status) {
+ if (fl)
locks_free_lock(fl);
+ if (status)
goto out_fput;
- }
- if (ret != fl)
- locks_free_lock(fl);
spin_lock(&state_lock);
spin_lock(&fp->fi_lock);
/* Did the lease get broken before we took the lock? */
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2668d054147f..70d22c12924f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -874,6 +874,7 @@ struct lock_manager_operations {
int (*lm_grant)(struct file_lock *, int);
void (*lm_break)(struct file_lock *);
int (*lm_change)(struct file_lock **, int);
+ void (*lm_setup)(struct file_lock *, void **);
};

struct lock_manager {

[Christoph Hellwig]

Post by Jeff Layton
+ /*
+ * Despite the fact that it's an int return function, __f_setown never
+ * returns an error. Just ignore any error return here, but spew a
+ * WARN_ON_ONCE in case this ever changes.
+ */
+ WARN_ON_ONCE(__f_setown(filp, task_pid(current), PIDTYPE_PID, 0));

I don't think this is a good idea. The errors in __f_setown come from
the security modules, and they could change easily. If you can convince
the LSM people to change their file_set_fowner routine to return void
we could change __f_setown to return void as well and be done with it,
but without that this looks too dangerous.

[Jeff Layton]

On Sun, 24 Aug 2014 08:58:58 -0700

Post by Christoph Hellwig

Post by Jeff Layton
+ /*
+ * Despite the fact that it's an int return function, __f_setown never
+ * returns an error. Just ignore any error return here, but spew a
+ * WARN_ON_ONCE in case this ever changes.
+ */
+ WARN_ON_ONCE(__f_setown(filp, task_pid(current), PIDTYPE_PID, 0));

I don't think this is a good idea. The errors in __f_setown come from
the security modules, and they could change easily. If you can convince
the LSM people to change their file_set_fowner routine to return void
we could change __f_setown to return void as well and be done with it,
but without that this looks too dangerous.

Understood. I figured that this might not be acceptable. I can make
this propagate the error back up, but cleaning up the mess may not be
easy. I'll see what I can do.

Note that the error handling in the existing code looks wrong to me
too. The lease gets added to the list (or updated), the fasync handler
gets set up. Then, if __f_setown returns an error, the code just
returns that error without unwinding anything.

[Christoph Hellwig]

Post by Jeff Layton

Post by Christoph Hellwig
I don't think this is a good idea. The errors in __f_setown come from
the security modules, and they could change easily. If you can convince
the LSM people to change their file_set_fowner routine to return void
we could change __f_setown to return void as well and be done with it,
but without that this looks too dangerous.

Understood. I figured that this might not be acceptable. I can make
this propagate the error back up, but cleaning up the mess may not be
easy. I'll see what I can do.

I'd say talk to the LSM people. Right now they are only using it to
set up private data pointers, so they might agree on turning it into
a void return. Or just be bold and do that work yourself, include it in
this series and just Cc them.

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Jeff Layton]

Signed-off-by: Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/***@public.gmane.org>
---
Documentation/filesystems/Locking | 21 ++++++++++-----------
Documentation/filesystems/vfs.txt | 7 ++++---
2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking
index f1997e9da61f..9e777f9fc859 100644
--- a/Documentation/filesystems/Locking
+++ b/Documentation/filesystems/Locking
@@ -349,11 +349,7 @@ prototypes:
locking rules:
inode->i_lock may block
fl_copy_lock: yes no
-fl_release_private: maybe maybe[1]
-
-[1]: ->fl_release_private for flock or POSIX locks is currently allowed
-to block. Leases however can still be freed while the i_lock is held and
-so fl_release_private called on a lease should not block.
+fl_release_private: no yes

----------------------- lock_manager_operations ---------------------------
prototypes:
@@ -362,7 +358,8 @@ prototypes:
void (*lm_notify)(struct file_lock *); /* unblock callback */
int (*lm_grant)(struct file_lock *, struct file_lock *, int);
void (*lm_break)(struct file_lock *); /* break_lease callback */
- int (*lm_change)(struct file_lock **, int);
+ int (*lm_change)(struct file_lock **, int, struct list_head *);
+ void (*lm_setup)(struct file_lock *, void **);

locking rules:

@@ -373,6 +370,7 @@ lm_notify: yes yes no
lm_grant: no no no
lm_break: yes no no
lm_change yes no no
+lm_setup yes no no

[1]: ->lm_compare_owner and ->lm_owner_key are generally called with
*an* inode->i_lock held. It may not be the i_lock of the inode
@@ -464,15 +462,12 @@ prototypes:
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 **);
+ int (*setlease)(struct file *, long, struct file_lock **, void **);
long (*fallocate)(struct file *, int, loff_t, loff_t);
};

locking rules:
- All may block except for ->setlease.
- No VFS locks held on entry except for ->setlease.
-
-->setlease has the file_list_lock held and must not sleep.
+ All may block.

->llseek() locking has moved from llseek to the individual llseek
implementations. If your fs is not using generic_file_llseek, you
@@ -496,6 +491,10 @@ components. And there are other reasons why the current interface is a mess...
->read on directories probably must go away - we should just enforce -EISDIR
in sys_read() and friends.

+->setlease operations should call generic_setlease() before or after setting
+the lease within the individual filesystem to record the result of the
+operation
+
--------------------------- dquot_operations -------------------------------
prototypes:
int (*write_dquot) (struct dquot *);
diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index 61d65cc65c54..af9441f32a62 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -826,7 +826,7 @@ struct file_operations {
int (*flock) (struct file *, int, struct file_lock *);
ssize_t (*splice_write)(struct pipe_inode_info *, struct file *, size_t, unsigned int);
ssize_t (*splice_read)(struct file *, struct pipe_inode_info *, size_t, unsigned int);
- int (*setlease)(struct file *, long arg, struct file_lock **);
+ int (*setlease)(struct file *, long arg, struct file_lock **, void **);
long (*fallocate)(struct file *, int mode, loff_t offset, loff_t len);
int (*show_fdinfo)(struct seq_file *m, struct file *f);
};
@@ -895,8 +895,9 @@ otherwise noted.
splice_read: called by the VFS to splice data from file to a pipe. This
method is used by the splice(2) system call

- setlease: called by the VFS to set or release a file lock lease.
- setlease has the file_lock_lock held and must not sleep.
+ setlease: called by the VFS to set or release a file lock lease. setlease
+ implementations should call generic_setlease to record or remove
+ the lease in the inode after setting it

fallocate: called by the VFS to preallocate blocks or punch a hole.

[Jeff Layton]

There was only one place where we still could free a file_lock while
holding the i_lock -- lease_modify. Add a new list_head argument to the
lm_change operation, pass in a private list when calling it, and fix
those callers to dispose of the list once the lock has been dropped.

Signed-off-by: Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/***@public.gmane.org>
---
fs/locks.c | 35 ++++++++++++++++++++++-------------
fs/nfsd/nfs4state.c | 6 +++---
include/linux/fs.h | 7 ++++---
3 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 49210d5cbf41..98907cecaa44 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1297,7 +1297,7 @@ static void lease_clear_pending(struct file_lock *fl, int arg)
}

/* We already had a lease on this file; just change its type */
-int lease_modify(struct file_lock **before, int arg)
+int lease_modify(struct file_lock **before, int arg, struct list_head *dispose)
{
struct file_lock *fl = *before;
int error = assign_type(fl, arg);
@@ -1316,7 +1316,7 @@ int lease_modify(struct file_lock **before, int arg)
printk(KERN_ERR "locks_delete_lock: fasync == %p\n", fl->fl_fasync);
fl->fl_fasync = NULL;
}
- locks_delete_lock(before, NULL);
+ locks_delete_lock(before, dispose);
}
return 0;
}
@@ -1330,7 +1330,7 @@ static bool past_time(unsigned long then)
return time_after(jiffies, then);
}

-static void time_out_leases(struct inode *inode)
+static void time_out_leases(struct inode *inode, struct list_head *dispose)
{
struct file_lock **before;
struct file_lock *fl;
@@ -1341,9 +1341,9 @@ static void time_out_leases(struct inode *inode)
while ((fl = *before) && IS_LEASE(fl) && lease_breaking(fl)) {
trace_time_out_leases(inode, fl);
if (past_time(fl->fl_downgrade_time))
- lease_modify(before, F_RDLCK);
+ lease_modify(before, F_RDLCK, dispose);
if (past_time(fl->fl_break_time))
- lease_modify(before, F_UNLCK);
+ lease_modify(before, F_UNLCK, dispose);
if (fl == *before) /* lease_modify may have freed fl */
before = &fl->fl_next;
}
@@ -1378,6 +1378,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
int i_have_this_lease = 0;
bool lease_conflict = false;
int want_write = (mode & O_ACCMODE) != O_RDONLY;
+ LIST_HEAD(dispose);

new_fl = lease_alloc(NULL, want_write ? F_WRLCK : F_RDLCK);
if (IS_ERR(new_fl))
@@ -1386,7 +1387,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)

spin_lock(&inode->i_lock);

- time_out_leases(inode);
+ time_out_leases(inode, &dispose);

flock = inode->i_flock;
if ((flock == NULL) || !IS_LEASE(flock))
@@ -1441,6 +1442,7 @@ restart:
locks_insert_block(flock, new_fl);
trace_break_lease_block(inode, new_fl);
spin_unlock(&inode->i_lock);
+ locks_dispose_list(&dispose);
error = wait_event_interruptible_timeout(new_fl->fl_wait,
!new_fl->fl_next, break_time);
spin_lock(&inode->i_lock);
@@ -1448,7 +1450,7 @@ restart:
locks_delete_block(new_fl);
if (error >= 0) {
if (error == 0)
- time_out_leases(inode);
+ time_out_leases(inode, &dispose);
/*
* Wait for the next conflicting lease that has not been
* broken yet
@@ -1463,6 +1465,7 @@ restart:

out:
spin_unlock(&inode->i_lock);
+ locks_dispose_list(&dispose);
locks_free_lock(new_fl);
return error;
}
@@ -1527,9 +1530,10 @@ int fcntl_getlease(struct file *filp)
struct file_lock *fl;
struct inode *inode = file_inode(filp);
int type = F_UNLCK;
+ LIST_HEAD(dispose);

spin_lock(&inode->i_lock);
- time_out_leases(file_inode(filp));
+ time_out_leases(file_inode(filp), &dispose);
for (fl = file_inode(filp)->i_flock; fl && IS_LEASE(fl);
fl = fl->fl_next) {
if (fl->fl_file == filp) {
@@ -1538,6 +1542,7 @@ int fcntl_getlease(struct file *filp)
}
}
spin_unlock(&inode->i_lock);
+ locks_dispose_list(&dispose);
return type;
}

@@ -1575,6 +1580,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **au
struct inode *inode = dentry->d_inode;
bool is_deleg = (*flp)->fl_flags & FL_DELEG;
int error;
+ LIST_HEAD(dispose);

lease = *flp;
trace_generic_add_lease(inode, lease);
@@ -1598,7 +1604,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **au
}

spin_lock(&inode->i_lock);
- time_out_leases(inode);
+ time_out_leases(inode, &dispose);

error = check_conflicting_open(dentry, arg);
if (error)
@@ -1636,7 +1642,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **au

if (my_before != NULL) {
lease = *my_before;
- error = lease->fl_lmops->lm_change(my_before, arg);
+ error = lease->fl_lmops->lm_change(my_before, arg, &dispose);
if (error)
goto out;
goto out_setup;
@@ -1666,6 +1672,7 @@ out_setup:
lease->fl_lmops->lm_setup(lease, aux);
out:
spin_unlock(&inode->i_lock);
+ locks_dispose_list(&dispose);
if (is_deleg)
mutex_unlock(&inode->i_mutex);
if (!error && !my_before)
@@ -1682,17 +1689,19 @@ static int generic_delete_lease(struct file *filp)
struct file_lock *fl, **before;
struct dentry *dentry = filp->f_path.dentry;
struct inode *inode = dentry->d_inode;
+ LIST_HEAD(dispose);

spin_lock(&inode->i_lock);
- time_out_leases(inode);
+ time_out_leases(inode, &dispose);
for (before = &inode->i_flock;
((fl = *before) != NULL) && IS_LEASE(fl);
before = &fl->fl_next) {
if (fl->fl_file != filp)
continue;
- error = fl->fl_lmops->lm_change(before, F_UNLCK);
+ error = fl->fl_lmops->lm_change(before, F_UNLCK, &dispose);
}
spin_unlock(&inode->i_lock);
+ locks_dispose_list(&dispose);
return error;
}

@@ -2388,7 +2397,7 @@ void locks_remove_file(struct file *filp)
while ((fl = *before) != NULL) {
if (fl->fl_file == filp) {
if (IS_LEASE(fl)) {
- lease_modify(before, F_UNLCK);
+ lease_modify(before, F_UNLCK, &dispose);
continue;
}

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 6af5d0890373..623576301e38 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3423,11 +3423,11 @@ static void nfsd_break_deleg_cb(struct file_lock *fl)
spin_unlock(&fp->fi_lock);
}

-static
-int nfsd_change_deleg_cb(struct file_lock **onlist, int arg)
+static int
+nfsd_change_deleg_cb(struct file_lock **onlist, int arg, struct list_head *dispose)
{
if (arg & F_UNLCK)
- return lease_modify(onlist, arg);
+ return lease_modify(onlist, arg, dispose);
else
return -EAGAIN;
}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 70d22c12924f..e3dbcf269cd2 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -873,7 +873,7 @@ struct lock_manager_operations {
void (*lm_notify)(struct file_lock *); /* unblock callback */
int (*lm_grant)(struct file_lock *, int);
void (*lm_break)(struct file_lock *);
- int (*lm_change)(struct file_lock **, int);
+ int (*lm_change)(struct file_lock **, int, struct list_head *);
void (*lm_setup)(struct file_lock *, void **);
};

@@ -985,7 +985,7 @@ extern int __break_lease(struct inode *inode, unsigned int flags, unsigned int t
extern void lease_get_mtime(struct inode *, struct timespec *time);
extern int generic_setlease(struct file *, long, struct file_lock **, void **aux);
extern int vfs_setlease(struct file *, long, struct file_lock **, void **aux);
-extern int lease_modify(struct file_lock **, int);
+extern int lease_modify(struct file_lock **, int, struct list_head *);
#else /* !CONFIG_FILE_LOCKING */
static inline int fcntl_getlk(struct file *file, unsigned int cmd,
struct flock __user *user)
@@ -1112,7 +1112,8 @@ static inline int vfs_setlease(struct file *filp, long arg,
return -EINVAL;
}

-static inline int lease_modify(struct file_lock **before, int arg)
+static inline int lease_modify(struct file_lock **before, int arg,
+ struct list_head *dispose)
{
return -EINVAL;
}

[Christoph Hellwig]

Post by Jeff Layton
There was only one place where we still could free a file_lock while
holding the i_lock -- lease_modify. Add a new list_head argument to the
lm_change operation, pass in a private list when calling it, and fix
those callers to dispose of the list once the lock has been dropped.

Why do we care about locks held when simply freeing a piece of memory?

[Jeff Layton]

On Sun, 24 Aug 2014 09:08:04 -0700

Post by Christoph Hellwig

Post by Jeff Layton
There was only one place where we still could free a file_lock while
holding the i_lock -- lease_modify. Add a new list_head argument to the
lm_change operation, pass in a private list when calling it, and fix
those callers to dispose of the list once the lock has been dropped.

Why do we care about locks held when simply freeing a piece of memory?

That's all that's done now, but locks_free_lock can call
fl->fl_release_private. Currently, no filesystem sets that for leases,
but while we're in here we might as well make it possible for that to
block for all lock types since that's simpler than having different
rules for them.

It doesn't really cost us anything other than a bit of list
manipulation and reduces contention on the i_lock slightly.

[Jeff Layton]

Now that we have a saner internal API for managing leases, we no longer
need to mandate that the inode->i_lock be held over most of the lease
code. Push it down into generic_add_lease and generic_delete_lease.

With this change ->setlease calls can now block, which is a necessary
(but not sufficient!) step toward allowing leases to work with
distributed filesystems.

Signed-off-by: Jeff Layton <***@primarydata.com>
---
fs/locks.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index b35b706c05fe..49210d5cbf41 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1335,6 +1335,8 @@ static void time_out_leases(struct inode *inode)
struct file_lock **before;
struct file_lock *fl;

+ lockdep_assert_held(&inode->i_lock);
+
before = &inode->i_flock;
while ((fl = *before) && IS_LEASE(fl) && lease_breaking(fl)) {
trace_time_out_leases(inode, fl);
@@ -1595,6 +1597,9 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **au
return -EINVAL;
}

+ spin_lock(&inode->i_lock);
+ time_out_leases(inode);
+
error = check_conflicting_open(dentry, arg);
if (error)
goto out;
@@ -1660,6 +1665,7 @@ out_setup:
if (lease->fl_lmops->lm_setup)
lease->fl_lmops->lm_setup(lease, aux);
out:
+ spin_unlock(&inode->i_lock);
if (is_deleg)
mutex_unlock(&inode->i_mutex);
if (!error && !my_before)
@@ -1672,18 +1678,22 @@ out_unlink:

static int generic_delete_lease(struct file *filp)
{
+ int error = -EAGAIN;
struct file_lock *fl, **before;
struct dentry *dentry = filp->f_path.dentry;
struct inode *inode = dentry->d_inode;

+ spin_lock(&inode->i_lock);
+ time_out_leases(inode);
for (before = &inode->i_flock;
((fl = *before) != NULL) && IS_LEASE(fl);
before = &fl->fl_next) {
if (fl->fl_file != filp)
continue;
- return fl->fl_lmops->lm_change(before, F_UNLCK);
+ error = fl->fl_lmops->lm_change(before, F_UNLCK);
}
- return -EAGAIN;
+ spin_unlock(&inode->i_lock);
+ return error;
}

/**
@@ -1702,8 +1712,7 @@ static int generic_delete_lease(struct file *filp)
int generic_setlease(struct file *filp, long arg, struct file_lock **flp,
void **aux)
{
- struct dentry *dentry = filp->f_path.dentry;
- struct inode *inode = dentry->d_inode;
+ struct inode *inode = file_inode(filp);
int error;

if ((!uid_eq(current_fsuid(), inode->i_uid)) && !capable(CAP_LEASE))
@@ -1714,8 +1723,6 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp,
if (error)
return error;

- time_out_leases(inode);
-
switch (arg) {
case F_UNLCK:
return generic_delete_lease(filp);
@@ -1759,16 +1766,10 @@ EXPORT_SYMBOL(generic_setlease);
int
vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **aux)
{
- struct inode *inode = file_inode(filp);
- int error;
-
- spin_lock(&inode->i_lock);
if (filp->f_op->setlease)
- error = filp->f_op->setlease(filp, arg, lease, aux);
+ return filp->f_op->setlease(filp, arg, lease, aux);
else
- error = generic_setlease(filp, arg, lease, aux);
- spin_unlock(&inode->i_lock);
- return error;
+ return generic_setlease(filp, arg, lease, aux);
}
EXPORT_SYMBOL_GPL(vfs_setlease);

[Christoph Hellwig]

Looks good,

Post by Jeff Layton
+ spin_lock(&inode->i_lock);
+ time_out_leases(inode);
for (before = &inode->i_flock;
((fl = *before) != NULL) && IS_LEASE(fl);
before = &fl->fl_next) {
if (fl->fl_file != filp)
continue;
- return fl->fl_lmops->lm_change(before, F_UNLCK);
+ error = fl->fl_lmops->lm_change(before, F_UNLCK);
}

We really should split a lm_release from lm_change, the way it is
used is highly confusing. In addition I think a lot of code
currently in lease_modify should move here instead, e.g. something like:


if (fl->fl_file != filp)
continue;

fl = *before;
fl->fl_type = F_UNLCK;
lease_clear_pending(fl, F_UNLCK);
locks_wake_up_blocks(fl);
if (fl->fl_lmops->lm_delete)
fl->fl_lmops->lm_delete(fl);
locks_delete_lock(before, NULL);

with lm_delete for user space leases as:

static void lease_delete(struct file_lock *fl)
{
struct file *filp = fl->fl_file;

f_delown(filp);
filp->f_owner.signum = 0;
fasync_helper(0, fl->fl_file, 0, &fl->fl_fasync);
if (fl->fl_fasync != NULL) {
printk(KERN_ERR "locks_delete_lock: fasync == %p\n",
fl->fl_fasync);
fl->fl_fasync = NULL;
}
}

and a NULL implementation for delegations.

[Christoph Hellwig]

Post by Christoph Hellwig
We really should split a lm_release from lm_change, the way it is
used is highly confusing. In addition I think a lot of code

At this point the old lm_change actually becomes superflous if we
simply disallow changes for delegation.

[Jeff Layton]

On Sun, 24 Aug 2014 09:11:34 -0700

Post by Christoph Hellwig

Post by Christoph Hellwig
We really should split a lm_release from lm_change, the way it is
used is highly confusing. In addition I think a lot of code

At this point the old lm_change actually becomes superflous if we
simply disallow changes for delegation.

This sounds like a reasonable change, but I think it'll be easier to
implement a little later once we've done some cleanup and change how
file locks are handled. I'm going to leave this out of this current
patchset, but I'll keep it in mind for a later cleanup.

[Jeff Layton]

On Sun, 24 Aug 2014 09:06:34 -0700

Post by Christoph Hellwig
Looks good,

Post by Jeff Layton
+ spin_lock(&inode->i_lock);
+ time_out_leases(inode);
for (before = &inode->i_flock;
((fl = *before) != NULL) && IS_LEASE(fl);
before = &fl->fl_next) {
if (fl->fl_file != filp)
continue;
- return fl->fl_lmops->lm_change(before, F_UNLCK);
+ error = fl->fl_lmops->lm_change(before, F_UNLCK);
}

We really should split a lm_release from lm_change, the way it is
used is highly confusing. In addition I think a lot of code
if (fl->fl_file != filp)
continue;
fl = *before;
fl->fl_type = F_UNLCK;
lease_clear_pending(fl, F_UNLCK);
locks_wake_up_blocks(fl);
if (fl->fl_lmops->lm_delete)
fl->fl_lmops->lm_delete(fl);
locks_delete_lock(before, NULL);
static void lease_delete(struct file_lock *fl)
{
struct file *filp = fl->fl_file;
f_delown(filp);
filp->f_owner.signum = 0;
fasync_helper(0, fl->fl_file, 0, &fl->fl_fasync);
if (fl->fl_fasync != NULL) {
printk(KERN_ERR "locks_delete_lock: fasync == %p\n",
fl->fl_fasync);
fl->fl_fasync = NULL;
}
}
and a NULL implementation for delegations.

Good idea. I'll spin that up on the next iteration.

Thanks,

[Jeff Layton]

In later patches, we're going to add a new lock_manager_operation to
finish setting up the lease while still holding the i_lock. To do
this, we'll need to pass a little bit of info in the fcntl setlease
case (primarily an fasync structure). Plumb the extra pointer into
there in advance of that.

We declare this pointer as a void ** to make it clear that this is
auxillary info, and that the caller isn't required to set this unless
the lm_setup specifically requires it.

Signed-off-by: Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/***@public.gmane.org>
---
fs/cifs/cifsfs.c | 7 ++++---
fs/gfs2/file.c | 3 ++-
fs/locks.c | 33 +++++++++++++++++++++------------
fs/nfs/file.c | 2 +-
fs/nfs/internal.h | 2 +-
fs/nfsd/nfs4state.c | 4 ++--
include/linux/fs.h | 10 +++++-----
7 files changed, 36 insertions(+), 25 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 889b98455750..f463d86f097a 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -813,7 +813,8 @@ static loff_t cifs_llseek(struct file *file, loff_t offset, int whence)
return generic_file_llseek(file, offset, whence);
}

-static int cifs_setlease(struct file *file, long arg, struct file_lock **lease)
+static int
+cifs_setlease(struct file *file, long arg, struct file_lock **lease, void **aux)
{
/*
* Note that this is called by vfs setlease with i_lock held to
@@ -829,7 +830,7 @@ static int cifs_setlease(struct file *file, long arg, struct file_lock **lease)
if (arg == F_UNLCK ||
((arg == F_RDLCK) && CIFS_CACHE_READ(CIFS_I(inode))) ||
((arg == F_WRLCK) && CIFS_CACHE_WRITE(CIFS_I(inode))))
- return generic_setlease(file, arg, lease);
+ return generic_setlease(file, arg, lease, aux);
else if (tlink_tcon(cfile->tlink)->local_lease &&
!CIFS_CACHE_READ(CIFS_I(inode)))
/*
@@ -840,7 +841,7 @@ static int cifs_setlease(struct file *file, long arg, struct file_lock **lease)
* knows that the file won't be changed on the server by anyone
* else.
*/
- return generic_setlease(file, arg, lease);
+ return generic_setlease(file, arg, lease, aux);
else
return -EAGAIN;
}
diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index 26b3f952e6b1..253feff90b4e 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -927,7 +927,8 @@ out_uninit:
* Returns: errno
*/

-static int gfs2_setlease(struct file *file, long arg, struct file_lock **fl)
+static int gfs2_setlease(struct file *file, long arg, struct file_lock **fl,
+ void **aux)
{
return -EINVAL;
}
diff --git a/fs/locks.c b/fs/locks.c
index 597e71a1e90f..906e09da115a 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1297,7 +1297,6 @@ int lease_modify(struct file_lock **before, int arg)
}
return 0;
}
-
EXPORT_SYMBOL(lease_modify);

static bool past_time(unsigned long then)
@@ -1543,7 +1542,8 @@ check_conflicting_open(const struct dentry *dentry, const long arg)
return ret;
}

-static int generic_add_lease(struct file *filp, long arg, struct file_lock **flp)
+static int
+generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **aux)
{
struct file_lock *fl, **before, **my_before = NULL, *lease;
struct dentry *dentry = filp->f_path.dentry;
@@ -1607,6 +1607,7 @@ static int generic_add_lease(struct file *filp, long arg, struct file_lock **flp
}

if (my_before != NULL) {
+ lease = *my_before;
error = lease->fl_lmops->lm_change(my_before, arg);
if (!error)
*flp = *my_before;
@@ -1630,11 +1631,14 @@ static int generic_add_lease(struct file *filp, long arg, struct file_lock **flp
smp_mb();
error = check_conflicting_open(dentry, arg);
if (error)
- locks_unlink_lock(before);
+ goto out_unlink;
out:
if (is_deleg)
mutex_unlock(&inode->i_mutex);
return error;
+out_unlink:
+ locks_unlink_lock(before);
+ goto out;
}

static int generic_delete_lease(struct file *filp)
@@ -1658,13 +1662,15 @@ static int generic_delete_lease(struct file *filp)
* @filp: file pointer
* @arg: type of lease to obtain
* @flp: input - file_lock to use, output - file_lock inserted
+ * @aux: auxillary data for lm_setup
*
* The (input) flp->fl_lmops->lm_break function is required
* by break_lease().
*
* Called with inode->i_lock held.
*/
-int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
+int generic_setlease(struct file *filp, long arg, struct file_lock **flp,
+ void **aux)
{
struct dentry *dentry = filp->f_path.dentry;
struct inode *inode = dentry->d_inode;
@@ -1689,19 +1695,20 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
WARN_ON_ONCE(1);
return -ENOLCK;
}
- return generic_add_lease(filp, arg, flp);
+ return generic_add_lease(filp, arg, flp, aux);
default:
return -EINVAL;
}
}
EXPORT_SYMBOL(generic_setlease);

-static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
+static int
+__vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **aux)
{
if (filp->f_op->setlease)
- return filp->f_op->setlease(filp, arg, lease);
+ return filp->f_op->setlease(filp, arg, lease, aux);
else
- return generic_setlease(filp, arg, lease);
+ return generic_setlease(filp, arg, lease, aux);
}

/**
@@ -1709,6 +1716,7 @@ static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
* @filp: file pointer
* @arg: type of lease to obtain
* @lease: file_lock to use when adding a lease
+ * @aux: auxillary info for lm_setup when adding a lease
*
* Call this to establish a lease on the file. The "lease" argument is not
* used for F_UNLCK requests and may be NULL. For commands that set or alter
@@ -1724,13 +1732,14 @@ static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
* this for leases held by processes on this node.
*/

-int vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
+int
+vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **aux)
{
struct inode *inode = file_inode(filp);
int error;

spin_lock(&inode->i_lock);
- error = __vfs_setlease(filp, arg, lease);
+ error = __vfs_setlease(filp, arg, lease, aux);
spin_unlock(&inode->i_lock);

return error;
@@ -1755,7 +1764,7 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
}
ret = fl;
spin_lock(&inode->i_lock);
- error = __vfs_setlease(filp, arg, &ret);
+ error = __vfs_setlease(filp, arg, &ret, NULL);
if (error)
goto out_unlock;
if (ret == fl)
@@ -1793,7 +1802,7 @@ out_unlock:
int fcntl_setlease(unsigned int fd, struct file *filp, long arg)
{
if (arg == F_UNLCK)
- return vfs_setlease(filp, F_UNLCK, NULL);
+ return vfs_setlease(filp, F_UNLCK, NULL, NULL);
return do_fcntl_add_lease(fd, filp, arg);
}

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 524dd80d1898..7221022232d9 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -895,7 +895,7 @@ EXPORT_SYMBOL_GPL(nfs_flock);
* There is no protocol support for leases, so we have no way to implement
* them correctly in the face of opens by other clients.
*/
-int nfs_setlease(struct file *file, long arg, struct file_lock **fl)
+int nfs_setlease(struct file *file, long arg, struct file_lock **fl, void **aux)
{
dprintk("NFS: setlease(%pD2, arg=%ld)\n", file, arg);
return -EINVAL;
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 9056622d2230..dcdc83072c10 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -346,7 +346,7 @@ int nfs_file_release(struct inode *, struct file *);
int nfs_lock(struct file *, int, struct file_lock *);
int nfs_flock(struct file *, int, struct file_lock *);
int nfs_check_flags(int);
-int nfs_setlease(struct file *, long, struct file_lock **);
+int nfs_setlease(struct file *, long, struct file_lock **, void **);

/* inode.c */
extern struct workqueue_struct *nfsiod_workqueue;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d0a6e8e022a2..d964a41c9151 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -683,7 +683,7 @@ static void nfs4_put_deleg_lease(struct nfs4_file *fp)
if (!fp->fi_deleg_file)
return;
if (atomic_dec_and_test(&fp->fi_delegees)) {
- vfs_setlease(fp->fi_deleg_file, F_UNLCK, NULL);
+ vfs_setlease(fp->fi_deleg_file, F_UNLCK, NULL, NULL);
fput(fp->fi_deleg_file);
fp->fi_deleg_file = NULL;
}
@@ -3788,7 +3788,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
}
fl->fl_file = filp;
ret = fl;
- status = vfs_setlease(filp, fl->fl_type, &ret);
+ status = vfs_setlease(filp, fl->fl_type, &fl, NULL);
if (status) {
locks_free_lock(fl);
goto out_fput;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 458f733c96bd..2668d054147f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -982,8 +982,8 @@ extern int vfs_cancel_lock(struct file *filp, struct file_lock *fl);
extern int flock_lock_file_wait(struct file *filp, struct file_lock *fl);
extern int __break_lease(struct inode *inode, unsigned int flags, unsigned int type);
extern void lease_get_mtime(struct inode *, struct timespec *time);
-extern int generic_setlease(struct file *, long, struct file_lock **);
-extern int vfs_setlease(struct file *, long, struct file_lock **);
+extern int generic_setlease(struct file *, long, struct file_lock **, void **aux);
+extern int vfs_setlease(struct file *, long, struct file_lock **, void **aux);
extern int lease_modify(struct file_lock **, int);
#else /* !CONFIG_FILE_LOCKING */
static inline int fcntl_getlk(struct file *file, unsigned int cmd,
@@ -1100,13 +1100,13 @@ static inline void lease_get_mtime(struct inode *inode, struct timespec *time)
}

static inline int generic_setlease(struct file *filp, long arg,
- struct file_lock **flp)
+ struct file_lock **flp, void **aux)
{
return -EINVAL;
}

static inline int vfs_setlease(struct file *filp, long arg,
- struct file_lock **lease)
+ struct file_lock **lease, void **aux)
{
return -EINVAL;
}
@@ -1494,7 +1494,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 **);
+ int (*setlease)(struct file *, long, struct file_lock **, void **);
long (*fallocate)(struct file *file, int mode, loff_t offset,
loff_t len);
int (*show_fdinfo)(struct seq_file *m, struct file *f);

[Christoph Hellwig]

Post by Jeff Layton
In later patches, we're going to add a new lock_manager_operation to
finish setting up the lease while still holding the i_lock. To do
this, we'll need to pass a little bit of info in the fcntl setlease
case (primarily an fasync structure). Plumb the extra pointer into
there in advance of that.
We declare this pointer as a void ** to make it clear that this is
auxillary info, and that the caller isn't required to set this unless
the lm_setup specifically requires it.

Can you just return -EEXIST if reusing an existing one and make it a
normal private pointer a we use elsewhere?

Btw, two things I came up with to make the lease filesystem API nicer:

- bypass vfs_setlease/->setlease for deletes and just call directly
into the lease code. no one does anything special for it (except
cifs, which forgets about it), and then rename the method to
->add_lease.
- provide a no_add_lease for nfs/gfs to centralize the no-op version
in a single place (similar no no_lseek).

Otherwise this looks really nice.

[Jeff Layton]

On Sat, 23 Aug 2014 18:33:05 -0700

Post by Christoph Hellwig

Post by Jeff Layton
In later patches, we're going to add a new lock_manager_operation to
finish setting up the lease while still holding the i_lock. To do
this, we'll need to pass a little bit of info in the fcntl setlease
case (primarily an fasync structure). Plumb the extra pointer into
there in advance of that.
We declare this pointer as a void ** to make it clear that this is
auxillary info, and that the caller isn't required to set this unless
the lm_setup specifically requires it.

Can you just return -EEXIST if reusing an existing one and make it a
normal private pointer a we use elsewhere?

That sounds a little confusing...

We have two pointers we pass down to generic_setlease: the file_lock
itself and with this patch, the "aux" pointer. We can end up using
either, neither or both during a call to generic_setlease.

A simple error code can't properly indicate which of the two pointers
got used. It might be clearer to turn the file_lock into a normal
pointer and return -EEXIST if we reused it, but leave aux as a double
pointer.

Post by Christoph Hellwig
- bypass vfs_setlease/->setlease for deletes and just call directly
into the lease code. no one does anything special for it (except
cifs, which forgets about it), and then rename the method to
->add_lease.
- provide a no_add_lease for nfs/gfs to centralize the no-op version
in a single place (similar no no_lseek).
Otherwise this looks really nice.

Both of those sound good. I'll plan to roll those changes in with the
next iteration.

Thanks!

[Christoph Hellwig]

Post by Jeff Layton

Post by Christoph Hellwig
Can you just return -EEXIST if reusing an existing one and make it a
normal private pointer a we use elsewhere?

That sounds a little confusing...
We have two pointers we pass down to generic_setlease: the file_lock
itself and with this patch, the "aux" pointer. We can end up using
either, neither or both during a call to generic_setlease.
A simple error code can't properly indicate which of the two pointers
got used. It might be clearer to turn the file_lock into a normal
pointer and return -EEXIST if we reused it, but leave aux as a double
pointer.

There is no way we could use a new file_lock but an existing
fasync_struct, as there won't be one on the newly allocated file_lock
structure, but otherwise you're right.

Just rename it to priv then and make me a little less grumpy ;-)

[J. Bruce Fields]

Post by Jeff Layton
In later patches, we're going to add a new lock_manager_operation to
finish setting up the lease while still holding the i_lock. To do
this, we'll need to pass a little bit of info in the fcntl setlease
case (primarily an fasync structure). Plumb the extra pointer into
there in advance of that.
We declare this pointer as a void ** to make it clear that this is
auxillary info, and that the caller isn't required to set this unless
the lm_setup specifically requires it.
---
fs/cifs/cifsfs.c | 7 ++++---
fs/gfs2/file.c | 3 ++-
fs/locks.c | 33 +++++++++++++++++++++------------
fs/nfs/file.c | 2 +-
fs/nfs/internal.h | 2 +-
fs/nfsd/nfs4state.c | 4 ++--
include/linux/fs.h | 10 +++++-----
7 files changed, 36 insertions(+), 25 deletions(-)
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 889b98455750..f463d86f097a 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -813,7 +813,8 @@ static loff_t cifs_llseek(struct file *file, loff_t offset, int whence)
return generic_file_llseek(file, offset, whence);
}
-static int cifs_setlease(struct file *file, long arg, struct file_lock **lease)
+static int
+cifs_setlease(struct file *file, long arg, struct file_lock **lease, void **aux)
{
/*
* Note that this is called by vfs setlease with i_lock held to
@@ -829,7 +830,7 @@ static int cifs_setlease(struct file *file, long arg, struct file_lock **lease)
if (arg == F_UNLCK ||
((arg == F_RDLCK) && CIFS_CACHE_READ(CIFS_I(inode))) ||
((arg == F_WRLCK) && CIFS_CACHE_WRITE(CIFS_I(inode))))
- return generic_setlease(file, arg, lease);
+ return generic_setlease(file, arg, lease, aux);
else if (tlink_tcon(cfile->tlink)->local_lease &&
!CIFS_CACHE_READ(CIFS_I(inode)))
/*
@@ -840,7 +841,7 @@ static int cifs_setlease(struct file *file, long arg, struct file_lock **lease)
* knows that the file won't be changed on the server by anyone
* else.
*/
- return generic_setlease(file, arg, lease);
+ return generic_setlease(file, arg, lease, aux);
else
return -EAGAIN;
}
diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index 26b3f952e6b1..253feff90b4e 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
* Returns: errno
*/
-static int gfs2_setlease(struct file *file, long arg, struct file_lock **fl)
+static int gfs2_setlease(struct file *file, long arg, struct file_lock **fl,
+ void **aux)
{
return -EINVAL;
}
diff --git a/fs/locks.c b/fs/locks.c
index 597e71a1e90f..906e09da115a 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1297,7 +1297,6 @@ int lease_modify(struct file_lock **before, int arg)
}
return 0;
}
-
EXPORT_SYMBOL(lease_modify);
static bool past_time(unsigned long then)
@@ -1543,7 +1542,8 @@ check_conflicting_open(const struct dentry *dentry, const long arg)
return ret;
}
-static int generic_add_lease(struct file *filp, long arg, struct file_lock **flp)
+static int
+generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **aux)
{
struct file_lock *fl, **before, **my_before = NULL, *lease;
struct dentry *dentry = filp->f_path.dentry;
@@ -1607,6 +1607,7 @@ static int generic_add_lease(struct file *filp, long arg, struct file_lock **flp
}
if (my_before != NULL) {
+ lease = *my_before;

What's this doing in this patch?

--b.

Post by Jeff Layton
error = lease->fl_lmops->lm_change(my_before, arg);
if (!error)
*flp = *my_before;
@@ -1630,11 +1631,14 @@ static int generic_add_lease(struct file *filp, long arg, struct file_lock **flp
smp_mb();
error = check_conflicting_open(dentry, arg);
if (error)
- locks_unlink_lock(before);
+ goto out_unlink;
if (is_deleg)
mutex_unlock(&inode->i_mutex);
return error;
+ locks_unlink_lock(before);
+ goto out;
}
static int generic_delete_lease(struct file *filp)
@@ -1658,13 +1662,15 @@ static int generic_delete_lease(struct file *filp)
*
* The (input) flp->fl_lmops->lm_break function is required
* by break_lease().
*
* Called with inode->i_lock held.
*/
-int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
+int generic_setlease(struct file *filp, long arg, struct file_lock **flp,
+ void **aux)
{
struct dentry *dentry = filp->f_path.dentry;
struct inode *inode = dentry->d_inode;
@@ -1689,19 +1695,20 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
WARN_ON_ONCE(1);
return -ENOLCK;
}
- return generic_add_lease(filp, arg, flp);
+ return generic_add_lease(filp, arg, flp, aux);
return -EINVAL;
}
}
EXPORT_SYMBOL(generic_setlease);
-static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
+static int
+__vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **aux)
{
if (filp->f_op->setlease)
- return filp->f_op->setlease(filp, arg, lease);
+ return filp->f_op->setlease(filp, arg, lease, aux);
else
- return generic_setlease(filp, arg, lease);
+ return generic_setlease(filp, arg, lease, aux);
}
/**
@@ -1709,6 +1716,7 @@ static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
*
* Call this to establish a lease on the file. The "lease" argument is not
* used for F_UNLCK requests and may be NULL. For commands that set or alter
@@ -1724,13 +1732,14 @@ static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
* this for leases held by processes on this node.
*/
-int vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
+int
+vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **aux)
{
struct inode *inode = file_inode(filp);
int error;
spin_lock(&inode->i_lock);
- error = __vfs_setlease(filp, arg, lease);
+ error = __vfs_setlease(filp, arg, lease, aux);
spin_unlock(&inode->i_lock);
return error;
@@ -1755,7 +1764,7 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
}
ret = fl;
spin_lock(&inode->i_lock);
- error = __vfs_setlease(filp, arg, &ret);
+ error = __vfs_setlease(filp, arg, &ret, NULL);
if (error)
goto out_unlock;
if (ret == fl)
int fcntl_setlease(unsigned int fd, struct file *filp, long arg)
{
if (arg == F_UNLCK)
- return vfs_setlease(filp, F_UNLCK, NULL);
+ return vfs_setlease(filp, F_UNLCK, NULL, NULL);
return do_fcntl_add_lease(fd, filp, arg);
}
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 524dd80d1898..7221022232d9 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -895,7 +895,7 @@ EXPORT_SYMBOL_GPL(nfs_flock);
* There is no protocol support for leases, so we have no way to implement
* them correctly in the face of opens by other clients.
*/
-int nfs_setlease(struct file *file, long arg, struct file_lock **fl)
+int nfs_setlease(struct file *file, long arg, struct file_lock **fl, void **aux)
{
dprintk("NFS: setlease(%pD2, arg=%ld)\n", file, arg);
return -EINVAL;
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 9056622d2230..dcdc83072c10 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -346,7 +346,7 @@ int nfs_file_release(struct inode *, struct file *);
int nfs_lock(struct file *, int, struct file_lock *);
int nfs_flock(struct file *, int, struct file_lock *);
int nfs_check_flags(int);
-int nfs_setlease(struct file *, long, struct file_lock **);
+int nfs_setlease(struct file *, long, struct file_lock **, void **);
/* inode.c */
extern struct workqueue_struct *nfsiod_workqueue;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d0a6e8e022a2..d964a41c9151 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -683,7 +683,7 @@ static void nfs4_put_deleg_lease(struct nfs4_file *fp)
if (!fp->fi_deleg_file)
return;
if (atomic_dec_and_test(&fp->fi_delegees)) {
- vfs_setlease(fp->fi_deleg_file, F_UNLCK, NULL);
+ vfs_setlease(fp->fi_deleg_file, F_UNLCK, NULL, NULL);
fput(fp->fi_deleg_file);
fp->fi_deleg_file = NULL;
}
@@ -3788,7 +3788,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
}
fl->fl_file = filp;
ret = fl;
- status = vfs_setlease(filp, fl->fl_type, &ret);
+ status = vfs_setlease(filp, fl->fl_type, &fl, NULL);
if (status) {
locks_free_lock(fl);
goto out_fput;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 458f733c96bd..2668d054147f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -982,8 +982,8 @@ extern int vfs_cancel_lock(struct file *filp, struct file_lock *fl);
extern int flock_lock_file_wait(struct file *filp, struct file_lock *fl);
extern int __break_lease(struct inode *inode, unsigned int flags, unsigned int type);
extern void lease_get_mtime(struct inode *, struct timespec *time);
-extern int generic_setlease(struct file *, long, struct file_lock **);
-extern int vfs_setlease(struct file *, long, struct file_lock **);
+extern int generic_setlease(struct file *, long, struct file_lock **, void **aux);
+extern int vfs_setlease(struct file *, long, struct file_lock **, void **aux);
extern int lease_modify(struct file_lock **, int);
#else /* !CONFIG_FILE_LOCKING */
static inline int fcntl_getlk(struct file *file, unsigned int cmd,
@@ -1100,13 +1100,13 @@ static inline void lease_get_mtime(struct inode *inode, struct timespec *time)
}
static inline int generic_setlease(struct file *filp, long arg,
- struct file_lock **flp)
+ struct file_lock **flp, void **aux)
{
return -EINVAL;
}
static inline int vfs_setlease(struct file *filp, long arg,
- struct file_lock **lease)
+ struct file_lock **lease, void **aux)
{
return -EINVAL;
}
@@ -1494,7 +1494,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 **);
+ int (*setlease)(struct file *, long, struct file_lock **, void **);
long (*fallocate)(struct file *file, int mode, loff_t offset,
loff_t len);
int (*show_fdinfo)(struct seq_file *m, struct file *f);
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Jeff Layton]

On Mon, 25 Aug 2014 16:28:52 -0400

Post by J. Bruce Fields

Post by Jeff Layton
In later patches, we're going to add a new lock_manager_operation to
finish setting up the lease while still holding the i_lock. To do
this, we'll need to pass a little bit of info in the fcntl setlease
case (primarily an fasync structure). Plumb the extra pointer into
there in advance of that.
We declare this pointer as a void ** to make it clear that this is
auxillary info, and that the caller isn't required to set this unless
the lm_setup specifically requires it.
---
fs/cifs/cifsfs.c | 7 ++++---
fs/gfs2/file.c | 3 ++-
fs/locks.c | 33 +++++++++++++++++++++------------
fs/nfs/file.c | 2 +-
fs/nfs/internal.h | 2 +-
fs/nfsd/nfs4state.c | 4 ++--
include/linux/fs.h | 10 +++++-----
7 files changed, 36 insertions(+), 25 deletions(-)
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 889b98455750..f463d86f097a 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -813,7 +813,8 @@ static loff_t cifs_llseek(struct file *file, loff_t offset, int whence)
return generic_file_llseek(file, offset, whence);
}
-static int cifs_setlease(struct file *file, long arg, struct file_lock **lease)
+static int
+cifs_setlease(struct file *file, long arg, struct file_lock **lease, void **aux)
{
/*
* Note that this is called by vfs setlease with i_lock held to
@@ -829,7 +830,7 @@ static int cifs_setlease(struct file *file, long arg, struct file_lock **lease)
if (arg == F_UNLCK ||
((arg == F_RDLCK) && CIFS_CACHE_READ(CIFS_I(inode))) ||
((arg == F_WRLCK) && CIFS_CACHE_WRITE(CIFS_I(inode))))
- return generic_setlease(file, arg, lease);
+ return generic_setlease(file, arg, lease, aux);
else if (tlink_tcon(cfile->tlink)->local_lease &&
!CIFS_CACHE_READ(CIFS_I(inode)))
/*
@@ -840,7 +841,7 @@ static int cifs_setlease(struct file *file, long arg, struct file_lock **lease)
* knows that the file won't be changed on the server by anyone
* else.
*/
- return generic_setlease(file, arg, lease);
+ return generic_setlease(file, arg, lease, aux);
else
return -EAGAIN;
}
diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index 26b3f952e6b1..253feff90b4e 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
* Returns: errno
*/
-static int gfs2_setlease(struct file *file, long arg, struct file_lock **fl)
+static int gfs2_setlease(struct file *file, long arg, struct file_lock **fl,
+ void **aux)
{
return -EINVAL;
}
diff --git a/fs/locks.c b/fs/locks.c
index 597e71a1e90f..906e09da115a 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1297,7 +1297,6 @@ int lease_modify(struct file_lock **before, int arg)
}
return 0;
}
-
EXPORT_SYMBOL(lease_modify);
static bool past_time(unsigned long then)
@@ -1543,7 +1542,8 @@ check_conflicting_open(const struct dentry *dentry, const long arg)
return ret;
}
-static int generic_add_lease(struct file *filp, long arg, struct file_lock **flp)
+static int
+generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **aux)
{
struct file_lock *fl, **before, **my_before = NULL, *lease;
struct dentry *dentry = filp->f_path.dentry;
@@ -1607,6 +1607,7 @@ static int generic_add_lease(struct file *filp, long arg, struct file_lock **flp
}
if (my_before != NULL) {
+ lease = *my_before;

What's this doing in this patch?
--b.

Yeah, it probably doesn't belong here, but it should be harmless. Once
we've found an existing lease in the list we have no need for the one
passed in. I'll move this change into one of the later patches.

Post by J. Bruce Fields

Post by Jeff Layton
error = lease->fl_lmops->lm_change(my_before, arg);

The main reason for this sort of change is the above wart. It's calling
"lease's" method (which currently points at *flp) on "*my_before",
which is just plain wrong.

Post by J. Bruce Fields

Post by Jeff Layton
if (!error)
*flp = *my_before;
@@ -1630,11 +1631,14 @@ static int generic_add_lease(struct file *filp, long arg, struct file_lock **flp
smp_mb();
error = check_conflicting_open(dentry, arg);
if (error)
- locks_unlink_lock(before);
+ goto out_unlink;
if (is_deleg)
mutex_unlock(&inode->i_mutex);
return error;
+ locks_unlink_lock(before);
+ goto out;
}
static int generic_delete_lease(struct file *filp)
@@ -1658,13 +1662,15 @@ static int generic_delete_lease(struct file *filp)
*
* The (input) flp->fl_lmops->lm_break function is required
* by break_lease().
*
* Called with inode->i_lock held.
*/
-int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
+int generic_setlease(struct file *filp, long arg, struct file_lock **flp,
+ void **aux)
{
struct dentry *dentry = filp->f_path.dentry;
struct inode *inode = dentry->d_inode;
@@ -1689,19 +1695,20 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
WARN_ON_ONCE(1);
return -ENOLCK;
}
- return generic_add_lease(filp, arg, flp);
+ return generic_add_lease(filp, arg, flp, aux);
return -EINVAL;
}
}
EXPORT_SYMBOL(generic_setlease);
-static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
+static int
+__vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **aux)
{
if (filp->f_op->setlease)
- return filp->f_op->setlease(filp, arg, lease);
+ return filp->f_op->setlease(filp, arg, lease, aux);
else
- return generic_setlease(filp, arg, lease);
+ return generic_setlease(filp, arg, lease, aux);
}
/**
@@ -1709,6 +1716,7 @@ static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
*
* Call this to establish a lease on the file. The "lease" argument is not
* used for F_UNLCK requests and may be NULL. For commands that set or alter
@@ -1724,13 +1732,14 @@ static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
* this for leases held by processes on this node.
*/
-int vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
+int
+vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **aux)
{
struct inode *inode = file_inode(filp);
int error;
spin_lock(&inode->i_lock);
- error = __vfs_setlease(filp, arg, lease);
+ error = __vfs_setlease(filp, arg, lease, aux);
spin_unlock(&inode->i_lock);
return error;
@@ -1755,7 +1764,7 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
}
ret = fl;
spin_lock(&inode->i_lock);
- error = __vfs_setlease(filp, arg, &ret);
+ error = __vfs_setlease(filp, arg, &ret, NULL);
if (error)
goto out_unlock;
if (ret == fl)
int fcntl_setlease(unsigned int fd, struct file *filp, long arg)
{
if (arg == F_UNLCK)
- return vfs_setlease(filp, F_UNLCK, NULL);
+ return vfs_setlease(filp, F_UNLCK, NULL, NULL);
return do_fcntl_add_lease(fd, filp, arg);
}
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 524dd80d1898..7221022232d9 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -895,7 +895,7 @@ EXPORT_SYMBOL_GPL(nfs_flock);
* There is no protocol support for leases, so we have no way to implement
* them correctly in the face of opens by other clients.
*/
-int nfs_setlease(struct file *file, long arg, struct file_lock **fl)
+int nfs_setlease(struct file *file, long arg, struct file_lock **fl, void **aux)
{
dprintk("NFS: setlease(%pD2, arg=%ld)\n", file, arg);
return -EINVAL;
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 9056622d2230..dcdc83072c10 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -346,7 +346,7 @@ int nfs_file_release(struct inode *, struct file *);
int nfs_lock(struct file *, int, struct file_lock *);
int nfs_flock(struct file *, int, struct file_lock *);
int nfs_check_flags(int);
-int nfs_setlease(struct file *, long, struct file_lock **);
+int nfs_setlease(struct file *, long, struct file_lock **, void **);
/* inode.c */
extern struct workqueue_struct *nfsiod_workqueue;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d0a6e8e022a2..d964a41c9151 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -683,7 +683,7 @@ static void nfs4_put_deleg_lease(struct nfs4_file *fp)
if (!fp->fi_deleg_file)
return;
if (atomic_dec_and_test(&fp->fi_delegees)) {
- vfs_setlease(fp->fi_deleg_file, F_UNLCK, NULL);
+ vfs_setlease(fp->fi_deleg_file, F_UNLCK, NULL, NULL);
fput(fp->fi_deleg_file);
fp->fi_deleg_file = NULL;
}
@@ -3788,7 +3788,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
}
fl->fl_file = filp;
ret = fl;
- status = vfs_setlease(filp, fl->fl_type, &ret);
+ status = vfs_setlease(filp, fl->fl_type, &fl, NULL);
if (status) {
locks_free_lock(fl);
goto out_fput;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 458f733c96bd..2668d054147f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -982,8 +982,8 @@ extern int vfs_cancel_lock(struct file *filp, struct file_lock *fl);
extern int flock_lock_file_wait(struct file *filp, struct file_lock *fl);
extern int __break_lease(struct inode *inode, unsigned int flags, unsigned int type);
extern void lease_get_mtime(struct inode *, struct timespec *time);
-extern int generic_setlease(struct file *, long, struct file_lock **);
-extern int vfs_setlease(struct file *, long, struct file_lock **);
+extern int generic_setlease(struct file *, long, struct file_lock **, void **aux);
+extern int vfs_setlease(struct file *, long, struct file_lock **, void **aux);
extern int lease_modify(struct file_lock **, int);
#else /* !CONFIG_FILE_LOCKING */
static inline int fcntl_getlk(struct file *file, unsigned int cmd,
@@ -1100,13 +1100,13 @@ static inline void lease_get_mtime(struct inode *inode, struct timespec *time)
}
static inline int generic_setlease(struct file *filp, long arg,
- struct file_lock **flp)
+ struct file_lock **flp, void **aux)
{
return -EINVAL;
}
static inline int vfs_setlease(struct file *filp, long arg,
- struct file_lock **lease)
+ struct file_lock **lease, void **aux)
{
return -EINVAL;
}
@@ -1494,7 +1494,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 **);
+ int (*setlease)(struct file *, long, struct file_lock **, void **);
long (*fallocate)(struct file *file, int mode, loff_t offset,
loff_t len);
int (*show_fdinfo)(struct seq_file *m, struct file *f);
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Jeff Layton]

Some of the latter paragraphs seem ambiguous and just plain wrong.
In particular the break_lease comment makes no sense. We call
break_lease (and break_deleg) from all sorts of vfs-layer functions,
so there is clearly such a method.

Also, we are close to being able to allow for "real" filesystem
setlease methods so remove the final comment about it not being a
full implementation yet.

Signed-off-by: Jeff Layton <***@primarydata.com>
---
fs/locks.c | 41 +++++++++++++++++------------------------
1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index bedb817a5cc4..597e71a1e90f 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1705,30 +1705,23 @@ static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
}

/**
- * vfs_setlease - sets a lease on an open file
- * @filp: file pointer
- * @arg: type of lease to obtain
- * @lease: file_lock to use
- *
- * Call this to establish a lease on the file.
- * The (*lease)->fl_lmops->lm_break operation must be set; if not,
- * break_lease will oops!
- *
- * This will call the filesystem's setlease file method, if
- * defined. Note that there is no getlease method; instead, the
- * filesystem setlease method should call back to setlease() to
- * add a lease to the inode's lease list, where fcntl_getlease() can
- * find it. Since fcntl_getlease() only reports whether the current
- * task holds a lease, a cluster filesystem need only do this for
- * leases held by processes on this node.
- *
- * There is also no break_lease method; filesystems that
- * handle their own leases should break leases themselves from the
- * filesystem's open, create, and (on truncate) setattr methods.
- *
- * Warning: the only current setlease methods exist only to disable
- * leases in certain cases. More vfs changes may be required to
- * allow a full filesystem lease implementation.
+ * vfs_setlease - sets a lease on an open file
+ * @filp: file pointer
+ * @arg: type of lease to obtain
+ * @lease: file_lock to use when adding a lease
+ *
+ * Call this to establish a lease on the file. The "lease" argument is not
+ * used for F_UNLCK requests and may be NULL. For commands that set or alter
+ * an existing lease, the (*lease)->fl_lmops->lm_break operation must be set;
+ * if not, this function will return -EINVAL (and generate a scary-looking
+ * stack trace).
+ *
+ * This will call the filesystem's setlease file method, if defined. Note that
+ * there is no getlease method; instead, the filesystem setlease method should
+ * call back to generic_setlease() to add a lease to the inode's lease list,
+ * where fcntl_getlease() can find it. Since fcntl_getlease() only reports
+ * whether the current task holds a lease, a cluster filesystem need only do
+ * this for leases held by processes on this node.
*/

int vfs_setlease(struct file *filp, long arg, struct file_lock **lease)

[Christoph Hellwig]

Post by Jeff Layton
Some of the latter paragraphs seem ambiguous and just plain wrong.
In particular the break_lease comment makes no sense. We call
break_lease (and break_deleg) from all sorts of vfs-layer functions,
so there is clearly such a method.
Also, we are close to being able to allow for "real" filesystem
setlease methods so remove the final comment about it not being a
full implementation yet.
+ *
+ * This will call the filesystem's setlease file method, if defined. Note that
+ * there is no getlease method; instead, the filesystem setlease method should
+ * call back to generic_setlease() to add a lease to the inode's lease list,
+ * where fcntl_getlease() can find it. Since fcntl_getlease() only reports
+ * whether the current task holds a lease, a cluster filesystem need only do
+ * this for leases held by processes on this node.
*/

If we'd ever want a full implementation I think we'd absolutely need
the getlease method. But instead of hypothetizing about future
implementation I'd rather leave it to those actually implementing such
support, if that ever happens.

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[J. Bruce Fields]

Post by Christoph Hellwig

Post by Jeff Layton
Some of the latter paragraphs seem ambiguous and just plain wrong.
In particular the break_lease comment makes no sense. We call
break_lease (and break_deleg) from all sorts of vfs-layer functions,
so there is clearly such a method.
Also, we are close to being able to allow for "real" filesystem
setlease methods so remove the final comment about it not being a
full implementation yet.
+ *
+ * This will call the filesystem's setlease file method, if defined. Note that
+ * there is no getlease method; instead, the filesystem setlease method should
+ * call back to generic_setlease() to add a lease to the inode's lease list,
+ * where fcntl_getlease() can find it. Since fcntl_getlease() only reports
+ * whether the current task holds a lease, a cluster filesystem need only do
+ * this for leases held by processes on this node.
*/

If we'd ever want a full implementation I think we'd absolutely need
the getlease method. But instead of hypothetizing about future
implementation I'd rather leave it to those actually implementing such
support, if that ever happens.

I agree, that makes sense.

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Jeff Layton]

Now that we don't need to pass in an actual lease pointer to
vfs_setlease on unlock, we can stop tracking a pointer to the lease in
the nfs4_file.

Switch all of the places that check the fi_lease to check fi_deleg_file
instead. We always set that at the same time so it will have the same
semantics.

Signed-off-by: Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/***@public.gmane.org>
---
fs/nfsd/nfs4state.c | 10 ++++------
fs/nfsd/state.h | 1 -
2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 0cd252916e1a..d0a6e8e022a2 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -680,11 +680,10 @@ static void nfs4_put_deleg_lease(struct nfs4_file *fp)
{
lockdep_assert_held(&state_lock);

- if (!fp->fi_lease)
+ if (!fp->fi_deleg_file)
return;
if (atomic_dec_and_test(&fp->fi_delegees)) {
vfs_setlease(fp->fi_deleg_file, F_UNLCK, NULL);
- fp->fi_lease = NULL;
fput(fp->fi_deleg_file);
fp->fi_deleg_file = NULL;
}
@@ -3061,8 +3060,8 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct knfsd_fh *fh)
INIT_LIST_HEAD(&fp->fi_stateids);
INIT_LIST_HEAD(&fp->fi_delegations);
fh_copy_shallow(&fp->fi_fhandle, fh);
+ fp->fi_deleg_file = NULL;
fp->fi_had_conflict = false;
- fp->fi_lease = NULL;
fp->fi_share_deny = 0;
memset(fp->fi_fds, 0, sizeof(fp->fi_fds));
memset(fp->fi_access, 0, sizeof(fp->fi_access));
@@ -3803,13 +3802,12 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
if (fp->fi_had_conflict)
goto out_unlock;
/* Race breaker */
- if (fp->fi_lease) {
+ if (fp->fi_deleg_file) {
status = 0;
atomic_inc(&fp->fi_delegees);
hash_delegation_locked(dp, fp);
goto out_unlock;
}
- fp->fi_lease = fl;
fp->fi_deleg_file = filp;
atomic_set(&fp->fi_delegees, 1);
hash_delegation_locked(dp, fp);
@@ -3842,7 +3840,7 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
spin_lock(&state_lock);
spin_lock(&fp->fi_lock);
dp->dl_stid.sc_file = fp;
- if (!fp->fi_lease) {
+ if (!fp->fi_deleg_file) {
spin_unlock(&fp->fi_lock);
spin_unlock(&state_lock);
status = nfs4_setlease(dp);
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 4a89e00d7461..64f291a25a8c 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -477,7 +477,6 @@ struct nfs4_file {
atomic_t fi_access[2];
u32 fi_share_deny;
struct file *fi_deleg_file;
- struct file_lock *fi_lease;
atomic_t fi_delegees;
struct knfsd_fh fi_fhandle;
bool fi_had_conflict;

[Christoph Hellwig]

Post by Jeff Layton
Now that we don't need to pass in an actual lease pointer to
vfs_setlease on unlock, we can stop tracking a pointer to the lease in
the nfs4_file.
Switch all of the places that check the fi_lease to check fi_deleg_file
instead. We always set that at the same time so it will have the same
semantics.

Looks good,

Reviewed-by: Christoph Hellwig <hch-***@public.gmane.org>

[Jeff Layton]

It's unlikely to ever occur, but if there were already a lease set on
the file then we could end up getting back a different pointer on a
successful setlease attempt than the one we allocated. If that happens,
the one we allocated could leak.

In practice, I don't think this will happen due to the fact that we only
try to set up the lease once per nfs4_file, but this error handling is a
bit more correct given the current lease API.

Cc: J. Bruce Fields <bfields-***@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/***@public.gmane.org>
---
fs/nfsd/nfs4state.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index fd5ff4b17292..29fac18d9102 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3774,7 +3774,7 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_file *fp, int flag)
static int nfs4_setlease(struct nfs4_delegation *dp)
{
struct nfs4_file *fp = dp->dl_stid.sc_file;
- struct file_lock *fl;
+ struct file_lock *fl, *ret;
struct file *filp;
int status = 0;

@@ -3788,11 +3788,14 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
return -EBADF;
}
fl->fl_file = filp;
- status = vfs_setlease(filp, fl->fl_type, &fl);
+ ret = fl;
+ status = vfs_setlease(filp, fl->fl_type, &ret);
if (status) {
locks_free_lock(fl);
goto out_fput;
}
+ if (ret != fl)
+ locks_free_lock(fl);
spin_lock(&state_lock);
spin_lock(&fp->fi_lock);
/* Did the lease get broken before we took the lock? */

[Christoph Hellwig]

Post by Jeff Layton
It's unlikely to ever occur, but if there were already a lease set on
the file then we could end up getting back a different pointer on a
successful setlease attempt than the one we allocated. If that happens,
the one we allocated could leak.
In practice, I don't think this will happen due to the fact that we only
try to set up the lease once per nfs4_file, but this error handling is a
bit more correct given the current lease API.

Looks good,

Reviewed-by: Christoph Hellwig <hch-***@public.gmane.org>

[Christoph Hellwig]

One more wishlist item in addition to the one mentioned in the patches:

- add a return value to lm_break so that the lock manager can tell the
core code "you can delete this lease right now". That gets rid of
the games with the timeout which require all kinds of race avoidance
code in the users.

And a big one that doesn't seem easily addressable, but I'll drop it in
anyway:

- calling ->lm_break without i_lock would make everyones life a heck
lot easier..

[Jeff Layton]

On Sun, 24 Aug 2014 09:10:46 -0700

Post by Christoph Hellwig
- add a return value to lm_break so that the lock manager can tell the
core code "you can delete this lease right now". That gets rid of
the games with the timeout which require all kinds of race avoidance
code in the users.

I'm not sure I understand what you're suggesting here. Isn't it just as
simple to have lm_break call lease_modify to just remove the lease?

Post by Christoph Hellwig
And a big one that doesn't seem easily addressable, but I'll drop it in
- calling ->lm_break without i_lock would make everyones life a heck
lot easier..

Indeed. I have some ideas for a larger cleanup of the file locking
code. This series is really sort of prep work for that overhaul, and
I'm hoping we'll be able to do something along those lines.

[Christoph Hellwig]

Post by Jeff Layton

Post by Christoph Hellwig
- add a return value to lm_break so that the lock manager can tell the
core code "you can delete this lease right now". That gets rid of
the games with the timeout which require all kinds of race avoidance
code in the users.

I'm not sure I understand what you're suggesting here. Isn't it just as
simple to have lm_break call lease_modify to just remove the lease?

Unfortunately it's not. lm_break gets a pointer to the file_lock,
and lease_modify needs a pointer to the list position of the file_lock.