Whamcloud - gitweb
LU-7670 mdt: allow changelog commands to return errors 30/18030/28
authorBen Evans <bevans@cray.com>
Thu, 14 Jan 2016 20:58:15 +0000 (14:58 -0600)
committerOleg Drokin <oleg.drokin@intel.com>
Thu, 23 Feb 2017 02:06:29 +0000 (02:06 +0000)
Return errors to lctl/lfs users for out of range, and invalid IDs.
Currently only 0 is ever returned, regardless of outcome.  Some
information is printed in the MDS logs, but nothing on the client
to indicate success or failure.

Split purge and clear code into 2 separate paths.

Signed-off-by: Ben Evans <bevans@cray.com>
Change-Id: Ie139b170da0c8bef1c315ddb5361783230bb51ad
Reviewed-on: https://review.whamcloud.com/18030
Tested-by: Jenkins
Tested-by: Maloo <hpdd-maloo@intel.com>
Reviewed-by: Quentin Bouget <quentin.bouget@cea.fr>
Reviewed-by: Frank Zago <fzago@cray.com>
Reviewed-by: Oleg Drokin <oleg.drokin@intel.com>
lustre/mdd/mdd_device.c
lustre/mdt/mdt_handler.c
lustre/tests/sanity.sh
lustre/utils/lfs.c

index a6697d5..88378b1 100644 (file)
@@ -1331,135 +1331,253 @@ out:
         RETURN(rc);
 }
 
-struct mdd_changelog_user_data {
-        __u64 mcud_endrec; /**< purge record for this user */
-        __u64 mcud_minrec; /**< lowest changelog recno still referenced */
-        __u32 mcud_id;
-        __u32 mcud_minid;  /**< user id with lowest rec reference */
-        __u32 mcud_usercount;
-       unsigned int mcud_found:1;
+struct mdd_changelog_user_purge {
+       __u32 mcup_id;
+       __u32 mcup_usercount;
+       __u64 mcup_minrec;
+       bool mcup_found;
 };
-#define MCUD_UNREGISTER -1LL
 
-/** Two things:
- * 1. Find the smallest record everyone is willing to purge
- * 2. Update the last purgeable record for this user
+/**
+ * changelog_user_purge callback
+ *
+ * Is called once per user.
+ *
+ * Check to see the user requested is available from rec.
+ * Truncate the changelog.
+ * Keep track of the total number of users (calls).
  */
 static int mdd_changelog_user_purge_cb(const struct lu_env *env,
                                       struct llog_handle *llh,
                                       struct llog_rec_hdr *hdr, void *data)
 {
        struct llog_changelog_user_rec  *rec;
-       struct mdd_changelog_user_data  *mcud = data;
+       struct mdd_changelog_user_purge *mcup = data;
+       struct llog_cookie               cookie;
        int                              rc;
 
-        ENTRY;
+       ENTRY;
 
-        LASSERT(llh->lgh_hdr->llh_flags & LLOG_F_IS_PLAIN);
+       if ((llh->lgh_hdr->llh_flags & LLOG_F_IS_PLAIN) == 0)
+               RETURN(-ENXIO);
 
-        rec = (struct llog_changelog_user_rec *)hdr;
+       rec = container_of(hdr, struct llog_changelog_user_rec, cur_hdr);
 
-        mcud->mcud_usercount++;
+       mcup->mcup_usercount++;
 
-        /* If we have a new endrec for this id, use it for the following
-           min check instead of its old value */
-        if (rec->cur_id == mcud->mcud_id)
-                rec->cur_endrec = max(rec->cur_endrec, mcud->mcud_endrec);
+       if (rec->cur_id != mcup->mcup_id) {
+               /* truncate to the lowest endrec that is not this user */
+               mcup->mcup_minrec = min(mcup->mcup_minrec,
+                                       rec->cur_endrec);
+               RETURN(0);
+       }
 
-        /* Track the minimum referenced record */
-        if (mcud->mcud_minid == 0 || mcud->mcud_minrec > rec->cur_endrec) {
-                mcud->mcud_minid = rec->cur_id;
-                mcud->mcud_minrec = rec->cur_endrec;
-        }
+       /* Unregister this user */
+       cookie.lgc_lgl = llh->lgh_id;
+       cookie.lgc_index = hdr->lrh_index;
 
-        if (rec->cur_id != mcud->mcud_id)
-                RETURN(0);
+       rc = llog_cat_cancel_records(env, llh->u.phd.phd_cat_handle,
+                                    1, &cookie);
+       if (rc == 0) {
+               mcup->mcup_found = true;
+               mcup->mcup_usercount--;
+       }
 
-        /* Update this user's record */
-        mcud->mcud_found = 1;
+       RETURN(rc);
+}
 
-        /* Special case: unregister this user */
-        if (mcud->mcud_endrec == MCUD_UNREGISTER) {
-                struct llog_cookie cookie;
+static int mdd_changelog_user_purge(const struct lu_env *env,
+                                   struct mdd_device *mdd, __u32 id)
+{
+       struct mdd_changelog_user_purge mcup = {
+               .mcup_id = id,
+               .mcup_found = false,
+               .mcup_usercount = 0,
+               .mcup_minrec = ULLONG_MAX,
+       };
+       struct llog_ctxt *ctxt;
+       int rc;
 
-                cookie.lgc_lgl = llh->lgh_id;
-                cookie.lgc_index = hdr->lrh_index;
+       ENTRY;
 
-               rc = llog_cat_cancel_records(env, llh->u.phd.phd_cat_handle,
-                                            1, &cookie);
-                if (rc == 0)
-                        mcud->mcud_usercount--;
+       CDEBUG(D_IOCTL, "%s: Purge request: id=%u\n",
+              mdd2obd_dev(mdd)->obd_name, id);
 
-                RETURN(rc);
-        }
+       ctxt = llog_get_context(mdd2obd_dev(mdd),
+                               LLOG_CHANGELOG_USER_ORIG_CTXT);
+       if (ctxt == NULL ||
+           (ctxt->loc_handle->lgh_hdr->llh_flags & LLOG_F_IS_CAT) == 0)
+               GOTO(out, rc = -ENXIO);
+
+       rc = llog_cat_process(env, ctxt->loc_handle,
+                             mdd_changelog_user_purge_cb, &mcup,
+                             0, 0);
+
+       if ((rc == 0) && mcup.mcup_found) {
+               CDEBUG(D_IOCTL, "%s: Purging changelog entries for user %d "
+                      "record=%llu\n",
+                      mdd2obd_dev(mdd)->obd_name, id, mcup.mcup_minrec);
+               /* Cancelling record 0 destroys the entire changelog, make sure
+                  we don't do that unless we mean it. */
+               if (mcup.mcup_minrec != 0 || mcup.mcup_usercount == 0) {
+                       rc = mdd_changelog_llog_cancel(env, mdd,
+                                                      mcup.mcup_minrec);
+               }
+       } else {
+               CWARN("%s: No changelog for user %u; rc=%d\n",
+                     mdd2obd_dev(mdd)->obd_name, id, rc);
+               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);
+       }
+
+       EXIT;
+out:
+       if (ctxt != NULL)
+               llog_ctxt_put(ctxt);
+
+       return rc;
+}
+
+struct mdd_changelog_user_clear {
+       __u64 mcuc_endrec;
+       __u64 mcuc_minrec;
+       __u32 mcuc_id;
+       bool mcuc_flush;
+};
+
+/**
+ * changelog_clear callback
+ *
+ * Is called once per user.
+ *
+ * Check to see the user requested is available from rec.
+ * Check the oldest (smallest) record for boundary conditions.
+ * Truncate the changelog.
+ */
+static int mdd_changelog_clear_cb(const struct lu_env *env,
+                                 struct llog_handle *llh,
+                                 struct llog_rec_hdr *hdr,
+                                 void *data)
+{
+       struct llog_changelog_user_rec *rec;
+       struct mdd_changelog_user_clear *mcuc = data;
+       int rc;
+
+       ENTRY;
+
+       if ((llh->lgh_hdr->llh_flags & LLOG_F_IS_PLAIN) == 0)
+               RETURN(-ENXIO);
+
+       rec = container_of(hdr, struct llog_changelog_user_rec, cur_hdr);
+
+       /* Does the changelog id match the requested id? */
+       if (rec->cur_id != mcuc->mcuc_id) {
+               mcuc->mcuc_minrec = min(mcuc->mcuc_minrec,
+                                       rec->cur_endrec);
+               RETURN(0);
+       }
+
+       /* cur_endrec is the oldest purgeable record, make sure we're newer */
+       if (rec->cur_endrec > mcuc->mcuc_endrec) {
+               CDEBUG(D_IOCTL, "Request %llu out of range: %llu\n",
+                      mcuc->mcuc_endrec, rec->cur_endrec);
+               RETURN(-EINVAL);
+       }
+
+       /* Flag that we've met all the range and user checks.
+        * We now know the record to flush.
+        */
+       rec->cur_endrec = mcuc->mcuc_endrec;
+       mcuc->mcuc_flush = true;
 
-        /* Update the endrec */
-       CDEBUG(D_IOCTL, "Rewriting changelog user %d endrec to %llu\n",
-               mcud->mcud_id, rec->cur_endrec);
+       CDEBUG(D_IOCTL, "Rewriting changelog user %u endrec to %llu\n",
+              mcuc->mcuc_id, rec->cur_endrec);
 
+       /* Update the endrec */
        rc = llog_write(env, llh, hdr, hdr->lrh_index);
 
-        RETURN(rc);
+       RETURN(rc);
 }
 
-static int mdd_changelog_user_purge(const struct lu_env *env,
-                                    struct mdd_device *mdd, int id,
-                                    long long endrec)
+/**
+ * Clear a changelog up to entry specified by endrec for user id.
+ */
+static int mdd_changelog_clear(const struct lu_env *env,
+                              struct mdd_device *mdd, __u32 id,
+                              __u64 endrec)
 {
-        struct mdd_changelog_user_data data;
-        struct llog_ctxt *ctxt;
-        int rc;
-        ENTRY;
+       struct mdd_changelog_user_clear mcuc = {
+               .mcuc_id = id,
+               .mcuc_minrec = endrec,
+               .mcuc_flush = false,
+       };
+       struct llog_ctxt *ctxt;
+       __u64 start_rec;
+       int rc;
 
-        CDEBUG(D_IOCTL, "Purge request: id=%d, endrec=%lld\n", id, endrec);
+       ENTRY;
 
-        data.mcud_id = id;
-        data.mcud_minid = 0;
-        data.mcud_minrec = 0;
-        data.mcud_usercount = 0;
-        data.mcud_endrec = endrec;
+       CDEBUG(D_IOCTL, "%s: Purge request: id=%u, endrec=%llu\n",
+              mdd2obd_dev(mdd)->obd_name, id, endrec);
+       /* start_rec is the newest (largest value) entry in the changelogs*/
        spin_lock(&mdd->mdd_cl.mc_lock);
-       endrec = mdd->mdd_cl.mc_index;
+       start_rec = mdd->mdd_cl.mc_index;
        spin_unlock(&mdd->mdd_cl.mc_lock);
-        if ((data.mcud_endrec == 0) ||
-            ((data.mcud_endrec > endrec) &&
-             (data.mcud_endrec != MCUD_UNREGISTER)))
-                data.mcud_endrec = endrec;
 
-        ctxt = llog_get_context(mdd2obd_dev(mdd),
-                               LLOG_CHANGELOG_USER_ORIG_CTXT);
-        if (ctxt == NULL)
-                return -ENXIO;
+       if (start_rec < endrec) {
+               CDEBUG(D_IOCTL, "%s: Could not clear changelog, requested "\
+                      "address out of range\n", mdd2obd_dev(mdd)->obd_name);
+               RETURN(-EINVAL);
+       }
 
-        LASSERT(ctxt->loc_handle->lgh_hdr->llh_flags & LLOG_F_IS_CAT);
+       if (endrec == 0) {
+               mcuc.mcuc_endrec = start_rec;
+               mcuc.mcuc_minrec = ULLONG_MAX;
+       } else {
+               mcuc.mcuc_endrec = endrec;
+       }
+
+       ctxt = llog_get_context(mdd2obd_dev(mdd),
+                               LLOG_CHANGELOG_USER_ORIG_CTXT);
+       if (ctxt == NULL ||
+           (ctxt->loc_handle->lgh_hdr->llh_flags & LLOG_F_IS_CAT) == 0)
+               GOTO(out, rc = -ENXIO);
 
        rc = llog_cat_process(env, ctxt->loc_handle,
-                             mdd_changelog_user_purge_cb, (void *)&data,
+                             mdd_changelog_clear_cb, (void *)&mcuc,
                              0, 0);
-        if ((rc >= 0) && (data.mcud_minrec > 0)) {
-               CDEBUG(D_IOCTL, "Purging changelog entries up to %lld"
-                       ", referenced by "CHANGELOG_USER_PREFIX"%d\n",
-                       data.mcud_minrec, data.mcud_minid);
-               rc = mdd_changelog_llog_cancel(env, mdd, data.mcud_minrec);
-        } else {
-                CWARN("Could not determine changelog records to purge; rc=%d\n",
-                      rc);
-        }
-
-        llog_ctxt_put(ctxt);
 
-        if (!data.mcud_found) {
-                CWARN("No entry for user %d.  Last changelog reference is "
-                     "%lld by changelog user %d\n", data.mcud_id,
-                      data.mcud_minrec, data.mcud_minid);
-               rc = -ENOENT;
-        }
+       if (rc < 0) {
+               CWARN("%s: Failure to clear the changelog for user %d: %d\n",
+                     mdd2obd_dev(mdd)->obd_name, id, rc);
+       } else if (mcuc.mcuc_flush) {
+               /* Cancelling record 0 destroys the entire changelog, make sure
+                  we don't do that unless we mean it. */
+               if (mcuc.mcuc_minrec != 0) {
+                       CDEBUG(D_IOCTL, "%s: Purging changelog entries up "\
+                              "to %llu\n", mdd2obd_dev(mdd)->obd_name,
+                              mcuc.mcuc_minrec);
+
+                       rc = mdd_changelog_llog_cancel(env, mdd,
+                                                     mcuc.mcuc_minrec);
+               }
+       } else {
+               CWARN("%s: No entry for user %d\n",
+                     mdd2obd_dev(mdd)->obd_name, id);
+               rc = -ENOENT;
+       }
 
-       if (!rc && data.mcud_usercount == 0)
-               /* No more users; turn changelogs off */
-               rc = mdd_changelog_off(env, mdd);
+       EXIT;
+out:
+       if (ctxt != NULL)
+               llog_ctxt_put(ctxt);
 
-       RETURN(rc);
+       return rc;
 }
 
 /** mdd_iocontrol
@@ -1473,26 +1591,27 @@ static int mdd_changelog_user_purge(const struct lu_env *env,
 static int mdd_iocontrol(const struct lu_env *env, struct md_device *m,
                          unsigned int cmd, int len, void *karg)
 {
-        struct mdd_device *mdd;
-        struct obd_ioctl_data *data = karg;
-        int rc;
-        ENTRY;
+       struct mdd_device *mdd;
+       struct obd_ioctl_data *data = karg;
+       int rc;
 
-        mdd = lu2mdd_dev(&m->md_lu_dev);
+       ENTRY;
 
-        /* Doesn't use obd_ioctl_data */
-        switch (cmd) {
-        case OBD_IOC_CHANGELOG_CLEAR: {
-                struct changelog_setinfo *cs = karg;
-                rc = mdd_changelog_user_purge(env, mdd, cs->cs_id,
-                                              cs->cs_recno);
-                RETURN(rc);
-        }
-        case OBD_IOC_GET_MNTOPT: {
-                mntopt_t *mntopts = (mntopt_t *)karg;
-                *mntopts = mdd->mdd_dt_conf.ddp_mntopts;
-                RETURN(0);
-        }
+       mdd = lu2mdd_dev(&m->md_lu_dev);
+
+       /* Doesn't use obd_ioctl_data */
+       switch (cmd) {
+       case OBD_IOC_CHANGELOG_CLEAR: {
+               struct changelog_setinfo *cs = karg;
+               rc = mdd_changelog_clear(env, mdd, cs->cs_id,
+                                        cs->cs_recno);
+               RETURN(rc);
+       }
+       case OBD_IOC_GET_MNTOPT: {
+               mntopt_t *mntopts = (mntopt_t *)karg;
+               *mntopts = mdd->mdd_dt_conf.ddp_mntopts;
+               RETURN(0);
+       }
        case OBD_IOC_START_LFSCK: {
                rc = lfsck_start(env, mdd->mdd_bottom,
                                 (struct lfsck_start_param *)karg);
@@ -1510,30 +1629,29 @@ static int mdd_iocontrol(const struct lu_env *env, struct md_device *m,
        }
        }
 
-        /* Below ioctls use obd_ioctl_data */
-        if (len != sizeof(*data)) {
-                CERROR("Bad ioctl size %d\n", len);
-                RETURN(-EINVAL);
-        }
-        if (data->ioc_version != OBD_IOCTL_VERSION) {
-                CERROR("Bad magic %x != %x\n", data->ioc_version,
-                       OBD_IOCTL_VERSION);
-                RETURN(-EINVAL);
-        }
+       /* Below ioctls use obd_ioctl_data */
+       if (len != sizeof(*data)) {
+               CERROR("Bad ioctl size %d\n", len);
+               RETURN(-EINVAL);
+       }
+       if (data->ioc_version != OBD_IOCTL_VERSION) {
+               CERROR("Bad magic %x != %x\n", data->ioc_version,
+                      OBD_IOCTL_VERSION);
+               RETURN(-EINVAL);
+       }
 
-        switch (cmd) {
-        case OBD_IOC_CHANGELOG_REG:
-                rc = mdd_changelog_user_register(env, mdd, &data->ioc_u32_1);
-                break;
-        case OBD_IOC_CHANGELOG_DEREG:
-                rc = mdd_changelog_user_purge(env, mdd, data->ioc_u32_1,
-                                              MCUD_UNREGISTER);
-                break;
-        default:
-                rc = -ENOTTY;
-        }
+       switch (cmd) {
+       case OBD_IOC_CHANGELOG_REG:
+               rc = mdd_changelog_user_register(env, mdd, &data->ioc_u32_1);
+               break;
+       case OBD_IOC_CHANGELOG_DEREG:
+               rc = mdd_changelog_user_purge(env, mdd, data->ioc_u32_1);
+               break;
+       default:
+               rc = -ENOTTY;
+       }
 
-        RETURN (rc);
+       RETURN(rc);
 }
 
 /* type constructor/destructor: mdd_type_init, mdd_type_fini */
index e5da354..7463c7d 100644 (file)
@@ -5857,30 +5857,6 @@ int mdt_get_info(struct tgt_session_info *tsi)
        RETURN(rc);
 }
 
-/* Pass the ioc down */
-static int mdt_ioc_child(struct lu_env *env, struct mdt_device *mdt,
-                        unsigned int cmd, int len, void *data)
-{
-       struct lu_context ioctl_session;
-       struct md_device *next = mdt->mdt_child;
-       int rc;
-       ENTRY;
-
-        rc = lu_context_init(&ioctl_session, LCT_SERVER_SESSION);
-       if (rc)
-               RETURN(rc);
-       ioctl_session.lc_thread = (struct ptlrpc_thread *)current;
-       lu_context_enter(&ioctl_session);
-       env->le_ses = &ioctl_session;
-
-       LASSERT(next->md_ops->mdo_iocontrol);
-       rc = next->md_ops->mdo_iocontrol(env, next, cmd, len, data);
-
-       lu_context_exit(&ioctl_session);
-       lu_context_fini(&ioctl_session);
-       RETURN(rc);
-}
-
 static int mdt_ioc_version_get(struct mdt_thread_info *mti, void *karg)
 {
        struct obd_ioctl_data *data = karg;
@@ -5962,7 +5938,9 @@ static int mdt_iocontrol(unsigned int cmd, struct obd_export *exp, int len,
         case OBD_IOC_CHANGELOG_REG:
         case OBD_IOC_CHANGELOG_DEREG:
         case OBD_IOC_CHANGELOG_CLEAR:
-                rc = mdt_ioc_child(&env, mdt, cmd, len, karg);
+               rc = mdt->mdt_child->md_ops->mdo_iocontrol(&env,
+                                                          mdt->mdt_child,
+                                                          cmd, len, karg);
                 break;
        case OBD_IOC_START_LFSCK: {
                struct md_device *next = mdt->mdt_child;
index c24e419..e2d5416 100755 (executable)
@@ -10945,6 +10945,36 @@ test_160d() {
 }
 run_test 160d "verify that changelog log catch the migrate event"
 
+test_160e() {
+       # Create a user
+       CL_USER=$(do_facet $SINGLEMDS $LCTL --device $MDT0 \
+               changelog_register -n)
+       echo "Registered as changelog user $CL_USER"
+       trap cleanup_changelog EXIT
+
+       # Delete a future user (expect fail)
+       do_facet $SINGLEMDS $LCTL --device $MDT0 changelog_deregister cl77
+       local rc=$?
+
+       if [ $rc -eq 0 ]; then
+               error "Deleted non-existant user cl77"
+       elif [ $rc -ne 2 ]; then
+               error "changelog_deregister failed with $rc, " \
+                       "expected 2 (ENOENT)"
+       fi
+
+       # Clear to a bad index (1 billion should be safe)
+       $LFS changelog_clear $MDT0 $CL_USER 1000000000
+       rc=$?
+
+       if [ $rc -eq 0 ]; then
+               error "Successfully cleared to invalid CL index"
+       elif [ $rc -ne 22 ]; then
+               error "changelog_clear failed with $rc, expected 22 (EINVAL)"
+       fi
+}
+run_test 160e "changelog negative testing"
+
 test_161a() {
        [ $PARALLEL == "yes" ] && skip "skip parallel run" && return
        test_mkdir -p -c1 $DIR/$tdir
index e2dc5bb..933f455 100644 (file)
@@ -3607,19 +3607,30 @@ static int lfs_changelog(int argc, char **argv)
 
 static int lfs_changelog_clear(int argc, char **argv)
 {
-        long long endrec;
-        int rc;
+       long long endrec;
+       int rc;
 
-        if (argc != 4)
-                return CMD_HELP;
+       if (argc != 4)
+               return CMD_HELP;
 
-        endrec = strtoll(argv[3], NULL, 10);
+       endrec = strtoll(argv[3], NULL, 10);
 
-        rc = llapi_changelog_clear(argv[1], argv[2], endrec);
-        if (rc)
-                fprintf(stderr, "%s error: %s\n", argv[0],
-                        strerror(errno = -rc));
-        return rc;
+       rc = llapi_changelog_clear(argv[1], argv[2], endrec);
+
+       if (rc == -EINVAL)
+               fprintf(stderr, "%s: record out of range: %llu\n",
+                       argv[0], endrec);
+       else if (rc == -ENOENT)
+               fprintf(stderr, "%s: no changelog user: %s\n",
+                       argv[0], argv[2]);
+       else if (rc)
+               fprintf(stderr, "%s error: %s\n", argv[0],
+                       strerror(-rc));
+
+       if (rc)
+               errno = -rc;
+
+       return rc;
 }
 
 static int lfs_fid2path(int argc, char **argv)