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 <bruno.faccini@intel.com>
Change-Id: I78f07068e89b1cf2cf0c83dd2068f82f8a4a6dd5
Reviewed-on: https://review.whamcloud.com/32007
Tested-by: Jenkins
Reviewed-by: Andreas Dilger <andreas.dilger@intel.com>
Tested-by: Maloo <hpdd-maloo@intel.com>
Reviewed-by: Mike Pershin <mike.pershin@intel.com>
Reviewed-by: Oleg Drokin <oleg.drokin@intel.com>

diff --git a/lustre/include/obd_support.h b/lustre/include/obd_support.h
index f2314c4..005a958 100644 (file)
--- 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 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
 
 #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 (file)
--- 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;
 
        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);
        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);
         }
 
                 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;
 
         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;
                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();
        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);
        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);
 
         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);
 out:
         OBD_FREE_PTR(rec);
         llog_ctxt_put(ctxt);


@@ -1364,6 +1377,7 @@ out:
 }
 
 struct mdd_changelog_user_purge {
 }
 
 struct mdd_changelog_user_purge {

+       struct mdd_device *mcup_mdd;

        __u32 mcup_id;
        __u32 mcup_usercount;
        __u64 mcup_minrec;
        __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--;
        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);
        }
 
        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 = {
                             struct mdd_device *mdd, __u32 id)
 {
        struct mdd_changelog_user_purge mcup = {

+               .mcup_mdd = mdd,

                .mcup_id = id,
                .mcup_found = false,
                .mcup_usercount = 0,
                .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);
 
                              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",
        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);
        }
 
                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:
 
        EXIT;
 out:

diff --git a/lustre/mdd/mdd_internal.h b/lustre/mdd/mdd_internal.h
index 9af66f0..d23dba7 100644 (file)
--- 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;
        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
        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 (executable)
--- a/lustre/tests/sanity.sh
+++ b/lustre/tests/sanity.sh
@@ -12349,6 +12349,66 @@ test_160g() {
 }
 run_test 160g "changelog garbage collect (old users)"
 
 }
 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"
 
 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 (executable)
--- a/lustre/tests/test-framework.sh
+++ b/lustre/tests/test-framework.sh
@@ -8972,8 +8972,12 @@ changelog_register() {
 
 changelog_deregister() {
        local cl_user
 
 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
                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
 # 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
                for cl_user in ${CL_USERS[$facet]}; do
                        __changelog_clear $facet $cl_user $1 || rc=${rc:-$?}
                done