New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disk full / write error on exit destroys config file? :-\ #817

Closed
gwillen opened this Issue Jan 17, 2018 · 4 comments

Comments

Projects
None yet
3 participants
@gwillen
Copy link

gwillen commented Jan 17, 2018

I encountered a disk full error in irssi, and shut down the machine to expand the VM's disk. When the machine came back and I started irssi, I discovered that the config file had been truncated to 0 bytes. I assume irssi tried to write it on exit when the machine shut down, but failed in a nasty way.

@dequis

This comment has been minimized.

Copy link
Member

dequis commented Jan 17, 2018

So what would be ideal here is to use g_file_set_contents() which writes to a temp file, fsyncs, and renames to the dest file. It's as atomic as possible and has decent error handling. It also has fancy implementation details like preallocating space with fallocate() (linux only), avoiding unnecessary fsyncs in filesystems where the rename is guaranteed to be atomic (like btrfs) and handling portability stuff like windows failing to rename when the target already exists.

The problem is that the config writing code relies heavily on GIOChannel streaming-like writes, and g_file_set_contents expects the whole thing to be in-memory.

So we can either

  1. Imitate g_file_set_contents as an example of good practices and do more or less the same thing (but definitely without explicit btrfs support). The result is going to be worse in some ways but possibly acceptable (definitely better than the current straight open with truncation).

    I took a shot at this here: dequis@7ea8def - just decided to stop to write this comment. This is missing a fsync and needs testing in cygwin (i'm almost sure the windows stuff is not needed there)

  2. Change the GIOChannel in CONFIG_REC to a GString, and all the g_io_channel_write_chars to string appends, so the file contents are built in-memory and the actual write is as atomic as it gets.

    We don't have a way to predict the total file size so that will result in a bunch of reallocs, which may be harmless but I'm tempted to prematurely optimize by preallocating 16kb (slightly above the 11kb of the default config), and since GString uses powers of two to pick allocation sizes, it just takes three reallocs to get to a size that fits my 89kb config file (massive, IMO - nine years worth of autosaved garbage)

@dequis dequis added the bug label Jan 17, 2018

@gwillen

This comment has been minimized.

Copy link

gwillen commented Jan 18, 2018

Well just to start with, let me say that I greatly appreciate you looking at this so quickly -- I didn't realize irssi development was still so active.

From an engineering perspective, approach 2 sounds nicest to me, given two conditions that I'm not sure whether they actually hold in practice:

(1) glib is well-maintained, g_set_file_contents is generally believed to be bug-free, and the glib maintainers are responsive to bug reports
(2) Config files are never so huge as to make building them in memory a bad idea. My own ridiculous config is only 85kB, which is also many years of autosaved garbage. But perhaps there are plugins or something that store excessive amounts of stuff in the config file? I can imagine such a thing could be broken by approach 2.

@ailin-nemui

This comment has been minimized.

Copy link
Contributor

ailin-nemui commented Jan 18, 2018

would something silly&simple like write to config.tmp and rename work, maybe? Edit: is that what you implemented there?

@dequis

This comment has been minimized.

Copy link
Member

dequis commented Jan 18, 2018

would something silly&simple like write to config.tmp and rename work, maybe? Edit: is that what you implemented there?

Yeah, that's what that commit is. It still needs fsync because according to comments in g_file_set_contents there's a risk of losing both files otherwise. But at that point I started wondering if duplicating what g_file_set_contents already did was worth it, and the comment above happened.

edit: lol copied the typoed version of the function from above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment