Skip to content
This repository has been archived by the owner. It is now read-only.

Enable swap file support (#574) #585

Merged
merged 3 commits into from Sep 20, 2017
Merged

Enable swap file support (#574) #585

merged 3 commits into from Sep 20, 2017

Conversation

@craftyguy
Copy link
Member

@craftyguy craftyguy commented Sep 15, 2017

This implements swap file support (as discussed in #574), and also removes swap partition settings from the N900.

New deviceinfo variable deviceinfo_swap_size_recommended can be used to specify a swap size for specific devices.

/etc/conf.d/swapfile contains two variables:

  • swap_size which will override deviceinfo_swap_size_recommended
  • swap_file which defaults to /swapfile

If the config file doesn't exist, then the service defaults looks to deviceinfo_swap_size_recommended for size. If that is unset, then the service will not set up any swap file.

PR configures the swapfile service to start on bootup.

If a user wants to reconfigure the swap size while pmos is booted, they just have to edit the config file and to sudo rc-service swapfile restart to have it applied.

Also, as discussed previously, "encrypted swap" comes for "free" since this file exists on the rootfs, which would presumably be encrypted (assuming no --no-fde at install time)

Please test!

Fixes #574

@craftyguy craftyguy self-assigned this Sep 15, 2017
@craftyguy craftyguy requested a review from ollieparanoid Sep 15, 2017
Copy link
Member

@ollieparanoid ollieparanoid left a comment

Here is my first iteration of reviewing it, I have not tested it and usually there are more things that stick out once I do that. Thanks for making this PR! 🎉

fallocate -l ${ss}M ${sf}

#format swap file
mkswap ${swap_file} 1>/dev/null

This comment has been minimized.

@ollieparanoid

ollieparanoid Sep 15, 2017
Member

I guess you meant to use sf there.

I am personally in favor of descriptive names btw, if you agree you could call the variable _swap_file instead (so it doesn't clash with the global variable name).

# #
# postmarketos-base is distributed in the hope that it will be useful, #
# but WITHOUT ANY WARRANTY; without even the implied warranty of #
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the #

This comment has been minimized.

@ollieparanoid

ollieparanoid Sep 15, 2017
Member

That hash looks out of line there. Maybe remove the hashes on the right side altogether?

source /etc/conf.d/swapfile

# Configure swap size. Size is in MB
swap_size="${swap_size:-${deviceinfo_swap_size}}"

This comment has been minimized.

@ollieparanoid

ollieparanoid Sep 15, 2017
Member

How about naming the variable deviceinfo_swap_size_recommended?

swap_size="${swap_size:-${deviceinfo_swap_size}}"
if [ -z "${swap_size}" ]; then
# Set safe default if nothing set swap_size
swap_size=768

This comment has been minimized.

@ollieparanoid

ollieparanoid Sep 15, 2017
Member

How about defaulting to 0 instead (and adding extra code to not create a swap file in that case, I think that wouldn't work right now). Reason is, that newer devices usually come with enough RAM, so SWAP isn't even necessary.

@@ -0,0 +1,3 @@
swap_file=/swapfile
# Size is in MB
swap_size=1024

This comment has been minimized.

@ollieparanoid

ollieparanoid Sep 15, 2017
Member

Again, I would default to 0 here.

@craftyguy craftyguy force-pushed the feature/574_swap_file branch 2 times, most recently from bf3294e to e653472 Sep 16, 2017
@craftyguy
Copy link
Member Author

@craftyguy craftyguy commented Sep 16, 2017

@ollieparanoid

I incorporated your changes and then added some logic to handle a 'bug' I identified.. So now the script will look at swap_size in the config file, if it is unset, then it looks at the deviceinfo_swap_size_recommended variable. It then writes the value of this deviceinfo variable to the config file (if deviceinfo var is not set, then it writes 0 to config).

The reason I did this is because it allows the value in deviceinfo_swap_size_recommended to actually be used by default. The user can then override this by editing the config file and then whatever is in deviceinfo_swap_size_recommended would have no effect on the swap size at that point (including device package updates, those would not override user-specific configuration).

@craftyguy craftyguy force-pushed the feature/574_swap_file branch 2 times, most recently from f418f77 to 3e34600 Sep 17, 2017
Copy link
Member

@ollieparanoid ollieparanoid left a comment

Thanks for making the changes! I've tested it now, and added new comments.

@@ -0,0 +1,3 @@
swap_file=/swapfile
# Size is in MB
swap_size=

This comment has been minimized.

@ollieparanoid

ollieparanoid Sep 18, 2017
Member

Surprisingly, OpenRC changes that into swap_size=0 as soon as it runs Caching service dependencies... (-> as soon as you do anything with it). Reported upstream: OpenRC/openrc#165

A workaround is changing that line into # swap_size=1024 for example, that does not get replaced.
(I've also tried swap_size="", which also gets replaced.)

(When swap_size=0, the recommended value from the deviceinfo does not get used - so currently it never gets used.)

if [ -f ${swap_file} ]; then
swapoff ${swap_file} 2>/dev/null
# Get current swap file size
cur_size=$(du -h /swap |awk '{split($0, s, ".");print s[1]};')

This comment has been minimized.

@ollieparanoid

ollieparanoid Sep 18, 2017
Member

That is error-prone, I've often got such lines:

localhost:/etc$ sudo service swapfile stop
 * Deactivating swap file ... [ ok ]
localhost:/etc$ sudo service swapfile start
 * Activating swap file ...
Setting up swap file of size 99MB at /swapfile...
du: /swap: No such file or directory
Unable to remove old swap file! [ ok ]
localhost:/etc$

I recommend always deleting the file and recreating it, it happens instantly with fallocate anyway.

make_swap ${swap_file} ${swap_size}
fi
else
# No previous file, so make a new one

This comment has been minimized.

@ollieparanoid

ollieparanoid Sep 18, 2017
Member

(Indentation)


# Set correct file permissions for swap file
chown root:root ${swap_file}
chmod 600 ${swap_file}

This comment has been minimized.

@ollieparanoid

ollieparanoid Sep 18, 2017
Member

Please move the chown and chmod lines into make_swap, before executing mkswap.
Otherwise we get warnings like the following:

localhost:/etc$ sudo service swapfile start
 * Activating swap file ...
Setting up swap file of size 99MB at /swapfile...
mkswap: /swapfile: insecure permissions 0644, 0600 suggested.
Successfully activated swap file! [ ok ]
@craftyguy craftyguy force-pushed the feature/574_swap_file branch 7 times, most recently from af2962a to d34d035 Sep 18, 2017
@craftyguy
Copy link
Member Author

@craftyguy craftyguy commented Sep 19, 2017

@ollieparanoid
Copy link
Member

@ollieparanoid ollieparanoid commented Sep 19, 2017

Yes it does, good catch! I'll fix it.

@ollieparanoid
Copy link
Member

@ollieparanoid ollieparanoid commented Sep 19, 2017

Wait a second, could it be that the magical overwrite comes from the /sbin/swapfile? TBH I did not realize that we write to the config file from there. That would make a lot more sense compared to having the bug in OpenRC. If you don't mind, could you check this?

@craftyguy
Copy link
Member Author

@craftyguy craftyguy commented Sep 19, 2017

Wait a second, could it be that the magical overwrite comes from the /sbin/swapfile?

I write the value from deviceinfo_swap_size_recommended to swap_size variable in the conf file here., but only if swap_size is unset (or missing) from the config file.. It (my script) shouldn't delete any existing values (e.g. if swap_size is set), or even replace the comment # swap_size= with a blank line..

craftyguy and others added 3 commits Sep 15, 2017
Turns out, OpenRC replaces the line, in case it is set to
`#swap_size=1024` (which would be consistent with other conf.d
files), but not if it is set to `# swap_size=1024`.

See also: OpenRC/openrc#165
Per discussion in chat, this is not necessary and may cause unintended consequences with openrc.
@craftyguy craftyguy force-pushed the feature/574_swap_file branch from 19c911a to 4306bbd Sep 20, 2017
Copy link
Member

@ollieparanoid ollieparanoid left a comment

Tested and working 👍

@ollieparanoid ollieparanoid merged commit 220cd7b into master Sep 20, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@ollieparanoid ollieparanoid deleted the feature/574_swap_file branch Sep 20, 2017
PureTryOut added a commit that referenced this pull request Feb 21, 2018
This adds a custom swap file service, which allows specifying a
recommended swap size in the deviceinfo file via
`deviceinfo_swap_size_recommended`. For the N900 this defaults
to 1024 MB now. As the swap file is created in the root partition,
we have encrypted swap now (unless encryption is disabled with
`--no-fde`).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.