From 8d103d88565efa90e545de57210c12edef77c938 Mon Sep 17 00:00:00 2001 From: Serapheim Dimitropoulos Date: Thu, 8 Feb 2018 09:32:45 -0700 Subject: [PATCH] OpenZFS 8604 - Simplify snapshots unmounting code Authored by: Serapheim Dimitropoulos Reviewed by: Matt Ahrens Reviewed by: George Wilson Reviewed by: Andy Stormont Approved by: Robert Mustacchi Ported-by: Don Brady Every time we want to unmount a snapshot (happens during snapshot deletion or renaming) we unnecessarily iterate through all the mountpoints in the VFS layer (see zfs_get_vfs). The current patch completely gets rid of that code and changes the approach while keeping the behavior of that code path the same. Specifically, it puts a hold on the dataset/snapshot and gets its vfs resource reference directly, instead of linearly searching for it. If that reference exists we attempt to amount it. With the above change, it became obvious that the nvlist manipulations that we do (add_boolean and add_nvlist) take a significant amount of time ensuring uniqueness of every new element even though they don't have too. Thus, we updated the patch so those nvlists are not trying to enforce the uniqueness of their elements. A more complete analysis of the problem solved by this patch can be found below: https://sdimitro.github.io/post/snap-unmount-perf/ OpenZFS-issue: https://www.illumos.org/issues/8604 OpenZFS-commit: https://github.com/openzfs/openzfs/commit/126118fb --- include/sys/zfs_ioctl.h | 5 +++-- module/zfs/dsl_destroy.c | 18 ++++++++++++------ module/zfs/zfs_ioctl.c | 35 +++++++++++++---------------------- 3 files changed, 28 insertions(+), 30 deletions(-) diff --git a/include/sys/zfs_ioctl.h b/include/sys/zfs_ioctl.h index 301f93fe445e..ab562d24bfa8 100644 --- a/include/sys/zfs_ioctl.h +++ b/include/sys/zfs_ioctl.h @@ -20,7 +20,7 @@ */ /* * Copyright (c) 2005, 2010, Oracle and/or its affiliates. All rights reserved. - * Copyright (c) 2012, 2016 by Delphix. All rights reserved. + * Copyright (c) 2012, 2017 by Delphix. All rights reserved. * Copyright 2016 RackTop Systems. * Copyright (c) 2017, Intel Corporation. */ @@ -484,10 +484,11 @@ typedef struct zfs_creat { extern int zfs_secpolicy_snapshot_perms(const char *, cred_t *); extern int zfs_secpolicy_rename_perms(const char *, const char *, cred_t *); extern int zfs_secpolicy_destroy_perms(const char *, cred_t *); -extern int zfs_unmount_snap(const char *); +extern void zfs_unmount_snap(const char *); extern void zfs_destroy_unmount_origin(const char *); extern boolean_t dataset_name_hidden(const char *); extern int getzfsvfs_impl(struct objset *, struct zfsvfs **); +extern int getzfsvfs(const char *, struct zfsvfs **); enum zfsdev_state_type { ZST_ONEXIT, diff --git a/module/zfs/dsl_destroy.c b/module/zfs/dsl_destroy.c index 0ea27c3df1d0..7b49de95f798 100644 --- a/module/zfs/dsl_destroy.c +++ b/module/zfs/dsl_destroy.c @@ -496,23 +496,29 @@ dsl_destroy_snapshots_nvl(nvlist_t *snaps, boolean_t defer, if (nvlist_next_nvpair(snaps, NULL) == NULL) return (0); - nvlist_t *arg = fnvlist_alloc(); - nvlist_t *snaps_normalized = fnvlist_alloc(); /* * lzc_destroy_snaps() is documented to take an nvlist whose - * values "don't matter". We need to convert that nvlist to one - * that we know can be converted to LUA. + * values "don't matter". We need to convert that nvlist to + * one that we know can be converted to LUA. We also don't + * care about any duplicate entries because the nvlist will + * be converted to a LUA table which should take care of this. */ + nvlist_t *snaps_normalized; + VERIFY0(nvlist_alloc(&snaps_normalized, 0, KM_SLEEP)); for (nvpair_t *pair = nvlist_next_nvpair(snaps, NULL); pair != NULL; pair = nvlist_next_nvpair(snaps, pair)) { fnvlist_add_boolean_value(snaps_normalized, nvpair_name(pair), B_TRUE); } + + nvlist_t *arg; + VERIFY0(nvlist_alloc(&arg, 0, KM_SLEEP)); fnvlist_add_nvlist(arg, "snaps", snaps_normalized); fnvlist_free(snaps_normalized); fnvlist_add_boolean_value(arg, "defer", defer); - nvlist_t *wrapper = fnvlist_alloc(); + nvlist_t *wrapper; + VERIFY0(nvlist_alloc(&wrapper, 0, KM_SLEEP)); fnvlist_add_nvlist(wrapper, ZCP_ARG_ARGLIST, arg); fnvlist_free(arg); @@ -546,7 +552,7 @@ dsl_destroy_snapshots_nvl(nvlist_t *snaps, boolean_t defer, program, 0, zfs_lua_max_memlimit, - fnvlist_lookup_nvpair(wrapper, ZCP_ARG_ARGLIST), result); + nvlist_next_nvpair(wrapper, NULL), result); if (error != 0) { char *errorstr = NULL; (void) nvlist_lookup_string(result, ZCP_RET_ERROR, &errorstr); diff --git a/module/zfs/zfs_ioctl.c b/module/zfs/zfs_ioctl.c index 8b0cbb40b121..443005165ad5 100644 --- a/module/zfs/zfs_ioctl.c +++ b/module/zfs/zfs_ioctl.c @@ -27,7 +27,7 @@ * Copyright (c) 2014, 2016 Joyent, Inc. All rights reserved. * Copyright 2016 Nexenta Systems, Inc. All rights reserved. * Copyright (c) 2014, Joyent, Inc. All rights reserved. - * Copyright (c) 2011, 2016 by Delphix. All rights reserved. + * Copyright (c) 2011, 2017 by Delphix. All rights reserved. * Copyright (c) 2013 by Saso Kiselkov. All rights reserved. * Copyright (c) 2013 Steven Hartland. All rights reserved. * Copyright (c) 2014 Integros [integros.com] @@ -1437,7 +1437,7 @@ getzfsvfs_impl(objset_t *os, zfsvfs_t **zfvp) return (error); } -static int +int getzfsvfs(const char *dsname, zfsvfs_t **zfvp) { objset_t *os; @@ -3502,26 +3502,21 @@ zfs_ioc_log_history(const char *unused, nvlist_t *innvl, nvlist_t *outnvl) * Returns 0 if the argument is not a snapshot, or it is not currently a * filesystem, or we were able to unmount it. Returns error code otherwise. */ -int +void zfs_unmount_snap(const char *snapname) { - int err; - if (strchr(snapname, '@') == NULL) - return (0); + return; - err = zfsctl_snapshot_unmount((char *)snapname, MNT_FORCE); - if (err != 0 && err != ENOENT) - return (SET_ERROR(err)); - - return (0); + (void) zfsctl_snapshot_unmount((char *)snapname, MNT_FORCE); } /* ARGSUSED */ static int zfs_unmount_snap_cb(const char *snapname, void *arg) { - return (zfs_unmount_snap(snapname)); + zfs_unmount_snap(snapname); + return (0); } /* @@ -3544,7 +3539,7 @@ zfs_destroy_unmount_origin(const char *fsname) char originname[ZFS_MAX_DATASET_NAME_LEN]; dsl_dataset_name(ds->ds_prev, originname); dmu_objset_rele(os, FTAG); - (void) zfs_unmount_snap(originname); + zfs_unmount_snap(originname); } else { dmu_objset_rele(os, FTAG); } @@ -3572,7 +3567,7 @@ zfs_ioc_destroy_snaps(const char *poolname, nvlist_t *innvl, nvlist_t *outnvl) for (pair = nvlist_next_nvpair(snaps, NULL); pair != NULL; pair = nvlist_next_nvpair(snaps, pair)) { - (void) zfs_unmount_snap(nvpair_name(pair)); + zfs_unmount_snap(nvpair_name(pair)); } return (dsl_destroy_snapshots_nvl(snaps, defer, outnvl)); @@ -3715,11 +3710,8 @@ zfs_ioc_destroy(zfs_cmd_t *zc) { int err; - if (zc->zc_objset_type == DMU_OST_ZFS) { - err = zfs_unmount_snap(zc->zc_name); - if (err != 0) - return (err); - } + if (zc->zc_objset_type == DMU_OST_ZFS) + zfs_unmount_snap(zc->zc_name); if (strchr(zc->zc_name, '@')) { err = dsl_destroy_snapshot(zc->zc_name, zc->zc_defer_destroy); @@ -3814,13 +3806,12 @@ recursive_unmount(const char *fsname, void *arg) { const char *snapname = arg; char *fullname; - int error; fullname = kmem_asprintf("%s@%s", fsname, snapname); - error = zfs_unmount_snap(fullname); + zfs_unmount_snap(fullname); strfree(fullname); - return (error); + return (0); } /*