On 06/06/2014 11:38 AM, Pranith Kumar Karampuri wrote:
On 06/06/2014 11:37 AM, Anand Avati wrote:
On Thu, Jun 5, 2014 at 10:56 PM, Pranith Kumar Karampuri
<pkara...@redhat.com <mailto:pkara...@redhat.com>> wrote:
On 06/06/2014 10:47 AM, Pranith Kumar Karampuri wrote:
On 06/06/2014 10:43 AM, Anand Avati wrote:
On Thu, Jun 5, 2014 at 9:54 PM, Pranith Kumar Karampuri
<pkara...@redhat.com <mailto:pkara...@redhat.com>> wrote:
On 06/06/2014 10:02 AM, Anand Avati wrote:
On Thu, Jun 5, 2014 at 7:52 PM, Pranith Kumar Karampuri
<pkara...@redhat.com <mailto:pkara...@redhat.com>> wrote:
This sounds a bit complicated. I think there is a
much simpler solution:
- First, make update_refkeeper() check for blocked
locks (which I mentioned as "optional" previously)
- Make grant_blocked_locks() double up and do the job
of update_refkeeper() internally.
Something which looks like this:
diff --git a/xlators/features/locks/src/common.c
b/xlators/features/locks/src/common.c
index f6c71c1..38df385 100644
--- a/xlators/features/locks/src/common.c
+++ b/xlators/features/locks/src/common.c
@@ -126,8 +126,14 @@ __pl_inode_is_empty (pl_inode_t
*pl_inode)
if (!list_empty (&dom->entrylk_list))
is_empty = 0;
+ if (!list_empty (&dom->blocked_entrylks))
+ is_empty = 0;
+
if (!list_empty (&dom->inodelk_list))
is_empty = 0;
+
+ if (!list_empty (&dom->blocked_inodelks))
+ is_empty = 0;
}
return is_empty;
@@ -944,12 +950,18 @@ grant_blocked_locks (xlator_t
*this, pl_inode_t *pl_inode)
struct list_head granted_list;
posix_lock_t *tmp = NULL;
posix_lock_t *lock = NULL;
+ inode_t *unref = NULL;
INIT_LIST_HEAD (&granted_list);
pthread_mutex_lock (&pl_inode->mutex);
{
__grant_blocked_locks (this, pl_inode, &granted_list);
+
+ if (__pl_inode_is_empty (pl_inode) &&
pl_inode->refkeeper) {
+ unref = pl_inode->refkeeper;
+ pl_inode->refkeeper = NULL;
+ }
}
pthread_mutex_unlock (&pl_inode->mutex);
@@ -965,6 +977,9 @@ grant_blocked_locks (xlator_t
*this, pl_inode_t *pl_inode)
GF_FREE (lock);
}
+ if (unref)
+ inode_unref (unref);
+
return;
}
This should make pl_disconnect_cbk() pretty much race
free w.r.t refkpeer. Thoughts?
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.
We also need:
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);
}
return 0;
Missed this in the last patch.
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.
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)
s/first lookup/first loop/ ?
Consider the following scenario:
There are two granted locks L1, L2 from C1, C2 clients
respectively on same inode.
C1 gets disconnected.
C2 issues a unlock.
This is the sequence of steps:
1) C1 executes first loop, increments in_cleanup to 1
2) C2 executes pl_inode_setlk and removed L2 from granted list.
It is now just before grant_blocked_inode_locks()
3) C1 starts 3rd for loop and unrefs L1, decrements in_cleanup to 0
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.
5) C1 calls grant_blocked_inode_locks with pl_inode which is free'd
Yeah, we need a version of grant_blocked_inode_locks() which
decrements in_cleanup in its locked region.
I was just thinking the same. I will update you if it works.
Avati,
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.
http://review.gluster.com/7981
Pranith
Pranith
_______________________________________________
Gluster-devel mailing list
Gluster-devel@gluster.org
http://supercolony.gluster.org/mailman/listinfo/gluster-devel
_______________________________________________
Gluster-devel mailing list
Gluster-devel@gluster.org
http://supercolony.gluster.org/mailman/listinfo/gluster-devel
