Prev: [[PATCH 11/11]] viafb: merge the remaining output path with enable functions
Next: [109/111] [SCSI] ibmvfc: Reduce error recovery timeout
From: Joel Becker on 11 Aug 2010 20:10
On Sat, Aug 07, 2010 at 11:09:13AM +0200, Julia Lawall wrote:
> From: Julia Lawall <julia(a)diku.dk>
>
> 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(a)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(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Joel Becker on 12 Aug 2010 05:40
On Thu, Aug 12, 2010 at 07:46:13AM +0200, Julia Lawall wrote:

> This seems a bit ugly to me, since it exposes the implementation of the
> list abstraction. What about the following:

Actually, we're going to go back to the original list_for_each()
and list_entry() version. It's a well understood idiom.
We're also going to re-audit the rest of the patch that changed
list_for_each() to list_for_each_entry(), just in case we missed another
semantic error.

Joel

--

Life's Little Instruction Book #30

"Never buy a house without a fireplace."

Joel Becker
Consulting Software Developer
Oracle
E-mail: joel.becker(a)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(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: [[PATCH 11/11]] viafb: merge the remaining output path with enable functions
Next: [109/111] [SCSI] ibmvfc: Reduce error recovery timeout