cluster-coherent leases

From: J. Bruce Fields <[email protected]>

Currently leases are only kept locally, so there's no way for a distributed filesystem to enforce them against multiple clients. We're particularly interested in the case of nfsd exporting a cluster filesystem, in which case nfsd needs cluster-coherent leases in order to implement delegations correctly.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/locks.c         |    5 ++++-
 include/linux/fs.h |    4 ++++
 2 files changed, 8 insertions(+), 1 deletions(-)

fs/locks.c +4 -1

index 3f366e1..40a7f39 100644
@@ -1444,7 +1444,10 @@ int setlease(struct file *filp, long arg, struct file_lock **lease)
 		return error;
 
 	lock_kernel();
-	error = __setlease(filp, arg, lease);
+	if (filp->f_op && filp->f_op->set_lease)
+		error = filp->f_op->set_lease(filp, arg, lease);
+        else
+		error = __setlease(filp, arg, lease);
 	unlock_kernel();
 
 	return error;

include/linux/fs.h +1 -0

index 7cf0c54..09aefb4 100644
@@ -1112,6 +1112,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 (*set_lease)(struct file *, long, struct file_lock **);
 };
 
 struct inode_operations {

Quoted 11 lines · J. Bruce Fields

include/linux/fs.h +1 -0

index 7cf0c54..09aefb4 100644
@@ -1112,6 +1112,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 (*set_lease)(struct file *, long, struct file_lock **);
 };
 

include/linux/fs.h struct inode_operations { · line 1141

Quoted 6 lines

include/linux/fs.h +1 -0

 struct inode_operations {

@@ -1137,6 +1138,7 @@ struct inode_operations {
 	ssize_t (*listxattr) (struct dentry *, char *, size_t);
 	int (*removexattr) (struct dentry *, const char *);
 	void (*truncate_range)(struct inode *, loff_t, loff_t);
+	int (*break_lease)(struct inode *, unsigned int);

Splitting the lease into a file_operation part and an inode_operation part looks really ugly.

It also means that you're calling twice down
into the filesystem for every call to may_open() (once for vfs_permission() and once for break_lease()) and 3 times in do_sys_truncate().

Would it perhaps make sense to package up the call to vfs_permission() and break_lease() as a single 'may_open()' inode operation that could be called by may_open(), do_sys_truncate() and nfsd?

Quoted 8 lines

It also means that you're calling twice down
into the filesystem for every call to may_open() (once for vfs_permission() and once for break_lease()) and 3 times in do_sys_truncate().

Would it perhaps make sense to package up the call to vfs_permission() and break_lease() as a single 'may_open()' inode operation that could be called by may_open(), do_sys_truncate() and nfsd?

Could be, but this may no longer make sense when we try to do lease-breaking on rename and unlink.

Quoted 6 lines

Quoted 6 lines

It also means that you're calling twice down
into the filesystem for every call to may_open() (once for vfs_permission() and once for break_lease()) and 3 times in do_sys_truncate().

Would it perhaps make sense to package up the call to

Quoted 1 line

Quoted 1 line

and break_lease() as a single 'may_open()' inode operation that

Quoted 4 lines

Quoted 1 line

called by may_open(), do_sys_truncate() and nfsd?

Could be, but this may no longer make sense when we try to do lease-breaking on rename and unlink.

Hmm... Clustered lease breaking on rename and unlink really needs to be done _atomically_ with the actual operation. It really needs to be done inside the ->rename() and ->unlink() operation because only the filesystem can guarantee atomicity.

Thinking about it, perhaps the may_open() thing is wrong too: again, you have atomicity requirements inside the open() call. A break_lease() inode operation won't help either...

Trond

This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

A cluster filesystem don't need the inode operation breake_lease. The fs must be able to detect the conflict and break the lease using __break_lease() call on all the nodes that hold a conflicting leases. Marc.

Currently, the lease handling is done all in the VFS, and is done prior to calling any filesystem operations. Bruce's break_lease() inode operation allows the VFS to notify the filesystem that some operation is going to be called that requires the lease to be broken.

My point is that in doing so, you are not atomic with the operation that requires the lease to be broken. Some different node may re-establish a lease while we're calling back down into the filesystem to perform the operation.
So I agree with you. The break_lease() inode operation isn't going to work. The filesystem is going to have to figure out for itself when it needs to notify other nodes that the lease needs breaking, and it needs to figure out its own methods for ensuring atomicity.

Quoted 5 lines · Trond Myklebust

Currently, the lease handling is done all in the VFS, and is done prior to calling any filesystem operations. Bruce's break_lease() inode operation allows the VFS to notify the filesystem that some operation is going to be called that requires the lease to be broken.

Quoted 8 lines

My point is that in doing so, you are not atomic with the operation that requires the lease to be broken. Some different node may re-establish a lease while we're calling back down into the filesystem to perform the operation.
So I agree with you. The break_lease() inode operation isn't going to work. The filesystem is going to have to figure out for itself when it needs to notify other nodes that the lease needs breaking, and it needs to figure out its own methods for ensuring atomicity.

OK, I agree with you both, thanks for the explanations.

It looks to me like there's probably a race in the existing code that will allow conflicting opens and leases to be granted simultaneously if the lease request is handled just after may_open() is called. These checks at the beginning of __setlease() are an attempt to prevent that race:

        if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
                goto out;
        if ((arg == F_WRLCK)
            && ((atomic_read(&dentry->d_count) > 1)
                || (atomic_read(&inode->i_count) > 1)))
                goto out;

But, for example, in the case of a simultaneous write open and RDLCK lease request, I believe the call to setlease could come after the may_open() but before the call to get_write_access() that bumps i_writecount.

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

I have had some previous communications with Bruce on these topics, and I am generally pleased with the proposed modifications that are presented here.

I am working on a clustered file system and there is one small additional modification that would be of great use to me. That would be to export the __setlease symbol.

In my implementation, my file system specific set_lease() function first determines whether the granting of the requested lease on the given inode is compatible with the cluster state of this inode, and then if it is, I simply invoke the __setlease() routine and have the kernel build the associated infrastructure.

Having this symbol globally available greatly simplifies things.

On 6/4/07, J. Bruce Fields <[email protected]> wrote:
On Sat, Jun 02, 2007 at 02:21:22PM -0400, Trond Myklebust wrote:
> Currently, the lease handling is done all in the VFS, and is done prior
> to calling any filesystem operations. Bruce's break_lease() inode
> operation allows the VFS to notify the filesystem that some operation is
> going to be called that requires the lease to be broken. >
> My point is that in doing so, you are not atomic with the operation that
> requires the lease to be broken. Some different node may re-establish a
> lease while we're calling back down into the filesystem to perform the
> operation.
> So I agree with you. The break_lease() inode operation isn't going to
> work. The filesystem is going to have to figure out for itself when it
> needs to notify other nodes that the lease needs breaking, and it needs
> to figure out its own methods for ensuring atomicity.

OK, I agree with you both, thanks for the explanations.

It looks to me like there's probably a race in the existing code that
will allow conflicting opens and leases to be granted simultaneously if
the lease request is handled just after may_open() is called. These
checks at the beginning of __setlease() are an attempt to prevent that
race:

if ((arg == F_RDLCK) &&
(atomic_read(&inode->i_writecount) > 0))
goto out;
if ((arg == F_WRLCK)
&& ((atomic_read(&dentry->d_count) > 1)
|| (atomic_read(&inode->i_count) > 1))) goto out;

But, for example, in the case of a simultaneous write open and RDLCK
lease request, I believe the call to setlease could come after the
may_open() but before the call to get_write_access() that bumps
i_writecount.

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

I have had some previous communications with Bruce on these topics, and I am generally pleased with the proposed modifications that are presented here.

I am working on a clustered file system and there is one small additional modification that would be of great use to me. That would be to export the __setlease symbol.

In my implementation, my file system specific set_lease() function first determines whether the granting of the requested lease on the given inode is compatible with the cluster state of this inode, and then if it is, I simply invoke the __setlease() routine and have the kernel build the associated infrastructure.

Having this symbol globally available greatly simplifies things.

On 6/4/07, J. Bruce Fields <[email protected] > wrote:

Quoted 1 line · Trond Myklebust

Currently, the lease handling is done all in the VFS, and is done

Quoted 2 lines

to calling any filesystem operations. Bruce's break_lease() inode operation allows the VFS to notify the filesystem that some operation

Quoted 3 lines

going to be called that requires the lease to be broken.

My point is that in doing so, you are not atomic with the operation

Quoted 1 line

requires the lease to be broken. Some different node may re-establish

Quoted 6 lines

lease while we're calling back down into the filesystem to perform the

operation.
So I agree with you. The break_lease() inode operation isn't going to work. The filesystem is going to have to figure out for itself when it needs to notify other nodes that the lease needs breaking, and it

Quoted 1 line

to figure out its own methods for ensuring atomicity.

OK, I agree with you both, thanks for the explanations.

It looks to me like there's probably a race in the existing code that will allow conflicting opens and leases to be granted simultaneously if the lease request is handled just after may_open() is called. These checks at the beginning of __setlease() are an attempt to prevent that race:

        if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0)) 

                goto out;
        if ((arg == F_WRLCK)
            && ((atomic_read(&dentry->d_count) > 1)
                || (atomic_read(&inode->i_count) > 1)))
                goto out;

But, for example, in the case of a simultaneous write open and RDLCK lease request, I believe the call to setlease could come after the may_open() but before the call to get_write_access() that bumps i_writecount.

--b.
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel"

the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Hi Bruce,
The file system does need to keep the local state up to date, like it does with posix locks, so it might need to call __setlease(). The why we had it before was that the call to the file system was done from outside of setlease() and the file system was able to call setlease() which is exported. Now that the call to the fs moved into setlease() the file system can not call it anymore so one possible solution would be to export __setlease().
Marc.

Quoted 2 lines · Marc Eshel

Hi Bruce,
The file system does need to keep the local state up to date, like it

does

Quoted 1 line

with posix locks, so it might need to call __setlease(). The why we had

it

Quoted 4 lines

before was that the call to the file system was done from outside of setlease() and the file system was able to call setlease() which is exported. Now that the call to the fs moved into setlease() the file system can not call it anymore so one possible solution would be to

export

Quoted 2 lines

__setlease().
Marc.

Please just make a vfs_setlease() which has the ability to call down into the filesystem and leave the exported setlease() as a generic method that can continue to be called by the filesystems themselves (and acts as a fallback for vfs_setlease()). That would be closer to the VFS naming conventions.

Trond

Quoted 3 lines · Marc Eshel

Quoted 2 lines

Hi Bruce,
The file system does need to keep the local state up to date, like it

does

Quoted 1 line

Quoted 1 line

with posix locks, so it might need to call __setlease(). The why we had

it

Quoted 4 lines

Quoted 4 lines

before was that the call to the file system was done from outside of setlease() and the file system was able to call setlease() which is exported. Now that the call to the fs moved into setlease() the file system can not call it anymore so one possible solution would be to

export

Quoted 12 lines

Quoted 2 lines

__setlease().
Marc.

Please just make a vfs_setlease() which has the ability to call down into the filesystem and leave the exported setlease() as a generic method that can continue to be called by the filesystems themselves (and acts as a fallback for vfs_setlease()). That would be closer to the VFS naming conventions.

Trond

My interpretation of the preceeding is that there is agreement that the functionality currently implemented in __setlease() should be exported, even though the exported name may not be __setlease(). Is this correct?

If so, that is just fine with me.

The question that I have now is when do you think it likely that these changes get into the released code? I hope that the plan is to get it there fairly soon.

OK, good. I'll revise and post a new series. (Do people prefer another mailbomb or a git url?)

OK, good. I'll revise and post a new series. (Do people prefer another mailbomb or a git url?)

The problem is that some file system such as NFSv2 and NFSv3 do not have sufficient support to be able to support leases correctly. In particular for these two file systems, there is no over the wire protocol support.

Currently, these two file systems fail the fcntl(F_SETLEASE) call accidentally, due to a reference counting difference. These file systems should fail more consciously, with a proper error to indicate that the call is invalid for them.

The problem is that some file system such as NFSv2 and NFSv3 do not have sufficient support to be able to support leases correctly. In particular for these two file systems, there is no over the wire protocol support.

Currently, these two file systems fail the fcntl(F_SETLEASE) call accidentally, due to a reference counting difference. These file systems should fail more consciously, with a proper error to indicate that the call is invalid for them.

From: Marc Eshel <[email protected]>

Since gfs2 can't prevent conflicting opens or leases on other nodes, we probably shouldn't allow it to give out leases at all.

Put the newly defined lease operation into use in gfs2 by turning off lease, unless we're using the "nolock' locking module (in which case all locking is local anyway).

Signed-off-by Marc Eshel <[email protected]>

Hi,

Quoted 3 lines · J. Bruce Fields

From: Marc Eshel <[email protected]>

Since gfs2 can't prevent conflicting opens or leases on other nodes,

Quoted 4 lines

probably shouldn't allow it to give out leases at all.

Put the newly defined lease operation into use in gfs2 by turning off lease, unless we're using the "nolock' locking module (in which case

Quoted 3 lines

locking is local anyway).

Signed-off-by Marc Eshel <[email protected]>

In general it looks fine, but EAGAIN seems like an odd error to return when we really mean that this function isn't supported in cluster mode. Is that really the correct value for that?

Steve.

This is the return code that setlease() currently returns when the lease can not be obtained. Although ENOTSUPP would be more accurately describing the error it will be a new return code from setlease() that is currently not expected by callers to setlease(), but either return code should work. Marc.

Hi,

Quoted 6 lines · Marc Eshel

This is the return code that setlease() currently returns when the lease can not be obtained. Although ENOTSUPP would be more accurately describing the error it will be a new return code from setlease() that is currently not expected by callers to setlease(), but either return code should work. Marc.

I prefer EOPNOTSUPP as it more accurately describes whats going on here. EAGAIN seems to me to imply that its something thats blocking temporarily and that trying again at a later time might result in success,

From: J. Bruce Fields <[email protected]>

We've been using the convention that vfs_foo is the function that calls a filesystem-specific foo method if it exists, or falls back on a generic method if it doesn't.

So rename setlease to vfs_setlease, and __setlease to setlease. Keep setlease exported to allow filesystems to use the generic method in addition to doing their own bookkeeping.

Signed-off-by "J. Bruce Fields" <[email protected]>

---
 fs/locks.c          |   16 ++++++++--------
 fs/nfsd/nfs4state.c |   10 +++++-----
 include/linux/fs.h  |    2 +-
 3 files changed, 14 insertions(+), 14 deletions(-)

fs/locks.c +1 -1

index 8fa4420..82ac90b 100644
@@ -1329,7 +1329,7 @@ int fcntl_getlease(struct file *filp)
 }
 
 /**
- *   __setlease   -   sets a lease on an open file
+ *   setlease   -   sets a lease on an open file
  *   @filp: file pointer
  *   @arg: type of lease to obtain
  *   @flp: input - file_lock to use, output - file_lock inserted

fs/locks.c +5 -5

 {
    struct file_lock *fl, **before, **my_before = NULL, *lease;
    struct dentry *dentry = filp->f_path.dentry;

@@ -1427,8 +1427,10 @@ out:
    return error;
 }
 
+EXPORT_SYMBOL(setlease);
+
  /**
- *   setlease        -       sets a lease on an open file
+ *   vfs_setlease        -       sets a lease on an open file
  *   @filp: file pointer
  *   @arg: type of lease to obtain
  *   @lease: file_lock to use

@@ -1437,7 +1439,7 @@ out:
  *   The fl_lmops fl_break function is required by break_lease
  */
 
-int setlease(struct file *filp, long arg, struct file_lock **lease)
+int vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
 {
    int error;
 

@@ -1445,14 +1447,12 @@ int setlease(struct file *filp, long arg, 
 struct file_lock **lease)
    if (filp->f_op && filp->f_op->setlease)
       error = filp->f_op->setlease(filp, arg, lease);
         else
-      error = __setlease(filp, arg, lease);
+      error = setlease(filp, arg, lease);
    unlock_kernel();
 
    return error;
 }
 
-EXPORT_SYMBOL(setlease);
-
 /**
  *   fcntl_setlease   -   sets a lease on an open file

* @fd: open file descriptor

fs/locks.c +1 -1

@@ -1477,7 +1477,7 @@ int fcntl_setlease(unsigned int fd, struct 
 file *filp, long arg)
 
    lock_kernel();
 
-   error = setlease(filp, arg, &flp);
+   error = vfs_setlease(filp, arg, &flp);
    if (error || arg == F_UNLCK)
       goto out_unlock;

fs/nfsd/nfs4state.c +1 -1

index 3cc8ce4..ed01a77 100644
@@ -251,7 +251,7 @@ nfs4_close_delegation(struct nfs4_delegation *dp)
    /* The following nfsd_close may not actually close the file,
     * but we want to remove the lease in any case. */
    if (dp->dl_flock)
-      setlease(filp, F_UNLCK, &dp->dl_flock);
+      vfs_setlease(filp, F_UNLCK, &dp->dl_flock);
    nfsd_close(filp);
 }
 

fs/nfsd/nfs4state.c +1 -1

@@ -1410,7 +1410,7 @@ void nfsd_copy_lock_deleg_cb(struct file_lock 
 *new, struct file_lock *fl)
 }
 
 /*
- * Called from __setlease() with lock_kernel() held
+ * Called from setlease() with lock_kernel() held
  */
 static

int nfsd_same_client_deleg_cb(struct file_lock *onlist, struct
file_lock *try)

fs/nfsd/nfs4state.c +0 -0

@@ -1710,10 +1710,10 @@ nfs4_open_delegation(struct svc_fh *fh, 

struct nfsd4_open *open, struct nfs4_sta

    fl.fl_file = stp->st_vfs_file;
    fl.fl_pid = current->tgid;

fs/nfsd/nfs4state.c +2 -2

-   /* setlease checks to see if delegation should be handed out.
+   /* vfs_setlease checks to see if delegation should be handed out.
     * the lock_manager callbacks fl_mylease and fl_change are used
     */
-   if ((status = setlease(stp->st_vfs_file,
+   if ((status = vfs_setlease(stp->st_vfs_file,
       flag == NFS4_OPEN_DELEGATE_READ? F_RDLCK: F_WRLCK, &flp))) {
       dprintk("NFSD: setlease failed [%d], no delegation\n", status);
       unhash_delegation(dp);

include/linux/fs.h +0 -0

index 6b07c61..cc62bfb 100644

extern int lock_may_write(struct inode *, loff_t start, unsigned long

Signature · 1 line

1.5.2

We need to export vfs_lease so nfsd can call it.

[email protected] wrote on 06/02/2007 10:39:10 AM:

From: J. Bruce Fields <[email protected]>

Currently leases are only kept locally, so there's no way for a distributed filesystem to enforce them against multiple clients. We're particularly interested in the case of nfsd exporting a cluster filesystem, in which case nfsd needs cluster-coherent leases in order to implement delegations correctly.

Quoted 7 lines · J. Bruce Fields

From: J. Bruce Fields <[email protected]>

Currently leases are only kept locally, so there's no way for a distributed filesystem to enforce them against multiple clients. We're particularly interested in the case of nfsd exporting a cluster filesystem, in which case nfsd needs cluster-coherent leases in order to implement delegations correctly.

A couple of naive questions --

Do the semantics of these leases really match the requirements from NFSv4?

Does the internal NFSv4 server state also need to be shared?

The only problem I'm aware of is that leases aren't broken on rename, link, and unlink. This is kind of tricky to fix. David Richter (cc'd) and I sketched out a few different approaches, and I think he has some patches implementing at least one of them.

This may require defining some new type of lease, to avoid changing the documented behavior of the fcntl lease operations (which only break on open). Although actually I believe Samba needs the same behavior we do, and they're probably the most important user of leases....

Samba internally prohibits renaming or deleting an open file, to match Windows semantics. So it won't notice the difference. At least, that's what I remember from a discussion with Tridge when we were implementing leases back in 2000.

I think it's an acceptable change in Linux semantics to break leases on rename/delete/link. Though I'm not quite sure why you need to -- nobody else is touching the contents of the file, so it's not like you need to write the data back to it, or discard your cached copy of it in the case of a read-only lease.

From: J. Bruce Fields <[email protected]>

Share more code between setlease (used by nfsd) and fcntl.

Also some minor cleanup.

Signed-off-by "J. Bruce Fields" <[email protected]>

---
 fs/locks.c |   14 +++-----------
 1 files changed, 3 insertions(+), 11 deletions(-)

fs/locks.c +1 -9

index 431a8b8..3f366e1 100644
@@ -1469,14 +1469,6 @@ int fcntl_setlease(unsigned int fd, struct file *filp, long arg)
 	struct inode *inode = dentry->d_inode;
 	int error;
 
-	if ((current->fsuid != inode->i_uid) && !capable(CAP_LEASE))
-		return -EACCES;
-	if (!S_ISREG(inode->i_mode))
-		return -EINVAL;
-	error = security_file_lock(filp, arg);
-	if (error)
-		return error;
-
 	locks_init_lock(&fl);
 	error = lease_init(filp, arg, &fl);
 	if (error)

@@ -1484,15 +1476,15 @@ int fcntl_setlease(unsigned int fd, struct file *filp, long arg)
 
 	lock_kernel();
 
-	error = __setlease(filp, arg, &flp);
+	error = setlease(filp, arg, &flp);
 	if (error || arg == F_UNLCK)
 		goto out_unlock;
 

Why not move the security checks from setlease() into __setlease()? That way you can continue to avoid the calls to (re)take the BKL, which are redundant as far as fcntl_setlease() is concerned.

From 4ce02551ab16d2812299ac0ced43652f1af0deb1 Mon Sep 17 00:00:00 2001

Quoted 3 lines · Trond Myklebust

Why not move the security checks from setlease() into __setlease()? That way you can continue to avoid the calls to (re)take the BKL, which are redundant as far as fcntl_setlease() is concerned.

Sure. Then setlease() just becomes a simple BKL-taking wrapper around __setlease(); is that what you were thinking of?

--b.

Quoted 2 lines

From 4ce02551ab16d2812299ac0ced43652f1af0deb1 Mon Sep 17 00:00:00 2001

From: J. Bruce Fields <[email protected]>
Date: Thu, 31 May 2007 17:03:46 -0400
Subject: [PATCH] locks: share more common lease code

Share more code between setlease (used by nfsd) and fcntl.

Also some minor cleanup.

Signed-off-by "J. Bruce Fields" <[email protected]>

---
 fs/locks.c |   30 ++++++++++--------------------
 1 files changed, 10 insertions(+), 20 deletions(-)

fs/locks.c +10 -20

index 431a8b8..6ad3c7b 100644
@@ -1346,6 +1346,14 @@ static int __setlease(struct file *filp, long arg, struct file_lock **flp)
 	struct inode *inode = dentry->d_inode;
 	int error, rdlease_count = 0, wrlease_count = 0;
 
+	if ((current->fsuid != inode->i_uid) && !capable(CAP_LEASE))
+		return -EACCES;
+	if (!S_ISREG(inode->i_mode))
+		return -EINVAL;
+	error = security_file_lock(filp, arg);
+	if (error)
+		return error;
+
 	time_out_leases(inode);
 
 	error = -EINVAL;

@@ -1431,18 +1439,8 @@ out:
 
 int setlease(struct file *filp, long arg, struct file_lock **lease)
 {
-	struct dentry *dentry = filp->f_path.dentry;
-	struct inode *inode = dentry->d_inode;
 	int error;
 
-	if ((current->fsuid != inode->i_uid) && !capable(CAP_LEASE))
-		return -EACCES;
-	if (!S_ISREG(inode->i_mode))
-		return -EINVAL;
-	error = security_file_lock(filp, arg);
-	if (error)
-		return error;
-
 	lock_kernel();
 	error = __setlease(filp, arg, lease);
 	unlock_kernel();

@@ -1469,14 +1467,6 @@ int fcntl_setlease(unsigned int fd, struct file *filp, long arg)
 	struct inode *inode = dentry->d_inode;
 	int error;
 
-	if ((current->fsuid != inode->i_uid) && !capable(CAP_LEASE))
-		return -EACCES;
-	if (!S_ISREG(inode->i_mode))
-		return -EINVAL;
-	error = security_file_lock(filp, arg);
-	if (error)
-		return error;
-
 	locks_init_lock(&fl);
 	error = lease_init(filp, arg, &fl);
 	if (error)

@@ -1490,9 +1480,9 @@ int fcntl_setlease(unsigned int fd, struct file *filp, long arg)
 
 	error = fasync_helper(fd, filp, 1, &flp->fl_fasync);
 	if (error < 0) {
-		/* remove lease just inserted by __setlease */
+		/* remove lease just inserted by setlease */
 		flp->fl_type = F_UNLCK | F_INPROGRESS;
-		flp->fl_break_time = jiffies- 10;
+		flp->fl_break_time = jiffies - 10;
 		time_out_leases(inode);
 		goto out_unlock;
 	}