kernel nextboot: Implement ddb command (knextboot 5/5)

[cem]

Summary

The ddb variant of the command is pretty similar to ordinary
nextboot(8), but has more limitations.

• Kernel names must be C identifiers
• Variables and values must be C identifiers
• Only -D, -e, -k, -o implemented

Basic command usage included.

[kib]

I cannot review the patch as the code change, see D7382. But my overall impression is that you are trying to write something to a UFS volume (either superblock or inode block, I am not sure) from the DDB context. And I can tell you that this cannot work.

First, UFS cache a lot of metadata in memory, so direct modifications of data on disk could contradict that cached metadata. Either your changes get lost, or metadata become apparently corrupted from the kernel PoV.

Second, to write something, current thread often needs help from many other threads, like bufdaemon or softdep thread of bufdaemon, or syncer etc. Part of the actions of that daemons is to rewind backward or forward the on-disk content according to the cached data.

Neither of this is possible to do from ddb. It is both unprepared for that context, and too complex for it. IMO, the right architectural approach is provided by kexec, where specially crafted post-morted kernel might mount a partition writeable and do necessary work. Somewhere in the phabricator live series of patches which implement kexec.

[cem]

In D7384#153491, @kib wrote:

I cannot review the patch as the code change, see D7382. But my overall impression is that you are trying to write something to a UFS volume (either superblock or inode block, I am not sure) from the DDB context.

Inode data block(s).

And I can tell you that this cannot work.

Well, you're wrong there :-).

First, UFS cache a lot of metadata in memory, so direct modifications of data on disk could contradict that cached metadata. Either your changes get lost, or metadata become apparently corrupted from the kernel PoV.

I don't think that is a problem in this case. We are rewriting existing file blocks.

Second, to write something, current thread often needs help from many other threads, like bufdaemon or softdep thread of bufdaemon, or syncer etc. Part of the actions of that daemons is to rewind backward or forward the on-disk content according to the cached data.

Obviously these threads are not running in DDB. Instead, we use the same polled IO path as kernel core dump.

Neither of this is possible to do from ddb. It is both unprepared for that context, and too complex for it. IMO, the right architectural approach is provided by kexec, where specially crafted post-morted kernel might mount a partition writeable and do necessary work. Somewhere in the phabricator live series of patches which implement kexec.

Seems like overkill.

[kib]

In D7384#153510, @cem wrote:

In D7384#153491, @kib wrote:

I cannot review the patch as the code change, see D7382. But my overall impression is that you are trying to write something to a UFS volume (either superblock or inode block, I am not sure) from the DDB context.

Inode data block(s).

And I can tell you that this cannot work.

Well, you're wrong there :-).

This is because you did not tested this enough.

First, UFS cache a lot of metadata in memory, so direct modifications of data on disk could contradict that cached metadata. Either your changes get lost, or metadata become apparently corrupted from the kernel PoV.

I don't think that is a problem in this case. We are rewriting existing file blocks.

File blocks are cached in the page cache, or in buffer cache, and have the same problem of inconsistency with the direct disk writes.

[cem]

In D7384#153524, @kib wrote:

File blocks are cached in the page cache, or in buffer cache, and have the same problem of inconsistency with the direct disk writes.

Can you explain how that is actually a problem? At panic time, cached pages are already lost and are never going to be written. Additionally, this is only for the /boot/nextboot.conf file, which is unlikely to have cached dirty blocks — the nextboot(8) tool explicitly fsync(1)s the file after writing it.

I don't think there are any consistency issues with the patchset at present, but am happy to address any concrete problems.

Probably db_nextboot_cmd should gate on panic_str != NULL, but that is a pretty trivial change.

[kib]

In D7384#153525, @cem wrote:

In D7384#153524, @kib wrote:

File blocks are cached in the page cache, or in buffer cache, and have the same problem of inconsistency with the direct disk writes.

Can you explain how that is actually a problem? At panic time, cached pages are already lost and are never going to be written. Additionally, this is only for the /boot/nextboot.conf file, which is unlikely to have cached dirty blocks — the nextboot(8) tool explicitly fsync(1)s the file after writing it.

I don't think there are any consistency issues with the patchset at present, but am happy to address any concrete problems.

Probably db_nextboot_cmd should gate on panic_str != NULL, but that is a pretty trivial change.

ddb can be entered during normal system operation, and continued after. This is a situation which you probably want to avoid with the panic check ?

After your note I realized that I do not understand the intended use case for the proposed command. If you have access to the console to command ddb, you also have access to the console to manage loader, and nextboot(8) is not needed. Nextboot only purpose is to help in a situation when you have no console access, and thus need to ensure that regardless of that, machine would switch to other kernel on next boot. If you can access the console, much more convenient and less dangerous methods of controlling the boot process are available.

[cem]

In D7384#153659, @kib wrote:

In D7384#153525, @cem wrote:

Probably db_nextboot_cmd should gate on panic_str != NULL, but that is a pretty trivial change.

ddb can be entered during normal system operation, and continued after. This is a situation which you probably want to avoid with the panic check ?

Right.

After your note I realized that I do not understand the intended use case for the proposed command. If you have access to the console to command ddb, you also have access to the console to manage loader, and nextboot(8) is not needed. Nextboot only purpose is to help in a situation when you have no console access, and thus need to ensure that regardless of that, machine would switch to other kernel on next boot. If you can access the console, much more convenient and less dangerous methods of controlling the boot process are available.

It is far more convenient to be able to specify in advance what kernel and options will be used than to interrupt and manipulate loader.

For example, some of the machines I have serial access to have seconds of latency (poor network infrastructure). Waiting several minutes for BIOS to start and run, followed by trying to interrupt autoboot at exactly the right time, is annoying and error-prone. Anyway, I regularly use nextboot(8) on such machines even though I have console access.

[cem]

Move unrelated changes to previous patch (#2).