Nir Soffer has posted comments on this change.
Change subject: block: convert getVolumeMetadataSlot into a contexmanager ......................................................................
Patch Set 14:
(2 comments)
We have two changes here: - Moving the metadata lock from blockVolume to blockSD - this is better since blockSD is managing the metadata slots. - Hiding the lock in a context manager - safer but strange api - we should review this later to make it more clear.
Do yo like to separate the changes?
https://gerrit.ovirt.org/#/c/25988/14/tests/volumeTests.py File tests/volumeTests.py:
Line 97: Line 98: def testMetaSlotLock(self): Line 99: with self.blksd.getVolumeMetadataSlot(None, 1): Line 100: lktaken = self.blksd._lvTagMetaSlotLock.acquire(False) Line 101: self.assertEqual(lktaken, False) lktaken -> acquired
https://gerrit.ovirt.org/#/c/25988/14/vdsm/storage/blockSD.py File vdsm/storage/blockSD.py:
Line 714: Line 715: _lvTagMetaSlotLock = threading.Lock() Line 716: Line 717: @contextmanager Line 718: def getVolumeMetadataSlot(self, vol_name, slotSize): Calling it like a getter is confusing:
slot = domain.getVolumeMetadataSlot(vol, size)
The code looks right, but it is wrong.
Maybe this will be little more clear?
with domain.acquireMetadataSlot(vol, size) as slot: ... Line 719: # TODO: _lvTagMetaSlotLock should be relevant only when we use Line 720: # getFreeMetadataSlot to avoid a race between getting the occupied Line 721: # metadata slots and tagging the new LV (commit to the new slot). Line 722: with self._lvTagMetaSlotLock: