Change in vdsm[master]: sparsify: integrating virt-sparsify into vdsm

nsoffer at redhat.com nsoffer at redhat.com
Thu Aug 14 10:24:47 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: sparsify: integrating virt-sparsify into vdsm
......................................................................


Patch Set 14:

(4 comments)

http://gerrit.ovirt.org/#/c/28328/14/vdsm/storage/image.py
File vdsm/storage/image.py:

Line 546: 
Line 547:         tmpVolume = self._getSparsifyVolume(tmpSdUUID, tmpImgUUID, tmpVolUUID)
Line 548:         dstVolume = self._getSparsifyVolume(dstSdUUID, dstImgUUID, dstVolUUID)
Line 549: 
Line 550:         if not tmpVolume.isSparse() or not dstVolume.isSparse():
In your comment (see http://gerrit.ovirt.org/#/c/28328/13..14/vdsm/storage/image.py), you said your are going to remove the sparseness check from tmpVolume, but you kept it. So my question is still unanswered  - why do we check sparseness of tmp volume and not of src volume?
Line 551:             raise se.VolumeNotSparse()
Line 552: 
Line 553:         srcVolume = self._getSparsifyVolume(tmpSdUUID, tmpImgUUID,
Line 554:                                             tmpVolume.getParent())


Line 553:         srcVolume = self._getSparsifyVolume(tmpSdUUID, tmpImgUUID,
Line 554:                                             tmpVolume.getParent())
Line 555: 
Line 556:         tmpVolume.prepare()
Line 557: 
I commented about the uneeded line below the next prepare, but assumed you recognize that this line is the same.
Line 558:         try:
Line 559:             dstVolume.prepare()
Line 560:             try:
Line 561:                 # TODO: Some extra space may be needed for QCOW2 headers


Line 558:         try:
Line 559:             dstVolume.prepare()
Line 560:             try:
Line 561:                 # TODO: Some extra space may be needed for QCOW2 headers
Line 562:                 tmpVolume.extend(tmpVolume.getSize())
So to sparsify a volume of size X, we need a temporary volume of size X, and a destination volume of size X, total 3X the size of the original volume?

This sounds little extreme for an operation that meant to save space on the storage. Are we using this correctly?
Line 563:                 dstVolume.extend(tmpVolume.getSize())
Line 564: 
Line 565:                 srcFormat = volume.fmt2str(srcVolume.getFormat())
Line 566:                 dstFormat = volume.fmt2str(dstVolume.getFormat())


http://gerrit.ovirt.org/#/c/28328/14/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(
When I wrote "explain why...", I meant that you will add a comments explaining the locking policy, so the next developer maintaining this does not have to research everything from scratch.
Line 1577:             rmanager.acquireResource(srcNamespace, tmpImgUUID,
Line 1578:                                      rm.LockType.exclusive),
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: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Utkarsh Singh <utkarshsins at gmail.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: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Utkarsh Singh <utkarshsins at gmail.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list