Utkarsh Singh has posted comments on this change. Change subject: sparsify: integrating virt-sparsify into vdsm ......................................................................
Patch Set 13: (9 comments) http://gerrit.ovirt.org/#/c/28328/13/vdsm/storage/image.py File vdsm/storage/image.py: Line 535: Line 536: # FIXME: Line 537: # sdCache.produce.produceVolume gives volumes that return getVolumePath Line 538: # with a colon (:) for NFS servers. So, we're using volClass(). Line 539: # RHBZ#1128942 > The RHBZ is not needed now when we have a url - right? Removed. Line 540: # https://bugzilla.redhat.com/show_bug.cgi?id=1128942 Line 541: # If, and when the bug gets solved, remove the tmpVolClass and Line 542: # dstVolClass variables, and get tmpVolume and dstVolume using Line 543: # sdCache.produce(xyzSdUUID).produceVolume(xyzImgUUID, xyzVolUUID) Line 536: # FIXME: Line 537: # sdCache.produce.produceVolume gives volumes that return getVolumePath Line 538: # with a colon (:) for NFS servers. So, we're using volClass(). Line 539: # RHBZ#1128942 Line 540: # https://bugzilla.redhat.com/show_bug.cgi?id=1128942 > The url can nicer using: Done Line 541: # If, and when the bug gets solved, remove the tmpVolClass and Line 542: # dstVolClass variables, and get tmpVolume and dstVolume using Line 543: # sdCache.produce(xyzSdUUID).produceVolume(xyzImgUUID, xyzVolUUID) Line 544: # Also, for baseVolume use Line 542: # dstVolClass variables, and get tmpVolume and dstVolume using Line 543: # sdCache.produce(xyzSdUUID).produceVolume(xyzImgUUID, xyzVolUUID) Line 544: # Also, for baseVolume use Line 545: # sdCache.produce(tmpSdUUID).produceVolume(tmpImgUUID, Line 546: # tmpVolume.getParent()) > I think this can be shrinked to: Done Line 547: tmpVolClass = sdCache.produce(tmpSdUUID).getVolumeClass() Line 548: dstVolClass = sdCache.produce(dstSdUUID).getVolumeClass() Line 549: Line 550: # Get source, destination and temporary volumes Line 550: # Get source, destination and temporary volumes Line 551: tmpVolume = tmpVolClass(self.repoPath, tmpSdUUID, tmpImgUUID, Line 552: tmpVolUUID) Line 553: dstVolume = dstVolClass(self.repoPath, dstSdUUID, dstImgUUID, Line 554: dstVolUUID) > It would be nice to remove this code and add a helper to return a volume fr Done Line 555: Line 556: if not tmpVolume.isSparse() or not dstVolume.isSparse(): Line 557: raise se.VolumeNotSparse() Line 558: Line 556: if not tmpVolume.isSparse() or not dstVolume.isSparse(): Line 557: raise se.VolumeNotSparse() Line 558: Line 559: srcVolume = tmpVolClass(self.repoPath, tmpSdUUID, tmpImgUUID, Line 560: tmpVolume.getParent()) > Should we check tmpVolume or srcVolume sparseness? or both? If by empty snapshot you mean untouched snapshot, then yes. I don't think we need to test sparseness on any of src and tmp, just destination. Updating to remove tmpVolume.isSparse() Line 561: Line 562: tmpVolume.prepare() Line 563: Line 564: try: Line 562: tmpVolume.prepare() Line 563: Line 564: try: Line 565: dstVolume.prepare() Line 566: > The empty line bellow the prepare() and teardown() is not needed. Done Line 567: try: Line 568: # TODO: Some extra space may be needed for QCOW2 headers Line 569: tmpVolume.extend(tmpVolume.getSize()) Line 570: dstVolume.extend(tmpVolume.getSize()) Line 565: dstVolume.prepare() Line 566: Line 567: try: Line 568: # TODO: Some extra space may be needed for QCOW2 headers Line 569: tmpVolume.extend(tmpVolume.getSize()) > Why do we need to extend tmpVolume? virt-sparsify will write into it? Yes it will be written to. Fixed sp.py to get exclusive lock. Line 570: dstVolume.extend(tmpVolume.getSize()) Line 571: Line 572: srcFormat = volume.fmt2str(srcVolume.getFormat()) Line 573: dstFormat = volume.fmt2str(dstVolume.getFormat()) Line 586: Line 587: finally: Line 588: tmpVolume.teardown(sdUUID=tmpSdUUID, volUUID=tmpVolUUID) Line 589: Line 590: # Shrink to optimal size > This comment is not adding any value. Done Line 591: tmpVolume.shrinkToOptimalSize() Line 592: dstVolume.shrinkToOptimalSize() Line 593: Line 594: def cloneStructure(self, sdUUID, imgUUID, dstSdUUID): http://gerrit.ovirt.org/#/c/28328/13/vdsm/storage/sp.py File vdsm/storage/sp.py: Line 1572: """ Line 1573: srcNamespace = sd.getNamespace(tmpSdUUID, IMAGE_NAMESPACE) Line 1574: dstNamespace = sd.getNamespace(dstSdUUID, IMAGE_NAMESPACE) Line 1575: Line 1576: with nested( > Explain why you take shared lock for the tmp image and exclusive lock on th I am writing to both, so updating to take exclusive locks on both. Line 1577: rmanager.acquireResource(srcNamespace, tmpImgUUID, Line 1578: rm.LockType.shared), Line 1579: rmanager.acquireResource(dstNamespace, dstImgUUID, Line 1580: rm.LockType.exclusive)): -- To view, visit http://gerrit.ovirt.org/28328 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id7bd2b4b6d45781fa27a128dd68d14b7561d0901 Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Utkarsh Singh <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Utkarsh Singh <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
