From dbe781330729ced62a0b9f270231b84ac39478b5 Mon Sep 17 00:00:00 2001 From: David Lehman Date: Wed, 13 Aug 2014 13:31:30 -0500 Subject: [PATCH 17/26] Add some locking to blivet.devicetree. Methods processActions and _processAction do not automatically acquire the devicetree lock, so they must take care to explicitly protect the device and action lists as necessary. The reason for this is so that uevents generated by executing actions can be handled. --- blivet/devicetree.py | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 1 deletion(-) diff --git a/blivet/devicetree.py b/blivet/devicetree.py index 18695a3..4b59679 100644 --- a/blivet/devicetree.py +++ b/blivet/devicetree.py @@ -27,6 +27,8 @@ import shutil import pprint import copy +from six import add_metaclass + from .errors import CryptoError, DeviceError, DeviceTreeError, DiskLabelCommitError, DMError, FSError, InvalidDiskLabelError, LUKSError, MDRaidError, StorageError from .devices import BTRFSDevice, BTRFSSubVolumeDevice, BTRFSVolumeDevice, BTRFSSnapShotDevice from .devices import DASDDevice, DMDevice, DMLinearDevice, DMRaidArrayDevice, DiskDevice @@ -55,11 +57,13 @@ from .flags import flags from .storage_log import log_exception_info, log_method_call, log_method_return from .i18n import _ from .size import Size -from .threads import LockList +from .threads import SynchronizedMeta, LockList, lock_args +from threading import RLock import logging log = logging.getLogger("blivet") +@add_metaclass(SynchronizedMeta) class DeviceTree(object): """ A quasi-tree that represents the devices in the system. @@ -79,6 +83,13 @@ class DeviceTree(object): :class:`~.deviceaction.DeviceAction` instances can only be registered for leaf devices, except for resize actions. """ + _unsynchronized_methods = ['processActions', '_processAction'] + + def __new__(cls, *args, **kwargs): + self = super(DeviceTree, cls).__new__(cls, *args, **kwargs) + self.lock = RLock() # pylint: disable=attribute-defined-outside-init + + return self def __init__(self, conf=None, passphrase=None, luksDict=None, iscsi=None, dasd=None): @@ -343,6 +354,56 @@ class DeviceTree(object): devices = [a.name for a in active if any(d in disks for d in a.disks)] return devices + """ + We won't be able to process uevents during processActions if we're + going to hold the devicetree lock for the duration. What do we actually + need to lock down while this method runs? + + - action list + - at least while we make a copy of it + - device list + - at least devices whose actions are still queued + + What needs to be locked? + create device: parent device, parent format, device + create format: device, format + destroy device: parent device, parent format, device + destroy format: device, format + resize device: parent device, parent format, device, format + resize format: device, format + container add: container device, container format, device, format + container remove: container device, container format, device, format + + Q: Should we also lock all descendant devices? + + Q: Is there any actual benefit to locking only what must be locked + (as opposed to locking the same set of things regardless of + action type)? + + What would users expect to be able to do while actions run? + + - schedule actions on devices that do not have queued actions + - device list, action list + - NO. Not at this time. + + XXX ASYNC ACTIONS CAN COME LATER + + The main thing we need to account for initially is events generated in + response to the actions we are executing. + + Q: Do we need to allow users read-only access to objects while they are + locked? + A: Yes, probably -- especially if we're locking down the entire stack + while we process an action. + XXX Not initially. We can just process the events related to each + action once the action has finished executing. + + Q: What changes do we need to prevent in ancestors while we process an + action? + A: Nothing external to blivet. Probably also a limited (?) set of + changes to the ancestors StorageDevice instances. + + """ def _processAction(self, action, callbacks=None, dryRun=False): """ Execute a single action. @@ -388,6 +449,7 @@ class DeviceTree(object): self._postProcessActions() + @lock_args('newdev') def _addDevice(self, newdev, new=True): """ Add a device to the tree. @@ -419,6 +481,7 @@ class DeviceTree(object): newdev.name, newdev.id) + @lock_args('dev') def _removeDevice(self, dev, force=None, modparent=True): """ Remove a device from the tree. @@ -459,6 +522,7 @@ class DeviceTree(object): dev.name, dev.id) + @lock_args('device') def _removeChildrenFromTree(self, device): devs_to_remove = self.getDependentDevices(device) while devs_to_remove: @@ -470,6 +534,7 @@ class DeviceTree(object): self._removeDevice(devs_to_remove[0], force=True, modparent=False) break + @lock_args('device') def recursiveRemove(self, device): """ Remove a device after removing its dependent devices. @@ -604,6 +669,7 @@ class DeviceTree(object): return actions + @lock_args('dep') def getDependentDevices(self, dep): """ Return a list of devices that depend on dep. @@ -1269,6 +1335,7 @@ class DeviceTree(object): self.handleUdevDeviceFormat(info, device) device.deviceLinks = udev.device_get_symlinks(info) + @lock_args('device') def handleUdevDiskLabelFormat(self, info, device): disklabel_type = udev.device_get_disklabel_type(info) log_method_call(self, device=device.name, label_type=disklabel_type) @@ -1315,6 +1382,7 @@ class DeviceTree(object): else: device.format = fmt + @lock_args('device') def handleUdevLUKSFormat(self, info, device): # pylint: disable=unused-argument log_method_call(self, name=device.name, type=device.format.type) @@ -1367,6 +1435,7 @@ class DeviceTree(object): log.warning("luks device %s already in the tree", device.format.mapName) + @lock_args('vg_device') def handleVgLvs(self, vg_device): """ Handle setup of the LV's in the vg_device. """ vg_name = vg_device.name @@ -1544,6 +1613,7 @@ class DeviceTree(object): lv_dev.name, lv_dev.copies, lv_dev.metaDataSize, lv_dev.logSize, lv_dev.vgSpaceUsed) + @lock_args('device') def handleUdevLVMPVFormat(self, info, device): # pylint: disable=unused-argument log_method_call(self, name=device.name, type=device.format.type) @@ -1589,6 +1659,7 @@ class DeviceTree(object): self.handleVgLvs(vg_device) + @lock_args('device') def handleUdevMDMemberFormat(self, info, device): # pylint: disable=unused-argument log_method_call(self, name=device.name, type=device.format.type) @@ -1645,6 +1716,7 @@ class DeviceTree(object): md_array.parents.append(device) self._addDevice(md_array) + @lock_args('device') def handleUdevDMRaidMemberFormat(self, info, device): # if dmraid usage is disabled skip any dmraid set activation if not flags.dmraid: @@ -1696,6 +1768,7 @@ class DeviceTree(object): #device.format.raidmem = block.getMemFromRaidSet(dm_array, # major=major, minor=minor, uuid=uuid, name=name) + @lock_args('device') def handleBTRFSFormat(self, info, device): log_method_call(self, name=device.name) uuid = udev.device_get_uuid(info) @@ -1754,6 +1827,7 @@ class DeviceTree(object): exists=True) self._addDevice(subvol) + @lock_args('device') def handleUdevDeviceFormat(self, info, device): log_method_call(self, name=getattr(device, "name", None)) @@ -1888,6 +1962,7 @@ class DeviceTree(object): device.originalFormat = copy.copy(device.format) + @lock_args('device') def updateDeviceFormat(self, device): log.info("updating format of device: %s", device) try: @@ -1911,6 +1986,7 @@ class DeviceTree(object): for pv in vg.pvs: lvm.lvm_cc_addFilterRejectRegexp(pv.name) + @lock_args('device') def hide(self, device): """ Hide the specified device. @@ -1985,6 +2061,7 @@ class DeviceTree(object): if device.name not in self.names: self.names.append(device.name) + @lock_args('device') def unhide(self, device): """ Restore a device's visibility. @@ -2485,6 +2562,7 @@ class DeviceTree(object): leaves = [d for d in self._devices if d.isleaf] return leaves + @lock_args('device') def getChildren(self, device): """ Return a list of a device's children. """ return [c for c in self._devices if device in c.parents] -- 1.9.3