Nir Soffer has posted comments on this change. Change subject: Implement SDM.copy_data ......................................................................
Patch Set 4: Code-Review-1 (2 comments) https://gerrit.ovirt.org/#/c/60420/4/vdsm/storage/sdm/api/copy_data.py File vdsm/storage/sdm/api/copy_data.py: Line 59: if self._operation: Line 60: self._operation.abort() Line 61: Line 62: def _run(self): Line 63: with self.source.acquire(), self.destination.acquire(exclusive=True): This locking pattern, acquire first source locks and the destination locks, is incorrect and does not match the locking done in other code. For example: Task 1: - src lock: "A" - dst lock: "B" Task 2: - src lock: "B" - dst lock: "A" Flow: - Task 1 takes lock "A" - Task 2 takes lock "B" - Task 2 blocks trying to lock "A" - Task 1 blocks trying to lock "B" In the rest of the code, we take: - sp lock - domain locks ordered by name of lock - image locks ordered by name of lock - image lock take volume locks in order of volumes The only safe way would be to sort all locks needed for this operations, and take all the locks in the same time. This cannot be done in the context of a single endpoint. Line 64: with (self.source.prepare(), Line 65: self.destination.prepare(writable=True)): Line 66: # Do not start copying if we have already been aborted Line 67: if self._status == jobs.STATUS.ABORTED: Line 135: # concurrent copy_data operations from taking place on the same host. Line 136: # To allow this, we'll need to lock the destination image in shared Line 137: # mode and the destination volume in exclusive mode. This requires Line 138: # changes to the resourceManager. Line 139: vars.task.getSharedLock(sc.STORAGE, self.sd_id) This lock will be released only when the task ends, not when acquire ends. You should use acquireResource like the image resource. Line 140: res_ns = sd.getNamespace(self.sd_id, IMAGE_NAMESPACE) Line 141: lock_type = rm.LockType.exclusive if exclusive else rm.LockType.shared Line 142: with rmanager.acquireResource(res_ns, self.img_id, lock_type): Line 143: yield -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
