* file locks: Use wait_event_interruptible_timeout()
2008-01-15 4:26 ` Matthew Wilcox
@ 2008-01-15 4:28 ` Matthew Wilcox
2008-01-15 14:48 ` J. Bruce Fields
2008-01-15 4:29 ` file locks: Split flock_find_conflict out of flock_lock_file Matthew Wilcox
2008-01-15 14:42 ` Leak in nlmsvc_testlock for async GETFL case J. Bruce Fields
2 siblings, 1 reply; 14+ messages in thread
From: Matthew Wilcox @ 2008-01-15 4:28 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Oleg Drokin, linux-fsdevel
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."
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: file locks: Use wait_event_interruptible_timeout()
2008-01-15 4:28 ` file locks: Use wait_event_interruptible_timeout() Matthew Wilcox
@ 2008-01-15 14:48 ` J. Bruce Fields
2008-01-15 15:04 ` Matthew Wilcox
0 siblings, 1 reply; 14+ messages in thread
From: J. Bruce Fields @ 2008-01-15 14:48 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Oleg Drokin, linux-fsdevel
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."
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: file locks: Use wait_event_interruptible_timeout()
2008-01-15 14:48 ` J. Bruce Fields
@ 2008-01-15 15:04 ` Matthew Wilcox
2008-01-15 18:54 ` J. Bruce Fields
0 siblings, 1 reply; 14+ messages in thread
From: Matthew Wilcox @ 2008-01-15 15:04 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Oleg Drokin, linux-fsdevel
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."
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: file locks: Use wait_event_interruptible_timeout()
2008-01-15 15:04 ` Matthew Wilcox
@ 2008-01-15 18:54 ` J. Bruce Fields
0 siblings, 0 replies; 14+ messages in thread
From: J. Bruce Fields @ 2008-01-15 18:54 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Oleg Drokin, linux-fsdevel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* file locks: Split flock_find_conflict out of flock_lock_file
2008-01-15 4:26 ` Matthew Wilcox
2008-01-15 4:28 ` file locks: Use wait_event_interruptible_timeout() Matthew Wilcox
@ 2008-01-15 4:29 ` Matthew Wilcox
2008-01-15 18:50 ` J. Bruce Fields
2008-01-15 14:42 ` Leak in nlmsvc_testlock for async GETFL case J. Bruce Fields
2 siblings, 1 reply; 14+ messages in thread
From: Matthew Wilcox @ 2008-01-15 4:29 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: linux-fsdevel
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."
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: file locks: Split flock_find_conflict out of flock_lock_file
2008-01-15 4:29 ` file locks: Split flock_find_conflict out of flock_lock_file Matthew Wilcox
@ 2008-01-15 18:50 ` J. Bruce Fields
0 siblings, 0 replies; 14+ messages in thread
From: J. Bruce Fields @ 2008-01-15 18:50 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: linux-fsdevel
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."
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Leak in nlmsvc_testlock for async GETFL case
2008-01-15 4:26 ` Matthew Wilcox
2008-01-15 4:28 ` file locks: Use wait_event_interruptible_timeout() Matthew Wilcox
2008-01-15 4:29 ` file locks: Split flock_find_conflict out of flock_lock_file Matthew Wilcox
@ 2008-01-15 14:42 ` J. Bruce Fields
2 siblings, 0 replies; 14+ messages in thread
From: J. Bruce Fields @ 2008-01-15 14:42 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Oleg Drokin, linux-fsdevel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread