svn commit: r309097 - in head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs: . sys
Andriy Gapon
avg at FreeBSD.org
Thu Nov 24 10:21:23 UTC 2016
Author: avg
Date: Thu Nov 24 10:21:22 2016
New Revision: 309097
URL: https://svnweb.freebsd.org/changeset/base/309097
Log:
MFV r308987: 7180 potential race between zfs_suspend_fs+zfs_resume_fs
and zfs_ioc_rename
illumos/illumos-gate at 690041b9caf801816f2d0bac90bc7cecefb73523
https://github.com/illumos/illumos-gate/commit/690041b9caf801816f2d0bac90bc7cecefb73523
https://www.illumos.org/issues/7180
If a filesystem is not unmounted while the rename is being performed, then, for
example, a concurrect zfs rollback may call zfs_suspend_fs followed by
zfs_resume_fs on the same filesystem.
The latter takes the filesystem's name as an argument. If the filesystem name
changes as a result of the rename, then dmu_objset_hold(osname, zfsvfs, &os)
call in zfs_resume_fs would fail resulting in a kernel panic.
So far I have been able to reproduce this problem on FreeBSD where zfs rename
has -u option that skips the unmounting before doing the renaming.
But I think that in theory the same problem can occur on illumos as well,
because the unmounting is done in userland before invoking the rename ioctl and
there could be a race with, e.g., zfs mount.
panic: solaris assert: dmu_objset_hold(osname, zfsvfs, &zfsvfs->z_os) == 0 (0x2
== 0x0), file: /usr/devel/svn/head/sys/cddl/contrib/opensolaris/uts/common/fs/
zfs/zfs_vfsops.c, line: 2210
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe004df30710
vpanic() at vpanic+0x182/frame 0xfffffe004df30790
panic() at panic+0x43/frame 0xfffffe004df307f0
assfail3() at assfail3+0x2c/frame 0xfffffe004df30810
zfs_resume_fs() at zfs_resume_fs+0xb9/frame 0xfffffe004df30860
zfs_ioc_rollback() at zfs_ioc_rollback+0x61/frame 0xfffffe004df308a0
zfsdev_ioctl() at zfsdev_ioctl+0x65c/frame 0xfffffe004df30940
devfs_ioctl_f() at devfs_ioctl_f+0x156/frame 0xfffffe004df309a0
kern_ioctl() at kern_ioctl+0x246/frame 0xfffffe004df30a00
sys_ioctl() at sys_ioctl+0x171/frame 0xfffffe004df30ae0
amd64_syscall() at amd64_syscall+0x2db/frame 0xfffffe004df30bf0
Xfast_syscall() at Xfast_syscall+0xfb/frame 0xfffffe004df30bf0
Reviewed by: Matt Ahrens <mahrens at delphix.com>
Reviewed by: Pavel Zakharov <pavel.zakharov at delphix.com>
Approved by: Richard Lowe <richlowe at richlowe.net>
MFC after: 2 weeks
Modified:
head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/zfs_vfsops.h
head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c
head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c
Directory Properties:
head/sys/cddl/contrib/opensolaris/ (props changed)
Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/zfs_vfsops.h
==============================================================================
--- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/zfs_vfsops.h Thu Nov 24 10:11:09 2016 (r309096)
+++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/zfs_vfsops.h Thu Nov 24 10:21:22 2016 (r309097)
@@ -142,7 +142,7 @@ extern uint_t zfs_fsyncer_key;
extern int zfs_super_owner;
extern int zfs_suspend_fs(zfsvfs_t *zfsvfs);
-extern int zfs_resume_fs(zfsvfs_t *zfsvfs, const char *osname);
+extern int zfs_resume_fs(zfsvfs_t *zfsvfs, struct dsl_dataset *ds);
extern int zfs_userspace_one(zfsvfs_t *zfsvfs, zfs_userquota_prop_t type,
const char *domain, uint64_t rid, uint64_t *valuep);
extern int zfs_userspace_many(zfsvfs_t *zfsvfs, zfs_userquota_prop_t type,
Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c
==============================================================================
--- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c Thu Nov 24 10:11:09 2016 (r309096)
+++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c Thu Nov 24 10:21:22 2016 (r309097)
@@ -3789,12 +3789,15 @@ zfs_ioc_rollback(const char *fsname, nvl
int error;
if (getzfsvfs(fsname, &zfsvfs) == 0) {
+ dsl_dataset_t *ds;
+
+ ds = dmu_objset_ds(zfsvfs->z_os);
error = zfs_suspend_fs(zfsvfs);
if (error == 0) {
int resume_err;
error = dsl_dataset_rollback(fsname, zfsvfs, outnvl);
- resume_err = zfs_resume_fs(zfsvfs, fsname);
+ resume_err = zfs_resume_fs(zfsvfs, ds);
error = error ? error : resume_err;
}
#ifdef illumos
@@ -4435,8 +4438,10 @@ zfs_ioc_recv(zfs_cmd_t *zc)
if (getzfsvfs(tofs, &zfsvfs) == 0) {
/* online recv */
+ dsl_dataset_t *ds;
int end_err;
+ ds = dmu_objset_ds(zfsvfs->z_os);
error = zfs_suspend_fs(zfsvfs);
/*
* If the suspend fails, then the recv_end will
@@ -4444,7 +4449,7 @@ zfs_ioc_recv(zfs_cmd_t *zc)
*/
end_err = dmu_recv_end(&drc, zfsvfs);
if (error == 0)
- error = zfs_resume_fs(zfsvfs, tofs);
+ error = zfs_resume_fs(zfsvfs, ds);
error = error ? error : end_err;
#ifdef illumos
VFS_RELE(zfsvfs->z_vfs);
@@ -4990,11 +4995,14 @@ zfs_ioc_userspace_upgrade(zfs_cmd_t *zc)
* objset needs to be closed & reopened (to grow the
* objset_phys_t). Suspend/resume the fs will do that.
*/
+ dsl_dataset_t *ds;
+
+ ds = dmu_objset_ds(zfsvfs->z_os);
error = zfs_suspend_fs(zfsvfs);
if (error == 0) {
dmu_objset_refresh_ownership(zfsvfs->z_os,
zfsvfs);
- error = zfs_resume_fs(zfsvfs, zc->zc_name);
+ error = zfs_resume_fs(zfsvfs, ds);
}
}
if (error == 0)
Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c
==============================================================================
--- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c Thu Nov 24 10:11:09 2016 (r309096)
+++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c Thu Nov 24 10:21:22 2016 (r309097)
@@ -2225,7 +2225,7 @@ zfs_suspend_fs(zfsvfs_t *zfsvfs)
* zfsvfs, held, and long held on entry.
*/
int
-zfs_resume_fs(zfsvfs_t *zfsvfs, const char *osname)
+zfs_resume_fs(zfsvfs_t *zfsvfs, dsl_dataset_t *ds)
{
int err;
znode_t *zp;
@@ -2234,14 +2234,13 @@ zfs_resume_fs(zfsvfs_t *zfsvfs, const ch
ASSERT(RW_WRITE_HELD(&zfsvfs->z_teardown_inactive_lock));
/*
- * We already own this, so just hold and rele it to update the
- * objset_t, as the one we had before may have been evicted.
+ * We already own this, so just update the objset_t, as the one we
+ * had before may have been evicted.
*/
objset_t *os;
- VERIFY0(dmu_objset_hold(osname, zfsvfs, &os));
- VERIFY3P(os->os_dsl_dataset->ds_owner, ==, zfsvfs);
- VERIFY(dsl_dataset_long_held(os->os_dsl_dataset));
- dmu_objset_rele(os, zfsvfs);
+ VERIFY3P(ds->ds_owner, ==, zfsvfs);
+ VERIFY(dsl_dataset_long_held(ds));
+ VERIFY0(dmu_objset_from_ds(ds, &os));
err = zfsvfs_init(zfsvfs, os);
if (err != 0)
More information about the svn-src-all
mailing list