Leak in nlmsvc_testlock for async GETFL case

Leak in nlmsvc_testlock for async GETFL case

[Oleg Drokin]

[-- Attachment #1: Type: text/plain, Size: 448 bytes --]

Hello!

    Per our discussion, I am resending this patch that fixes a leak in  
nlmsvc_testlock.
    It is addition to another leak fixing patch you already have.
    Without the patch, there is a leakage of nlmblock structure  
refcount that holds a
    reference nlmfile structure, that holds a reference to struct  
file, when async GETFL
    is used (-EINPROGRESS return from file_ops->lock()), and also in  
some error cases.

Bye,
     Oleg

[-- Attachment #2: nlmblock-leak_fix-1.diff --]
[-- Type: application/octet-stream, Size: 1043 bytes --]

--- linux-2.6.22.orig/fs/lockd/svclock.c	2007-07-08 19:32:17.000000000 -0400
+++ linux-2.6.22/fs/lockd/svclock.c	2007-11-25 03:09:46.000000000 -0500
@@ -492,7 +498,8 @@
 			block, block->b_flags, block->b_fl);
 		if (block->b_flags & B_TIMED_OUT) {
 			nlmsvc_unlink_block(block);
-			return nlm_lck_denied;
+			ret = nlm_lck_denied;
+                        goto out;
 		}
 		if (block->b_flags & B_GOT_CALLBACK) {
 			if (block->b_fl != NULL
@@ -502,15 +509,19 @@
 			}
 			else {
 				nlmsvc_unlink_block(block);
-				return nlm_granted;
+				ret = nlm_granted;
+                                goto out;
 			}
 		}
-		return nlm_drop_reply;
+		ret = nlm_drop_reply;
+                goto out;
 	}
 
 	error = vfs_test_lock(file->f_file, &lock->fl);
-	if (error == -EINPROGRESS)
-		return nlmsvc_defer_lock_rqst(rqstp, block);
+	if (error == -EINPROGRESS) {
+		ret = nlmsvc_defer_lock_rqst(rqstp, block);
+                goto out;
+        }
 	if (error) {
 		ret = nlm_lck_denied_nolocks;
 		goto out;

[-- Attachment #3: Type: text/plain, Size: 1 bytes --]

Re: Leak in nlmsvc_testlock for async GETFL case

[J. Bruce Fields]

On Thu, Nov 29, 2007 at 01:46:04PM -0500, Oleg Drokin wrote:
> Hello!
>
>    Per our discussion, I am resending this patch that fixes a leak in
>    nlmsvc_testlock.  It is addition to another leak fixing patch you
>    already have.  Without the patch, there is a leakage of nlmblock
>    structure refcount that holds a reference nlmfile structure, that
>    holds a reference to struct file, when async GETFL is used
>    (-EINPROGRESS return from file_ops->lock()), and also in some error
>    cases.

Thanks for the fix!  Looks right to me.  Yes, somehow I missed this one
when you sent it privately.  Applied and pushed out to

	git://linux-nfs.org/~bfields/linux.git nfs-server-stable

and I'll submit it for 2.6.25.

Minor nit: your editor is messing up the whitespace; keep the indents
all tabs.  Also, for future patches, could you see
Documentation/SubmittingPatches?  In particular, see "12) Sign your
work".  And it'd be better to inline the patch if at all possible.

Thanks again.

--b.

Re: Leak in nlmsvc_testlock for async GETFL case

[Felix Blyakher]

On Nov 29, 2007, at 12:46 PM, Oleg Drokin wrote:

> Hello!
>
>    Per our discussion, I am resending this patch that fixes a leak  
> in nlmsvc_testlock.
>    It is addition to another leak fixing patch you already have.
>    Without the patch, there is a leakage of nlmblock structure  
> refcount that holds a
>    reference nlmfile structure, that holds a reference to struct  
> file, when async GETFL
>    is used (-EINPROGRESS return from file_ops->lock()), and also in  
> some error cases.
>
> Bye,
>     Oleg
> <nlmblock-leak_fix-1.diff>
>

 > @@ -502,15 +509,19 @@
 > 			}
 > 			else {
 > 				nlmsvc_unlink_block(block);
 > -				return nlm_granted;
 > +				ret = nlm_granted;
 > +                                goto out;
 > 			}

Do we really need to release block with 'goto out' here?
nlmsvc_unlink_block() is doing it internally in the following
call chain:

     nlmsvc_unlink_block
         nlmsvc_remove_block
             nlmsvc_release_block

And on a last reference 'block' will be freed in nlmsvc_free_block.
Are you sure we have an extra reference here and won't go through
nlmsvc_free_block?

Felix

Re: Leak in nlmsvc_testlock for async GETFL case

[Oleg Drokin]

Hello!

On Dec 3, 2007, at 12:00 PM, Felix Blyakher wrote:
>>   Per our discussion, I am resending this patch that fixes a leak  
>> in nlmsvc_testlock.
>>   It is addition to another leak fixing patch you already have.
>>   Without the patch, there is a leakage of nlmblock structure  
>> refcount that holds a
>>   reference nlmfile structure, that holds a reference to struct  
>> file, when async GETFL
>>   is used (-EINPROGRESS return from file_ops->lock()), and also in  
>> some error cases.
> > @@ -502,15 +509,19 @@
> > 			}
> > 			else {
> > 				nlmsvc_unlink_block(block);
> > -				return nlm_granted;
> > +				ret = nlm_granted;
> > +                                goto out;
> > 			}
>
> Do we really need to release block with 'goto out' here?

Yes, absolutely.

> nlmsvc_unlink_block() is doing it internally in the following
> call chain:
...
> And on a last reference 'block' will be freed in nlmsvc_free_block.
> Are you sure we have an extra reference here and won't go through
> nlmsvc_free_block?

It releases reference that was obtained when lock was linked into a  
queue by
nlmsvc_insert_block (which is indicated by B_QUEUED flag in the  
nlmblock)
And then we need to release another reference that we got from block  
lookup.
This path you pointed out is definitely leaking, it is a success path  
for
async GETFL handling so I tested this case.

Bye,
     Oleg

Re: Leak in nlmsvc_testlock for async GETFL case

[Oleg Drokin]

[-- Attachment #1: Type: text/plain, Size: 1244 bytes --]

Hello!

On Nov 29, 2007, at 2:08 PM, J. Bruce Fields wrote:

> On Thu, Nov 29, 2007 at 01:46:04PM -0500, Oleg Drokin wrote:
>> Hello!
>>
>>   Per our discussion, I am resending this patch that fixes a leak in
>>   nlmsvc_testlock.  It is addition to another leak fixing patch you
>>   already have.  Without the patch, there is a leakage of nlmblock
>>   structure refcount that holds a reference nlmfile structure, that
>>   holds a reference to struct file, when async GETFL is used
>>   (-EINPROGRESS return from file_ops->lock()), and also in some error
>>   cases
> Thanks for the fix!  Looks right to me.  Yes, somehow I missed this  
> one
> when you sent it privately.  Applied and pushed out to
> 	git://linux-nfs.org/~bfields/linux.git nfs-server-stable
> and I'll submit it for 2.6.25.

After playing around that code a bit more, I figured out the leak was  
not
completely fixed by that first patch, the case where there is  
conflicting
lock passed in by callback still leaks block reference.
This simple incremental fix (against your current tree) takes care of  
that.

Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
I guess there is no way to safely include patches in messages in apple
mail, so the patch is still attached.

[-- Attachment #2: nlmblock-leak_fix-2-incr.diff --]
[-- Type: application/octet-stream, Size: 631 bytes --]

Fix nlm_block leak for the case of supplied blocking lock info.
Signed-off-by: Oleg Drokin <green@linuxhacker.ru>

--- /tmp/svclock.c	2008-01-11 21:40:39.000000000 -0500
+++ linux-2.6.22/fs/lockd/svclock.c	2008-01-11 21:41:54.000000000 -0500
@@ -505,12 +501,12 @@ nlmsvc_testlock(struct svc_rqst *rqstp, 
 			goto out;
 		}
 		if (block->b_flags & B_GOT_CALLBACK) {
+			nlmsvc_unlink_block(block);
 			if (block->b_fl != NULL
 					&& block->b_fl->fl_type != F_UNLCK) {
 				lock->fl = *block->b_fl;
 				goto conf_lock;
 			} else {
-				nlmsvc_unlink_block(block);
 				ret = nlm_granted;
 				goto out;
 			}

[-- Attachment #3: Type: text/plain, Size: 1 bytes --]

Re: Leak in nlmsvc_testlock for async GETFL case

[J. Bruce Fields]

On Fri, Jan 11, 2008 at 09:57:35PM -0500, Oleg Drokin wrote:
> Hello!
>
> On Nov 29, 2007, at 2:08 PM, J. Bruce Fields wrote:
>
>> On Thu, Nov 29, 2007 at 01:46:04PM -0500, Oleg Drokin wrote:
>>> Hello!
>>>
>>>   Per our discussion, I am resending this patch that fixes a leak in
>>>   nlmsvc_testlock.  It is addition to another leak fixing patch you
>>>   already have.  Without the patch, there is a leakage of nlmblock
>>>   structure refcount that holds a reference nlmfile structure, that
>>>   holds a reference to struct file, when async GETFL is used
>>>   (-EINPROGRESS return from file_ops->lock()), and also in some error
>>>   cases
>> Thanks for the fix!  Looks right to me.  Yes, somehow I missed this  
>> one
>> when you sent it privately.  Applied and pushed out to
>> 	git://linux-nfs.org/~bfields/linux.git nfs-server-stable
>> and I'll submit it for 2.6.25.
>
> After playing around that code a bit more, I figured out the leak was  
> not
> completely fixed by that first patch, the case where there is  
> conflicting
> lock passed in by callback still leaks block reference.
> This simple incremental fix (against your current tree) takes care of  
> that.
>
> Signed-off-by: Oleg Drokin <green@linuxhacker.ru>

Thanks!  I've queued it up for 2.6.25.

--b.

Re: Leak in nlmsvc_testlock for async GETFL case

[Matthew Wilcox]

On Mon, Jan 14, 2008 at 03:44:19PM -0500, J. Bruce Fields wrote:
> Thanks!  I've queued it up for 2.6.25.

Hi Bruce,

I haven't had as much time to play with de-BKL-ising fs/locks.c as I
would like, so fixing that for 2.6.25 is probably out of the question,
but here are two janitorial patches that hopefully can be applied and
will make the next steps easier.  They make sense all by themselves,
even if I don't get back to this project for a few months.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

file locks: Use wait_event_interruptible_timeout()

[Matthew Wilcox]

interruptible_sleep_on_locked() is just an open-coded
wait_event_interruptible_timeout() with a few assumptions since we know
we hold the BKL.  locks_block_on_timeout() is only used in one place, so
it's actually simpler to inline it into its caller.

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>

 locks.c |   33 ++++-----------------------------
 1 file changed, 4 insertions(+), 29 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 8b8388e..b681459 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -634,33 +634,6 @@ static int flock_locks_conflict(struct file_lock *caller_fl, struct file_lock *s
 	return (locks_conflict(caller_fl, sys_fl));
 }
 
-static int interruptible_sleep_on_locked(wait_queue_head_t *fl_wait, int timeout)
-{
-	int result = 0;
-	DECLARE_WAITQUEUE(wait, current);
-
-	__set_current_state(TASK_INTERRUPTIBLE);
-	add_wait_queue(fl_wait, &wait);
-	if (timeout == 0)
-		schedule();
-	else
-		result = schedule_timeout(timeout);
-	if (signal_pending(current))
-		result = -ERESTARTSYS;
-	remove_wait_queue(fl_wait, &wait);
-	__set_current_state(TASK_RUNNING);
-	return result;
-}
-
-static int locks_block_on_timeout(struct file_lock *blocker, struct file_lock *waiter, int time)
-{
-	int result;
-	locks_insert_block(blocker, waiter);
-	result = interruptible_sleep_on_locked(&waiter->fl_wait, time);
-	__locks_delete_block(waiter);
-	return result;
-}
-
 void
 posix_test_lock(struct file *filp, struct file_lock *fl)
 {
@@ -1256,7 +1229,10 @@ restart:
 		if (break_time == 0)
 			break_time++;
 	}
-	error = locks_block_on_timeout(flock, new_fl, break_time);
+	locks_insert_block(flock, new_fl);
+	error = wait_event_interruptible_timeout(new_fl->fl_wait,
+						!new_fl->fl_next, break_time);
+	__locks_delete_block(new_fl);
 	if (error >= 0) {
 		if (error == 0)
 			time_out_leases(inode);

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

file locks: Split flock_find_conflict out of flock_lock_file

[Matthew Wilcox]

Reduce the spaghetti-like nature of flock_lock_file by making the chunk
of code labelled find_conflict into its own function.  Also allocate
memory before taking the kernel lock in preparation for switching to a
normal spinlock.

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>

diff --git a/fs/locks.c b/fs/locks.c
index b681459..bc691e5 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -699,6 +699,33 @@ next_task:
 	return 0;
 }
 
+/*
+ * A helper function for flock_lock_file().  It searches the list of locks
+ * for @inode looking for one which would conflict with @request.
+ * If it does not find one, it returns the address where the requested lock
+ * should be inserted.  If it does find a conflicting lock, it returns NULL.
+ */
+static struct file_lock **
+flock_find_conflict(struct inode *inode, struct file_lock *request)
+{
+	struct file_lock **before;
+
+	for_each_lock(inode, before) {
+		struct file_lock *fl = *before;
+		if (IS_POSIX(fl))
+			break;
+		if (IS_LEASE(fl))
+			continue;
+		if (!flock_locks_conflict(request, fl))
+			continue;
+
+		if (request->fl_flags & FL_SLEEP)
+			locks_insert_block(fl, request);
+		return NULL;
+	}
+	return before;
+}
+
 /* Try to create a FLOCK lock on filp. We always insert new FLOCK locks
  * after any leases, but before any posix locks.
  *
@@ -714,18 +741,21 @@ static int flock_lock_file(struct file *filp, struct file_lock *request)
 	int error = 0;
 	int found = 0;
 
-	lock_kernel();
-	if (request->fl_flags & FL_ACCESS)
-		goto find_conflict;
+	if (request->fl_flags & FL_ACCESS) {
+		lock_kernel();
+		before = flock_find_conflict(inode, request);
+		unlock_kernel();
+		return before ? 0 : -EAGAIN;
+	}
 
 	if (request->fl_type != F_UNLCK) {
-		error = -ENOMEM;
 		new_fl = locks_alloc_lock();
-		if (new_fl == NULL)
-			goto out;
-		error = 0;
+		if (!new_fl)
+			return -ENOMEM;
 	}
 
+	lock_kernel();
+
 	for_each_lock(inode, before) {
 		struct file_lock *fl = *before;
 		if (IS_POSIX(fl))
@@ -754,22 +784,12 @@ static int flock_lock_file(struct file *filp, struct file_lock *request)
 	if (found)
 		cond_resched();
 
-find_conflict:
-	for_each_lock(inode, before) {
-		struct file_lock *fl = *before;
-		if (IS_POSIX(fl))
-			break;
-		if (IS_LEASE(fl))
-			continue;
-		if (!flock_locks_conflict(request, fl))
-			continue;
+	before = flock_find_conflict(inode, request);
+	if (!before) {
 		error = -EAGAIN;
-		if (request->fl_flags & FL_SLEEP)
-			locks_insert_block(fl, request);
 		goto out;
 	}
-	if (request->fl_flags & FL_ACCESS)
-		goto out;
+
 	locks_copy_lock(new_fl, request);
 	locks_insert_lock(before, new_fl);
 	new_fl = NULL;
-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: Leak in nlmsvc_testlock for async GETFL case

[J. Bruce Fields]

On Mon, Jan 14, 2008 at 09:26:06PM -0700, Matthew Wilcox wrote:
> On Mon, Jan 14, 2008 at 03:44:19PM -0500, J. Bruce Fields wrote:
> > Thanks!  I've queued it up for 2.6.25.
> 
> Hi Bruce,
> 
> I haven't had as much time to play with de-BKL-ising fs/locks.c as I
> would like, so fixing that for 2.6.25 is probably out of the question,
> but here are two janitorial patches that hopefully can be applied and
> will make the next steps easier.  They make sense all by themselves,
> even if I don't get back to this project for a few months.

OK, thanks.

Hopefully we will get back to this--it'll be nice to finally make
progress on it.

--b.

Re: file locks: Use wait_event_interruptible_timeout()

[J. Bruce Fields]

On Mon, Jan 14, 2008 at 09:28:30PM -0700, Matthew Wilcox wrote:
> 
> interruptible_sleep_on_locked() is just an open-coded
> wait_event_interruptible_timeout() with a few assumptions since we know
> we hold the BKL.  locks_block_on_timeout() is only used in one place, so
> it's actually simpler to inline it into its caller.

Makes sense, thanks.  So the assumption we were depending on the BKL for
was that we could count on the wake-up not coming till after we block,
so we could skip a check ->fl_next that's normally needed to resolve the
usual sleeping-on-some-condition race?

--b.

> 
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> 
>  locks.c |   33 ++++-----------------------------
>  1 file changed, 4 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 8b8388e..b681459 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -634,33 +634,6 @@ static int flock_locks_conflict(struct file_lock *caller_fl, struct file_lock *s
>  	return (locks_conflict(caller_fl, sys_fl));
>  }
>  
> -static int interruptible_sleep_on_locked(wait_queue_head_t *fl_wait, int timeout)
> -{
> -	int result = 0;
> -	DECLARE_WAITQUEUE(wait, current);
> -
> -	__set_current_state(TASK_INTERRUPTIBLE);
> -	add_wait_queue(fl_wait, &wait);
> -	if (timeout == 0)
> -		schedule();
> -	else
> -		result = schedule_timeout(timeout);
> -	if (signal_pending(current))
> -		result = -ERESTARTSYS;
> -	remove_wait_queue(fl_wait, &wait);
> -	__set_current_state(TASK_RUNNING);
> -	return result;
> -}
> -
> -static int locks_block_on_timeout(struct file_lock *blocker, struct file_lock *waiter, int time)
> -{
> -	int result;
> -	locks_insert_block(blocker, waiter);
> -	result = interruptible_sleep_on_locked(&waiter->fl_wait, time);
> -	__locks_delete_block(waiter);
> -	return result;
> -}
> -
>  void
>  posix_test_lock(struct file *filp, struct file_lock *fl)
>  {
> @@ -1256,7 +1229,10 @@ restart:
>  		if (break_time == 0)
>  			break_time++;
>  	}
> -	error = locks_block_on_timeout(flock, new_fl, break_time);
> +	locks_insert_block(flock, new_fl);
> +	error = wait_event_interruptible_timeout(new_fl->fl_wait,
> +						!new_fl->fl_next, break_time);
> +	__locks_delete_block(new_fl);
>  	if (error >= 0) {
>  		if (error == 0)
>  			time_out_leases(inode);
> 
> -- 
> Intel are signing my paycheques ... these opinions are still mine
> "Bill, look, we understand that you're interested in selling us this
> operating system, but compare it to ours.  We can't possibly take such
> a retrograde step."

Re: file locks: Use wait_event_interruptible_timeout()

[Matthew Wilcox]

On Tue, Jan 15, 2008 at 09:48:51AM -0500, J. Bruce Fields wrote:
> On Mon, Jan 14, 2008 at 09:28:30PM -0700, Matthew Wilcox wrote:
> > interruptible_sleep_on_locked() is just an open-coded
> > wait_event_interruptible_timeout() with a few assumptions since we know
> > we hold the BKL.  locks_block_on_timeout() is only used in one place, so
> > it's actually simpler to inline it into its caller.
> 
> Makes sense, thanks.  So the assumption we were depending on the BKL for
> was that we could count on the wake-up not coming till after we block,
> so we could skip a check ->fl_next that's normally needed to resolve the
> usual sleeping-on-some-condition race?

That's right.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: file locks: Split flock_find_conflict out of flock_lock_file

[J. Bruce Fields]

On Mon, Jan 14, 2008 at 09:29:39PM -0700, Matthew Wilcox wrote:
> 
> Reduce the spaghetti-like nature of flock_lock_file by making the chunk
> of code labelled find_conflict into its own function.  Also allocate
> memory before taking the kernel lock in preparation for switching to a
> normal spinlock.

If we did those two steps separately, would the result be two simpler
patches?

> 
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index b681459..bc691e5 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -699,6 +699,33 @@ next_task:
>  	return 0;
>  }
>  
> +/*
> + * A helper function for flock_lock_file().  It searches the list of locks
> + * for @inode looking for one which would conflict with @request.
> + * If it does not find one, it returns the address where the requested lock
> + * should be inserted.  If it does find a conflicting lock, it returns NULL.
> + */

The return value is the reverse of what I'd naively expect--I don't
expect something named flock_find_conflict to return something exactly
when conflict is *not* found, but I don't see a better way to do it.
Perhaps there's a better name?

--b.


> +static struct file_lock **
> +flock_find_conflict(struct inode *inode, struct file_lock *request)
> +{
> +	struct file_lock **before;
> +
> +	for_each_lock(inode, before) {
> +		struct file_lock *fl = *before;
> +		if (IS_POSIX(fl))
> +			break;
> +		if (IS_LEASE(fl))
> +			continue;
> +		if (!flock_locks_conflict(request, fl))
> +			continue;
> +
> +		if (request->fl_flags & FL_SLEEP)
> +			locks_insert_block(fl, request);
> +		return NULL;
> +	}
> +	return before;
> +}
> +
>  /* Try to create a FLOCK lock on filp. We always insert new FLOCK locks
>   * after any leases, but before any posix locks.
>   *
> @@ -714,18 +741,21 @@ static int flock_lock_file(struct file *filp, struct file_lock *request)
>  	int error = 0;
>  	int found = 0;
>  
> -	lock_kernel();
> -	if (request->fl_flags & FL_ACCESS)
> -		goto find_conflict;
> +	if (request->fl_flags & FL_ACCESS) {
> +		lock_kernel();
> +		before = flock_find_conflict(inode, request);
> +		unlock_kernel();
> +		return before ? 0 : -EAGAIN;
> +	}
>  
>  	if (request->fl_type != F_UNLCK) {
> -		error = -ENOMEM;
>  		new_fl = locks_alloc_lock();
> -		if (new_fl == NULL)
> -			goto out;
> -		error = 0;
> +		if (!new_fl)
> +			return -ENOMEM;
>  	}
>  
> +	lock_kernel();
> +
>  	for_each_lock(inode, before) {
>  		struct file_lock *fl = *before;
>  		if (IS_POSIX(fl))
> @@ -754,22 +784,12 @@ static int flock_lock_file(struct file *filp, struct file_lock *request)
>  	if (found)
>  		cond_resched();
>  
> -find_conflict:
> -	for_each_lock(inode, before) {
> -		struct file_lock *fl = *before;
> -		if (IS_POSIX(fl))
> -			break;
> -		if (IS_LEASE(fl))
> -			continue;
> -		if (!flock_locks_conflict(request, fl))
> -			continue;
> +	before = flock_find_conflict(inode, request);
> +	if (!before) {
>  		error = -EAGAIN;
> -		if (request->fl_flags & FL_SLEEP)
> -			locks_insert_block(fl, request);
>  		goto out;
>  	}
> -	if (request->fl_flags & FL_ACCESS)
> -		goto out;
> +
>  	locks_copy_lock(new_fl, request);
>  	locks_insert_lock(before, new_fl);
>  	new_fl = NULL;
> -- 
> Intel are signing my paycheques ... these opinions are still mine
> "Bill, look, we understand that you're interested in selling us this
> operating system, but compare it to ours.  We can't possibly take such
> a retrograde step."

Re: file locks: Use wait_event_interruptible_timeout()

[J. Bruce Fields]

On Tue, Jan 15, 2008 at 08:04:47AM -0700, Matthew Wilcox wrote:
> On Tue, Jan 15, 2008 at 09:48:51AM -0500, J. Bruce Fields wrote:
> > On Mon, Jan 14, 2008 at 09:28:30PM -0700, Matthew Wilcox wrote:
> > > interruptible_sleep_on_locked() is just an open-coded
> > > wait_event_interruptible_timeout() with a few assumptions since we know
> > > we hold the BKL.  locks_block_on_timeout() is only used in one place, so
> > > it's actually simpler to inline it into its caller.
> > 
> > Makes sense, thanks.  So the assumption we were depending on the BKL for
> > was that we could count on the wake-up not coming till after we block,
> > so we could skip a check ->fl_next that's normally needed to resolve the
> > usual sleeping-on-some-condition race?
> 
> That's right.

OK, thanks, applied just with the "few assumptions" replaced by a
description of that particular problem:

	"interruptible_sleep_on_locked() is just an open-coded
	wait_event_interruptible_timeout(), with the one difference that
	interruptible_sleep_on_locked() doesn't bother to check the
	condition on which it waits, depending instead on the BKL to
	avoid the case where it blocks after the wakeup has already been
	called.

	locks_block_on_timeout() is only used in one place, so it's
	actually simpler to inline it into its caller."

Pending locks patches available from:

	git://linux-nfs.org/~bfields/linux.git locks


--b.