Fix filesystems not unmounted at shutdown #219
Conversation
|
Hi, @troglobit I mistakenly closed #209, I created this new MR, please help me review it. Paste the answers to your question here:
The original author Robert Andersson said he does not remember the exact reason, so I dropped it in V2, it might be some specific requirements from our system.
Have changed as you suggested in this MR.
Maybe I can explain this, this is basically for PREEMPT-RT kernel, finit during shutdown must run as a RT process to ensure it would not be scheduled out, so it must run at the highest priority 99. This part of change should also work on non-preempt-rt kernel.
I am not 100% sure about the answer, but we do deploy finit to NXP imx27 boards (maybe thousands of them), I have not seen error report about this change, so 2500 USECS should be long enough I guess. |
There was a problem hiding this comment.
OK, I think I understand a bit better now why you need this change. I'm still not overly happy with the change, but I cannot see a better way to solve this at the moment.
Please address my review comments, in particular the clock_nanosleep() issue, then I'll merge it. If you're quick it'll make it into the next release :)
src/util.c
Outdated
| deadline.tv_sec += deadline.tv_nsec / 1000000000; | ||
| deadline.tv_nsec = deadline.tv_nsec % 1000000000; | ||
|
|
||
| while (clock_nanosleep(CLOCK_MONOTONIC, TIMER_ABSTIME, &deadline, NULL) < 0 && errno == EINTR) |
There was a problem hiding this comment.
The clock_nanosleep() API seems to return positive error codes, never negative ones. Yes, I was also surprised.
There was a problem hiding this comment.
Arha, did not notice that, nice catch, will fix it in V2.
src/sig.c
Outdated
| struct sched_param sched_param; | ||
| sched_param.sched_priority = 99; |
There was a problem hiding this comment.
Nitpick, empty line after declarations, or sched_param = { .sched_priority = 99 };
src/sig.c
Outdated
| /* PID 1 run as RR process with highest priority */ | ||
| sched_setscheduler(1, SCHED_RR, &sched_param); |
There was a problem hiding this comment.
The comment is useless as-is. It states in text what the next line does ... which can be read from the code itself. Instead, use comments to explain why a code path is needed. Here's a good place to add the reasoning from the discussion. E.g., "On a PREEMPT-RT system, Finit must run as the highest prioritized RT process to ensure it completes the shutdown sequence."
There was a problem hiding this comment.
@troglobit Good point, will do that.
Instead of unconditionally waiting 2 seconds for processes to die, check continuously for remaining processes, and break the loop when none remain. Turn PID 1 to a RT process with highest priority 99 during shutdown, this ensures it would not be preempted by other RT processes. Signed-off-by: Robert Andersson <robert.m.andersson@atlascopco.com> Signed-off-by: Mathias Thore <mathias.thore@atlascopco.com> Signed-off-by: Ming Liu <liu.ming50@gmail.com>
liuming50
force-pushed
the
fix-filesystems-not-unmounted-at-shutdown
branch
from
38586ec to
3e0063e
Compare
|
@troglobit Please kindly help me review the V2. Have fixed all issues as you suggested in V1. |
Instead of unconditionally waiting 2 seconds for processes to die,
check continuously for remaining processes, and break the loop when
none remain.
Turn PID 1 to a RT process with highest priority 99 during shutdown,
this ensures it would not be preempted by other RT processes.
Signed-off-by: Robert Andersson robert.m.andersson@atlascopco.com
Signed-off-by: Mathias Thore mathias.thore@atlascopco.com
Signed-off-by: Ming Liu liu.ming50@gmail.com