[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [rhel6-branch 1/2] edd: use pci_dev as an additional clue for disk boot priority (#621175)



Both of these look okay to me, but I'd feel better having all of this
new code validated by the reporter of the bug.

Dave

On Thu, 2010-12-16 at 09:59 +0100, Ales Kozumplik wrote:
> In general there is not always mbrsig on a disk (new device), so we can't
> always succeed in creating the disk -> bios id mapping. On the other hand,
> the pci_dev symlink in edd does not give us complete information about the
> device. So we have to consider both in compareDisks().
> ---
>  kickstart.py              |    4 +-
>  storage/__init__.py       |   18 ++++--
>  storage/devicelibs/edd.py |  167 ++++++++++++++++++++++++++++++---------------
>  3 files changed, 126 insertions(+), 63 deletions(-)
> 
> diff --git a/kickstart.py b/kickstart.py
> index b606ab8..67b1480 100644
> --- a/kickstart.py
> +++ b/kickstart.py
> @@ -665,8 +665,8 @@ class PartitionData(commands.partition.F12_PartData):
>          storage.doAutoPart = False
>  
>          if self.onbiosdisk != "":
> -            for (disk, biosdisk) in storage.eddDict.iteritems():
> -                if str(biosdisk) == self.onbiosdisk:
> +            for (disk, eddinfo) in storage.eddDict.iteritems():
> +                if str(eddinfo.biosdisk) == self.onbiosdisk:
>                      self.disk = disk
>                      break
>  
> diff --git a/storage/__init__.py b/storage/__init__.py
> index 464eebf..09dab1f 100644
> --- a/storage/__init__.py
> +++ b/storage/__init__.py
> @@ -1180,18 +1180,26 @@ class Storage(object):
>          return self.fsset.rootDevice
>  
>      def compareDisks(self, first, second):
> -        if self.eddDict.has_key(first) and self.eddDict.has_key(second):
> -            one = self.eddDict[first]
> -            two = self.eddDict[second]
> +        # if exactly one of the devices has a pcidev, prefer it
> +        if bool(self.eddDict[first].pcidev) ^ bool(self.eddDict[second].pcidev):
> +            if self.eddDict[first].pcidev:
> +                return -1
> +            if self.eddDict[second].pcidev:
> +                return 1
> +
> +        # next, compare the biosdev numbers
> +        if self.eddDict[first].biosdev and self.eddDict[second].biosdev:
> +            one = self.eddDict[first].biosdev
> +            two = self.eddDict[second].biosdev
>              if (one < two):
>                  return -1
>              elif (one > two):
>                  return 1
>  
>          # if one is in the BIOS and the other not prefer the one in the BIOS
> -        if self.eddDict.has_key(first):
> +        if self.eddDict[first].biosdev:
>              return -1
> -        if self.eddDict.has_key(second):
> +        if self.eddDict[second].biosdev:
>              return 1
>  
>          if first.startswith("hd"):
> diff --git a/storage/devicelibs/edd.py b/storage/devicelibs/edd.py
> index da03914..db2ba57 100644
> --- a/storage/devicelibs/edd.py
> +++ b/storage/devicelibs/edd.py
> @@ -21,77 +21,132 @@
>  #
>  
>  import os
> +import os.path
>  import struct
>  
>  import logging
>  log = logging.getLogger("storage")
>  
> +class EddInfo:
> +    def __init__(self, biosdev):
> +        self.biosdev = biosdev
> +        self.pcidev = None
> +
>  def get_edd_dict(devices):
> -    """Given an array of devices return a dict with the BIOS ID for them."""
> +    """
> +    Given an array of devices return a mapping from their devices names to their
> +    EddInfo.
> +
> +    CAVEATS: It is important to note that the current implementation of the edd
> +    kernel module is incomplete and does not give us a reliable method about
> +    discovering the complete device <-> BIOS device mapping. Checking the MBR
> +    signature works in most cases but will always fail e.g. on new disks that do
> +    not contain an MBR yet. On the other hand, the pcidev entry which is
> +    guaranteed to exist is shared across all disks connected through the same
> +    PCI device and so does not provide a one-to-one relation.
> +    """
> +
> +    # initialize the dict with blank info
>      edd_dict = {}
> +    for dev in devices:
> +        edd_dict[dev.name] = EddInfo(None)
>  
>      for biosdev in range(80, 80 + 15):
> +
>          sysfspath = "/sys/firmware/edd/int13_dev%d" % biosdev
>          if not os.path.exists(sysfspath):
>              break # We are done
>  
> -        sysfspath = "/sys/firmware/edd/int13_dev%d/mbr_signature" % biosdev
> -        if not os.path.exists(sysfspath):
> -            log.warning("No mbrsig for biosdev: %d" % biosdev)
> -            continue
> +        dev = _find_device_from_mbrsig(devices, biosdev)
> +        if dev:
> +            edd_dict[dev].biosdev = biosdev
> +
> +        (pcidev, devs_for_pcidev) = _devices_for_pcidev(devices, biosdev)
> +        for d in devs_for_pcidev:
> +            edd_dict[d.name].pcidev = pcidev
> +
> +    _dump_dict(edd_dict)
> +    return edd_dict
> +
> +def _dump_dict(edd_dict):
> +    log.debug("edd:generated dict:")
> +    for key in edd_dict:
> +        log.debug("%s: biosdev: %s, pcidev: %s" %
> +                  (key, edd_dict[key].biosdev, edd_dict[key].pcidev))
> +    log.debug("edd:end of generated dict dump")
> +
> +def _find_device_from_mbrsig(devices, biosdev):
> +    sysfspath = "/sys/firmware/edd/int13_dev%d/mbr_signature" % biosdev
> +    if not os.path.exists(sysfspath):
> +        log.info("edd: no mbrsig for biosdev: %d" % biosdev)
> +        return None
> +
> +    try:
> +        file = open(sysfspath, "r")
> +        eddsig = file.read()
> +        file.close()
> +    except (IOError, OSError) as e:
> +        log.info("edd: error reading EDD mbrsig for %d: %s" %
> +                    (biosdev, str(e)))
> +        return None
>  
> +    sysfspath = "/sys/firmware/edd/int13_dev%d/sectors" % biosdev
> +    try:
> +        file = open(sysfspath, "r")
> +        eddsize = file.read()
> +        file.close()
> +    except (IOError, OSError) as e:
> +        eddsize = None
> +
> +    found = []
> +    for dev in devices:
>          try:
> -            file = open(sysfspath, "r")
> -            eddsig = file.read()
> -            file.close()
> -        except (IOError, OSError) as e:
> -            log.warning("Error reading EDD mbrsig for %d: %s" %
> -                        (biosdev, str(e)))
> +            fd = os.open(dev.path, os.O_RDONLY)
> +            os.lseek(fd, 440, 0)
> +            mbrsig = struct.unpack('I', os.read(fd, 4))
> +            os.close(fd)
> +        except OSError as e:
> +            log.info("edd: error reading mbrsig from disk %s: %s" %
> +                        (dev.name, str(e)))
>              continue
>  
> -        sysfspath = "/sys/firmware/edd/int13_dev%d/sectors" % biosdev
> -        try:
> -            file = open(sysfspath, "r")
> -            eddsize = file.read()
> -            file.close()
> -        except (IOError, OSError) as e:
> -            eddsize = None
> -
> -        found = []
> -        for dev in devices:
> -            try:
> -                fd = os.open(dev.path, os.O_RDONLY)
> -                os.lseek(fd, 440, 0) 
> -                mbrsig = struct.unpack('I', os.read(fd, 4))
> -                os.close(fd)
> -            except OSError as e:
> -                log.warning("Error reading mbrsig from disk %s: %s" %
> -                            (dev.name, str(e)))
> -                continue
> -
> -            mbrsigStr = "0x%08x\n" % mbrsig
> -            if mbrsigStr == eddsig:
> -                if eddsize:
> -                    sysfspath = "/sys%s/size" % dev.sysfsPath
> -                    try:
> -                        file = open(sysfspath, "r")
> -                        size = file.read()
> -                        file.close()
> -                    except (IOError, OSError) as e:
> -                        log.warning("Error getting size for: %s" % dev.name)
> -                        continue
> -                    if eddsize != size:
> -                        continue
> -                found.append(dev.name)
> -
> -        if not found:
> -            log.error("No matching mbr signature found for biosdev %d" %
> -                      biosdev)
> -        elif len(found) > 1:
> -            log.error("Multiple signature matches found for biosdev %d: %s" %
> -                      (biosdev, str(found)))
> -        else:
> -            log.info("Found %s for biosdev %d" %(found[0], biosdev))
> -            edd_dict[found[0]] = biosdev
> +        mbrsigStr = "0x%08x\n" % mbrsig
> +        if mbrsigStr == eddsig:
> +            if eddsize:
> +                sysfspath = "/sys%s/size" % dev.sysfsPath
> +                try:
> +                    file = open(sysfspath, "r")
> +                    size = file.read()
> +                    file.close()
> +                except (IOError, OSError) as e:
> +                    log.warning("Error getting size for: %s" % dev.name)
> +                    continue
> +                if eddsize != size:
> +                    continue
> +            found.append(dev.name)
>  
> -    return edd_dict
> +    if not found:
> +        log.error("edd: no matching mbr signature found for biosdev %d" %
> +                  biosdev)
> +        return None
> +    elif len(found) > 1:
> +        log.error("edd: multiple signature matches found for biosdev %d: %s" %
> +                  (biosdev, str(found)))
> +        return None
> +    else:
> +        return found[0]
> +
> +def _devices_for_pcidev(devices, biosdev):
> +    edd_dir = "/sys/firmware/edd/int13_dev%d" % biosdev
> +    try:
> +        dereferenced = os.readlink("%s/pci_dev" % edd_dir)
> +    except OSError as e:
> +        log.info ("edd: no pci_dev for biosdev %d" % biosdev)
> +        return (None, [])
> +    # get absolute and normalized path
> +    sysfs_path = os.path.normpath(os.path.join(edd_dir, dereferenced))
> +    if sysfs_path.startswith("/sys/"):
> +        sysfs_path = sysfs_path[4:]
> +    return (sysfs_path,
> +            [dev for dev in devices if dev.sysfsPath.startswith(sysfs_path)]
> +            )



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]