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