<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:43 AM, Anand Avati
wrote:<br>
</div>
<blockquote
cite="mid:CAFboF2w0gDCJsuSVMqcR022GvAzDB0ndkYYfjxqunjY6BkEB2A@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 9:54 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:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<div>
<div class="h5"> <br>
<div>On 06/06/2014 10:02 AM, Anand Avati wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<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>
<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>
</div>
</div>
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.</div>
</blockquote>
<div><br>
</div>
<div><br>
</div>
<div>Right, we will need to introduce an "in_cleanup"
counter, if set pl_update_refkeeper() should not unref.
Increment the in_cleanup() in the first lookup, and
decrement it in the last loop, just before calling
grant_blocked_locks() (along with the patches in my last 2
mails) <br>
</div>
</div>
</div>
</div>
</blockquote>
s/first lookup/first loop/ ?<br>
<br>
Pranith<br>
<br>
</body>
</html>