[Gluster-devel] Regarding doing away with refkeeper in locks xlator

Pranith Kumar Karampuri pkarampu at redhat.com
Sun Jun 8 17:42:00 UTC 2014


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 
>> <pkarampu at redhat.com <mailto:pkarampu at 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
>>>>     <pkarampu at redhat.com <mailto:pkarampu at 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
>>>>>         <pkarampu at redhat.com <mailto:pkarampu at 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 at gluster.org
> http://supercolony.gluster.org/mailman/listinfo/gluster-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://supercolony.gluster.org/pipermail/gluster-devel/attachments/20140608/85227277/attachment-0001.html>


More information about the Gluster-devel mailing list