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 6815179, 6844191, 5518, 1778 #4252

Closed
wants to merge 3 commits into from

Conversation

behlendorf
Copy link
Contributor

Port the parallel zpool scanning code and subsequent changes from Illumos to Linux. This was skipped long ago since it was never critical and it depended on the illumos specific thread_pool interface. We definitely don't want to support yet another compatibility interface, luckily the taskq interfaces are almost identical. Therefore, the patch was updated to use taskqs instead. Additional porting comments can be found in the commit comments.

Illumos 1778 - Assertion failed: rn->rn_nozpool == B_FALSE
Illumos 5518 - Memory leaks in libzfs import implementation
Illumos 6815179, 6844191

@odoucet
Copy link

odoucet commented Jan 21, 2016

If I understand correctly, zpool import will be magnitude faster with this. Any numbers ?

@behlendorf
Copy link
Contributor Author

@odoucet yes that's the idea. How much of a speed up depends entirely on how many entries exist under /dev/ to be scanned. As a test I created 500 symlinks under /dev/ to scan. This roughly corresponds to a pool with 32 disks since each disk will have 2 partitions (plus the base device) and 5 symlinks to each entry is common (32 * 3 * 5 = 480).

In this configuration I saw the import time drop from ~5.71 seconds to ~0.36. That's roughly a 15x improvement so this patch definitely does help. As always your mileage may vary.

@odoucet
Copy link

odoucet commented Jan 21, 2016

OK great. Considering your startup time, I assume you have very small amount of zfs volumes or snapshots.
zfs import is really slow when there is a lot of volumes / snapshots (it takes 15 minutes with ~ 10k snapshots and 32 drives like in your example). Will this behaviour be speed up too in the same order of magnitude ?

@behlendorf
Copy link
Contributor Author

@odoucet this change this just optimizes the drive scanning so it's not going to help in your case. But there are other which were being developed speed up the issue your seeing.

@odoucet
Copy link

odoucet commented Jan 21, 2016

Are you referring to Illumos 5269 ?
illumos/illumos-gate@12380e1

@behlendorf
Copy link
Contributor Author

No, actually #3830 which is overdue for a code review and some testing.

6815179 zpool import with a large number of LUNs is too slow
6844191 zpool import, scanning of disks should be multi-threaded

References:
  illumos/illumos-gate@4f67d75

Porting notes:
- This change was originally never ported to Linux due to it
  dependence on the thread pool interface.  This patch solves
  that issue by switching the code to use the existing taskq
  implementation which provides the same basic functionality.
  However, in order for this to work properly thread_init()
  and thread_fini() must be called around to taskq consumer
  to perform the needed thread initialization.

- The check_one_slice, nozpool_all_slices, and check_slices
  functions have been disabled for Linux.  They are difficult,
  but possible, to implement for Linux due to how partitions
  are get names.  Since this is only an optimization this code
  can be added at a latter date.

Signed-off-by: Brian Behlendorf <[email protected]>
5518 Memory leaks in libzfs import implementation
Reviewed by: Dan Fields <[email protected]>
Reviewed by: Matthew Ahrens <[email protected]>
Reviewed by: Serghei Samsi <[email protected]>
Approved by: Dan McDonald <[email protected]>

References:
  https://www.illumos.org/issues/5518
  illumos/illumos-gate@078266a

Porting notes:
- One hunk of this change was already applied independently in
  commit 4def05f.

Signed-off-by: Brian Behlendorf <[email protected]>
1778 Assertion failed: rn->rn_nozpool == B_FALSE
Reviewed by: Matthew Ahrens <[email protected]>
Reviewed by: George Wilson <[email protected]>
Reviewed by: Richard Elling <[email protected]>
Approved by: Gordon Ross <[email protected]>

References:
  https://www.illumos.org/issues/1778
  illumos/illumos-gate@bd0f709

Signed-off-by: Brian Behlendorf <[email protected]>
@behlendorf
Copy link
Contributor Author

Merged as:

ee42b3d Illumos 1778 - Assertion failed: rn->rn_nozpool == B_FALSE
0fdd8d6 Illumos 5518 - Memory leaks in libzfs import implementation
519129f Illumos 6815179, 6844191

@behlendorf behlendorf closed this Jan 23, 2016
@behlendorf behlendorf deleted the illumos-5518 branch May 18, 2018 18:38
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.

2 participants