Change in vdsm[master]: block: simplify zeroImgVolumes using threads

nsoffer at redhat.com nsoffer at redhat.com
Tue May 12 21:40:46 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: block: simplify zeroImgVolumes using threads
......................................................................


Patch Set 3:

(4 comments)

https://gerrit.ovirt.org/#/c/35630/3/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 207:     try:
Line 208:         lvm.changelv(sdUUID, volUUIDs, ("--permission", "rw"))
Line 209:     except se.StorageException as e:
Line 210:         # We ignore the failure hoping that the volumes were
Line 211:         # already writable.
The original comment was better.
Line 212:         log.debug('ignoring failed permission change: %s', e)
Line 213: 
Line 214:     def zeroVolume(volUUID):
Line 215:         log.debug('starting to zero volume %s on domain %s for '


Line 218:         path = lvm.lvPath(sdUUID, volUUID)
Line 219:         size = multipath.getDeviceSize(lvm.lvDmDev(sdUUID, volUUID))
Line 220: 
Line 221:         try:
Line 222:             misc.ddWatchCopy("/dev/zero", path, aborting, size)
Previously we use specific dd command for this operation, that seems to do the write thing. Now you call ddWatchCopy, which is 10 times more complex, running dd twice and trying to support any odd image size, while our images are always in 128MB chunks.

I don't feel confident with using ddWatchCopy, and prefer the current direct and simple code.
Line 223:         except Exception:
Line 224:             log.exception('zeroing operation failed')
Line 225:             raise se.VolumesZeroingError(path)
Line 226: 


Line 223:         except Exception:
Line 224:             log.exception('zeroing operation failed')
Line 225:             raise se.VolumesZeroingError(path)
Line 226: 
Line 227:         deleteVolumes(sdUUID, volUUID)
We can have 100-150 threads running this in the same time - does it make sense to run concurrent deleteVolumes like this?
Line 228:         log.debug('zeroing of volume %s on domain %s for task %s '
Line 229:                   'completed successfully', volUUID, sdUUID, taskid)
Line 230: 
Line 231:     misc.tmap(zeroVolume, volUUIDs)


Line 227:         deleteVolumes(sdUUID, volUUID)
Line 228:         log.debug('zeroing of volume %s on domain %s for task %s '
Line 229:                   'completed successfully', volUUID, sdUUID, taskid)
Line 230: 
Line 231:     misc.tmap(zeroVolume, volUUIDs)
We can use itmap to get one uuid of zeroed and discarded volume and delete it. This is more similar to the old code, running one deleteVolumes operation per vdsm task at the same time.
Line 232: 
Line 233: 
Line 234: class VGTagMetadataRW(object):
Line 235:     log = logging.getLogger("Storage.Metadata.VGTagMetadataRW")


-- 
To view, visit https://gerrit.ovirt.org/35630
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I665d863085842de8ff93c9aacf7c26277dfb031d
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list