Illumos #3744

3744 zfs shouldn't ignore errors unmounting snapshots
Reviewed by: Matthew Ahrens <[email protected]>
Approved by: Christopher Siden <[email protected]>

References:
  https://www.illumos.org/issues/3744
  illumos/illumos-gate@fc7a6e3fefc649cb65c8e2a35d194781445008b0

Ported-by: Richard Yao <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Issue #1775

Porting notes:

1. There is no clear way to distinguish between a failure when we
   tried to unmount the snapdir of a zvol (which does not exist)
   and the failure when we try to unmount a snapdir of a dataset,
   so the changes to zfs_unmount_snap() were dropped in favor of
   an altered Linux function that unconditionally returns 0.

3 files changed, 32 insertions, 15 deletions

diff --git a/include/sys/zfs_ioctl.h b/include/sys/zfs_ioctl.h
index 0ee6cc1cd..4a717b7d1 100644
--- a/include/sys/zfs_ioctl.h
+++ b/include/sys/zfs_ioctl.h
@@ -356,7 +356,7 @@ extern int zfs_secpolicy_snapshot_perms(const char *name, cred_t *cr);
 extern int zfs_secpolicy_rename_perms(const char *from,
     const char *to, cred_t *cr);
 extern int zfs_secpolicy_destroy_perms(const char *name, cred_t *cr);
-extern void zfs_unmount_snap(const char *);
+extern int zfs_unmount_snap(const char *);
 extern void zfs_destroy_unmount_origin(const char *);
 
 enum zfsdev_state_type {
diff --git a/module/zfs/dsl_userhold.c b/module/zfs/dsl_userhold.c
index 50adfabb0..cda4081f3 100644
--- a/module/zfs/dsl_userhold.c
+++ b/module/zfs/dsl_userhold.c
@@ -434,7 +434,7 @@ dsl_dataset_user_release_tmp(dsl_pool_t *dp, uint64_t dsobj, const char *htag)
 		dsl_dataset_name(ds, name);
 		dsl_dataset_rele(ds, FTAG);
 		dsl_pool_config_exit(dp, FTAG);
-		zfs_unmount_snap(name);
+		(void) zfs_unmount_snap(name);
 	} else {
 		dsl_pool_config_exit(dp, FTAG);
 	}
diff --git a/module/zfs/zfs_ioctl.c b/module/zfs/zfs_ioctl.c
index a0da3b996..f476fc183 100644
--- a/module/zfs/zfs_ioctl.c
+++ b/module/zfs/zfs_ioctl.c
@@ -3348,8 +3348,16 @@ zfs_ioc_log_history(const char *unused, nvlist_t *innvl, nvlist_t *outnvl)
  *
  * This function is best-effort.  Callers must deal gracefully if it
  * remains mounted (or is remounted after this call).
+ *
+ * XXX: This function should detect a failure to unmount a snapdir of a dataset
+ * and return the appropriate error code when it is mounted. Its Illumos and
+ * FreeBSD counterparts do this. We do not do this on Linux because there is no
+ * clear way to access the mount information that FreeBSD and Illumos use to
+ * distinguish between things with mounted snapshot directories, and things
+ * without mounted snapshot directories, which include zvols. Returning a
+ * failure for the latter causes `zfs destroy` to fail on zvol snapshots.
  */
-void
+int
 zfs_unmount_snap(const char *snapname)
 {
 	zfs_sb_t *zsb = NULL;
@@ -3358,7 +3366,7 @@ zfs_unmount_snap(const char *snapname)
 	char *ptr;
 
 	if ((ptr = strchr(snapname, '@')) == NULL)
-		return;
+		return (0);
 
 	dsname = kmem_alloc(ptr - snapname + 1, KM_SLEEP);
 	strlcpy(dsname, snapname, ptr - snapname + 1);
@@ -3373,15 +3381,14 @@ zfs_unmount_snap(const char *snapname)
 	kmem_free(dsname, ptr - snapname + 1);
 	strfree(fullname);
 
-	return;
+	return (0);
 }
 
 /* ARGSUSED */
 static int
 zfs_unmount_snap_cb(const char *snapname, void *arg)
 {
-	zfs_unmount_snap(snapname);
-	return (0);
+	return (zfs_unmount_snap(snapname));
 }
 
 /*
@@ -3404,7 +3411,7 @@ zfs_destroy_unmount_origin(const char *fsname)
 		char originname[MAXNAMELEN];
 		dsl_dataset_name(ds->ds_prev, originname);
 		dmu_objset_rele(os, FTAG);
-		zfs_unmount_snap(originname);
+		(void) zfs_unmount_snap(originname);
 	} else {
 		dmu_objset_rele(os, FTAG);
 	}
@@ -3421,7 +3428,7 @@ zfs_destroy_unmount_origin(const char *fsname)
 static int
 zfs_ioc_destroy_snaps(const char *poolname, nvlist_t *innvl, nvlist_t *outnvl)
 {
-	int poollen;
+	int error, poollen;
 	nvlist_t *snaps;
 	nvpair_t *pair;
 	boolean_t defer;
@@ -3442,8 +3449,10 @@ zfs_ioc_destroy_snaps(const char *poolname, nvlist_t *innvl, nvlist_t *outnvl)
 		    (name[poollen] != '/' && name[poollen] != '@'))
 			return (SET_ERROR(EXDEV));
 
-		zfs_unmount_snap(name);
 		(void) zvol_remove_minor(name);
+		error = zfs_unmount_snap(name);
+		if (error != 0)
+			return (error);
 	}
 
 	return (dsl_destroy_snapshots_nvl(snaps, defer, outnvl));
@@ -3461,8 +3470,12 @@ static int
 zfs_ioc_destroy(zfs_cmd_t *zc)
 {
 	int err;
-	if (strchr(zc->zc_name, '@') && zc->zc_objset_type == DMU_OST_ZFS)
-		zfs_unmount_snap(zc->zc_name);
+
+	if (zc->zc_objset_type == DMU_OST_ZFS) {
+		err = zfs_unmount_snap(zc->zc_name);
+		if (err != 0)
+			return (err);
+	}
 
 	if (strchr(zc->zc_name, '@'))
 		err = dsl_destroy_snapshot(zc->zc_name, zc->zc_defer_destroy);
@@ -3510,7 +3523,7 @@ recursive_unmount(const char *fsname, void *arg)
 	fullname = kmem_asprintf("%s@%s", fsname, snapname);
 	zfs_unmount_snap(fullname);
 	strfree(fullname);
-	return (0);
+	return (zfs_unmount_snap(fullname));
 }
 
 /*
@@ -4855,14 +4868,18 @@ static int
 zfs_ioc_release(const char *pool, nvlist_t *holds, nvlist_t *errlist)
 {
 	nvpair_t *pair;
+	int err;
 
 	/*
 	 * The release may cause the snapshot to be destroyed; make sure it
 	 * is not mounted.
 	 */
 	for (pair = nvlist_next_nvpair(holds, NULL); pair != NULL;
-	    pair = nvlist_next_nvpair(holds, pair))
-		zfs_unmount_snap(nvpair_name(pair));
+	    pair = nvlist_next_nvpair(holds, pair)) {
+		err = zfs_unmount_snap(nvpair_name(pair));
+		if (err != 0)
+			return (err);
+	}
 
 	return (dsl_dataset_user_release(holds, errlist));
 }