From: Bruno Faccini Date: Mon, 16 Apr 2018 13:31:28 +0000 (+0200) Subject: LU-10902 mdd: force disable changelogs early and safely X-Git-Tag: 2.11.52~22 X-Git-Url: https://git.whamcloud.com/?p=fs%2Flustre-release.git;a=commitdiff_plain;h=99cc32b013d411335a694c5f94a83cef839c68c4 LU-10902 mdd: force disable changelogs early and safely In mdd_changelog_user_purge() changelogs were disabled too late allowing for new records to be unnecessarily gathered. Also, a race was possible, causing changelogs to be disabled even with registered users. sanity/tests_160h has been added to check race is fixed. Also, changelog_[unregister,clear]() functions in test-framework.sh had to be modified to handle facets in same order than changelog_register() to allow the race simulation to work in DNE environment. Signed-off-by: Bruno Faccini Change-Id: I78f07068e89b1cf2cf0c83dd2068f82f8a4a6dd5 Reviewed-on: https://review.whamcloud.com/32007 Tested-by: Jenkins Reviewed-by: Andreas Dilger Tested-by: Maloo Reviewed-by: Mike Pershin Reviewed-by: Oleg Drokin --- diff --git a/lustre/include/obd_support.h b/lustre/include/obd_support.h index f2314c4..005a958 100644 --- a/lustre/include/obd_support.h +++ b/lustre/include/obd_support.h @@ -525,6 +525,7 @@ extern char obd_jobid_var[]; #define OBD_FAIL_CAT_RECORDS 0x1312 #define OBD_FAIL_CAT_FREE_RECORDS 0x1313 #define OBD_FAIL_TIME_IN_CHLOG_USER 0x1314 +#define CFS_FAIL_CHLOG_USER_REG_UNREG_RACE 0x1315 #define OBD_FAIL_LLITE 0x1400 #define OBD_FAIL_LLITE_FAULT_TRUNC_RACE 0x1401 diff --git a/lustre/mdd/mdd_device.c b/lustre/mdd/mdd_device.c index 91ad2bd..a4273cd 100644 --- a/lustre/mdd/mdd_device.c +++ b/lustre/mdd/mdd_device.c @@ -215,6 +215,7 @@ static int changelog_user_init_cb(const struct lu_env *env, spin_lock(&mdd->mdd_cl.mc_user_lock); mdd->mdd_cl.mc_lastuser = rec->cur_id; + mdd->mdd_cl.mc_users++; if (rec->cur_endrec > mdd->mdd_cl.mc_index) mdd->mdd_cl.mc_index = rec->cur_endrec; spin_unlock(&mdd->mdd_cl.mc_user_lock); @@ -1332,10 +1333,7 @@ static int mdd_changelog_user_register(const struct lu_env *env, RETURN(-ENOMEM); } - /* Assume we want it on since somebody registered */ - rc = mdd_changelog_on(env, mdd); - if (rc) - GOTO(out, rc); + CFS_RACE(CFS_FAIL_CHLOG_USER_REG_UNREG_RACE); rec->cur_hdr.lrh_len = sizeof(*rec); rec->cur_hdr.lrh_type = CHANGELOG_USER_REC; @@ -1346,6 +1344,7 @@ static int mdd_changelog_user_register(const struct lu_env *env, GOTO(out, rc = -EOVERFLOW); } *id = rec->cur_id = ++mdd->mdd_cl.mc_lastuser; + mdd->mdd_cl.mc_users++; rec->cur_endrec = mdd->mdd_cl.mc_index; rec->cur_time = (__u32)get_seconds(); @@ -1355,8 +1354,22 @@ static int mdd_changelog_user_register(const struct lu_env *env, spin_unlock(&mdd->mdd_cl.mc_user_lock); rc = llog_cat_add(env, ctxt->loc_handle, &rec->cur_hdr, NULL); + if (rc) { + CWARN("%s: Failed to register changelog user %d: rc=%d\n", + mdd2obd_dev(mdd)->obd_name, *id, rc); + spin_lock(&mdd->mdd_cl.mc_user_lock); + mdd->mdd_cl.mc_users--; + spin_unlock(&mdd->mdd_cl.mc_user_lock); + GOTO(out, rc); + } CDEBUG(D_IOCTL, "Registered changelog user %d\n", *id); + + /* Assume we want it on since somebody registered */ + rc = mdd_changelog_on(env, mdd); + if (rc) + GOTO(out, rc); + out: OBD_FREE_PTR(rec); llog_ctxt_put(ctxt); @@ -1364,6 +1377,7 @@ out: } struct mdd_changelog_user_purge { + struct mdd_device *mcup_mdd; __u32 mcup_id; __u32 mcup_usercount; __u64 mcup_minrec; @@ -1413,6 +1427,9 @@ static int mdd_changelog_user_purge_cb(const struct lu_env *env, if (rc == 0) { mcup->mcup_found = true; mcup->mcup_usercount--; + spin_lock(&mcup->mcup_mdd->mdd_cl.mc_user_lock); + mcup->mcup_mdd->mdd_cl.mc_users--; + spin_unlock(&mcup->mcup_mdd->mdd_cl.mc_user_lock); } RETURN(rc); @@ -1422,6 +1439,7 @@ int mdd_changelog_user_purge(const struct lu_env *env, struct mdd_device *mdd, __u32 id) { struct mdd_changelog_user_purge mcup = { + .mcup_mdd = mdd, .mcup_id = id, .mcup_found = false, .mcup_usercount = 0, @@ -1445,6 +1463,16 @@ int mdd_changelog_user_purge(const struct lu_env *env, mdd_changelog_user_purge_cb, &mcup, 0, 0); + if ((rc == 0) && (mcup.mcup_usercount == 0)) { + spin_lock(&mdd->mdd_cl.mc_user_lock); + if (mdd->mdd_cl.mc_users == 0) { + /* No more users; turn changelogs off */ + CDEBUG(D_IOCTL, "turning off changelogs\n"); + rc = mdd_changelog_off(env, mdd); + } + spin_unlock(&mdd->mdd_cl.mc_user_lock); + } + if ((rc == 0) && mcup.mcup_found) { CDEBUG(D_IOCTL, "%s: Purging changelog entries for user %d " "record=%llu\n", @@ -1461,11 +1489,7 @@ int mdd_changelog_user_purge(const struct lu_env *env, GOTO(out, rc = -ENOENT); } - if ((rc == 0) && (mcup.mcup_usercount == 0)) { - /* No more users; turn changelogs off */ - CDEBUG(D_IOCTL, "turning off changelogs\n"); - rc = mdd_changelog_off(env, mdd); - } + CFS_RACE(CFS_FAIL_CHLOG_USER_REG_UNREG_RACE); EXIT; out: diff --git a/lustre/mdd/mdd_internal.h b/lustre/mdd/mdd_internal.h index 9af66f0..d23dba7 100644 --- a/lustre/mdd/mdd_internal.h +++ b/lustre/mdd/mdd_internal.h @@ -92,6 +92,7 @@ struct mdd_changelog { ktime_t mc_starttime; spinlock_t mc_user_lock; int mc_lastuser; + int mc_users; /* registered users number */ struct task_struct *mc_gc_task; time64_t mc_gc_time; unsigned int mc_deniednext; /* interval for recording denied diff --git a/lustre/tests/sanity.sh b/lustre/tests/sanity.sh index 4dfed55..29def34 100755 --- a/lustre/tests/sanity.sh +++ b/lustre/tests/sanity.sh @@ -12349,6 +12349,66 @@ test_160g() { } run_test 160g "changelog garbage collect (old users)" +test_160h() { + + local mdts=$(comma_list $(mdts_nodes)) + + changelog_register || error "first changelog_register failed" + + # generate some changelog records to accumulate on each MDT + test_mkdir -c $MDSCOUNT $DIR/$tdir || error "mkdir $tdir failed" + createmany -m $DIR/$tdir/$tfile $((MDSCOUNT * 2)) || + error "create $DIR/$tdir/$tfile failed" + + # check changelogs have been generated + local nbcl=$(changelog_dump | wc -l) + [[ $nbcl -eq 0 ]] && error "no changelogs found" + + # simulate race between register and unregister + # XXX as fail_loc is set per-MDS, with DNE configs the race + # simulation will only occur for one MDT per MDS and for the + # others the normal race scenario will take place + #define CFS_FAIL_CHLOG_USER_REG_UNREG_RACE 0x1315 + do_nodes $mdts $LCTL set_param fail_loc=0x10001315 + do_nodes $mdts $LCTL set_param fail_val=1 + + # unregister 1st user + changelog_deregister & + local pid1=$! + # wait some time for deregister work to reach race rdv + sleep 2 + # register 2nd user + changelog_register || error "2nd user register failed" + + wait $pid1 || error "1st user deregister failed" + + local i + local last_rec + declare -A LAST_REC + for i in $(seq $MDSCOUNT); do + if changelog_users mds$i | grep "^cl"; then + # make sure new records are added with one user present + LAST_REC[mds$i]=$(changelog_users $SINGLEMDS | + awk '/^current.index:/ { print $NF }') + else + error "mds$i has no user registered" + fi + done + + # generate more changelog records to accumulate on each MDT + createmany -m $DIR/$tdir/${tfile}bis $((MDSCOUNT * 2)) || + error "create $DIR/$tdir/${tfile}bis failed" + + for i in $(seq $MDSCOUNT); do + last_rec=$(changelog_users $SINGLEMDS | + awk '/^current.index:/ { print $NF }') + echo "verify changelogs are on: $last_rec != ${LAST_REC[mds$i]}" + [ $last_rec != ${LAST_REC[mds$i]} ] || + error "changelogs are off on mds$i" + done +} +run_test 160h "changelog user register/unregister race" + test_161a() { [ $PARALLEL == "yes" ] && skip "skip parallel run" diff --git a/lustre/tests/test-framework.sh b/lustre/tests/test-framework.sh index c88646d..edac2a1 100755 --- a/lustre/tests/test-framework.sh +++ b/lustre/tests/test-framework.sh @@ -8972,8 +8972,12 @@ changelog_register() { changelog_deregister() { local cl_user + # bash assoc arrays do not guarantee to list keys in created order + # so reorder to get same order than in changelog_register() + local cl_facets=$(echo "${!CL_USERS[@]}" | tr " " "\n" | sort | + tr "\n" " ") - for facet in "${!CL_USERS[@]}"; do + for facet in $cl_facets; do for cl_user in ${CL_USERS[$facet]}; do __changelog_deregister $facet $cl_user || return $? done @@ -9037,7 +9041,12 @@ __changelog_clear() # users. changelog_clear() { local rc - for facet in ${!CL_USERS[@]}; do + # bash assoc arrays do not guarantee to list keys in created order + # so reorder to get same order than in changelog_register() + local cl_facets=$(echo "${!CL_USERS[@]}" | tr " " "\n" | sort | + tr "\n" " ") + + for facet in $cl_facets; do for cl_user in ${CL_USERS[$facet]}; do __changelog_clear $facet $cl_user $1 || rc=${rc:-$?} done