Recent illumos/Delphix features #2149
Conversation
|
Nice. Thanks for doing this. Let's plan to get it in shortly after we close out 0.6.3. |
|
@behlendorf I agree. We don't want to be merging disk format changes this late in the 0.6.3 cycle. |
|
I would like to check, if this solves the inactivity problem at #1761. but it didnt compile, something is wrong here. range_tree.h is somehow missing. make[5]: Entering directory `/usr/src/linux-3.4.33-2.24' |
|
I only build out-of-tree so I missed this. Add it to include/sys/Makefile.am and re-autogen & configure. |
|
I'll push a fixed patch stack later today that fixes this and also fixes a handful of unused variables (I also only build with --enable-debug while testing). |
|
Refreshed the patch stack to fix the missing range_tree.h header file from Makefile.am and also to fix some unused variables in non-debug mode. |
|
There are 2 other missing *.h files in Makefile.am, but nevermind. Corrected it, compiled. After that i tried to zfs recv a zfs send stream ( same as #1761). It runs at full speed for some minutes. I was happy. But then it stops : cannot receive: invalid backup stream. No luck today |
|
dweeezil/zfs@aec4797 contains a regression from illumos/illumos-gate@0713e23. The metaslab_group_tasksq that is created leaks, causing thread exhaustion with a sufficient number of pool imports. It is documented on Illumos ZFS list: |
|
@ryao I believe I found that problem on my own in a completely separate context: namely that zdb wouldn't exit properly and fixed it in dweeezil/zfs@69b0687 as a separate commit (which is included in my current patch stack). |
|
Added some more recent changes from Illumos. Rebased on a current master. |
|
@dweeezil Only 4 issues remain between us and an 0.6.3 tag. Once I cut that tag I'd like to start testing and merging these changes to catch us back up. Since you refreshed this branch just a couple weeks ago I suspect it is already up to date, but if it's not could you please refresh it. Thanks! |
|
I think instead of dweeezil@020123c you should now be able to pull in the almost identical illumos/illumos-gate@be08211. I believe the bug was originally introduced in illumos/illumos-gate@0713e23. |
|
@csiden Thanks for mentioning this. In my distant fuzzy memory, I seem to recall someone coming across my dweeezil/zfs@020123c patch on another mailing list but, as you mentioned, it got pulled into illumos in a slightly different form. I'll replace it with the illumos version and reference to same when I refresh the patch stack. |
|
@dweeezil I'd like to spend some time this week looking these patches over and getting some confidence with them. If you could push any pending work, I'd appreciate it; that'd keep me from re-reviewing the patches a second time around. Hopefully we can get this work merged shortly after the 0.6.3 tag is out. |
|
Hmm.. I'm not even able to compile the pull request as it stands on a Fedora 20 system: As far as I can tell, neither |
|
Fixing the above uncovered this failure: |
|
@behlendorf How would you feel about improving the commit message for illumos ports like these? I really dislike the fact that there's virtually no useful information in the commit message, and I'd hate to adopt this convention for ported patches simply because upstream works this way. Especially since we often bring in large changes via upstream ported patches, and these really need proper commit messages and documentation. I think it'd be good to keep the original commit message in there somewhere (to aid searching through the illumos tree, bug tracker, etc), but also add some useful commentary to "our" commit message as is usually done; we're rewriting the commit SHA anyways so I don't see much downside. |
|
Historically when we're pulling in Illumos changes we at a minimum preserve their commit message and reference the upstream commit. However, there are many examples in our tree where we add another section to the commit with detailed notes. Often they describe changes which were needed for Linux or just expound on the reason for the patch. So I have absolutely no objection to making the commit messages as useful and relevant as possible. As you said, there's really no downside. |
|
@prakashsurya There is a corresponding spl branch. I can't remember whether I've made a pull request yet for it. For the moment, however, it is in dweeezil/spl@5bbbe57. As to the brief commit logs, I fully agree with your assessment. I was simply following the convention (which I only learned through observation). One good source for better commit messages is the illumos issue tracker and I think in some cases, that text has been used as a basis for a better commit log message. |
|
I created openzfs/spl#363 to correspond with #2149. |
|
@dweeezil Would you be opposed to me splitting the individual commits apart into separate pull requests? I think some of them will take longer to review and test than others, and I'd hate to hold the smaller changes. |
|
@prakashsurya I'm not sure that would work. Separate pull requests would imply a separate git branch for each of them. Some/all of the latter patches require that the previous ones already exist. What would the commit history look like for a hypothetical standalone version of one of the last two commits? They both depend on the extensible dataset facility which would have to already exist. In terms of ultimate grouping, yes, the first two could be a single pull request and then each of the others could be their own pull request but those pull requests' git branch would have to be based on something and that "something" doesn't currently exist. Also, BTW, dweeezil/zfs@63153ae has been folded into the illumos code so we could use their patch rather than mine (which is non-functionally different) in order to maintain better "commit compatibility". I will try to get the rest of the patches layered atop this ASAP. I think we could do this as separate commits once 0.6.3 is tagged. Then we could do a cycle of:
|
|
@dweeezil Ah OK. I've only looked at the very first patch, which is isolated; but if the other commits have dependencies then we can't do that. So, let's just keep that stack as is. |
|
@prakashsurya Yep, it's the metaslab histogram stuff. Matt talks about it on one of the youtube videos he's in (forgot which one). There are also some decent comments on the illumos issue pages. Reviewing it a bit, myself, reminds me that I was surprised that we had to use kmem_zalloc() in places where the original code could use kmem_alloc() and made me wonder whether the latter zeroed memory in illumos. This and one of the following were what I was referring to on the mailing list in response to the issue in which the person's pool got slow when it was near full. I think the combination of this one and the latter vdev-avoiding code will handle those problems in which a pool was filled and fragmented and then more vdevs were added. |
|
@prakashsurya I don't think your description of the zdb histogram patch is quite correct. As an extension to the "-b" option, it's producing a histogram per DMU object type and not a histogram for a particular object. Your example output above is showing the summary for all blocks being used to store DMU dnodes. There are currently 23 blocks being used to store DMU dnodes and the histogram shows the block size distribution among those. A more interesting item to look at would be, for example, "ZFS plain file". For example, on an otherwise clean pool, I do the following: to create 3 files, each containing a single block of the given size. We see the following: Of course things get more interesting when we have files consisting of more than one block. I'll create a file consisting of 2 1k blocks: zdb -ddddd shows me this for the file: we now have an L1 block involved and see the following in the "-b" histogram: The L1 is new and the two L0 blocks are accumulated in the histogram under the 1K (2) bin. I'm pretty certain I'm explaining this correctly. In summary, the feature is used to get a distribution of the block sizes used within the pool on a per-object-type basis. |
|
@dweeezil Ah, yes, you are right. And that's part of the reason I like to have human understandable explanations in the commit message! I misunderstood what I was looking at with only the code as a reference, but it's much more clear now with your explanation. I'd love to see that rolled into our commit message when the patch lands. As far as the histogram patch, I'm really keen on getting that in (I have a hunch we're seeing performance issues due to fragmentation in production pools), it just might take some time to fully understand it with all the churn in the patch. I've been inspecting it off and on the past couple days. It probably will work just fine for us, assuming it's been working just fine upstream; I'm just cautious. Subtle differences between linux and illumos are easy to over look, and can cause a lot of grief. |
|
Status update: I'm testing the big illumos 4757 (data embedded in blkptrs) update now. There's one other small related fix I want to get into this patch stack and then I'll consider it "done" (as in it should give us all the major new features that I was interested in an think are somewhat related). |
|
@behlendorf What's the story with c90 vs. c99 compilation? We seem to run c99-isms all the time when porting upstream code. In the illumos 4757 patch there were a ton of new macros which used comma operators, conditionals, etc. which I had to convert to inline functions in order to compile. Are we (ZoL) locked into c90 because of the Linux kernel build environment? Are there plans up move to c99? In any case, I've got the 4757 commit working and have given it light testing. This one could be a real boon to the zillions-of-small-compressible-files crowd. DANGER! Both the 4757 feature and possibly others will render pools unbootable by most versions of grub! I do plan on porting the necessary changes to grub (to the canonical upstream version because that's the only version I've been using) and hopefully the author will pick them up. NOTE[2]: There is one more minor related patch I want to apply which fixes a regression with the embedded blkptr feature. |
|
@behlendorf After sleeping on the c90/c99 issue, I played with it a bit more this morning and it seems these new macros are doing something a lot more than just c99 stuff. I must admit that I didn't fully unwind the macros to see what they were doing and had simply assumed they were using c99 features. I tried building (arc.c) with a bunch of various -std=XXX options (including -std=gnu1x) and nothing worked. The macros are all the new ones in spa.h like BPE_GET_ETYPE(). |
|
The issue is our definition of ASSERT() in spl. I thought the do/while construct was coming from one of the new SPA macros but it was actually coming from ASSERT(). The problem is that these new macros contain ASSERT()s. |
4757 ZFS embedded-data block pointers ("zero block compression")
4913 zfs release should not be subject to space checks
Reviewed by: Adam Leventhal <ahl@delphix.com>
Reviewed by: Max Grossman <max.grossman@delphix.com>
Reviewed by: George Wilson <george.wilson@delphix.com>
Reviewed by: Christopher Siden <christopher.siden@delphix.com>
Reviewed by: Dan McDonald <danmcd@omniti.com>
Approved by: Dan McDonald <danmcd@omniti.com>
Ported by: Tim Chase <tim@chase2k.com>
|
I reverted the macros to their original form. We'll need to figure out a way to allow compilation |
4390 i/o errors when deleting filesystem/zvol can lead to space map corruption Reviewed by: George Wilson <george.wilson@delphix.com> Reviewed by: Christopher Siden <christopher.siden@delphix.com> Reviewed by: Adam Leventhal <ahl@delphix.com> Reviewed by: Dan McDonald <danmcd@omniti.com> Reviewed by: Saso Kiselkov <saso.kiselkov@nexenta.com> Approved by: Dan McDonald <danmcd@omniti.com> Ported by: Tim Chase <tim@chase2k.com> Porting notes: Previous stack-reduction efforts in traverse_visitb() caused a fair number of un-mergable pieces of code. This patch should reduce its stack footprint a bit more. The new local bptree_entry_phys_t in bptree_add() is dynamically-allocated using kmem_zalloc() for the purpose of stack reduction. The new global zfs_free_leak_on_eio has been defined as an integer rather than a boolean_t as was the case with the related zfs_recover global. Also, zfs_free_leak_on_eio's definition has been inserted into zfs_debug.c for consistency with the existing definition of zfs_recover. Illumos placed it in spa_misc.c.
4881 zfs send performance degradation when embedded block pointers are encountered Reviewed by: George Wilson <george.wilson@delphix.com> Reviewed by: Christopher Siden <christopher.siden@delphix.com> Approved by: Dan McDonald <danmcd@omniti.com> Ported by: Tim Chase <tim@chase2k.com>
|
I just added the last patch which makes sense for this patch stack and will now start testing it more. I'll also be working on patches for grub to support the embedded data block pointers. Preliminary tests show the embedded block feature has the potential to save quite a bit of space even on filesystems without lots of small files; since the metadata (indirect blocks) are compressed, they can also take advantage of embedded data. As a quick benchmark, a copy of a typical Ubuntu /etc directory hierarchy on an uncompressed filesystem has 2062 embedded data block pointers and enabling lz4 compression increases it to 4170. Those number should directly related to blocks of saved space. |
The kernel build system forces c90 compilation and I haven't seen anything to indicate they're interested in changing that. There were lightly tested patches to override that default so we could build c99. But thus far I'm only 99% sure that's safe. In the interest of not introducing any mysterious new bugs we've kept the code c90.
That's great! I've been looking forward to this one. Your initial results are really encouraging! |
|
@behlendorf What are your thoughts on something like dweeezil/spl@8fd4e47 to deal with |
|
The new ASSERT looks like a reasonable approach. I suspect you could have just dropped the do/while bit and left the braces to define the block. But moving to an && operator makes good sense too. Why did you end up adding a PANIC1 wrapped? Was it just additional cleanup or was it needed for some reason? Also if we do this we'll want to get ASSERTF, VERIFY, and any other similar constructs. |
|
My concern about simply dropping the do/while in ASSERT is that it might have subtly broken one of the 2092 (!) uses of it within the ZFS code, but you're right that the odds are that all of them would have worked just fine. The reason for wrapping PANIC in an inline wrapper as PANIC1 is because PANIC(), itself has a do/while which we can't have expanded within the "&&" of the new ASSERT. I do agree that it would make sense to update the other constructs such as ASSERTF and VERIFY but I only did ASSERT because that's all it took in order to get the new code to compile. Since it sounds like you're OK with this approach, I'll update the patch to similarly modify the other macros as you suggested and then submit it as a standalone pull request which should be perfectly compatible with the existing code and should be able to be merged prior to all this new core ZFS stuff. Doing the job completely looks like it'll require va_list versions of spl_debug_msg & friends (which I'll name using the canonical "v" prefix). |
|
@dweeezil Just an update. I've brought in the needed macro changes to the SPL which should make testings this easier. Also @prakashsurya been reviewing this changes as well so we can get comfortable with merging them. |
|
@behlendorf Thanks. I did notice the SPL macro changes were merged. I've not done much other than rudimentary testing on this patch stack yet. I've been working on some unrelated ZFS bugs in grub prior to trying to port the necessary grub changes for these new features. |
|
@dweeezil @behlendorf I just opened #2456 with the single Illumos 3641 patch. This comment thread here is getting a bit unwieldy, so I wanted to try and split that patch out and get it landed quickly. I plan to do the same with the space_map histogram patch (and associated bug-fix) once I'm more comfortable with it. |
|
@dweeezil Won't make debs without: "insert where needed" :). Have just compiled it and so far so good. I'll start some testing later (I'm busy with other things at the moment, and I only have one test bed, which is busy, at the moment). |
|
@FransUrbo It looks like af5f9b1 is the |
|
@ryao just pointed me to illumos/illumos-gate@30beaff I think we need to pull in that fix as well. |
|
@prakashsurya Yes, I got the email from him as well. A quick peek at it makes me think it ought to layer on to the patch stack cleanly. |
|
As per my chat with @prakashsurya in IRC, I think we should squash illumos-gate@0713e232b7712cd27d99e1e935ebb8d5de61c57d, illumos-gate@30beaff42d8240ebf5386e8b7a14e3d137a1631f and illumos/illumos-gate@be08211 into a single commit to avoid laying unnecessary traps for people doing That being said, the commit messages need to be amended to include references to the Illumos repository to make it easy to access the upstream commits from a web browser. It also should make it easy to catch attribution mistakes when performing review. For instance, illumos/illumos-gate@be08211 is by George Wilson, but dweeezil/zfs@6b0d278 misattributes it to Christopher Siden. I have not checked the others, but I did just check these three because they were candidates to squash. All commits that I ported in the recent past included references to the Illumos repository for these reasons. |
|
@ryao Yes, I concur that those 3 commits ought to be squashed. I'm thinking I ought to re-roll this entire stack over the weekend to fix a number of things that have been found. So far, it looks like:
I added the last one because it looks to me like some of the new parameters in the metaslab code should be adjustable. EDIT: Add the following:
|
|
I think in light of @prakashsurya's decomposed pull requests, I'm going to leave this stack alone as a reference point for review. Apparently there's no problem making a pull request that's based on commits which haven't been committed to master. |
|
I opened a new pull request here: #2488 It has the illumos 4101/2/3/5/6 patch allong with the two illumos bug fix patches. I've also split up the fix for metaslab_group_alloc_update into a separate patch as @ryao suggested. I did not squash them all into a single commit, though. I'm not yet convinced that's the "right" approach to take, but that can easily be done by @behlendorf if/when it's landed. I added the minor fixes @dweeezil mentioned in the comment above: I didn't pay much attention to author attribution, so if I made any mistakes there please speak up. Also, we can add another patch to export any parameters that we deem useful. |
|
@dweeezil @prakashsurya I'd like to close this issue now that some of these patches have been merged as part of #2488. @dweeezil it would great if you could open a new pull request with the remaining patches rebased against master. Then we can make another push to get them reviewed and merged. |
|
I'm going to create stacked pull requests for the rest of this patch stack. Note, too, that I'm re-doing the first few commits of the stack because I found a few mistakes and also found some cross-patch merging that snuck in. |
This patch stack brings most of the missing ZFS features from Illumos into ZoL.
This set will need a lot of review and testing. There was a bit of interference with existing ZoL-specific features. In particular, the space map/metaslab changes interfered with FASTWRITE (920dd52) and the bookmark facility interfered with the
zfs list -t snapshotspeedups in 0cee240.Also, the hole-send-avoiding code introduced the need for atomic_swap_64() which we don't (seem to) have in the Linux kernel so I punted and tried to emulate it with a spinlock.
Other than that, these changes ported fairly cleanly and should bring the ZoL code base much more in line with current Illumos code. The usual disclaimers apply for the moment: don't use on production pools, etc. It does, however, seem to function and has passed 10-20 minutes of
ztesttesting.