Supersedes PR#105.
First two patches are very minor.
"Make mountsCache..." and "Make default subvolspec" are substantially as before."
"Use context manager..." and "Cache mountinfo..." are new.
From: mulhern amulhern@redhat.com
Externally to the mountsCache it seems that sometimes the subvolspec is a str, sometimes an int. Make mountsCache store and retrieve treating subvolspec always as a str.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/mounts.py | 40 +++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 13 deletions(-)
diff --git a/blivet/mounts.py b/blivet/mounts.py index 617ca21..af45c7e 100644 --- a/blivet/mounts.py +++ b/blivet/mounts.py @@ -21,6 +21,7 @@ # from collections import defaultdict from . import util +from .devicelibs import btrfs
import logging log = logging.getLogger("blivet") @@ -40,7 +41,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 +51,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): @@ -61,6 +65,23 @@ def isMountpoint(self, path):
return any(path in p for p in self.mountpoints.values())
+ def _getSubvolSpec(self, devspec, mountpoint): + """ Get the subvolume specification for this btrfs volume. + + :param str devspec: the device specification + :param str mountpoint: the mountpoint + + :returns: the subvolume specification, 5 for a top-level volume + :rtype: str or NoneType + """ + for line in open("/proc/self/mountinfo").readlines(): + fields = line.split() + if fields[4] == mountpoint and fields[9] == devspec: + # empty _subvol[1:] means it is a top-level volume + subvolspec = fields[3] + return subvolspec[1:] or str(btrfs.MAIN_VOLUME_ID) + return None + def _getActiveMounts(self): """ Get information about mounted devices from /proc/mounts and /proc/self/mountinfo @@ -76,18 +97,11 @@ def _getActiveMounts(self): 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 - - self.mountpoints[(devspec, subvolspec)].append(mountpoint) - + subvolspec = self._getSubvolSpec(devspec, mountpoint) + if subvolspec is not None: + self.mountpoints[(devspec, subvolspec)].append(mountpoint) + else: + log.error("failed to obtain subvolspec for btrfs device %s", devspec) else: self.mountpoints[(devspec, None)].append(mountpoint)
I'm sorry, but I don't see the relation between the commit message and code changes here.
In getMountpoints subvolspec is converted to a str before lookup (unless it is None). Previously, there was no conversion.
subvolspec returned by _getSubvolSpec and used as a a key is always a str or None, previously the subvolspec used as part of the key could have been a str (_subvol[1:]) or an int(5).
I understand that, but still I think the commit message should be something like "put code to get subvol spec into a separate function" because that's what the commit primarly does.
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
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 | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/blivet/mounts.py b/blivet/mounts.py index af45c7e..d1a4eb6 100644 --- a/blivet/mounts.py +++ b/blivet/mounts.py @@ -89,21 +89,23 @@ 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": - subvolspec = self._getSubvolSpec(devspec, mountpoint) - if subvolspec is not None: - self.mountpoints[(devspec, subvolspec)].append(mountpoint) + + 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": + subvolspec = self._getSubvolSpec(devspec, mountpoint) + if subvolspec is not None: + self.mountpoints[(devspec, subvolspec)].append(mountpoint) + else: + log.error("failed to obtain subvolspec for btrfs device %s", devspec) else: - log.error("failed to obtain subvolspec for btrfs device %s", devspec) - else: - self.mountpoints[(devspec, None)].append(mountpoint) + self.mountpoints[(devspec, None)].append(mountpoint)
def _cacheCheck(self): """ Computes the MD5 hash on /proc/mounts and updates the cache on change
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
It is possible that /proc/self/mountinfo changes during execution of _getActiveMounts, but it is also possible that /proc/mounts changes, and we do not guard against that.
The motivation behind this is that it is expensive to parse on average half the file for some supplementary information about btrfs everytime it is needed.
The downside of this cacheing is that a data structure gets held in memory for the duration of _getActiveMounts execution, although this only happens if btrfs info is requested.
Also, there is a slight improvement in the way the slightly variable format data in mountinfo is parsed.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/mounts.py | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 61 insertions(+), 7 deletions(-)
diff --git a/blivet/mounts.py b/blivet/mounts.py index d1a4eb6..d4abeb5 100644 --- a/blivet/mounts.py +++ b/blivet/mounts.py @@ -26,6 +26,58 @@ 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 reset(self): + """ Resets the cache. """ + self._cache = None + + 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. @@ -34,6 +86,7 @@ class MountsCache(object): def __init__(self): self.mountsHash = 0 self.mountpoints = defaultdict(list) + self._mountinfo = _MountinfoCache()
def getMountpoints(self, devspec, subvolspec=None): """ Get mountpoints for selected device @@ -74,13 +127,11 @@ def _getSubvolSpec(self, devspec, mountpoint): :returns: the subvolume specification, 5 for a top-level volume :rtype: str or NoneType """ - for line in open("/proc/self/mountinfo").readlines(): - fields = line.split() - if fields[4] == mountpoint and fields[9] == devspec: - # empty _subvol[1:] means it is a top-level volume - subvolspec = fields[3] - return subvolspec[1:] or str(btrfs.MAIN_VOLUME_ID) - return None + root = self._mountinfo.getRoot(devspec, mountpoint) + if root is not None: + return root[1:] or str(btrfs.MAIN_VOLUME_ID) + else: + return None
def _getActiveMounts(self): """ Get information about mounted devices from /proc/mounts and @@ -89,6 +140,7 @@ def _getActiveMounts(self): Refreshes self.mountpoints with current mountpoint information """ self.mountpoints = defaultdict(list) + self._mountinfo.reset()
with open("/proc/mounts") as mounts: for line in mounts: @@ -107,6 +159,8 @@ def _getActiveMounts(self): else: self.mountpoints[(devspec, None)].append(mountpoint)
+ self._mountinfo.reset() + def _cacheCheck(self): """ Computes the MD5 hash on /proc/mounts and updates the cache on change """
In reply to line 163 of blivet/mounts.py:
I don't understand the point of this ``reset()`` at the very end. AFAICT, it needlessly throws out and repopulates the cache for nothing being able to use it before it all happens again.
Closed.
anaconda-patches@lists.fedorahosted.org