Adam Litke has uploaded a new change for review. Change subject: storage: Introduce guarded utilities ......................................................................
storage: Introduce guarded utilities Throughout the storage code we have implicit locking rules which we follow in order to ensure that concurrent operations will not conflict with one another. We also require a specific locking order to prevent deadlocks. This code is repeated throughout the storage subsystem and is error prone. To alleviate this, introduce the guarded module with a context manager that can perform the necessary locking. At first we will support only resourceManager locks but this model can be easily extended to a support for volume leases. The contextmanager accepts a list of entities which need to be guarded within the context. It then collects all of the required locks from the entities and locks them according to the rules. When exiting the context (or upon error) the locks are released in reverse order. Entities must provide the get_rm_locks() method which returns a list of ResourceManagerLocks required. The following rules are currently enforced: - Locks are collected from all entities and sorted - Duplicate locks are removed - Exclusive mode takes precedence over shared mode Change-Id: I2b0a204818d44b6205515277f4c2834cb2b7a057 Signed-off-by: Adam Litke <[email protected]> --- A lib/vdsm/storage/guarded.py M tests/Makefile.am A tests/storage_guarded_test.py M vdsm.spec.in 4 files changed, 236 insertions(+), 0 deletions(-) git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/35/61435/1 diff --git a/lib/vdsm/storage/guarded.py b/lib/vdsm/storage/guarded.py new file mode 100644 index 0000000..5614cf0 --- /dev/null +++ b/lib/vdsm/storage/guarded.py @@ -0,0 +1,94 @@ +# +# Copyright 2016 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +# +# Refer to the README and COPYING files for full details of the license +# + +from __future__ import absolute_import +import logging + +from storage import resourceManager as rm + +log = logging.getLogger('storage.guarded') +rmanager = rm.ResourceManager.getInstance() + + +class operation_context(object): + + def __init__(self, entities): + self._rm_locks = self._collect_rm_locks(entities) + self._held_locks = [] + + def __enter__(self): + try: + for lock in self._rm_locks: + lock.acquire() + self._held_locks.append(lock) + except: + self._release() + raise + + def __exit__(self, exc_type, exc_val, exc_tb): + self._release() + + def _collect_rm_locks(self, entities): + locks = [] + for entity in entities: + locks.extend(entity.get_rm_locks()) + + # Exclusive locks always take precedence over shared + lock_set = set([l for l in locks if l.mode == rm.LockType.exclusive]) + shared_locks = set([l for l in locks if l.mode == rm.LockType.shared]) + lock_set.update(shared_locks) + return sorted(lock_set) + + def _release(self): + while self._held_locks: + lock = self._held_locks.pop() + lock.release() + + +class Entity(object): + """ + Defines the interface that must be implemented by objects that wish to be + guarded by operation_context. + """ + + def get_rm_locks(self): + return [] + + +class ResourceManagerLock(object): + def __init__(self, ns, name, mode): + self.ns = ns + self.name = name + self.mode = mode + + def acquire(self): + rmanager.acquireResource(self.ns, self.name, self.mode) + + def release(self): + rmanager.releaseResource(self.ns, self.name) + + def _key(self): + return self.ns, self.name + + def __cmp__(self, other): + return cmp(self._key(), other._key()) + + def __hash__(self): + return hash(self._key()) diff --git a/tests/Makefile.am b/tests/Makefile.am index 467f2d4..c6e527f 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -122,6 +122,7 @@ storage_asyncevent_test.py \ storage_check_test.py \ storage_directio_test.py \ + storage_guarded_test.py \ storage_hsm_test.py \ storage_monitor_test.py \ storage_rwlock_test.py \ @@ -221,6 +222,7 @@ stompAsyncDispatcherTests.py \ stompTests.py \ storageMailboxTests.py \ + storage_guarded_test.py \ storage_hsm_test.py \ storage_monitor_test.py \ storageServerTests.py \ diff --git a/tests/storage_guarded_test.py b/tests/storage_guarded_test.py new file mode 100644 index 0000000..29db745 --- /dev/null +++ b/tests/storage_guarded_test.py @@ -0,0 +1,139 @@ +# +# Copyright 2016 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +# +# Refer to the README and COPYING files for full details of the license +# + +from __future__ import absolute_import +from contextlib import contextmanager + +from monkeypatch import MonkeyPatchScope +from storagefakelib import FakeResourceManager +from testlib import VdsmTestCase, expandPermutations, permutations + +from vdsm.storage import guarded + +from storage import resourceManager as rm + + +@expandPermutations +class ContextTest(VdsmTestCase): + + @contextmanager + def env(self): + rmanager = FakeResourceManager() + with MonkeyPatchScope([(guarded, 'rmanager', rmanager)]): + yield rmanager + + def test_none(self): + with self.env(): + with guarded.operation_context([]): + pass + + def test_remove_duplicates(self): + resource_locks = [ + guarded.ResourceManagerLock('A', 'a', rm.LockType.shared), + ] + src = GuardedEntity(resource_locks) + dst = GuardedEntity(resource_locks) + with self.env() as rmanager: + calls = [('acquireResource', ('A', 'a', rm.LockType.shared), {})] + with guarded.operation_context((src, dst)): + self.assertEqual(calls, rmanager.__calls__) + calls.append(('releaseResource', ('A', 'a'), {})) + + def test_sorting(self): + src_locks = [ + guarded.ResourceManagerLock('C', 'a', rm.LockType.shared), + guarded.ResourceManagerLock('A', 'b', rm.LockType.shared), + ] + src = GuardedEntity(src_locks) + dst_locks = [ + guarded.ResourceManagerLock('A', 'a', rm.LockType.shared), + guarded.ResourceManagerLock('B', 'a', rm.LockType.shared), + ] + dst = GuardedEntity(dst_locks) + with self.env() as rmanager: + calls = [ + ('acquireResource', ('A', 'a', rm.LockType.shared), {}), + ('acquireResource', ('A', 'b', rm.LockType.shared), {}), + ('acquireResource', ('B', 'a', rm.LockType.shared), {}), + ('acquireResource', ('C', 'a', rm.LockType.shared), {}), + ] + with guarded.operation_context((src, dst)): + self.assertEqual(calls, rmanager.__calls__) + calls.extend([ + ('releaseResource', ('C', 'a'), {}), + ('releaseResource', ('B', 'a'), {}), + ('releaseResource', ('A', 'b'), {}), + ('releaseResource', ('A', 'a'), {}), + ]) + self.assertEqual(calls, rmanager.__calls__) + + def test_resource_exclusive_and_shared(self): + locks = [guarded.ResourceManagerLock('A', 'a', rm.LockType.shared)] + src = GuardedEntity(locks) + locks = [guarded.ResourceManagerLock('A', 'a', rm.LockType.exclusive)] + dst = GuardedEntity(locks) + with self.env() as rmanager: + calls = [('acquireResource', + ('A', 'a', rm.LockType.exclusive), {}), + ('releaseResource', ('A', 'a'), {})] + with guarded.operation_context((src, dst)): + pass + self.assertEqual(calls, rmanager.__calls__) + + +class GuardedEntity(guarded.Entity): + + def __init__(self, rm_list): + self.rm_list = rm_list + + def get_rm_locks(self): + return self.rm_list + + +@expandPermutations +class ResourceManagerLockTest(VdsmTestCase): + + def test_acquire_release(self): + res_mgr = FakeResourceManager() + + res = guarded.ResourceManagerLock('ns_A', 'name_A', rm.LockType.shared) + with MonkeyPatchScope([(guarded, 'rmanager', res_mgr)]): + expected = [] + res.acquire() + expected.append(('acquireResource', + (res.ns, res.name, res.mode), {})) + self.assertEqual(expected, res_mgr.__calls__) + res.release() + expected.append(('releaseResource', (res.ns, res.name), {})) + self.assertEqual(expected, res_mgr.__calls__) + + @permutations(( + (('A', 'a', rm.LockType.shared), ('B', 'a', rm.LockType.shared)), + (('A', 'a', rm.LockType.shared), ('A', 'b', rm.LockType.shared)), + )) + def test_less_than(self, a, b): + b = guarded.ResourceManagerLock(*b) + a = guarded.ResourceManagerLock(*a) + self.assertLess(a, b) + + def test_equal_with_different_mode(self): + a = guarded.ResourceManagerLock('A', 'a', rm.LockType.shared) + b = guarded.ResourceManagerLock('A', 'a', rm.LockType.exclusive) + self.assertEqual(a, b) diff --git a/vdsm.spec.in b/vdsm.spec.in index b5d4f67..9b95e98 100644 --- a/vdsm.spec.in +++ b/vdsm.spec.in @@ -1240,6 +1240,7 @@ %{python_sitelib}/%{vdsm_name}/storage/exception.py* %{python_sitelib}/%{vdsm_name}/storage/fileUtils.py* %{python_sitelib}/%{vdsm_name}/storage/fuser.py* +%{python_sitelib}/%{vdsm_name}/storage/guarded.py* %{python_sitelib}/%{vdsm_name}/storage/hba.py* %{python_sitelib}/%{vdsm_name}/storage/misc.py* %{python_sitelib}/%{vdsm_name}/storage/mount.py* -- To view, visit https://gerrit.ovirt.org/61435 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I2b0a204818d44b6205515277f4c2834cb2b7a057 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
