From 2b898184f17dd95ee2f7c17fb4f8ec3aad598135 Mon Sep 17 00:00:00 2001
From: Poornima G <pgurusid@redhat.com>
Date: Fri, 26 May 2017 15:45:57 +0530
Subject: [PATCH 505/509] nl-cache: Fix a possible crash and stale cache
Issue1:
Consider the followinf sequence of operations:
...
nlc_ctx = nlc_ctx_get (inode i1)
....... -> nlc_clear_cache (i1) gets called as a part of nlc_invalidate
or any other callers
...
GF_FREE (ii nlc_ctx)
LOCK (nlc_ctx->lock); -> This will result in crash as the ctx
got freed in nlc_clear_cache.
Issue2:
lookup on dir1/file1 result in ENOENT
add cache to dir1 at time T1
....
CHILD_DOWN at T2
lookup on dir1/file2 result in ENOENT
add cache to dir1, but the cache time is still T1
lookup on dir1/file2 - should have been served from cache
but the cache time is T1 < T2, hence
cache is considered as invalid.
So, after CHILD_DOWN the right thing would be to clear the cache
and restart caching on that inode.
Solution:
Do not free nlc_ctx in nlc_clear_cache, but only in inode_forget()
The fix for both issue1 and 2 is interleaved hence sending it as
single patch.
> Reviewed-on: https://review.gluster.org/17453
> Smoke: Gluster Build System <jenkins@build.gluster.org>
> NetBSD-regression: NetBSD Build System <jenkins@build.gluster.org>
> Reviewed-by: Pranith Kumar Karampuri <pkarampu@redhat.com>
> CentOS-regression: Gluster Build System <jenkins@build.gluster.org>
> Signed-off-by: Poornima G <pgurusid@redhat.com>
Change-Id: I83d8ed36c049a93567c6d7e63d045dc14ccbb397
BUG: 1450080
Signed-off-by: Poornima G <pgurusid@redhat.com>
Reviewed-on: https://code.engineering.redhat.com/gerrit/108892
Reviewed-by: Atin Mukherjee <amukherj@redhat.com>
---
tests/basic/nl-cache.t | 1 +
xlators/mgmt/glusterd/src/glusterd-volume-set.c | 1 -
xlators/performance/nl-cache/src/nl-cache-helper.c | 167 +++++++++++++++------
xlators/performance/nl-cache/src/nl-cache.c | 21 ++-
xlators/performance/nl-cache/src/nl-cache.h | 1 -
5 files changed, 142 insertions(+), 49 deletions(-)
diff --git a/tests/basic/nl-cache.t b/tests/basic/nl-cache.t
index f615328..2979a9b 100755
--- a/tests/basic/nl-cache.t
+++ b/tests/basic/nl-cache.t
@@ -16,6 +16,7 @@ EXPECT 'on' volinfo_field $V0 'performance.nl-cache'
EXPECT '600' volinfo_field $V0 'features.cache-invalidation-timeout'
EXPECT 'on' volinfo_field $V0 'features.cache-invalidation'
EXPECT '50000' volinfo_field $V0 'network.inode-lru-limit'
+TEST $CLI volume set $V0 nl-cache-positive-entry on
TEST $CLI volume start $V0;
EXPECT 'Started' volinfo_field $V0 'Status';
diff --git a/xlators/mgmt/glusterd/src/glusterd-volume-set.c b/xlators/mgmt/glusterd/src/glusterd-volume-set.c
index dd3f4fd..d5fa23b 100644
--- a/xlators/mgmt/glusterd/src/glusterd-volume-set.c
+++ b/xlators/mgmt/glusterd/src/glusterd-volume-set.c
@@ -3148,7 +3148,6 @@ struct volopt_map_entry glusterd_volopt_map[] = {
},
{ .key = "performance.nl-cache-positive-entry",
.voltype = "performance/nl-cache",
- .value = "on",
.type = DOC,
.flags = OPT_FLAG_CLIENT_OPT,
.op_version = GD_OP_VERSION_3_11_0,
diff --git a/xlators/performance/nl-cache/src/nl-cache-helper.c b/xlators/performance/nl-cache/src/nl-cache-helper.c
index edc8c16..2ca6bb9 100644
--- a/xlators/performance/nl-cache/src/nl-cache-helper.c
+++ b/xlators/performance/nl-cache/src/nl-cache-helper.c
@@ -67,6 +67,8 @@ int __nlc_add_to_lru (xlator_t *this, inode_t *inode, nlc_ctx_t *nlc_ctx);
void nlc_remove_from_lru (xlator_t *this, inode_t *inode);
void __nlc_inode_ctx_timer_delete (xlator_t *this, nlc_ctx_t *nlc_ctx);
gf_boolean_t __nlc_search_ne (nlc_ctx_t *nlc_ctx, const char *name);
+void __nlc_free_pe (xlator_t *this, nlc_ctx_t *nlc_ctx, nlc_pe_t *pe);
+void __nlc_free_ne (xlator_t *this, nlc_ctx_t *nlc_ctx, nlc_ne_t *ne);
static int32_t
nlc_get_cache_timeout (xlator_t *this)
@@ -98,7 +100,8 @@ __nlc_is_cache_valid (xlator_t *this, nlc_ctx_t *nlc_ctx)
}
UNLOCK (&conf->lock);
- if (last_val_time <= nlc_ctx->cache_time)
+ if ((last_val_time <= nlc_ctx->cache_time) &&
+ (nlc_ctx->cache_time != 0))
ret = _gf_true;
out:
return ret;
@@ -202,6 +205,88 @@ nlc_inode_ctx_get (xlator_t *this, inode_t *inode, nlc_ctx_t **nlc_ctx_p,
}
+static void
+__nlc_inode_clear_entries (xlator_t *this, nlc_ctx_t *nlc_ctx)
+{
+ nlc_pe_t *pe = NULL;
+ nlc_pe_t *tmp = NULL;
+ nlc_ne_t *ne = NULL;
+ nlc_ne_t *tmp1 = NULL;
+
+ if (!nlc_ctx)
+ goto out;
+
+ if (IS_PE_VALID (nlc_ctx->state))
+ list_for_each_entry_safe (pe, tmp, &nlc_ctx->pe, list) {
+ __nlc_free_pe (this, nlc_ctx, pe);
+ }
+
+ if (IS_NE_VALID (nlc_ctx->state))
+ list_for_each_entry_safe (ne, tmp1, &nlc_ctx->ne, list) {
+ __nlc_free_ne (this, nlc_ctx, ne);
+ }
+
+ nlc_ctx->cache_time = 0;
+ nlc_ctx->state = 0;
+ GF_ASSERT (nlc_ctx->cache_size == sizeof (*nlc_ctx));
+ GF_ASSERT (nlc_ctx->refd_inodes == 0);
+out:
+ return;
+}
+
+
+static void
+nlc_init_invalid_ctx (xlator_t *this, inode_t *inode, nlc_ctx_t *nlc_ctx)
+{
+ nlc_conf_t *conf = NULL;
+ int ret = -1;
+
+ conf = this->private;
+
+ LOCK (&nlc_ctx->lock);
+ {
+ if (__nlc_is_cache_valid (this, nlc_ctx))
+ goto unlock;
+
+ /* The cache/nlc_ctx can be invalid for 2 reasons:
+ * - Because of a child-down/timer expiry, cache is
+ * invalid but the nlc_ctx is not yet cleaned up.
+ * - nlc_ctx is cleaned up, because of invalidations
+ * or lru prune etc.*/
+
+ /* If the cache is present but invalid, clear the cache and
+ * reset the timer. */
+ __nlc_inode_clear_entries (this, nlc_ctx);
+
+ /* If timer is present, then it is already part of lru as well
+ * Hence reset the timer and return.*/
+ if (nlc_ctx->timer) {
+ gf_tw_mod_timer_pending (conf->timer_wheel,
+ nlc_ctx->timer,
+ conf->cache_timeout);
+ time (&nlc_ctx->cache_time);
+ goto unlock;
+ }
+
+ /* If timer was NULL, the nlc_ctx is already cleanedup,
+ * and we need to start timer and add to lru, so that it is
+ * ready to cache entries a fresh */
+ ret = __nlc_inode_ctx_timer_start (this, inode, nlc_ctx);
+ if (ret < 0)
+ goto unlock;
+
+ ret = __nlc_add_to_lru (this, inode, nlc_ctx);
+ if (ret < 0) {
+ __nlc_inode_ctx_timer_delete (this, nlc_ctx);
+ goto unlock;
+ }
+ }
+unlock:
+ UNLOCK (&nlc_ctx->lock);
+
+ return;
+}
+
static nlc_ctx_t *
nlc_inode_ctx_get_set (xlator_t *this, inode_t *inode, nlc_ctx_t **nlc_ctx_p,
nlc_pe_t **nlc_pe_p)
@@ -252,8 +337,10 @@ nlc_inode_ctx_get_set (xlator_t *this, inode_t *inode, nlc_ctx_t **nlc_ctx_p,
unlock:
UNLOCK (&inode->lock);
- if (ret == 0 && nlc_ctx_p)
+ if (ret == 0 && nlc_ctx_p) {
*nlc_ctx_p = nlc_ctx;
+ nlc_init_invalid_ctx (this, inode, nlc_ctx);
+ }
if (ret < 0 && nlc_ctx) {
LOCK_DESTROY (&nlc_ctx->lock);
@@ -261,6 +348,7 @@ unlock:
nlc_ctx = NULL;
goto out;
}
+
out:
return nlc_ctx;
}
@@ -342,14 +430,20 @@ static void
nlc_cache_timeout_handler (struct gf_tw_timer_list *timer,
void *data, unsigned long calltime)
{
- nlc_timer_data_t *tmp = data;
-
- nlc_inode_clear_cache (tmp->this, tmp->inode, NLC_TIMER_EXPIRED);
- inode_unref (tmp->inode);
+ nlc_timer_data_t *tmp = data;
+ nlc_ctx_t *nlc_ctx = NULL;
- GF_FREE (tmp);
- GF_FREE (timer);
+ nlc_inode_ctx_get (tmp->this, tmp->inode, &nlc_ctx, NULL);
+ if (!nlc_ctx)
+ goto out;
+ /* Taking nlc_ctx->lock will lead to deadlock, hence updating
+ * the cache is invalid outside of lock, instead of clear_cache.
+ * Since cache_time is assigned outside of lock, the value can
+ * be invalid for short time, this may result in false negative
+ * which is better than deadlock */
+ nlc_ctx->cache_time = 0;
+out:
return;
}
@@ -361,10 +455,14 @@ __nlc_inode_ctx_timer_delete (xlator_t *this, nlc_ctx_t *nlc_ctx)
conf = this->private;
- gf_tw_del_timer (conf->timer_wheel, nlc_ctx->timer);
+ if (nlc_ctx->timer)
+ gf_tw_del_timer (conf->timer_wheel, nlc_ctx->timer);
- inode_unref (nlc_ctx->timer_data->inode);
- GF_FREE (nlc_ctx->timer_data);
+ if (nlc_ctx->timer_data) {
+ inode_unref (nlc_ctx->timer_data->inode);
+ GF_FREE (nlc_ctx->timer_data);
+ nlc_ctx->timer_data = NULL;
+ }
GF_FREE (nlc_ctx->timer);
nlc_ctx->timer = NULL;
@@ -553,7 +651,7 @@ nlc_clear_all_cache (xlator_t *this)
}
-static void
+void
__nlc_free_pe (xlator_t *this, nlc_ctx_t *nlc_ctx, nlc_pe_t *pe)
{
uint64_t pe_int = 0;
@@ -584,7 +682,7 @@ __nlc_free_pe (xlator_t *this, nlc_ctx_t *nlc_ctx, nlc_pe_t *pe)
}
-static void
+void
__nlc_free_ne (xlator_t *this, nlc_ctx_t *nlc_ctx, nlc_ne_t *ne)
{
nlc_conf_t *conf = NULL;
@@ -606,49 +704,22 @@ __nlc_free_ne (xlator_t *this, nlc_ctx_t *nlc_ctx, nlc_ne_t *ne)
void
nlc_inode_clear_cache (xlator_t *this, inode_t *inode, int reason)
{
- uint64_t nlc_ctx_int = 0;
nlc_ctx_t *nlc_ctx = NULL;
- nlc_pe_t *pe = NULL;
- nlc_pe_t *tmp = NULL;
- nlc_ne_t *ne = NULL;
- nlc_ne_t *tmp1 = NULL;
- nlc_conf_t *conf = NULL;
-
- conf = this->private;
- inode_ctx_reset0 (inode, this, &nlc_ctx_int);
- if (nlc_ctx_int == 0)
+ nlc_inode_ctx_get (this, inode, &nlc_ctx, NULL);
+ if (!nlc_ctx)
goto out;
- nlc_ctx = (void *) (long) nlc_ctx_int;
-
- if (reason != NLC_LRU_PRUNE)
- nlc_remove_from_lru (this, inode);
-
LOCK (&nlc_ctx->lock);
{
- if (reason != NLC_TIMER_EXPIRED)
- __nlc_inode_ctx_timer_delete (this, nlc_ctx);
-
- if (IS_PE_VALID (nlc_ctx->state))
- list_for_each_entry_safe (pe, tmp, &nlc_ctx->pe, list) {
- __nlc_free_pe (this, nlc_ctx, pe);
- }
+ __nlc_inode_ctx_timer_delete (this, nlc_ctx);
- if (IS_NE_VALID (nlc_ctx->state))
- list_for_each_entry_safe (ne, tmp1, &nlc_ctx->ne, list) {
- __nlc_free_ne (this, nlc_ctx, ne);
- }
+ __nlc_inode_clear_entries (this, nlc_ctx);
}
UNLOCK (&nlc_ctx->lock);
- LOCK_DESTROY (&nlc_ctx->lock);
-
- nlc_ctx->cache_size -= sizeof (*nlc_ctx);
- GF_ASSERT (nlc_ctx->cache_size == 0);
- GF_FREE (nlc_ctx);
-
- GF_ATOMIC_SUB (conf->current_cache_size, sizeof (*nlc_ctx));
+ if (reason != NLC_LRU_PRUNE)
+ nlc_remove_from_lru (this, inode);
out:
return;
@@ -864,10 +935,14 @@ nlc_dir_remove_pe (xlator_t *this, inode_t *parent, inode_t *entry_ino,
LOCK (&nlc_ctx->lock);
{
+ if (!__nlc_is_cache_valid (this, nlc_ctx))
+ goto unlock;
+
__nlc_del_pe (this, nlc_ctx, entry_ino, name, multilink);
__nlc_add_ne (this, nlc_ctx, name);
__nlc_set_dir_state (nlc_ctx, NLC_NE_VALID);
}
+unlock:
UNLOCK (&nlc_ctx->lock);
out:
return;
diff --git a/xlators/performance/nl-cache/src/nl-cache.c b/xlators/performance/nl-cache/src/nl-cache.c
index a72f039..7dad8d9 100644
--- a/xlators/performance/nl-cache/src/nl-cache.c
+++ b/xlators/performance/nl-cache/src/nl-cache.c
@@ -473,12 +473,17 @@ nlc_invalidate (xlator_t *this, void *data)
inode_t *parent2 = NULL;
int ret = 0;
inode_table_t *itable = NULL;
+ nlc_conf_t *conf = NULL;
up_data = (struct gf_upcall *)data;
if (up_data->event_type != GF_UPCALL_CACHE_INVALIDATION)
goto out;
+ conf = this->private;
+ if (!conf)
+ goto out;
+
up_ci = (struct gf_upcall_cache_invalidation *)up_data->data;
/*TODO: Add he inodes found as a member in gf_upcall_cache_invalidation
@@ -520,6 +525,9 @@ nlc_invalidate (xlator_t *this, void *data)
nlc_inode_clear_cache (this, parent1, NLC_NONE);
if (parent2)
nlc_inode_clear_cache (this, parent2, NLC_NONE);
+
+ GF_ATOMIC_INC (conf->nlc_counter.nlc_invals);
+
out:
if (inode)
inode_unref (inode);
@@ -568,12 +576,23 @@ notify (xlator_t *this, int event, void *data, ...)
static int32_t
nlc_forget (xlator_t *this, inode_t *inode)
{
- uint64_t pe_int = 0;
+ uint64_t pe_int = 0;
+ uint64_t nlc_ctx_int = 0;
+ nlc_ctx_t *nlc_ctx = NULL;
+ nlc_conf_t *conf = NULL;
+
+ conf = this->private;
inode_ctx_reset1 (inode, this, &pe_int);
GF_ASSERT (pe_int == 0);
nlc_inode_clear_cache (this, inode, NLC_NONE);
+ inode_ctx_reset0 (inode, this, &nlc_ctx_int);
+ nlc_ctx = (void *) (long) nlc_ctx_int;
+ if (nlc_ctx) {
+ GF_FREE (nlc_ctx);
+ GF_ATOMIC_SUB (conf->current_cache_size, sizeof (*nlc_ctx));
+ }
return 0;
}
diff --git a/xlators/performance/nl-cache/src/nl-cache.h b/xlators/performance/nl-cache/src/nl-cache.h
index 10ec022..3bd7c83 100644
--- a/xlators/performance/nl-cache/src/nl-cache.h
+++ b/xlators/performance/nl-cache/src/nl-cache.h
@@ -44,7 +44,6 @@
enum nlc_cache_clear_reason {
NLC_NONE = 0,
- NLC_TIMER_EXPIRED,
NLC_LRU_PRUNE,
};
--
1.8.3.1