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(a)gmail.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Utkarsh Singh <utkarshsins(a)gmail.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes