Supersedes PR#107.
The final patch, "Cache data..." is the only interesting one.
The main change from previous PR is that since mountinfo cache is only needed in _getActiveMounts(), it is only instantiated there. In #107 is was an attribute of the mounts cache, which was excessive. This way, the cache needs to be managed less, which is good.
From: mulhern amulhern@redhat.com
Signed-off-by: mulhern amulhern@redhat.com --- blivet/mounts.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/blivet/mounts.py b/blivet/mounts.py index b781d50..617ca21 100644 --- a/blivet/mounts.py +++ b/blivet/mounts.py @@ -70,7 +70,7 @@ def _getActiveMounts(self): self.mountpoints = defaultdict(list) for line in open("/proc/mounts").readlines(): try: - (devspec, mountpoint, fstype, _options, _rest) = line.split(None, 4) + (devspec, mountpoint, fstype, _rest) = line.split(None, 3) except ValueError: log.error("failed to parse /proc/mounts line: %s", line) continue
From: mulhern amulhern@redhat.com
Signed-off-by: mulhern amulhern@redhat.com --- blivet/mounts.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/blivet/mounts.py b/blivet/mounts.py index d7d6851..b781d50 100644 --- a/blivet/mounts.py +++ b/blivet/mounts.py @@ -65,7 +65,7 @@ def _getActiveMounts(self): """ Get information about mounted devices from /proc/mounts and /proc/self/mountinfo
- Refreshes self.mountpoints with current moutpoint information + Refreshes self.mountpoints with current mountpoint information """ self.mountpoints = defaultdict(list) for line in open("/proc/mounts").readlines():
From: mulhern amulhern@redhat.com
Signed-off-by: mulhern amulhern@redhat.com --- blivet/formats/fs.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py index dd282eb..f9ec7ac 100644 --- a/blivet/formats/fs.py +++ b/blivet/formats/fs.py @@ -40,6 +40,7 @@ from ..i18n import _, N_ from .. import udev from ..mounts import mountsCache +from ..devicelibs import btrfs
import logging log = logging.getLogger("blivet") @@ -1157,7 +1158,7 @@ class BTRFS(FS): def __init__(self, **kwargs): super(BTRFS, self).__init__(**kwargs) self.volUUID = kwargs.pop("volUUID", None) - self.subvolspec = kwargs.pop("subvolspec", None) + self.subvolspec = kwargs.pop("subvolspec", btrfs.MAIN_VOLUME_ID)
def create(self, **kwargs): # filesystem creation is done in blockdev.btrfs.create_volume
From: mulhern amulhern@redhat.com
This avoids reading and parsing the file whenever additional btrfs information needs to be looked up.
Changes are: * Addition of a small, special purpose cache for btrfs information obtained from mountinfo. The only facility the cache offers is laziness. Since it currently is instantiated and destroyed in a single method where it is used in a very simple way it does not require other management facilities. * Slightly improved parsing of /proc/self/mountinfo which takes into account some variable number fields. * Make sure all components of key values are str or NoneType when inserted into mounts cache or when looked up. * Use context manager for "/proc/mounts" processing. * Log an error if a line in "/proc/mounts" is skipped because no subvolspec was obtained for the btrfs volume entry. * Use named constant MAIN_VOLUME_ID instead of 5 where appropriate.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/mounts.py | 93 ++++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 71 insertions(+), 22 deletions(-)
diff --git a/blivet/mounts.py b/blivet/mounts.py index 617ca21..a7763cf 100644 --- a/blivet/mounts.py +++ b/blivet/mounts.py @@ -21,10 +21,59 @@ # from collections import defaultdict from . import util +from .devicelibs import btrfs
import logging log = logging.getLogger("blivet")
+class _MountinfoCache(object): + """ Cache for info from /proc/self/mountinfo. Looks up the root of the + mount within the filesystem using a pair of mountpoint, mount + source as keys. + + This is a very simple helper class for MountsCache, and would + have to be altered for general purpose use. + + Note that only info for fstype btrfs is stored within the cache, + as MountsCache methods only require information for btrfs. + """ + + def __init__(self): + self._cache = None + + def _getCache(self): + """ Reads lines in /proc/self/mountinfo and builds a table. """ + cache = {} + + with open("/proc/self/mountinfo") as mountinfos: + for line in mountinfos: + fields = line.split() + separator_index = fields.index("-") + fstype = fields[separator_index + 1] + if not fstype.startswith("btrfs"): + continue + + root = fields[3] + mountpoint = fields[4] + devspec = fields[separator_index + 2] + cache[(devspec, mountpoint)] = root + + return cache + + def getRoot(self, devspec, mountpoint): + """ Retrieves the root of the mount within the filesystem + that corresponds to devspec and mountpoint. + + :param str devspec: device specification + :param str mountpoint: mountpoint + :rtype: str or NoneType + :returns: the root of the mount within the filesystem, if available + """ + if self._cache is None: + self._cache = self._getCache() + + return self._cache.get((devspec, mountpoint)) + class MountsCache(object): """ Cache object for system mountpoints; checks /proc/mounts and /proc/self/mountinfo for up-to-date information. @@ -40,7 +89,7 @@ def getMountpoints(self, devspec, subvolspec=None): :param devscpec: device specification, eg. "/dev/vda1" :type devspec: str :param subvolspec: btrfs subvolume specification, eg. ID or name - :type subvolspec: str + :type subvolspec: object (may be NoneType) :returns: list of mountpoints (path) :rtype: list of str or empty list
@@ -50,6 +99,9 @@ def getMountpoints(self, devspec, subvolspec=None): """ self._cacheCheck()
+ if subvolspec is not None: + subvolspec = str(subvolspec) + return self.mountpoints[(devspec, subvolspec)]
def isMountpoint(self, path): @@ -68,28 +120,25 @@ def _getActiveMounts(self): Refreshes self.mountpoints with current mountpoint information """ self.mountpoints = defaultdict(list) - for line in open("/proc/mounts").readlines(): - try: - (devspec, mountpoint, fstype, _rest) = line.split(None, 3) - except ValueError: - log.error("failed to parse /proc/mounts line: %s", line) - continue - - if fstype == "btrfs": - # get the subvol name from /proc/self/mountinfo - for line in open("/proc/self/mountinfo").readlines(): - fields = line.split() - _subvol = fields[3] - _mountpoint = fields[4] - _devspec = fields[9] - if _mountpoint == mountpoint and _devspec == devspec: - # empty _subvol[1:] means it is a top-level volume - subvolspec = _subvol[1:] or 5 - + mountinfo = _MountinfoCache() + + with open("/proc/mounts") as mounts: + for line in mounts: + try: + (devspec, mountpoint, fstype, _rest) = line.split(None, 3) + except ValueError: + log.error("failed to parse /proc/mounts line: %s", line) + continue + + if fstype == "btrfs": + root = mountinfo.getRoot(devspec, mountpoint) + if root is not None: + subvolspec = root[1:] or str(btrfs.MAIN_VOLUME_ID) self.mountpoints[(devspec, subvolspec)].append(mountpoint) - - else: - self.mountpoints[(devspec, None)].append(mountpoint) + else: + log.error("failed to obtain subvolspec for btrfs device %s at mountpoint %s", devspec, mountpoint) + else: + self.mountpoints[(devspec, None)].append(mountpoint)
def _cacheCheck(self): """ Computes the MD5 hash on /proc/mounts and updates the cache on change
What's the purpose of this? I don't think it's a good idea since member devices also have BTRFS formats and this will make it seem like they have valid subvolids, which is just confusing.
It's too bad it "fixed" the bug.
The problem is that during an install a subvolume won't be created unless its volume is already mounted. But mountsCache won't believe that the parent volume is mounted unless it is given the proper subvolspec. When doing a BTRFS install, it appears that the btrfs format for the parent is given no subvolspec so failure during subvolume creation occurs. Essentially, I think subvolspec is assigned correctly during discovery, but not during creation.
anaconda-patches@lists.fedorahosted.org
-
dashea