Re: git: e4ab361e5394 - main - fix poweroff regression from 9cdf326b4f by delaying shutdown_halt

From: Andriy Gapon <avg_at_FreeBSD.org>
Date: Tue, 06 Feb 2024 10:13:46 UTC
On 06/02/2024 11:41, Andriy Gapon wrote:
> The branch main has been updated by avg:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=e4ab361e53945a6c3e9d68c5e5ffc11de40a35f2
> 
> commit e4ab361e53945a6c3e9d68c5e5ffc11de40a35f2
> Author:     Andriy Gapon <avg@FreeBSD.org>
> AuthorDate: 2024-02-06 08:55:13 +0000
> Commit:     Andriy Gapon <avg@FreeBSD.org>
> CommitDate: 2024-02-06 08:55:13 +0000
> 
>      fix poweroff regression from 9cdf326b4f by delaying shutdown_halt
>      
>      The regression affected ACPI-based systems without EFI poweroff support
>      (including VMs).
>      
>      The key reason for the regression is that I overlooked that poweroff is
>      requested by RB_POWEROFF | RB_HALT combination of flags.  In my opinion,
>      that command is a bit bipolar, but since we've been doing that forever,
>      then so be it.  Because of that flag combination, the order of
>      shutdown_final handlers that check for either flag does matter.
>      
>      Some additional complexity comes from platform-specific shutdown_final
>      handlers that aim to handle multiple reboot options at once.  E.g.,
>      acpi_shutdown_final handles both poweroff and reboot / reset.  As
>      explained in 9cdf326b4f, such a handler must run after shutdown_panic to
>      give it a chance.  But as the change revealed, the handler must also run
>      before shutdown_halt, so that the system can actually power off before
>      entering the halt limbo.
>      
>      Previously, shutdown_panic and shutdown_halt had the same priority which
>      appears to be incompatible with handlers that can do both poweroff and
>      reset.

I want to add that having many handlers with priorities expressed like 
SHUTDOWN_PRI_LAST ± N while some of those handlers have implicit 
inter-dependencies (interactions, interference) also does not help to see a 
clear picture.

Perhaps it would be better to handle all (reasonable) RB flag combinations 
centrally in kern_reboot and then dispatch events like shutdown_reset, 
shutdown_poweroff, etc.  Handlers for those events would have a single and 
simple job of performing that one action (perhaps failing and letting another 
handler try).

Also, I would split reboot howto into command and flag portions, so that only 
one command can be specified at a time.  E.g., I would consider RB_AUTOBOOT 
("RB_REBOOT"), RB_POWEROFF, RB_HALT to be distinct commands.  Then, flags like 
RB_NOSYNC or RB_DUMP could be optional flags.

As an aside, some flags documented for reboot(2) do not seem to have much to do 
with reboot.  E.g., RB_DFLTROOT affects how a system boots up, but not how the 
system goes for a reboot.  Not surprisingly, that option is not handled by 
anything kicked off with reboot(2).
Maybe, it would make more sense if we had fast reboot support and the running 
kernel could instruct the next kernel directly.  But, it's still a bit weird 
that flags like RB_POWEROFF and RB_DFLTROOT belong in the same domain and can be 
set together.

-- 
Andriy Gapon