<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
</head>
<body bgcolor="#ffffff" text="#000000">
On 6/16/2010 6:39 PM, Joel Becker wrote:
<blockquote cite="mid:20100617013930.GA14014@mail.oracle.com"
type="cite">
<pre wrap="">On Tue, Jun 15, 2010 at 09:43:02PM -0700, Srinivas Eeda wrote:
</pre>
<blockquote type="cite">
<pre wrap="">There are two problems in dlm_run_purgelist
1. If a lockres is found to be in use, dlm_run_purgelist keeps trying to purge
the same lockres instead of trying the next lockres.
2. When a lockres is found unused, dlm_run_purgelist releases lockres spinlock
before setting DLM_LOCK_RES_DROPPING_REF and calls dlm_purge_lockres.
spinlock is reacquired but in this window lockres can get reused. This leads
to BUG.
This patch modifies dlm_run_purgelist to skip lockres if it's in use and purge
next lockres. It also sets DLM_LOCK_RES_DROPPING_REF before releasing the
lockres spinlock protecting it from getting reused.
Signed-off-by: Srinivas Eeda <a class="moz-txt-link-rfc2396E" href="mailto:srinivas.eeda@oracle.com"><srinivas.eeda@oracle.com></a>
</pre>
</blockquote>
<pre wrap=""><!---->
I don't really like the way you did this. You're absolutely
right that we need to hold the spinlock while setting DROPPING REF. But
there's no need to lift the lockres check logic into run_purge_list.
</pre>
</blockquote>
As of today, we always get the lockres from the head of the
dlm->purge_list. If it is in use, we keep trying. If that gets
purged, we move to next res which again would be on the head of the
dlm->purge_list. But with my change that won't be the case. If first
(few) lockres are in use, we will try the one we could purge. So we
should get the next lockres before list_del_init(&res->purge)
happens and hence I moved the code. If you didn't like the delete code
in dlm_run_purge_list, then we have to make dlm_purge_lockres to return
the next lockres that should get purged.<br>
<br>
If you are suggesting that we move the lockres to the tail if we found
it in use, then the code will be lot more readable. The current code
doesn't move the unused lockres to tail, so wanted to preserve that
logic as I am not sure what was the original intent. If move used
lockres to tail, would you suggest we also update lockres->last_used?<br>
<blockquote cite="mid:20100617013930.GA14014@mail.oracle.com"
type="cite">
<blockquote type="cite">
<pre wrap="">@@ -257,15 +224,12 @@ static void dlm_run_purge_list(struct dlm_ctxt *dlm,
* refs on it -- there's no need to keep the lockres
* spinlock. */
spin_lock(&lockres->spinlock);
- unused = __dlm_lockres_unused(lockres);
- spin_unlock(&lockres->spinlock);
-
- if (!unused)
- continue;
purge_jiffies = lockres->last_used +
msecs_to_jiffies(DLM_PURGE_INTERVAL_MS);
+ mlog(0, "purging lockres %.*s\n", lockres->lockname.len,
+ lockres->lockname.name);
/* Make sure that we want to be processing this guy at
* this time. */
if (!purge_now && time_after(purge_jiffies, jiffies)) {
</pre>
</blockquote>
<pre wrap=""><!---->
In fact, I'd move the __dlm_lockres_unused() and
purge_now||time_after() checks into dlm_purge_lockres(). It can return
-EBUSY if the lockres is in use. It can return -ETIME if purge_now==0
and time_after hits. Then inside run_purge_list() you just do:
spin_lock(&lockres->spinlock);
ret = dlm_purge_lockres(dlm, res, purge_now);
spin_unlock(&lockres->spinlock);
if (ret == -EAGAIN)
break;
else if (ret == -EBUSY) {
lockres = list_entry(lockres->next);
continue;
else if (ret)
BUG();
</pre>
</blockquote>
what would list_entry return, if the lockres was the last on the list.
I was thinking it would return something random .. <br>
<blockquote cite="mid:20100617013930.GA14014@mail.oracle.com"
type="cite">
<pre wrap="">
What about the dlm_lockres_get()? That's only held while we
drop the dlm spinlock in dlm_purge_lockres(), so you can move it there.
You take the kref only after the _unused() and time_after() checks.
This actually would make run_purge_list() more readable, not
less.
</pre>
</blockquote>
I agree that it is less readable now, but didn't find a better way as I
am not sure how to get to the next lockres if the current lockres that
got dropped is the middle one. :)<br>
<blockquote cite="mid:20100617013930.GA14014@mail.oracle.com"
type="cite">
<pre wrap="">
Joel
</pre>
</blockquote>
</body>
</html>