summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorRaghavendra Bhat <raghavendrabhat@gluster.com>2011-08-20 15:00:49 +0530
committerVijay Bellur <vijay@gluster.com>2011-08-20 05:07:06 -0700
commit0564d1198bd7fa9cc18b7ecf2756d7239a052276 (patch)
tree1e7143dddbd77e0978c26838cae8a89eadbf1604
parent1116e4a45a86383a87c7a73b1ccacd356f91f102 (diff)
features/marker: avoid race conditions in marker-quota and avoid memory corruptionv3.2.3qa4
Change-Id: I9e69c7fcf47d611ea960f9969bbc3fb96d93d58e BUG: 3389 Reviewed-on: http://review.gluster.com/278 Tested-by: Gluster Build System <jenkins@build.gluster.com> Reviewed-by: Vijay Bellur <vijay@gluster.com>
-rw-r--r--xlators/features/marker/src/marker-quota-helper.c13
-rw-r--r--xlators/features/marker/src/marker-quota.c236
2 files changed, 199 insertions, 50 deletions
diff --git a/xlators/features/marker/src/marker-quota-helper.c b/xlators/features/marker/src/marker-quota-helper.c
index 778f0a7..d701cb5 100644
--- a/xlators/features/marker/src/marker-quota-helper.c
+++ b/xlators/features/marker/src/marker-quota-helper.c
@@ -32,8 +32,12 @@ quota_loc_fill (loc_t *loc, inode_t *inode, inode_t *parent, char *path)
{
int ret = -1;
- if (!loc)
- return ret;
+ GF_VALIDATE_OR_GOTO ("marker", loc, out);
+ GF_VALIDATE_OR_GOTO ("marker", inode, out);
+ GF_VALIDATE_OR_GOTO ("marker", path, out);
+ /* Not checking for parent because while filling
+ * loc of root, parent will be NULL
+ */
if (inode) {
loc->inode = inode_ref (inode);
@@ -59,7 +63,7 @@ quota_loc_fill (loc_t *loc, inode_t *inode, inode_t *parent, char *path)
loc_wipe:
if (ret < 0)
loc_wipe (loc);
-
+out:
return ret;
}
@@ -180,6 +184,7 @@ __add_new_contribution_node (xlator_t *this, quota_inode_ctx_t *ctx, loc_t *loc)
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);
@@ -363,7 +368,7 @@ quota_local_unref (xlator_t *this, quota_local_t *local)
QUOTA_SAFE_DECREMENT (&local->lock, local->ref, ref);
- if (ref > 0)
+ if (ref != 0)
goto out;
if (local->fd != NULL)
diff --git a/xlators/features/marker/src/marker-quota.c b/xlators/features/marker/src/marker-quota.c
index ac59032..8b46e85 100644
--- a/xlators/features/marker/src/marker-quota.c
+++ b/xlators/features/marker/src/marker-quota.c
@@ -30,6 +30,46 @@
#include "marker-quota.h"
#include "marker-quota-helper.h"
+int
+mq_loc_copy (loc_t *dst, loc_t *src)
+{
+ int ret = -1;
+
+ GF_VALIDATE_OR_GOTO ("marker", dst, out);
+ GF_VALIDATE_OR_GOTO ("marker", src, out);
+
+ if (src->inode == NULL ||
+ src->path == NULL) {
+ gf_log ("marker", GF_LOG_WARNING,
+ "src loc is not valid");
+ goto out;
+ }
+
+ ret = loc_copy (dst, src);
+out:
+ return ret;
+}
+
+int32_t
+mq_get_local_err (quota_local_t *local,
+ int32_t *val)
+{
+ int32_t ret = -1;
+
+ GF_VALIDATE_OR_GOTO ("marker", local, out);
+ GF_VALIDATE_OR_GOTO ("marker", val, out);
+
+ LOCK (&local->lock);
+ {
+ *val = local->err;
+ }
+ UNLOCK (&local->lock);
+
+ ret = 0;
+out:
+ return ret;
+}
+
int32_t
mq_get_ctx_updation_status (quota_inode_ctx_t *ctx,
gf_boolean_t *status)
@@ -211,7 +251,12 @@ release_lock_on_dirty_inode (call_frame_t *frame, void *cookie, xlator_t *this,
dirty_inode_updation_done (frame, NULL, this, 0, 0);
return 0;
}
- frame->local = NULL;
+
+ if (local->loc.inode == NULL) {
+ gf_log (this->name, GF_LOG_WARNING,
+ "Inode is NULL, so can't stackwind.");
+ goto out;
+ }
STACK_WIND (frame,
dirty_inode_updation_done,
@@ -220,6 +265,11 @@ release_lock_on_dirty_inode (call_frame_t *frame, void *cookie, xlator_t *this,
this->name, &loc, F_SETLKW, &lock);
loc_wipe (&loc);
+
+ return 0;
+out:
+ dirty_inode_updation_done (frame, NULL, this, -1, 0);
+
return 0;
}
@@ -351,6 +401,29 @@ err:
}
int32_t
+mq_test_and_set_local_err(quota_local_t *local,
+ int32_t *val)
+{
+ int tmp = 0;
+ int32_t ret = -1;
+
+ GF_VALIDATE_OR_GOTO ("marker", local, out);
+ GF_VALIDATE_OR_GOTO ("marker", val, out);
+
+ LOCK (&local->lock);
+ {
+ tmp = local->err;
+ local->err = *val;
+ *val = tmp;
+ }
+ UNLOCK (&local->lock);
+
+ ret = 0;
+out:
+ return ret;
+}
+
+int32_t
get_dirty_inode_size (call_frame_t *frame, xlator_t *this)
{
int32_t ret = -1;
@@ -413,16 +486,19 @@ get_child_contribution (call_frame_t *frame,
QUOTA_STACK_DESTROY (frame, this);
if (op_ret == -1) {
- gf_log (this->name, GF_LOG_ERROR, "%s", strerror (op_errno));
-
- local->err = -2;
- release_lock_on_dirty_inode (local->frame, NULL, this, 0, 0);
+ gf_log (this->name, GF_LOG_ERROR, "%s",
+ strerror (op_errno));
+ val = -2;
+ if (!mq_test_and_set_local_err (local, &val) &&
+ val != -2)
+ release_lock_on_dirty_inode (local->frame, NULL, this, 0, 0);
- goto out;
+ goto exit;
}
- if (local->err)
- goto out;
+ ret = mq_get_local_err (local, &val);
+ if (!ret && val == -2)
+ goto exit;
GET_CONTRI_KEY (contri_key, local->loc.inode->gfid, ret);
if (ret < 0)
@@ -441,15 +517,16 @@ out:
}
UNLOCK (&local->lock);
- if (val== 0) {
- if (local->err)
- quota_local_unref (this, local);
- else
- quota_dirty_inode_readdir (local->frame, NULL, this,
+ if (val == 0) {
+ quota_dirty_inode_readdir (local->frame, NULL, this,
0, 0, NULL);
}
+ quota_local_unref (this, local);
return 0;
+exit:
+ quota_local_unref (this, local);
+ return 0;
}
int32_t
@@ -462,6 +539,7 @@ quota_readdir_cbk (call_frame_t *frame,
{
char contri_key [512] = {0, };
int32_t ret = 0;
+ int32_t val = 0;
off_t offset = 0;
int32_t count = 0;
dict_t *dict = NULL;
@@ -502,16 +580,19 @@ quota_readdir_cbk (call_frame_t *frame,
count++;
}
+ if (count == 0) {
+ get_dirty_inode_size (frame, this);
+ return 0;
+ }
+
local->frame = frame;
- if (count > 0) {
- LOCK (&local->lock);
- {
- local->dentry_child_count = count;
- local->d_off = offset;
- }
- UNLOCK (&local->lock);
+ LOCK (&local->lock);
+ {
+ local->dentry_child_count = count;
+ local->d_off = offset;
}
+ UNLOCK (&local->lock);
list_for_each_entry (entry, (&entries->list), list) {
@@ -568,18 +649,12 @@ quota_readdir_cbk (call_frame_t *frame,
}
if (ret) {
- LOCK (&local->lock);
- {
- if (local->dentry_child_count == 0)
- local->err = -1;
- else
- local->err = -2;
- }
- UNLOCK (&local->lock);
+ val = -2;
+ mq_test_and_set_local_err (local, &val);
if (newframe) {
newframe->local = NULL;
-
+ quota_local_unref(this, local);
QUOTA_STACK_DESTROY (newframe, this);
}
@@ -587,10 +662,8 @@ quota_readdir_cbk (call_frame_t *frame,
}
}
- if (ret) {
+ if (ret && val != -2) {
release_lock_on_dirty_inode (frame, NULL, this, 0, 0);
- } else if (count == 0 ) {
- get_dirty_inode_size (frame, this);
}
return 0;
@@ -776,8 +849,7 @@ update_dirty_inode (xlator_t *this,
goto fr_destroy;
frame->local = local;
-
- ret = loc_copy (&local->loc, loc);
+ ret = mq_loc_copy (&local->loc, loc);
if (ret < 0)
goto fr_destroy;
@@ -790,6 +862,13 @@ update_dirty_inode (xlator_t *this,
lock.l_start = 0;
lock.l_len = 0;
+ if (local->loc.inode == NULL) {
+ ret = -1;
+ gf_log (this->name, GF_LOG_WARNING,
+ "Inode is NULL, so can't stackwind.");
+ goto fr_destroy;
+ }
+
STACK_WIND (frame,
get_dirty_xattr,
FIRST_CHILD(this),
@@ -1122,26 +1201,53 @@ err:
int32_t
get_parent_inode_local (xlator_t *this, quota_local_t *local)
{
- int32_t ret;
+ int32_t ret = -1;
quota_inode_ctx_t *ctx = NULL;
+ GF_VALIDATE_OR_GOTO ("marker", this, out);
+ GF_VALIDATE_OR_GOTO ("marker", local, out);
+
loc_wipe (&local->loc);
- loc_copy (&local->loc, &local->parent_loc);
+ ret = mq_loc_copy (&local->loc, &local->parent_loc);
+ if (ret < 0) {
+ gf_log_callingfn (this->name, GF_LOG_WARNING,
+ "loc copy failed");
+ goto out;
+ }
loc_wipe (&local->parent_loc);
- quota_inode_loc_fill (NULL, local->loc.parent, &local->parent_loc);
+ ret = quota_inode_loc_fill (NULL, local->loc.parent,
+ &local->parent_loc);
+ if (ret < 0) {
+ gf_log_callingfn (this->name, GF_LOG_WARNING,
+ "failed to build parent loc of %s",
+ local->loc.path);
+ goto out;
+ }
ret = quota_inode_ctx_get (local->loc.inode, this, &ctx);
- if (ret < 0)
- return -1;
+ if (ret < 0) {
+ gf_log_callingfn (this->name, GF_LOG_WARNING,
+ "inode ctx get failed");
+ goto out;
+ }
local->ctx = ctx;
+ if (list_empty (&ctx->contribution_head)) {
+ gf_log_callingfn (this->name, GF_LOG_WARNING,
+ "contribution node list is empty which "
+ "is an error");
+ goto out;
+ }
+
local->contri = (inode_contribution_t *) ctx->contribution_head.next;
- return 0;
+ ret = 0;
+out:
+ return ret;
}
@@ -1238,6 +1344,12 @@ quota_release_parent_lock (call_frame_t *frame, void *cookie,
}
UNLOCK (&ctx->lock);
+ if (local->parent_loc.inode == NULL) {
+ gf_log (this->name, GF_LOG_WARNING,
+ "Invalid parent inode.");
+ goto err;
+ }
+
wind:
lock.l_type = F_UNLCK;
lock.l_whence = SEEK_SET;
@@ -1253,6 +1365,10 @@ wind:
F_SETLKW, &lock);
return 0;
+err:
+ xattr_updation_done (frame, NULL, this,
+ 0, 0 , NULL);
+ return 0;
}
@@ -1603,6 +1719,11 @@ quota_fetch_child_size_and_contri (call_frame_t *frame, void *cookie,
if (local->loc.inode->ia_type == IA_IFDIR) {
ret = dict_set_int64 (newdict, QUOTA_SIZE_KEY, 0);
+ if (ret < 0) {
+ gf_log (this->name, GF_LOG_WARNING,
+ "dict_set failed.");
+ goto err;
+ }
}
GET_CONTRI_KEY (contri_key, local->contri->gfid, ret);
@@ -1612,6 +1733,11 @@ quota_fetch_child_size_and_contri (call_frame_t *frame, void *cookie,
}
ret = dict_set_int64 (newdict, contri_key, 0);
+ if (ret < 0) {
+ gf_log (this->name, GF_LOG_WARNING,
+ "dict_set failed.");
+ goto err;
+ }
mq_set_ctx_updation_status (local->ctx, _gf_false);
@@ -1621,7 +1747,7 @@ quota_fetch_child_size_and_contri (call_frame_t *frame, void *cookie,
ret = 0;
err:
- if (op_ret == -1 || ret == -1) {
+ if (op_ret == -1 || ret < 0) {
local->err = op_errno;
mq_set_ctx_updation_status (local->ctx, _gf_false);
@@ -1710,6 +1836,13 @@ get_lock_on_parent (call_frame_t *frame, xlator_t *this)
gf_log (this->name, GF_LOG_DEBUG, "taking lock on %s",
local->parent_loc.path);
+ if (local->parent_loc.inode == NULL) {
+ gf_log (this->name, GF_LOG_DEBUG,
+ "parent inode is not valid, aborting "
+ "transaction.");
+ goto fr_destroy;
+ }
+
lock.l_len = 0;
lock.l_start = 0;
lock.l_type = F_WRLCK;
@@ -1751,7 +1884,7 @@ start_quota_txn (xlator_t *this, loc_t *loc,
frame->local = local;
- ret = loc_copy (&local->loc, loc);
+ ret = mq_loc_copy (&local->loc, loc);
if (ret < 0)
goto fr_destroy;
@@ -1786,7 +1919,9 @@ initiate_quota_txn (xlator_t *this, loc_t *loc)
quota_inode_ctx_t *ctx = NULL;
inode_contribution_t *contribution = NULL;
- VALIDATE_OR_GOTO (loc, out);
+ GF_VALIDATE_OR_GOTO ("marker", this, out);
+ GF_VALIDATE_OR_GOTO ("marker", loc, out);
+ GF_VALIDATE_OR_GOTO ("marker", loc->inode, out);
ret = quota_inode_ctx_get (loc->inode, this, &ctx);
if (ret == -1) {
@@ -2087,11 +2222,13 @@ quota_inode_remove_done (call_frame_t *frame, void *cookie, xlator_t *this,
}
if (strcmp (local->parent_loc.path, "/") != 0) {
- get_parent_inode_local (this, local);
+ ret = get_parent_inode_local (this, local);
+ if (ret < 0)
+ goto out;
start_quota_txn (this, &local->loc, local->ctx, local->contri);
}
-
+out:
quota_local_unref (this, local);
return 0;
@@ -2249,7 +2386,7 @@ reduce_parent_size (xlator_t *this, loc_t *loc, int64_t contri)
goto out;
}
- ret = loc_copy (&local->loc, loc);
+ ret = mq_loc_copy (&local->loc, loc);
if (ret < 0)
goto out;
@@ -2275,6 +2412,13 @@ reduce_parent_size (xlator_t *this, loc_t *loc, int64_t contri)
lock.l_type = F_WRLCK;
lock.l_whence = SEEK_SET;
+ if (local->parent_loc.inode == NULL) {
+ ret = -1;
+ gf_log (this->name, GF_LOG_WARNING,
+ "Inode is NULL, so can't stackwind.");
+ goto out;
+ }
+
STACK_WIND (frame,
mq_reduce_parent_size_xattr,
FIRST_CHILD(this),