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 4958, 5164, 5165 #2697

Closed
wants to merge 2 commits into from
Closed

Conversation

FransUrbo
Copy link
Contributor

4958 zdb trips assert on pools with ashift >= 0xe
Reviewed by: Matthew Ahrens mahrens@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
Approved by: Garrett D'Amore garrett@damore.org

References:
https://www.illumos.org/issues/4958
illumos/illumos-gate@2a104a5

Porting notes:
Keep the ZIO_FLAG_FASTWRITE define, because that is used in Linux but not in *BSD.

5164 space_map_max_blksz causes panic, does not work
5165 zdb fails assertion when run on pool with recently-enabled spacemap_histogram feature
Reviewed by: George Wilson george.wilson@delphix.com
Reviewed by: Christopher Siden christopher.siden@delphix.com

References:
https://www.illumos.org/issues/5164
https://www.illumos.org/issues/5165
illumos/illumos-gate@b1be289

Ported by: Turbo Fredriksson turbo@bayour.com

@FransUrbo FransUrbo force-pushed the illumos-5164+5165 branch 3 times, most recently from 7c72213 to 7e31172 Compare September 17, 2014 07:22
@FransUrbo
Copy link
Contributor Author

@behlendorf This have now been accepted into illumos-gate...

@behlendorf
Copy link
Contributor

This depends on https://www.illumos.org/issues/4958, we'll need to port illumos/illumos-gate@2a104a5 before this can be merged.

@behlendorf
Copy link
Contributor

As an aside, we really need to come up with some system for tracking which Illumos patches we have merged and which ones we haven't.

@FransUrbo
Copy link
Contributor Author

@behlendorf Do you want that into a separate PR or is it ok to put it in this one?

It might also be dependent on something more. It (Illumos 4958) removes ZIO_FLAG_FASTWRITE, but ZoL still have a lot of references to it...

@FransUrbo
Copy link
Contributor Author

As an aside, we really need to come up with some system for tracking which Illumos patches we have merged and which ones we haven't.

Yeah, I got nothing... You?

@behlendorf
Copy link
Contributor

@FransUrbo @kpande what would be idea (at least from my point of view) is if we could automate the process of automatically creating a new issue in the ZoL tracker for each change as it's merged in to Illumos. Arguably we should be more involved in the entire review process going on for Illumos but there are only so many hours in the day.

@behlendorf
Copy link
Contributor

@behlendorf Do you want that into a separate PR or is it ok to put it in this one?

It's OK to put it in this pull request, just make sure it's a separate patch.

It might also be dependent on something more. It (Illumos 4958) removes ZIO_FLAG_FASTWRITE, but ZoL still have a lot of references to it...

Support for ZIO_FLAG_FASTWRITE was never pushed upstream to Illumos and they never spent the time to port it. It's a Linux-only feature at the moment.

@FransUrbo
Copy link
Contributor Author

Support for ZIO_FLAG_FASTWRITE was never pushed upstream to Illumos and they never spont the time to port it. It's a Linux-only feature at the moment.

Ok, so should we keep that then?

@behlendorf
Copy link
Contributor

Yes. It's something we need to keep in mind when porting the patch.

@FransUrbo
Copy link
Contributor Author

Ok, I put back ZIO_FLAG_FASTWRITE in the Illumos 4958 port.

@FransUrbo FransUrbo changed the title Illumos 5164+5165 - space_map fixes Illumos 4958 - zdb trips assert on pools with ashift >= 0xe; Illumos 5164+5165 - space_map fixes Sep 24, 2014
@FransUrbo FransUrbo changed the title Illumos 4958 - zdb trips assert on pools with ashift >= 0xe; Illumos 5164+5165 - space_map fixes Illumos 4958, 5164, 5165 Sep 24, 2014
@FransUrbo
Copy link
Contributor Author

if we could automate the process of automatically creating a new issue in the ZoL tracker for each change as it's merged in to Illumos.

@behlendorf I'm not sure that's a good idea. A lot/some (?) of the commits isn't ZFS related…

But I hacked this http://bayour.com/misc/tail-illumos-gate-commits.txt real quick.

It will get a list of commits since last check (configured in /var/cache/illumos-gate) and will send them as separate mails (starting with the OLDEST it find).=

@behlendorf
Copy link
Contributor

Right. We'd want to somehow restrict this to zfs related changes.

@FransUrbo
Copy link
Contributor Author

@behlendorf I've updated it to do simple check and see if it's relevant.

@behlendorf
Copy link
Contributor

Neat, I'll check it out. It would be great if we could automate the majority of this process. Even if it isn't foolproof.

Reviewed by: Matthew Ahrens <mahrens@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>
Approved by: Garrett D'Amore <garrett@damore.org>

References:
  https://www.illumos.org/issues/4958
  illumos/illumos-gate@2a104a5

Porting notes:

Keep the ZIO_FLAG_FASTWRITE define, because that is used in
Linux but not in *BSD.

Ported by: Turbo Fredriksson <turbo@bayour.com>
5164 space_map_max_blksz causes panic, does not work
5165 zdb fails assertion when run on pool with recently-enabled spacemap_histogram feature
Reviewed by: George Wilson george.wilson@delphix.com
Reviewed by: Christopher Siden christopher.siden@delphix.com

References:
  https://www.illumos.org/issues/5164
  https://www.illumos.org/issues/5165
  illumos/illumos-gate@b1be289

Ported by: Turbo Fredriksson <turbo@bayour.com>
@FransUrbo
Copy link
Contributor Author

@behlendorf I've added my "check illumos commits" script to my scripts repo - https://github.com/FransUrbo/scripts/blob/master/tail-illumos-gate-commits.pl.

I've updated it somewhat to also include the issue link(s) in the mail. I've started running this on my own machine once a day to see it "in action"...

@behlendorf
Copy link
Contributor

@FransUrbo Regarding a "check illumos commits" script, I took at look at what you've written and it's a nice start. What I was thinking would be nice first step would be to add a utility like this to the scripts directory. But all it would do is the following:

  • Checkout the Illumos source to a temporary directory, /var/tmp/xxx
    • Maybe provide an option to point it at an existing tree.
  • Checkout the ZoL source to a temporary directory, /var/tmp/xxx
    • Maybe provide an option to point it at an existing tree.
  • Run through the last N commits in the Illumos source.
    • Automatically identify ZFS commits by some reasonable heuristic (perhaps filenames)
    • Compare the Illumos bug numbers in the commit against ZoLs commit history or a known white list of Illumos bug numbers which have been merged.
    • We could leave a state file committed in the ZoL tree which includes
      • The commit hash to start scanning from
      • A white list of ZFS hashes which have already been merged
      • A black list of non-ZFS hashes to be ignored as false positives
    • The script would output all ZFS commits in Illumous but not in ZoL.
  • If it were written in bash it would be able to source the common.sh script like many of the other helper scripts. This would make easy to run certain commands in tree.
  • Add changes were merged from Illumos we'd need to make a point to update the state file.

This might provide as a sane way to track what has and has not been pulled in from Illumos. If we wanted to go one step further we could extend it to attempt to auto-generate patches from those commits which apply. What do you think?

Also, I should be able to get these changes merged today. I've tweaked thing slightly and assuming testing doesn't uncover anything unexpected they'll go in.

behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Oct 23, 2014
5164 space_map_max_blksz causes panic, does not work
5165 zdb fails assertion when run on pool with recently-enabled
     space map_histogram feature
Reviewed by: Christopher Siden <christopher.siden@delphix.com>
Reviewed by: George Wilson <george.wilson@delphix.com>
Reviewed by: Saso Kiselkov <skiselkov.ml@gmail.com>
Approved by: Dan McDonald <danmcd@omniti.com>

References:
  https://www.illumos.org/issues/5164
  https://www.illumos.org/issues/5165
  illumos/illumos-gate@b1be289

Porting Notes:

The metaslab_fragmentation() hunk was dropped from this patch
because it was already resolved by commit 8b0a084.

The comment modified in metaslab.c was updated to use the correct
variable name, space_map_blksz.  The upstream commit incorrectly
used space_map_blksize.

Ported by: Turbo Fredriksson <turbo@bayour.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#2697
@behlendorf
Copy link
Contributor

Merged as:

9635861 Illumos 5164-5165 - space map fixes
b02fe35 Illumos 4958 zdb trips assert on pools with ashift >= 0xe

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