Change in vdsm[master]: oop: Use a single instance of IOProcess per SD

smizrahi at redhat.com smizrahi at redhat.com
Mon Sep 8 13:41:44 UTC 2014


Saggi Mizrahi has posted comments on this change.

Change subject: oop: Use a single instance of IOProcess per SD
......................................................................


Patch Set 4: Code-Review-1

(7 comments)

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 completely different things.

I'd call it "max_ioprocess_idle_time".
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 what going on pretty hard.

create _getRfhPool(clientName) and _getIOProcessPool(clientName)

have getProcessPool(clientName) be
def getProcessPool(clientName):
    if __oopImpl == IOPROC:
        return _getIOProcessPool(clientName)
    else:
        return _getRfhPool(clientName)

That way it's simple to follow what is related to what.
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 things up a bit.
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. In any case, it's better to use the monotonic system timer and not the system clock as the former cannot be set.


you can get it with:

elapsed_time = lambda : os.times()[4]


Also, it's better to put the EOL time and not the last-use time. That way you leave the EOL calculation to the function that creates the instance in the list and not the one that clears it.


Practically:


84:

_procPool[clientName] = (elapsed_time() + DEF_IOPROCE_TIMEOUT, proc)


74:

now = elapsed_time()
for clientName, (eol, proc) in _procPool.items():
    if (el < now):
       del _procPool[clientName]
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:

  proc = _refProcPool[clientName]()
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.

del _refProcPool[clientName]
Line 82:                     raise KeyError
Line 83:                 else:
Line 84:                     _procPool[clientName] = (time.time(), proc)
Line 85:                     return proc


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
Line 95:                 _refProcPool[clientName] = weakref.ref(proc)
Line 96:                 procref = _refProcPool[clientName]
Line 97: 
Line 98:                 return procref()


-- 
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