On Thu, Jun 5, 2014 at 9:54 PM, Pranith Kumar Karampuri <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> 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)
_______________________________________________ Gluster-devel mailing list Gluster-devel@gluster.org http://supercolony.gluster.org/mailman/listinfo/gluster-devel