<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
<div class="moz-cite-prefix">On 06/06/2014 10:02 AM, Anand Avati
wrote:<br>
</div>
<blockquote
cite="mid:CAFboF2w=gnPDN4uRwUW0fs_Y1A31Oh3m5X+RYCFRdpgfk1NfYw@mail.gmail.com"
type="cite">
<div dir="ltr"><br>
<div class="gmail_extra"><br>
<br>
<div class="gmail_quote">On Thu, Jun 5, 2014 at 7:52 PM,
Pranith Kumar Karampuri <span dir="ltr"><<a
moz-do-not-send="true" href="mailto:pkarampu@redhat.com"
target="_blank">pkarampu@redhat.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<div>
<div class="h5">
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
</div>
<div>This sounds a bit complicated. I think
there is a much simpler solution:</div>
<div><br>
</div>
<div>- First, make update_refkeeper() check
for blocked locks (which I mentioned as
"optional" previously)</div>
<div><br>
</div>
<div>- Make grant_blocked_locks() double up
and do the job of update_refkeeper()
internally.</div>
<div><br>
</div>
<div>Something which looks like this:</div>
<div><br>
</div>
<div>
<div>
<div>diff --git
a/xlators/features/locks/src/common.c
b/xlators/features/locks/src/common.c</div>
<div>index f6c71c1..38df385 100644</div>
<div>---
a/xlators/features/locks/src/common.c</div>
<div>+++
b/xlators/features/locks/src/common.c</div>
<div>@@ -126,8 +126,14 @@
__pl_inode_is_empty (pl_inode_t
*pl_inode)</div>
<div> if (!list_empty
(&dom->entrylk_list))</div>
<div> is_empty =
0;</div>
<div> </div>
<div>+ if (!list_empty
(&dom->blocked_entrylks))</div>
<div>+ is_empty =
0;</div>
<div>+</div>
<div> if (!list_empty
(&dom->inodelk_list))</div>
<div> is_empty =
0;</div>
<div>+</div>
<div>+ if (!list_empty
(&dom->blocked_inodelks))</div>
<div>+ is_empty =
0;</div>
<div> }</div>
<div> </div>
<div> return is_empty;</div>
<div>@@ -944,12 +950,18 @@
grant_blocked_locks (xlator_t *this,
pl_inode_t *pl_inode)</div>
<div> struct list_head
granted_list;</div>
<div> posix_lock_t *tmp =
NULL;</div>
<div> posix_lock_t *lock =
NULL;</div>
<div>+ inode_t *unref = NULL;</div>
<div> </div>
<div> INIT_LIST_HEAD
(&granted_list);</div>
<div> </div>
<div> pthread_mutex_lock
(&pl_inode->mutex);</div>
<div> {</div>
<div>
__grant_blocked_locks (this,
pl_inode, &granted_list);</div>
<div>+</div>
<div>+ if
(__pl_inode_is_empty (pl_inode)
&& pl_inode->refkeeper) {</div>
<div> + unref =
pl_inode->refkeeper;</div>
<div>+
pl_inode->refkeeper = NULL;</div>
<div>+ }</div>
<div> }</div>
<div> pthread_mutex_unlock
(&pl_inode->mutex);</div>
<div> </div>
<div>@@ -965,6 +977,9 @@
grant_blocked_locks (xlator_t *this,
pl_inode_t *pl_inode)</div>
<div> GF_FREE (lock);</div>
<div> }</div>
<div> </div>
<div>+ if (unref)</div>
<div>+ inode_unref
(unref);</div>
<div>+</div>
<div> return;</div>
<div> }</div>
</div>
<div><br>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>
<div><br>
</div>
<div>This should make pl_disconnect_cbk()
pretty much race free w.r.t refkpeer.
Thoughts?</div>
</div>
</div>
</div>
</div>
</blockquote>
</div>
</div>
Lets say C1 is doing pl_inodelk_client_cleanup. After
the second for-loop(All granted and blocked locks are
out of the domain) if an unlock on the final granted
lock on that inode from client C2 completes, refkeeper
would be set to NULL and unrefed leading to zero refs on
that inode i.e. pl_forget will also happen. In 3rd
for-loop pl_inode is already freed and leads to free'd
memory access and will crash.</div>
</blockquote>
<div><br>
</div>
<div><br>
</div>
<div>We also need:</div>
<div><br>
</div>
<pre style="color:rgb(0,0,0)">diff --git a/xlators/features/locks/src/inodelk.c b/xlators/features/locks/src/inodelk.c
index c76cb7f..2aceb8a 100644
--- a/xlators/features/locks/src/inodelk.c
+++ b/xlators/features/locks/src/inodelk.c
@@ -494,13 +494,13 @@ pl_inodelk_client_cleanup (xlator_t *this, pl_ctx_t *ctx)
dom = get_domain (pl_inode, l->volume);
- grant_blocked_inode_locks (this, pl_inode, dom);
-
pthread_mutex_lock (&pl_inode->mutex);
{
__pl_inodelk_unref (l);
}
pthread_mutex_unlock (&pl_inode->mutex);
+
+ grant_blocked_inode_locks (this, pl_inode, dom);
}
</pre>
<div><span style="color:rgb(0,0,0)"> return 0;</span></div>
<div><br>
</div>
<div>Missed this in the last patch. <br>
</div>
</div>
</div>
</div>
</blockquote>
It still doesn't solve the problem I described earlier. By the time
it executes this third loop refkeeper is already unreffed when C2
unlocks.<br>
<br>
Pranith<br>
<br>
</body>
</html>