From 2389042e64ae20f88bd0e53d1fe879dc2856b6c4 Mon Sep 17 00:00:00 2001 From: Jeff Darcy Date: Fri, 12 Oct 2012 10:18:58 -0400 Subject: replicate: don't stop checking xattrs because one was absent The functional issue is described by the subject line. This patch also addresses several efficiency/structure issues, such as... * Calling dict_set_ptr once for each txn type, instead of once overall. * Calling afr_index_for_transaction_type once per iteration instead of once per call (or better yet zero since the conversion is unnecessary). * Implementation of inner functions in a different file than their one caller, creating a spurious header-file dependency. Change-Id: I29e0df906a820533b66b9ced73e015dfe77267d2 BUG: 865825 Signed-off-by: Jeff Darcy Reviewed-on: http://review.gluster.org/4070 Tested-by: Gluster Build System Reviewed-by: Pranith Kumar Karampuri Reviewed-by: Anand Avati --- tests/bugs/bug-865825.t | 74 +++++++++++++++++++++ xlators/cluster/afr/src/afr-common.c | 56 +++++++++++----- xlators/cluster/afr/src/afr-self-heal-common.c | 92 -------------------------- xlators/cluster/afr/src/afr-self-heal.h | 7 -- 4 files changed, 115 insertions(+), 114 deletions(-) create mode 100755 tests/bugs/bug-865825.t diff --git a/tests/bugs/bug-865825.t b/tests/bugs/bug-865825.t new file mode 100755 index 000000000..82e1476f7 --- /dev/null +++ b/tests/bugs/bug-865825.t @@ -0,0 +1,74 @@ +#!/bin/bash + +. $(dirname $0)/../include.rc + +TEST glusterd +TEST pidof glusterd +TEST $CLI volume info; + +## Start and create a volume +mkdir -p ${B0}/${V0}-0 +mkdir -p ${B0}/${V0}-1 +mkdir -p ${B0}/${V0}-2 +TEST $CLI volume create $V0 replica 3 $H0:$B0/${V0}-{0,1,2} + +function volinfo_field() +{ + local vol=$1; + local field=$2; + + $CLI volume info $vol | grep "^$field: " | sed 's/.*: //'; +} + + +## Verify volume is created +EXPECT "$V0" volinfo_field $V0 'Volume Name'; +EXPECT 'Created' volinfo_field $V0 'Status'; + +## Make sure io-cache and write-behind don't interfere. +TEST $CLI volume set $V0 performance.io-cache off; +TEST $CLI volume set $V0 performance.write-behind off; + +## Make sure automatic self-heal doesn't perturb our results. +TEST $CLI volume set $V0 cluster.self-heal-daemon off + +## Start volume and verify +TEST $CLI volume start $V0; +EXPECT 'Started' volinfo_field $V0 'Status'; + +## Mount native +TEST glusterfs --volfile-server=$H0 --volfile-id=$V0 $M0 + +## Create a file with some recognizable contents. +echo "test_data" > $M0/a_file; + +## Unmount. +TEST umount $M0; + +## Mess with the flags as though brick-0 accuses brick-2 while brick-1 is +## missing its brick-2 changelog altogether. +value=0x000000010000000000000000 +setfattr -n trusted.afr.${V0}-client-2 -v $value $B0/${V0}-0/a_file +setfattr -x trusted.afr.${V0}-client-2 $B0/${V0}-1/a_file +echo "wrong_data" > $B0/${V0}-2/a_file + +## Remount and force a self-heal. +TEST glusterfs --volfile-server=$H0 --volfile-id=$V0 $M0 +stat ${M0}/a_file > /dev/null + +## Make sure brick 2 now has the correct contents. +EXPECT "test_data" cat $B0/${V0}-2/a_file + +if [ "$EXIT_EARLY" = "1" ]; then + exit 0; +fi + +## Finish up +TEST umount $M0; +TEST $CLI volume stop $V0; +EXPECT 'Stopped' volinfo_field $V0 'Status'; + +TEST $CLI volume delete $V0; +TEST ! $CLI volume info $V0; + +cleanup; diff --git a/xlators/cluster/afr/src/afr-common.c b/xlators/cluster/afr/src/afr-common.c index 693c3a070..a1dd4a5ce 100644 --- a/xlators/cluster/afr/src/afr-common.c +++ b/xlators/cluster/afr/src/afr-common.c @@ -1157,31 +1157,57 @@ afr_lookup_update_lk_counts (afr_local_t *local, xlator_t *this, local->cont.lookup.parent_entrylk += parent_entrylk; } +/* + * It's important to maintain a commutative property on do_*_self_heal and + * found*; once set, they must not be cleared by a subsequent iteration or + * call, so that they represent a logical OR of all iterations and calls + * regardless of child/key order. That allows the caller to call us multiple + * times without having to use a separate variable as a "reduce" accumulator. + */ static void afr_lookup_set_self_heal_params_by_xattr (afr_local_t *local, xlator_t *this, dict_t *xattr) { + afr_private_t *priv = NULL; + int i = 0; + int ret = -1; + void *pending_raw = NULL; + int32_t *pending = NULL; + GF_ASSERT (local); GF_ASSERT (this); GF_ASSERT (xattr); - if (afr_sh_has_metadata_pending (xattr, this)) { - local->self_heal.do_metadata_self_heal = _gf_true; - gf_log(this->name, GF_LOG_DEBUG, - "metadata self-heal is pending for %s.", - local->loc.path); - } + priv = this->private; - if (afr_sh_has_entry_pending (xattr, this)) { - local->self_heal.do_entry_self_heal = _gf_true; - gf_log(this->name, GF_LOG_DEBUG, - "entry self-heal is pending for %s.", local->loc.path); - } + for (i = 0; i < priv->child_count; i++) { + ret = dict_get_ptr (xattr, priv->pending_key[i], + &pending_raw); + if (ret != 0) { + continue; + } + pending = pending_raw; - if (afr_sh_has_data_pending (xattr, this)) { - local->self_heal.do_data_self_heal = _gf_true; - gf_log(this->name, GF_LOG_DEBUG, - "data self-heal is pending for %s.", local->loc.path); + if (pending[AFR_METADATA_TRANSACTION]) { + gf_log(this->name, GF_LOG_DEBUG, + "metadata self-heal is pending for %s.", + local->loc.path); + local->self_heal.do_metadata_self_heal = _gf_true; + } + + if (pending[AFR_ENTRY_TRANSACTION]) { + gf_log(this->name, GF_LOG_DEBUG, + "entry self-heal is pending for %s.", + local->loc.path); + local->self_heal.do_entry_self_heal = _gf_true; + } + + if (pending[AFR_DATA_TRANSACTION]) { + gf_log(this->name, GF_LOG_DEBUG, + "data self-heal is pending for %s.", + local->loc.path); + local->self_heal.do_data_self_heal = _gf_true; + } } } diff --git a/xlators/cluster/afr/src/afr-self-heal-common.c b/xlators/cluster/afr/src/afr-self-heal-common.c index 49484bf7b..07603c5b2 100644 --- a/xlators/cluster/afr/src/afr-self-heal-common.c +++ b/xlators/cluster/afr/src/afr-self-heal-common.c @@ -903,98 +903,6 @@ afr_sh_delta_to_xattr (xlator_t *this, } -int -afr_sh_has_metadata_pending (dict_t *xattr, xlator_t *this) -{ - /* Indexable by result of afr_index_for_transaction_type(): 0 -- 2. */ - int32_t pending[3] = {0,}; - void *pending_raw = NULL; - afr_private_t *priv = NULL; - int ret = -1; - int i = 0; - int j = 0; - - priv = this->private; - - for (i = 0; i < priv->child_count; i++) { - ret = dict_get_ptr (xattr, priv->pending_key[i], - &pending_raw); - - if (ret != 0) - return 0; - - memcpy (pending, pending_raw, sizeof(pending)); - j = afr_index_for_transaction_type (AFR_METADATA_TRANSACTION); - - if (pending[j]) - return 1; - } - - return 0; -} - - -int -afr_sh_has_data_pending (dict_t *xattr, xlator_t *this) -{ - /* Indexable by result of afr_index_for_transaction_type(): 0 -- 2. */ - int32_t pending[3] = {0,}; - void *pending_raw = NULL; - afr_private_t *priv = NULL; - int ret = -1; - int i = 0; - int j = 0; - - priv = this->private; - - for (i = 0; i < priv->child_count; i++) { - ret = dict_get_ptr (xattr, priv->pending_key[i], - &pending_raw); - - if (ret != 0) - return 0; - - memcpy (pending, pending_raw, sizeof(pending)); - j = afr_index_for_transaction_type (AFR_DATA_TRANSACTION); - - if (pending[j]) - return 1; - } - - return 0; -} - - -int -afr_sh_has_entry_pending (dict_t *xattr, xlator_t *this) -{ - /* Indexable by result of afr_index_for_transaction_type(): 0 -- 2. */ - int32_t pending[3] = {0,}; - void *pending_raw = NULL; - afr_private_t *priv = NULL; - int ret = -1; - int i = 0; - int j = 0; - - priv = this->private; - - for (i = 0; i < priv->child_count; i++) { - ret = dict_get_ptr (xattr, priv->pending_key[i], - &pending_raw); - - if (ret != 0) - return 0; - - memcpy (pending, pending_raw, sizeof(pending)); - j = afr_index_for_transaction_type (AFR_ENTRY_TRANSACTION); - - if (pending[j]) - return 1; - } - - return 0; -} - int afr_sh_missing_entries_done (call_frame_t *frame, xlator_t *this) { diff --git a/xlators/cluster/afr/src/afr-self-heal.h b/xlators/cluster/afr/src/afr-self-heal.h index 2efc1116d..7c9bc8111 100644 --- a/xlators/cluster/afr/src/afr-self-heal.h +++ b/xlators/cluster/afr/src/afr-self-heal.h @@ -20,13 +20,6 @@ #define SIZE_GREATER(buf1,buf2) ((buf1)->ia_size > (buf2)->ia_size) -int -afr_sh_has_metadata_pending (dict_t *xattr, xlator_t *this); -int -afr_sh_has_entry_pending (dict_t *xattr, xlator_t *this); -int -afr_sh_has_data_pending (dict_t *xattr, xlator_t *this); - int afr_self_heal_entry (call_frame_t *frame, xlator_t *this); -- cgit