Change in vdsm[master]: block: use the contextmanager for getVolumeMetadataSlot

nsoffer at redhat.com nsoffer at redhat.com
Wed Mar 11 07:04:12 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: block: use the contextmanager for getVolumeMetadataSlot
......................................................................


Patch Set 6:

(1 comment)

https://gerrit.ovirt.org/#/c/25988/6/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 716: 
Line 717:     @contextmanager
Line 718:     def getVolumeMetadataSlot(self, vol_name, slotSize):
Line 719:         # TODO: _lvTagMetaSlotLock should be relevant only when we use
Line 720:         # getFreeMetadataSlot to avoid a race between getting the occupied
> We'll have to get Federico to weigh in on it.
Seems that _lvTagMetaSlotLock should be used only when using getFreeMetadataSlot(), and not when using getVolumeMetadataOffsetFromPvMapping().

When calling getFreeMetaDataSlot(), we must prevent adding or removing new slots when we search for the occupied ones.

When calling getVolumeMetadataOffsetFromPvMapping() we seems to use the slot according to the pv layout which cannot change, and mysterious "mapoffset" value which I don't fully understand, added in downstream d282ec2e (BZ#675683, BZ#684584, BZ#664432 - Metadata refactoring).

I would replace the TODO with: "Check if the lock is needed when using getVolumeMetadataOffsetFromPvMapping()", but I would not touch this code since it had no tests, no documentation, and too many "tricks".
Line 721:         # metadata slots and tagging the new LV (commit to the new slot).
Line 722:         with self._lvTagMetaSlotLock:
Line 723:             if self.getVersion() in VERS_METADATA_LV:
Line 724:                 yield self.getVolumeMetadataOffsetFromPvMapping(vol_name)


-- 
To view, visit https://gerrit.ovirt.org/25988
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id1496238ea4f9a1902f816b20959ce3379e87871
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer 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