<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
<div class="moz-cite-prefix">On 06/06/2014 11:38 AM, Pranith Kumar
Karampuri wrote:<br>
</div>
<blockquote cite="mid:53915AD8.60500@redhat.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<br>
<div class="moz-cite-prefix">On 06/06/2014 11:37 AM, Anand Avati
wrote:<br>
</div>
<blockquote
cite="mid:CAFboF2xDvmvPvbfUUajM7L0+8cSJNEziNpyCs2=-7BJYoz1aCA@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 10:56 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:47 AM, Pranith Kumar
Karampuri wrote:<br>
</div>
<blockquote type="cite"> <br>
<div>On 06/06/2014 10:43 AM, Anand Avati wrote:<br>
</div>
<blockquote 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> <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>
</blockquote>
</div>
</div>
Consider the following scenario:<br>
There are two granted locks L1, L2 from C1, C2 clients
respectively on same inode.<br>
C1 gets disconnected.<br>
C2 issues a unlock.<br>
<br>
This is the sequence of steps:<br>
1) C1 executes first loop, increments in_cleanup to 1<br>
2) C2 executes pl_inode_setlk and removed L2 from
granted list. It is now just before
grant_blocked_inode_locks()<br>
3) C1 starts 3rd for loop and unrefs L1, decrements
in_cleanup to 0<br>
4) C2 executes grant_blocked_inode_locks() and
decrements the refkeepr, sets it to NULL and unwinds.
This destroys the inode so pl_inode is freed.<br>
5) C1 calls grant_blocked_inode_locks with pl_inode
which is free'd</div>
</blockquote>
<div><br>
</div>
<div>Yeah, we need a version of
grant_blocked_inode_locks() which decrements in_cleanup
in its locked region.</div>
</div>
</div>
</div>
</blockquote>
I was just thinking the same. I will update you if it works.<br>
</blockquote>
<br>
Avati,<br>
This is the final implementation of the solution kritika and I
decided upon. No double locks are needed. Most of the code is
comments :-). I guess the patch is less than 50 lines. Let us know
your inputs.<br>
<br>
<a class="moz-txt-link-freetext" href="http://review.gluster.com/7981">http://review.gluster.com/7981</a><br>
<br>
Pranith<br>
<blockquote cite="mid:53915AD8.60500@redhat.com" type="cite"> <br>
Pranith<br>
<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
Gluster-devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:Gluster-devel@gluster.org">Gluster-devel@gluster.org</a>
<a class="moz-txt-link-freetext" href="http://supercolony.gluster.org/mailman/listinfo/gluster-devel">http://supercolony.gluster.org/mailman/listinfo/gluster-devel</a>
</pre>
</blockquote>
<br>
</body>
</html>