Change in vdsm[master]: oop: Use a single instance of IOProcess per SD
ykaplan at redhat.com
ykaplan at redhat.com
Wed Sep 10 14:43:54 UTC 2014
Yeela Kaplan has posted comments on this change.
Change subject: oop: Use a single instance of IOProcess per SD
......................................................................
Patch Set 4:
(10 comments)
http://gerrit.ovirt.org/#/c/31501/4/lib/vdsm/config.py.in
File lib/vdsm/config.py.in:
Line 318: 'Process pool configuration.'),
Line 319:
Line 320: ('process_pool_timeout', '60', None),
Line 321:
Line 322: ('ioprocess_timeout', '60', 'TTL of IOProcess that is not renewed'),
> TTL of an unused IOProcess instance
Done
Line 323:
Line 324: ('process_pool_max_slots_per_domain', '10', None),
Line 325:
Line 326: ('process_pool_max_queued_slots_per_domain', '10', None),
http://gerrit.ovirt.org/#/c/31501/4/vdsm/storage/outOfProcess.py
File vdsm/storage/outOfProcess.py:
Line 46:
Line 47: _oopImpl = RFH
Line 48:
Line 49: DEFAULT_TIMEOUT = config.getint("irs", "process_pool_timeout")
Line 50: DEF_IOPROC_TIMEOUT = config.getint("irs", "ioprocess_timeout")
> process_pool_timeout and ioprocess_timeout are confusing as they mean compl
Done
Line 51: HELPERS_PER_DOMAIN = config.getint("irs", "process_pool_max_slots_per_domain")
Line 52: MAX_QUEUED = config.getint("irs", "process_pool_max_queued_slots_per_domain")
Line 53:
Line 54: _procPoolLock = threading.Lock()
Line 67: _oopImpl = RFH
Line 68:
Line 69:
Line 70: def getProcessPool(clientName):
Line 71: try:
> There are two different logic paths intertwined and that makes following wh
Done
Line 72: if _oopImpl == IOPROC:
Line 73: with _procPoolLock:
Line 74: for item in _procPool.items():
Line 75: if (time.time() - item[1][0]) > DEF_IOPROC_TIMEOUT and \
Line 69:
Line 70: def getProcessPool(clientName):
Line 71: try:
Line 72: if _oopImpl == IOPROC:
Line 73: with _procPoolLock:
> 74-78 could be it's own function [cleanIdleIOProcesses()] to simplify thing
Done
Line 74: for item in _procPool.items():
Line 75: if (time.time() - item[1][0]) > DEF_IOPROC_TIMEOUT and \
Line 76: item[0] != clientName:
Line 77: del _procPool[item[0]]
Line 71: try:
Line 72: if _oopImpl == IOPROC:
Line 73: with _procPoolLock:
Line 74: for item in _procPool.items():
Line 75: if (time.time() - item[1][0]) > DEF_IOPROC_TIMEOUT and \
> You could only get time.time() once before the loop to save on system calls
Done
Line 76: item[0] != clientName:
Line 77: del _procPool[item[0]]
Line 78:
Line 79: procref = _refProcPool[clientName]
Line 75: if (time.time() - item[1][0]) > DEF_IOPROC_TIMEOUT and \
Line 76: item[0] != clientName:
Line 77: del _procPool[item[0]]
Line 78:
Line 79: procref = _refProcPool[clientName]
> you could just do:
Done
Line 80: proc = procref()
Line 81: if proc is None:
Line 82: raise KeyError
Line 83: else:
Line 77: del _procPool[item[0]]
Line 78:
Line 79: procref = _refProcPool[clientName]
Line 80: proc = procref()
Line 81: if proc is None:
> We don't want another leak.
Done
Line 82: raise KeyError
Line 83: else:
Line 84: _procPool[clientName] = (time.time(), proc)
Line 85: return proc
Line 83: else:
Line 84: _procPool[clientName] = (time.time(), proc)
Line 85: return proc
Line 86: else:
Line 87: return _rfhPool[clientName]
> don't you need that also under the _procPoolLock?
Done
Line 88: except KeyError:
Line 89: with _procPoolLock:
Line 90: if _oopImpl == IOPROC:
Line 91: proc = _OopWrapper(IOProcess(max_threads=HELPERS_PER_DOMAIN,
Line 90: if _oopImpl == IOPROC:
Line 91: proc = _OopWrapper(IOProcess(max_threads=HELPERS_PER_DOMAIN,
Line 92: timeout=DEFAULT_TIMEOUT,
Line 93: max_queued_requests=MAX_QUEUED))
Line 94: _procPool[clientName] = (time.time(), proc)
> See previous comments
Done
Line 95: _refProcPool[clientName] = weakref.ref(proc)
Line 96: procref = _refProcPool[clientName]
Line 97:
Line 98: return procref()
Line 99: else:
Line 100: _rfhPool[clientName] = _OopWrapper(
Line 101: RemoteFileHandlerPool(HELPERS_PER_DOMAIN))
Line 102:
Line 103: return _rfhPool[clientName]
> i'd put all of it under "with _procPoolLock"
Done
Line 104:
Line 105:
Line 106: def getGlobalProcPool():
Line 107: return getProcessPool(GLOBAL)
--
To view, visit http://gerrit.ovirt.org/31501
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I383fe617ee4ce22de368ba54f980887d70ff37c5
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan 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