- User Since
- Jul 9 2015, 9:56 PM (178 w, 4 d)
Netflix doesn't use efi_system_reset() in our version of FreeBSD. Changing the signature is likely fine.
Put 'local' variables on the same line; just forgot to do this last revision.
I'm not familiar with this tool or the test anymore, and not interested in studying it again now. I'm sure your fix is fine, but I am not a good reviewer.
Sat, Dec 8
- comment the full behavior of 'local -' rather than the subset used in this function, per dteske
- Use the slightly more explicit 'set -o noglob', in preference to 'set -f', per jilles
- De-indent cases, to match existing style (per dteske and jilles)
- Drop case body statements to independent lines -- unlike in C, there is a lot going on in the 'case' itself
I didn't know we built a rescue savecore!
Fri, Dec 7
- set -f
Generally LGTM. Thanks for doing this.
Thu, Dec 6
LGTM. I still find the 'mirror' name confusing, but maybe that's a personal problem ;-).
Mostly LGTM, thanks.
- Remove extraneous sleeps
- Check that generation ids compare in expectated order
- Add additional tests that mirror components are evicted during STARTING, prior to 'gmirror status', via geom reference counts.
- Use gmirror label -h and insert -h instead of gnop -o 512
Fix brain-o in cleanup leading to tty-read stall / kyua failure.
Add a basic test case showing we evict stale mirror components (on the basis of
Wed, Dec 5
- gmirrors -> mirrors in sysctl description
- drop g_mirror_ctl.c change for now
Move metadata kickout loop above quorum check.
Tue, Dec 4
Mon, Dec 3
This isn't the leak, and radeon crashed on me too many times for non-memleak reasons. Now happily using the nvidia binary driver with the 610 GT I had lying around :-).
Some initial comments. I'm not necessarily a good reviewer to wait on for this, sorry. I'm unfamiliar with TPM and don't have time to study it now.
Sun, Dec 2
The new changes LGTM. The scope of this review is getting broader than I'd prefer, though. It'd be easier on me as a reviewer if any future scope increases went into a new review. (I understand that's more work for you as long as this portion is in flux, but maybe that's motivation for getting this batch committed first? :-))
Sat, Dec 1
Mostly looks good to me. A few suggestions but nothing show-stopping other than the "Unknown command" thing. Thanks!
Fri, Nov 30
Wed, Nov 28
Tue, Nov 27
- Restore stale component kick-out to g_mirror_device_update, but relocate it *prior* to determining whether or not we have quorum to RUN the mirrorset.
- Perform sanity check assertions re: accumulated sc_genid and sc_syncid at the same time.
- For now, continue to kill mismatched md_all components *of the quorum genid*. Newer genid md_all components will override mirrorset configuration at taste time.
- Skip stale mirrors during g_mirror_add_disk prior to attempting to refresh from them.
- Remove stale component kick-out from g_mirror_add_disk. As Markj@ points out, g_mirror_add_disk synchronously invokes g_mirror_disk_update and g_mirror_device_update via g_mirror_event_send() in the middle of g_mirror_add_disk. So we might as well centralize back in g_mirror_device_update.
- Simplify early exit logic in g_mirror_refresh_device(), per markj@.
Mon, Nov 26
- Fix indentation
- Common subroutine for softc field initialization from md
Mark might be on to something. I went back and looked at my syslog from before the system totally hung and guess what? After a bunch of swap_pager_getswapspace spam (seriously, 62567/69256 log lines between Nov 19 and 24 are this message), and buried a little (it's not in the bottom 100 lines or so prior to the shutdown due to more swap_pager lines), we see:
Sun, Nov 25
Sat, Nov 24
Fri, Nov 23
Thu, Nov 22
Wed, Nov 21
Tue, Nov 20
Works for me, thanks. Could we use a similar trick for userspace asserts?
Mon, Nov 19
LGTM modulo nits and assuming that test case will be committed shortly.
Sun, Nov 18
I like the change. All of my comments describe existing issues in the code, nothing you introduced in this change. Maybe they can be addressed separately.
Looks great to me, thanks!
Sat, Nov 17
I'm not a fan of the general concept. As far as execution goes, I don't believe stdout from rc scripts is logged? So someone would have to be watching boot to notice this? If we must do this, put it in a log file.
Thu, Nov 15
The code and documentation look good; I'm just not sure if we want a new API for this, or if changing taskqueue_drain_all()'s behavior to match this would be unobjectionable.