On Sat, Aug 07, 2010 at 11:09:13AM +0200, Julia Lawall wrote: > From: Julia Lawall <julia@xxxxxxx> > > list_for_each_entry uses its first argument to move from one element to the > next, so modifying it can break the iteration. Thanks for catching the bug. It was introduced by 800deef3 [ocfs2: use list_for_each_entry where benefical]. I blame Christoph. > diff --git a/fs/ocfs2/dlm/dlmrecovery.c b/fs/ocfs2/dlm/dlmrecovery.c > index 9dfaac7..7084a11 100644 > --- a/fs/ocfs2/dlm/dlmrecovery.c > +++ b/fs/ocfs2/dlm/dlmrecovery.c > @@ -1792,10 +1792,10 @@ static int dlm_process_recovery_data(struct dlm_ctxt *dlm, > for (j = DLM_GRANTED_LIST; j <= DLM_BLOCKED_LIST; j++) { > tmpq = dlm_list_idx_to_ptr(res, j); > list_for_each_entry(lock, tmpq, list) { > - if (lock->ml.cookie != ml->cookie) > + if (lock->ml.cookie != ml->cookie) { > lock = NULL; > - else > break; > + } > } > if (lock) > break; However, this is not the correct solution. The goal of the original code, which used to use list_for_each(), was to leave lock non-NULL if the cookie was found. Your version merely exits the loop on the first non-matching entry, always leaving lock==NULL if there is a non-matching entry. One possible solution is to return the original code: --8<----------------------------------------------------------------- @@ -1747,7 +1747,7 @@ static int dlm_process_recovery_data(struct dlm_ctxt *dlm, struct dlm_migratable_lockres *mres) { struct dlm_migratable_lock *ml; - struct list_head *queue; + struct list_head *queue, *iter; struct list_head *tmpq = NULL; struct dlm_lock *newlock = NULL; struct dlm_lockstatus *lksb = NULL; @@ -1791,11 +1791,12 @@ static int dlm_process_recovery_data(struct dlm_ctxt *dlm, spin_lock(&res->spinlock); for (j = DLM_GRANTED_LIST; j <= DLM_BLOCKED_LIST; j++) { tmpq = dlm_list_idx_to_ptr(res, j); - list_for_each_entry(lock, tmpq, list) { - if (lock->ml.cookie != ml->cookie) - lock = NULL; - else + list_for_each(iter, tmpq) { + lock = list_entry(iter, struct dlm_lock, list); + + if (lock->ml.cookie == ml->cookie) break; + lock = NULL; } if (lock) break; -->8----------------------------------------------------------------- Another approach would be to keep list_for_each_entry() around, but use a better check for entry existence: --8<----------------------------------------------------------------- @@ -1792,13 +1792,12 @@ static int dlm_process_recovery_data(struct dlm_ctxt *dlm, for (j = DLM_GRANTED_LIST; j <= DLM_BLOCKED_LIST; j++) { tmpq = dlm_list_idx_to_ptr(res, j); list_for_each_entry(lock, tmpq, list) { - if (lock->ml.cookie != ml->cookie) - lock = NULL; - else + if (lock->ml.cookie == ml->cookie) break; } - if (lock) + if (&lock->list != tmpq) break; + lock = NULL; } /* lock is always created locally first, and -->8----------------------------------------------------------------- I think I like the second one better. Sunil, what do you think? Joel -- Life's Little Instruction Book #335 "Every so often, push your luck." Joel Becker Consulting Software Developer Oracle E-mail: joel.becker@xxxxxxxxxx Phone: (650) 506-8127 -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html