Nir Soffer has posted comments on this change. Change subject: Implement SDM.copy_data ......................................................................
Patch Set 3: (1 comment) https://gerrit.ovirt.org/#/c/60420/3/vdsm/storage/sdm/api/copy_data.py File vdsm/storage/sdm/api/copy_data.py: Line 150: yield Line 151: Line 152: @contextmanager Line 153: def prepare(self, writable=False): Line 154: vars.task.getSharedLock(sc.STORAGE, self.sd_id) > Hiding a lock in prepare is surprizing. Locking should be explicit and clea This also violates the locking pattern that we planned, since we prepare after taking the locks. Maybe we can do: with src.sd_lock(), dst.sd_lock(), src.img_lock(), dst.img_lock(exclusive): Although implementing empty sd_lock, img_lock and vol_lock on all endpoints seems stupid. If we are ok with taking all the locks on the source and then all the locks on the destination, we can take both the domain and image locks in acquire. If we always take the locks in same order, I don't think the order matters. The old code is sometimes ordering locks by the uuid - this will clash with these locks. Line 155: dom = sdCache.produce_manifest(self.sd_id) Line 156: self._vol = dom.produceVolume(self.img_id, self.vol_id) Line 157: try: Line 158: self._vol.prepare(rw=writable, justme=True) -- To view, visit https://gerrit.ovirt.org/60420 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I30ff635c0c73f67b296033b4a506fc3b9ededfbe Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
