summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--tests/basic/afr/root-squash-self-heal.t1
-rw-r--r--tests/basic/afr/split-brain-healing.t4
-rwxr-xr-xtests/bugs/glusterd/859927/repl.t6
-rw-r--r--tests/bugs/replicate/bug-821056.t2
-rw-r--r--xlators/cluster/afr/src/afr-common.c228
-rw-r--r--xlators/cluster/afr/src/afr-inode-read.c6
-rw-r--r--xlators/cluster/afr/src/afr-self-heal-common.c5
-rw-r--r--xlators/cluster/afr/src/afr-self-heal-data.c12
-rw-r--r--xlators/cluster/afr/src/afr-self-heal-metadata.c2
-rw-r--r--xlators/cluster/afr/src/afr.h3
-rw-r--r--xlators/cluster/afr/src/pump.c2
11 files changed, 161 insertions, 110 deletions
diff --git a/tests/basic/afr/root-squash-self-heal.t b/tests/basic/afr/root-squash-self-heal.t
index 8337432dbc9..ff0aa5cecb7 100644
--- a/tests/basic/afr/root-squash-self-heal.t
+++ b/tests/basic/afr/root-squash-self-heal.t
@@ -14,7 +14,6 @@ TEST $CLI volume set $V0 server.root-squash on
TEST $CLI volume start $V0
TEST glusterfs --volfile-id=/$V0 --volfile-server=$H0 $M0 --attribute-timeout=0 --entry-timeout=0 --no-root-squash=yes --use-readdirp=no
TEST kill_brick $V0 $H0 $B0/${V0}0
-HEAL_FILES=0
echo abc > $M0/a
TEST $CLI volume start $V0 force
diff --git a/tests/basic/afr/split-brain-healing.t b/tests/basic/afr/split-brain-healing.t
index 2171de3029d..302a3e6144b 100644
--- a/tests/basic/afr/split-brain-healing.t
+++ b/tests/basic/afr/split-brain-healing.t
@@ -124,7 +124,7 @@ subvolume=$(get_replicate_subvol_number file3)
if [ $subvolume == 0 ]
then
$CLI volume heal $V0 split-brain source-brick $H0:$B0/${V0}2 /file3
-elif [ $subvolume == 1]
+elif [ $subvolume == 1 ]
then
$CLI volume heal $V0 split-brain source-brick $H0:$B0/${V0}4 /file3
fi
@@ -139,7 +139,7 @@ then
GFID=$(gf_get_gfid_xattr $B0/${V0}1/file4)
GFIDSTR="gfid:$(gf_gfid_xattr_to_str $GFID)"
$CLI volume heal $V0 split-brain source-brick $H0:$B0/${V0}2 $GFIDSTR
-elif [ $subvolume == 1]
+elif [ $subvolume == 1 ]
then
GFID=$(gf_get_gfid_xattr $B0/${V0}3/file4)
GFIDSTR="gfid:$(gf_gfid_xattr_to_str $GFID)"
diff --git a/tests/bugs/glusterd/859927/repl.t b/tests/bugs/glusterd/859927/repl.t
index 40e86029685..70143e2c193 100755
--- a/tests/bugs/glusterd/859927/repl.t
+++ b/tests/bugs/glusterd/859927/repl.t
@@ -32,7 +32,7 @@ TEST $CLI volume set $V0 cluster.data-self-heal-algorithm full
EXPECT full volume_option $V0 cluster.data-self-heal-algorithm
create_setup_for_self_heal $M0/a
EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status $V0 0
-cat $file 2>&1 > /dev/null
+cat $file > /dev/null 2>&1
EXPECT_WITHIN $HEAL_TIMEOUT 0 get_pending_heal_count $V0
TEST cmp $B0/${V0}1/a $B0/${V0}2/a
@@ -40,14 +40,14 @@ TEST $CLI volume set $V0 cluster.data-self-heal-algorithm diff
EXPECT diff volume_option $V0 cluster.data-self-heal-algorithm
create_setup_for_self_heal $M0/a
EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status $V0 0
-cat $file 2>&1 > /dev/null
+cat $file > /dev/null 2>&1
EXPECT_WITHIN $HEAL_TIMEOUT 0 get_pending_heal_count $V0
TEST cmp $B0/${V0}1/a $B0/${V0}2/a
TEST $CLI volume reset $V0 cluster.data-self-heal-algorithm
create_setup_for_self_heal $M0/a
EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status $V0 0
-cat $file 2>&1 > /dev/null
+cat $file > /dev/null 2>&1
EXPECT_WITHIN $HEAL_TIMEOUT 0 get_pending_heal_count $V0
TEST cmp $B0/${V0}1/a $B0/${V0}2/a
diff --git a/tests/bugs/replicate/bug-821056.t b/tests/bugs/replicate/bug-821056.t
index a1633004404..81186d86309 100644
--- a/tests/bugs/replicate/bug-821056.t
+++ b/tests/bugs/replicate/bug-821056.t
@@ -35,7 +35,7 @@ kill_brick $V0 $H0 $B0/${V0}0
TEST gf_rm_file_and_gfid_link $B0/${V0}0 "a"
TEST $CLI volume start $V0 force
EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status $V0 0
-ls -l $M0/a 2>&1 > /dev/null #Make sure the file is re-created
+ls -l $M0/a > /dev/null 2>&1 #Make sure the file is re-created
EXPECT "N" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}0 "$realpath"
EXPECT "N" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}0 $B0/${V0}0/a
diff --git a/xlators/cluster/afr/src/afr-common.c b/xlators/cluster/afr/src/afr-common.c
index e59f160db0c..557b2cd8891 100644
--- a/xlators/cluster/afr/src/afr-common.c
+++ b/xlators/cluster/afr/src/afr-common.c
@@ -499,8 +499,8 @@ afr_spb_choice_timeout_cancel (xlator_t *this, inode_t *inode)
LOCK(&inode->lock);
{
- __afr_inode_ctx_get (this, inode, &ctx);
- if (!ctx) {
+ ret = __afr_inode_ctx_get (this, inode, &ctx);
+ if (ret < 0 || !ctx) {
gf_msg (this->name, GF_LOG_WARNING, 0,
AFR_MSG_SPLIT_BRAIN_CHOICE_ERROR,
"Failed to cancel split-brain choice timer.");
@@ -533,14 +533,18 @@ afr_set_split_brain_choice_cbk (void *data)
int
afr_set_split_brain_choice (int ret, call_frame_t *frame, void *opaque)
{
- int op_errno = ENOMEM;
- afr_private_t *priv = NULL;
- afr_inode_ctx_t *ctx = NULL;
- inode_t *inode = NULL;
- loc_t *loc = NULL;
- xlator_t *this = NULL;
- afr_spbc_timeout_t *data = opaque;
- struct timespec delta = {0, };
+ int op_errno = ENOMEM;
+ afr_private_t *priv = NULL;
+ afr_inode_ctx_t *ctx = NULL;
+ inode_t *inode = NULL;
+ loc_t *loc = NULL;
+ xlator_t *this = NULL;
+ afr_spbc_timeout_t *data = opaque;
+ struct timespec delta = {0, };
+ gf_boolean_t timer_set = _gf_false;
+ gf_boolean_t timer_cancelled = _gf_false;
+ gf_boolean_t timer_reset = _gf_false;
+ int old_spb_choice = -1;
if (ret)
goto out;
@@ -553,9 +557,11 @@ afr_set_split_brain_choice (int ret, call_frame_t *frame, void *opaque)
delta.tv_sec = priv->spb_choice_timeout;
delta.tv_nsec = 0;
- inode = loc->inode;
- if (!inode)
+ if (!loc->inode) {
+ ret = -1;
+ op_errno = EINVAL;
goto out;
+ }
if (!(data->d_spb || data->m_spb)) {
gf_msg (this->name, GF_LOG_WARNING, 0,
@@ -568,6 +574,12 @@ afr_set_split_brain_choice (int ret, call_frame_t *frame, void *opaque)
goto out;
}
+ /*
+ * we're ref'ing the inode before LOCK like it is done elsewhere in the
+ * code. If we ref after LOCK, coverity complains of possible deadlocks.
+ */
+ inode = inode_ref (loc->inode);
+
LOCK(&inode->lock);
{
ret = __afr_inode_ctx_get (this, inode, &ctx);
@@ -578,16 +590,14 @@ afr_set_split_brain_choice (int ret, call_frame_t *frame, void *opaque)
goto unlock;
}
+ old_spb_choice = ctx->spb_choice;
ctx->spb_choice = data->spb_child_index;
/* Possible changes in spb-choice :
- * -1 to valid : ref and inject timer
- *
- * valid to valid : cancel timer and inject new one
- *
* valid to -1 : cancel timer and unref
- *
- * -1 to -1 : do not do anything
+ * valid to valid : cancel timer and inject new one
+ * -1 to -1 : unref and do not do anything
+ * -1 to valid : inject timer
*/
/* ctx->timer is NULL iff previous value of
@@ -595,31 +605,66 @@ afr_set_split_brain_choice (int ret, call_frame_t *frame, void *opaque)
*/
if (ctx->timer) {
if (ctx->spb_choice == -1) {
- gf_timer_call_cancel (this->ctx, ctx->timer);
- ctx->timer = NULL;
- inode_unref (inode);
+ if (!gf_timer_call_cancel (this->ctx,
+ ctx->timer)) {
+ ctx->timer = NULL;
+ timer_cancelled = _gf_true;
+ }
+ /* If timer cancel failed here it means that the
+ * previous cbk will be executed which will set
+ * spb_choice to -1. So we can consider the
+ * 'valid to -1' case to be a sucess
+ * (i.e. ret = 0) and goto unlock.
+ */
goto unlock;
}
goto reset_timer;
} else {
if (ctx->spb_choice == -1)
goto unlock;
+ goto set_timer;
}
- inode = inode_ref (loc->inode);
- goto set_timer;
-
reset_timer:
- gf_timer_call_cancel (this->ctx, ctx->timer);
+ ret = gf_timer_call_cancel (this->ctx, ctx->timer);
+ if (ret != 0) {
+ /* We need to bail out now instead of launching a new
+ * timer. Otherwise the cbk of the previous timer event
+ * will cancel the new ctx->timer.
+ */
+ ctx->spb_choice = old_spb_choice;
+ ret = -1;
+ op_errno = EAGAIN;
+ goto unlock;
+ }
ctx->timer = NULL;
+ timer_reset = _gf_true;
set_timer:
ctx->timer = gf_timer_call_after (this->ctx, delta,
afr_set_split_brain_choice_cbk,
inode);
+ if (!ctx->timer) {
+ ctx->spb_choice = old_spb_choice;
+ ret = -1;
+ op_errno = ENOMEM;
+ }
+ if (!timer_reset && ctx->timer)
+ timer_set = _gf_true;
+ if (timer_reset && !ctx->timer)
+ timer_cancelled = _gf_true;
}
unlock:
UNLOCK(&inode->lock);
+ if (!timer_set)
+ inode_unref (inode);
+ if (timer_cancelled)
+ inode_unref (inode);
+ /*
+ * We need to invalidate the inode to prevent the kernel from serving
+ * reads from an older cached value despite a change in spb_choice to
+ * a new value.
+ */
inode_invalidate (inode);
out:
if (data)
@@ -2580,8 +2625,64 @@ out:
return 0;
}
+void
+_afr_cleanup_fd_ctx (afr_fd_ctx_t *fd_ctx)
+{
+ int i = 0;
+
+
+ for (i = 0; i < AFR_NUM_CHANGE_LOGS; i++)
+ GF_FREE (fd_ctx->pre_op_done[i]);
+
+ GF_FREE (fd_ctx->opened_on);
+
+ GF_FREE (fd_ctx->lock_piggyback);
+
+ GF_FREE (fd_ctx->lock_acquired);
+
+ pthread_mutex_destroy (&fd_ctx->delay_lock);
+
+ GF_FREE (fd_ctx);
+
+ return;
+}
+
+int
+afr_cleanup_fd_ctx (xlator_t *this, fd_t *fd)
+{
+ uint64_t ctx = 0;
+ afr_fd_ctx_t *fd_ctx = NULL;
+ int ret = 0;
+
+ ret = fd_ctx_get (fd, this, &ctx);
+ if (ret < 0)
+ goto out;
+
+ fd_ctx = (afr_fd_ctx_t *)(long) ctx;
+
+ if (fd_ctx) {
+ /*no need to take any locks*/
+ if (!list_empty (&fd_ctx->eager_locked))
+ gf_msg (this->name, GF_LOG_WARNING, 0,
+ AFR_MSG_INVALID_DATA, "%s: Stale "
+ "Eager-lock stubs found",
+ uuid_utoa (fd->inode->gfid));
+
+ _afr_cleanup_fd_ctx (fd_ctx);
+
+ }
+
+out:
+ return 0;
+}
+
+int
+afr_release (xlator_t *this, fd_t *fd)
+{
+ afr_cleanup_fd_ctx (this, fd);
-/* {{{ open */
+ return 0;
+}
afr_fd_ctx_t *
__afr_fd_ctx_get (fd_t *fd, xlator_t *this)
@@ -2649,6 +2750,13 @@ __afr_fd_ctx_set (xlator_t *this, fd_t *fd)
goto out;
}
+ ret = pthread_mutex_init (&fd_ctx->delay_lock, NULL);
+ if (ret) {
+ GF_FREE (fd_ctx);
+ fd_ctx = NULL;
+ goto out;
+ }
+
for (i = 0; i < AFR_NUM_CHANGE_LOGS; i++) {
fd_ctx->pre_op_done[i] = GF_CALLOC (sizeof (*fd_ctx->pre_op_done[i]),
priv->child_count,
@@ -2692,8 +2800,6 @@ __afr_fd_ctx_set (xlator_t *this, fd_t *fd)
fd_ctx->readdir_subvol = -1;
- pthread_mutex_init (&fd_ctx->delay_lock, NULL);
-
INIT_LIST_HEAD (&fd_ctx->eager_locked);
ret = __fd_ctx_set (fd, this, (uint64_t)(long) fd_ctx);
@@ -2701,24 +2807,12 @@ __afr_fd_ctx_set (xlator_t *this, fd_t *fd)
gf_msg_debug (this->name, 0,
"failed to set fd ctx (%p)", fd);
out:
+ if (ret && fd_ctx)
+ _afr_cleanup_fd_ctx (fd_ctx);
return ret;
}
-int
-afr_fd_ctx_set (xlator_t *this, fd_t *fd)
-{
- int ret = -1;
-
- LOCK (&fd->lock);
- {
- ret = __afr_fd_ctx_set (this, fd);
- }
- UNLOCK (&fd->lock);
-
- return ret;
-}
-
/* {{{ flush */
int
@@ -2812,56 +2906,6 @@ out:
/* }}} */
-int
-afr_cleanup_fd_ctx (xlator_t *this, fd_t *fd)
-{
- uint64_t ctx = 0;
- afr_fd_ctx_t *fd_ctx = NULL;
- int ret = 0;
- int i = 0;
-
- ret = fd_ctx_get (fd, this, &ctx);
- if (ret < 0)
- goto out;
-
- fd_ctx = (afr_fd_ctx_t *)(long) ctx;
-
- if (fd_ctx) {
- //no need to take any locks
- if (!list_empty (&fd_ctx->eager_locked))
- gf_msg (this->name, GF_LOG_WARNING, 0,
- AFR_MSG_INVALID_DATA, "%s: Stale "
- "Eager-lock stubs found",
- uuid_utoa (fd->inode->gfid));
-
- for (i = 0; i < AFR_NUM_CHANGE_LOGS; i++)
- GF_FREE (fd_ctx->pre_op_done[i]);
-
- GF_FREE (fd_ctx->opened_on);
-
- GF_FREE (fd_ctx->lock_piggyback);
-
- GF_FREE (fd_ctx->lock_acquired);
-
- pthread_mutex_destroy (&fd_ctx->delay_lock);
-
- GF_FREE (fd_ctx);
- }
-
-out:
- return 0;
-}
-
-
-int
-afr_release (xlator_t *this, fd_t *fd)
-{
- afr_cleanup_fd_ctx (this, fd);
-
- return 0;
-}
-
-
/* {{{ fsync */
int
diff --git a/xlators/cluster/afr/src/afr-inode-read.c b/xlators/cluster/afr/src/afr-inode-read.c
index 1690cb684dd..2b369ca3c68 100644
--- a/xlators/cluster/afr/src/afr-inode-read.c
+++ b/xlators/cluster/afr/src/afr-inode-read.c
@@ -615,10 +615,9 @@ unlock:
goto unwind;
}
- unwind:
- // Updating child_errno with more recent 'events'
op_errno = afr_final_errno (local, priv);
+unwind:
AFR_STACK_UNWIND (fgetxattr, frame, op_ret, op_errno, xattr,
xdata);
if (xattr)
@@ -702,10 +701,9 @@ unlock:
goto unwind;
}
- unwind:
- // Updating child_errno with more recent 'events'
op_errno = afr_final_errno (local, priv);
+unwind:
AFR_STACK_UNWIND (getxattr, frame, op_ret, op_errno, xattr, xdata);
if (xattr)
diff --git a/xlators/cluster/afr/src/afr-self-heal-common.c b/xlators/cluster/afr/src/afr-self-heal-common.c
index 0ad4b644c42..52e0c75d73e 100644
--- a/xlators/cluster/afr/src/afr-self-heal-common.c
+++ b/xlators/cluster/afr/src/afr-self-heal-common.c
@@ -1839,7 +1839,10 @@ afr_selfheal_do (call_frame_t *frame, xlator_t *this, uuid_t gfid)
gf_boolean_t dataheal_enabled = _gf_false;
priv = this->private;
- gf_string2boolean (priv->data_self_heal, &dataheal_enabled);
+
+ ret = gf_string2boolean (priv->data_self_heal, &dataheal_enabled);
+ if (ret)
+ goto out;
ret = afr_selfheal_unlocked_inspect (frame, this, gfid, &inode,
&data_selfheal,
diff --git a/xlators/cluster/afr/src/afr-self-heal-data.c b/xlators/cluster/afr/src/afr-self-heal-data.c
index 2a33e53764c..4becfb835e8 100644
--- a/xlators/cluster/afr/src/afr-self-heal-data.c
+++ b/xlators/cluster/afr/src/afr-self-heal-data.c
@@ -89,9 +89,15 @@ __afr_can_skip_data_block_heal (call_frame_t *frame, xlator_t *this, fd_t *fd,
priv = this->private;
local = frame->local;
+
xdata = dict_new();
- if (xdata)
- i = dict_set_int32 (xdata, "check-zero-filled", 1);
+ if (!xdata)
+ goto out;
+ if (dict_set_int32 (xdata, "check-zero-filled", 1)) {
+ dict_unref (xdata);
+ goto out;
+ }
+
wind_subvols = alloca0 (priv->child_count);
for (i = 0; i < priv->child_count; i++) {
if (i == source || healed_sinks[i])
@@ -130,7 +136,7 @@ __afr_can_skip_data_block_heal (call_frame_t *frame, xlator_t *this, fd_t *fd,
else
return _gf_true;
}
-
+out:
return _gf_false;
}
diff --git a/xlators/cluster/afr/src/afr-self-heal-metadata.c b/xlators/cluster/afr/src/afr-self-heal-metadata.c
index 130a3daa203..85aaca7cefd 100644
--- a/xlators/cluster/afr/src/afr-self-heal-metadata.c
+++ b/xlators/cluster/afr/src/afr-self-heal-metadata.c
@@ -80,6 +80,8 @@ __afr_selfheal_metadata_do (call_frame_t *frame, xlator_t *this, inode_t *inode,
afr_delete_ignorable_xattrs (old_xattr);
ret = syncop_removexattr (priv->children[i], &loc, "",
old_xattr, NULL);
+ if (ret)
+ healed_sinks[i] = 0;
}
ret = syncop_setxattr (priv->children[i], &loc, xattr, 0, NULL,
diff --git a/xlators/cluster/afr/src/afr.h b/xlators/cluster/afr/src/afr.h
index 31d761f638d..71247c2c573 100644
--- a/xlators/cluster/afr/src/afr.h
+++ b/xlators/cluster/afr/src/afr.h
@@ -923,9 +923,6 @@ afr_lk_transfer_datalock (call_frame_t *dst, call_frame_t *src, char *dom,
int
__afr_fd_ctx_set (xlator_t *this, fd_t *fd);
-int
-afr_fd_ctx_set (xlator_t *this, fd_t *fd);
-
afr_fd_ctx_t *
afr_fd_ctx_get (fd_t *fd, xlator_t *this);
diff --git a/xlators/cluster/afr/src/pump.c b/xlators/cluster/afr/src/pump.c
index ef299ec5855..d322a9d67b5 100644
--- a/xlators/cluster/afr/src/pump.c
+++ b/xlators/cluster/afr/src/pump.c
@@ -2252,6 +2252,8 @@ init (xlator_t *this)
0, AFR_MSG_CHILD_MISCONFIGURED,
"There should be exactly 2 children - one source "
"and one sink");
+ LOCK_DESTROY (&priv->lock);
+ GF_FREE (priv);
return -1;
}
priv->child_count = child_count;
generated by cgit (git 2.34.1) at 2024-11-18 10:13:35 +0000