On Mon, Feb 16, 2015 at 4:02 PM, Jeff Layton <jlayton@xxxxxxxxxxxxxxx> wrote:
>
> Now that I look, it may be best to just revert this whole set for now.
> Linus, are you amenable to doing that?
Sure. But I'd prefer seeing how hard it would be to fix things first.
If this was at the end of the release cycle, I'd revert it
immediately. As it is, try to see how had it is.
The bugs I found might be as easy as just the attached (TOTALLY
UNTESTED) patch. The comment about a higher-priority process and
sched_resced() is just complete and utter crap. If somebody holds a
read lock and upgrades it to a write lock, there is absolutely *zero*
reason to let some higher-priority process get the write-lock first -
that's just simply semantically wrong bullshit. "Higher priority" does
not mean "can punch through locks".
Removing the silly incorrect counts should be trivial too. There
really are not very many users, and they can just walk the list
instead.
Now, if you've found other more fundamental bugs that look unfixable,
then that might mean that reverting it all is unavoidable, but..
Linus
fs/locks.c | 22 +++++++---------------
1 file changed, 7 insertions(+), 15 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index 4753218f308e..8fbf81429608 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -867,12 +867,11 @@ static int posix_locks_deadlock(struct file_lock *caller_fl,
*/
static int flock_lock_file(struct file *filp, struct file_lock *request)
{
- struct file_lock *new_fl = NULL;
+ struct file_lock *new_fl = NULL, *old_fl = NULL;
struct file_lock *fl;
struct file_lock_context *ctx;
struct inode *inode = file_inode(filp);
int error = 0;
- bool found = false;
LIST_HEAD(dispose);
ctx = locks_get_lock_context(inode);
@@ -894,27 +893,18 @@ static int flock_lock_file(struct file *filp, struct file_lock *request)
continue;
if (request->fl_type == fl->fl_type)
goto out;
- found = true;
- locks_delete_lock_ctx(fl, &ctx->flc_flock_cnt, &dispose);
+ old_fl = NULL;
break;
}
if (request->fl_type == F_UNLCK) {
- if ((request->fl_flags & FL_EXISTS) && !found)
+ if (old_fl)
+ locks_delete_lock_ctx(old_fl, &ctx->flc_flock_cnt, &dispose);
+ else if (request->fl_flags & FL_EXISTS)
error = -ENOENT;
goto out;
}
- /*
- * If a higher-priority process was blocked on the old file lock,
- * give it the opportunity to lock the file.
- */
- if (found) {
- spin_unlock(&ctx->flc_lock);
- cond_resched();
- spin_lock(&ctx->flc_lock);
- }
-
find_conflict:
list_for_each_entry(fl, &ctx->flc_flock, fl_list) {
if (!flock_locks_conflict(request, fl))
@@ -928,6 +918,8 @@ find_conflict:
}
if (request->fl_flags & FL_ACCESS)
goto out;
+ if (old_fl)
+ locks_delete_lock_ctx(old_fl, &ctx->flc_flock_cnt, &dispose);
locks_copy_lock(new_fl, request);
locks_insert_lock_ctx(new_fl, &ctx->flc_flock_cnt, &ctx->flc_flock);
new_fl = NULL;
