From 9a3de81fe5c42c0495dccc5877cecbc2edb81f3c Mon Sep 17 00:00:00 2001 From: Susant Palai Date: Tue, 18 Feb 2014 13:03:50 +0000 Subject: DHT/Rebalance : Hard link Migration Failure Probelm : __is_file_migratable used to return ENOTSUP for all the cases. Hence, it will add to the failure count. And the remove-brick status will show failure for all the files. Solution : Added 'ret = -2' to gf_defrag_handle_hardlink to be deemed as success. Otherwise dht_migrate_file will try to migrate each of the hard link, which not intended. Change-Id: Iff74f6634fb64e4b91fc5d016e87ff1290b7a0d6 BUG: 1066798 Signed-off-by: Susant Palai Reviewed-on: http://review.gluster.org/7124 Reviewed-by: Raghavendra G Tested-by: Gluster Build System Reviewed-by: Vijay Bellur --- tests/bugs/bug-1066798.t | 86 +++++++++++++++++++++++++++++++++ xlators/cluster/dht/src/dht-rebalance.c | 62 +++++++++++++++++++++--- 2 files changed, 142 insertions(+), 6 deletions(-) create mode 100755 tests/bugs/bug-1066798.t diff --git a/tests/bugs/bug-1066798.t b/tests/bugs/bug-1066798.t new file mode 100755 index 000000000..635b143f0 --- /dev/null +++ b/tests/bugs/bug-1066798.t @@ -0,0 +1,86 @@ +#!/bin/bash + +. $(dirname $0)/../include.rc +. $(dirname $0)/../volume.rc + +cleanup; + +TESTS_EXPECTED_IN_LOOP=200 + +## Start glusterd +TEST glusterd; +TEST pidof glusterd; +TEST $CLI volume info; + +## Lets create volume +TEST $CLI volume create $V0 $H0:$B0/${V0}{1,2}; + +## Verify volume is created +EXPECT "$V0" volinfo_field $V0 'Volume Name'; +EXPECT 'Created' volinfo_field $V0 'Status'; + +## Start volume and verify +TEST $CLI volume start $V0; +EXPECT 'Started' volinfo_field $V0 'Status'; +TEST glusterfs -s $H0 --volfile-id=$V0 $M0 + +############################################################ +#TEST_PLAN# +#Create a file +#Store the hashed brick information +#Create hard links to it +#Remove the hashed brick +#Check now all the hardlinks are migrated in to "OTHERBRICK" +#Check also in mount point for all the files +#check there is no failures and skips for migration +############################################################ + +TEST touch $M0/file1; + +file_perm=`ls -l $M0/file1 | grep file1 | awk '{print $1}'`; + +if [ -f $B0/${V0}1/file1 ] +then + HASHED=$B0/${V0}1 + OTHER=$B0/${V0}2 +else + HASHED=$B0/${V0}2 + OTHER=$B0/${V0}1 +fi + +#create hundred hard links +for i in {1..50}; +do +TEST_IN_LOOP ln $M0/file1 $M0/link$i; +done + + +TEST $CLI volume remove-brick $V0 $H0:${HASHED} start +EXPECT_WITHIN 20 "completed" remove_brick_status_completed_field "$V0" "$H0:${HASHED}"; + +#check consistency in mount point +#And also check all the links are migrated to OTHER +for i in {1..50} +do +TEST_IN_LOOP [ -f ${OTHER}/link${i} ]; +TEST_IN_LOOP [ -f ${M0}/link${i} ]; +done; + +#check in OTHER that all the files has proper permission (Means no +#linkto files) + +for i in {1..50} +do +link_perm=`ls -l $OTHER | grep -w link${i} | awk '{print $1}'`; +TEST_IN_LOOP [ "${file_perm}" == "${link_perm}" ] + +done + +#check that remove-brick status should not have any failed or skipped files + +var=`$CLI volume remove-brick $V0 $H0:${HASHED} status | grep completed` + +TEST [ `echo $var | awk '{print $5}'` = "0" ] +TEST [ `echo $var | awk '{print $6}'` = "0" ] + +cleanup diff --git a/xlators/cluster/dht/src/dht-rebalance.c b/xlators/cluster/dht/src/dht-rebalance.c index a17319ba6..4f78f5203 100644 --- a/xlators/cluster/dht/src/dht-rebalance.c +++ b/xlators/cluster/dht/src/dht-rebalance.c @@ -94,6 +94,41 @@ out: } +/* + return values: + -1 : failure + -2 : success + +Hard link migration is carried out in three stages. + +(Say there are n hardlinks) +Stage 1: Setting the new hashed subvol information on the 1st hardlink + encountered (linkto setxattr) + +Stage 2: Creating hardlinks on new hashed subvol for the 2nd to (n-1)th + hardlink + +Stage 3: Physical migration of the data file for nth hardlink + +Why to deem "-2" as success and not "0": + + dht_migrate_file expects return value "0" from _is_file_migratable if +the file has to be migrated. + + _is_file_migratable returns zero only when it is called with the +flag "GF_DHT_MIGRATE_HARDLINK_IN_PROGRESS". + + gf_defrag_handle_hardlink calls dht_migrate_file for physical migration +of the data file with the flag "GF_DHT_MIGRATE_HARDLINK_IN_PROGRESS" + +Hence, gf_defrag_handle_hardlink returning "0" for success will force +"dht_migrate_file" to migrate each of the hardlink which is not intended. + +For each of the three stage mentioned above "-2" will be returned and will +be converted to "0" in dht_migrate_file. + +*/ + int32_t gf_defrag_handle_hardlink (xlator_t *this, loc_t *loc, dict_t *xattrs, struct iatt *stbuf) @@ -164,6 +199,7 @@ gf_defrag_handle_hardlink (xlator_t *this, loc_t *loc, dict_t *xattrs, ret = -1; goto out; } + ret = -2; goto out; } else { linkto_subvol = dht_linkfile_subvol (this, NULL, NULL, xattrs); @@ -200,12 +236,19 @@ gf_defrag_handle_hardlink (xlator_t *this, loc_t *loc, dict_t *xattrs, if (ret) goto out; } - ret = 0; + ret = -2; out: return ret; } - +/* + return values + 0 : File will be migrated + -2 : File will not be migrated + (This is the return value from gf_defrag_handle_hardlink. Checkout + gf_defrag_handle_hardlink for description of "returning -2") + -1 : failure +*/ static inline int __is_file_migratable (xlator_t *this, loc_t *loc, struct iatt *stbuf, dict_t *xattrs, int flags) @@ -228,7 +271,12 @@ __is_file_migratable (xlator_t *this, loc_t *loc, if (flags == GF_DHT_MIGRATE_HARDLINK) { ret = gf_defrag_handle_hardlink (this, loc, xattrs, stbuf); - if (ret) { + + /* + Returning zero will force the file to be remigrated. + Checkout gf_defrag_handle_hardlink for more information. + */ + if (ret && ret != -2) { gf_log (this->name, GF_LOG_WARNING, "%s: failed to migrate file with link", loc->path); @@ -236,8 +284,8 @@ __is_file_migratable (xlator_t *this, loc_t *loc, } else { gf_log (this->name, GF_LOG_WARNING, "%s: file has hardlinks", loc->path); + ret = -ENOTSUP; } - ret = ENOTSUP; goto out; } @@ -743,9 +791,11 @@ dht_migrate_file (xlator_t *this, loc_t *loc, xlator_t *from, xlator_t *to, /* Check if file can be migrated */ ret = __is_file_migratable (this, loc, &stbuf, xattr_rsp, flag); - if (ret) + if (ret) { + if (ret == -2) + ret = 0; goto out; - + } /* Take care of the special files */ if (!IA_ISREG (stbuf.ia_type)) { /* Special files */ -- cgit