Change in vdsm[master]: hba: Rescan using SCSI layer
danken at redhat.com
danken at redhat.com
Sun Nov 23 20:20:48 UTC 2014
Dan Kenigsberg has posted comments on this change.
Change subject: hba: Rescan using SCSI layer
......................................................................
Patch Set 5: Code-Review-1
(4 comments)
http://gerrit.ovirt.org/#/c/34245/5//COMMIT_MSG
Commit Message:
Line 22: This patch restores the missing scsi scan for fc_hosts, enabling discovery of
Line 23: new FC LUNs in the multiapth rescan flow.
Line 24:
Line 25: Since scsi scan is a blocking operation, we perform the scan using external
Line 26: command line tool, ensuring that vdsm cannot get stuck. This tool can be also
vdsm cannot get stuck; but can scanning of one host block others?
Line 27: useful outside of vdsm.
Line 28:
Line 29: In the fc-scan tool, we perform the scan in parallel, so blocking on
Line 30: unresponsive fc_host will not prevent discovery of new devices on other
http://gerrit.ovirt.org/#/c/34245/5/lib/vdsm/constants.py.in
File lib/vdsm/constants.py.in:
Line 163:
Line 164: EXT_VDSM_TOOL = os.path.join('@BINDIR@', 'vdsm-tool')
Line 165:
Line 166: EXT_CURL_IMG_WRAP = '@LIBEXECDIR@/curl-img-wrap'
Line 167: EXT_FC_SCAN = '@LIBEXECDIR@/fc-scan'
Please use CommandPath or just the explicit path to your libexec. This constants module is bad since it exposes entities that should better be kept private to another module/subsystem.
http://gerrit.ovirt.org/#/c/34245/5/vdsm/storage/fc-scan
File vdsm/storage/fc-scan:
Line 66: log.debug("Scanning %s", path)
Line 67: start = time.time()
Line 68: fd = os.open(path, os.O_WRONLY)
Line 69: try:
Line 70: os.write(fd, "- - -")
If I recall correctly, we have seen this write causing the process to get into the D state, which means that concurrent threads for other hosts would block (possibly forever). Am I wrong?
Line 71: finally:
Line 72: os.close(fd)
Line 73: self.succeeded = True
Line 74: elapsed = time.time() - start
Line 100: sys.exit(0)
Line 101:
Line 102: scans = []
Line 103:
Line 104: for host in hosts:
have you considered using storage.misc.tmap()? there's a wide overlap with the suggested code.
Line 105: s = Scan(host)
Line 106: s.start()
Line 107: scans.append(s)
Line 108:
--
To view, visit http://gerrit.ovirt.org/34245
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7deb3047f1b75c4c65d59602b908835515290993
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list