[PATCH] Add tmpfs support (#918621)

David Lehman dlehman at redhat.com
Wed Oct 2 17:09:43 UTC 2013


On Wed, 2013-10-02 at 16:33 +0200, Martin Kolman wrote:
> 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 ?

Tmpfs mountpoints should not have any effect on the total/free space in
custom. That makes me wonder, though -- do your patches handle the case
where the user tries to create multiple tmpfs mountpoints with a total
of >100% ? Also, surely specifying 100% of RAM for tmpfs size is madness
-- no?

> 
> 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).

I would consider making it so that for tmpfs the size is always treated
as a percentage.

> > 
> > > 
> > > 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.

My question is related more to the fact that nothing will look at
kwargs["size"] after you set it there (or am I missing something).
You've already run the superclass constructors, and you're at the end of
the TmpFS constructor, so who's going to see that size you're putting in
there?

> > 
> > > +
> > > +        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
> 
> 
> _______________________________________________
> 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