From 99fb19d49ecbeb390e023f58867c227a15f422f7 Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Mon, 22 Mar 2010 15:03:54 +0300 Subject: [PATCH 1/2] dlm: cleanup remove unused code Smatch complains because "lkb" is never NULL. Looking at it, the original code actually adds the new element to the end of the list fine, so we can just get rid of the if condition. This code is four years old and no one has complained so it must work. Signed-off-by: Dan Carpenter Signed-off-by: David Teigland --- fs/dlm/lock.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c index 17903b491298..031dbe3a15ca 100644 --- a/fs/dlm/lock.c +++ b/fs/dlm/lock.c @@ -733,10 +733,7 @@ static void lkb_add_ordered(struct list_head *new, struct list_head *head, if (lkb->lkb_rqmode < mode) break; - if (!lkb) - list_add_tail(new, head); - else - __list_add(new, lkb->lkb_statequeue.prev, &lkb->lkb_statequeue); + __list_add(new, lkb->lkb_statequeue.prev, &lkb->lkb_statequeue); } /* add/remove lkb to rsb's grant/convert/wait queue */ From 89d799d008710e048ee14df4f4e5441e9f4d5d50 Mon Sep 17 00:00:00 2001 From: David Teigland Date: Thu, 25 Mar 2010 14:23:13 -0500 Subject: [PATCH 2/2] dlm: fix ast ordering for user locks Commit 7fe2b3190b8b299409f13cf3a6f85c2bd371f8bb fixed possible misordering of completion asts (casts) and blocking asts (basts) for kernel locks. This patch does the same for locks taken by user space applications. Signed-off-by: David Teigland --- fs/dlm/user.c | 88 +++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 74 insertions(+), 14 deletions(-) diff --git a/fs/dlm/user.c b/fs/dlm/user.c index 8b6e73c47435..b6272853130c 100644 --- a/fs/dlm/user.c +++ b/fs/dlm/user.c @@ -215,6 +215,7 @@ void dlm_user_add_ast(struct dlm_lkb *lkb, int type, int mode) if (!ast_type) { kref_get(&lkb->lkb_ref); list_add_tail(&lkb->lkb_astqueue, &proc->asts); + lkb->lkb_ast_first = type; wake_up_interruptible(&proc->wait); } if (type == AST_COMP && (ast_type & AST_COMP)) @@ -223,7 +224,6 @@ void dlm_user_add_ast(struct dlm_lkb *lkb, int type, int mode) eol = lkb_is_endoflife(lkb, ua->lksb.sb_status, type); if (eol) { - lkb->lkb_ast_type &= ~AST_BAST; lkb->lkb_flags |= DLM_IFL_ENDOFLIFE; } @@ -706,7 +706,7 @@ static int device_close(struct inode *inode, struct file *file) } static int copy_result_to_user(struct dlm_user_args *ua, int compat, int type, - int bmode, char __user *buf, size_t count) + int mode, char __user *buf, size_t count) { #ifdef CONFIG_COMPAT struct dlm_lock_result32 result32; @@ -733,7 +733,7 @@ static int copy_result_to_user(struct dlm_user_args *ua, int compat, int type, if (type == AST_BAST) { result.user_astaddr = ua->bastaddr; result.user_astparam = ua->bastparam; - result.bast_mode = bmode; + result.bast_mode = mode; } else { result.user_astaddr = ua->castaddr; result.user_astparam = ua->castparam; @@ -801,7 +801,9 @@ static ssize_t device_read(struct file *file, char __user *buf, size_t count, struct dlm_user_proc *proc = file->private_data; struct dlm_lkb *lkb; DECLARE_WAITQUEUE(wait, current); - int error, type=0, bmode=0, removed = 0; + int error = 0, removed; + int ret_type, ret_mode; + int bastmode, castmode, do_bast, do_cast; if (count == sizeof(struct dlm_device_version)) { error = copy_version_to_user(buf, count); @@ -820,6 +822,8 @@ static ssize_t device_read(struct file *file, char __user *buf, size_t count, #endif return -EINVAL; + try_another: + /* do we really need this? can a read happen after a close? */ if (test_bit(DLM_PROC_FLAGS_CLOSING, &proc->flags)) return -EINVAL; @@ -855,13 +859,55 @@ static ssize_t device_read(struct file *file, char __user *buf, size_t count, lkb = list_entry(proc->asts.next, struct dlm_lkb, lkb_astqueue); - if (lkb->lkb_ast_type & AST_COMP) { - lkb->lkb_ast_type &= ~AST_COMP; - type = AST_COMP; - } else if (lkb->lkb_ast_type & AST_BAST) { - lkb->lkb_ast_type &= ~AST_BAST; - type = AST_BAST; - bmode = lkb->lkb_bastmode; + removed = 0; + ret_type = 0; + ret_mode = 0; + do_bast = lkb->lkb_ast_type & AST_BAST; + do_cast = lkb->lkb_ast_type & AST_COMP; + bastmode = lkb->lkb_bastmode; + castmode = lkb->lkb_castmode; + + /* when both are queued figure out which to do first and + switch first so the other goes in the next read */ + + if (do_cast && do_bast) { + if (lkb->lkb_ast_first == AST_COMP) { + ret_type = AST_COMP; + ret_mode = castmode; + lkb->lkb_ast_type &= ~AST_COMP; + lkb->lkb_ast_first = AST_BAST; + } else { + ret_type = AST_BAST; + ret_mode = bastmode; + lkb->lkb_ast_type &= ~AST_BAST; + lkb->lkb_ast_first = AST_COMP; + } + } else { + ret_type = lkb->lkb_ast_first; + ret_mode = (ret_type == AST_COMP) ? castmode : bastmode; + lkb->lkb_ast_type &= ~ret_type; + lkb->lkb_ast_first = 0; + } + + /* if we're doing a bast but the bast is unnecessary, then + switch to do nothing or do a cast if that was needed next */ + + if ((ret_type == AST_BAST) && + dlm_modes_compat(bastmode, lkb->lkb_castmode_done)) { + ret_type = 0; + ret_mode = 0; + + if (do_cast) { + ret_type = AST_COMP; + ret_mode = castmode; + lkb->lkb_ast_type &= ~AST_COMP; + lkb->lkb_ast_first = 0; + } + } + + if (lkb->lkb_ast_first != lkb->lkb_ast_type) { + log_print("device_read %x ast_first %x ast_type %x", + lkb->lkb_id, lkb->lkb_ast_first, lkb->lkb_ast_type); } if (!lkb->lkb_ast_type) { @@ -870,15 +916,29 @@ static ssize_t device_read(struct file *file, char __user *buf, size_t count, } spin_unlock(&proc->asts_spin); - error = copy_result_to_user(lkb->lkb_ua, - test_bit(DLM_PROC_FLAGS_COMPAT, &proc->flags), - type, bmode, buf, count); + if (ret_type) { + error = copy_result_to_user(lkb->lkb_ua, + test_bit(DLM_PROC_FLAGS_COMPAT, &proc->flags), + ret_type, ret_mode, buf, count); + + if (ret_type == AST_COMP) + lkb->lkb_castmode_done = castmode; + if (ret_type == AST_BAST) + lkb->lkb_bastmode_done = bastmode; + } /* removes reference for the proc->asts lists added by dlm_user_add_ast() and may result in the lkb being freed */ + if (removed) dlm_put_lkb(lkb); + /* the bast that was queued was eliminated (see unnecessary above), + leaving nothing to return */ + + if (!ret_type) + goto try_another; + return error; }