From 9b1305e549879e7698c46553bd91c722877d44cb Mon Sep 17 00:00:00 2001 From: vmallika Date: Wed, 24 Jun 2015 11:56:30 +0530 Subject: 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 Reviewed-on: http://review.gluster.org/11361 Reviewed-by: Krishnan Parthasarathi Tested-by: NetBSD Build System Reviewed-by: Raghavendra G Tested-by: Raghavendra G --- 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(-) (limited to 'xlators/features/marker') diff --git a/xlators/features/marker/src/marker-quota-helper.c b/xlators/features/marker/src/marker-quota-helper.c index 9a0cf56a3fe..6265ffbac6e 100644 --- a/xlators/features/marker/src/marker-quota-helper.c +++ b/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); diff --git a/xlators/features/marker/src/marker-quota-helper.h b/xlators/features/marker/src/marker-quota-helper.h index b333a0ec52f..7450a6e3604 100644 --- a/xlators/features/marker/src/marker-quota-helper.h +++ b/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 *); diff --git a/xlators/features/marker/src/marker-quota.c b/xlators/features/marker/src/marker-quota.c index c33d8a46707..90aec646116 100644 --- a/xlators/features/marker/src/marker-quota.c +++ b/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) - 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; } @@ -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); diff --git a/xlators/features/marker/src/marker-quota.h b/xlators/features/marker/src/marker-quota.h index 9dfac2e3bb5..2a618976e75 100644 --- a/xlators/features/marker/src/marker-quota.h +++ b/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; -- cgit