From d09f25dc66774959499a89bf3680d09c6e541ce8 Mon Sep 17 00:00:00 2001 From: Will Andrews Date: Tue, 11 Jun 2013 09:13:43 -0800 Subject: [PATCH] Illumos #3744 3744 zfs shouldn't ignore errors unmounting snapshots Reviewed by: Matthew Ahrens Approved by: Christopher Siden References: https://www.illumos.org/issues/3744 illumos/illumos-gate@fc7a6e3fefc649cb65c8e2a35d194781445008b0 Ported-by: Richard Yao Signed-off-by: Brian Behlendorf 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. --- include/sys/zfs_ioctl.h | 2 +- module/zfs/dsl_userhold.c | 2 +- module/zfs/zfs_ioctl.c | 43 +++++++++++++++++++++++++++------------ 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)); }