forget less NVMe state across migration#1156
Conversation
add an additional test for propolis-to-itself migration and run the smoke tests there. extend the smoke test to try making sure disks are at least kind of basically still functional.
iximeow
added
bug
| // we have a small conundrum: it's possible that the home | ||
| // directory we wrote into is a tmpfs anyway. so by succeeding | ||
| // this much, we may have really just tested memory is intact. |
There was a problem hiding this comment.
@hawkw i would like for you to look at this change just because i can imagine the eye roll and swear when you read this comment thanks in advance. it's so much more annoying than i thought it was.
|
hm. the failures are fascinating!
so.. the test detected a problem! also what in the world.
... I presumably should be ignoring the exit code of the grep, oops. |
|
I ran a quick test using the same setup I did previously and things have certainly improved, the migration actually finished now 😄 But I still ran into issues after the migration, but each time it was a different. The first time I ran a migration the disk seemed to be "stuck" for about 2min, but then I saw these kernel messages in the guest and the disk seemed to be fine: After the second test the disk seemed to be fine, but then I left the guest running while I was in a meeting and when I came it was unresponsive and I had several messages like these: In my last test, the disk started OK for read, but writes would hang and then I started to get: For reference, these are my test steps:
|
|
after staring at this some more: it looks like there's a condition where we can import a VM, have a submission enqueued, resume devices, but incorrectly decide the queue is empty. the guest doesn't submit another I/O because the one it's waiting for is outstanding, so it discovers the disk is stuck. then all the normal "your disk stopped" happens. this happens with and without doorbell buffers, I've been looking with them disabled for simplicity. as we saw earlier, migrating while the I/O is submitted but unprocessed does "unstick" it, but I don't quite follow how. where it's stuck, so... why does |
the comment included here gets at the problem, but I spotted this by attaching to every propolis probe and pouring over all the output, touching up each relevant probe a bit as-needed. the tell there is that when waking a worker collection, eventually if you have a device with two queues (such as in a VM with two vCPUs) you'll see one scroll by that has a notify_workers of 0x00ab, while the other queue has a notify_workers of 0x0055. this is clearly wrong: worker 0 has a fixed assignment, but both minders believe that worker 0 will handle their I/Os. the wrong queue wakes thread 0, which looks at its assigned queue and sees no I/O, idles, and we never discover I/O on the queue that we'd intended to wake a worker for. the problem here is that when a worker idles, it sets a bit in the minder's notify_workers, but then when the worker is reassigned we don't clear that bit from the notify_workers bitmask. we *should* just flush all the bits when reassigning queues, but there's not a handle to the queue collection to do that. so instead, when attempting to wake queues, accept we might see bits for workers we really shouldn't wake, and if that worker has a fixed assignment that is not our queue, do not count it towards waking the requested number of workers. this involves more locking in the wake path than i want to add, but is good enough to prove out that the thing works.
|
67c2524 looks like it covers the issue from here. clippy hates it, i think the change itself is not the right way to spell it, but it describes the issue i see and a fix in the interim until i figure out plumbing in a QueueCollection or .... something? |
|
I ran a few tests using a similar setup as my previous test and the migrated disks seem to work fine now 🎉 |
| continue; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
so, at this point we'll drop slot.state.lock(). we could reconfigure the worker at this point, so that slot.wake() just below will wake the worker which then goes and checks a different fixed queue anyway. an example here is if a guest has gone and enabled (or disabled) an I/O queue and moved assignments around.
this is an improvement only as long as queues are not being reassigned concurrently with I/O, and if there is I/O then we have the same issue of queues poking workers that have no hope of serving them.
I might want to revert this and land 1156 as a marginal improvement for now, and have an issue about the shape of this problem and notify_workers in the face of device reconfiguration...
| continue; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
so, at this point we'll drop slot.state.lock(). we could reconfigure the worker at this point, so that slot.wake() just below will wake the worker which then goes and checks a different fixed queue anyway. an example here is if a guest has gone and enabled (or disabled) an I/O queue and moved assignments around.
this is an improvement only as long as queues are not being reassigned concurrently with I/O, and if there is I/O then we have the same issue of queues poking workers that have no hope of serving them.
I might want to revert this and land 1156 as a marginal improvement for now, and have an issue about the shape of this problem and notify_workers in the face of device reconfiguration...
|
|
||
| fn update_assignment(&self, assign: &Assignment) { | ||
| /// Reconfigure this worker for a polling strategy matching the current | ||
| /// assignment. Returns if the worker has actually been modified; `false` if |
There was a problem hiding this comment.
this is a super ultra nitpick, but perhaps
| /// assignment. Returns if the worker has actually been modified; `false` if | |
| /// assignment. Returns whether the worker has actually been modified; `false` if |
since i feel "returns if" is misinterpretable as "does not return until"? even though it's probably obvious.
| if slot.update_assignment(&assign) { | ||
| any_updated = true; | ||
| } |
There was a problem hiding this comment.
could also be:
| if slot.update_assignment(&assign) { | |
| any_updated = true; | |
| } | |
| any_updated |= slot.update_assignment(&assign); |
but it's up to you whether this is better or worse
| let mut ctrl = self.state.lock().unwrap(); | ||
| ctrl.import(input, self)?; | ||
| // Now that the controller is imported, update our mirror of its | ||
| // enablement bit. |
There was a problem hiding this comment.
mmmmmmaybe worth explicitly calling out that this is inside the lock?
| // the guest is *probably* an Alpine. `/` is a figment of | ||
| // its imagination; a tmpfs with the actual disk somewhere | ||
| // under /media, like /media/nvme0n1. try looking there, and | ||
| // if that fails we'll need to improve the test, phd, or | ||
| // both. |
There was a problem hiding this comment.
lmao this rocks alpine is such a distro
| // ignore the status from `grep` because it returns 1 when the | ||
| // regex matches no line. we don't want to `check_err()` | ||
| // because it's useful for debugging to see what *did* match. |
There was a problem hiding this comment.
isn't there a grep flag that makes it not do that, or did i make that up?
3 participants
add an additional test for propolis-to-itself migration and run the smoke tests there. extend the smoke test to try making sure disks are at least kind of basically still functional.
this should fix #981, but I have to admit I couldn't really reproduce the second failure mode that I alluded to there or @lgfa29 saw last week. I think that returning incorrect submission queue heads could have an outcome where the guest OS thinks it's submitting I/Os in a way we wouldn't get to, but beyond the status quo obviously being wrong, I'm not sure I've actually tickled that condition much at all.
I mostly really want to extend #966 to actually drive I/Os now, because either of the improvements here should fail quickly through a fuzzer...