Re: grub-mkconfig does not fsync() or sync() - can result in boot failure

[Daniel Kiper]

On Tue, Apr 24, 2018 at 08:48:05PM -0600, Chris Murphy wrote:
> Hi,
>
> I've done a 'UB's docs then it would be perfect. -ff -o stracegrub.out 
> grub-mkconfig' and there is no
> fsync() or sync() at all, so this does not seem crash safe for either
> non-journaled or journaled file systems.
>
> The most typical result is the grub.cfg is stale, the grub.cfg.new is
> truncated. This isn't in a state a lot of users will even understand, it's
> going to be a silent failure by booting an older kernel. And many distros
> hide the menu by default so a user would not know unless they go looking
> for a problem.
>
> Note that fsync() is not adequate for journaled file systems, changes might
> only be in the journal which GRUB does not read, so it can still see stale
> data until kernel code does log replay and fixes up the file system for
> GRUB to read at the next reboot.
>
> And there's a further adorable aspect of XFS where sync() behaves the same
> way, only log replay makes the file system consistent. ext4 and Btrfs
> appear to flush everything on sync().
>
> XFS devs have insisted this is correct behavior, that the file system
> metadata is all on disk, and it's not their problem GRUB can't do log
> replay, if GRUB requires the log flushed, then it needs to call
> FIFREEZE/FITHAW. No joke.
>
> Hence I filed this bug a few months ago.
> https://savannah.gnu.org/bugs/index.php?52657
>
> Anyway, in my limited testing, just adding sync on the 3rd to last line of
> grub-mkconfig, right above
>
> gettext "done" >&2
>
> fixes this problem on all file systems except XFS, which is a lot better
> than nothing. I really don't think it's ok for grub-mkconfig to assume
> something else is going to properly sync, unmount, or remount-ro whatever
> file system the grub.cfg is being written to. grub-mkconfig needs to take
> responsibility for its own action, and ensure complete and full commit of
> its changes before exit.

May I ask you to post a patch here? If you could add this nice description
into GRUB's docs then it would be perfect.

Daniel