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