Maor Lipchuk has posted comments on this change. Change subject: image: Calculate qcow volume size on import. ......................................................................
Patch Set 2: (15 comments) https://gerrit.ovirt.org/#/c/65039/2/vdsm/storage/image.py File vdsm/storage/image.py: Line 842 Line 843 Line 844 Line 845 Line 846 > We are adding too much code to existing function which was to long before w Done Line 35: from vdsm.storage import misc Line 36: from vdsm.storage import workarounds Line 37: from vdsm.storage.threadlocal import vars Line 38: Line 39: import qcow2 > This should be in lib/vdsm/storage, so: Done Line 40: from sdc import sdCache Line 41: import sd Line 42: import imageSharing Line 43: from vdsm.common.exception import ActionStopped Line 842: # For convert to 'raw' we need use the virtual disk size Line 843: # instead of apparent size Line 844: if dstVolFormat == sc.RAW_FORMAT: Line 845: newsize = volParams['size'] Line 846: elif volParams['volFormat'] == sc.RAW_FORMAT: > I think this checks also that the destination volume is cow. For raw, using I'm not sure I understood this comment, can u please try to rephrase Line 847: # TODO: Some extra space may be needed for QCOW2 headers Line 848: newsize = qcow2.estimate_required_size( Line 849: volParams['size'], Line 850: volParams['truesize']) Line 843: # instead of apparent size Line 844: if dstVolFormat == sc.RAW_FORMAT: Line 845: newsize = volParams['size'] Line 846: elif volParams['volFormat'] == sc.RAW_FORMAT: Line 847: # TODO: Some extra space may be needed for QCOW2 headers > Put this todo in qcow2.estimate_required_size Done Line 848: newsize = qcow2.estimate_required_size( Line 849: volParams['size'], Line 850: volParams['truesize']) Line 851: extend_size = config.getint("irs", Line 844: if dstVolFormat == sc.RAW_FORMAT: Line 845: newsize = volParams['size'] Line 846: elif volParams['volFormat'] == sc.RAW_FORMAT: Line 847: # TODO: Some extra space may be needed for QCOW2 headers Line 848: newsize = qcow2.estimate_required_size( > Using newsize for this is not a good idea, better keep two separate variabl Done Line 849: volParams['size'], Line 850: volParams['truesize']) Line 851: extend_size = config.getint("irs", Line 852: "volume_utilization_chunk_mb") Line 846: elif volParams['volFormat'] == sc.RAW_FORMAT: Line 847: # TODO: Some extra space may be needed for QCOW2 headers Line 848: newsize = qcow2.estimate_required_size( Line 849: volParams['size'], Line 850: volParams['truesize']) > truesize is not correct for block volume, this is always the same as the vi If we can't handle this now what do you suggest? Line 851: extend_size = config.getint("irs", Line 852: "volume_utilization_chunk_mb") Line 853: extend_size *= (10 ** 6) Line 854: newsize += extend_size Line 848: newsize = qcow2.estimate_required_size( Line 849: volParams['size'], Line 850: volParams['truesize']) Line 851: extend_size = config.getint("irs", Line 852: "volume_utilization_chunk_mb") > I would call it chunk_size, and initialize it in bytes: Done Line 853: extend_size *= (10 ** 6) Line 854: newsize += extend_size Line 855: self.log.debug("Estimated allocation for qcow2 volume:" Line 856: "%i bytes", newsize) Line 849: volParams['size'], Line 850: volParams['truesize']) Line 851: extend_size = config.getint("irs", Line 852: "volume_utilization_chunk_mb") Line 853: extend_size *= (10 ** 6) > We have a constant for MB. Done Line 854: newsize += extend_size Line 855: self.log.debug("Estimated allocation for qcow2 volume:" Line 856: "%i bytes", newsize) Line 857: else: Line 850: volParams['truesize']) Line 851: extend_size = config.getint("irs", Line 852: "volume_utilization_chunk_mb") Line 853: extend_size *= (10 ** 6) Line 854: newsize += extend_size > I would do: Done Line 855: self.log.debug("Estimated allocation for qcow2 volume:" Line 856: "%i bytes", newsize) Line 857: else: Line 858: newsize = volParams['apparentsize'] Line 852: "volume_utilization_chunk_mb") Line 853: extend_size *= (10 ** 6) Line 854: newsize += extend_size Line 855: self.log.debug("Estimated allocation for qcow2 volume:" Line 856: "%i bytes", newsize) > %i may work but we always use %d, lets keep the tradition. Done Line 857: else: Line 858: newsize = volParams['apparentsize'] Line 859: Line 860: dstVol.extend(newsize) https://gerrit.ovirt.org/#/c/65039/2/vdsm/storage/qcow2.py File vdsm/storage/qcow2.py: Line 16: # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA Line 17: # Line 18: # Refer to the README and COPYING files for full details of the license Line 19: # Line 20: > Please add: Done Line 21: from vdsm.storage import constants as sc Line 22: Line 23: # Pre-configured QCOW volume size. Line 24: CLUSTER_SIZE = 65536 Line 17: # Line 18: # Refer to the README and COPYING files for full details of the license Line 19: # Line 20: Line 21: from vdsm.storage import constants as sc > We don't need this import, since we can require used_size in bytes. Done Line 22: Line 23: # Pre-configured QCOW volume size. Line 24: CLUSTER_SIZE = 65536 Line 25: L2TABLE_ENTRY_SIZE = 8 Line 24: CLUSTER_SIZE = 65536 Line 25: L2TABLE_ENTRY_SIZE = 8 Line 26: Line 27: Line 28: def estimate_required_size(virtual_size, used_size): > Both virtual_size and used_size must be in bytes. Please document the argum What do you suggest to do with qemiimg.map()? Line 29: """ Line 30: Estimating qcow2 file size in bytes when converting from raw to Line 31: qcow as advised by [email protected]. Line 32: The calculation for minimum allocation is as follow: Line 27: Line 28: def estimate_required_size(virtual_size, used_size): Line 29: """ Line 30: Estimating qcow2 file size in bytes when converting from raw to Line 31: qcow as advised by [email protected]. > I think Kevin Wolf is better then his email, in case he don't want his emai I think it is better to use Red Hat email since it is more proffesional that way, but I won't mind changing this. Done Line 32: The calculation for minimum allocation is as follow: Line 33: (used_blocks * block_size) + Line 34: (virtual_size / cluster_size * 64 bit L2 table entry) Line 35: """ Line 33: (used_blocks * block_size) + Line 34: (virtual_size / cluster_size * 64 bit L2 table entry) Line 35: """ Line 36: worst_case_overhead = virtual_size / CLUSTER_SIZE * L2TABLE_ENTRY_SIZE Line 37: return used_size * sc.BLOCK_SIZE + worst_case_overhead > used_size must be in bytes, we don't need to multiple with bloc size. Done, working on the tests -- To view, visit https://gerrit.ovirt.org/65039 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia87df2b0d5378f93c5cb2cc68a37458fe3b4467b Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list -- [email protected] To unsubscribe send an email to [email protected]
