Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Illumos 3464 #1658

Closed
wants to merge 2 commits into from
Closed

Illumos 3464 #1658

wants to merge 2 commits into from

Conversation

dweeezil
Copy link
Contributor

Notes from the Illumos commit:

Description

The locking code around dsl_datasets and dsl_dir's is responsible for several deadlocks
and race conditions, most prominently:

a concurrent "zfs list" while running "zfs destroy" can cause the destroy to fail
with EBUSY. bug #3041 deadlock between dp->dp_config_rwlock and
ds->ds_opening_lock

To fix this, we must restructure the locking, and change the interface to synctasks.

The wad that will fix this also addresses the following:

improve performance of "zfs recv" by reducing the number of calls to txg_wait_synced()
fix zfs refquota can be violated by "zfs rollback" and "zfs receive" operations
fix ztest fails in ztest_dmu_snapshot_hold due to bad printf string
remove unused zc_top_ds from zfs_cmd_t
fix zdb crashes on pools < SPA_VERSION_DEADLISTS
remove undocumented "zfs hold -t" flag (for temporary hold)
use ASSERT0 / VERIFY0 in more places
new libzfs_core routines: lzc_hold(), lzc_release(), lzc_get_holds() (for user holds)
move some routines from dsl_dataset.c to dsl_destroy.c, dsl_userhold.c
make rrwlock.c work in userland (libzpool.so)
change all synctask consumers to new model
remove ds_recvlock; all receives use "%recv" child to prevent concurrent receives
"zfs destroy" destroys multiple snapshots at once, so it is much faster

This patch is based on the pull request #1496 which is based on master as of a day ago or so.

@dweeezil
Copy link
Contributor Author

Pushed dweeezil/zfs@4a39697 to rebase to dweeezil/zfs@1826f91 of #1496.

2882 implement libzfs_core
2883 changing "canmount" property to "on" should not always remount dataset
2900 "zfs snapshot" should be able to create multiple, arbitrary snapshots at once

Reviewed by: George Wilson <george.wilson@delphix.com>
Reviewed by: Chris Siden <christopher.siden@delphix.com>
Reviewed by: Garrett D'Amore <garrett@damore.org>
Reviewed by: Bill Pijewski <wdp@joyent.com>
Reviewed by: Dan Kruchinin <dan.kruchinin@gmail.com>
Approved by: Eric Schrock <Eric.Schrock@delphix.com>

References:
  illumos/illumos-gate@4445fff

Ported-by: Tim Chase <tim@chase2k.com>
Closes openzfs#1293

Porting notes:

Add zvol minor device creation to the new zfs_snapshot_nvl function.

Remove the logging of the "release" operation in
dsl_dataset_user_release_sync().  The logging caused a null dereference
because ds->ds_dir is zeroed in dsl_dataset_destroy_sync() and the
logging functions try to get the ds name via the dsl_dataset_name()
function. I've got no idea why this particular code would have worked
in Illumos.  This code has subsequently been completely reworked in
Illumos commit 3b2aab1 (3464 zfs synctask code needs restructuring).

Squash some "may be used uninitialized" warning/erorrs.

Fix some printf format warnings for %lld and %llu.

Apply a few spa_writeable() changes that were made to Illumos in
illumos/illumos-gate.git@cd1c8b8 as part of the 3112, 3113, 3114 and
3115 fixes.

Add a missing call to fnvlist_free(nvl) in log_internal() that was added
in Illumos to fix issue 3085 but couldn't be ported to ZoL at the time
(openzfs/zfs@9e11c73) because it depended on future work.
@dweeezil
Copy link
Contributor Author

Pushed dweeezil/zfs@f1632c7 to rebase to dweeezil/zfs@ac02a59 and, therefore, current zfsonlinux/zfs master (0.6.2).

@dweeezil
Copy link
Contributor Author

Pushed dweeezil/zfs@6d3a0c7 to fix a long-standing porting issue in which zvol minors were re-created prior to a rename rather than after a rename. This is, of course, still based on dweeezil/zfs@ac02a59 which is, itself, based on zfsonlinux/zfs master as of 0.6.2.

@behlendorf
Copy link
Contributor

@dweeezil I ran in to some minor build issues with the patch when building pacakges. Please include the new headers in include/sys/Makefile.am so make dist includes them in the tarball. Like this:

diff --git a/include/sys/Makefile.am b/include/sys/Makefile.am
index 2245ff4..008a3ac 100644
--- a/include/sys/Makefile.am
+++ b/include/sys/Makefile.am
@@ -13,17 +13,20 @@ COMMON_H = \
        $(top_srcdir)/include/sys/dmu_impl.h \
        $(top_srcdir)/include/sys/dmu_objset.h \
        $(top_srcdir)/include/sys/dmu_traverse.h \
+       $(top_srcdir)/include/sys/dmu_send.h \
        $(top_srcdir)/include/sys/dmu_tx.h \
        $(top_srcdir)/include/sys/dmu_zfetch.h \
        $(top_srcdir)/include/sys/dnode.h \
        $(top_srcdir)/include/sys/dsl_dataset.h \
        $(top_srcdir)/include/sys/dsl_deadlist.h \
        $(top_srcdir)/include/sys/dsl_deleg.h \
+       $(top_srcdir)/include/sys/dsl_destroy.h \
        $(top_srcdir)/include/sys/dsl_dir.h \
        $(top_srcdir)/include/sys/dsl_pool.h \
        $(top_srcdir)/include/sys/dsl_prop.h \
        $(top_srcdir)/include/sys/dsl_scan.h \
        $(top_srcdir)/include/sys/dsl_synctask.h \
+       $(top_srcdir)/include/sys/dsl_userhold.h \
        $(top_srcdir)/include/sys/efi_partition.h \
        $(top_srcdir)/include/sys/metaslab.h \
        $(top_srcdir)/include/sys/metaslab_impl.h \

3464 zfs synctask code needs restructuring
Reviewed by: Dan Kimmel <dan.kimmel@delphix.com>
Reviewed by: Adam Leventhal <ahl@delphix.com>
Reviewed by: George Wilson <george.wilson@delphix.com>
Reviewed by: Christopher Siden <christopher.siden@delphix.com>
Approved by: Garrett D'Amore <garrett@damore.org>

References:
  illumos/illumos-gate@3b2aab1

Ported-by: Tim Chase <tim@chase2k.com>
@dweeezil
Copy link
Contributor Author

dweeezil commented Sep 4, 2013

@behlendorf I just pushed dweeezil/zfs@4789bce which adds the missing headers to include/sys/Makefile.am and also re-alphabetizes the list by re-generating it from scratch.

@behlendorf
Copy link
Contributor

Merged. The patch passed all of the automated testing and it's been in Illumos for some time. However since it so large I expect that we missed something so let's keep an eye out for new regressions. @dweeezil thank you again for doing this it was a huge amount of work and I appreciate it greatly!

13fe019 Illumos #3464

@behlendorf behlendorf closed this Sep 4, 2013
@svaroqui svaroqui mentioned this pull request Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants