[PATCH] Add tmpfs support (#918621)

Martin Kolman mkolman at redhat.com
Wed Oct 2 14:33:03 UTC 2013


On Fri, 2013-09-27 at 09:12 -0500, David Lehman wrote:
> On Thu, 2013-09-26 at 18:56 +0200, Martin Kolman wrote:
> > Add support for creating tmpfs mounts, mounting them
> > and adding them to fstab.
> 
> This looks good overall. I have several comments/questions. It's worth
> noting that this is going to be the first nodev filesystem that anaconda
> supports direct configuration of. That will present some challenges --
> particularly in the custom spoke.
Yeah, that might indeed by an issue - for example partition size.
Theoretically, there is no limit the size of a tmps partition.
That could do nasty things with the displayed total space/available
space - I guess it probably should not be counted into either ?

Also setting the size of the tmpfs mount could be an issue, as it can be
done both as an absolute value and as a % of RAM (more about this
further down).
> 
> > 
> > Tmpfs is a filesystem that lives in the Kernel page cache
> > and stores data primarily in RAM and in swap once RAM runs out.
> > 
> > Data stored on tmpfs mounts will not survive a system
> > reboot/crash/shutdown.
> > 
> > About tmpfs mount size:
> > - if no size is specified, the size will be 50% by default
> 
> 50% of what?
Oh, that should be "50% of system RAM by default".

> 
> > - if size is specified, the mount will have this size
> > -> there is no limit on the size on tmpfs mounts
> > -> just note that once it fills RAM (and any swap),
> > the system will grind to a halt
> > - grow and maxsize are supported
> > -> just using grow without maxsize will set the size to 100% RAM
> > -> if maxsize is > RAM, the size will be 100% RAM
> > -> if maxize is < RAM, the size will be a corresponding percentage of RAM
> 
> When I read this the first time I thought you meant maxsize would be
> treated as a percentage -- not that you have to convert it to a
> percentage to specify for the mount options.
Well, yeah, this might be kinda confusing. :)
Ill try to summarize all the ways that can be used to set the size of a
tmpfs mount:

* not specifying the size - size will correspond to 50% of system RAM
Kickstart example:
part /tmp --fstype=tmpfs

* setting a fixed size in megabytes - the tmpfs mount will have a fixed
size corresponding to the given number of megabytes.
Kickstart example:
part /tmp --size=3000 --fstype=tmpfs

* setting directly setting the percentage with --fsoptions - the tmpf
mount will have a size in the % of system RAM given. 
Kickstart example:
part /tmp --fstype=tmpfs --fsoptions="size=25%"
-> this tmpfs mount will have a size of 25% of the system RAM

* using the --grow option without --maxsize - the tmpfs mount size will
correspond to 100% of system RAM
Kickstart example:
part /tmp --fstype=tmpfs --grow
BTW, the end result is the same as using:
part /tmp --fstype=tmpfs --fsoptions="size=100%"

* using the --grow option with --maxsize - it gets a bit more
complicated:
- if maxsize > system RAM - the size of the tmpfs mount will be 100% RAM
- if maxsize < system RAM - the size of the tmpfs mount will be <100%
RAM
- as long as --grow is used, the size is always specified in %
- also --grow always grows only up to 100% od system RAM, never above,
even if the number given by --maxsize is larger than system RAM
Kickstart example for a system with 1GB RAM
part /tmp --fstype=tmpfs --maxsize=3000 --grow
-> the size of the tmpfs mount will be 100% RAM
part /tmp --fstype=tmpfs --maxsize=512 --grow
-> the size of the tmpfs mount will be 50% RAM


This is how the size works, and it has some consequences:
* there are some "undefined" combinations for the kickstart command,
such as for example:
part /tmp --fstype=tmpfs --fsoptions="size=25%" --size=300
part /tmp --fstype=tmpfs --fsoptions="size=25%" --grow
part /tmp --fstype=tmpfs --fsoptions="size=25%" --grow --maxisze=300
part /tmp --fstype=tmpfs --size=300 --grow
part /tmp --fstype=tmpfs --size=300 --grow --maxsize=500
part /tmp --fstype=tmpfs --size=300 --grow --maxsize=500
-fsoptions="size=25%"

The question is, how to handle these combinations ? Just document
the correct usage and leave the behavior undefined or throw a kickstart
parsing error if such combination is found in the kickstart file (as has
been recently done for combining autopart and other partitioning
commands)?

* --size is the only option that can set the size of the tmpfs mount
to an amount larger than the size of the system RAM

* also the --size option is the only one it can set the size to an
absolute value, all the other options (and the 50% default) all work
with % sizes

* if the size is specified in percent and RAM is added/removed, the size
of the tmpfs mounts automatically grows/shrinks accordingly
> 
> > 
> > Signed-off-by: Martin Kolman <mkolman at redhat.com>
> > ---
> >  blivet/__init__.py     |   4 ++
> >  blivet/deviceaction.py |   7 ++-
> >  blivet/devices.py      |  24 ++++++++++
> >  blivet/formats/fs.py   | 125 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >  4 files changed, 157 insertions(+), 3 deletions(-)
> > 
> > diff --git a/blivet/__init__.py b/blivet/__init__.py
> > index d15a0d0..afd5a49 100644
> > --- a/blivet/__init__.py
> > +++ b/blivet/__init__.py
> > @@ -1192,6 +1192,10 @@ class Blivet(object):
> >          kwargs["subvol"] = True
> >          return self.newBTRFS(*args, **kwargs)
> >  
> > +    def newTmpFS(self, *args, **kwargs):
> > +        """ Return a new TmpFSDevice. """
> > +        return TmpFSDevice(*args, **kwargs)
> > +
> >      def createDevice(self, device):
> >          """ Schedule creation of a device.
> >  
> > diff --git a/blivet/deviceaction.py b/blivet/deviceaction.py
> > index 202a66c..7160cce 100644
> > --- a/blivet/deviceaction.py
> > +++ b/blivet/deviceaction.py
> > @@ -473,8 +473,11 @@ class ActionCreateFormat(DeviceAction):
> >              udev_settle()
> >              self.device.updateSysfsPath()
> >              info = udev_get_block_device(self.device.sysfsPath)
> > -            self.device.format.uuid = udev_device_get_uuid(info)
> > -            self.device.deviceLinks = udev_device_get_symlinks(info)
> > +            # only do this if the format has a device known to udev
> > +            # (the format might not have a normal device at all)
> > +            if info:
> > +                self.device.format.uuid = udev_device_get_uuid(info)
> > +                self.device.deviceLinks = udev_device_get_symlinks(info)
> >  
> >      def cancel(self):
> >          self.device.format = self.origFormat
> > diff --git a/blivet/devices.py b/blivet/devices.py
> > index 6650086..c278433 100644
> > --- a/blivet/devices.py
> > +++ b/blivet/devices.py
> > @@ -3760,6 +3760,30 @@ class NoDevice(StorageDevice):
> >          self._preDestroy()
> >  
> > 
> > +class TmpFSDevice(NoDevice):
> 
> You could safely set _type to "tmpfs" here so that .type is meaningful.
OK.
> 
> > +    def __init__(self, *args, **kwargs):
> > +        """Create a tmpfs device"""
> > +        format = kwargs.get('format')
> > +        NoDevice.__init__(self, format)
> > +        # the tmpfs device does not exist until mounted
> > +        self.exists = False
> > +        self._size = kwargs["size"]
> > +        self._targetSize = self._size
> > +
> > +    @property
> > +    def size(self):
> > +        if self._size is not None:
> > +            return self._size
> > +        elif self.format:
> > +            return self.format.size
> > +        else:
> > +            return None
> > +
> > +    @property
> > +    def fstabSpec(self):
> > +        return "tmpfs"
> > +
> > +
> >  class FileDevice(StorageDevice):
> >      """ A file on a filesystem.
> >  
> > diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
> > index 13896c9..f7e1da8 100644
> > --- a/blivet/formats/fs.py
> > +++ b/blivet/formats/fs.py
> > @@ -1406,7 +1406,7 @@ class NoDevFS(FS):
> >      def __init__(self, *args, **kwargs):
> >          FS.__init__(self, *args, **kwargs)
> >          self.exists = True
> > -        self.device = self.type
> > +        self.device = self._type
> >  
> >      def _setDevice(self, devspec):
> >          self._device = devspec
> > @@ -1451,6 +1451,129 @@ register_device_format(SysFS)
> >  
> >  class TmpFS(NoDevFS):
> >      _type = "tmpfs"
> > +    _supported = True
> > +    _resizable = True
> > +    # as tmpfs is part of the Linux kernel,
> > +    # I guess it is linux-native
> > +    _linuxNative = True
> > +    # empty tmpfs has zero overhead
> > +    _minSize = 0
> > +    _minInstanceSize = 0
> > +    # tmpfs really does not occupy any space by itself
> > +    _minSize = 0
> 
> You've got minSize in here twice :)
Oops. :)
> 
> > +    # in a sense, I guess tmpfs is formatable
> > +    # in the regard that the format is atuomatically created
> > +    # once mounted
> > +    _formattable = True
> > +
> > +    def __init__(self, *args, **kwargs):
> > +        NoDevFS.__init__(self, *args, **kwargs)
> > +        self.exists = True
> > +        self._device = "tmpfs"
> > +
> > +        # check if fixed filesystem size has been specified,
> > +        # if no size is specified, tmpfs will by default
> > +        # be limited to half of the system RAM
> > +        # (sizes of all tmpfs mounts are independent)
> > +        fsoptions = kwargs.get("mountopts")
> > +        grow = kwargs.get("grow")
> > +        maxsize = kwargs.get("maxsize")
> > +        size_option = ""
> > +        if fsoptions:
> > +            self._apendOptions(fsoptions)
> > +        if self._size:
> > +            self._apendOptions("size=%dm" % self._size)
> > +        elif grow:
> > +            # grow to 100% RAM
> > +            size_option = "size=100%%"
> > +            system_ram = util.total_memory()/1024  # kB to MB
> > +            fs_size = system_ram
> > +            if maxsize:
> > +                # check if maxsize is < RAM
> > +                if maxsize < system_ram:
> 
> Not necessary, but you could combine the two if statements above.
Oh, now I see it - good point.
> 
> > +                    # as maxsize is smaller than RAM, filesystem size = maxsize
> > +                    fs_size = maxsize
> > +                    # maxsize is less than size of RAM
> > +                    # -> convert to % of RAM
> > +                    size_fraction = maxsize/float(system_ram)
> > +                    # "size=%1.0f%%" % (0.5555*100) -> "size=56%"
> > +                    size_option = "size=%1.0f%%" % (size_fraction*100)
> > +            self._size = fs_size
> > +            kwargs["size"] = fs_size
> 
> What the above line for?

If the tmpfs --size kickstart option is not used, the "size" is never
set kwargs, which breaks stuff such as checking the size of the
filesystem. 
So in this case the % is converted to a number (based on the current
system RAM) and set in kwargs.
Now when I think about this, this should be also done in case no
explicit size is specified and the 50%-of-system-RAM default is used.
> 
> > +
> > +        if size_option:
> > +            self._apendOptions(size_option)
> > +
> > +    def mount(self, *args, **kwargs):
> > +        """The filesystem doesn't need to be separately mounted"""
> > +        pass
> > +
> > +    def create(self, *arg, **kwargs):
> > +        """A filesystem is created automatically once tmpfs is mounted"""
> > +        pass
> 
> So we don't do anything with tmpfs mounts except put them in /etc/fstab?
Yes, at least for the bugreport doing this should be enough.

> This may be fine when flags.installer_mode is True, but for blivet it
> will have to be possible to mount and unmount tmpfs as well. My advice
> would be to stub create and destroy and just let mount/umount do the
> work.
OK, I'll do that.
> 
> > +
> > +    @property
> > +    def mountable(self):
> > +        return True
> > +
> > +    def _getOptions(self):
> > +        if self._options:
> > +            return self._options
> > +        else:
> > +            return "defaults"
> > +
> > +    def _setOptions(self, options):
> > +        self._options = options
> > +
> > +    # override the options property
> > +    # so that the size and other options
> > +    # are correctly added to fstab
> > +    options = property(_getOptions, _setOptions)
> > +
> > +    @property
> > +    def size(self):
> > +        return self._size
> > +
> > +    @property
> > +    def minSize(self):
> > +        """ The minimum filesystem size in megabytes. """
> > +        return self._minInstanceSize
> > +
> > +    @property
> > +    def free(self):
> > +        free_space = 0
> > +        if self.mountpoint:
> 
> I think you want self._mountpoint throughout this method -- it holds the
> current/active mountpoint and provides an implicit status check since it
> is only set when the filesystem is actually mounted somewhere.
OK, good to know. :)
> 
> > +            # we need both the output and the return code
> > +            rc, buf = util._run_program(['df', '-P', self.mountpoint])
> > +            # XXXX, what about changeroot ?
> > +            if rc == 0:
> > +                lines = buf.readlines()
> > +                if len(lines) == 2:  # header and a single data row
> > +                    space_data = lines[0].split(" ")
> > +                    if len(space_data) >= 6:
> > +                        free_space = int(space_data[3])
> > +                        # convert to MB
> > +                        free_space = math.ceil(size / 1024.0)
> > +        return free_space
> > +
> > +    def _setDevice(self, devspec):
> > +        self._device = devspec
> > +
> > +    def _getDevice(self):
> > +        return self._device
> > +
> > +    device = property(lambda f: f._getDevice(),
> > +                      lambda f,d: f._setDevice(d),
> > +                      doc="Full path the device this format occupies")
> 
> Why not just use a plain attribute for self.device if the get/set are as
> above? It seems like we might want to use a read-only property here,
> actually.
Well, yeah, that makes sense - it would always be "tmpfs", for all tmpfs
mounts so a read-only property should do just fine.

> 
> > +
> > +    def _apendOptions(self, new_options):
> > +        """Append free form option string
> > +        to the options (used among others in fstab)
> > +        """
> > +        if self._options:  # append to existing options
> > +            self._options = "%s,%s" % (self._options, new_options)
> > +        else:  # not yet set, just assign it
> > +            self._options = new_options
> >  
> >  register_device_format(TmpFS)
> >  
> 
> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches




More information about the anaconda-patches mailing list