From 76017cf65433b7f42e6bfdc2eaddfc36685e2c61 Mon Sep 17 00:00:00 2001 From: Barak Sason Rofman Date: Tue, 7 Jul 2020 20:55:35 +0300 Subject: dht - fixing xattr inconsistency The scenario of setting an xattr to a dir, killing one of the bricks, removing the xattr, bringing back the brick results in xattr inconsistency - The downed brick will still have the xattr, but the rest won't. This patch add a mechanism that will remove the extra xattrs during lookup. This patch is a modification to a previous patch based on comments that were made after merge: https://review.gluster.org/#/c/glusterfs/+/24613/ fixes: #1324 Change-Id: Ifec0b7aea6cd40daa8b0319b881191cf83e031d1 Signed-off-by: Barak Sason Rofman --- libglusterfs/src/common-utils.c | 16 ++++++ libglusterfs/src/glusterfs/common-utils.h | 6 +++ libglusterfs/src/libglusterfs.sym | 1 + tests/bugs/distribute/bug-1600379.t | 54 ++++++++++++++++++++ xlators/cluster/dht/src/dht-common.c | 14 ++---- xlators/cluster/dht/src/dht-common.h | 4 -- xlators/cluster/dht/src/dht-helper.c | 4 ++ xlators/cluster/dht/src/dht-selfheal.c | 11 ++++ xlators/storage/posix/src/posix-helpers.c | 19 +++++++ xlators/storage/posix/src/posix-inode-fd-ops.c | 69 ++++++++++++++++++++++++++ xlators/storage/posix/src/posix.h | 3 ++ 11 files changed, 188 insertions(+), 13 deletions(-) create mode 100644 tests/bugs/distribute/bug-1600379.t diff --git a/libglusterfs/src/common-utils.c b/libglusterfs/src/common-utils.c index 4ca7ada59a1..521073d21c4 100644 --- a/libglusterfs/src/common-utils.c +++ b/libglusterfs/src/common-utils.c @@ -53,6 +53,7 @@ #include "xxhash.h" #include #include "glusterfs/libglusterfs-messages.h" +#include "glusterfs/glusterfs-acl.h" #ifdef __FreeBSD__ #include #undef BIT_SET @@ -78,6 +79,15 @@ char *vol_type_str[] = { typedef int32_t (*rw_op_t)(int32_t fd, char *buf, int32_t size); typedef int32_t (*rwv_op_t)(int32_t fd, const struct iovec *buf, int32_t size); +char *xattrs_to_heal[] = {"user.", + POSIX_ACL_ACCESS_XATTR, + POSIX_ACL_DEFAULT_XATTR, + QUOTA_LIMIT_KEY, + QUOTA_LIMIT_OBJECTS_KEY, + GF_SELINUX_XATTR_KEY, + GF_XATTR_MDATA_KEY, + NULL}; + void gf_xxh64_wrapper(const unsigned char *data, size_t const len, unsigned long long const seed, char *xxh64) @@ -5431,3 +5441,9 @@ gf_syncfs(int fd) #endif return ret; } + +char ** +get_xattrs_to_heal() +{ + return xattrs_to_heal; +} diff --git a/libglusterfs/src/glusterfs/common-utils.h b/libglusterfs/src/glusterfs/common-utils.h index b1a2462ee7e..f05bda1f537 100644 --- a/libglusterfs/src/glusterfs/common-utils.h +++ b/libglusterfs/src/glusterfs/common-utils.h @@ -189,6 +189,12 @@ enum _gf_xlator_ipc_targets { typedef enum _gf_special_pid gf_special_pid_t; typedef enum _gf_xlator_ipc_targets _gf_xlator_ipc_targets_t; +/* Array to hold custom xattr keys */ +extern char *xattrs_to_heal[]; + +char ** +get_xattrs_to_heal(); + /* The DHT file rename operation is not a straightforward rename. * It involves creating linkto and linkfiles, and can unlink or rename the * source file depending on the hashed and cached subvols for the source diff --git a/libglusterfs/src/libglusterfs.sym b/libglusterfs/src/libglusterfs.sym index 70408711c57..00a6e484df6 100644 --- a/libglusterfs/src/libglusterfs.sym +++ b/libglusterfs/src/libglusterfs.sym @@ -1182,3 +1182,4 @@ xlator_is_cleanup_starting gf_nanosleep gf_syncfs graph_total_client_xlator +get_xattrs_to_heal \ No newline at end of file diff --git a/tests/bugs/distribute/bug-1600379.t b/tests/bugs/distribute/bug-1600379.t new file mode 100644 index 00000000000..8d2f6154100 --- /dev/null +++ b/tests/bugs/distribute/bug-1600379.t @@ -0,0 +1,54 @@ +#!/bin/bash + +. $(dirname $0)/../../include.rc +. $(dirname $0)/../../volume.rc + +# Initialize +#------------------------------------------------------------ +cleanup; + +# Start glusterd +TEST glusterd; +TEST pidof glusterd; +TEST $CLI volume info; + +# Create a volume +TEST $CLI volume create $V0 $H0:$B0/${V0}{1,2} + +# Verify volume creation +EXPECT "$V0" volinfo_field $V0 'Volume Name'; +EXPECT 'Created' volinfo_field $V0 'Status'; + +# Start volume and verify successful start +TEST $CLI volume start $V0; +EXPECT 'Started' volinfo_field $V0 'Status'; +TEST glusterfs --volfile-id=$V0 --volfile-server=$H0 --entry-timeout=0 $M0; +#------------------------------------------------------------ + +# Test case - Remove xattr from killed brick on lookup +#------------------------------------------------------------ +# Create a dir and set custom xattr +TEST mkdir $M0/testdir +TEST setfattr -n user.attr -v val $M0/testdir +xattr_val=`getfattr -d $B0/${V0}2/testdir | awk '{print $1}'`; +TEST ${xattr_val}='user.attr="val"'; + +# Kill 2nd brick process +TEST kill_brick $V0 $H0 $B0/${V0}2 +EXPECT_WITHIN ${PROCESS_UP_TIMEOUT} "1" online_brick_count + +# Remove custom xattr +TEST setfattr -x user.attr $M0/testdir + +# Bring up the killed brick process +TEST $CLI volume start $V0 force + +# Perform lookup +sleep 5 +TEST ls $M0/testdir + +# Check brick xattrs +xattr_val_2=`getfattr -d $B0/${V0}2/testdir`; +TEST [ ${xattr_val_2} = ''] ; + +cleanup; diff --git a/xlators/cluster/dht/src/dht-common.c b/xlators/cluster/dht/src/dht-common.c index 6a649ac4f56..65c9c0b0a31 100644 --- a/xlators/cluster/dht/src/dht-common.c +++ b/xlators/cluster/dht/src/dht-common.c @@ -17,6 +17,7 @@ #include #include #include "glusterfs/compat-errno.h" // for ENODATA on BSD +#include #include #include @@ -43,15 +44,6 @@ dht_common_mark_mdsxattr_cbk(call_frame_t *frame, void *cookie, xlator_t *this, static int dht_rmdir_unlock(call_frame_t *frame, xlator_t *this); -char *xattrs_to_heal[] = {"user.", - POSIX_ACL_ACCESS_XATTR, - POSIX_ACL_DEFAULT_XATTR, - QUOTA_LIMIT_KEY, - QUOTA_LIMIT_OBJECTS_KEY, - GF_SELINUX_XATTR_KEY, - GF_XATTR_MDATA_KEY, - NULL}; - static const char *dht_dbg_vxattrs[] = {DHT_DBG_HASHED_SUBVOL_PATTERN, NULL}; /* Check the xdata to make sure EBADF has been set by client xlator */ @@ -84,6 +76,8 @@ dht_set_fixed_dir_stat(struct iatt *stat) static gf_boolean_t dht_match_xattr(const char *key) { + char **xattrs_to_heal = get_xattrs_to_heal(); + return gf_get_index_by_elem(xattrs_to_heal, (char *)key) >= 0; } @@ -5460,11 +5454,13 @@ dht_dir_common_set_remove_xattr(call_frame_t *frame, xlator_t *this, loc_t *loc, int call_cnt = 0; dht_local_t *local = NULL; char gfid_local[GF_UUID_BUF_SIZE] = {0}; + char **xattrs_to_heal; conf = this->private; local = frame->local; call_cnt = conf->subvolume_cnt; local->flags = flags; + xattrs_to_heal = get_xattrs_to_heal(); if (!gf_uuid_is_null(local->gfid)) { gf_uuid_unparse(local->gfid, gfid_local); diff --git a/xlators/cluster/dht/src/dht-common.h b/xlators/cluster/dht/src/dht-common.h index 8d539f1e360..028c6ac6b9f 100644 --- a/xlators/cluster/dht/src/dht-common.h +++ b/xlators/cluster/dht/src/dht-common.h @@ -52,10 +52,6 @@ #define DHT_DBG_HASHED_SUBVOL_PATTERN "dht.file.hashed-subvol.*" #define DHT_DBG_HASHED_SUBVOL_KEY "dht.file.hashed-subvol." -/* Array to hold custom xattr keys - */ -extern char *xattrs_to_heal[]; - /* Rebalance nodeuuid flags */ #define REBAL_NODEUUID_MINE 0x01 diff --git a/xlators/cluster/dht/src/dht-helper.c b/xlators/cluster/dht/src/dht-helper.c index 632c9b97970..3f2fe43d5f3 100644 --- a/xlators/cluster/dht/src/dht-helper.c +++ b/xlators/cluster/dht/src/dht-helper.c @@ -2267,6 +2267,7 @@ dht_dir_set_heal_xattr(xlator_t *this, dht_local_t *local, dict_t *dst, int luret = -1; int luflag = -1; int i = 0; + char **xattrs_to_heal; if (!src || !dst) { gf_smsg(this->name, GF_LOG_WARNING, EINVAL, DHT_MSG_DST_NULL_SET_FAILED, @@ -2281,6 +2282,9 @@ dht_dir_set_heal_xattr(xlator_t *this, dht_local_t *local, dict_t *dst, and set it to dst dict, here index start from 1 because user xattr already checked in previous statement */ + + xattrs_to_heal = get_xattrs_to_heal(); + for (i = 1; xattrs_to_heal[i]; i++) { keyval = dict_get(src, xattrs_to_heal[i]); if (keyval) { diff --git a/xlators/cluster/dht/src/dht-selfheal.c b/xlators/cluster/dht/src/dht-selfheal.c index 6004ab09ff6..1b6571cd43c 100644 --- a/xlators/cluster/dht/src/dht-selfheal.c +++ b/xlators/cluster/dht/src/dht-selfheal.c @@ -2156,6 +2156,15 @@ dht_dir_heal_xattrs(void *data) if (subvol == mds_subvol) continue; if (uret || uflag) { + /* Custom xattr heal is required - let posix handle it */ + ret = dict_set_int8(xdata, "sync_backend_xattrs", _gf_true); + if (ret) { + gf_smsg(this->name, GF_LOG_WARNING, 0, DHT_MSG_DICT_SET_FAILED, + "path=%s", local->loc.path, "key=%s", + "sync_backend_xattrs", NULL); + goto out; + } + ret = syncop_setxattr(subvol, &local->loc, user_xattr, 0, xdata, NULL); if (ret) { @@ -2164,6 +2173,8 @@ dht_dir_heal_xattrs(void *data) DHT_MSG_DIR_XATTR_HEAL_FAILED, "set-user-xattr-failed path=%s", local->loc.path, "subvol=%s", subvol->name, "gfid=%s", gfid, NULL); + } else { + dict_del(xdata, "sync_backend_xattrs"); } } } diff --git a/xlators/storage/posix/src/posix-helpers.c b/xlators/storage/posix/src/posix-helpers.c index 7f69afedf1f..bb4a5309f45 100644 --- a/xlators/storage/posix/src/posix-helpers.c +++ b/xlators/storage/posix/src/posix-helpers.c @@ -3555,3 +3555,22 @@ out: return is_stale; } + +/* Delete user xattr from the file at the file-path specified by data and from + * dict */ +int +posix_delete_user_xattr(dict_t *dict, char *k, data_t *v, void *data) +{ + int ret; + char *real_path = data; + + ret = sys_lremovexattr(real_path, k); + if (ret) { + gf_msg("posix-helpers", GF_LOG_ERROR, P_MSG_XATTR_NOT_REMOVED, errno, + "removexattr failed. key %s path %s", k, real_path); + } + + dict_del(dict, k); + + return ret; +} diff --git a/xlators/storage/posix/src/posix-inode-fd-ops.c b/xlators/storage/posix/src/posix-inode-fd-ops.c index 904dbe57578..762041b5831 100644 --- a/xlators/storage/posix/src/posix-inode-fd-ops.c +++ b/xlators/storage/posix/src/posix-inode-fd-ops.c @@ -54,6 +54,7 @@ #include #include "posix-gfid-path.h" #include +#include extern char *marker_xattrs[]; #define ALIGN_SIZE 4096 @@ -2708,6 +2709,7 @@ posix_setxattr(call_frame_t *frame, xlator_t *this, loc_t *loc, dict_t *dict, int32_t ret = 0; ssize_t acl_size = 0; dict_t *xattr = NULL; + dict_t *subvol_xattrs = NULL; posix_xattr_filler_t filler = { 0, }; @@ -2723,6 +2725,10 @@ posix_setxattr(call_frame_t *frame, xlator_t *this, loc_t *loc, dict_t *dict, struct mdata_iatt mdata_iatt = { 0, }; + int8_t sync_backend_xattrs = _gf_false; + data_pair_t *custom_xattrs; + data_t *keyval = NULL; + char **xattrs_to_heal = get_xattrs_to_heal(); DECLARE_OLD_FS_ID_VAR; SET_FS_ID(frame->root->uid, frame->root->gid); @@ -2905,6 +2911,66 @@ posix_setxattr(call_frame_t *frame, xlator_t *this, loc_t *loc, dict_t *dict, goto out; } + ret = dict_get_int8(xdata, "sync_backend_xattrs", &sync_backend_xattrs); + if (ret) { + gf_msg_debug(this->name, -ret, "Unable to get sync_backend_xattrs"); + } + + if (sync_backend_xattrs) { + /* List all custom xattrs */ + subvol_xattrs = dict_new(); + if (!subvol_xattrs) + goto out; + + ret = dict_set_int32_sizen(xdata, "list-xattr", 1); + if (ret) { + gf_msg(this->name, GF_LOG_ERROR, 0, ENOMEM, + "Unable to set list-xattr in dict "); + goto out; + } + + subvol_xattrs = posix_xattr_fill(this, real_path, loc, NULL, -1, xdata, + NULL); + + /* Remove all user xattrs from the file */ + dict_foreach_fnmatch(subvol_xattrs, "user.*", posix_delete_user_xattr, + real_path); + + /* Remove all custom xattrs from the file */ + for (i = 1; xattrs_to_heal[i]; i++) { + keyval = dict_get(subvol_xattrs, xattrs_to_heal[i]); + if (keyval) { + ret = sys_lremovexattr(real_path, xattrs_to_heal[i]); + if (ret) { + gf_msg(this->name, GF_LOG_ERROR, P_MSG_XATTR_NOT_REMOVED, + errno, "removexattr failed. key %s path %s", + xattrs_to_heal[i], loc->path); + goto out; + } + + dict_del(subvol_xattrs, xattrs_to_heal[i]); + keyval = NULL; + } + } + + /* Set custom xattrs based on info provided by DHT */ + custom_xattrs = dict->members_list; + + while (custom_xattrs != NULL) { + ret = sys_lsetxattr(real_path, custom_xattrs->key, + custom_xattrs->value->data, + custom_xattrs->value->len, flags); + if (ret) { + op_errno = errno; + gf_log(this->name, GF_LOG_ERROR, "setxattr failed - %s %d", + custom_xattrs->key, ret); + goto out; + } + + custom_xattrs = custom_xattrs->next; + } + } + xattr = dict_new(); if (!xattr) goto out; @@ -3012,6 +3078,9 @@ out: if (xattr) dict_unref(xattr); + if (subvol_xattrs) + dict_unref(subvol_xattrs); + return 0; } diff --git a/xlators/storage/posix/src/posix.h b/xlators/storage/posix/src/posix.h index 662b5c69f8a..35be197c869 100644 --- a/xlators/storage/posix/src/posix.h +++ b/xlators/storage/posix/src/posix.h @@ -668,4 +668,7 @@ posix_update_iatt_buf(struct iatt *buf, int fd, char *loc, dict_t *xdata); gf_boolean_t posix_is_layout_stale(dict_t *xdata, char *par_path, xlator_t *this); +int +posix_delete_user_xattr(dict_t *dict, char *k, data_t *v, void *data); + #endif /* _POSIX_H */ -- cgit