See way below...
On Thu, 2010-01-21 at 14:04 -0500, Peter Jones wrote:
> On 01/20/2010 04:35 PM, Hans de Goede wrote:
> > Hi,
> >
> > On 01/21/2010 01:03 AM, Peter Jones wrote:
> >> This makes PartitionDevice.teardown() remove device-mapper tables when
> >> appropriate, so devices using them don't have to clean up the partition
> >> and all the conditional testing that comes with that.
> >> ---
> >> storage/devices.py | 19 +++++++++++++++++++
> >> 1 files changed, 19 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/storage/devices.py b/storage/devices.py
> >> index 3bb0991..237fc75 100644
> >> --- a/storage/devices.py
> >> +++ b/storage/devices.py
> >> @@ -1314,6 +1314,25 @@ class PartitionDevice(StorageDevice):
> >>
> >> self.exists = False
> >>
> >> + def teardown(self, recursive=None):
> >> + """ Close, or tear down, a device. """
> >> + log_method_call(self, self.name, status=self.status)
> >> + if not self.exists and not recursive:
> >> + raise DeviceError("device has not been created", self.name)
> >> +
> >> + if self.status:
> >> + if self.format.exists:
> >> + self.format.teardown()
> >> + devmap = block.getMap(major=self.major, minor=self.minor)
> >> + if devmap:
> >> + try:
> >> + block.removeDeviceMap(devmap)
> >> + except Exception as e:
> >> + raise PartitioningError("failed to tear down
> >> device-mapper partition %s: %s" % (self.name, e))
> >> +
> >> + udev_settle()
> >> + StorageDevice.teardown(self, recursive=recursive)
> >> +
> >
> > Erm this is going to be a problem for dmraid, if you do this you
> > should also add a modified setup() which restores the partitions
> > again, because nothing else is going to do that for dmraid sets.
> >
> > But I wonder, are there reasons to take the mpath set down once up ?
>
> Well, there certainly are if the only place we've got partition setup
> is in the MultipathDevice, not the PartitionDevice, which is currently
> the case (and I'm not aiming to change that; realization of partitions
> is easy there.)
>
> > With BIOS RAID (both the mdraid and dmraid flavors) I started out
> > implementing setup() and teardown() methods too. But in the end it
> > turned out it was better to just set them up once, and be done with
> > it (partition handling problems was one of the reasons to do this,
> > pyblock and parted both modifying the partition mappings in turn got
> > ugly pretty quickly).>
> > So currently for dmraid the model is:
> >
> > 1) Assemble it using pyblock, which creates the initial partition mappings
> > 2) Don't let pyblock do anything else with it
> > 3) Let parted modify the mappings as it adds / removes partitions
> > (when executing actions).
>
> I tried this (among about 6 other approaches now...) for mpath and had trouble
> getting it to work, mostly because we currently get teardowns for devices
> before they have 0 holders. On normal disk devices, that fails /unless/
> the only holder are the partitions, and they themselves have no holders.
> This behavior is amazingly hard to emulate without having tracking of the
> children.
>
> The version I've got here works for mpath. So, given that, How about if I do
> it as the patch below, instead.
>
> > You're above patch breaks this as, once the teardown removes the
> > partition nothing is going to restore it. Also keep in mind that
> > certain actions, only setup and teardown a single device. So you
> > cannot count on the disk setup() restoring the partition mappings, if
> > you are going to do this you need to add a setup() which restores the
> > mapping. But I think using the dmraid model instead is better.
>
> I realize it's kindof weirdly asymetrical, but the MultipathDevice.setup()
> does create the mapping.
>
>
>
> commit a1f6eb3d1fd515f47ccb2aa1ae6ba67e7a0342d2
> Author: Peter Jones <pjones@xxxxxxxxxx>
> Date: Wed Jan 20 18:56:59 2010 -0500
>
> Make PartitionDevice have its own teardown() when used with mpath.
>
> This makes PartitionDevice.teardown() remove device-mapper tables when
> appropriate, so devices using them don't have to clean up the partition
> and all the conditional testing that comes with that.
>
> diff --git a/storage/devices.py b/storage/devices.py
> index 9020463..8bc68c3 100644
> --- a/storage/devices.py
> +++ b/storage/devices.py
> @@ -1314,6 +1314,26 @@ class PartitionDevice(StorageDevice):
>
> self.exists = False
>
> + def teardown(self, recursive=None):
> + """ Close, or tear down, a device. """
> + log_method_call(self, self.name, status=self.status)
> + if not self.exists and not recursive:
> + raise DeviceError("device has not been created", self.name)
> +
> + if self.status:
> + if self.format.exists:
> + self.format.teardown()
> + if self.parents[0].type == 'dm-multipath':
> + devmap = block.getMap(major=self.major, minor=self.minor)
> + if devmap:
> + try:
> + block.removeDeviceMap(devmap)
> + except Exception as e:
> + raise PartitioningError("failed to tear down device-mapper partition %s: %s" % (self.name, e))
PartitioningError is for partition allocation failures.
DeviceTeardownError would be most appropriate here.
Otherwise it looks fine.
Dave
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
