Repository navigation
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...
| 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
There was a problem hiding this comment.
i have a very hard time expressing why i preferred to = true here but i think i've taken up some illumos style guide bits that really resonated with me and do not love to |= booleans? but i might be making up justifications..
| // 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?
There was a problem hiding this comment.
i don't think there is, but in retrospect i probably could have done this more reasonably with some kind of bash. i did find -q which is like the exact opposite where you get a status and no output, which i think is basically just as good here. it's a little funky because you get 1 if you matched no line, and 0 if you matched any, but 2a07c11#diff-2485d318d3cefa6c6126da3d19f664617d3d4f45a5cd753780bce8f3bbc307caR410 i think is a little better now!
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
if i was migrating an emulated NVMe device, i would simply not forget its state
| // Callers must hold the worker's lock while polling the next request for | ||
| // the selected device queue. This ensures that if the worker is to have its | ||
| // queue assignment changed, this is mutually exclusive with the worker | ||
| // indicating idleness to a QueueMinder. |
There was a problem hiding this comment.
TURBO NIT SORRY: "callers should" feels like it belongs in a Doc Comment even if it's a private function (saying this only because the next function in this diff has a Doc Comment
| // Callers must hold the worker's lock while polling the next request for | |
| // the selected device queue. This ensures that if the worker is to have its | |
| // queue assignment changed, this is mutually exclusive with the worker | |
| // indicating idleness to a QueueMinder. | |
| /// Callers must hold the worker's lock while polling the next request for | |
| /// the selected device queue. This ensures that if the worker is to have its | |
| /// queue assignment changed, this is mutually exclusive with the worker | |
| /// indicating idleness to a QueueMinder. |
| // NUM_WORKERS-many times. Queue reassignment is already terribly | ||
| // expensive what with having to kick all the workers. Please do not | ||
| // reconfigure device queues while sensitive to I/O latency or | ||
| // throughput.. |
There was a problem hiding this comment.
| // throughput.. | |
| // throughput... |
| let _ = minder.take_notifications(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
i believe we could save an entire tabstop of rightward drift here if we were to instead write:
| } | |
| for queue in state.queues | |
| .as_ref() | |
| .into_iter() | |
| .flat_map(|qcol| qcol.queues.iter()) | |
| { | |
| let qs = queue.state.lock().unwrap(); | |
| if let Some(minder) = qs.minder.as_ref() { | |
| let _ = minder.take_notifications(); | |
| } | |
| } |
but that may not compile, and also you might hate it!
There was a problem hiding this comment.
i'd be thinking about it if we got so far to the right that we might wrap differently, but my feeling is this is a local minima of gross, you're right!
(if i were going to take a more serious swing at this, i'd probably want to be able to walk all the queues directly instead of this attachment->workers->state->queues->minder stuff.. but plumbing wasn't and isn't straightforward to me vs this loop. sorry future-us)
| // do. If they do not, the device is already broken, we just hadn't | ||
| // noticed yet. And if they don't match, things will be more and | ||
| // differently broken. |
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...