CP-52821: use Mtime in Xapi_periodic_scheduler #6161
Conversation
|
Isn't it better to rebase on master already and build on top of the scheduler changes there? Those rebase changes would have to be done later anyway, but then end up in a complicated and hard to review merge commit. |
|
There's a further improvement here: replace all |
I'll rebase this PR on top of master once Frediano's PR is merged. |
It's not clear what overflows you are talking about. Internally the timer is a counter since boot, that will overflow the PS: @psafont are you referring to floating point? Maybe it would be sensible to consider a too big timeout (like 10 years) as an error? |
One function returns an option in case of overflow, the other does not. Because, as you point out, the overflow won't happen in practise, I prefer to use the latter rather than the former.
I fundamentally disagree, timestamps are actually differences as well (between an epoch and a point in time), and the only practical difference is replacing is_{earlier,later} with is_{shorter,longer}. I'm fine with the improvement not being in this PR, it was only an observation |
Test that the event is correctly executed. Signed-off-by: Frediano Ziglio <[email protected]>
Signed-off-by: Frediano Ziglio <[email protected]>
Do not use ">" or other operators to compare Mtime.t, the value is intended to be unsigned and should be compared with Int64.unsigned_compare as Mtime functions do. Signed-off-by: Frediano Ziglio <[email protected]>
Signed-off-by: Frediano Ziglio <[email protected]>
The parameter was false only to support an internal usage, external users should always alert the thread loop. Signed-off-by: Frediano Ziglio <[email protected]>
- Do not wait huge amount of time if the queue is empty but use Delay.wait if possible; - Fix delete of periodic events. In case the event is processed it's removed from the queue. Previously remove_from_queue was not able to mark this event as removed; - Do not race between checking the first event and removing it. These 2 actions were done in 2 separate critical section, now they are done in a single one. Signed-off-by: Frediano Ziglio <[email protected]>
Potentially a periodic event can be cancelled while this is processed. Simulate this behavior removing the event inside the handler. This was fixed by previous commit. Signed-off-by: Frediano Ziglio <[email protected]>
Previously if the queue was empty and the loop thread was active the scheduler took quite some time to pick up the new event. Check that this is done in a timely fashion to avoid regressions in code. Signed-off-by: Frediano Ziglio <[email protected]>
|
Moved this to draft until the referenced PR is merged to master as this will need a rebase. #6155 |
Similar to test_empty test however the state of the loop should be different. Signed-off-by: Frediano Ziglio <[email protected]>
For the scan retry, previously we were comparing the entire vdi data
structure from the database using the (<>) operator. This is a bit
wasteful and not very stable. Instead let us just compare the vdi refs,
since the race here comes from `VDI.db_{introduce,forget}`, which would
only add/remove vdis from the db, but not change its actual data
structure.
Also add a bit more logging when retrying, since this should not happen
very often.
Signed-off-by: Vincent Liu <[email protected]>
|
Rebased on latest feature/perf (which in turn has been updated to include latest master). E.g. previously there was a +0.5 in the periodic scheduler, but now we just use Clock.Timer.remaining, so there could be some subtle differences in behaviour. |
When add leaked datapath: 1. add leaked datapath to Sr.vdis 2. write to db file 3. log enhance If there are storage exceptions raised when destroying datapath, the procedure fails and the state of VDI becomes incorrect, which leads to various abnormalresults in subsequent operations. To handle this, the leaked datapath is designed to redestroy the datapath and refresh the state before next storage operation via function remove_datapaths_andthen_nolock. But this mechanism doesn't take effect in current code. This commit is to fix this bug. Leaked datapath should be added to Sr.vdis to make the leaked datapath really work. And write to db file to avoid losing the leaked datapath if xapi restarts. Signed-off-by: Changlei Li <[email protected]>
There was some issue in the current code, from structure corruption to thread safety. Add some test and fix discovered issues. More details on commit messages.
When add leaked datapath: 1. add leaked datapath to Sr.vdis 2. write to db file 3. log enhance If there are storage exceptions raised when destroying datapath, the procedure fails and the state of VDI becomes incorrect, which leads to various abnormalresults in subsequent operations. To handle this, the leaked datapath is designed to redestroy the datapath and refresh the state before next storage operation via function remove_datapaths_andthen_nolock. But this mechanism doesn't take effect in current code. This commit is to fix this bug. leaked datapath should be added to Sr.vdis to make the leaked datapath really work. And write to db file to avoid losing the leaked datapath if xapi restarts.
For the scan retry, previously we were comparing the entire vdi data
structure from the database using the (<>) operator. This is a bit
wasteful and not very stable. Instead let us just compare the vdi refs,
since the race here comes from `VDI.db_{introduce,forget}`, which would
only add/remove vdis from the db, but not change its actual data
structure.
Also add a bit more logging when retrying, since this should not happen
very often.
Signed-off-by: Edwin Török <[email protected]>
Signed-off-by: Edwin Török <[email protected]>
Signed-off-by: Edwin Török <[email protected]>
Avoids dealing with overflow Signed-off-by: Edwin Török <[email protected]>
Target: feature/perf
Note that this conflicts with 7e9310f?diff=unified&w=1, but is a pre-requisite of #6126