Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759661Ab0HLAFv (ORCPT ); Wed, 11 Aug 2010 20:05:51 -0400 Received: from rcsinet10.oracle.com ([148.87.113.121]:36281 "EHLO rcsinet10.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759486Ab0HLAFr (ORCPT ); Wed, 11 Aug 2010 20:05:47 -0400 Date: Wed, 11 Aug 2010 17:03:56 -0700 From: Joel Becker To: Julia Lawall Cc: Mark Fasheh , ocfs2-devel@oss.oracle.com, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org, Sunil Mushran Subject: Re: [PATCH 1/2] fs/ocfs2/dlm: Eliminate update of list_for_each_entry loop cursor Message-ID: <20100812000355.GA7195@mail.oracle.com> Mail-Followup-To: Julia Lawall , Mark Fasheh , ocfs2-devel@oss.oracle.com, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org, Sunil Mushran References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Burt-Line: Trees are cool. X-Red-Smith: Ninety feet between bases is perhaps as close as man has ever come to perfection. User-Agent: Mutt/1.5.20 (2009-06-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3562 Lines: 106 On Sat, Aug 07, 2010 at 11:09:13AM +0200, Julia Lawall wrote: > From: Julia Lawall > > 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@oracle.com Phone: (650) 506-8127 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/