OpenZFS 8520 - lzc_rollback

8520 lzc_rollback_to should support rolling back to origin
7198 libzfs should gracefully handle EINVAL from lzc_rollback

lzc_rollback_to() should support rolling back to a clone's origin.
The current checks in zfs_ioc_rollback() would not allow that
because the origin snapshot belongs to a different filesystem.
The overly restrictive check was in introduced in 7600, but it
was not a regression as none of the existing tools provided a
way to rollback to the origin.

Authored by: Andriy Gapon <avg@FreeBSD.org>
Reviewed by: Matthew Ahrens <mahrens@delphix.com>
Approved by: Dan McDonald <danmcd@joyent.com>
Ported-by: Brian Behlendorf <behlendorf1@llnl.gov>
OpenZFS-issue: https://www.illumos.org/issues/8520
OpenZFS-issue: https://www.illumos.org/issues/7198
OpenZFS-commit: https://github.com/openzfs/openzfs/commit/78a5a1a25a
Closes #7150

diff --git a/lib/libzfs/libzfs_dataset.c b/lib/libzfs/libzfs_dataset.c
index a0e8e7fb6bf1583bd523e3a46382e8061aa7a620..3d47180257b7b3c3806256c98878ae2c2fd22a96 100644 (file)
--- a/lib/libzfs/libzfs_dataset.c
+++ b/lib/libzfs/libzfs_dataset.c
@@ -4211,17 +4211,31 @@ zfs_rollback(zfs_handle_t *zhp, zfs_handle_t *snap, boolean_t force)
         * a new snapshot is created before this request is processed.
         */
        err = lzc_rollback_to(zhp->zfs_name, snap->zfs_name);
-       if (err == EXDEV) {
-               zfs_error_aux(zhp->zfs_hdl, dgettext(TEXT_DOMAIN,
-                   "'%s' is not the latest snapshot"), snap->zfs_name);
-               (void) zfs_error_fmt(zhp->zfs_hdl, EZFS_BUSY,
-                   dgettext(TEXT_DOMAIN, "cannot rollback '%s'"),
-                   zhp->zfs_name);
-               return (err);
-       } else if (err != 0) {
-               (void) zfs_standard_error_fmt(zhp->zfs_hdl, errno,
+       if (err != 0) {
+               char errbuf[1024];
+
+               (void) snprintf(errbuf, sizeof (errbuf),
                    dgettext(TEXT_DOMAIN, "cannot rollback '%s'"),
                    zhp->zfs_name);
+               switch (err) {
+               case EEXIST:
+                       zfs_error_aux(zhp->zfs_hdl, dgettext(TEXT_DOMAIN,
+                           "there is a snapshot or bookmark more recent "
+                           "than '%s'"), snap->zfs_name);
+                       (void) zfs_error(zhp->zfs_hdl, EZFS_EXISTS, errbuf);
+                       break;
+               case ESRCH:
+                       zfs_error_aux(zhp->zfs_hdl, dgettext(TEXT_DOMAIN,
+                           "'%s' is not found among snapshots of '%s'"),
+                           snap->zfs_name, zhp->zfs_name);
+                       (void) zfs_error(zhp->zfs_hdl, EZFS_NOENT, errbuf);
+                       break;
+               case EINVAL:
+                       (void) zfs_error(zhp->zfs_hdl, EZFS_BADTYPE, errbuf);
+                       break;
+               default:
+                       (void) zfs_standard_error(zhp->zfs_hdl, err, errbuf);
+               }
                return (err);
        }
 

diff --git a/module/zfs/dsl_dataset.c b/module/zfs/dsl_dataset.c
index 0bfc4cd7a24d24c457ab2f3d7ac48fbbbfa3dc8b..86f088889059fd907404ba78c8eaa722d489b900 100644 (file)
--- a/module/zfs/dsl_dataset.c
+++ b/module/zfs/dsl_dataset.c
@@ -2541,7 +2541,7 @@ dsl_dataset_rollback_check(void *arg, dmu_tx_t *tx)
        /* must have a most recent snapshot */
        if (dsl_dataset_phys(ds)->ds_prev_snap_txg < TXG_INITIAL) {
                dsl_dataset_rele(ds, FTAG);
-               return (SET_ERROR(EINVAL));
+               return (SET_ERROR(ESRCH));
        }
 
        /*
@@ -2561,11 +2561,46 @@ dsl_dataset_rollback_check(void *arg, dmu_tx_t *tx)
         * the latest snapshot is it.
         */
        if (ddra->ddra_tosnap != NULL) {
-               char namebuf[ZFS_MAX_DATASET_NAME_LEN];
+               dsl_dataset_t *snapds;
+
+               /* Check if the target snapshot exists at all. */
+               error = dsl_dataset_hold(dp, ddra->ddra_tosnap, FTAG, &snapds);
+               if (error != 0) {
+                       /*
+                        * ESRCH is used to signal that the target snapshot does
+                        * not exist, while ENOENT is used to report that
+                        * the rolled back dataset does not exist.
+                        * ESRCH is also used to cover other cases where the
+                        * target snapshot is not related to the dataset being
+                        * rolled back such as being in a different pool.
+                        */
+                       if (error == ENOENT || error == EXDEV)
+                               error = SET_ERROR(ESRCH);
+                       dsl_dataset_rele(ds, FTAG);
+                       return (error);
+               }
+               ASSERT(snapds->ds_is_snapshot);
 
-               dsl_dataset_name(ds->ds_prev, namebuf);
-               if (strcmp(namebuf, ddra->ddra_tosnap) != 0)
-                       return (SET_ERROR(EXDEV));
+               /* Check if the snapshot is the latest snapshot indeed. */
+               if (snapds != ds->ds_prev) {
+                       /*
+                        * Distinguish between the case where the only problem
+                        * is intervening snapshots (EEXIST) vs the snapshot
+                        * not being a valid target for rollback (ESRCH).
+                        */
+                       if (snapds->ds_dir == ds->ds_dir ||
+                           (dsl_dir_is_clone(ds->ds_dir) &&
+                           dsl_dir_phys(ds->ds_dir)->dd_origin_obj ==
+                           snapds->ds_object)) {
+                               error = SET_ERROR(EEXIST);
+                       } else {
+                               error = SET_ERROR(ESRCH);
+                       }
+                       dsl_dataset_rele(snapds, FTAG);
+                       dsl_dataset_rele(ds, FTAG);
+                       return (error);
+               }
+               dsl_dataset_rele(snapds, FTAG);
        }
 
        /* must not have any bookmarks after the most recent snapshot */
@@ -2574,8 +2609,10 @@ dsl_dataset_rollback_check(void *arg, dmu_tx_t *tx)
        nvlist_t *bookmarks = fnvlist_alloc();
        error = dsl_get_bookmarks_impl(ds, proprequest, bookmarks);
        fnvlist_free(proprequest);
-       if (error != 0)
+       if (error != 0) {
+               dsl_dataset_rele(ds, FTAG);
                return (error);
+       }
        for (nvpair_t *pair = nvlist_next_nvpair(bookmarks, NULL);
            pair != NULL; pair = nvlist_next_nvpair(bookmarks, pair)) {
                nvlist_t *valuenv =

diff --git a/module/zfs/zfs_ioctl.c b/module/zfs/zfs_ioctl.c
index cc7155a78ad192899a58c0ecfceda6c32480b49e..fcd2fca128d34eda5375be318837ee9f2d291e9d 100644 (file)
--- a/module/zfs/zfs_ioctl.c
+++ b/module/zfs/zfs_ioctl.c
@@ -3773,11 +3773,14 @@ zfs_ioc_rollback(const char *fsname, nvlist_t *innvl, nvlist_t *outnvl)
 
        (void) nvlist_lookup_string(innvl, "target", &target);
        if (target != NULL) {
-               int fslen = strlen(fsname);
+               const char *cp = strchr(target, '@');
 
-               if (strncmp(fsname, target, fslen) != 0)
-                       return (SET_ERROR(EINVAL));
-               if (target[fslen] != '@')
+               /*
+                * The snap name must contain an @, and the part after it must
+                * contain only valid characters.
+                */
+               if (cp == NULL ||
+                   zfs_component_namecheck(cp + 1, NULL, NULL) != 0)
                        return (SET_ERROR(EINVAL));
        }