On Wed, 11 Aug 2010, Joel Becker wrote:
> 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;
> }
This seems a bit ugly to me, since it exposes the implementation of the
list abstraction. What about the following:
lock = NULL;
list_for_each_entry(x, tmpq, list) {
if (x->ml.cookie == ml->cookie) {
lock = x;
break;
}
}
julia
--
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
