OpenZFS 7600 - zfs rollback should pass target snapshot to kernel

Authored by: Andriy Gapon <avg@FreeBSD.org>
Reviewed by: Matthew Ahrens <mahrens@delphix.com>
Reviewed by: Pavel Zakharov <pavel.zakharov@delphix.com>
Approved by: Robert Mustacchi <rm@joyent.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Ported-by: Giuseppe Di Natale <dinatale2@llnl.gov>
The existing kernel-side code only provides a method to rollback to a
latest snapshot, whatever it happens to be at the time when the rollback
is actually done.  That could be unsafe or confusing in environments
where concurrent DSL changes are possible as the resulting state could
correspond to a newer or older snapshot than the originally requested
one.
This change allows to amend that method such that the rollback is
performed only when the latest snapshot has a specific name.  That is,
if a new snapshot is concurrently created or the target snapshot is
destroyed, then no rollback is done and EXDEV error is returned.
New libzfs_core function lzc_rollback_to() is provided for the new
functionality.  libzfs is changed to use lzc_rollback_to() to implement
zfs rollback command.
Perhaps we should return different errors to distinguish the case where
the desired snapshot exists but it's not the latest snapshot and the
case where the desired snapshot does not exist.

OpenZFS-issue: https://www.illumos.org/issues/7600
OpenZFS-commit: https://github.com/openzfs/openzfs/commit/3d645eb
Closes #6292

diff --git a/include/libzfs_core.h b/include/libzfs_core.h
index cf7ea38370d81aed1ed17fbc02fc21a596d0a68c..b4f61151c472db1cf182db5715120722b2ad857e 100644 (file)
--- a/include/libzfs_core.h
+++ b/include/libzfs_core.h
@@ -91,6 +91,7 @@ int lzc_receive_with_cmdprops(const char *, nvlist_t *, nvlist_t *,
 boolean_t lzc_exists(const char *);
 
 int lzc_rollback(const char *, char *, int);
+int lzc_rollback_to(const char *, const char *);
 
 int lzc_sync(const char *, nvlist_t *, nvlist_t **);
 

diff --git a/include/sys/dsl_dataset.h b/include/sys/dsl_dataset.h
index f6499a760a8792c8079c0252b724644f121a3a2c..50c1e9337f5b6f8da83ebad82a130a62ffb2dfe1 100644 (file)
--- a/include/sys/dsl_dataset.h
+++ b/include/sys/dsl_dataset.h
@@ -340,7 +340,8 @@ void dsl_dataset_set_refreservation_sync_impl(dsl_dataset_t *ds,
 void dsl_dataset_zapify(dsl_dataset_t *ds, dmu_tx_t *tx);
 boolean_t dsl_dataset_is_zapified(dsl_dataset_t *ds);
 boolean_t dsl_dataset_has_resume_receive_state(dsl_dataset_t *ds);
-int dsl_dataset_rollback(const char *fsname, void *owner, nvlist_t *result);
+int dsl_dataset_rollback(const char *fsname, const char *tosnap, void *owner,
+    nvlist_t *result);
 
 void dsl_dataset_deactivate_feature(uint64_t dsobj,
     spa_feature_t f, dmu_tx_t *tx);

diff --git a/lib/libzfs/libzfs_dataset.c b/lib/libzfs/libzfs_dataset.c
index a4f65b3972254053ba25a92ef654d9542d7505f6..d6e85024d55014ac9502218c1f69b045a57c6808 100644 (file)
--- a/lib/libzfs/libzfs_dataset.c
+++ b/lib/libzfs/libzfs_dataset.c
@@ -4018,14 +4018,19 @@ zfs_rollback(zfs_handle_t *zhp, zfs_handle_t *snap, boolean_t force)
        }
 
        /*
-        * We rely on zfs_iter_children() to verify that there are no
-        * newer snapshots for the given dataset.  Therefore, we can
-        * simply pass the name on to the ioctl() call.  There is still
-        * an unlikely race condition where the user has taken a
-        * snapshot since we verified that this was the most recent.
+        * Pass both the filesystem and the wanted snapshot names,
+        * we would get an error back if the snapshot is destroyed or
+        * a new snapshot is created before this request is processed.
         */
-       err = lzc_rollback(zhp->zfs_name, NULL, 0);
-       if (err != 0) {
+       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,
                    dgettext(TEXT_DOMAIN, "cannot rollback '%s'"),
                    zhp->zfs_name);

diff --git a/lib/libzfs_core/libzfs_core.c b/lib/libzfs_core/libzfs_core.c
index 8142f2c632a55bf040783e66c3500c5c43f508c4..347d825e2d4ece14bcea9b2b462c0dada1207799 100644 (file)
--- a/lib/libzfs_core/libzfs_core.c
+++ b/lib/libzfs_core/libzfs_core.c
@@ -888,6 +888,9 @@ int lzc_receive_with_cmdprops(const char *snapname, nvlist_t *props,
  * Roll back this filesystem or volume to its most recent snapshot.
  * If snapnamebuf is not NULL, it will be filled in with the name
  * of the most recent snapshot.
+ * Note that the latest snapshot may change if a new one is concurrently
+ * created or the current one is destroyed.  lzc_rollback_to can be used
+ * to roll back to a specific latest snapshot.
  *
  * Return 0 on success or an errno on failure.
  */
@@ -910,6 +913,27 @@ lzc_rollback(const char *fsname, char *snapnamebuf, int snapnamelen)
        return (err);
 }
 
+/*
+ * Roll back this filesystem or volume to the specified snapshot,
+ * if possible.
+ *
+ * Return 0 on success or an errno on failure.
+ */
+int
+lzc_rollback_to(const char *fsname, const char *snapname)
+{
+       nvlist_t *args;
+       nvlist_t *result;
+       int err;
+
+       args = fnvlist_alloc();
+       fnvlist_add_string(args, "target", snapname);
+       err = lzc_ioctl(ZFS_IOC_ROLLBACK, fsname, args, &result);
+       nvlist_free(args);
+       nvlist_free(result);
+       return (err);
+}
+
 /*
  * Creates bookmarks.
  *

diff --git a/module/zfs/dsl_dataset.c b/module/zfs/dsl_dataset.c
index 5fa04e7ba715e71bdaa5f6bb7e6800c4cb323f33..bd03b486858bd3ba039c957bc5f94839ef13a9a4 100644 (file)
--- a/module/zfs/dsl_dataset.c
+++ b/module/zfs/dsl_dataset.c
@@ -2210,6 +2210,7 @@ dsl_dataset_handoff_check(dsl_dataset_t *ds, void *owner, dmu_tx_t *tx)
 
 typedef struct dsl_dataset_rollback_arg {
        const char *ddra_fsname;
+       const char *ddra_tosnap;
        void *ddra_owner;
        nvlist_t *ddra_result;
 } dsl_dataset_rollback_arg_t;
@@ -2253,6 +2254,18 @@ dsl_dataset_rollback_check(void *arg, dmu_tx_t *tx)
                return (SET_ERROR(EAGAIN));
        }
 
+       /*
+        * If the expected target snapshot is specified, then check that
+        * the latest snapshot is it.
+        */
+       if (ddra->ddra_tosnap != NULL) {
+               char namebuf[ZFS_MAX_DATASET_NAME_LEN];
+
+               dsl_dataset_name(ds->ds_prev, namebuf);
+               if (strcmp(namebuf, ddra->ddra_tosnap) != 0)
+                       return (SET_ERROR(EXDEV));
+       }
+
        /* must not have any bookmarks after the most recent snapshot */
        proprequest = fnvlist_alloc();
        fnvlist_add_boolean(proprequest, zfs_prop_to_name(ZFS_PROP_CREATETXG));
@@ -2354,11 +2367,13 @@ dsl_dataset_rollback_sync(void *arg, dmu_tx_t *tx)
  * notes above zfs_suspend_fs() for further details.
  */
 int
-dsl_dataset_rollback(const char *fsname, void *owner, nvlist_t *result)
+dsl_dataset_rollback(const char *fsname, const char *tosnap, void *owner,
+    nvlist_t *result)
 {
        dsl_dataset_rollback_arg_t ddra;
 
        ddra.ddra_fsname = fsname;
+       ddra.ddra_tosnap = tosnap;
        ddra.ddra_owner = owner;
        ddra.ddra_result = result;
 

diff --git a/module/zfs/zfs_ioctl.c b/module/zfs/zfs_ioctl.c
index a2f7f045f1f6113df16d8efb3eb81093602e781b..fff1a3c06230325dc2b6c5396380832456fac453 100644 (file)
--- a/module/zfs/zfs_ioctl.c
+++ b/module/zfs/zfs_ioctl.c
@@ -3653,19 +3653,30 @@ zfs_ioc_destroy(zfs_cmd_t *zc)
 /*
  * fsname is name of dataset to rollback (to most recent snapshot)
  *
- * innvl is not used.
+ * innvl may contain name of expected target snapshot
  *
  * outnvl: "target" -> name of most recent snapshot
  * }
  */
 /* ARGSUSED */
 static int
-zfs_ioc_rollback(const char *fsname, nvlist_t *args, nvlist_t *outnvl)
+zfs_ioc_rollback(const char *fsname, nvlist_t *innvl, nvlist_t *outnvl)
 {
        zfsvfs_t *zfsvfs;
        zvol_state_t *zv;
+       char *target = NULL;
        int error;
 
+       (void) nvlist_lookup_string(innvl, "target", &target);
+       if (target != NULL) {
+               int fslen = strlen(fsname);
+
+               if (strncmp(fsname, target, fslen) != 0)
+                       return (SET_ERROR(EINVAL));
+               if (target[fslen] != '@')
+                       return (SET_ERROR(EINVAL));
+       }
+
        if (getzfsvfs(fsname, &zfsvfs) == 0) {
                dsl_dataset_t *ds;
 
@@ -3674,16 +3685,18 @@ zfs_ioc_rollback(const char *fsname, nvlist_t *args, nvlist_t *outnvl)
                if (error == 0) {
                        int resume_err;
 
-                       error = dsl_dataset_rollback(fsname, zfsvfs, outnvl);
+                       error = dsl_dataset_rollback(fsname, target, zfsvfs,
+                           outnvl);
                        resume_err = zfs_resume_fs(zfsvfs, ds);
                        error = error ? error : resume_err;
                }
                deactivate_super(zfsvfs->z_sb);
        } else if ((zv = zvol_suspend(fsname)) != NULL) {
-               error = dsl_dataset_rollback(fsname, zvol_tag(zv), outnvl);
+               error = dsl_dataset_rollback(fsname, target, zvol_tag(zv),
+                   outnvl);
                zvol_resume(zv);
        } else {
-               error = dsl_dataset_rollback(fsname, NULL, outnvl);
+               error = dsl_dataset_rollback(fsname, target, NULL, outnvl);
        }
        return (error);
 }