From 9f819fe340a346844b75cde8a676470bd8b1b907 Mon Sep 17 00:00:00 2001

From: vmallika <vmallika@redhat.com>

Date: Wed, 24 Jun 2015 11:56:30 +0530

Subject: [PATCH 145/190] quota/marker: fix mem-leak in marker

This is a backport of http://review.gluster.org/#/c/11361/

> When removing contribution xattr, we also need to free

> contribution node in memory.

> Use ref/unref mechanism to handle contribution node memory

>

> local->xdata should be freed in mq_local_unref

>

> There is another huge memory consumption happens

> in function mq_inspect_directory_xattr_task

> where dirty flag is not set.

>

> Change-Id: Ieca3ab4bf410c51259560e778bce4e81b9d888bf

> BUG: 1207735

> Signed-off-by: vmallika <vmallika@redhat.com>

> Reviewed-on: http://review.gluster.org/11361

> Reviewed-by: Krishnan Parthasarathi <kparthas@redhat.com>

> Tested-by: NetBSD Build System <jenkins@build.gluster.org>

> Reviewed-by: Raghavendra G <rgowdapp@redhat.com>

> Tested-by: Raghavendra G <rgowdapp@redhat.com>

Change-Id: Id82bd071433f8141c6fa1d4ad1a488604ff33978

BUG: 1224177

Signed-off-by: vmallika <vmallika@redhat.com>

Reviewed-on: https://code.engineering.redhat.com/gerrit/51565

Reviewed-by: Raghavendra Gowdappa <rgowdapp@redhat.com>

Tested-by: Raghavendra Gowdappa <rgowdapp@redhat.com>

---

 xlators/features/marker/src/marker-quota-helper.c |   81 ++++++++----

 xlators/features/marker/src/marker-quota-helper.h |   18 ++-

 xlators/features/marker/src/marker-quota.c        |  137 +++++++++++++--------

 xlators/features/marker/src/marker-quota.h        |    4 +-

 4 files changed, 157 insertions(+), 83 deletions(-)

diff --git a/xlators/features/marker/src/marker-quota-helper.c b/xlators/features/marker/src/marker-quota-helper.c

index 67801c8..a103bb9 100644

--- a/xlators/features/marker/src/marker-quota-helper.c

+++ b/xlators/features/marker/src/marker-quota-helper.c

@@ -130,6 +130,39 @@ out:

         return ctx;

 }

+void

+mq_contri_fini (void *data)

+{

+        inode_contribution_t *contri = data;

+

+        LOCK_DESTROY (&contri->lock);

+        GF_FREE (contri);

+}

+

+inode_contribution_t*

+mq_contri_init (inode_t *inode)

+{

+        inode_contribution_t *contri   = NULL;

+        int32_t               ret      = 0;

+

+        QUOTA_ALLOC (contri, inode_contribution_t, ret);

+        if (ret == -1)

+                goto out;

+

+        GF_REF_INIT (contri, mq_contri_fini);

+

+        contri->contribution = 0;

+        contri->file_count = 0;

+        contri->dir_count = 0;

+        gf_uuid_copy (contri->gfid, inode->gfid);

+

+        LOCK_INIT (&contri->lock);

+        INIT_LIST_HEAD (&contri->contri_list);

+

+out:

+        return contri;

+}

+

 inode_contribution_t *

 mq_get_contribution_node (inode_t *inode, quota_inode_ctx_t *ctx)

 {

@@ -139,35 +172,26 @@ mq_get_contribution_node (inode_t *inode, quota_inode_ctx_t *ctx)

         if (!inode || !ctx)

                 goto out;

-        list_for_each_entry (temp, &ctx->contribution_head, contri_list) {

-                if (gf_uuid_compare (temp->gfid, inode->gfid) == 0) {

-                        contri = temp;

-                        goto out;

+        LOCK (&ctx->lock);

+        {

+                list_for_each_entry (temp, &ctx->contribution_head,

+                                     contri_list) {

+                        if (gf_uuid_compare (temp->gfid, inode->gfid) == 0) {

+                                contri = temp;

+                                GF_REF_GET (contri);

+                                break;

+                        }

                 }

         }

+        UNLOCK (&ctx->lock);

 out:

         return contri;

 }

-

-int32_t

-mq_delete_contribution_node (dict_t *dict, char *key,

-                             inode_contribution_t *contribution)

-{

-        if (dict_get (dict, key) != NULL)

-                goto out;

-

-        QUOTA_FREE_CONTRIBUTION_NODE (contribution);

-out:

-        return 0;

-}

-

-

 inode_contribution_t *

 __mq_add_new_contribution_node (xlator_t *this, quota_inode_ctx_t *ctx,

                                 loc_t *loc)

 {

-        int32_t               ret          = 0;

         inode_contribution_t *contribution = NULL;

         if (!loc->parent) {

@@ -190,17 +214,10 @@ __mq_add_new_contribution_node (xlator_t *this, quota_inode_ctx_t *ctx,

                 }

         }

-        QUOTA_ALLOC (contribution, inode_contribution_t, ret);

-        if (ret == -1)

+        contribution = mq_contri_init (loc->parent);

+        if (contribution == NULL)

                 goto out;

-        contribution->contribution = 0;

-

-        gf_uuid_copy (contribution->gfid, loc->parent->gfid);

-

-	LOCK_INIT (&contribution->lock);

-        INIT_LIST_HEAD (&contribution->contri_list);

-

         list_add_tail (&contribution->contri_list, &ctx->contribution_head);

 out:

@@ -224,6 +241,8 @@ mq_add_new_contribution_node (xlator_t *this, quota_inode_ctx_t *ctx,

         LOCK (&ctx->lock);

         {

                 contribution = __mq_add_new_contribution_node (this, ctx, loc);

+                if (contribution)

+                        GF_REF_GET (contribution);

         }

         UNLOCK (&ctx->lock);

@@ -392,6 +411,12 @@ mq_local_unref (xlator_t *this, quota_local_t *local)

         if (local->fd != NULL)

                 fd_unref (local->fd);

+        if (local->contri)

+                GF_REF_PUT (local->contri);

+

+        if (local->xdata)

+                dict_unref (local->xdata);

+

         loc_wipe (&local->loc);

         loc_wipe (&local->parent_loc);

diff --git a/xlators/features/marker/src/marker-quota-helper.h b/xlators/features/marker/src/marker-quota-helper.h

index 161413d..f69447b 100644

--- a/xlators/features/marker/src/marker-quota-helper.h

+++ b/xlators/features/marker/src/marker-quota-helper.h

@@ -18,10 +18,14 @@

 #include "marker.h"

-#define QUOTA_FREE_CONTRIBUTION_NODE(_contribution)     \

-        do {                                            \

-                list_del (&_contribution->contri_list); \

-                GF_FREE (_contribution);                \

+#define QUOTA_FREE_CONTRIBUTION_NODE(ctx, _contribution)          \

+        do {                                                      \

+                LOCK (&ctx->lock);                                \

+                {                                                 \

+                        list_del (&_contribution->contri_list);   \

+                        GF_REF_PUT (_contribution);               \

+                }                                                 \

+                UNLOCK (&ctx->lock);                              \

         } while (0)

 #define QUOTA_SAFE_INCREMENT(lock, var)                 \

@@ -67,6 +71,12 @@ mq_local_ref (quota_local_t *);

 int32_t

 mq_local_unref (xlator_t *, quota_local_t *);

+void

+mq_contri_fini (void *data);

+

+inode_contribution_t*

+mq_contri_init (inode_t *inode);

+

 inode_contribution_t *

 mq_get_contribution_node (inode_t *, quota_inode_ctx_t *);

diff --git a/xlators/features/marker/src/marker-quota.c b/xlators/features/marker/src/marker-quota.c

index e183829..bbe2d9d 100644

--- a/xlators/features/marker/src/marker-quota.c

+++ b/xlators/features/marker/src/marker-quota.c

@@ -887,7 +887,10 @@ mq_update_dirty_inode (xlator_t *this, loc_t *loc, quota_inode_ctx_t *ctx,

         local->ctx = ctx;

-        local->contri = contribution;

+        if (contribution) {

+                local->contri = contribution;

+                GF_REF_GET (local->contri);

+        }

         lock.l_type = F_WRLCK;

         lock.l_whence = SEEK_SET;

@@ -1094,6 +1097,9 @@ free_value:

 err:

         dict_unref (dict);

+        if (contri)

+                GF_REF_PUT (contri);

+

 out:

         if (ret < 0) {

                 mq_xattr_creation_release_lock (frame, NULL, this, 0, 0, NULL);

@@ -1267,7 +1273,6 @@ mq_get_parent_inode_local (xlator_t *this, quota_local_t *local)

 {

         int32_t            ret = -1;

         quota_inode_ctx_t *ctx = NULL;

-        inode_contribution_t *contribution = NULL;

         GF_VALIDATE_OR_GOTO ("marker", this, out);

         GF_VALIDATE_OR_GOTO ("marker", local, out);

@@ -1332,9 +1337,8 @@ mq_get_parent_inode_local (xlator_t *this, quota_local_t *local)

            contribution will be at the end of the list. So get the proper

            parent's contribution, by searching the entire list.

         */

-        contribution = mq_get_contribution_node (local->loc.parent, ctx);

-        GF_ASSERT (contribution != NULL);

-        local->contri = contribution;

+        local->contri = mq_get_contribution_node (local->loc.parent, ctx);

+        GF_ASSERT (local->contri != NULL);

         ret = 0;

 out:

@@ -1997,7 +2001,10 @@ mq_prepare_txn_frame (xlator_t *this, loc_t *loc,

                 goto fr_destroy;

         local->ctx = ctx;

-        local->contri = contri;

+        if (contri) {

+                local->contri = contri;

+                GF_REF_GET (local->contri);

+        }

         ret = 0;

         *new_frame = frame;

@@ -2247,6 +2254,9 @@ out:

         if (dict)

                 dict_unref (dict);

+        if (contribution)

+                GF_REF_PUT (contribution);

+

         return ret;

 }

@@ -2574,7 +2584,8 @@ out:

 }

 int32_t

-mq_remove_contri (xlator_t *this, loc_t *loc, inode_contribution_t *contri)

+mq_remove_contri (xlator_t *this, loc_t *loc, quota_inode_ctx_t *ctx,

+                  inode_contribution_t *contri, quota_meta_t *delta)

 {

         int32_t              ret                         = -1;

         char                 contri_key[CONTRI_KEY_MAX]  = {0, };

@@ -2588,7 +2599,8 @@ mq_remove_contri (xlator_t *this, loc_t *loc, inode_contribution_t *contri)

         ret = syncop_removexattr (FIRST_CHILD(this), loc, contri_key, 0, NULL);

         if (ret < 0) {

-                if (-ret == ENOENT || -ret == ESTALE || -ret == ENODATA) {

+                if (-ret == ENOENT || -ret == ESTALE || -ret == ENODATA ||

+                    -ret == ENOATTR) {

                         /* Remove contri in done when unlink operation is

                          * performed, so return success on ENOENT/ESTSLE

                          * rename operation removes xattr earlier,

@@ -2605,14 +2617,16 @@ mq_remove_contri (xlator_t *this, loc_t *loc, inode_contribution_t *contri)

         LOCK (&contri->lock);

         {

-                contri->contribution = 0;

-                contri->file_count = 0;

-                contri->dir_count = 0;

+                contri->contribution += delta->size;

+                contri->file_count += delta->file_count;

+                contri->dir_count += delta->dir_count;

         }

         UNLOCK (&contri->lock);

         ret = 0;

+

 out:

+        QUOTA_FREE_CONTRIBUTION_NODE (ctx, contri);

         return ret;

 }

@@ -2809,10 +2823,12 @@ mq_start_quota_txn_v2 (xlator_t *this, loc_t *loc, quota_inode_ctx_t *ctx,

         gf_boolean_t       status     = _gf_true;

         quota_meta_t       delta      = {0, };

+        GF_VALIDATE_OR_GOTO ("marker", contri, out);

+        GF_REF_GET (contri);

+

         GF_VALIDATE_OR_GOTO ("marker", loc, out);

         GF_VALIDATE_OR_GOTO ("marker", loc->inode, out);

         GF_VALIDATE_OR_GOTO ("marker", ctx, out);

-        GF_VALIDATE_OR_GOTO ("marker", contri, out);

         ret = mq_loc_copy (&child_loc, loc);

         if (ret < 0) {

@@ -2907,6 +2923,8 @@ mq_start_quota_txn_v2 (xlator_t *this, loc_t *loc, quota_inode_ctx_t *ctx,

                         ret = -1;

                         goto out;

                 }

+

+                GF_REF_PUT (contri);

                 contri = mq_get_contribution_node (child_loc.parent, ctx);

                 GF_ASSERT (contri != NULL);

         }

@@ -2923,6 +2941,8 @@ out:

         loc_wipe (&child_loc);

         loc_wipe (&parent_loc);

+        if (contri)

+                GF_REF_PUT (contri);

         return ret;

 }

@@ -3100,11 +3120,12 @@ mq_reduce_parent_size_task (void *opaque)

                 goto out;

         dirty = _gf_true;

-        ret = mq_remove_contri (this, loc, contribution);

+        mq_sub_meta (&delta, NULL);

+

+        ret = mq_remove_contri (this, loc, ctx, contribution, &delta);

         if (ret < 0)

                 goto out;

-        mq_sub_meta (&delta, NULL);

         ret = mq_update_size (this, &parent_loc, &delta);

         if (ret < 0)

                 goto out;

@@ -3121,6 +3142,9 @@ out:

         loc_wipe (&parent_loc);

+        if (contribution)

+                GF_REF_PUT (contribution);

+

         return ret;

 }

@@ -3203,6 +3227,9 @@ mq_initiate_quota_task (void *opaque)

         ret = 0;

 out:

+        if (contribution)

+                GF_REF_PUT (contribution);

+

         return ret;

 }

@@ -3452,13 +3479,18 @@ mq_inspect_directory_xattr_task (void *opaque)

         }

         ret = dict_get_int8 (dict, QUOTA_DIRTY_KEY, &dirty);

-        if (ret < 0)

-                goto out;

+        if (ret < 0) {

+                /* dirty is set only on the first file write operation

+                 * so ignore this error

+                 */

+                ret = 0;

+                dirty = 0;

+        }

         ret = _quota_dict_get_meta (this, dict, QUOTA_SIZE_KEY, &size,

                                     IA_IFDIR, _gf_false);

         if (ret < 0)

-                goto out;

+                goto create_xattr;

         if (!loc_is_root(loc)) {

                 GET_CONTRI_KEY (contri_key, contribution->gfid, ret);

@@ -3468,7 +3500,7 @@ mq_inspect_directory_xattr_task (void *opaque)

                 ret = _quota_dict_get_meta (this, dict, contri_key, &contri,

                                             IA_IFDIR, _gf_false);

                 if (ret < 0)

-                        goto out;

+                        goto create_xattr;

                 LOCK (&contribution->lock);

                 {

@@ -3499,11 +3531,15 @@ mq_inspect_directory_xattr_task (void *opaque)

                 mq_initiate_quota_blocking_txn (this, loc);

         ret = 0;

-out:

+

+create_xattr:

         if (ret < 0)

                 ret = mq_create_xattrs_blocking_txn (this, loc);

 err:

+        if (contribution)

+                GF_REF_PUT (contribution);

+

         return ret;

 }

@@ -3575,41 +3611,32 @@ mq_inspect_file_xattr_task (void *opaque)

         }

         UNLOCK (&ctx->lock);

-        list_for_each_entry (contribution, &ctx->contribution_head,

-                             contri_list) {

-

-                GET_CONTRI_KEY (contri_key, contribution->gfid, ret);

-                if (ret < 0)

-                        continue;

-

-                ret = _quota_dict_get_meta (this, dict, contri_key, &contri,

-                                            IA_IFREG, _gf_true);

-                if (ret < 0) {

-                        ret = mq_create_xattrs_blocking_txn (this, loc);

-                } else {

-                        LOCK (&contribution->lock);

-                        {

-                                contribution->contribution = contri.size;

-                                contribution->file_count = contri.file_count;

-                                contribution->dir_count = contri.dir_count;

-                        }

-                        UNLOCK (&contribution->lock);

+        GET_CONTRI_KEY (contri_key, contribution->gfid, ret);

+        if (ret < 0)

+                goto out;

-                        mq_compute_delta (&delta, &size, &contri);

-                        if (!quota_meta_is_null (&delta)) {

-                                mq_initiate_quota_blocking_txn (this, loc);

-                                /* TODO: revist this code when fixing hardlinks

-                                 */

-                                break;

-                        }

+        ret = _quota_dict_get_meta (this, dict, contri_key, &contri,

+                                    IA_IFREG, _gf_true);

+        if (ret < 0) {

+                ret = mq_create_xattrs_blocking_txn (this, loc);

+        } else {

+                LOCK (&contribution->lock);

+                {

+                        contribution->contribution = contri.size;

+                        contribution->file_count = contri.file_count;

+                        contribution->dir_count = contri.dir_count;

                 }

+                UNLOCK (&contribution->lock);

-                /* TODO: loc->parent might need to be assigned to corresponding

-                 * contribution inode. We need to handle hard links here

-                */

+                mq_compute_delta (&delta, &size, &contri);

+                if (!quota_meta_is_null (&delta))

+                        mq_initiate_quota_blocking_txn (this, loc);

         }

+        /* TODO: revist this code when fixing hardlinks */

 out:

+        if (contribution)

+                GF_REF_PUT (contribution);

         return ret;

 }

@@ -3952,7 +3979,10 @@ mq_reduce_parent_size (xlator_t *this, loc_t *loc, int64_t contri)

                 goto out;

         local->ctx = ctx;

-        local->contri = contribution;

+        if (contribution) {

+                local->contri = contribution;

+                GF_REF_GET (local->contri);

+        }

         ret = mq_inode_loc_fill (NULL, loc->parent, &local->parent_loc);

         if (ret < 0) {

@@ -3996,6 +4026,9 @@ out:

         if (local != NULL)

                 mq_local_unref (this, local);

+        if (contribution)

+                GF_REF_PUT (contribution);

+

         return ret;

 }

@@ -4030,6 +4063,10 @@ mq_rename_update_newpath (xlator_t *this, loc_t *loc)

         mq_initiate_quota_txn (this, loc);

 out:

+

+        if (contribution)

+                GF_REF_PUT (contribution);

+

         return ret;

 }

@@ -4045,7 +4082,7 @@ mq_forget (xlator_t *this, quota_inode_ctx_t *ctx)

         list_for_each_entry_safe (contri, next, &ctx->contribution_head,

                                   contri_list) {

                 list_del (&contri->contri_list);

-                GF_FREE (contri);

+                GF_REF_PUT (contri);

         }

         LOCK_DESTROY (&ctx->lock);

diff --git a/xlators/features/marker/src/marker-quota.h b/xlators/features/marker/src/marker-quota.h

index a81db7f..2d03dfd 100644

--- a/xlators/features/marker/src/marker-quota.h

+++ b/xlators/features/marker/src/marker-quota.h

@@ -17,6 +17,7 @@

 #include "xlator.h"

 #include "marker-mem-types.h"

+#include "refcount.h"

 #define QUOTA_XATTR_PREFIX "trusted.glusterfs"

 #define QUOTA_DIRTY_KEY "trusted.glusterfs.quota.dirty"

@@ -109,7 +110,8 @@ struct inode_contribution {

         int64_t          file_count;

         int64_t          dir_count;

         uuid_t           gfid;

-  gf_lock_t lock;

+        gf_lock_t        lock;

+        GF_REF_DECL;

 };

 typedef struct inode_contribution inode_contribution_t;

-- 

1.7.1