Merge Illumos 4445fff (Illumos feature #2882, issue #2883, feature #2900) #1496
Conversation
| @@ -563,9 +562,11 @@ extern int zfs_create(libzfs_handle_t *, const char *, zfs_type_t, | |||
| extern int zfs_create_ancestors(libzfs_handle_t *, const char *); | |||
| extern int zfs_destroy(zfs_handle_t *, boolean_t); | |||
| extern int zfs_destroy_snaps(zfs_handle_t *, char *, boolean_t); | |||
| extern int zfs_destroy_snaps_nvl(zfs_handle_t *, nvlist_t *, boolean_t); | |||
There was a problem hiding this comment.
The parameter change to zfs_destroy_snaps_nvl was accidentally picked up from illumos 3b2aab1. There should be no problem with it in this commit, but it doesn't totally track the history of changes to Illumos.
|
#1495 Port Illumos #3464 (3b2aab1) - zfs synctask code needs restructuring a day ago While working on issue #1495, I noticed I included a few changes from that commit (Illumos 3b2aab1) in this one. In particular, the zfs_destroy_snaps_nvl() function was updated to use a libzfs_handle_t * argument rather than a zfs_handle_t * argument. There should be no problem with this change being in this commit. |
|
Note: Commit 8592f00 is an amend of 9bdf8c to use the original author attribution. |
|
I have a feeling part of this commit may be causing the problem reported in #1518. See my comments there. |
|
@dweeezil Thanks for starting this port. These are changes we absolutely want to bring in but they're so extensive it's going to be a challenge to review and test them, I won't have a chance to carefully review these for a little but I wanted to ask how difficult was this to port? Were there a large number of conflicts? |
|
@behlendorf Yes, there were a lot of conflicts. In some cases, I took parts wholesale from Illumos and modified them after-the-fact as necessary (dmu_objset.c dweeezil/zfs@f0379f4). I'd also like to refer you to my "illumos-4445fff" branch (dweeezil/zfs@357fa8e) which is my "working branch" (the pull request is a roll-up branch) for a bit more of a blow-by-blow commentary on the porting effort. Testing this all for correctness will, indeed, be a bear. I've got reason to believe that parts of it may not be playing nice with your changes from 3558fd7 and, among other things, think that some interaction between 3558fd7 and mine may be causing the problem indicated in #1518. While trying to track that one down, I came across #1536 which occurs even in master. There seems to be some underlying problem that can be triggered as part of the various operations that need to remount a file system such as the post-receive part of a zfs receive and also rollbacks. In any case, this patch does currently pass all the tests in "zconfig.sh" and can run ztest for long stretches of time without errors. I'm going to continue to try to track down the cause of some of these recent issues and also stress it on a few personal systems of mine. |
|
@dweeezil OK, well I'll queue it up for automated testing and additional review. Beyond that I'm going to need a significant amount of run time with this patch before I'm comfortable merging it. |
|
@dweeezil I am building this on centos 6.4 and make pkg fails with include/libzfs_core.h the patch that fixes it for me is diff --git a/include/Makefile.am b/include/Makefile.am
index 64141d9..2e1c31a 100644
--- a/include/Makefile.am
+++ b/include/Makefile.am
@@ -18,6 +18,7 @@ USER_H = \
$(top_srcdir)/include/libuutil.h \
$(top_srcdir)/include/libuutil_impl.h \
$(top_srcdir)/include/libzfs.h \
+ $(top_srcdir)/include/libzfs_core.h \
$(top_srcdir)/include/libzfs_impl.h
EXTRA_DIST = $(COMMON_H) $(KERNEL_H) $(USER_H) |
|
@imp Thanks. I guess I'm not too familiar with all the various make targets. All I've ever done are plain makes and installs. One of my test systems is actually CentOS 6.4 so I suppose I should try some of the other building options there. I'll put this into my work-in-progress branch (illumos-4445fff). NOTE: my WIP branch has already been rebased against against a newer master (but not tested). I'm planning on testing a rebase against a current master over the long weekend and then I'll push a new unified commit into the pull-2 branch. |
|
@dweeezil I needed to make the following change to build: diff --git a/module/zfs/dsl_dataset.c b/module/zfs/dsl_dataset.c
index a98d003..e289931 100644
--- a/module/zfs/dsl_dataset.c
+++ b/module/zfs/dsl_dataset.c
@@ -1109,7 +1109,7 @@ dsl_dataset_destroy(dsl_dataset_t *ds, void *tag, boolean_
*/
if (ds->ds_phys->ds_bp.blk_fill == 0 &&
dmu_objset_userused_enabled(os)) {
- uint64_t count;
+ ASSERTV(uint64_t count);
ASSERT(zap_count(os, DMU_USERUSED_OBJECT, &count) != 0 ||
count == 0); |
|
@behlendorf I did actually notice the unused variable warnings the last time I compiled it. I had thought that warnings were promoted to errors but apparently, at least on my current systems, they aren't. I just cherry-picked this and a Makefile fix found by @imp into the illumos-4445fff-pull-2 branch (from my WIP illumos-4445fff branch). As previously mentioned, I'd like to get this rebased to a newer master and then I'll roll it all into a single commit at that time. |
Observed while testing with the zconfig.sh script. It appears that during module remove when we're cleaning up the tsd_hash we find some remaining entries. Those entries now have a destructor registered which we try to call but that symbol was in the zfs module which has been removed from the system. zfs should make sure it destroys the zfs_allow_log_key in |
|
@behlendorf I thought I had fixed that in dweeezil/zfs@944351e (commit from my WIP branch). I wonder if the tsd_destroy isn't in the right place. |
|
@behlendorf Hmm, somehow that didn't make it into my all-in-one commit in the illumos-4445fff-pull-2 branch. Now I wonder what else might be missing. I either didn't squash all the commits correctly or I snuck in a few fixes after making the unified commit. |
|
@behlendorf OK, I'm really sorry. Somehow I totally screwed up the commit for the pull request. I'll have to re-do it. In the mean time, the equivalent is supposed to be on my illumos-4445fff-orig branch. That branch is my original work and is based on master from early May. The illumos-4445fff branch is based on a much newer master. I'm going to re-make the pull request commit one more time. |
|
@behlendorf and @imp and anyone else trying this, I just re-did this pull request and now, hopefully, it represents what I really wanted to have here. It's based on master as of 0377189 and includes all my work-in-progress as well as the 2 cherry-picked commits to deal with the Makefile and the unused count variable. |
|
@dweeezil I haven't verify using the refreshed branch but I'm also seeing zconfig tests 3,4 failing because the ZoL specific snapdev properly isn't being honored properly. Is this something you addressed in you latest version? |
|
@behlendorf In all honesty, I don't remember there being any issues specific to the snapdev property, however, I've run all of the zconfig tests plenty of times and they all passed. I just looked again at the stuff that was missing from the earlier pull request and it looks to me like the other big missing chunk is the zvol minor handling code from dweeezil/zfs@6ed1435 and dweeezil/zfs@357fa8e. |
|
@dweeezil OK, I'll refresh to your latest patch and give it a spin again. The failure I saw was related to minor creation. |
|
@behlendorf I just tried the new illumos-4445fff-pull-2 branch (dweeezil/zfs@751c7d0) on a test Ubuntu 12.04 system and it does seem to, at least, pass the zconfig.sh tests. |
|
@dweeezil I'm unable to reproduce this issue with the refreshed code. Currently I'm running xfstests and filebench against it I'll let you know if they turn up anything. |
|
I just refreshed the illumos-4445fff-pull-2 (and also the underlying illumos-4445fff) branch with a rebase on this evening's master and consolidated the 2 extra commits into it. |
|
I just refreshed to today's master and also included a few other fixes from dweeezil/zfs@aa2bc8c. I discovered a couple of missing spa_writeable() changes and an fnvlist_free() call that I missed because when its illumos patch (illumos/illumos-gate@347eec8) was ported to ZoL (9e11c7e), ZoL hadn't quite "caught up" yet and the call wasn't needed. |
|
My patch caused a regression in dsl_dataset_destroy() due to an improper patch. See dweeezil/zfs@bbb374a for details. I just pushed dweeezil/zfs@84d8e54 to fix it. |
|
Just pushed dweeezil/zfs@a0dc667 to rebase on a current master. I'm trying to keep this patch current because my work on Illumos 3464 (ZoL issue #1495) must be based on it. |
|
@dweeezil Expect fairly little churn is master over the next week or so while we finalize the code for the 0.6.2 tag. It would be great if this branch was in a state ready to merge after we cut that tag. |
|
@behlendorf This branch appears to be ready to merge. |
|
@ryao Just got back from vacation. I'm getting this rebased and tested right now. It seems to apply to a current master pretty well. |
|
I just pushed dweeezil/zfs@1726a6b based on today's master. It seems to work, however, there are a number of stack size warnings generated when I run ztest.sh. Also, I applied this little patch to suppress a pointer type warning: I'm going to install this on a semi-production server now. |
|
This commit contains code from Illumos 3464. ryao/zfs@bd06e62 removes it. |
|
@ryao Ugh, while working on this one, the illumos 3464 came on to my radar which I realized was another major patch we needed. Unfortunately, you're totally correct in that a couple of chunks from that one leaked into this patch and I tried to back them out appropriately. As you likely realize, I've also got that one mostly ported and working (see #1495) but I guess I really should clean this up. Update... I've just read (finally) your comments in #1495. My Illumos 3464 branch does mostly work but there are some PUSHPAGE fixes necessary in the nvlist code, IIRC. Obviously, the 3464 patch totally relies on the "4445fff" branch and is based on same. That makes it a pain in the butt when I've got to fix something in the 4445fff branch because I've then got to repair the 3464 patch. Unfortunately, I just got off vacation and haven't had much quality time to spend working on this yet but I do feel I could get it cleaned up pretty quickly. How do you think we should proceed with this? |
|
@ryao I just pushed dweeezil/zfs@d138d88 which incorporates your illumos 3464 back-out. Now I've got to put that back into my issue-1495-illumos-3464-synctask branch. |
|
@behlendorf I just pushed dweeezil/zfs@1826f91 which should hopefully be the final change to this patch. I removed the various fnvlist*nosleep() functions I had added and replaced them with calls using the existing infrastructure. It is also now based on the most current master. When I originally did this patch, I wasn't aware of the "allocation handler" feature built in to the nvlist library. I'll be rebasing the commit in #1658 to this commit shortly. |
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 <[email protected]> Reviewed by: Chris Siden <[email protected]> Reviewed by: Garrett D'Amore <[email protected]> Reviewed by: Bill Pijewski <[email protected]> Reviewed by: Dan Kruchinin <[email protected]> Approved by: Eric Schrock <[email protected]> References: illumos/illumos-gate@4445fff Ported-by: Tim Chase <[email protected]> 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.
|
Pushed dweeezil/zfs@ac02a59 to refresh to current zfsonlinux/zfs master (0.6.2). |
|
@dweeezil Your latest patch passes all the automated testing cleanly and I had a chance to review it. It looks good. So if you don't have any last minute concerns I'm going to merge it in to master. Thank you for for working on this and keeping it up to date while we got 0.6.2 out the door. Once it's in master lots of systems are going to pick it up rather quickly so let's keep any eye out for regressions. |
|
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. At a minimum people should be aware that this change breaks the user/kernel ABI so the zfs/zpool utilities from master no longer inter-operate with the 0.6.2 modules. |
Notes from the Illumos commit:
From feature #2882:
We should implement libzfs_core, under the principles proposed here:
http://blog.delphix.com/matt/2012/01/17/the-future-of-libzfs/
This feature request will cover the first phase of this project,
specifically the following tasks:
From issue #2883:
From feature #2900:
ZoL-specific changes:
Misc generic fixes: