[bioshd] Implement park for ancient MFM/RLL disks #1977
Conversation
|
Hello @vkoskiv, Thanks for the PR! Your code looks well-written; I'll comment separately on various kernel- and ELKS-related coding issues next to the source lines for better clarity.
In general, yes, this is probably much better than calling specialized driver routines from other areas of the kernel. This is likely why the initial build is failing, as we build for IBM PC, PC-98 and embedded 8018x systems, each of which may use different configurations (or none) of he BIOS disk driver.
The comment from 2020 suggests Agreed that there should be a way to unmount all drives, and then (optionally?) park the drives in the normal case. This could be done by enhancing the
If the "park" flag is set for unmount, the remount of root as read-only could be skipped.
Perhaps that should join this PR, but its certainly OK to keep them separate. We can use the Thank you! |
elks/kernel/sys.c
Outdated
| @@ -24,6 +24,8 @@ | |||
|
|
|||
| static int C_A_D = 1; | |||
|
|
|||
| void bioshd_park_all(void); | |||
There was a problem hiding this comment.
No forward declarations are allowed in .c files, all would need to go somewhere in the include/linuxmt or include/arch header files. In this case, the function will likely live as a static function within the BIOSHD driver.
The reason for the build failing is that the code below is calling a specific block driver function which isn't part of the configured build. We'll be better off passing a flag from a user mode application that can find its way to the standard block device interface functions, which will eliminate this problem.
|
As I look through the kernel source a bit more, I don't yet see any easy way of passing a flag to the block driver release routine, and some thought needs to go into whether to change the parms of the |
|
Another way this might be done would be to move the sync, unmount (all), sleep, and park to within the kernel sys_reboot function (calling park like you have designed it, actually). The user programs currently trying to do nearly the same thing include Some advantages of using sys_reboot are 1) we don't build up specialized flag-passing in the kernel for what will be an unused function on almost all systems, 2) the poweroff, reboot, and shutdown programs could be consolidated and save disk space for floppy systems, which is valuable, and 3) some of the current code could be reworked in the reboot function, as, for instance, there are no programs currently using the "change ctrl-alt-del function" flag of the reboot system call. Things would actually become simpler. I also don't think we need separate shutdown and poweroff programs. Parking the disk(s) only during poweroff/shutdown is probably better than during driver release, because its not certain that the disk should be parked every time its unmounted, as they may be mounted again. What do you think, is it bad to park disks too much, or should this only be done before poweroff? |
|
Thank you for the thorough review! I applied some of the suggested fixes, but I'm struggling with Beyond that issue, I think the approach you suggested in the last comment there makes sense to me. While parking more than necessary doesn't really cause harm beyond the expected mechanical wear the drive encounters in normal use, it also doesn't serve much of a purpose. We only really care about parking when the disk is actually spinning down with the power removed, as the heads crash on the disk surface by design. I doubt any hardware configuration this old would have hot-swappable drives where parking when unmounting would be relevant, so just doing it right before shutting down makes the most sense to me. Amusingly, Github doesn't support standard git patches as attachments, so here's the patch on pastebin instead: https://pastebin.com/y6ix8x5i |
That's very strange, but I happen to be in the middle of a somewhat nasty bug hunt for a suspected divide-by-zero problem over here. I don't yet have a full answer, but I think our new divide-by-zero fault handler works OK. I'll keep you posted on that one.
It might be easier just to do another Looking at your change, I don't know why its happening. I am thinking that rather than declaring another
Yes, using any but a very few locals in any function is calling for a stack overflow, long story short the kernel can use upwards of 500+ bytes stack doing disk I/O by the time the fullest stack depth of functions is executed. The dots on boot are sector reads of the kernel /linux image by the boot sector, and the 'F' means a failed read for /bootopts on FAT, which is normal for MINIX images. The reason you don't see much else I think is very tricky - also being investigated in the above bug hunt link - it appears that QEMU runs serial and possibly console I/O asynchronously from the code being executed and a hard crash sometimes cuts output short. However, in this case it could just be that for some reason the kernel Image (/linux) is corrupted. I have seen this occur when the kernel is not fully rebuilt. Use |
Very interesting analysis on that issue! Though I'm doing all my testing on my physical 5150, so it's unlikely that the issue is caused by a QEMU bug, at least in my case.
I forgot to note that the dots and |
I moved the parking implementation to |
|
Also renamed to |
I must say, that's absolutely strange. It might be worth diving in to see why two separate static declarations of the same variable (if that's even the real issue) caused this to break, but I'm glad to hear it is now working. I'll take a note of the previous issue for another day. Extern declarations aren't allowed in .c files, the Given all the complexity discussed as well as the strange unsolved problem with the multiple static declarations, it might be better to add park capability enhancements in multiple PRs. I'm inclined to go with your original thoughts of parking the drives only in the poweroff/shutdown portions of sys_reboot(). The direct call to bios_disk_park from there can be Let me know when you're closer to finalizing this portion, and I'll turn back on the CI workflow for you. Thank you! |
|
Addressed the forward declare in |
|
I'll do those changes you just mentioned too, and then ping you :] |
Yes, nice catch. Mark it BFPROC. The reason is that we're very nearly completely out of .text space so groups of related functions are possibly placed in the single far section based on the configuration. Call speed is rarely an issue for disk-related functions, but FYI calling far from near is quick, but calling near from far requires thunking by the compiler to ensure the |
Moving it there would require us to include
Agreed, having parking logic in multiple places isn't necessary, and adds unneeded complexity. |
No, we're trying to stay away from the complicatedness of having includes within includes. I'm not right looking at it now, perhaps the struct and extern definition should go in genhd.h instead then. If that can work and not be messy, including genhd.h in bios.c if it isn't already is fine. In summary, its no problem to put the externs in the header file where they are declared, I just kind of jumped at the idea of having it in biosparms.h without much looking. Sorry this is all more complicated than it should be - but we're getting there! |
|
Removing parking from New finding: marking
I've really enjoyed the thorough feedback and discussion, working on this PR has been quite enjoyable :] |
|
Moving the extern |
|
One more note, earlier I forgot to call |
vkoskiv
force-pushed
the
bioshd-park
branch
2 times, most recently
from
e8ffddb
to
ad46cd7
Compare
|
Gated this thing behind |
Wow, another strange finding. Believe it or not, we don't usually have too much of that anymore. (Except for my current ongoing bug hunt in Mellvik/TLVC#76 which interestingly sometimes exhibits this behavior). It appears that when the CS segment gets mangled, this kind of thing happens on ELKS. However, seeing new Divide by Zero faults could just be that we're now seeing what's actually happening, given that fault handler was very recently added to ELKS.
That's ok for now, but make entirely sure that the reason wasn't that a forward declaration was not BFPROC and the other was, or vice versa. This is actually the reason why forward decls aren't allowed in .c files - too easy to have this error and not reported by the compiler. Needless to say, there should be no warnings produced during kernel compilation. |
I changed them both back and forth a few times when testing. That is, I added/removed I left |
|
As for testing, I've only tested this on my 5150 system so far, and it is working as expected. Any other systems using the |
Yes, I was thinking of just that and what how that might affect other users with unnecessary head movements. It might be worth some research... is there a min or maximum disk size typical of these very old disks? Perhaps only do it for very small hard drives? It's probably not worth writing a bunch of code to query the disk interface, unless it would be simple. Unfortunately, all our new kernel features have to sit core-resident even when not used. After commit, I plan on testing playing with the BFPROC issue on QEMU or another emulator. |
|
I tried to find out if there is a commonly accepted way to detect this, but nothing really came up. As far as I know, all IDE drives autopark, as well as a few of the later MFM/RLL drives. |
Really old drives, such as the two ST-412s in my IBM 5161, required manual parking to ensure the r/w heads are over a safe location when the disk spins down and the heads (intentionally!) crash onto the disk surface. In DOS land, this vital task was left up to the user, in the form of a PARK command to be run before shutting down. In ELKS however, the kernel should manage this automatically. An INT 13/AH=0Ch is invoked to seek the disk to the last track when unmounted, and to all drives right before halting the system.
|
Thank you @vkoskiv! |
|
Is this park also called from PC-98 shutdown? Reboot, Thank you. |
Currently, yes if PC-98 defines CONFIG_DEV_BIOS_BHD, which I think it does. This will mean the defines in sys.c will need to change to: I will make this change, as otherwise the system may crash on reboot/shutdown/poweroff on PC-98.
I plan on a simplification/rewrite of the reboot() system call code, and will take this into account, allowing for you to submit a PC98-compatble way of handling reboot. Thank you! |
|
Hello @ghaerr Does the shutdown for PC-98 still call the bios_disk_park_all? Thank you. |
|
(Or it may be calling int1B with wrong paramaters by call_bios.) |
Yes, the disk park function is called during the sys_reboot syscall for all platforms that have the BIOS fd/hd driver configured. This performs an INT 13h function 0Ch disk seek function using Is there a BIOS track seek function for PC-98, or should the entire function be commented out on PC-98? |
Yes - it is calling INT 1Bh function 0Ch for PC-98, since it is using call_bios. |
This patch still requires work before merging IMO, but I wanted to file this PR now to gather some feedback.
I'm running two ST-412 hard disks on my IBM 5150+5161 system so I don't ever have to worry about storage again. I noticed that there doesn't seem to be any support in ELKS for parking hard disks of this vintage, so I took a stab at adding it.
There was some discussion about this in 2020, but I assume most people playing around with ELKS aren't running disks this old :]
It seems to me that the correct way to do this is to park the drive in
bioshd_release().This does work for unmounting non-root disks, but there is a special case for the root drive, where the drive is remounted as read-only instead, and
bioshd_releasenever gets called.So my hack to ensure all drives are parked when shutting down is to just park every drive in
sys_reboot(), which feels very hacky.I'd welcome any advice on a better approach, and any nits/comments on my implementation, as I'm not too familiar with writing kernel code.
Tangentially related, I observed that the userland
rebootandshutdowncommands don't seem to unmount all mounted volumes. I have a patch that makes them iterate all mounted volumes to unmount them before a shutdown/reboot, but I'll leave that for another PR, and pending more discussion. Shouldn't the kernel sync/unmount mounted filesystems before shutting down? Or is that generally left to the userspace to handle?