Once I started untangling BZ 659324 using a mpathed PPC64 machine I discovered more than one problem. The last patch has a detailed explanation of why those problems exist. I'll only push the set after it's acked, your questions answered and it's confirmed by the reporter that the fix works.
All of the set is to be merged to master too except "mpath: remove a harmful udev_trigger() in filter_gui" where I am going to remove add udev_trigger calls instead (will post the patch before pushing there).
Ales
And 7ecfc716475b89c43808962c757dfd9d775e7c76 will cause a traceback there.
Related: rhbz#636570 --- anaconda | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/anaconda b/anaconda index 417e114..0f9f88e 100755 --- a/anaconda +++ b/anaconda @@ -679,7 +679,10 @@ if __name__ == "__main__":
setupEnvironment() # make sure we have /var/log soon, some programs fail to start without it - os.mkdir("/var/log", 0755) + try: + os.mkdir("/var/log", 0755) + except OSError as e: + pass
pidfile = open("/var/run/anaconda.pid", "w") pidfile.write("%s\n" % (os.getpid(),))
On 12/22/2010 05:22 AM, Ales Kozumplik wrote:
And 7ecfc716475b89c43808962c757dfd9d775e7c76 will cause a traceback there.
Related: rhbz#636570
anaconda | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/anaconda b/anaconda index 417e114..0f9f88e 100755 --- a/anaconda +++ b/anaconda @@ -679,7 +679,10 @@ if __name__ == "__main__":
setupEnvironment() # make sure we have /var/log soon, some programs fail to start without it
- os.mkdir("/var/log", 0755)
try:
os.mkdir("/var/log", 0755)except OSError as e:
passpidfile = open("/var/run/anaconda.pid", "w") pidfile.write("%s\n" % (os.getpid(),))
Looks fine, ACK.
Sometimes (race condition?) the udev_get_block_devices() call in getScreen() returns incomplete dictionaries for mpath devices. If ID_VENDOR is missing this causes a traceback in MPathCallbacks._populateUI().
Related: rhbz#636570 --- anaconda | 1 + iw/filter_gui.py | 1 - 2 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/anaconda b/anaconda index 0f9f88e..071442b 100755 --- a/anaconda +++ b/anaconda @@ -654,6 +654,7 @@ if __name__ == "__main__": # NetworkManager in the udev database from baseudev import udev_trigger, udev_settle udev_trigger("net") + udev_trigger("block") # trigger the block subsys too while at it udev_settle() # and for added fun, once doesn't seem to be enough? so we # do it twice, it works and we scream at the world "OH WHY?" diff --git a/iw/filter_gui.py b/iw/filter_gui.py index 98ae480..63655a0 100644 --- a/iw/filter_gui.py +++ b/iw/filter_gui.py @@ -586,7 +586,6 @@ class FilterWindow(InstallWindow): self.store.set_sort_column_id(MODEL_COL, gtk.SORT_ASCENDING)
anaconda.id.storage.devicetree.teardownAll() - udev_trigger(subsystem="block") # So that drives onlined by these show up in the filter UI storage.iscsi.iscsi().startup(anaconda.intf) storage.fcoe.fcoe().startup(anaconda.intf)
On 12/22/2010 05:22 AM, Ales Kozumplik wrote:
Sometimes (race condition?) the udev_get_block_devices() call in getScreen() returns incomplete dictionaries for mpath devices. If ID_VENDOR is missing this causes a traceback in MPathCallbacks._populateUI().
Related: rhbz#636570
anaconda | 1 + iw/filter_gui.py | 1 - 2 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/anaconda b/anaconda index 0f9f88e..071442b 100755 --- a/anaconda +++ b/anaconda @@ -654,6 +654,7 @@ if __name__ == "__main__": # NetworkManager in the udev database from baseudev import udev_trigger, udev_settle udev_trigger("net")
- udev_trigger("block") # trigger the block subsys too while at it udev_settle() # and for added fun, once doesn't seem to be enough? so we # do it twice, it works and we scream at the world "OH WHY?"
diff --git a/iw/filter_gui.py b/iw/filter_gui.py index 98ae480..63655a0 100644 --- a/iw/filter_gui.py +++ b/iw/filter_gui.py @@ -586,7 +586,6 @@ class FilterWindow(InstallWindow): self.store.set_sort_column_id(MODEL_COL, gtk.SORT_ASCENDING)
anaconda.id.storage.devicetree.teardownAll()
udev_trigger(subsystem="block") # So that drives onlined by these show up in the filter UI storage.iscsi.iscsi().startup(anaconda.intf) storage.fcoe.fcoe().startup(anaconda.intf)
I wish we knew more of when this happens, but if this seems to solve your problem, there's no real downside to doing it, so ACK.
Related: rhbz#636570 --- storage/devicelibs/mpath.py | 77 ++++++++++++++++++++++++++----------------- 1 files changed, 47 insertions(+), 30 deletions(-)
diff --git a/storage/devicelibs/mpath.py b/storage/devicelibs/mpath.py index 4909314..0d0efde 100644 --- a/storage/devicelibs/mpath.py +++ b/storage/devicelibs/mpath.py @@ -8,32 +8,44 @@ import logging log = logging.getLogger("storage")
def parseMultipathOutput(output): - # this function parses output from "multipath -d", so we can use its - # logic for our topology. - # The input looks like: - # create: mpathc (1ATA ST3120026AS 5M) undef ATA,ST3120026AS - # size=112G features='0' hwhandler='0' wp=undef - # `-+- policy='round-robin 0' prio=1 status=undef - # `- 2:0:0:0 sda 8:0 undef ready running - # create: mpathb (36006016092d21800703762872c60db11) undef DGC,RAID 5 - # size=10G features='1 queue_if_no_path' hwhandler='1 emc' wp=undef - # `-+- policy='round-robin 0' prio=2 status=undef - # |- 6:0:0:0 sdb 8:16 undef ready running - # `- 7:0:0:0 sdc 8:32 undef ready running - # create: mpatha (36001438005deb4710000500000270000) dm-0 HP,HSV400 - # size=20G features='0' hwhandler='0' wp=rw - # |-+- policy='round-robin 0' prio=-1 status=active - # | |- 7:0:0:1 sda 8:0 active undef running - # | `- 7:0:1:1 sdb 8:16 active undef running - # `-+- policy='round-robin 0' prio=-1 status=enabled - # |- 7:0:2:1 sdc 8:32 active undef running - # `- 7:0:3:1 sdd 8:48 active undef running - # - # (In anaconda, the first one there won't be included because we blacklist - # "ATA" as a vendor.) - # - # It returns a structure like: - # [ {'mpatha':['sdb','sdc']}, ... ] + """ + Parse output from "multipath -d" or "multipath -ll" and form a topology. + + Returns a dictionary: + {'mpatha':['sdb','sdc'], 'mpathb': ['sdd', 'sde'], ... } + + The 'multipath -d' output looks like: + create: mpathc (1ATA ST3120026AS 5M) undef ATA,ST3120026AS + size=112G features='0' hwhandler='0' wp=undef + `-+- policy='round-robin 0' prio=1 status=undef + `- 2:0:0:0 sda 8:0 undef ready running + create: mpathb (36006016092d21800703762872c60db11) undef DGC,RAID 5 + size=10G features='1 queue_if_no_path' hwhandler='1 emc' wp=undef + `-+- policy='round-robin 0' prio=2 status=undef + |- 6:0:0:0 sdb 8:16 undef ready running + `- 7:0:0:0 sdc 8:32 undef ready running + create: mpatha (36001438005deb4710000500000270000) dm-0 HP,HSV400 + size=20G features='0' hwhandler='0' wp=rw + |-+- policy='round-robin 0' prio=-1 status=active + | |- 7:0:0:1 sda 8:0 active undef running + | `- 7:0:1:1 sdb 8:16 active undef running + `-+- policy='round-robin 0' prio=-1 status=enabled + |- 7:0:2:1 sdc 8:32 active undef running + `- 7:0:3:1 sdd 8:48 active undef running + + (In anaconda, the first one there won't be included because we blacklist + "ATA" as a vendor.) + + The 'multipath -ll' output looks like (notice the missing 'create' before + 'mpatha'): + + mpatha (3600a0b800067fcc9000001694b557dd1) dm-0 IBM,1726-4xx FAStT + size=360G features='0' hwhandler='1 rdac' wp=rw + `-+- policy='round-robin 0' prio=3 status=active + |- 2:0:0:0 sda 8:0 active ready running + `- 3:0:0:0 sdb 8:16 active ready running + + """ mpaths = {} if output is None: return mpaths @@ -43,20 +55,22 @@ def parseMultipathOutput(output):
policy = re.compile('^[|+` -]+policy') device = re.compile('^[|+` -]+[0-9]+:[0-9]+:[0-9]+:[0-9]+ +([a-zA-Z0-9!/]+)') + create = re.compile('^(create: )?(mpath\w+)')
lines = output.split('\n') for line in lines: pmatch = policy.match(line) dmatch = device.match(line) + cmatch = create.match(line) lexemes = line.split() if not lexemes: break - if lexemes[0] == 'create:': + if cmatch and cmatch.group(2): if name and devices: mpaths[name] = devices name = None devices = [] - name = lexemes[1] + name = cmatch.group(2) elif lexemes[0].startswith('size='): pass elif pmatch: @@ -85,7 +99,10 @@ def identifyMultipaths(devices): map(lambda line: log.debug(line.rstrip()), conf) log.debug("(end of /etc/multipath.conf)")
- topology = parseMultipathOutput(iutil.execWithCapture("multipath", ["-d",])) + topology = parseMultipathOutput( + iutil.execWithCapture("multipath", ["-d",])) + topology.update(parseMultipathOutput( + iutil.execWithCapture("multipath", ["-ll",]))) # find the devices that aren't in topology, and add them into it... topodevs = reduce(lambda x,y: x.union(y), topology.values(), set()) for name in set([d['name'] for d in devices]).difference(topodevs): @@ -145,7 +162,7 @@ def identifyMultipaths(devices): log.info("adding %s to multipath_disks" % (disk,)) d["ID_FS_TYPE"] = "multipath_member" d["ID_MPATH_NAME"] = name - + multipaths.append([devmap[d] for d in disks])
non_disk_serials = {}
On 12/22/2010 05:22 AM, Ales Kozumplik wrote:
Related: rhbz#636570
storage/devicelibs/mpath.py | 77 ++++++++++++++++++++++++++----------------- 1 files changed, 47 insertions(+), 30 deletions(-)
This looks okay, so long as 4/4 is also applied.
diff --git a/storage/devicelibs/mpath.py b/storage/devicelibs/mpath.py index 4909314..0d0efde 100644 --- a/storage/devicelibs/mpath.py +++ b/storage/devicelibs/mpath.py @@ -8,32 +8,44 @@ import logging log = logging.getLogger("storage")
def parseMultipathOutput(output):
- # this function parses output from "multipath -d", so we can use its
- # logic for our topology.
- # The input looks like:
- # create: mpathc (1ATA ST3120026AS 5M) undef ATA,ST3120026AS
- # size=112G features='0' hwhandler='0' wp=undef
- # `-+- policy='round-robin 0' prio=1 status=undef
- # `- 2:0:0:0 sda 8:0 undef ready running
- # create: mpathb (36006016092d21800703762872c60db11) undef DGC,RAID 5
- # size=10G features='1 queue_if_no_path' hwhandler='1 emc' wp=undef
- # `-+- policy='round-robin 0' prio=2 status=undef
- # |- 6:0:0:0 sdb 8:16 undef ready running
- # `- 7:0:0:0 sdc 8:32 undef ready running
- # create: mpatha (36001438005deb4710000500000270000) dm-0 HP,HSV400
- # size=20G features='0' hwhandler='0' wp=rw
- # |-+- policy='round-robin 0' prio=-1 status=active
- # | |- 7:0:0:1 sda 8:0 active undef running
- # | `- 7:0:1:1 sdb 8:16 active undef running
- # `-+- policy='round-robin 0' prio=-1 status=enabled
- # |- 7:0:2:1 sdc 8:32 active undef running
- # `- 7:0:3:1 sdd 8:48 active undef running
- #
- # (In anaconda, the first one there won't be included because we blacklist
- # "ATA" as a vendor.)
- #
- # It returns a structure like:
- # [ {'mpatha':['sdb','sdc']}, ... ]
- """
- Parse output from "multipath -d" or "multipath -ll" and form a topology.
- Returns a dictionary:
- {'mpatha':['sdb','sdc'], 'mpathb': ['sdd', 'sde'], ... }
- The 'multipath -d' output looks like:
- create: mpathc (1ATA ST3120026AS 5M) undef ATA,ST3120026AS
- size=112G features='0' hwhandler='0' wp=undef
- `-+- policy='round-robin 0' prio=1 status=undef
- `- 2:0:0:0 sda 8:0 undef ready running
- create: mpathb (36006016092d21800703762872c60db11) undef DGC,RAID 5
- size=10G features='1 queue_if_no_path' hwhandler='1 emc' wp=undef
- `-+- policy='round-robin 0' prio=2 status=undef
- |- 6:0:0:0 sdb 8:16 undef ready running
- `- 7:0:0:0 sdc 8:32 undef ready running
- create: mpatha (36001438005deb4710000500000270000) dm-0 HP,HSV400
- size=20G features='0' hwhandler='0' wp=rw
- |-+- policy='round-robin 0' prio=-1 status=active
- | |- 7:0:0:1 sda 8:0 active undef running
- | `- 7:0:1:1 sdb 8:16 active undef running
- `-+- policy='round-robin 0' prio=-1 status=enabled
- |- 7:0:2:1 sdc 8:32 active undef running
- `- 7:0:3:1 sdd 8:48 active undef running
- (In anaconda, the first one there won't be included because we blacklist
- "ATA" as a vendor.)
- The 'multipath -ll' output looks like (notice the missing 'create' before
- 'mpatha'):
- mpatha (3600a0b800067fcc9000001694b557dd1) dm-0 IBM,1726-4xx FAStT
- size=360G features='0' hwhandler='1 rdac' wp=rw
- `-+- policy='round-robin 0' prio=3 status=active
|- 2:0:0:0 sda 8:0 active ready running`- 3:0:0:0 sdb 8:16 active ready running- """
Incidentally, I'm not really a fan of the blobs that change from comments to docstrings, as I don't see that we are big users of docstrings overall and it just inflates the patch size.
- Chris
On 01/03/2011 08:48 PM, Chris Lumens wrote:
diff --git a/storage/devicelibs/mpath.py b/storage/devicelibs/mpath.py index 4909314..0d0efde 100644 --- a/storage/devicelibs/mpath.py +++ b/storage/devicelibs/mpath.py @@ -8,32 +8,44 @@ import logging log = logging.getLogger("storage")
def parseMultipathOutput(output):
- # this function parses output from "multipath -d", so we can use its
- # logic for our topology.
- # The input looks like:
- # create: mpathc (1ATA ST3120026AS 5M) undef ATA,ST3120026AS
- # size=112G features='0' hwhandler='0' wp=undef
- # `-+- policy='round-robin 0' prio=1 status=undef
- # `- 2:0:0:0 sda 8:0 undef ready running
- # create: mpathb (36006016092d21800703762872c60db11) undef DGC,RAID 5
- # size=10G features='1 queue_if_no_path' hwhandler='1 emc' wp=undef
- # `-+- policy='round-robin 0' prio=2 status=undef
- # |- 6:0:0:0 sdb 8:16 undef ready running
- # `- 7:0:0:0 sdc 8:32 undef ready running
- # create: mpatha (36001438005deb4710000500000270000) dm-0 HP,HSV400
- # size=20G features='0' hwhandler='0' wp=rw
- # |-+- policy='round-robin 0' prio=-1 status=active
- # | |- 7:0:0:1 sda 8:0 active undef running
- # | `- 7:0:1:1 sdb 8:16 active undef running
- # `-+- policy='round-robin 0' prio=-1 status=enabled
- # |- 7:0:2:1 sdc 8:32 active undef running
- # `- 7:0:3:1 sdd 8:48 active undef running
- #
- # (In anaconda, the first one there won't be included because we blacklist
- # "ATA" as a vendor.)
- #
- # It returns a structure like:
- # [ {'mpatha':['sdb','sdc']}, ... ]
- """
- Parse output from "multipath -d" or "multipath -ll" and form a topology.
- Returns a dictionary:
- {'mpatha':['sdb','sdc'], 'mpathb': ['sdd', 'sde'], ... }
- The 'multipath -d' output looks like:
- create: mpathc (1ATA ST3120026AS 5M) undef ATA,ST3120026AS
- size=112G features='0' hwhandler='0' wp=undef
- `-+- policy='round-robin 0' prio=1 status=undef
- `- 2:0:0:0 sda 8:0 undef ready running
- create: mpathb (36006016092d21800703762872c60db11) undef DGC,RAID 5
- size=10G features='1 queue_if_no_path' hwhandler='1 emc' wp=undef
- `-+- policy='round-robin 0' prio=2 status=undef
- |- 6:0:0:0 sdb 8:16 undef ready running
- `- 7:0:0:0 sdc 8:32 undef ready running
- create: mpatha (36001438005deb4710000500000270000) dm-0 HP,HSV400
- size=20G features='0' hwhandler='0' wp=rw
- |-+- policy='round-robin 0' prio=-1 status=active
- | |- 7:0:0:1 sda 8:0 active undef running
- | `- 7:0:1:1 sdb 8:16 active undef running
- `-+- policy='round-robin 0' prio=-1 status=enabled
- |- 7:0:2:1 sdc 8:32 active undef running
- `- 7:0:3:1 sdd 8:48 active undef running
- (In anaconda, the first one there won't be included because we blacklist
- "ATA" as a vendor.)
- The 'multipath -ll' output looks like (notice the missing 'create' before
- 'mpatha'):
- mpatha (3600a0b800067fcc9000001694b557dd1) dm-0 IBM,1726-4xx FAStT
- size=360G features='0' hwhandler='1 rdac' wp=rw
- `-+- policy='round-robin 0' prio=3 status=active
|- 2:0:0:0 sda 8:0 active ready running`- 3:0:0:0 sdb 8:16 active ready running- """
Incidentally, I'm not really a fan of the blobs that change from comments to docstrings, as I don't see that we are big users of docstrings overall and it just inflates the patch size.
- Chris
Yeah, but I am sortof feeling like I've taken over the ownership of this file so I want it to look nice (i.e. rest of the storage code). It's the same problem like the tabs vs spaces or trailing whitespace. Not needed but while at it.. (I changed the docstring too).
Ales
This change necessitates from two previous commits: 1) 6e6d3df9a78730996e6ac7d463e3b50dbc164b4c, resulting in DeviceTree.populate() being called twice now, for the second time at the end of autopart.
2) 5c83bbf01800ca86d1c8e4cd95d14480165b344c, which causes already coalesced mpath devices (disks and partitions) never to be torn down during anaconda run.
Given that, we don't want to let identifyMultipath give DeviceTree.populate() existing mpath devices (dm-0, dm-1, etc.). populate() would try to add those before the slave mpath members (sda, sdb) and although the addUdevDevice() logic will eventually recursively request sda and sdb while working on dm-0, the udev info we get for sda in sdb at that point is obtained thorugh a call to udev_get_block_device() which will not give us a correctly filled in ID_FS_TYPE (unlike the enriched info dictionaries that idenitfyMultipath() gives us).
Alternative solution would be to embed a layer between DeviceTree and udev.py. This layer would remember the additional database values that are not returned by udev itself but rather discovered by us, such as the "multipath_member" value of ID_FS_TYPE.
For the partitions part, the patch only does what identifyMultipath()'s documentation promises: "3) removes the individual members of an mpath's partitions". This step is optional and to make the function better defined: note that e.g. sda3 will be scanned later in populate() anyway as it will naturally still appear in udev_get_block_devices().
Resolves: rhbz#636570 --- storage/devicelibs/mpath.py | 70 +++++++++++++++++++++++++++--------------- storage/devicetree.py | 13 ++++---- storage/udev.py | 8 +++++ 3 files changed, 59 insertions(+), 32 deletions(-)
diff --git a/storage/devicelibs/mpath.py b/storage/devicelibs/mpath.py index 0d0efde..113c544 100644 --- a/storage/devicelibs/mpath.py +++ b/storage/devicelibs/mpath.py @@ -7,6 +7,31 @@ import logging
log = logging.getLogger("storage")
+def _filter_out_mpath_devices(devices): + retval = [] + for d in devices: + if udev_device_is_multipath(d): + log.debug("filtering out coalesced mpath device: %s" % d['name']) + else: + retval.append(d) + return retval + +def _filter_out_mpath_partitions(devices, multipaths): + """ + Use serial numbers of the multipath members to filter devices from the + devices list. The returned list will only partitions that are NOT partitions + of the multipath members. + """ + serials = set(udev_device_get_serial(d) + for mpath_members in multipaths for d in mpath_members) + retval = [] + for d in devices: + if udev_device_get_serial(d) in serials: + log.debug("filtering out mpath partition: %s" % d['name']) + else: + retval.append(d) + return retval + def parseMultipathOutput(output): """ Parse output from "multipath -d" or "multipath -ll" and form a topology. @@ -84,14 +109,22 @@ def parseMultipathOutput(output): return mpaths
def identifyMultipaths(devices): - # this function does a couple of things - # 1) identifies multipath disks - # 2) sets their ID_FS_TYPE to multipath_member - # 3) removes the individual members of an mpath's partitions - # sample input with multipath pair [sdb,sdc] - # [sr0, sda, sda1, sdb, sdb1, sdb2, sdc, sdc1, sdd, sdd1, sdd2] - # sample output: - # [sda, sdd], [[sdb, sdc]], [sr0, sda1, sdd1, sdd2]] + """ + This function does a couple of things: + 1) identifies multipath disks, + 2) sets their ID_FS_TYPE to multipath_member, + 3) removes the individual members of an mpath's partitions as well as any + coalesced multipath devices: + + sample input: + [sr0, sda, sda1, sdb, sdb1, sdb2, sdc, sdc1, sdd, sdd1, sdd2, dm-0] + where: + [sdb, sdc] is a multipath pair + dm-0 is a mutliapth device already coalesced from [sdb, sdc] + + sample output: + [sda, sdd], [[sdb, sdc]], [sr0, sda1, sdd1, sdd2]] + """ log.info("devices to scan for multipath: %s" % [d['name'] for d in devices])
with open("/etc/multipath.conf") as conf: @@ -165,23 +198,10 @@ def identifyMultipaths(devices):
multipaths.append([devmap[d] for d in disks])
- non_disk_serials = {} - for name,device in non_disk_devices.items(): - serial = udev_device_get_serial(device) - non_disk_serials.setdefault(serial, []) - non_disk_serials[serial].append(device) - - for mpath in multipaths: - for serial in [d.get('ID_SERIAL_SHORT') for d in mpath]: - if non_disk_serials.has_key(serial): - log.info("filtering out non disk devices [%s]" % [d['name'] for d in non_disk_serials[serial]]) - for name in [d['name'] for d in non_disk_serials[serial]]: - if non_disk_devices.has_key(name): - del non_disk_devices[name] - - partition_devices = [] - for device in non_disk_devices.values(): - partition_devices.append(device) + # singlepaths and partitions should not contain multipath devices: + singlepath_disks = _filter_out_mpath_devices(singlepath_disks) + partition_devices = _filter_out_mpath_partitions( + non_disk_devices.values(), multipaths)
# this is the list of devices we want to keep from the original # device list, but we want to maintain its original order. diff --git a/storage/devicetree.py b/storage/devicetree.py index 45fc455..f45e656 100644 --- a/storage/devicetree.py +++ b/storage/devicetree.py @@ -2026,19 +2026,18 @@ class DeviceTree(object): % (livetarget,)) self.protectedDevNames.append(livetarget)
- # First iteration - let's just look for disks. - old_devices = {} - - devices = udev_get_block_devices() - for dev in devices: - old_devices[dev['name']] = dev - cfg = self.__multipathConfigWriter.write() open("/etc/multipath.conf", "w+").write(cfg) del cfg
+ devices = udev_get_block_devices() (singles, mpaths, partitions) = devicelibs.mpath.identifyMultipaths(devices) devices = singles + reduce(list.__add__, mpaths, []) + partitions + # remember all the devices idenitfyMultipaths() gave us at this point + old_devices = {} + for dev in devices: + old_devices[dev['name']] = dev + log.info("devices to scan: %s" % [d['name'] for d in devices]) for dev in devices: self.addUdevDevice(dev) diff --git a/storage/udev.py b/storage/udev.py index 59b7519..7ba5a9c 100644 --- a/storage/udev.py +++ b/storage/udev.py @@ -472,6 +472,14 @@ def udev_device_is_dmraid_partition(info, devicetree):
return False
+def udev_device_is_multipath(info): + """ Return True if the device is a multipath device or a partition of one.""" + if not udev_device_is_dm(info): + return False + if udev_device_get_name(info).startswith("mpath"): + return True + return False + def udev_device_is_multipath_partition(info, devicetree): """ Return True if the device is a partition of a multipath device. """ if not udev_device_is_dm(info):
On Wed, Dec 22, 2010 at 11:22:41AM +0100, Ales Kozumplik wrote:
- sample input:
- [sr0, sda, sda1, sdb, sdb1, sdb2, sdc, sdc1, sdd, sdd1, sdd2, dm-0]
- where:
- [sdb, sdc] is a multipath pair
- dm-0 is a mutliapth device already coalesced from [sdb, sdc]
- sample output:
- [sda, sdd], [[sdb, sdc]], [sr0, sda1, sdd1, sdd2]]
Could you explain the return values in more detail? Is this always a 3 element list or does it depend on the number of devices? What does it look like with no multipath devices?
Thanks, Brian
On 01/03/2011 07:22 PM, Brian C. Lane wrote:
On Wed, Dec 22, 2010 at 11:22:41AM +0100, Ales Kozumplik wrote:
- sample input:
- [sr0, sda, sda1, sdb, sdb1, sdb2, sdc, sdc1, sdd, sdd1, sdd2, dm-0]
- where:
- [sdb, sdc] is a multipath pair
- dm-0 is a mutliapth device already coalesced from [sdb, sdc]
- sample output:
- [sda, sdd], [[sdb, sdc]], [sr0, sda1, sdd1, sdd2]]
Could you explain the return values in more detail? Is this always a 3 element list or does it depend on the number of devices? What does it look like with no multipath devices?
Thanks, Brian
Yes,
it is always a tuple of 3 lists, the middle list is empty if there are no multipath devices found. I'll make sure to document it before pushing.
Ales
This seems okay to me. We should be able to handle this better in the device tree, but this approach will do for now.
Dave
On Wed, 2010-12-22 at 11:22 +0100, Ales Kozumplik wrote:
This change necessitates from two previous commits:
- 6e6d3df9a78730996e6ac7d463e3b50dbc164b4c, resulting in
DeviceTree.populate() being called twice now, for the second time at the end of autopart.
- 5c83bbf01800ca86d1c8e4cd95d14480165b344c, which causes already
coalesced mpath devices (disks and partitions) never to be torn down during anaconda run.
Given that, we don't want to let identifyMultipath give DeviceTree.populate() existing mpath devices (dm-0, dm-1, etc.). populate() would try to add those before the slave mpath members (sda, sdb) and although the addUdevDevice() logic will eventually recursively request sda and sdb while working on dm-0, the udev info we get for sda in sdb at that point is obtained thorugh a call to udev_get_block_device() which will not give us a correctly filled in ID_FS_TYPE (unlike the enriched info dictionaries that idenitfyMultipath() gives us).
Alternative solution would be to embed a layer between DeviceTree and udev.py. This layer would remember the additional database values that are not returned by udev itself but rather discovered by us, such as the "multipath_member" value of ID_FS_TYPE.
For the partitions part, the patch only does what identifyMultipath()'s documentation promises: "3) removes the individual members of an mpath's partitions". This step is optional and to make the function better defined: note that e.g. sda3 will be scanned later in populate() anyway as it will naturally still appear in udev_get_block_devices().
Resolves: rhbz#636570
storage/devicelibs/mpath.py | 70 +++++++++++++++++++++++++++--------------- storage/devicetree.py | 13 ++++---- storage/udev.py | 8 +++++ 3 files changed, 59 insertions(+), 32 deletions(-)
diff --git a/storage/devicelibs/mpath.py b/storage/devicelibs/mpath.py index 0d0efde..113c544 100644 --- a/storage/devicelibs/mpath.py +++ b/storage/devicelibs/mpath.py @@ -7,6 +7,31 @@ import logging
log = logging.getLogger("storage")
+def _filter_out_mpath_devices(devices):
- retval = []
- for d in devices:
if udev_device_is_multipath(d):log.debug("filtering out coalesced mpath device: %s" % d['name'])else:retval.append(d)- return retval
+def _filter_out_mpath_partitions(devices, multipaths):
- """
- Use serial numbers of the multipath members to filter devices from the
- devices list. The returned list will only partitions that are NOT partitions
- of the multipath members.
- """
- serials = set(udev_device_get_serial(d)
for mpath_members in multipaths for d in mpath_members)- retval = []
- for d in devices:
if udev_device_get_serial(d) in serials:log.debug("filtering out mpath partition: %s" % d['name'])else:retval.append(d)- return retval
def parseMultipathOutput(output): """ Parse output from "multipath -d" or "multipath -ll" and form a topology. @@ -84,14 +109,22 @@ def parseMultipathOutput(output): return mpaths
def identifyMultipaths(devices):
- # this function does a couple of things
- # 1) identifies multipath disks
- # 2) sets their ID_FS_TYPE to multipath_member
- # 3) removes the individual members of an mpath's partitions
- # sample input with multipath pair [sdb,sdc]
- # [sr0, sda, sda1, sdb, sdb1, sdb2, sdc, sdc1, sdd, sdd1, sdd2]
- # sample output:
- # [sda, sdd], [[sdb, sdc]], [sr0, sda1, sdd1, sdd2]]
"""
This function does a couple of things:
- identifies multipath disks,
- sets their ID_FS_TYPE to multipath_member,
- removes the individual members of an mpath's partitions as well as any
coalesced multipath devices:
sample input:
[sr0, sda, sda1, sdb, sdb1, sdb2, sdc, sdc1, sdd, sdd1, sdd2, dm-0]
where:
[sdb, sdc] is a multipath pair
dm-0 is a mutliapth device already coalesced from [sdb, sdc]
sample output:
[sda, sdd], [[sdb, sdc]], [sr0, sda1, sdd1, sdd2]]
""" log.info("devices to scan for multipath: %s" % [d['name'] for d in devices])
with open("/etc/multipath.conf") as conf:
@@ -165,23 +198,10 @@ def identifyMultipaths(devices):
multipaths.append([devmap[d] for d in disks])
- non_disk_serials = {}
- for name,device in non_disk_devices.items():
serial = udev_device_get_serial(device)non_disk_serials.setdefault(serial, [])non_disk_serials[serial].append(device)- for mpath in multipaths:
for serial in [d.get('ID_SERIAL_SHORT') for d in mpath]:if non_disk_serials.has_key(serial):log.info("filtering out non disk devices [%s]" % [d['name'] for d in non_disk_serials[serial]])for name in [d['name'] for d in non_disk_serials[serial]]:if non_disk_devices.has_key(name):del non_disk_devices[name]- partition_devices = []
- for device in non_disk_devices.values():
partition_devices.append(device)
# singlepaths and partitions should not contain multipath devices:
singlepath_disks = _filter_out_mpath_devices(singlepath_disks)
partition_devices = _filter_out_mpath_partitions(
non_disk_devices.values(), multipaths)# this is the list of devices we want to keep from the original # device list, but we want to maintain its original order.
diff --git a/storage/devicetree.py b/storage/devicetree.py index 45fc455..f45e656 100644 --- a/storage/devicetree.py +++ b/storage/devicetree.py @@ -2026,19 +2026,18 @@ class DeviceTree(object): % (livetarget,)) self.protectedDevNames.append(livetarget)
# First iteration - let's just look for disks.old_devices = {}devices = udev_get_block_devices()for dev in devices:old_devices[dev['name']] = devcfg = self.__multipathConfigWriter.write() open("/etc/multipath.conf", "w+").write(cfg) del cfg
devices = udev_get_block_devices() (singles, mpaths, partitions) = devicelibs.mpath.identifyMultipaths(devices) devices = singles + reduce(list.__add__, mpaths, []) + partitions# remember all the devices idenitfyMultipaths() gave us at this pointold_devices = {}for dev in devices:old_devices[dev['name']] = devlog.info("devices to scan: %s" % [d['name'] for d in devices]) for dev in devices: self.addUdevDevice(dev)diff --git a/storage/udev.py b/storage/udev.py index 59b7519..7ba5a9c 100644 --- a/storage/udev.py +++ b/storage/udev.py @@ -472,6 +472,14 @@ def udev_device_is_dmraid_partition(info, devicetree):
return False+def udev_device_is_multipath(info):
- """ Return True if the device is a multipath device or a partition of one."""
- if not udev_device_is_dm(info):
return False- if udev_device_get_name(info).startswith("mpath"):
return True- return False
def udev_device_is_multipath_partition(info, devicetree): """ Return True if the device is a partition of a multipath device. """ if not udev_device_is_dm(info):
Please do use udev_device_is_dm_mpath instead of adding another function to do the same thing. I forgot that function had made it to rhel6-branch.
Dave
diff --git a/storage/udev.py b/storage/udev.py index 59b7519..7ba5a9c 100644 --- a/storage/udev.py +++ b/storage/udev.py @@ -472,6 +472,14 @@ def udev_device_is_dmraid_partition(info, devicetree):
return False+def udev_device_is_multipath(info):
- """ Return True if the device is a multipath device or a partition of one."""
- if not udev_device_is_dm(info):
return False- if udev_device_get_name(info).startswith("mpath"):
return True- return False
def udev_device_is_multipath_partition(info, devicetree): """ Return True if the device is a partition of a multipath device. """ if not udev_device_is_dm(info):
On 01/06/2011 05:24 PM, David Lehman wrote:
Please do use udev_device_is_dm_mpath instead of adding another function to do the same thing. I forgot that function had made it to rhel6-branch.
Dave
I said yesterday this is good, but now I've found one more thing we need to address first. I can not use udev_device_is_dm_mpath to test for a partition of an mpath device, the function uses udev_device_dm_subsystem_match() that looks at the first field in DM_UUID which is like "mpath-2003013842bcb000c" for multipath devices but "part1-mpath-2003013842bcb000c" for multipath partitions.
I could workaround this on rhel6 branch where there still is the disgraceful udev_device_is_multipath_partition(), but then I'd hit the problem on master where the function was removed.
I am thinking about solving this by adding a parameter to ude_device_dm_subsystem_match() that will make it skip the first DM_UUID field if it was 'part\d+'.
Ales
This change necessitates from two previous commits: 1) 6e6d3df9a78730996e6ac7d463e3b50dbc164b4c, resulting in DeviceTree.populate() being called twice now, for the second time at the end of autopart.
2) 5c83bbf01800ca86d1c8e4cd95d14480165b344c, which causes already coalesced mpath devices (disks and partitions) never to be torn down during anaconda run.
Given that, we don't want to let identifyMultipath give DeviceTree.populate() existing mpath devices (dm-0, dm-1, etc.). populate() would try to add those before the slave mpath members (sda, sdb) and although the addUdevDevice() logic will eventually recursively request sda and sdb while working on dm-0, the udev info we get for sda in sdb at that point is obtained thorugh a call to udev_get_block_device() which will not give us a correctly filled in ID_FS_TYPE (unlike the enriched info dictionaries that idenitfyMultipath() gives us).
Alternative solution would be to embed a layer between DeviceTree and udev.py. This layer would remember the additional database values that are not returned by udev itself but rather discovered by us, such as the "multipath_member" value of ID_FS_TYPE.
For the partitions part, the patch only does what identifyMultipath()'s documentation promises: "3) removes the individual members of an mpath's partitions". This step is optional and to make the function better defined: note that e.g. sda3 will be scanned later in populate() anyway as it will naturally still appear in udev_get_block_devices().
Resolves: rhbz#636570 --- storage/devicelibs/mpath.py | 75 ++++++++++++++++++++++++++++-------------- storage/devicetree.py | 13 +++---- storage/udev.py | 22 ++++++++++--- 3 files changed, 73 insertions(+), 37 deletions(-)
diff --git a/storage/devicelibs/mpath.py b/storage/devicelibs/mpath.py index 0d0efde..fb7e1aa 100644 --- a/storage/devicelibs/mpath.py +++ b/storage/devicelibs/mpath.py @@ -7,6 +7,31 @@ import logging
log = logging.getLogger("storage")
+def _filter_out_mpath_devices(devices): + retval = [] + for d in devices: + if udev_device_is_dm_mpath(d): + log.debug("filtering out coalesced mpath device: %s" % d['name']) + else: + retval.append(d) + return retval + +def _filter_out_mpath_partitions(devices, multipaths): + """ + Use serial numbers of the multipath members to filter devices from the + devices list. The returned list will only partitions that are NOT partitions + of the multipath members. + """ + serials = set(udev_device_get_serial(d) + for mpath_members in multipaths for d in mpath_members) + retval = [] + for d in devices: + if udev_device_get_serial(d) in serials: + log.debug("filtering out mpath partition: %s" % d['name']) + else: + retval.append(d) + return retval + def parseMultipathOutput(output): """ Parse output from "multipath -d" or "multipath -ll" and form a topology. @@ -84,14 +109,27 @@ def parseMultipathOutput(output): return mpaths
def identifyMultipaths(devices): - # this function does a couple of things - # 1) identifies multipath disks - # 2) sets their ID_FS_TYPE to multipath_member - # 3) removes the individual members of an mpath's partitions - # sample input with multipath pair [sdb,sdc] - # [sr0, sda, sda1, sdb, sdb1, sdb2, sdc, sdc1, sdd, sdd1, sdd2] - # sample output: - # [sda, sdd], [[sdb, sdc]], [sr0, sda1, sdd1, sdd2]] + """ + This function does a couple of things: + 1) identifies multipath disks, + 2) sets their ID_FS_TYPE to multipath_member, + 3) removes the individual members of an mpath's partitions as well as any + coalesced multipath devices: + + The return value is a tuple of 3 lists, the first list containing all + devices except multipath members and partitions, the second list containing + only the multipath members and the last list only partitions. Specifically, + the second list is empty if there are no multipath devices found. + + sample input: + [sr0, sda, sda1, sdb, sdb1, sdb2, sdc, sdc1, sdd, sdd1, sdd2, dm-0] + where: + [sdb, sdc] is a multipath pair + dm-0 is a mutliapth device already coalesced from [sdb, sdc] + + sample output: + [sda, sdd], [[sdb, sdc]], [sr0, sda1, sdd1, sdd2]] + """ log.info("devices to scan for multipath: %s" % [d['name'] for d in devices])
with open("/etc/multipath.conf") as conf: @@ -165,23 +203,10 @@ def identifyMultipaths(devices):
multipaths.append([devmap[d] for d in disks])
- non_disk_serials = {} - for name,device in non_disk_devices.items(): - serial = udev_device_get_serial(device) - non_disk_serials.setdefault(serial, []) - non_disk_serials[serial].append(device) - - for mpath in multipaths: - for serial in [d.get('ID_SERIAL_SHORT') for d in mpath]: - if non_disk_serials.has_key(serial): - log.info("filtering out non disk devices [%s]" % [d['name'] for d in non_disk_serials[serial]]) - for name in [d['name'] for d in non_disk_serials[serial]]: - if non_disk_devices.has_key(name): - del non_disk_devices[name] - - partition_devices = [] - for device in non_disk_devices.values(): - partition_devices.append(device) + # singlepaths and partitions should not contain multipath devices: + singlepath_disks = _filter_out_mpath_devices(singlepath_disks) + partition_devices = _filter_out_mpath_partitions( + non_disk_devices.values(), multipaths)
# this is the list of devices we want to keep from the original # device list, but we want to maintain its original order. diff --git a/storage/devicetree.py b/storage/devicetree.py index 45fc455..f45e656 100644 --- a/storage/devicetree.py +++ b/storage/devicetree.py @@ -2026,19 +2026,18 @@ class DeviceTree(object): % (livetarget,)) self.protectedDevNames.append(livetarget)
- # First iteration - let's just look for disks. - old_devices = {} - - devices = udev_get_block_devices() - for dev in devices: - old_devices[dev['name']] = dev - cfg = self.__multipathConfigWriter.write() open("/etc/multipath.conf", "w+").write(cfg) del cfg
+ devices = udev_get_block_devices() (singles, mpaths, partitions) = devicelibs.mpath.identifyMultipaths(devices) devices = singles + reduce(list.__add__, mpaths, []) + partitions + # remember all the devices idenitfyMultipaths() gave us at this point + old_devices = {} + for dev in devices: + old_devices[dev['name']] = dev + log.info("devices to scan: %s" % [d['name'] for d in devices]) for dev in devices: self.addUdevDevice(dev) diff --git a/storage/udev.py b/storage/udev.py index 59b7519..c489da5 100644 --- a/storage/udev.py +++ b/storage/udev.py @@ -399,10 +399,22 @@ def udev_device_get_lv_attr(info): attr = [attr] return attr
-def udev_device_dm_subsystem_match(info, subsystem): - """ Return True if the device matches a given device-mapper subsystem. """ +def udev_device_dm_subsystem_match(info, subsystem, accept_partitions=False): + """ Return True if the device matches a given device-mapper subsystem. + + Examples of DM_UUID values, mpath device: + mpath-2003013842bcb000c + partition of an mpath device: + part1-mpath-2003013842bcb000c + """ uuid = info.get("DM_UUID", "") - _subsystem = uuid.split("-")[0] + split = uuid.split("-") + subsystem_idx = 0 + if (accept_partitions and + len(split) > 1 and + split[0].startswith("part")): + subsystem_idx += 1 + _subsystem = split[subsystem_idx] if _subsystem == uuid or not _subsystem: return False
@@ -431,8 +443,8 @@ def udev_device_is_dm_raid(info): return udev_device_dm_subsystem_match(info, "dmraid")
def udev_device_is_dm_mpath(info): - """ Return True if the device is an. """ - return udev_device_dm_subsystem_match(info, "mpath") + """ Return True if the device is a multipath device. """ + return udev_device_dm_subsystem_match(info, "mpath", accept_partitions=True)
def udev_device_is_biosraid(info): # Note that this function does *not* identify raid sets.
Check the version of udev_device_dm_subsystem_match on master -- it does what you want done.
Dave
On 01/07/2011 04:02 PM, David Lehman wrote:
Check the version of udev_device_dm_subsystem_match on master -- it does what you want done.
Dave
Thanks,
I picked the patch that did that back to rhel6-branch.
Al_
On Fri, Jan 07, 2011 at 02:04:15PM +0100, Ales Kozumplik wrote:
On 01/06/2011 05:24 PM, David Lehman wrote:
Please do use udev_device_is_dm_mpath instead of adding another function to do the same thing. I forgot that function had made it to rhel6-branch.
Dave
I said yesterday this is good, but now I've found one more thing we need to address first. I can not use udev_device_is_dm_mpath to test for a partition of an mpath device, the function uses udev_device_dm_subsystem_match() that looks at the first field in DM_UUID which is like "mpath-2003013842bcb000c" for multipath devices but "part1-mpath-2003013842bcb000c" for multipath partitions.
I could workaround this on rhel6 branch where there still is the disgraceful udev_device_is_multipath_partition(), but then I'd hit the problem on master where the function was removed.
I am thinking about solving this by adding a parameter to ude_device_dm_subsystem_match() that will make it skip the first DM_UUID field if it was 'part\d+'.
FYI, this is related to rhbz#634771, so a special check for ^part\d+ is valid for RHEL but not Fedora.
On 01/07/2011 06:36 PM, Brian C. Lane wrote:
On Fri, Jan 07, 2011 at 02:04:15PM +0100, Ales Kozumplik wrote:
On 01/06/2011 05:24 PM, David Lehman wrote:
Please do use udev_device_is_dm_mpath instead of adding another function to do the same thing. I forgot that function had made it to rhel6-branch.
Dave
I said yesterday this is good, but now I've found one more thing we need to address first. I can not use udev_device_is_dm_mpath to test for a partition of an mpath device, the function uses udev_device_dm_subsystem_match() that looks at the first field in DM_UUID which is like "mpath-2003013842bcb000c" for multipath devices but "part1-mpath-2003013842bcb000c" for multipath partitions.
I could workaround this on rhel6 branch where there still is the disgraceful udev_device_is_multipath_partition(), but then I'd hit the problem on master where the function was removed.
I am thinking about solving this by adding a parameter to ude_device_dm_subsystem_match() that will make it skip the first DM_UUID field if it was 'part\d+'.
FYI, this is related to rhbz#634771, so a special check for ^part\d+ is valid for RHEL but not Fedora.
I'm not sure if this has been fixed on master yet, or do you think so? In that case we can simplify udev_dm_subystem_match() again.
Ales
On Mon, Jan 10, 2011 at 01:07:24PM +0100, Ales Kozumplik wrote:
On 01/07/2011 06:36 PM, Brian C. Lane wrote:
FYI, this is related to rhbz#634771, so a special check for ^part\d+ is valid for RHEL but not Fedora.
I'm not sure if this has been fixed on master yet, or do you think so? In that case we can simplify udev_dm_subystem_match() again.
Upstream hasn't changed it yet. I'll clone the bug against rawhide and re-attach my patch to it.
anaconda-devel@lists.fedoraproject.org