[PATCH] Add tmpfs support (#918621)

Martin Kolman mkolman at redhat.com
Thu Oct 3 12:23:29 UTC 2013


On Wed, 2013-10-02 at 12:09 -0500, David Lehman wrote:
> 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?
No, that might still be a valid usecase - tmpfs uses swap if there is no
space left in system RAM. So even when a tmpfs mount is over-committed
to say 200% of system RAM (or there are multiple mounts set to 100%,
etc.), as long as there is enough swap to cover the case of all the
tmpfs mounts filling up, everything should work just fine. So I can
imagine people with a lot of RAM making multiple tmpf temporary data
partitions and throwing a big swap partition to back it all up.

I can see this working quite nicely - as long as enough RAM is free, you
have a really fast storage and once RAM runs out, your tmpfs mounts will
just allocate more space from the swap partition page pool, without the
need to allocate a fixed backing storage partition for every mount.

Of course, if there is no swap or if swap is too small or full and can't
cover for the tmpfs mount/mounts filling up, the system will pretty much
lock up.

I've tried what it does on a VM without swap with a tmpfs partition
bigger than the system RAM. I've started filling the tmpf partition and
once RAM run out, the system became pretty much unusable. It was not
even possible to run any utilities to correct the issue - top, mount,
umount, df, nothing.

So one possibility would be to get RAM & swap size and then check if
there are any tmpfs mounts that are bigger than RAM & swap combined. 
But even this is not bulletproof, because the RAM & swap can be
independently filled by normal usage and the system still can lock-up if
the tmpfs mounts start to fill-up and there are no more pages to
allocate from RAM or SWAP.

BTW, having multiple mounts that are individually smaller than RAM +
swap, but combined bigger than RAM + swap might still be a valid
usecase, as long as the user makes sure the mounts don't all fill-up at
once. A rather extreme situation though.

On the other hand, no tmpfs mounts are created by default and the user
needs to explicitly specify he wants to have a tmpfs mount with a given
size.
So I guess describing all the possible issues/shortfalls in the docs and
expecting the user user does an informed decision might be enough ?

> 
> > 
> > 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.
I've done some additional tests and it appears this would actually be
possible - the size= option used in fstab or when mounting a tmpfs mount
accepts percentages >100%. If the device has 1GB RAM and a tmpfs mount
uses size=200%, the size of the mount will be 2GB.

But I'm not really sure this would be the behavior expected by the user
when he specifies a fixed size for the mount. In sucha a case, he might
not expect the size of the mount to automatically expand/contract if RAM
is added/removed.

> 
> > > 
> > > > 
> > > > 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?
Oh! Yes, you are right, it is not needed. Also looks like even if it was
assigned before superclass constructors were run, all relevant size
related stuff has been overridden anyway. So I'll just remove it.

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