quota/marker: fix mem-leak in marker

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>

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

@@ -125,6 +125,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)
 {
@@ -134,35 +167,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) {
@@ -185,17 +209,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:
@@ -219,6 +236,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);
 
@@ -387,6 +406,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);

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

@@ -13,10 +13,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)                 \
@@ -62,6 +66,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 *);
 

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

@@ -882,7 +882,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;
@@ -1089,6 +1092,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);
@@ -1262,7 +1268,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);
@@ -1327,9 +1332,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:
@@ -1992,7 +1996,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;
@@ -2242,6 +2249,9 @@ out:
         if (dict)
                 dict_unref (dict);
 
+        if (contribution)
+                GF_REF_PUT (contribution);
+
         return ret;
 }
 
@@ -2569,7 +2579,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, };
@@ -2583,7 +2594,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,
@@ -2600,14 +2612,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;
 }
@@ -2804,10 +2818,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) {
@@ -2902,6 +2918,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);
         }
@@ -2918,6 +2936,8 @@ out:
 
         loc_wipe (&child_loc);
         loc_wipe (&parent_loc);
+        if (contri)
+                GF_REF_PUT (contri);
 
         return ret;
 }
@@ -3095,11 +3115,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;
@@ -3116,6 +3137,9 @@ out:
 
         loc_wipe (&parent_loc);
 
+        if (contribution)
+                GF_REF_PUT (contribution);
+
         return ret;
 }
 
@@ -3198,6 +3222,9 @@ mq_initiate_quota_task (void *opaque)
 
         ret = 0;
 out:
+        if (contribution)
+                GF_REF_PUT (contribution);
+
         return ret;
 }
 
@@ -3447,13 +3474,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);
@@ -3463,7 +3495,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);
                 {
@@ -3494,11 +3526,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;
 }
 
@@ -3570,41 +3606,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)
+                goto out;
 
-                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);
-
-                        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;
 }
@@ -3947,7 +3974,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) {
@@ -3991,6 +4021,9 @@ out:
         if (local != NULL)
                 mq_local_unref (this, local);
 
+        if (contribution)
+                GF_REF_PUT (contribution);
+
         return ret;
 }
 
@@ -4025,6 +4058,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;
 }
 
@@ -4040,7 +4077,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);

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

@@ -12,6 +12,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"
@@ -104,7 +105,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;