On 09/18/2014 10:08 AM, Raghavendra Gowdappa wrote:
For eg., if a dictionary is not freed because of non-zero refcount, if there is an information on who has held these references would help to narrow down the code path or component.
Yes that is the aim. The implementation I suggested tries to get that
information per xlator. Are you saying it is better to store in the
dict/inode/fd itself? I actually wrote one patch sometime back to do
something similar (Not tested yet :-) ).
diff --git a/libglusterfs/src/dict.c b/libglusterfs/src/dict.c
index 37a6b2c..bd4c438 100644
--- a/libglusterfs/src/dict.c
+++ b/libglusterfs/src/dict.c
@@ -100,8 +100,10 @@ dict_new (void)
dict = get_new_dict_full(1);
- if (dict)
+ if (dict) {
+ dict->refs = get_new_dict_full(1);
dict_ref (dict);
+ }
return dict;
}
@@ -446,6 +448,7 @@ dict_destroy (dict_t *this)
data_pair_t *pair = this->members_list;
data_pair_t *prev = this->members_list;
+ dict_destroy (this->refs);
LOCK_DESTROY (&this->lock);
while (prev) {
@@ -495,15 +498,21 @@ dict_unref (dict_t *this)
dict_t *
dict_ref (dict_t *this)
{
+ int32_t ref = 0;
+ xlat_t *x = NULL;
if (!this) {
gf_log_callingfn ("dict", GF_LOG_WARNING, "dict is NULL");
return NULL;
}
+ x = THIS;
LOCK (&this->lock);
this->refcount++;
+ dict_get_in32 (this->refs, x->name, &ref);
+ dict_set_int32 (this->refs, x->name, ref+1);
+
UNLOCK (&this->lock);
return this;
@@ -513,15 +522,20 @@ void
data_unref (data_t *this)
{
int32_t ref;
+ xlator_t *x = NULL;
if (!this) {
gf_log_callingfn ("dict", GF_LOG_WARNING, "dict is NULL");
return;
}
+ x = THIS;
LOCK (&this->lock);
this->refcount--;
+ dict_get_in32 (this->refs, x->name, &ref);
+ dict_set_int32 (this->refs, x->name, ref-1);
+
ref = this->refcount;
UNLOCK (&this->lock);
diff --git a/libglusterfs/src/dict.h b/libglusterfs/src/dict.h
index 682c152..33ed7bd 100644
--- a/libglusterfs/src/dict.h
+++ b/libglusterfs/src/dict.h
@@ -93,6 +93,7 @@ struct _dict {
data_pair_t *members_internal;
data_pair_t free_pair;
gf_boolean_t free_pair_in_use;
+ struct _dict *refs;
};
I was not happy with this implementation either.
similar implementation for inode here: http://review.gluster.com/8302
But I am not happy with any of these implementations. Probably because
it is still not granular enough, i.e. fop info is missing. It is better
than no info but still bad. We can't have it on statedump either. So
when User reports high memory usage we still have to spend lot of time
looking over all the places where the dicts are allocated which is bad :-(.
Pranith
----- Original Message -----
From: "Pranith Kumar Karampuri" <pkarampu@xxxxxxxxxx>
To: "Raghavendra Gowdappa" <rgowdapp@xxxxxxxxxx>
Cc: "Gluster Devel" <gluster-devel@xxxxxxxxxxx>
Sent: Thursday, September 18, 2014 10:05:18 AM
Subject: Re: how do you debug ref leaks?
On 09/18/2014 09:59 AM, Raghavendra Gowdappa wrote:
One thing that would be helpful is "allocator" info for generic objects
like dict, inode, fd etc. That way we wouldn't have to sift through large
amount of code.
Could you elaborate the idea please.
Pranith
----- Original Message -----
From: "Pranith Kumar Karampuri" <pkarampu@xxxxxxxxxx>
To: "Gluster Devel" <gluster-devel@xxxxxxxxxxx>
Sent: Thursday, September 18, 2014 7:43:00 AM
Subject: how do you debug ref leaks?
hi,
Till now the only method I used to find ref leaks effectively is to
find what operation is causing ref leaks and read the code to find if
there is a ref-leak somewhere. Valgrind doesn't solve this problem
because it is reachable memory from inode-table etc. I am just wondering
if there is an effective way anyone else knows of. Do you guys think we
need a better mechanism of finding refleaks? At least which decreases
the search space significantly i.e. xlator y, fop f etc? It would be
better if we can come up with ways to integrate statedump and this infra
just like we did for mem-accounting.
One way I thought was to introduce new apis called
xl_fop_dict/inode/fd_ref/unref (). Each xl keeps an array of num_fops
per inode/dict/fd and increments/decrements accordingly. Dump this info
on statedump.
I myself am not completely sure about this idea. It requires all xlators
to change.
Any ideas?
Pranith
_______________________________________________
Gluster-devel mailing list
Gluster-devel@xxxxxxxxxxx
http://supercolony.gluster.org/mailman/listinfo/gluster-devel
_______________________________________________
Gluster-devel mailing list
Gluster-devel@xxxxxxxxxxx
http://supercolony.gluster.org/mailman/listinfo/gluster-devel
