[Gluster-devel] crypt xlator bug

[Raghavendra Bhat]

On Thursday 02 April 2015 01:00 PM, Pranith Kumar Karampuri wrote:
>
> On 04/02/2015 12:27 AM, Raghavendra Talur wrote:
>>
>>
>> On Wed, Apr 1, 2015 at 10:34 PM, Justin Clift <justin at gluster.org 
>> <mailto:justin at gluster.org>> wrote:
>>
>>     On 1 Apr 2015, at 10:57, Emmanuel Dreyfus <manu at netbsd.org
>>     <mailto:manu at netbsd.org>> wrote:
>>     > Hi
>>     >
>>     > crypt.t was recently broken in NetBSD regression. The glusterfs
>>     returns
>>     > a node with file type invalid to FUSE, and that breaks the test.
>>     >
>>     > After running a git bisect, I found the offending commit after
>>     which
>>     > this behavior appeared:
>>     >    8a2e2b88fc21dc7879f838d18cd0413dd88023b7
>>     >    mem-pool: invalidate memory on GF_FREE to aid debugging
>>     >
>>     > This means the bug has always been there, but this debugging aid
>>     > caused it to be reliable.
>>
>>     Sounds like that commit is a good win then. :)
>>
>>     Harsha/Pranith/Lala, your names are on the git blame for crypt.c...
>>     any ideas? :)
>>
>>
>> I found one issue that local is not allocated using GF_CALLOC and 
>> with a mem-type.
>> This is a patch which *might* fix it.
>>
>> diff --git a/xlators/encryption/crypt/src/crypt-mem-types.h 
>> b/xlators/encryption/crypt/src/crypt-mem-types.h
>> index 2eab921..c417b67 100644
>> --- a/xlators/encryption/crypt/src/crypt-mem-types.h
>> +++ b/xlators/encryption/crypt/src/crypt-mem-types.h
>> @@ -24,6 +24,7 @@ enum gf_crypt_mem_types_ {
>>         gf_crypt_mt_key,
>>         gf_crypt_mt_iovec,
>>         gf_crypt_mt_char,
>> +        gf_crypt_mt_local,
>>         gf_crypt_mt_end,
>>  };
>> diff --git a/xlators/encryption/crypt/src/crypt.c 
>> b/xlators/encryption/crypt/src/crypt.c
>> index ae8cdb2..63c0977 100644
>> --- a/xlators/encryption/crypt/src/crypt.c
>> +++ b/xlators/encryption/crypt/src/crypt.c
>> @@ -48,7 +48,7 @@ static crypt_local_t 
>> *crypt_alloc_local(call_frame_t *frame, xlator_t *this,
>>  {
>>         crypt_local_t *local = NULL;
>> -       local = mem_get0(this->local_pool);
>> +        local = GF_CALLOC (sizeof (*local), 1, gf_crypt_mt_local);
> local is using the memory from pool earlier(i.e. with mem_get0()). 
> Which seems ok to me. Changing it this way will include memory 
> allocation in fop I/O path which is why xlators generally use the 
> mem-pool approach.
>
> Pranith

I think, crypt xlator should do a mem_put of local after doing 
STACK_UNWIND like other xlators which also use mem_get for local (such 
as AFR). I am suspecting crypt not doing mem_put might be the reason for 
the bug mentioned.

Regards,
Raghavendra Bat

>>         if (!local) {
>>                 gf_log(this->name, GF_LOG_ERROR, "out of memory");
>>                 return NULL;
>>
>>
>> Niels should be able to recognize if this is sufficient fix or not.
>>
>> Thanks,
>> Raghavendra Talur
>>
>>     + Justin
>>
>>     --
>>     GlusterFS - http://www.gluster.org
>>
>>     An open source, distributed file system scaling to several
>>     petabytes, and handling thousands of clients.
>>
>>     My personal twitter: twitter.com/realjustinclift
>>     <http://twitter.com/realjustinclift>
>>
>>     _______________________________________________
>>     Gluster-devel mailing list
>>     Gluster-devel at gluster.org <mailto:Gluster-devel at gluster.org>
>>     http://www.gluster.org/mailman/listinfo/gluster-devel
>>
>>
>>
>>
>> -- 
>> *Raghavendra Talur *
>>
>
>
>
> _______________________________________________
> Gluster-devel mailing list
> Gluster-devel at gluster.org
> http://www.gluster.org/mailman/listinfo/gluster-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.gluster.org/pipermail/gluster-devel/attachments/20150402/3e756f0e/attachment.html>