Whamcloud - gitweb
LU-16356 hsm: add running ref to the coordinator 56/51256/20
authorEtienne AUJAMES <etienne.aujames@cea.fr>
Thu, 31 Aug 2023 14:46:20 +0000 (16:46 +0200)
committerOleg Drokin <green@whamcloud.com>
Wed, 8 Nov 2023 21:58:58 +0000 (21:58 +0000)
This patch replaces the fe5706e by adding a reference "cdt_ref" when
the coordinator is running (it does not trust HSM state).
This avoids to de-init the coordinator while still in use (e.g:
thread to add an hsm request) and avoids complex locking on HSM state.

It also causes the coordinator thread to exit if
cdt_start_pending_restore() fails. Otherwise, this can produce a lot
of unexpected behavior (hang, crash).

The patch modifies mdc_kuc_reregister() to register the hsm agent in
background. This make independent reconnect and the agent
registration. It enables to re-activate resend for HSM_CT_REGISTER
without the LU-13455. The coordinator returns EINPROGRESS if not
ready and the client will resend the request for that case. So the
copytools can wait the coordinator to be ready.

Add regression test sanity-hsm 409a.

Fixes: fe5706e ("LU-16235 hsm: check CDT state before adding actions llog")
Fixes: 3d58403 ("LU-13455 ptlrpc: connect to MDT stucks")
Test-Parameters: testlist=sanity-hsm
Test-Parameters: testlist=sanity-hsm env=ONLY=107,ONLY_REPEAT=20
Test-Parameters: testlist=sanity-hsm env=ONLY=409a,ONLY_REPEAT=20
Test-Parameters: testlist=conf-sanity env=ONLY=132,ONLY_REPEAT=20
Signed-off-by: Etienne AUJAMES <eaujames@ddn.com>
Signed-off-by: Nikitas Angelinas <nikitas.angelinas@hpe.com>
Change-Id: I14302d1053abbe76eeaaa1a63c6fd6d9b530baa9
Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/51256
Reviewed-by: Sergey Cheremencev <scherementsev@ddn.com>
Reviewed-by: Oleg Drokin <green@whamcloud.com>
Tested-by: jenkins <devops@whamcloud.com>
Tested-by: Maloo <maloo@whamcloud.com>
lustre/include/uapi/linux/lustre/lustre_user.h
lustre/mdc/mdc_request.c
lustre/mdt/mdt_coordinator.c
lustre/mdt/mdt_hsm.c
lustre/mdt/mdt_hsm_cdt_actions.c
lustre/mdt/mdt_hsm_cdt_agent.c
lustre/mdt/mdt_hsm_cdt_client.c
lustre/mdt/mdt_internal.h
lustre/mdt/mdt_lib.c
lustre/tests/sanity-hsm.sh

index 0c09c76..113dc45 100644 (file)
@@ -2585,11 +2585,6 @@ static inline char *hai_dump_data_field(const struct hsm_action_item *hai,
        return buffer;
 }
 
-enum hal_flags {
-       /* Register even when the CDT is shutdown or being initialized */
-       HAL_CDT_FORCE = 1 << 1,
-};
-
 /* Copytool action list */
 #define HAL_VERSION 1
 #define HAL_MAXSIZE LNET_MTU /* bytes, used in userspace only */
index dec052c..6e501d1 100644 (file)
@@ -1765,6 +1765,7 @@ out:
        ptlrpc_req_finished(req);
        return rc;
 }
+
 /**
  * Send hsm_ct_register to MDS
  *
@@ -1814,7 +1815,6 @@ static int mdc_ioc_hsm_ct_register(struct obd_import *imp, __u32 archive_count,
                *archive_array = archive_count;
 
        ptlrpc_request_set_replen(req);
-       req->rq_no_resend = 1;
 
        rc = mdc_queue_wait(req);
        GOTO(out, rc);
@@ -2496,15 +2496,43 @@ static int mdc_hsm_ct_reregister(void *data, void *cb_arg)
        return (rc == -EEXIST) ? 0 : rc;
 }
 
+static int mdc_kuc_reregister_thread(void *data)
+{
+       struct obd_import *imp = data;
+       int rc;
+       ENTRY;
+
+       /* re-register HSM agents */
+       rc = libcfs_kkuc_group_foreach(&imp->imp_obd->obd_uuid, KUC_GRP_HSM,
+                                      mdc_hsm_ct_reregister, imp);
+       if (rc < 0 && rc != -EEXIST)
+               CWARN("%s: Failed to re-register HSM agents (uuid: %s): rc = %d\n",
+                     imp->imp_obd->obd_name,
+                     obd_uuid2str(&imp->imp_obd->obd_uuid), rc);
+
+       class_import_put(imp);
+       RETURN(rc);
+}
+
 /**
  * Re-establish all kuc contexts with MDT
  * after MDT shutdown/recovery.
+ * This is done in background.
  */
 static int mdc_kuc_reregister(struct obd_import *imp)
 {
-       /* re-register HSM agents */
-       return libcfs_kkuc_group_foreach(&imp->imp_obd->obd_uuid, KUC_GRP_HSM,
-                                        mdc_hsm_ct_reregister, imp);
+       struct task_struct *task;
+       int rc = 0;
+
+       class_import_get(imp);
+       task = kthread_run(mdc_kuc_reregister_thread, imp, "kuc_reregister");
+
+       if (IS_ERR(task)) {
+               class_import_put(imp);
+               rc = PTR_ERR(task);
+       }
+
+       return rc;
 }
 
 static int mdc_set_info_async(const struct lu_env *env,
index 439e0cc..2c8cb88 100644 (file)
@@ -489,9 +489,6 @@ static void mdt_hsm_cdt_cleanup(struct mdt_device *mdt)
        mutex_lock(&cdt->cdt_restore_lock);
        list_for_each_entry_safe(crh, tmp3, &cdt->cdt_restore_handle_list,
                                 crh_list) {
-               /* not locked yet, cleanup by cdt_restore_handle_add() */
-               if (crh->crh_lh.mlh_type == MDT_NUL_LOCK)
-                       continue;
                list_del(&crh->crh_list);
                /* give back layout lock */
                mdt_object_unlock(cdt_mti, NULL, &crh->crh_lh, 1);
@@ -553,10 +550,21 @@ static int set_cdt_state(struct coordinator *cdt, enum cdt_states new_state)
        return rc;
 }
 
+int cdt_getref_try(struct coordinator *cdt)
+{
+       return refcount_inc_not_zero(&cdt->cdt_ref);
+}
+
+void cdt_putref(struct coordinator *cdt)
+{
+       if (refcount_dec_and_test(&cdt->cdt_ref))
+               wake_up(&cdt->cdt_waitq);
+}
+
 static int mdt_hsm_pending_restore(struct mdt_thread_info *mti);
 
-static void cdt_start_pending_restore(struct mdt_device *mdt,
-                                     struct coordinator *cdt)
+static int cdt_start_pending_restore(struct mdt_device *mdt,
+                                    struct coordinator *cdt)
 {
        struct mdt_thread_info *cdt_mti;
        unsigned int i = 0;
@@ -565,6 +573,8 @@ static void cdt_start_pending_restore(struct mdt_device *mdt,
        /* wait until MDD initialize hsm actions llog */
        while (!test_bit(MDT_FL_CFGLOG, &mdt->mdt_state) && i < obd_timeout) {
                schedule_timeout_interruptible(cfs_time_seconds(1));
+               if (kthread_should_stop())
+                       return -ESHUTDOWN;
                i++;
        }
        if (!test_bit(MDT_FL_CFGLOG, &mdt->mdt_state))
@@ -577,6 +587,7 @@ static void cdt_start_pending_restore(struct mdt_device *mdt,
                CERROR("%s: cannot take the layout locks needed for registered restore: %d\n",
                       mdt_obd_name(mdt), rc);
 
+       return rc;
 }
 
 /**
@@ -603,9 +614,18 @@ static int mdt_coordinator(void *data)
        obd_uuid2fsname(hsd.hsd_fsname, mdt_obd_name(mdt),
                        sizeof(hsd.hsd_fsname));
 
-       cdt_start_pending_restore(mdt, cdt);
        set_cdt_state(cdt, CDT_RUNNING);
 
+       /* Inform mdt_hsm_cdt_start(). */
+       wake_up(&cdt->cdt_waitq);
+
+       /* this initilazes cdt_last_cookie too */
+       rc = cdt_start_pending_restore(mdt, cdt);
+       if (rc < 0 || kthread_should_stop())
+               GOTO(fail_to_start, rc);
+
+       refcount_set(&cdt->cdt_ref, 1);
+
        while (1) {
                int i;
                int update_idx = 0;
@@ -630,6 +650,12 @@ static int mdt_coordinator(void *data)
 
                if (kthread_should_stop()) {
                        CDEBUG(D_HSM, "Coordinator stops\n");
+
+                       /* Drop the running ref */
+                       cdt_putref(cdt);
+                       /* Wait threads to finish */
+                       wait_event(cdt->cdt_waitq,
+                                  refcount_read(&cdt->cdt_ref) == 0);
                        rc = 0;
                        break;
                }
@@ -789,6 +815,7 @@ clean_cb_alloc:
        if (hsd.hsd_request != NULL)
                OBD_FREE_LARGE(hsd.hsd_request, request_sz);
 
+fail_to_start:
        mdt_hsm_cdt_cleanup(mdt);
 
        if (rc != 0)
@@ -799,6 +826,11 @@ clean_cb_alloc:
                              " no error\n",
                       mdt_obd_name(mdt), current->pid);
 
+       set_cdt_state(cdt, CDT_STOPPED);
+
+       /* Inform mdt_hsm_cdt_stop(). */
+       wake_up(&cdt->cdt_waitq);
+
        RETURN(rc);
 }
 
@@ -816,7 +848,6 @@ int cdt_restore_handle_add(struct mdt_thread_info *mti, struct coordinator *cdt,
                           const struct lu_fid *fid,
                           const struct hsm_extent *he)
 {
-       struct mdt_lock_handle lh = { 0 };
        struct cdt_restore_handle *crh;
        struct mdt_object *obj;
        int rc;
@@ -833,44 +864,28 @@ int cdt_restore_handle_add(struct mdt_thread_info *mti, struct coordinator *cdt,
         */
        crh->crh_extent.start = 0;
        crh->crh_extent.end = he->length;
-       crh->crh_lh.mlh_type = MDT_NUL_LOCK;
 
        mutex_lock(&cdt->cdt_restore_lock);
        if (cdt_restore_handle_find(cdt, fid) != NULL)
                GOTO(out_crl, rc = 1);
 
-       if (unlikely(cdt->cdt_state == CDT_STOPPED ||
-                    cdt->cdt_state == CDT_STOPPING))
-               GOTO(out_crl, rc = -EAGAIN);
-
        list_add_tail(&crh->crh_list, &cdt->cdt_restore_handle_list);
        mutex_unlock(&cdt->cdt_restore_lock);
 
        /* get the layout lock */
-       obj = mdt_object_find_lock(mti, &crh->crh_fid, &lh,
+       obj = mdt_object_find_lock(mti, &crh->crh_fid, &crh->crh_lh,
                                   MDS_INODELOCK_LAYOUT, LCK_EX);
-       if (IS_ERR(obj)) {
-               mutex_lock(&cdt->cdt_restore_lock);
+       if (IS_ERR(obj))
                GOTO(out_ldel, rc = PTR_ERR(obj));
-       }
 
        /* We do not keep a reference on the object during the restore
         * which can be very long.
         */
        mdt_object_put(mti->mti_env, obj);
 
-       mutex_lock(&cdt->cdt_restore_lock);
-       if (unlikely(cdt->cdt_state == CDT_STOPPED ||
-                    cdt->cdt_state == CDT_STOPPING))
-               GOTO(out_lh, rc = -EAGAIN);
-
-       crh->crh_lh = lh;
-       mutex_unlock(&cdt->cdt_restore_lock);
-
        RETURN(0);
-out_lh:
-       mdt_object_unlock(mti, NULL, &crh->crh_lh, 1);
 out_ldel:
+       mutex_lock(&cdt->cdt_restore_lock);
        list_del(&crh->crh_list);
 out_crl:
        mutex_unlock(&cdt->cdt_restore_lock);
@@ -1211,6 +1226,10 @@ static int mdt_hsm_cdt_start(struct mdt_device *mdt)
                       mdt_obd_name(mdt), rc);
        } else {
                cdt->cdt_task = task;
+               wait_event(cdt->cdt_waitq,
+                          cdt->cdt_state != CDT_INIT);
+               CDEBUG(D_HSM, "%s: coordinator thread started\n",
+                      mdt_obd_name(mdt));
                rc = 0;
        }
 
@@ -1227,15 +1246,20 @@ int mdt_hsm_cdt_stop(struct mdt_device *mdt)
        int rc;
 
        ENTRY;
+
        /* stop coordinator thread */
        rc = set_cdt_state(cdt, CDT_STOPPING);
-       if (rc == 0) {
-               kthread_stop(cdt->cdt_task);
-               cdt->cdt_task = NULL;
-               set_cdt_state(cdt, CDT_STOPPED);
-       }
+       if (rc)
+               RETURN(rc);
 
-       RETURN(rc);
+       kthread_stop(cdt->cdt_task);
+       rc = wait_event_interruptible(cdt->cdt_waitq,
+                                     cdt->cdt_state == CDT_STOPPED);
+       if (rc)
+               RETURN(-EINTR);
+
+       cdt->cdt_task = NULL;
+       RETURN(0);
 }
 
 static int mdt_hsm_set_exists(struct mdt_thread_info *mti,
@@ -1643,7 +1667,7 @@ int mdt_hsm_update_request_state(struct mdt_thread_info *mti,
        ENTRY;
 
        /* no coordinator started, so we cannot serve requests */
-       if (cdt->cdt_state == CDT_STOPPED)
+       if (!cdt_getref_try(cdt))
                RETURN(-EAGAIN);
 
        /* first do sanity checks */
@@ -1654,7 +1678,7 @@ int mdt_hsm_update_request_state(struct mdt_thread_info *mti,
                       mdt_obd_name(mdt),
                       pgs->hpk_cookie, PFID(&pgs->hpk_fid));
 
-               RETURN(PTR_ERR(car));
+               GOTO(putref, rc = PTR_ERR(car));
        }
 
        CDEBUG(D_HSM, "Progress received for fid="DFID" cookie=%#llx"
@@ -1748,6 +1772,8 @@ out:
        /* remove ref got from mdt_cdt_update_request() */
        mdt_cdt_put_request(car);
 
+putref:
+       cdt_putref(cdt);
        return rc;
 }
 
index 98f9e5d..63b7c30 100644 (file)
@@ -572,8 +572,7 @@ int mdt_hsm_request(struct tgt_session_info *tsi)
 
        hal->hal_version = HAL_VERSION;
        hal->hal_archive_id = hr->hr_archive_id;
-       hal->hal_flags = hr->hr_flags & ~HAL_CDT_FORCE;
-
+       hal->hal_flags = hr->hr_flags;
        obd_uuid2fsname(hal->hal_fsname, mdt_obd_name(info->mti_mdt),
                        MTI_NAME_MAXLEN);
 
index 773300d..4cf0f11 100644 (file)
@@ -290,20 +290,8 @@ int mdt_agent_record_add(const struct lu_env *env, struct mdt_device *mdt,
        if (lctxt == NULL || lctxt->loc_handle == NULL)
                GOTO(free, rc = -ENOENT);
 
-       /* Preserve lock order wrt hsm_cancel_all_actions() */
-       mutex_lock(&cdt->cdt_state_lock);
        down_write(&cdt->cdt_llog_lock);
 
-       /* Need cdt_last_cookie to be set during CDT startup; allow RAoLU
-        * requests, even though this can trigger the assertions in
-        * cdt_agent_record_hash_add(). This could be improved e.g. by failing
-        * the unlink during CDT_INIT, or adding RAoLU requests in an llog and
-        * issuing them if the CDT is available later
-        */
-       if ((cdt->cdt_state == CDT_STOPPED || cdt->cdt_state == CDT_STOPPING ||
-           cdt->cdt_state == CDT_INIT) && !(flags & HAL_CDT_FORCE))
-               GOTO(unavail, rc = -EAGAIN);
-
        /* in case of cancel request, the cookie is already set to the
         * value of the request cookie to be cancelled
         * so we do not change it */
@@ -315,9 +303,8 @@ int mdt_agent_record_add(const struct lu_env *env, struct mdt_device *mdt,
        rc = llog_cat_add(env, lctxt->loc_handle, &larr->arr_hdr, NULL);
        if (rc > 0)
                rc = 0;
-unavail:
+
        up_write(&cdt->cdt_llog_lock);
-       mutex_unlock(&cdt->cdt_state_lock);
        llog_ctxt_put(lctxt);
 
        EXIT;
index 9bfe711..379730b 100644 (file)
@@ -86,10 +86,11 @@ int mdt_hsm_agent_register(struct mdt_thread_info *mti,
        ENTRY;
 
        /* no coordinator started, so we cannot serve requests */
-       if (cdt->cdt_state == CDT_STOPPED) {
+       if (!cdt_getref_try(cdt)) {
                LCONSOLE_WARN("HSM coordinator thread is not running - "
                              "denying agent registration.\n");
-               RETURN(-ENXIO);
+               /* The client will resend the request if starting */
+               RETURN(cdt->cdt_state == CDT_RUNNING ? -EINPROGRESS : -ENXIO);
        }
 
        OBD_ALLOC_PTR(ha);
@@ -143,6 +144,7 @@ out:
        if (rc == -EEXIST || rc == 0)
                mdt_hsm_cdt_event(cdt);
 
+       cdt_putref(cdt);
        return rc;
 }
 
@@ -201,7 +203,7 @@ int mdt_hsm_agent_unregister(struct mdt_thread_info *mti,
        ENTRY;
 
        /* no coordinator started, so we cannot serve requests */
-       if (cdt->cdt_state == CDT_STOPPED)
+       if (!cdt_getref_try(cdt))
                RETURN(-ENXIO);
 
        down_write(&cdt->cdt_agent_lock);
@@ -223,6 +225,7 @@ int mdt_hsm_agent_unregister(struct mdt_thread_info *mti,
 out:
        CDEBUG(D_HSM, "agent %s unregistration: %d\n", obd_uuid2str(uuid), rc);
 
+       cdt_putref(cdt);
        return rc;
 }
 
index 0530cb6..33cce3d 100644 (file)
@@ -426,11 +426,11 @@ int mdt_hsm_add_actions(struct mdt_thread_info *mti,
        ENTRY;
 
        /* no coordinator started, so we cannot serve requests */
-       if (cdt->cdt_state == CDT_STOPPED || cdt->cdt_state == CDT_INIT)
+       if (cdt->cdt_state == CDT_STOPPING || !cdt_getref_try(cdt))
                RETURN(-EAGAIN);
 
        if (!hal_is_sane(hal))
-               RETURN(-EINVAL);
+               GOTO(out, rc = -EINVAL);
 
        /* search for compatible request, if found hai_cookie is set
         * to the request cookie
@@ -448,6 +448,7 @@ out:
        if (rc == 0 || rc == -ENODATA)
                mdt_hsm_cdt_event(cdt);
 
+       cdt_putref(cdt);
        return rc;
 }
 
@@ -467,10 +468,16 @@ bool mdt_hsm_restore_is_running(struct mdt_thread_info *mti,
        bool is_running;
        ENTRY;
 
+       /* the coordinator is not started */
+       if (!cdt_getref_try(cdt))
+               return false;
+
        mutex_lock(&cdt->cdt_restore_lock);
        is_running = (cdt_restore_handle_find(cdt, fid) != NULL);
        mutex_unlock(&cdt->cdt_restore_lock);
 
+       cdt_putref(cdt);
+
        RETURN(is_running);
 }
 
index 33cfa1a..15ff9f9 100644 (file)
@@ -127,6 +127,7 @@ static inline char *cdt_mdt_state2str(int state)
  * cdt_request_lock
  */
 struct coordinator {
+       refcount_t               cdt_ref;            /**< cdt refcount */
        wait_queue_head_t        cdt_waitq;          /**< cdt wait queue */
        bool                     cdt_event;          /**< coordinator event */
        struct task_struct      *cdt_task;           /**< cdt thread handle */
@@ -1145,6 +1146,8 @@ struct cdt_restore_handle *cdt_restore_handle_find(struct coordinator *cdt,
                                                   const struct lu_fid *fid);
 void cdt_restore_handle_del(struct mdt_thread_info *mti,
                            struct coordinator *cdt, const struct lu_fid *fid);
+int cdt_getref_try(struct coordinator *cdt);
+void cdt_putref(struct coordinator *cdt);
 /* coordinator management */
 int mdt_hsm_cdt_init(struct mdt_device *mdt);
 int mdt_hsm_cdt_stop(struct mdt_device *mdt);
index 6c9932f..47a338e 100644 (file)
@@ -984,8 +984,8 @@ int mdt_handle_last_unlink(struct mdt_thread_info *info, struct mdt_object *mo,
 
        hai.hai_fid = *mdt_object_fid(mo);
 
-       rc = mdt_agent_record_add(info->mti_env, info->mti_mdt, archive_id,
-                                 HAL_CDT_FORCE, &hai);
+       rc = mdt_agent_record_add(info->mti_env, info->mti_mdt, archive_id, 0,
+                                 &hai);
        if (rc)
                CERROR("%s: unable to add HSM remove request for "DFID
                       ": rc=%d\n", mdt_obd_name(info->mti_mdt),
index 414649d..ca45541 100755 (executable)
@@ -5442,6 +5442,53 @@ test_408 () { #LU-17110
 }
 run_test 408 "Verify fiemap on release file"
 
+test_409a() {
+       mkdir_on_mdt0 $DIR/$tdir
+
+       local restore_pid shutdown_pid
+       local mdt0_hsm_state
+       local f=$DIR/$tdir/$tfile
+       local fid=$(create_empty_file "$f")
+
+       copytool setup
+
+       $LFS hsm_archive --archive $HSM_ARCHIVE_NUMBER $f ||
+               error "could not archive file $f"
+       wait_request_state $fid ARCHIVE SUCCEED
+       $LFS hsm_release $f || error "could not release file $f"
+
+#define OBD_FAIL_MDS_HSM_CDT_DELAY      0x164
+       do_facet $SINGLEMDS $LCTL set_param fail_val=5 fail_loc=0x164
+
+       # send a restore request
+       cat $f > /dev/null & restore_pid=$!
+
+       # stop the coordinator while it handle the request
+       cdt_shutdown & shutdown_pid=$!
+       stack_trap "cdt_enable" EXIT
+
+       sleep 1;
+       mdt0_hsm_state=$(do_facet mds1 "$LCTL get_param -n mdt.*MDT0000.hsm_control")
+       [[ "$mdt0_hsm_state" == "stopping" ]] ||
+               error "HSM state of MDT0000 is not 'stopping' (hsm_control=$mdt0_hsm_state)"
+
+       wait $shutdown_pid
+       cdt_check_state stopped
+       wait_request_state $fid RESTORE WAITING
+
+       cdt_enable
+
+       # copytool must re-register
+       kill_copytools
+       wait_copytools || error "copytool failed to stop"
+       copytool setup
+
+       wait $restore_pid || true
+       wait_request_state $fid RESTORE SUCCEED
+       cat $f > /dev/null || error "fail to read $f"
+}
+run_test 409a "Coordinator should not stop when in use"
+
 test_500()
 {
        [ "$MDS1_VERSION" -lt $(version_code 2.6.92) ] &&