Re: file locks: Use wait_event_interruptible_timeout()

["J. Bruce Fields" <bfields@fieldses.org>]

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."