Re: [PATCH 2/4] locks: don't unnecessarily fail posix lockoperations

[Trond Myklebust]

On Fri, 2006-03-31 at 21:46 +0200, Miklos Szeredi wrote:
> > > In the first case no new locks are needed.  In the second, no locks
> > > are modified prior to the check.
> > 
> > Consider something like
> > 
> > fcntl(SETLK, 0, 100)
> > fcntl(SETLK, 0, 100)
> > fcntl(SETLK, 0, 100)
> 
> Huh?  What is the type of lock in each case.
> 
> But anyway your example is no good.  If the new lock completely covers
> the previous one, then the old lock will simply be adjusted and no new
> lock is inserted.

Slip of the mailer. It posted when I wanted to cancel the mail (I had to
step out for an errand)...

OK. I see what you mean now. Do you agree with the following analysis?

        1) We need 2 extra locks for the case where we
        upgrade/downgrade, a single existing lock and end up splitting
        it.

        2) We need to use 1 extra lock in the case where we unlock and
        split a single existing lock.

        3) We also need to use 1 extra lock in the case where there is
        no existing lock that is contiguous with the region to lock.

In all other cases, we resort to modifying existing locks instead of
using new_fl/new_fl2.

In cases (1) and (2) we do need to modify the existing lock. Since this
is only done after we've set up the extra locks, we're safe.

Could I still suggest a couple of modifications to your patch? Firstly,
we only need to test for 'added' once. Secondly, in cases (2) and (3),
we can still complete the lock despite one of new_fl/new_fl2 failing to
be allocated.

Cheers,
  Trond
VFS,locks: locks: don't unnecessarily fail posix lock operations

---

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

diff --git a/fs/locks.c b/fs/locks.c
index 6ba3756..973c1d9 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -839,10 +839,6 @@ static int __posix_lock_file_conf(struct
 	if (request->fl_flags & FL_ACCESS)
 		goto out;
 
-	error = -ENOLCK; /* "no luck" */
-	if (!(new_fl && new_fl2))
-		goto out;
-
 	/*
 	 * We've allocated the new locks in advance, so there are no
 	 * errors possible (and no blocking operations) from here on.
@@ -943,9 +939,25 @@ static int __posix_lock_file_conf(struct
 		before = &fl->fl_next;
 	}
 
-	error = 0;
-	if (!added) {
-		if (request->fl_type == F_UNLCK)
+	if (request->fl_type == F_UNLCK) {
+		if (!added)
+			goto out;
+		if (!new_fl2 && right && left == right) {
+			new_fl2 = new_fl;
+			error = -ENOLCK;
+			if (!new_fl2)
+				goto out;
+			new_fl = NULL;
+		}
+	} else if (!added) {
+		error = -ENOLCK;
+		if (!new_fl) {
+			new_fl = new_fl2;
+			if (!new_fl)
+				goto out;
+			new_fl2 = NULL;
+		}
+		if (!new_fl2 && right && left == right)
 			goto out;
 		locks_copy_lock(new_fl, request);
 		locks_insert_lock(before, new_fl);
@@ -968,6 +980,7 @@ static int __posix_lock_file_conf(struct
 		left->fl_end = request->fl_start - 1;
 		locks_wake_up_blocks(left);
 	}
+	error = 0;
  out:
 	unlock_kernel();
 	/*