Bug 1136831 - [AFR-V2] - dict_t leaks
Summary: [AFR-V2] - dict_t leaks
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: GlusterFS
Classification: Community
Component: replicate
Version: 3.6.0
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Krutika Dhananjay
QA Contact:
URL:
Whiteboard:
Depends On: 1134221
Blocks: glusterfs-3.6.0
TreeView+ depends on / blocked
 
Reported: 2014-09-03 12:01 UTC by Krutika Dhananjay
Modified: 2014-11-11 08:37 UTC (History)
2 users (show)

Fixed In Version: glusterfs-3.6.0beta1
Clone Of: 1134221
Environment:
Last Closed: 2014-11-11 08:37:57 UTC
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Embargoed:


Attachments (Terms of Use)

Description Krutika Dhananjay 2014-09-03 12:01:47 UTC
+++ This bug was initially created as a clone of Bug #1134221 +++

Description of problem:

While reading code, I figured dict_t objects that are ref'd by 'replies' objects that are allocated via alloca(), are not unref'd when 'replies' goes out of scope. (see Additional info section for more info)


Version-Release number of selected component (if applicable):


How reproducible:
always

Steps to Reproduce:
1.
2.
3.

Actual results:
This leads to dict_t leaks.

Expected results:
There should be no leaks.

Additional info:

  8 afr_selfheal_name_unlocked_inspect (call_frame_t *frame, xlator_t *this,         
  7                                     inode_t *parent, uuid_t pargfid,             
  6                                     const char *bname, gf_boolean_t *need_heal)  
  5 {                                                                                
  4         afr_private_t *priv = NULL;                                              
  3         int i = 0;                                                               
  2         struct afr_reply *replies = NULL;                                        
  1         inode_t *inode = NULL;                                                   
  0         int first_idx = -1;                                                                                                                                                                                                                                                 
  1                                                                                  
  2         priv = this->private;                                                    
  3                                                                                  
  4         replies = alloca0 (sizeof (*replies) * priv->child_count);             <---- replies allocated via alloca()  
  5                                                                                  
  6         inode = afr_selfheal_unlocked_lookup_on (frame, parent, bname,          ****function of interest
  7                                                  replies, priv->child_up);       
  8         if (!inode)                                                              
  9                 return -ENOMEM;                                                  
 10                                                                                  
 11         for (i = 0; i < priv->child_count; i++) {                                
 12                 if (!replies[i].valid)                                           
 13                         continue;                                                
 14                                                                                  
 15                 if ((replies[i].op_ret == -1) &&                                 
 16                     (replies[i].op_errno == ENODATA))                            
 17                         *need_heal = _gf_true;                                   
 18                                                                                  
 19                 if (first_idx == -1) {                                           
 20                         first_idx = i;                                           
 21                         continue;                                                
 22                 }                                                                
 23                                                                                  
 24                 if (replies[i].op_ret != replies[first_idx].op_ret)              
 25                         *need_heal = _gf_true;                                   
 26                                                                                  
 27                 if (uuid_compare (replies[i].poststat.ia_gfid,                   
 28                                   replies[first_idx].poststat.ia_gfid))          
 29                         *need_heal = _gf_true;                                   
 30         }                                                                        
 31                                                                                  
 32         if (inode)                                                               
 33                 inode_unref (inode);                                             
 34         return 0;                                                                
 35 }              

****
 28 inode_t *                                                                        
 27 afr_selfheal_unlocked_lookup_on (call_frame_t *frame, inode_t *parent,           
 26                                  const char *name, struct afr_reply *replies,    
 25                                  unsigned char *lookup_on)                       
 24 {                                                                                
 23         loc_t loc = {0, };                                                       
 22         dict_t *xattr_req = NULL;                                                
 21         afr_local_t *local = NULL;                                               
 20         afr_private_t *priv = NULL;                                              
 19         inode_t *inode = NULL;                                                   
 18                                                                                  
 17         local = frame->local;                                                    
 16         priv = frame->this->private;                                             
 15                                                                                  
 14         xattr_req = dict_new ();                                                 
 13         if (!xattr_req)                                                          
 12                 return NULL;                                                     
 11                                                                                  
 10         if (afr_xattr_req_prepare (frame->this, xattr_req) != 0) {               
  9                 dict_destroy (xattr_req);                                        
  8                 return NULL;                                                     
  7         }                                                                        
  6                                                                                  
  5         inode = inode_new (parent->table);                                       
  4         if (!inode) {                                                            
  3                 dict_destroy (xattr_req);                                        
  2                 return NULL;                                                     
  1         }                                                                        
  0                                                                                                                                                                                                                                                                             
  1         loc.parent = inode_ref (parent);                                         
  2         uuid_copy (loc.pargfid, parent->gfid);                                   
  3         loc.name = name;                                                         
  4         loc.inode = inode_ref (inode);                                           
  5                                                                                  
  6         AFR_ONLIST (lookup_on, frame, afr_selfheal_discover_cbk, lookup, &loc,   
  7                     xattr_req);                                                  
  8                                                                                  
  9         afr_replies_copy (replies, local->replies, priv->child_count);           <--- response dicts are ref'd here into replies[i].xdata but never unrefd even after replies goes out of scope in afr_selfheal_name_unlocked_inspect()
 10                                                                                  
 11         loc_wipe (&loc);                                                         
 12         dict_unref (xattr_req);                                                  
 13                                                                                  
 14         return inode;                                                            
 15 }

--- Additional comment from Anand Avati on 2014-08-27 06:09:29 EDT ---

REVIEW: http://review.gluster.org/8553 (cluster/afr: Fix dict_t leaks) posted (#1) for review on master by Krutika Dhananjay (kdhananj)

--- Additional comment from Anand Avati on 2014-08-28 02:12:47 EDT ---

REVIEW: http://review.gluster.org/8557 (statedump: Print curr_stdalloc in mempool statedump ...) posted (#1) for review on master by Krutika Dhananjay (kdhananj)

--- Additional comment from Anand Avati on 2014-08-28 02:31:46 EDT ---

REVIEW: http://review.gluster.org/8553 (cluster/afr: Fix dict_t leaks) posted (#2) for review on master by Krutika Dhananjay (kdhananj)

--- Additional comment from Anand Avati on 2014-08-28 03:55:36 EDT ---

REVIEW: http://review.gluster.org/8557 (statedump: Print curr_stdalloc in mempool statedump ...) posted (#2) for review on master by Krutika Dhananjay (kdhananj)

--- Additional comment from Anand Avati on 2014-08-28 04:19:52 EDT ---

COMMIT: http://review.gluster.org/8553 committed in master by Pranith Kumar Karampuri (pkarampu) 
------
commit dc844c545caa7f2cf08fd71caa5051348a5f3c78
Author: Krutika Dhananjay <kdhananj>
Date:   Wed Aug 27 15:14:04 2014 +0530

    cluster/afr: Fix dict_t leaks
    
    dict_t objects that are ref'd in alloca'd "replies" in
    afr_replies_copy() are not unref'd after "replies" go out of scope.
    
    Change-Id: Id5a6ca3c17a8de72b94b3e0f92165609da5a36ea
    BUG: 1134221
    Signed-off-by: Krutika Dhananjay <kdhananj>
    Reviewed-on: http://review.gluster.org/8553
    Reviewed-by: Pranith Kumar Karampuri <pkarampu>
    Tested-by: Pranith Kumar Karampuri <pkarampu>
    Tested-by: Gluster Build System <jenkins.com>

--- Additional comment from Anand Avati on 2014-08-29 22:31:54 EDT ---

COMMIT: http://review.gluster.org/8557 committed in master by Pranith Kumar Karampuri (pkarampu) 
------
commit f52efa681b1a16c287ed00e2a79cc7f05e65fed1
Author: Krutika Dhananjay <kdhananj>
Date:   Thu Aug 28 09:58:43 2014 +0530

    statedump: Print curr_stdalloc in mempool statedump ...
    
     ...for, it is curr_stdalloc that is incremented for every mem_get()
    and decremented on every call to mem_put() and can be used to detect
    leaks, when cold_count is 0.
    
    Change-Id: I418a132c3ea4c0b99ea5c6840ff3024d8d19ddf4
    BUG: 1134221
    Signed-off-by: Krutika Dhananjay <kdhananj>
    Reviewed-on: http://review.gluster.org/8557
    Tested-by: Gluster Build System <jenkins.com>
    Reviewed-by: Humble Devassy Chirammal <humble.devassy>
    Reviewed-by: Pranith Kumar Karampuri <pkarampu>
    Tested-by: Pranith Kumar Karampuri <pkarampu>

Comment 1 Anand Avati 2014-09-12 10:36:01 UTC
REVIEW: http://review.gluster.org/8704 (cluster/afr: Fix dict_t leaks) posted (#1) for review on release-3.6 by Krutika Dhananjay (kdhananj)

Comment 2 Anand Avati 2014-09-12 10:36:05 UTC
REVIEW: http://review.gluster.org/8705 (statedump: Print curr_stdalloc in mempool statedump ...) posted (#1) for review on release-3.6 by Krutika Dhananjay (kdhananj)

Comment 3 Anand Avati 2014-09-16 11:14:41 UTC
COMMIT: http://review.gluster.org/8704 committed in release-3.6 by Vijay Bellur (vbellur) 
------
commit 1343efc2b5fc65b84afabb51ac537537ccb27722
Author: Krutika Dhananjay <kdhananj>
Date:   Wed Aug 27 15:14:04 2014 +0530

    cluster/afr: Fix dict_t leaks
    
            Backport of: http://review.gluster.org/#/c/8557/
    
    dict_t objects that are ref'd in alloca'd "replies" in
    afr_replies_copy() are not unref'd before "replies" go out of scope.
    
    Change-Id: I9bb45bc673ec13292ac96dda060aceb48739ebe8
    BUG: 1136831
    Signed-off-by: Krutika Dhananjay <kdhananj>
    Reviewed-on: http://review.gluster.org/8704
    Tested-by: Gluster Build System <jenkins.com>
    Reviewed-by: Pranith Kumar Karampuri <pkarampu>
    Reviewed-by: Vijay Bellur <vbellur>

Comment 4 Anand Avati 2014-09-16 11:15:28 UTC
COMMIT: http://review.gluster.org/8705 committed in release-3.6 by Vijay Bellur (vbellur) 
------
commit cd5a5d32996d2f1141adac94816ad2292a693ebf
Author: Krutika Dhananjay <kdhananj>
Date:   Fri Sep 12 16:01:57 2014 +0530

    statedump: Print curr_stdalloc in mempool statedump ...
    
    ...for, it is curr_stdalloc that is incremented for every mem_get()
    and decremented on every call to mem_put() and can be used to detect
    leaks, when cold_count is 0.
    
            Backport of: http://review.gluster.org/8557
    
    Change-Id: I1f4aac3af1234b9a76be7cb86ef26d728788950c
    BUG: 1136831
    Signed-off-by: Krutika Dhananjay <kdhananj>
    Reviewed-on: http://review.gluster.org/8705
    Tested-by: Gluster Build System <jenkins.com>
    Reviewed-by: Pranith Kumar Karampuri <pkarampu>
    Reviewed-by: Vijay Bellur <vbellur>

Comment 5 Anand Avati 2014-09-19 07:26:27 UTC
REVIEW: http://review.gluster.org/8775 (statedump: Print curr_stdalloc in mempool statedump ...) posted (#1) for review on release-3.5 by Krutika Dhananjay (kdhananj)

Comment 6 Niels de Vos 2014-09-22 12:45:33 UTC
A beta release for GlusterFS 3.6.0 has been released. Please verify if the release solves this bug report for you. In case the glusterfs-3.6.0beta1 release does not have a resolution for this issue, leave a comment in this bug and move the status to ASSIGNED. If this release fixes the problem for you, leave a note and change the status to VERIFIED.

Packages for several distributions should become available in the near future. Keep an eye on the Gluster Users mailinglist [2] and the update (possibly an "updates-testing" repository) infrastructure for your distribution.

[1] http://supercolony.gluster.org/pipermail/gluster-users/2014-September/018836.html
[2] http://supercolony.gluster.org/pipermail/gluster-users/

Comment 7 Anand Avati 2014-09-26 10:17:38 UTC
REVIEW: http://review.gluster.org/8869 (cluster/afr: More dict_t leak fixes) posted (#1) for review on release-3.6 by Krutika Dhananjay (kdhananj)

Comment 8 Anand Avati 2014-09-26 12:23:22 UTC
COMMIT: http://review.gluster.org/8869 committed in release-3.6 by Vijay Bellur (vbellur) 
------
commit b32224c8a5caff175a83e6089cf2cbf90253a2d7
Author: Krutika Dhananjay <kdhananj>
Date:   Thu Sep 25 16:27:52 2014 +0530

    cluster/afr: More dict_t leak fixes
    
            Backport of: http://review.gluster.org/8852
    
    Change-Id: I2c927092dc5a834fabdd3495a7f7a3527604a6d2
    BUG: 1136831
    Signed-off-by: Krutika Dhananjay <kdhananj>
    Reviewed-on: http://review.gluster.org/8869
    Reviewed-by: Pranith Kumar Karampuri <pkarampu>
    Tested-by: Gluster Build System <jenkins.com>
    Reviewed-by: Vijay Bellur <vbellur>

Comment 9 Niels de Vos 2014-11-11 08:37:57 UTC
This bug is getting closed because a release has been made available that should address the reported issue. In case the problem is still not fixed with glusterfs-3.6.1, please reopen this bug report.

glusterfs-3.6.1 has been announced [1], packages for several distributions should become available in the near future. Keep an eye on the Gluster Users mailinglist [2] and the update infrastructure for your distribution.

[1] http://supercolony.gluster.org/pipermail/gluster-users/2014-November/019410.html
[2] http://supercolony.gluster.org/mailman/listinfo/gluster-users


Note You need to log in before you can comment on or make changes to this bug.