Re: crypt xlator bug

[Raghavendra Bhat <rabhat@xxxxxxxxxx>]

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@xxxxxxxxxxx> wrote:

On 1 Apr 2015, at 10:57, Emmanuel Dreyfus <manu@xxxxxxxxxx> 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

_______________________________________________
Gluster-devel mailing list
Gluster-devel@xxxxxxxxxxx
http://www.gluster.org/mailman/listinfo/gluster-devel

--

Raghavendra Talur 

_______________________________________________
Gluster-devel mailing list
Gluster-devel@xxxxxxxxxxx
http://www.gluster.org/mailman/listinfo/gluster-devel

_______________________________________________
Gluster-devel mailing list
Gluster-devel@xxxxxxxxxxx
http://www.gluster.org/mailman/listinfo/gluster-devel