[PATCH] locks: breaking read lease should not block read open

[PATCH] locks: breaking read lease should not block read open

[J. Bruce Fields]

The lease code behavior during the lease-breaking process is strange.
Fixing it completely would be complicated by the fact that the current
code allows a lease break to downgrade the lease instead of necessarily
removing it.

But I can't see what the point of that feature is.  And googling around
and looking at the Samba code, I can't see any evidence that anyone uses
it.  Think we could just do away with removing the ability to downgrade
to satisfy a lease break?

--b.

commit 70fc3ee9d3e9d61203d4310db4a8128886747272
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Thu Jun 9 09:31:30 2011 -0400

    locks: breaking read lease should not block read open
    
    Currently read opens block while a lease break is in progress, even if
    the lease being broken was only a read lease.
    
    This is an annoyance for v4, since clients may need to do read opens
    before they can return a delegation.
    
    This happens because we use fl_type on a breaking lease to track the
    type it will have when the break is finished (F_RDLCK or F_UNLCK) as
    opposed to the type it had before the break started, so we no longer
    even know at that point whether it was a write lease or a read lease.
    
    The only reason we do that is to allow a write lease broken by a read
    open to be downgraded to a read lease instead of removed completely.
    
    However, that doesn't seem like a useful feature--as far as I can tell,
    nobody uses it or likely ever will.
    
    So, just keep the original lease type in fl_type.
    
    Reported-by: Casey Bodley <cbodley@citi.umich.edu>
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/locks.c b/fs/locks.c
index 0a4f50d..171391f 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1158,9 +1158,9 @@ static void time_out_leases(struct inode *inode)
 			before = &fl->fl_next;
 			continue;
 		}
-		lease_modify(before, fl->fl_type & ~F_INPROGRESS);
-		if (fl == *before)	/* lease_modify may have freed fl */
-			before = &fl->fl_next;
+		lease_modify(before, F_UNLCK);
+		/* lease_modify has freed fl */
+		before = &fl->fl_next;
 	}
 }
 
@@ -1176,7 +1176,7 @@ static void time_out_leases(struct inode *inode)
  */
 int __break_lease(struct inode *inode, unsigned int mode)
 {
-	int error = 0, future;
+	int error = 0;
 	struct file_lock *new_fl, *flock;
 	struct file_lock *fl;
 	unsigned long break_time;
@@ -1197,19 +1197,8 @@ int __break_lease(struct inode *inode, unsigned int mode)
 		if (fl->fl_owner == current->files)
 			i_have_this_lease = 1;
 
-	if (want_write) {
-		/* If we want write access, we have to revoke any lease. */
-		future = F_UNLCK | F_INPROGRESS;
-	} else if (flock->fl_type & F_INPROGRESS) {
-		/* If the lease is already being broken, we just leave it */
-		future = flock->fl_type;
-	} else if (flock->fl_type & F_WRLCK) {
-		/* Downgrade the exclusive lease to a read-only lease. */
-		future = F_RDLCK | F_INPROGRESS;
-	} else {
-		/* the existing lease was read-only, so we can read too. */
-		goto out;
-	}
+	if (!want_write && !(flock->fl_type & F_WRLCK))
+		goto out; /* no conflict */
 
 	if (IS_ERR(new_fl) && !i_have_this_lease
 			&& ((mode & O_NONBLOCK) == 0)) {
@@ -1225,8 +1214,8 @@ int __break_lease(struct inode *inode, unsigned int mode)
 	}
 
 	for (fl = flock; fl && IS_LEASE(fl); fl = fl->fl_next) {
-		if (fl->fl_type != future) {
-			fl->fl_type = future;
+		if (!(fl->fl_type & F_INPROGRESS)) {
+			fl->fl_type |= F_INPROGRESS;
 			fl->fl_break_time = break_time;
 			/* lease must have lmops break callback */
 			fl->fl_lmops->fl_break(fl);
@@ -1254,10 +1243,10 @@ restart:
 	if (error >= 0) {
 		if (error == 0)
 			time_out_leases(inode);
-		/* Wait for the next lease that has not been broken yet */
+		/* Wait for the next lease that conflicts */
 		for (flock = inode->i_flock; flock && IS_LEASE(flock);
 				flock = flock->fl_next) {
-			if (flock->fl_type & F_INPROGRESS)
+			if (want_write || flock->fl_type & F_WRLCK)
 				goto restart;
 		}
 		error = 0;
@@ -1390,11 +1379,10 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
 			before = &fl->fl_next) {
 		if (fl->fl_file == filp)
 			my_before = before;
-		else if (fl->fl_type == (F_INPROGRESS | F_UNLCK))
+		else if (fl->fl_type & F_INPROGRESS)
 			/*
-			 * Someone is in the process of opening this
-			 * file for writing so we may not take an
-			 * exclusive lease on it.
+			 * Don't allow new leases while any lease is
+			 * being broken:
 			 */
 			wrlease_count++;
 		else

Re: [PATCH] locks: breaking read lease should not block read open

[Volker Lendecke]

On Thu, Jun 09, 2011 at 07:16:06PM -0400, J. Bruce Fields wrote:
> The lease code behavior during the lease-breaking process is strange.
> Fixing it completely would be complicated by the fact that the current
> code allows a lease break to downgrade the lease instead of necessarily
> removing it.
> 
> But I can't see what the point of that feature is.  And googling around
> and looking at the Samba code, I can't see any evidence that anyone uses
> it.  Think we could just do away with removing the ability to downgrade
> to satisfy a lease break?

Without having looked too deeply, just let me point out that
Samba here has a plain flaw. Early Linux Kernel versions
that we programmed against did not properly support read
only leases, so we did not implement that initially. If I
remember correctly we never got around to finally do it once
it became available. Eventually we will probably, as read
only leases are a pretty important feature to present to
CIFS clients.

Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen

Re: [PATCH] locks: breaking read lease should not block read open

[J. Bruce Fields]

On Fri, Jun 10, 2011 at 09:56:49AM +0200, Volker Lendecke wrote:
> On Thu, Jun 09, 2011 at 07:16:06PM -0400, J. Bruce Fields wrote:
> > The lease code behavior during the lease-breaking process is strange.
> > Fixing it completely would be complicated by the fact that the current
> > code allows a lease break to downgrade the lease instead of necessarily
> > removing it.
> > 
> > But I can't see what the point of that feature is.  And googling around
> > and looking at the Samba code, I can't see any evidence that anyone uses
> > it.  Think we could just do away with removing the ability to downgrade
> > to satisfy a lease break?
> 
> Without having looked too deeply, just let me point out that
> Samba here has a plain flaw. Early Linux Kernel versions
> that we programmed against did not properly support read
> only leases, so we did not implement that initially. If I
> remember correctly we never got around to finally do it once
> it became available. Eventually we will probably, as read
> only leases are a pretty important feature to present to
> CIFS clients.

Thanks, I didn't know that.  (Or I did, and I forgot.)

When you *do* implement that, is there any chance you'd have this need
to be able to downgrade to a read lease in the case of a conflict?

--b.

Re: [PATCH] locks: breaking read lease should not block read open

[J. Bruce Fields]

On Fri, Jun 10, 2011 at 09:48:59AM -0400, J. Bruce Fields wrote:
> On Fri, Jun 10, 2011 at 09:56:49AM +0200, Volker Lendecke wrote:
> > Without having looked too deeply, just let me point out that
> > Samba here has a plain flaw. Early Linux Kernel versions
> > that we programmed against did not properly support read
> > only leases, so we did not implement that initially. If I
> > remember correctly we never got around to finally do it once
> > it became available. Eventually we will probably, as read
> > only leases are a pretty important feature to present to
> > CIFS clients.
> 
> Thanks, I didn't know that.  (Or I did, and I forgot.)
> 
> When you *do* implement that, is there any chance you'd have this need
> to be able to downgrade to a read lease in the case of a conflict?

So it's a question about the protocols samba implements:

	- Do they allow an atomic downgrade from an exclusive to a
	  shared oplock?  (Or to a level 2 oplock, or whatever the right
	  term is).
	- If so, can that happen as a response to a conflicting open?
	  (So, if you're holding an exclusive oplock, and a conflicting
	  open comes in, can the server-to-client break message say "now
	  you're getting a shared oplock instead"?  Or is the client
	  left without any oplock until it requests a new one?)

I'm not sure how to approach the lease code.

On the one hand, I've never seen any evidence that anyone outside Samba
and NFSv4 has ever used it, and both currently make extremely limited
use of it.  So we could probably get away with "fixing" the lease code
to do whatever both of us need.

On the other hand, I don't know how to figure out what exactly Samba
will actually need.  And the conservative thing to do would be to leave
leases alone.

So maybe I'm better off with a new "NFSv4 delegation lock type" that
does exactly what I need it to, and that's only available from inside
the kernel for now.

Then later if it proves useful to Samba as well, we could figure out how
to export an interface for it to userspace.

I'm not sure.

--b.

Re: [PATCH] locks: breaking read lease should not block read open

[Jeremy Allison]

On Wed, Jul 20, 2011 at 08:07:58PM -0400, J. Bruce Fields wrote:
> On Fri, Jun 10, 2011 at 09:48:59AM -0400, J. Bruce Fields wrote:
> > On Fri, Jun 10, 2011 at 09:56:49AM +0200, Volker Lendecke wrote:
> > > Without having looked too deeply, just let me point out that
> > > Samba here has a plain flaw. Early Linux Kernel versions
> > > that we programmed against did not properly support read
> > > only leases, so we did not implement that initially. If I
> > > remember correctly we never got around to finally do it once
> > > it became available. Eventually we will probably, as read
> > > only leases are a pretty important feature to present to
> > > CIFS clients.
> > 
> > Thanks, I didn't know that.  (Or I did, and I forgot.)
> > 
> > When you *do* implement that, is there any chance you'd have this need
> > to be able to downgrade to a read lease in the case of a conflict?
> 
> So it's a question about the protocols samba implements:
> 
> 	- Do they allow an atomic downgrade from an exclusive to a
> 	  shared oplock?  (Or to a level 2 oplock, or whatever the right
> 	  term is).

Yes. Exclusive can go to level 2 - in fact that's the default
downgrade we do (unless an smb.conf option explicity denies it).

> 	- If so, can that happen as a response to a conflicting open?
> 	  (So, if you're holding an exclusive oplock, and a conflicting
> 	  open comes in, can the server-to-client break message say "now
> 	  you're getting a shared oplock instead"?  Or is the client
> 	  left without any oplock until it requests a new one?)

Yes, this can happen.

In SMB, we only break to no lease when a write request comes
in on a exclusive or level2 oplock (read-lease) handle.

Jeremy.

Re: [PATCH] locks: breaking read lease should not block read open

[J. Bruce Fields]

On Wed, Jul 20, 2011 at 05:15:42PM -0700, Jeremy Allison wrote:
> On Wed, Jul 20, 2011 at 08:07:58PM -0400, J. Bruce Fields wrote:
> > On Fri, Jun 10, 2011 at 09:48:59AM -0400, J. Bruce Fields wrote:
> > > On Fri, Jun 10, 2011 at 09:56:49AM +0200, Volker Lendecke wrote:
> > > > Without having looked too deeply, just let me point out that
> > > > Samba here has a plain flaw. Early Linux Kernel versions
> > > > that we programmed against did not properly support read
> > > > only leases, so we did not implement that initially. If I
> > > > remember correctly we never got around to finally do it once
> > > > it became available. Eventually we will probably, as read
> > > > only leases are a pretty important feature to present to
> > > > CIFS clients.
> > > 
> > > Thanks, I didn't know that.  (Or I did, and I forgot.)
> > > 
> > > When you *do* implement that, is there any chance you'd have this need
> > > to be able to downgrade to a read lease in the case of a conflict?
> > 
> > So it's a question about the protocols samba implements:
> > 
> > 	- Do they allow an atomic downgrade from an exclusive to a
> > 	  shared oplock?  (Or to a level 2 oplock, or whatever the right
> > 	  term is).
> 
> Yes. Exclusive can go to level 2 - in fact that's the default
> downgrade we do (unless an smb.conf option explicity denies it).
> 
> > 	- If so, can that happen as a response to a conflicting open?
> > 	  (So, if you're holding an exclusive oplock, and a conflicting
> > 	  open comes in, can the server-to-client break message say "now
> > 	  you're getting a shared oplock instead"?  Or is the client
> > 	  left without any oplock until it requests a new one?)
> 
> Yes, this can happen.
> 
> In SMB, we only break to no lease when a write request comes
> in on a exclusive or level2 oplock (read-lease) handle.

Ok, thanks, that means we need a more complicated fix here--I'll work on
that....

--b.

Re: [PATCH] locks: breaking read lease should not block read open

[J. Bruce Fields]

On Thu, Jul 21, 2011 at 12:35:20PM -0400, J. Bruce Fields wrote:
> On Wed, Jul 20, 2011 at 05:15:42PM -0700, Jeremy Allison wrote:
> > On Wed, Jul 20, 2011 at 08:07:58PM -0400, J. Bruce Fields wrote:
> > > So it's a question about the protocols samba implements:
> > > 
> > > 	- Do they allow an atomic downgrade from an exclusive to a
> > > 	  shared oplock?  (Or to a level 2 oplock, or whatever the right
> > > 	  term is).
> > 
> > Yes. Exclusive can go to level 2 - in fact that's the default
> > downgrade we do (unless an smb.conf option explicity denies it).
> > 
> > > 	- If so, can that happen as a response to a conflicting open?
> > > 	  (So, if you're holding an exclusive oplock, and a conflicting
> > > 	  open comes in, can the server-to-client break message say "now
> > > 	  you're getting a shared oplock instead"?  Or is the client
> > > 	  left without any oplock until it requests a new one?)
> > 
> > Yes, this can happen.
> > 
> > In SMB, we only break to no lease when a write request comes
> > in on a exclusive or level2 oplock (read-lease) handle.
> 
> Ok, thanks, that means we need a more complicated fix here--I'll work on
> that....

My attempt follows.  Lightly tested.

I'll probably try writing a test or two for it, then queueing up
something like this for 3.2, absent objections.

--b.

[PATCH 1/3] locks: minor lease cleanup

[J. Bruce Fields]

From: J. Bruce Fields <bfields@redhat.com>

Use a helper function, to simplify upcoming changes.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/locks.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 703f545..c528522 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -133,6 +133,11 @@
 #define IS_FLOCK(fl)	(fl->fl_flags & FL_FLOCK)
 #define IS_LEASE(fl)	(fl->fl_flags & FL_LEASE)
 
+static bool lease_breaking(struct file_lock *fl)
+{
+	return fl->fl_type & F_INPROGRESS;
+}
+
 int leases_enable = 1;
 int lease_break_time = 45;
 
@@ -1141,7 +1146,7 @@ static void time_out_leases(struct inode *inode)
 	struct file_lock *fl;
 
 	before = &inode->i_flock;
-	while ((fl = *before) && IS_LEASE(fl) && (fl->fl_type & F_INPROGRESS)) {
+	while ((fl = *before) && IS_LEASE(fl) && lease_breaking(fl)) {
 		if ((fl->fl_break_time == 0)
 				|| time_before(jiffies, fl->fl_break_time)) {
 			before = &fl->fl_next;
@@ -1189,7 +1194,7 @@ int __break_lease(struct inode *inode, unsigned int mode)
 	if (want_write) {
 		/* If we want write access, we have to revoke any lease. */
 		future = F_UNLCK | F_INPROGRESS;
-	} else if (flock->fl_type & F_INPROGRESS) {
+	} else if (lease_breaking(flock)) {
 		/* If the lease is already being broken, we just leave it */
 		future = flock->fl_type;
 	} else if (flock->fl_type & F_WRLCK) {
@@ -1246,7 +1251,7 @@ restart:
 		/* Wait for the next lease that has not been broken yet */
 		for (flock = inode->i_flock; flock && IS_LEASE(flock);
 				flock = flock->fl_next) {
-			if (flock->fl_type & F_INPROGRESS)
+			if (lease_breaking(flock))
 				goto restart;
 		}
 		error = 0;
@@ -2126,7 +2131,7 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
 		}
 	} else if (IS_LEASE(fl)) {
 		seq_printf(f, "LEASE  ");
-		if (fl->fl_type & F_INPROGRESS)
+		if (lease_breaking(fl))
 			seq_printf(f, "BREAKING  ");
 		else if (fl->fl_file)
 			seq_printf(f, "ACTIVE    ");
@@ -2142,7 +2147,7 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
 			       : (fl->fl_type & LOCK_WRITE) ? "WRITE" : "NONE ");
 	} else {
 		seq_printf(f, "%s ",
-			       (fl->fl_type & F_INPROGRESS)
+			       (lease_breaking(fl))
 			       ? (fl->fl_type & F_UNLCK) ? "UNLCK" : "READ "
 			       : (fl->fl_type & F_WRLCK) ? "WRITE" : "READ ");
 	}
-- 
1.7.4.1

[PATCH 2/3] locks: move F_INPROGRESS from fl_type to fl_flags field

[J. Bruce Fields]

From: J. Bruce Fields <bfields@redhat.com>

F_INPROGRESS isn't exposed to userspace.  To me it makes more sense in
fl_flags....

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 arch/alpha/include/asm/fcntl.h |    2 --
 fs/locks.c                     |   14 ++++++++------
 include/asm-generic/fcntl.h    |    5 -----
 include/linux/fs.h             |    3 ++-
 4 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/arch/alpha/include/asm/fcntl.h b/arch/alpha/include/asm/fcntl.h
index 1b71ca7..6d9e805 100644
--- a/arch/alpha/include/asm/fcntl.h
+++ b/arch/alpha/include/asm/fcntl.h
@@ -51,8 +51,6 @@
 #define F_EXLCK		16	/* or 3 */
 #define F_SHLCK		32	/* or 4 */
 
-#define F_INPROGRESS	64
-
 #include <asm-generic/fcntl.h>
 
 #endif
diff --git a/fs/locks.c b/fs/locks.c
index c528522..c421541 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -135,7 +135,7 @@
 
 static bool lease_breaking(struct file_lock *fl)
 {
-	return fl->fl_type & F_INPROGRESS;
+	return fl->fl_flags & FL_INPROGRESS;
 }
 
 int leases_enable = 1;
@@ -1132,6 +1132,7 @@ int lease_modify(struct file_lock **before, int arg)
 
 	if (error)
 		return error;
+	fl->fl_flags &= ~FL_INPROGRESS;
 	locks_wake_up_blocks(fl);
 	if (arg == F_UNLCK)
 		locks_delete_lock(before);
@@ -1152,7 +1153,7 @@ static void time_out_leases(struct inode *inode)
 			before = &fl->fl_next;
 			continue;
 		}
-		lease_modify(before, fl->fl_type & ~F_INPROGRESS);
+		lease_modify(before, fl->fl_type);
 		if (fl == *before)	/* lease_modify may have freed fl */
 			before = &fl->fl_next;
 	}
@@ -1193,13 +1194,13 @@ int __break_lease(struct inode *inode, unsigned int mode)
 
 	if (want_write) {
 		/* If we want write access, we have to revoke any lease. */
-		future = F_UNLCK | F_INPROGRESS;
+		future = F_UNLCK;
 	} else if (lease_breaking(flock)) {
 		/* If the lease is already being broken, we just leave it */
 		future = flock->fl_type;
 	} else if (flock->fl_type & F_WRLCK) {
 		/* Downgrade the exclusive lease to a read-only lease. */
-		future = F_RDLCK | F_INPROGRESS;
+		future = F_RDLCK;
 	} else {
 		/* the existing lease was read-only, so we can read too. */
 		goto out;
@@ -1221,6 +1222,7 @@ int __break_lease(struct inode *inode, unsigned int mode)
 	for (fl = flock; fl && IS_LEASE(fl); fl = fl->fl_next) {
 		if (fl->fl_type != future) {
 			fl->fl_type = future;
+			fl->fl_flags |= FL_INPROGRESS;
 			fl->fl_break_time = break_time;
 			/* lease must have lmops break callback */
 			fl->fl_lmops->lm_break(fl);
@@ -1319,7 +1321,7 @@ int fcntl_getlease(struct file *filp)
 	for (fl = filp->f_path.dentry->d_inode->i_flock; fl && IS_LEASE(fl);
 			fl = fl->fl_next) {
 		if (fl->fl_file == filp) {
-			type = fl->fl_type & ~F_INPROGRESS;
+			type = fl->fl_type;
 			break;
 		}
 	}
@@ -1384,7 +1386,7 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
 			before = &fl->fl_next) {
 		if (fl->fl_file == filp)
 			my_before = before;
-		else if (fl->fl_type == (F_INPROGRESS | F_UNLCK))
+		else if ((fl->fl_type == F_UNLCK) && lease_breaking(fl))
 			/*
 			 * Someone is in the process of opening this
 			 * file for writing so we may not take an
diff --git a/include/asm-generic/fcntl.h b/include/asm-generic/fcntl.h
index 84793c7..9e5b035 100644
--- a/include/asm-generic/fcntl.h
+++ b/include/asm-generic/fcntl.h
@@ -145,11 +145,6 @@ struct f_owner_ex {
 #define F_SHLCK		8	/* or 4 */
 #endif
 
-/* for leases */
-#ifndef F_INPROGRESS
-#define F_INPROGRESS	16
-#endif
-
 /* operations for bsd flock(), also used by the kernel implementation */
 #define LOCK_SH		1	/* shared lock */
 #define LOCK_EX		2	/* exclusive lock */
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f23bcb7..f0b692c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1050,6 +1050,7 @@ static inline int file_check_writeable(struct file *filp)
 #define FL_LEASE	32	/* lease held on this file */
 #define FL_CLOSE	64	/* unlock on close */
 #define FL_SLEEP	128	/* A blocking lock */
+#define FL_INPROGRESS	256	/* Lease is being broken */
 
 /*
  * Special return value from posix_lock_file() and vfs_lock_file() for
@@ -1096,7 +1097,7 @@ struct file_lock {
 	struct list_head fl_link;	/* doubly linked list of all locks */
 	struct list_head fl_block;	/* circular list of blocked processes */
 	fl_owner_t fl_owner;
-	unsigned char fl_flags;
+	unsigned int fl_flags;
 	unsigned char fl_type;
 	unsigned int fl_pid;
 	struct pid *fl_nspid;
-- 
1.7.4.1

[PATCH 3/3] locks: fix tracking of inprogress lease breaks

[J. Bruce Fields]

From: J. Bruce Fields <bfields@redhat.com>

We currently use a bit in fl_flags to record whether a lease is being
broken, and set fl_type to the type (RDLCK or UNLCK) that it will
eventually have.  This means that once the lease break starts, we forget
what the lease's type *used* to be.  Breaking a read lease will then
result in blocking read opens, even though there's no conflict--because
the lease type is now F_UNLCK and we can no longer tell whether it was
previously a read or write lease.

So, instead keep fl_type as the original type (the type which we
enforce), and keep track of whether we're unlocking or merely
downgrading by replacing the single FL_INPROGRESS flag by
FL_UNLOCK_PENDING and FL_DOWNGRADE_PENDING flags.

To get this right we also need to track separate downgrade and break
times, to handle the case where a write-leased file gets conflicting
opens first for read, then later for write.

(I first considered just eliminating the downgrade behavior
completely--nfsv4 doesn't need it, and nobody as far as I can tell
actually uses it currently--but Jeremy Allison tells me that Windows
oplocks do behave this way, so Samba will probably use this some day.)

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/locks.c         |   70 +++++++++++++++++++++++++++++++++++++--------------
 include/linux/fs.h |    7 +++-
 2 files changed, 56 insertions(+), 21 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index c421541..d7089a8 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -135,7 +135,16 @@
 
 static bool lease_breaking(struct file_lock *fl)
 {
-	return fl->fl_flags & FL_INPROGRESS;
+	return fl->fl_flags & (FL_UNLOCK_PENDING | FL_DOWNGRADE_PENDING);
+}
+
+static int target_leasetype(struct file_lock *fl)
+{
+	if (fl->fl_flags & FL_UNLOCK_PENDING)
+		return F_UNLCK;
+	if (fl->fl_flags & FL_DOWNGRADE_PENDING)
+		return F_RDLCK;
+	return fl->fl_type;
 }
 
 int leases_enable = 1;
@@ -1124,6 +1133,17 @@ int locks_mandatory_area(int read_write, struct inode *inode,
 
 EXPORT_SYMBOL(locks_mandatory_area);
 
+int lease_clear_pending(struct file_lock *fl, int arg)
+{
+	switch (arg) {
+	case F_UNLCK:
+		fl->fl_flags &= ~FL_UNLOCK_PENDING;
+		/* fall through: */
+	case F_RDLCK:
+		fl->fl_flags &= ~FL_DOWNGRADE_PENDING;
+	}
+}
+
 /* We already had a lease on this file; just change its type */
 int lease_modify(struct file_lock **before, int arg)
 {
@@ -1132,7 +1152,7 @@ int lease_modify(struct file_lock **before, int arg)
 
 	if (error)
 		return error;
-	fl->fl_flags &= ~FL_INPROGRESS;
+	lease_clear_pending(fl, arg);
 	locks_wake_up_blocks(fl);
 	if (arg == F_UNLCK)
 		locks_delete_lock(before);
@@ -1141,6 +1161,14 @@ int lease_modify(struct file_lock **before, int arg)
 
 EXPORT_SYMBOL(lease_modify);
 
+static bool past_time(unsigned long then)
+{
+	if (!then)
+		/* 0 is a special value meaning "this never expires": */
+		return false;
+	return time_after(jiffies, then);
+}
+
 static void time_out_leases(struct inode *inode)
 {
 	struct file_lock **before;
@@ -1148,12 +1176,10 @@ static void time_out_leases(struct inode *inode)
 
 	before = &inode->i_flock;
 	while ((fl = *before) && IS_LEASE(fl) && lease_breaking(fl)) {
-		if ((fl->fl_break_time == 0)
-				|| time_before(jiffies, fl->fl_break_time)) {
-			before = &fl->fl_next;
-			continue;
-		}
-		lease_modify(before, fl->fl_type);
+		if (past_time(fl->fl_downgrade_time))
+			lease_modify(before, F_RDLCK);
+		if (past_time(fl->fl_break_time))
+			lease_modify(before, F_UNLCK);
 		if (fl == *before)	/* lease_modify may have freed fl */
 			before = &fl->fl_next;
 	}
@@ -1194,13 +1220,13 @@ int __break_lease(struct inode *inode, unsigned int mode)
 
 	if (want_write) {
 		/* If we want write access, we have to revoke any lease. */
-		future = F_UNLCK;
+		future = FL_UNLOCK_PENDING;
 	} else if (lease_breaking(flock)) {
 		/* If the lease is already being broken, we just leave it */
-		future = flock->fl_type;
+		future = 0;
 	} else if (flock->fl_type & F_WRLCK) {
 		/* Downgrade the exclusive lease to a read-only lease. */
-		future = F_RDLCK;
+		future = FL_DOWNGRADE_PENDING;
 	} else {
 		/* the existing lease was read-only, so we can read too. */
 		goto out;
@@ -1220,10 +1246,12 @@ int __break_lease(struct inode *inode, unsigned int mode)
 	}
 
 	for (fl = flock; fl && IS_LEASE(fl); fl = fl->fl_next) {
-		if (fl->fl_type != future) {
-			fl->fl_type = future;
-			fl->fl_flags |= FL_INPROGRESS;
-			fl->fl_break_time = break_time;
+		if (!(fl->fl_flags & future)) {
+			fl->fl_flags |= future;
+			if (future == FL_UNLOCK_PENDING)
+				fl->fl_break_time = break_time;
+			else /* FL_DOWNGRADE_PENDING */
+				fl->fl_downgrade_time = break_time;
 			/* lease must have lmops break callback */
 			fl->fl_lmops->lm_break(fl);
 		}
@@ -1250,10 +1278,14 @@ restart:
 	if (error >= 0) {
 		if (error == 0)
 			time_out_leases(inode);
-		/* Wait for the next lease that has not been broken yet */
+		/*
+		 * Wait for the next conflicting lease that has not been
+		 * broken yet
+		 */
 		for (flock = inode->i_flock; flock && IS_LEASE(flock);
 				flock = flock->fl_next) {
-			if (lease_breaking(flock))
+			if ((want_write && flock->fl_type == F_RDLCK)
+			    || flock->fl_type == F_WRLCK)
 				goto restart;
 		}
 		error = 0;
@@ -1321,7 +1353,7 @@ int fcntl_getlease(struct file *filp)
 	for (fl = filp->f_path.dentry->d_inode->i_flock; fl && IS_LEASE(fl);
 			fl = fl->fl_next) {
 		if (fl->fl_file == filp) {
-			type = fl->fl_type;
+			type = target_leasetype(fl);
 			break;
 		}
 	}
@@ -1386,7 +1418,7 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
 			before = &fl->fl_next) {
 		if (fl->fl_file == filp)
 			my_before = before;
-		else if ((fl->fl_type == F_UNLCK) && lease_breaking(fl))
+		else if (fl->fl_flags & FL_UNLOCK_PENDING)
 			/*
 			 * Someone is in the process of opening this
 			 * file for writing so we may not take an
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f0b692c..225676c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1050,7 +1050,8 @@ static inline int file_check_writeable(struct file *filp)
 #define FL_LEASE	32	/* lease held on this file */
 #define FL_CLOSE	64	/* unlock on close */
 #define FL_SLEEP	128	/* A blocking lock */
-#define FL_INPROGRESS	256	/* Lease is being broken */
+#define FL_UNLOCK_PENDING	256 /* Lease is being broken */
+#define FL_DOWNGRADE_PENDING	512 /* Lease is being downgraded */
 
 /*
  * Special return value from posix_lock_file() and vfs_lock_file() for
@@ -1107,7 +1108,9 @@ struct file_lock {
 	loff_t fl_end;
 
 	struct fasync_struct *	fl_fasync; /* for lease break notifications */
-	unsigned long fl_break_time;	/* for nonblocking lease breaks */
+	/* for lease breaks: */
+	unsigned long fl_break_time;
+	unsigned long fl_downgrade_time;
 
 	const struct file_lock_operations *fl_ops;	/* Callbacks for filesystems */
 	const struct lock_manager_operations *fl_lmops;	/* Callbacks for lockmanagers */
-- 
1.7.4.1

Re: [PATCH] locks: breaking read lease should not block read open

[J. Bruce Fields]

On Thu, Jul 28, 2011 at 10:27:58PM -0400, J. Bruce Fields wrote:
> On Thu, Jul 21, 2011 at 12:35:20PM -0400, J. Bruce Fields wrote:
> > On Wed, Jul 20, 2011 at 05:15:42PM -0700, Jeremy Allison wrote:
> > > On Wed, Jul 20, 2011 at 08:07:58PM -0400, J. Bruce Fields wrote:
> > > > So it's a question about the protocols samba implements:
> > > > 
> > > > 	- Do they allow an atomic downgrade from an exclusive to a
> > > > 	  shared oplock?  (Or to a level 2 oplock, or whatever the right
> > > > 	  term is).
> > > 
> > > Yes. Exclusive can go to level 2 - in fact that's the default
> > > downgrade we do (unless an smb.conf option explicity denies it).
> > > 
> > > > 	- If so, can that happen as a response to a conflicting open?
> > > > 	  (So, if you're holding an exclusive oplock, and a conflicting
> > > > 	  open comes in, can the server-to-client break message say "now
> > > > 	  you're getting a shared oplock instead"?  Or is the client
> > > > 	  left without any oplock until it requests a new one?)
> > > 
> > > Yes, this can happen.
> > > 
> > > In SMB, we only break to no lease when a write request comes
> > > in on a exclusive or level2 oplock (read-lease) handle.
> > 
> > Ok, thanks, that means we need a more complicated fix here--I'll work on
> > that....
> 
> My attempt follows.  Lightly tested.
> 
> I'll probably try writing a test or two for it, then queueing up
> something like this for 3.2, absent objections.

Second take follows, this time after a little more testing
(lease_tests.c from git://linux-nfs.org/~bfields/lock-tests.git), some
bug fixes, and minor simplification of the logic.  This is what I intend
to queue up for 3.2.

--b.

[PATCH 1/4] locks: minor lease cleanup

[J. Bruce Fields]

Use a helper function, to simplify upcoming changes.

Reviewed-by: Jeff Layton <jlayton@redhat.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/locks.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 703f545..c528522 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -133,6 +133,11 @@
 #define IS_FLOCK(fl)	(fl->fl_flags & FL_FLOCK)
 #define IS_LEASE(fl)	(fl->fl_flags & FL_LEASE)
 
+static bool lease_breaking(struct file_lock *fl)
+{
+	return fl->fl_type & F_INPROGRESS;
+}
+
 int leases_enable = 1;
 int lease_break_time = 45;
 
@@ -1141,7 +1146,7 @@ static void time_out_leases(struct inode *inode)
 	struct file_lock *fl;
 
 	before = &inode->i_flock;
-	while ((fl = *before) && IS_LEASE(fl) && (fl->fl_type & F_INPROGRESS)) {
+	while ((fl = *before) && IS_LEASE(fl) && lease_breaking(fl)) {
 		if ((fl->fl_break_time == 0)
 				|| time_before(jiffies, fl->fl_break_time)) {
 			before = &fl->fl_next;
@@ -1189,7 +1194,7 @@ int __break_lease(struct inode *inode, unsigned int mode)
 	if (want_write) {
 		/* If we want write access, we have to revoke any lease. */
 		future = F_UNLCK | F_INPROGRESS;
-	} else if (flock->fl_type & F_INPROGRESS) {
+	} else if (lease_breaking(flock)) {
 		/* If the lease is already being broken, we just leave it */
 		future = flock->fl_type;
 	} else if (flock->fl_type & F_WRLCK) {
@@ -1246,7 +1251,7 @@ restart:
 		/* Wait for the next lease that has not been broken yet */
 		for (flock = inode->i_flock; flock && IS_LEASE(flock);
 				flock = flock->fl_next) {
-			if (flock->fl_type & F_INPROGRESS)
+			if (lease_breaking(flock))
 				goto restart;
 		}
 		error = 0;
@@ -2126,7 +2131,7 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
 		}
 	} else if (IS_LEASE(fl)) {
 		seq_printf(f, "LEASE  ");
-		if (fl->fl_type & F_INPROGRESS)
+		if (lease_breaking(fl))
 			seq_printf(f, "BREAKING  ");
 		else if (fl->fl_file)
 			seq_printf(f, "ACTIVE    ");
@@ -2142,7 +2147,7 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
 			       : (fl->fl_type & LOCK_WRITE) ? "WRITE" : "NONE ");
 	} else {
 		seq_printf(f, "%s ",
-			       (fl->fl_type & F_INPROGRESS)
+			       (lease_breaking(fl))
 			       ? (fl->fl_type & F_UNLCK) ? "UNLCK" : "READ "
 			       : (fl->fl_type & F_WRLCK) ? "WRITE" : "READ ");
 	}
-- 
1.7.4.1

[PATCH 2/4] locks: move F_INPROGRESS from fl_type to fl_flags field

[J. Bruce Fields]

F_INPROGRESS isn't exposed to userspace.  To me it makes more sense in
fl_flags....

Reviewed-by: Jeff Layton <jlayton@redhat.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 arch/alpha/include/asm/fcntl.h |    2 --
 fs/locks.c                     |   14 ++++++++------
 include/asm-generic/fcntl.h    |    5 -----
 include/linux/fs.h             |    3 ++-
 4 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/arch/alpha/include/asm/fcntl.h b/arch/alpha/include/asm/fcntl.h
index 1b71ca7..6d9e805 100644
--- a/arch/alpha/include/asm/fcntl.h
+++ b/arch/alpha/include/asm/fcntl.h
@@ -51,8 +51,6 @@
 #define F_EXLCK		16	/* or 3 */
 #define F_SHLCK		32	/* or 4 */
 
-#define F_INPROGRESS	64
-
 #include <asm-generic/fcntl.h>
 
 #endif
diff --git a/fs/locks.c b/fs/locks.c
index c528522..c421541 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -135,7 +135,7 @@
 
 static bool lease_breaking(struct file_lock *fl)
 {
-	return fl->fl_type & F_INPROGRESS;
+	return fl->fl_flags & FL_INPROGRESS;
 }
 
 int leases_enable = 1;
@@ -1132,6 +1132,7 @@ int lease_modify(struct file_lock **before, int arg)
 
 	if (error)
 		return error;
+	fl->fl_flags &= ~FL_INPROGRESS;
 	locks_wake_up_blocks(fl);
 	if (arg == F_UNLCK)
 		locks_delete_lock(before);
@@ -1152,7 +1153,7 @@ static void time_out_leases(struct inode *inode)
 			before = &fl->fl_next;
 			continue;
 		}
-		lease_modify(before, fl->fl_type & ~F_INPROGRESS);
+		lease_modify(before, fl->fl_type);
 		if (fl == *before)	/* lease_modify may have freed fl */
 			before = &fl->fl_next;
 	}
@@ -1193,13 +1194,13 @@ int __break_lease(struct inode *inode, unsigned int mode)
 
 	if (want_write) {
 		/* If we want write access, we have to revoke any lease. */
-		future = F_UNLCK | F_INPROGRESS;
+		future = F_UNLCK;
 	} else if (lease_breaking(flock)) {
 		/* If the lease is already being broken, we just leave it */
 		future = flock->fl_type;
 	} else if (flock->fl_type & F_WRLCK) {
 		/* Downgrade the exclusive lease to a read-only lease. */
-		future = F_RDLCK | F_INPROGRESS;
+		future = F_RDLCK;
 	} else {
 		/* the existing lease was read-only, so we can read too. */
 		goto out;
@@ -1221,6 +1222,7 @@ int __break_lease(struct inode *inode, unsigned int mode)
 	for (fl = flock; fl && IS_LEASE(fl); fl = fl->fl_next) {
 		if (fl->fl_type != future) {
 			fl->fl_type = future;
+			fl->fl_flags |= FL_INPROGRESS;
 			fl->fl_break_time = break_time;
 			/* lease must have lmops break callback */
 			fl->fl_lmops->lm_break(fl);
@@ -1319,7 +1321,7 @@ int fcntl_getlease(struct file *filp)
 	for (fl = filp->f_path.dentry->d_inode->i_flock; fl && IS_LEASE(fl);
 			fl = fl->fl_next) {
 		if (fl->fl_file == filp) {
-			type = fl->fl_type & ~F_INPROGRESS;
+			type = fl->fl_type;
 			break;
 		}
 	}
@@ -1384,7 +1386,7 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
 			before = &fl->fl_next) {
 		if (fl->fl_file == filp)
 			my_before = before;
-		else if (fl->fl_type == (F_INPROGRESS | F_UNLCK))
+		else if ((fl->fl_type == F_UNLCK) && lease_breaking(fl))
 			/*
 			 * Someone is in the process of opening this
 			 * file for writing so we may not take an
diff --git a/include/asm-generic/fcntl.h b/include/asm-generic/fcntl.h
index 84793c7..9e5b035 100644
--- a/include/asm-generic/fcntl.h
+++ b/include/asm-generic/fcntl.h
@@ -145,11 +145,6 @@ struct f_owner_ex {
 #define F_SHLCK		8	/* or 4 */
 #endif
 
-/* for leases */
-#ifndef F_INPROGRESS
-#define F_INPROGRESS	16
-#endif
-
 /* operations for bsd flock(), also used by the kernel implementation */
 #define LOCK_SH		1	/* shared lock */
 #define LOCK_EX		2	/* exclusive lock */
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 178cdb4..327fdd4 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1065,6 +1065,7 @@ static inline int file_check_writeable(struct file *filp)
 #define FL_LEASE	32	/* lease held on this file */
 #define FL_CLOSE	64	/* unlock on close */
 #define FL_SLEEP	128	/* A blocking lock */
+#define FL_INPROGRESS	256	/* Lease is being broken */
 
 /*
  * Special return value from posix_lock_file() and vfs_lock_file() for
@@ -1111,7 +1112,7 @@ struct file_lock {
 	struct list_head fl_link;	/* doubly linked list of all locks */
 	struct list_head fl_block;	/* circular list of blocked processes */
 	fl_owner_t fl_owner;
-	unsigned char fl_flags;
+	unsigned int fl_flags;
 	unsigned char fl_type;
 	unsigned int fl_pid;
 	struct pid *fl_nspid;
-- 
1.7.4.1

[PATCH 3/4] locks: fix tracking of inprogress lease breaks

[J. Bruce Fields]

We currently use a bit in fl_flags to record whether a lease is being
broken, and set fl_type to the type (RDLCK or UNLCK) that it will
eventually have.  This means that once the lease break starts, we forget
what the lease's type *used* to be.  Breaking a read lease will then
result in blocking read opens, even though there's no conflict--because
the lease type is now F_UNLCK and we can no longer tell whether it was
previously a read or write lease.

So, instead keep fl_type as the original type (the type which we
enforce), and keep track of whether we're unlocking or merely
downgrading by replacing the single FL_INPROGRESS flag by
FL_UNLOCK_PENDING and FL_DOWNGRADE_PENDING flags.

To get this right we also need to track separate downgrade and break
times, to handle the case where a write-leased file gets conflicting
opens first for read, then later for write.

(I first considered just eliminating the downgrade behavior
completely--nfsv4 doesn't need it, and nobody as far as I can tell
actually uses it currently--but Jeremy Allison tells me that Windows
oplocks do behave this way, so Samba will probably use this some day.)

Reviewed-by: Jeff Layton <jlayton@redhat.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/locks.c         |   87 +++++++++++++++++++++++++++++++++-------------------
 include/linux/fs.h |    7 +++-
 2 files changed, 60 insertions(+), 34 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index c421541..c525aa4 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -135,7 +135,16 @@
 
 static bool lease_breaking(struct file_lock *fl)
 {
-	return fl->fl_flags & FL_INPROGRESS;
+	return fl->fl_flags & (FL_UNLOCK_PENDING | FL_DOWNGRADE_PENDING);
+}
+
+static int target_leasetype(struct file_lock *fl)
+{
+	if (fl->fl_flags & FL_UNLOCK_PENDING)
+		return F_UNLCK;
+	if (fl->fl_flags & FL_DOWNGRADE_PENDING)
+		return F_RDLCK;
+	return fl->fl_type;
 }
 
 int leases_enable = 1;
@@ -1124,6 +1133,17 @@ int locks_mandatory_area(int read_write, struct inode *inode,
 
 EXPORT_SYMBOL(locks_mandatory_area);
 
+static void lease_clear_pending(struct file_lock *fl, int arg)
+{
+	switch (arg) {
+	case F_UNLCK:
+		fl->fl_flags &= ~FL_UNLOCK_PENDING;
+		/* fall through: */
+	case F_RDLCK:
+		fl->fl_flags &= ~FL_DOWNGRADE_PENDING;
+	}
+}
+
 /* We already had a lease on this file; just change its type */
 int lease_modify(struct file_lock **before, int arg)
 {
@@ -1132,7 +1152,7 @@ int lease_modify(struct file_lock **before, int arg)
 
 	if (error)
 		return error;
-	fl->fl_flags &= ~FL_INPROGRESS;
+	lease_clear_pending(fl, arg);
 	locks_wake_up_blocks(fl);
 	if (arg == F_UNLCK)
 		locks_delete_lock(before);
@@ -1141,6 +1161,14 @@ int lease_modify(struct file_lock **before, int arg)
 
 EXPORT_SYMBOL(lease_modify);
 
+static bool past_time(unsigned long then)
+{
+	if (!then)
+		/* 0 is a special value meaning "this never expires": */
+		return false;
+	return time_after(jiffies, then);
+}
+
 static void time_out_leases(struct inode *inode)
 {
 	struct file_lock **before;
@@ -1148,12 +1176,10 @@ static void time_out_leases(struct inode *inode)
 
 	before = &inode->i_flock;
 	while ((fl = *before) && IS_LEASE(fl) && lease_breaking(fl)) {
-		if ((fl->fl_break_time == 0)
-				|| time_before(jiffies, fl->fl_break_time)) {
-			before = &fl->fl_next;
-			continue;
-		}
-		lease_modify(before, fl->fl_type);
+		if (past_time(fl->fl_downgrade_time))
+			lease_modify(before, F_RDLCK);
+		if (past_time(fl->fl_break_time))
+			lease_modify(before, F_UNLCK);
 		if (fl == *before)	/* lease_modify may have freed fl */
 			before = &fl->fl_next;
 	}
@@ -1171,7 +1197,7 @@ static void time_out_leases(struct inode *inode)
  */
 int __break_lease(struct inode *inode, unsigned int mode)
 {
-	int error = 0, future;
+	int error = 0;
 	struct file_lock *new_fl, *flock;
 	struct file_lock *fl;
 	unsigned long break_time;
@@ -1188,24 +1214,13 @@ int __break_lease(struct inode *inode, unsigned int mode)
 	if ((flock == NULL) || !IS_LEASE(flock))
 		goto out;
 
+	if (!locks_conflict(flock, new_fl))
+		goto out;
+
 	for (fl = flock; fl && IS_LEASE(fl); fl = fl->fl_next)
 		if (fl->fl_owner == current->files)
 			i_have_this_lease = 1;
 
-	if (want_write) {
-		/* If we want write access, we have to revoke any lease. */
-		future = F_UNLCK;
-	} else if (lease_breaking(flock)) {
-		/* If the lease is already being broken, we just leave it */
-		future = flock->fl_type;
-	} else if (flock->fl_type & F_WRLCK) {
-		/* Downgrade the exclusive lease to a read-only lease. */
-		future = F_RDLCK;
-	} else {
-		/* the existing lease was read-only, so we can read too. */
-		goto out;
-	}
-
 	if (IS_ERR(new_fl) && !i_have_this_lease
 			&& ((mode & O_NONBLOCK) == 0)) {
 		error = PTR_ERR(new_fl);
@@ -1220,13 +1235,18 @@ int __break_lease(struct inode *inode, unsigned int mode)
 	}
 
 	for (fl = flock; fl && IS_LEASE(fl); fl = fl->fl_next) {
-		if (fl->fl_type != future) {
-			fl->fl_type = future;
-			fl->fl_flags |= FL_INPROGRESS;
+		if (want_write) {
+			if (fl->fl_flags & FL_UNLOCK_PENDING)
+				continue;
+			fl->fl_flags |= FL_UNLOCK_PENDING;
 			fl->fl_break_time = break_time;
-			/* lease must have lmops break callback */
-			fl->fl_lmops->lm_break(fl);
+		} else {
+			if (lease_breaking(flock))
+				continue;
+			fl->fl_flags |= FL_DOWNGRADE_PENDING;
+			fl->fl_downgrade_time = break_time;
 		}
+		fl->fl_lmops->lm_break(fl);
 	}
 
 	if (i_have_this_lease || (mode & O_NONBLOCK)) {
@@ -1250,10 +1270,13 @@ restart:
 	if (error >= 0) {
 		if (error == 0)
 			time_out_leases(inode);
-		/* Wait for the next lease that has not been broken yet */
+		/*
+		 * Wait for the next conflicting lease that has not been
+		 * broken yet
+		 */
 		for (flock = inode->i_flock; flock && IS_LEASE(flock);
 				flock = flock->fl_next) {
-			if (lease_breaking(flock))
+			if (locks_conflict(new_fl, flock))
 				goto restart;
 		}
 		error = 0;
@@ -1321,7 +1344,7 @@ int fcntl_getlease(struct file *filp)
 	for (fl = filp->f_path.dentry->d_inode->i_flock; fl && IS_LEASE(fl);
 			fl = fl->fl_next) {
 		if (fl->fl_file == filp) {
-			type = fl->fl_type;
+			type = target_leasetype(fl);
 			break;
 		}
 	}
@@ -1386,7 +1409,7 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
 			before = &fl->fl_next) {
 		if (fl->fl_file == filp)
 			my_before = before;
-		else if ((fl->fl_type == F_UNLCK) && lease_breaking(fl))
+		else if (fl->fl_flags & FL_UNLOCK_PENDING)
 			/*
 			 * Someone is in the process of opening this
 			 * file for writing so we may not take an
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 327fdd4..76460ed 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1065,7 +1065,8 @@ static inline int file_check_writeable(struct file *filp)
 #define FL_LEASE	32	/* lease held on this file */
 #define FL_CLOSE	64	/* unlock on close */
 #define FL_SLEEP	128	/* A blocking lock */
-#define FL_INPROGRESS	256	/* Lease is being broken */
+#define FL_DOWNGRADE_PENDING	256 /* Lease is being downgraded */
+#define FL_UNLOCK_PENDING	512 /* Lease is being broken */
 
 /*
  * Special return value from posix_lock_file() and vfs_lock_file() for
@@ -1122,7 +1123,9 @@ struct file_lock {
 	loff_t fl_end;
 
 	struct fasync_struct *	fl_fasync; /* for lease break notifications */
-	unsigned long fl_break_time;	/* for nonblocking lease breaks */
+	/* for lease breaks: */
+	unsigned long fl_break_time;
+	unsigned long fl_downgrade_time;
 
 	const struct file_lock_operations *fl_ops;	/* Callbacks for filesystems */
 	const struct lock_manager_operations *fl_lmops;	/* Callbacks for lockmanagers */
-- 
1.7.4.1

[PATCH 4/4] locks: setlease cleanup

[J. Bruce Fields]

There's an incorrect comment here.  Also clean up the logic: the
"rdlease" and "wrlease" locals are confusingly named, and don't really
add anything since we can make a decision as soon as we hit one of these
cases.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/locks.c |   33 +++++++++++++++++----------------
 1 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index c525aa4..9b8408e 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1368,7 +1368,7 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
 	struct file_lock *fl, **before, **my_before = NULL, *lease;
 	struct dentry *dentry = filp->f_path.dentry;
 	struct inode *inode = dentry->d_inode;
-	int error, rdlease_count = 0, wrlease_count = 0;
+	int error;
 
 	lease = *flp;
 
@@ -1404,27 +1404,28 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
 	 * then the file is not open by anyone (including us)
 	 * except for this filp.
 	 */
+	error = -EAGAIN;
 	for (before = &inode->i_flock;
 			((fl = *before) != NULL) && IS_LEASE(fl);
 			before = &fl->fl_next) {
-		if (fl->fl_file == filp)
+		if (fl->fl_file == filp) {
 			my_before = before;
-		else if (fl->fl_flags & FL_UNLOCK_PENDING)
-			/*
-			 * Someone is in the process of opening this
-			 * file for writing so we may not take an
-			 * exclusive lease on it.
-			 */
-			wrlease_count++;
-		else
-			rdlease_count++;
+			continue;
+		}
+		/*
+		 * No exclusive leases if someone else has a lease on
+		 * this file:
+		 */
+		if (arg == F_WRLCK)
+			goto out;
+		/*
+		 * Modifying our existing lease is OK, but no getting a
+		 * new lease if someone else is opening for write:
+		 */
+		if (fl->fl_flags & FL_UNLOCK_PENDING)
+			goto out;
 	}
 
-	error = -EAGAIN;
-	if ((arg == F_RDLCK && (wrlease_count > 0)) ||
-	    (arg == F_WRLCK && ((rdlease_count + wrlease_count) > 0)))
-		goto out;
-
 	if (my_before != NULL) {
 		error = lease->fl_lmops->lm_change(my_before, arg);
 		if (!error)
-- 
1.7.4.1

Re: [PATCH] locks: breaking read lease should not block read open

[Jamie Lokier]

J. Bruce Fields wrote:
> I'm not sure how to approach the lease code.
> 
> On the one hand, I've never seen any evidence that anyone outside Samba
> and NFSv4 has ever used it, and both currently make extremely limited
> use of it.  So we could probably get away with "fixing" the lease code
> to do whatever both of us need.

I've never used it, but I've _nearly_ used it (project took a
different direction), in a web application framework.

Pretty much the way CIFS/NFS use it, to keep other things (remote
state, database state, derived files) transactionally coherent with
changes to file contents by programs that only know about the files
they access.

The SIGIO stuff is a horrible interface.
I could still see me trying to use it sometime in the future.
In which case I really don't mind if you make the semantics saner :-)

Now we have fanotify which does something very similar and could have
generalised leases, but unfortunately fanotify came from such a
different motivation that it's not well suited for ordinary user
applications.

-- Jamie

Re: [PATCH] locks: breaking read lease should not block read open

[J. Bruce Fields]

On Fri, Aug 19, 2011 at 08:08:29PM +0100, Jamie Lokier wrote:
> J. Bruce Fields wrote:
> > I'm not sure how to approach the lease code.
> > 
> > On the one hand, I've never seen any evidence that anyone outside Samba
> > and NFSv4 has ever used it, and both currently make extremely limited
> > use of it.  So we could probably get away with "fixing" the lease code
> > to do whatever both of us need.
> 
> I've never used it, but I've _nearly_ used it (project took a
> different direction), in a web application framework.
> 
> Pretty much the way CIFS/NFS use it, to keep other things (remote
> state, database state, derived files) transactionally coherent with
> changes to file contents by programs that only know about the files
> they access.
> 
> The SIGIO stuff is a horrible interface.
> I could still see me trying to use it sometime in the future.
> In which case I really don't mind if you make the semantics saner :-)
> 
> Now we have fanotify which does something very similar and could have
> generalised leases, but unfortunately fanotify came from such a
> different motivation that it's not well suited for ordinary user
> applications.

I'm not sure what you mean by that--mainly just because I'm not as
familiar with fanotify as I should be.

For my case the important difference between leases and the various
notification interfaces is that leases are synchronous--the lease-holder
is notified and has a chance to clean up before releasing its lease and
allowing the conflicting operation to continue--whereas the the various
notification interfaces tell you "tough luck, something just happened".

--b.

Re: [PATCH] locks: breaking read lease should not block read open

[Jamie Lokier]

J. Bruce Fields wrote:
> On Fri, Aug 19, 2011 at 08:08:29PM +0100, Jamie Lokier wrote:
> > J. Bruce Fields wrote:
> > > I'm not sure how to approach the lease code.
> > > 
> > > On the one hand, I've never seen any evidence that anyone outside Samba
> > > and NFSv4 has ever used it, and both currently make extremely limited
> > > use of it.  So we could probably get away with "fixing" the lease code
> > > to do whatever both of us need.
> > 
> > I've never used it, but I've _nearly_ used it (project took a
> > different direction), in a web application framework.
> > 
> > Pretty much the way CIFS/NFS use it, to keep other things (remote
> > state, database state, derived files) transactionally coherent with
> > changes to file contents by programs that only know about the files
> > they access.
> > 
> > The SIGIO stuff is a horrible interface.
> > I could still see me trying to use it sometime in the future.
> > In which case I really don't mind if you make the semantics saner :-)
> > 
> > Now we have fanotify which does something very similar and could have
> > generalised leases, but unfortunately fanotify came from such a
> > different motivation that it's not well suited for ordinary user
> > applications.
> 
> I'm not sure what you mean by that--mainly just because I'm not as
> familiar with fanotify as I should be.
> 
> For my case the important difference between leases and the various
> notification interfaces is that leases are synchronous--the lease-holder
> is notified and has a chance to clean up before releasing its lease and
> allowing the conflicting operation to continue--whereas the the various
> notification interfaces tell you "tough luck, something just happened".

Hi Bruce,

My apologies for not responding earlier; I just spotted this mail
among an ocean of mails.

Fyi, fanotify is also synchronous: It blocks the conflicting operation
until the fanotify-using application allows it to proceed - or
alternatively it can prevent the conflicting operating from proceeding
at all.

It's got a nicer interface than leases (like inotify compared with
dnotify), but because it can interfere with legitimate applications
it's not suitable as a lease replacement for non-root applications;
and because it doesn't provide enough information about directory
operations, it's not a drop-in synchronous upgrade of {i,d}notify.

All the best,
-- Jamie

Re: [PATCH] locks: breaking read lease should not block read open

[J. Bruce Fields]

On Mon, Nov 21, 2011 at 12:46:50PM +0000, Jamie Lokier wrote:
> J. Bruce Fields wrote:
> > On Fri, Aug 19, 2011 at 08:08:29PM +0100, Jamie Lokier wrote:
> > > J. Bruce Fields wrote:
> > > > I'm not sure how to approach the lease code.
> > > > 
> > > > On the one hand, I've never seen any evidence that anyone outside Samba
> > > > and NFSv4 has ever used it, and both currently make extremely limited
> > > > use of it.  So we could probably get away with "fixing" the lease code
> > > > to do whatever both of us need.
> > > 
> > > I've never used it, but I've _nearly_ used it (project took a
> > > different direction), in a web application framework.
> > > 
> > > Pretty much the way CIFS/NFS use it, to keep other things (remote
> > > state, database state, derived files) transactionally coherent with
> > > changes to file contents by programs that only know about the files
> > > they access.
> > > 
> > > The SIGIO stuff is a horrible interface.
> > > I could still see me trying to use it sometime in the future.
> > > In which case I really don't mind if you make the semantics saner :-)
> > > 
> > > Now we have fanotify which does something very similar and could have
> > > generalised leases, but unfortunately fanotify came from such a
> > > different motivation that it's not well suited for ordinary user
> > > applications.
> > 
> > I'm not sure what you mean by that--mainly just because I'm not as
> > familiar with fanotify as I should be.
> > 
> > For my case the important difference between leases and the various
> > notification interfaces is that leases are synchronous--the lease-holder
> > is notified and has a chance to clean up before releasing its lease and
> > allowing the conflicting operation to continue--whereas the the various
> > notification interfaces tell you "tough luck, something just happened".
> 
> Hi Bruce,
> 
> My apologies for not responding earlier; I just spotted this mail
> among an ocean of mails.

I understand!

> Fyi, fanotify is also synchronous:

Oh, I didn't know that, apologies for the confusion.

> It blocks the conflicting operation
> until the fanotify-using application allows it to proceed - or
> alternatively it can prevent the conflicting operating from proceeding
> at all.

Just grepping for "fsnotify" (which fanotify is built on?) I find a lot
of hooks in the vfs that all appear to have void returns and to be
called after the operation is done.  How does it fail an operation?  No
doubt I'm missing something.

> It's got a nicer interface than leases (like inotify compared with
> dnotify), but because it can interfere with legitimate applications
> it's not suitable as a lease replacement for non-root applications;
> and because it doesn't provide enough information about directory
> operations, it's not a drop-in synchronous upgrade of {i,d}notify.

OK, thanks.  I don't think it's directly useful to me but I should still
take a closer to look at it to see what I can learn.

--b.

Re: [PATCH] locks: breaking read lease should not block read open

[Jamie Lokier]

J. Bruce Fields wrote:
> > Fyi, fanotify is also synchronous:
> 
> Oh, I didn't know that, apologies for the confusion.
> 
> > It blocks the conflicting operation
> > until the fanotify-using application allows it to proceed - or
> > alternatively it can prevent the conflicting operating from proceeding
> > at all.
> 
> Just grepping for "fsnotify" (which fanotify is built on?) I find a lot
> of hooks in the vfs that all appear to have void returns and to be
> called after the operation is done.  How does it fail an operation?  No
> doubt I'm missing something.

The hook is fsnotify_perm(), called in security/security.c.

-- Jamie

Re: [PATCH] locks: breaking read lease should not block read open

[J. Bruce Fields]

On Wed, Nov 23, 2011 at 12:30:57AM +0000, Jamie Lokier wrote:
> J. Bruce Fields wrote:
> > > Fyi, fanotify is also synchronous:
> > 
> > Oh, I didn't know that, apologies for the confusion.
> > 
> > > It blocks the conflicting operation
> > > until the fanotify-using application allows it to proceed - or
> > > alternatively it can prevent the conflicting operating from proceeding
> > > at all.
> > 
> > Just grepping for "fsnotify" (which fanotify is built on?) I find a lot
> > of hooks in the vfs that all appear to have void returns and to be
> > called after the operation is done.  How does it fail an operation?  No
> > doubt I'm missing something.
> 
> The hook is fsnotify_perm(), called in security/security.c.

Got it.

Hm, we could almost move a lease_break() call in there.  We wouldn't
want it turning into a no-op in the absence of CONFIG_SECURITY, though.

--b.