vdev_id: Support daisy-chained JBODs in multipath mode #10736
Conversation
Replaced reliance on userspace commands with sysfs data. Four JBOD, 240 SAS drive udev processing speedup from 3m30s to 12s. Signed-off-by: Contributor <jeff.johnson@aeoncomputing.com>
|
Probably should have cleaned up my previous commits into one, sorry for the confusion. 1b225f9 is the one that matters. |
Codecov Report
@@ Coverage Diff @@
## master #10736 +/- ##
===========================================
- Coverage 79.24% 63.02% -16.23%
===========================================
Files 400 296 -104
Lines 122012 102099 -19913
===========================================
- Hits 96687 64343 -32344
- Misses 25325 37756 +12431
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #10736 +/- ##
==========================================
- Coverage 79.24% 72.29% -6.95%
==========================================
Files 400 371 -29
Lines 122012 119441 -2571
==========================================
- Hits 96687 86355 -10332
- Misses 25325 33086 +7761
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Can you please run shellcheck and fix any issues it reports? Thanks! |
|
I can close this, run the shellcheck and do a new PR since I didn't rebase prior. Let me know. |
|
@AeonJJohnson thanks! What I'd suggest is fixing up the shellcheck warnings, squashing everything in to a single commit, then force update this PR. That'll kick off a new CI run. |
|
@behlendorf @gdevenyi how far do you want me to take the shellcheck cleanup? My additions only or the entire script where functions use local variables and all the other complaints shellcheck reports, even on the master vdev_id? |
|
@AeonJJohnson as you're currently probably the most well versed in this code I would love if you could tackle the whole file. I have it on my todos at #10512 but I would've had to learn exactly how vdev_id works, whereas you've already put in that work. I would suggest however that the fixes to your code you can just squash into your existing commits, however the fixes for the rest of the code be broken out separately. |
|
@gdevenyi I'll tackle the whole thing. Does it have to be 100% POSIX sh compliant? Retiring backticking I get, not using an array kinda drops a wrench into my multijbod approach. |
|
@AeonJJohnson good question but above my pay grade. @behlendorf can you comment on upgrading this script to bash only? |
|
As it happens I opened #10512 yesterday which takes care of the current warnings I saw in As for whether it needs to be POSIX sh compliant that's debatable. While bash is available basically everywhere there are a surprising number of distributions where it's not the default shell. If we want this to just work everywhere POSIX sh is the way to go. We could change the shebang to |
So I think this PR has gotten stuck on the question of whether we need to stick to POSIX sh or we can use bashisms. Can we get an official ruling from the devs that this script can't be upgraded to bash? |
|
I believe @AeonJJohnson sorted out the bashisms and was just working on a few more improvements. But I'd love to see this updated so we can get it included. |
|
While testing the current state of this to see if it can handle my JBOD (see #11095) I found that this version of the script won't run properly under sh: |
|
Updated version is under #11520 - It is almost same Jeff's code. Mostly rebased to latest, 3 patch squashed to single and minor posix compliant fix. My intention was to update this PR. Somehow could not get to update this PR. (Maybe permission?) - Once I figure what wrong is happening I will update this PR. Since this has lots of history and comments of reviewers. |
|
@arshad512 thanks! Migrating to a new PR would be fine if that's easier for you. We can always link back to this original PR so those commends and feedback won't be lost. |
|
Closing, replaced by #11526 |
Motivation and Context
The current version of vdev_id is not able to differentiate between devices in multiple/daisy-chained JBOD enclosures.
Description
Within function sas_handler() userspace commands like /usr/sbin/multipath have been replaced with sourcing device details from within sysfs which reduced a significant amount of overhead and processing time. Multiple JBOD enclosures and their order are sourced from the bsg driver (/sys/class/enclosure) to isolate chassis top-level expanders, which are then dynamically indexed based on host channel of the multipath subordinate disk member device being processed. Additionally added a "mixed" mode for slot identification for environments where a ZFS server system may contain SAS disk slots where there is no expander (direct connect to HBA) while an attached external JBOD with an expander have different slot identifier methods.
How Has This Been Tested?
Testing was performed on a AMD EPYC based dual-server high-availability multipath environment with multiple HBAs per ZFS server and four SAS JBODs. The two primary JBODs were multipath/cross-connected between the two ZFS-HA servers. The secondary JBODs were daisy-chained off of the primary JBODs using aligned SAS expander channels (JBOD-0 expanderA--->JBOD-1 expanderA, JBOD-0 expanderB--->JBOD-1 expanderB, etc). Pools were created, exported and re-imported, imported globally with
zpool import -a -d /dev/disk/by-vdev. Low level udev debug outputs were traced to isolate and resolve errors.Initial testing of a previous version of this change showed how reliance on userspace utilities like /usr/sbin/multipath and /usr/bin/lsscsi were exacerbated by increasing numbers of disks and JBODs. With four 60-disk SAS JBODs and 240 disks the time to process a
udevadm triggerwas 3 minutes 30 seconds during which nearly all CPU cores were above 80% utilization. By switching reliance on userspace utilities to sysfs in this version, theudevadm triggerprocessing time was reduced to 12.2 seconds and negligible CPU load./dev/disk/by-vdev
zpool status
Types of changes
Checklist:
Signed-off-by]