Send deactivate flush if need_flush is set
#1573
Conversation
integration_tests/src/lib.rs
Outdated
|
|
||
| // Send exactly IO_CACHED_MAX_JOBS to force a Barrier to be sent. The | ||
| // barrier empties out the active list, so deactivation may proceed. | ||
| for _ in 0..10000 { |
There was a problem hiding this comment.
nit: should this instead be IO_CACHED_MAX_JOBS + N
There was a problem hiding this comment.
IO_CACHED_MAX_JOBS isn't public in the crucible crate, because it's (mostly) an implementation detail. I'm ambivalent about making it public versus hard-coding it here, let me know if you have strong opinions.
There was a problem hiding this comment.
...and it must be an exact multiple of IO_CACHED_MAX_JOBS, because otherwise there would be leftover jobs in can_deactivate_immediately and deactivation wouldn't skip the final flush.
There was a problem hiding this comment.
if it must be exactly that value, I'd say make it public or have a way to return it (less excited about that option)
There was a problem hiding this comment.
alternatively, mimic what omicron does and use
#[cfg(any(test, feature = "testing"))]
to guard if it's pub or not?
There was a problem hiding this comment.
Okay, done in 7dc0be1
There's not a particularly clean way to enable / disable the pub visibility qualifier, so I added a dedicated module that re-exports the constant.
|
I've also been running this on the dublin racklett setup. I (accidentally) verified I could reproduce the problem, which is good. I'm now at 17 instances created and no issues. |
|
Overnight testing of 40 more instance starts, still good. |
Otherwise, deactivate can fail in a case where there are no active jobs but
need_flushis set: it will deactivate right away (because there are no active jobs), then the automatic flush will be sent to a Downstairs in an invalid state (DeactivatedorNew).This PR will hopefully fix #1571