08:17:54 kevin joined the channel
10:12:22 XanClic joined the channel
13:42:18 kevin: XanClic: Kann man nicht theoretisch von zwei iothreads aus gleichzeitig drainen?
14:26:51 XanClic: kevin, aber sollte nicht trotzdem nur eins der Drains tatsächlich den Job drainen?
14:27:25 XanClic: also wenn die Drains gleichzeitig sind, dann sollte nur eins der beiden drained_end den pause_count auf 0 bringen
14:27:51 XanClic: und wenn dazwischen ein neues Drain beginnt, dann muss das immer noch den Job pollen, der in seinem einen AioContext serialisiert sein müsste
14:28:15 XanClic: also wenn ein Thread sein Drain beendet und kurz darauf ein anderer seins beginnt
14:28:46 XanClic: (Wenn der andere seins eher beginnt, dann wird das drained_end des ersten pause_count nicht auf 0 bringen und somit muss *drained_end_counter auch nicht angefasst werden)
14:29:30 kevin: XanClic: Macht das die Sache nicht noch schlimmer, weil beide ihren Counter erhöhen, aber nur einer wieder dekrementiert wird?
14:31:10 XanClic: kevin, man erhöht doch aber den Counter nur, wenn job_resume() aufgerufen wird, und am Ende auch aio_co_enter()
14:31:23 XanClic: Ansonsten gibts ja keine Background Operation, die geschedulet wird
14:31:48 kevin: job_resume() ruft man immer aus, das returnt nur früher, wenn job->pause_count noch > 0 ist
14:32:16 XanClic: Ja, gut, wie gesagt, wenn am Ende auch aio_co_enter() passiert
14:32:58 XanClic: bin kurz weg
14:33:01 kevin: Hm, nein, ist so oder so falsch. Wir müssen ja trotzdem in beiden Drains warten, bis der Job wieder läuft, nicht nur in einem davon
14:33:49 kevin: Also muss der Counter erhöht werden, egal wer dafür sorgt, dass der Job wieder läuft
14:34:14 XanClic: das ist dann aber nicht mehr, wofür der Counter laut Definition da ist
14:34:49 XanClic: ich muss mir noch überlegen, wie ich das finde, wenn wir drained_end so lange blockieren, bis tatsächlich alles läuft, nur weil jemand anderes da was gedraint hat
14:47:38 kevin: Ah, hm. Ich habe nur an den Fall gedacht, indem beide gleichzeitig drain_end machen. Aber ja, das ist natürlich ein guter Punkt, dass der eine noch weiterdrainen könnte.
14:56:56 XanClic: jedenfalls dachte ich, dass der Counter primär dafür da war, dass nicht durch das drained_end irgendwelche Resourcen benutzt werden, die vorher noch da waren und danach nicht mehr
14:57:20 XanClic: dass deshalb vom Caller gewartet werden muss, dass alle Hintergrundoperationen, die gestartet werden, auch gesettlet sind
14:57:54 XanClic: aber dann beträfe das eben auch nur diese Operationen, und hätte wenig mit „Warten, bis alles wieder läuft, was hätte pausiert sein können“ zu tun
14:58:44 XanClic: wenn das so ist, hieße das dann leider, dass der Counter bei dem im BZ genannten Problem nicht so wirklich helfen würde, wie dein Beispiel zeigt
16:34:02 kevin: Naja, gedraint werden sollte eh nur für kurze Zeit, von daher ist es in der Praxis wahrscheinlich wurscht, wenn man tatsächlich darauf wartet, dass der Job tatsächlich wieder läuft
16:34:29 kevin: Es sei denn, du hast eine Idee, wie man das sonst ohne größere Umbauarbeiten löst
16:34:45 XanClic: block-job-complete während standby akzeptieren klingt doch ok
16:35:17 kevin: Und dann in einen neuen Zustand wechseln?
16:35:23 XanClic: eigentlich erst beim resume
16:35:40 XanClic: weiß sowieso nicht genau, wieso das nach außen sichtbar sein soll, dass der Job wegen eines internen Drains pausiert ist
16:35:58 kevin: Ich glaube nicht, dass das geplant war
16:36:01 kevin: Ist halt so passiert
16:36:04 XanClic: wenn der Job das also darf, dann darf er auch complete deferren, bis er nicht mehr pausiert ist
16:37:24 kevin: job->driver->complete() aufrufen, während der Job eigentlich pausiert ist, fühlt sich gewagt an
16:37:32 XanClic: würde ich mir ja vormerken
16:37:37 XanClic: to_be_completed
16:37:56 kevin: Und dann nach dem Pause Point umsetzen?
16:38:00 XanClic: ja
16:38:20 XanClic: Gut, von mir aus kann man auch job->driver->complete() aufrufen und das eben Mirror machen lassen
16:38:29 kevin: Nur für interne Pausierung oder auch wenn es vom Benutzer gekommen ist?
16:38:34 XanClic: block-job-complete ist ja nunmal sowieso spezifisch für Mirror
16:38:42 kevin: Psst ;-)
16:38:50 XanClic: Von mir aus auch für Benutzer, aber John wollte ja nur für intern
16:38:54 XanClic: Mir ist das eigentlich gleich
16:39:12 XanClic: Sehe das nicht als problematisch, wenn der Benutzer einen Job completen lassen will, der gerade pausiert ist
16:39:47 kevin: Was John will, weiß ich nicht so genau, der hat im BZ nichts geschrieben. Da soll es wohl eine Mail auf der Liste geben, aber zum qemu-block-Mailorder bin ich noch nicht gekommen
16:39:51 kevin: Das typische Nachurlaubsproblem
16:40:04 XanClic: In seinem Statusreport war da ein Link, Moment…
16:40:14 XanClic: https://lists.gnu.org/archive/html/qemu-block/2021-04/msg00035.html
16:41:09 XanClic: Also, wenn ich mir mirror_complete() so ansehe
16:41:10 kevin: > Do we have the ability to easily accept an async "intent" to complete a job without tying up the monitor?
16:41:13 kevin: Das ist es doch eh nur?
16:41:27 XanClic: dann würde es doch reichen, da zu prüfen, ob pausiert ist, und wenn ja, dann lässt man das job_enter() weg
16:41:28 kevin: Complete war noch nie synchron, der Mirror muss erstmal fertigkopieren
16:42:35 XanClic: Wobei das Problem vermutlich sein könnte, dass wir ja in einer Drained-Sektion keine Graph-Änderungen wollen, und da gibts ein bisschen was in mirror_complete()
16:42:37 kevin: XanClic: Hm, ja, vielleicht?
16:42:52 XanClic: glaube nicht, dass die problematisch wären, aber falsch wärs schon
16:42:52 kevin: Gehen Graphänderungen nicht mittlerweile?
16:43:06 XanClic: Toll :)
16:43:17 kevin: Da war ein Fragezeichen *g*
16:43:27 XanClic: Schade
16:43:36 XanClic: Na ja, ansonsten kann man das sicher in die Mirror-Main-Loop verschieben
16:43:45 XanClic: und einfach da machen, wenn should_complete gesetzt ist
16:43:49 XanClic: Ach, nein
16:43:53 XanClic: Sollte ja *errp setzen
16:44:29 XanClic: Das wäre dann ein generelles Problem
16:44:38 kevin: Ich wollte jetzt grad was über AioContext sagen, aber ich habe keine Ahnung, in welchem .complete eigentlich läuft...
16:44:58 XanClic: Dass so ein to_be_completed dann -complete immer Erfolg anzeigen lassen würde, aber der Fehler kommt dann später
16:45:39 kevin: Ich glaube .complete läuft im Mainloop-Thread mit AioContext-Lock vom Job
16:45:41 XanClic: müsste man sich dann wohl angucken, ob das, was mirror_complete() macht, in Drained ok wäre
16:45:53 kevin: Nach dem Verschieben würde es im iothread laufen, und der darf den Graph nicht verändern, oder?
16:46:26 XanClic: Wie gesagt, vielleicht ist Verschieben eh nicht so doll
16:46:32 XanClic: Weil -complete dann keinen Fehler mehr geben kann
16:46:43 XanClic: Aber komm, das ist doch eh ein Bug im Managementlayer.
16:46:51 XanClic: -complete geht halt nicht, wenn der Job in Standby ist.
16:46:59 XanClic: Und da gibts Events für, das hätte libvirt sehen müssen!
16:47:02 XanClic: O:)
16:47:25 XanClic: Also ist nur halb ein Scherz
16:47:37 XanClic: Wobei das natürlich eine Race Condition gibt, wenn man das so anwendet
16:47:45 kevin: Ja, das geht nicht
16:47:49 XanClic: dann müsste libvirt halt retryen
16:47:58 XanClic: wo dann die Frage wäre, wie sie mitbekommen, dass es deshalb nicht gegangen ist
16:48:05 kevin: Retry wäre eine "Lösung", aber halt ziemlich hässlich
16:50:09 kevin: Irgendwie kommt mir drained_end_counter immer noch am einfachsten vor, auch wenn man dann notfalls mal auf einen zweiten Drain warten muss
16:50:29 XanClic: Finde das nicht einfach
16:50:36 XanClic: Klingt für mich erstens nach Missbrauch
16:50:52 XanClic: Und zweitens kam deine Frage mit den Concurrent Drains ja im Zusammenhang damit auf, ob man dann eine Liste von Pointern im Job braucht
16:51:04 kevin: Jo, dann nimmt man halt eine Liste?
16:51:13 XanClic: Ich find, das ist alles Missbrauch
16:51:23 XanClic: Also gut, dann halt eine Liste, aber mir gefällt das wirklich gar nicht
16:51:27 XanClic: Das klingt so richtig falsch für mich
16:51:51 XanClic: Ich kann mich nicht daran erinnern, dass drained_end jemals garantiert hat, dass danach Jobs aktiv laufen
16:52:00 kevin: Das ist ja genau der Bug
16:52:03 XanClic: Und jetzt brauchen wir das nur, weil libvirt manchmal kein -complete absetzen kann
16:52:15 kevin: drained_begin stoppt den Job, also muss drained_end ihn auch wieder fortsetzen bevor es returnt
16:52:27 XanClic: Für mich ist der Bug, dass man mit pausierten Jobs nicht arbeiten kann
16:53:02 kevin: Findest du nicht, dass wenn drained_end fertig ist, alles wieder im Zustand wie davor sein sollte?
16:53:05 XanClic: Nein
16:53:14 kevin: Plötzlich einen pausierten Job zu sehen, finde ich ziemlich überraschend
16:53:31 kevin: Das ist nur deswegen halbwegs ok, weil wir solange den BQL halten und keine anderen QMP-Commands ausführen
16:53:56 kevin: Also eigentlich dürfte es niemand merken, wenn drained_end lange genug wartet
16:54:04 XanClic: Was wäre denn eigentlich, wenn ich einen Job pausiere und dann draint irgendwas
16:54:15 XanClic: dann muss ich ja auch unterscheiden, ob das pause_count jetzt vom Nutzer kommt oder von einem anderen Drain
16:54:34 XanClic: Also
16:54:38 kevin: Hm
16:55:00 XanClic: Was ich vor allem sage, ist, dass ich mir nicht vorstellen kann, ein drained_end auf ein anderes Drain warten zu lassen
16:55:43 XanClic: Und da muss man sagen, geht es ja nicht darum, dass es nachher so ist wie vorher
16:55:59 XanClic: Weil ja mindestens bei einem der beiden Drains der Job vorher schon pausiert war
16:56:16 kevin: Ja, und dann hast du zwei Fälle, je nachdem, wer sein Drain zuerst beendet
16:56:30 XanClic: Aber beide sollen ja darauf warten, dass der Job läuft
16:56:45 XanClic: Unabhängig vom vor-Drain-Zustand des jeweiligen Drains
16:56:50 XanClic: Den kann man ja auch gar nicht kennen
16:56:51 kevin: Wenn es das zweite Drain ist, hast du kein Problem, denn da war der Job vorher pausiert und nachher auch
16:56:56 kevin: Wenn sie sich überschneiden, wird es blöd
16:57:40 XanClic: Von mir aus können wir auch vor in der complete-Implementierung alle Jobs pollen
16:57:57 XanClic: Falls die intern pausiert sein sollten
16:58:48 kevin: Hm, haben wir das Problem eigentlich nicht auch, wenn es nur ein Drain gibt, und zwar im iothread?
16:59:01 kevin: Dann kann die Mainloop ja immer noch QMP-Befehle ausführen
16:59:25 XanClic: Klingt erstmal so
17:00:04 kevin: Möglicherweise fängt der AioContext-Lock das ab, aber Geld drauf wetten würde ich nicht
17:01:19 kevin: Kommt halt drauf an, was der Drain-Caller so macht bevor er drain_end aufruft
17:02:47 kevin: Könnten wir PAUSED/STANDBY auch nur dann machen, wenn der Job vom User pausiert ist? (Oder wegen einem I/O-Fehler, aber das ist ja dasselbe)
17:03:00 XanClic: Na ja
17:03:27 XanClic: Das Problem ist doch, dass wenn das gedraint ist, dann kann man nicht completen
17:03:35 XanClic: Nicht, wie der dem Nutzer sichtbare Zustand ist
17:03:42 kevin: Hm, würde auf dasselbe rauslaufen, wie complete in STANDBY erlauben, ja
17:03:44 XanClic: äh, nicht gedraint, aber halt irgendwie pausiert
17:04:01 XanClic: Ich weiß halt nicht, ob man einfach in -complete den Job pollen kann, bis er wieder läuft
17:04:11 XanClic: oder ob es die Gefahr gibt, dass irgendein Thread wieder draint
17:04:19 XanClic: und man dann wieder gutes tocttou hat
17:05:03 kevin: Solange der AioContext gelockt ist (das weißt du ja in .complete), kann niemand drainen
17:05:34 kevin: Aber in .complete pollen klingt trotzdem irgendwie gewagt
17:05:46 kevin: Ohne dass ich sagen könnte, warum
17:06:46 XanClic: meinte auch nicht .complete, sondern -complete
17:06:52 XanClic: also entweder qmp_blockdev_complete()
17:06:59 XanClic: oder die job.c-Implementierung oder wo das ist
17:07:05 kevin: Ach von ganz außen?
17:07:50 XanClic: Ja
17:07:53 kevin: Das heißt, dann würde job-complete einfach so lange blockieren, bis der Job wieder lauffähig ist?
17:07:58 XanClic: Ja
17:08:07 XanClic: Also, wenn es wegen internem Drain ist
17:08:09 kevin: Was es logischerweise auf interne Pausen beschränkt
17:08:23 kevin: Die anderen müssen dann halt weiterhin einen Fehler geben
17:08:28 XanClic: Jo
17:08:51 kevin: Jo, würde das aktuelle Problem vermutlich lösen
17:09:14 kevin: Fühlt sich halt mehr nach Symptombehandlung an als nach echtem Fix, aber naja...
17:09:36 kevin: Irgendjemand wird schon schreien, wenn wir in den nächsten Fall laufen ;-)
17:10:59 XanClic: Ich hätte auch noch ein “Führen wir doch ein execute_when ein“ zu bieten *g*
17:11:56 XanClic: {"execute":"block-job-complete","when":{"cond":"job-unpaused","job":"job0"},"arguments":{"device":"job0"}}
17:11:58 XanClic: das wär doch toll.
17:14:08 kevin: Ich glaube, das einzige tolle daran wäre, dir dabei zuzuschauen, wie du versuchst, die Patches durch Markus' Review zu kriegen *g*
17:14:17 XanClic: Au, au…
17:15:25 kevin: Eigentlich missbrauchen wir halt die Paused-Status für etwas ganz anderes in Drain
17:15:46 kevin: Normal sollten die Jobs da einfach eine Weile nichts tun, ohne dass das nach außen sichtbar ist
17:16:15 XanClic: Das ist auch wahr
17:16:15 kevin: Paused zu missbrauchen war bequem, aber halt irgendwie falsch
17:16:38 XanClic: Aber ich weiß gar nicht, ob es das jetzt besser machen würde, wenn wir das separat gemacht hätten
17:16:59 XanClic: Hab das Gefühl, wir könnten zu guter Wahrscheinlichkeit vor praktisch dem gleichen Problem stehen, dass der Job eben nichts machen darf, und da gehört nunmal auch -complete dazu
17:17:43 kevin: Auch wieder wahr
17:18:09 kevin: Es würde nur die unerwarteten Events wegmachen
17:18:24 XanClic: Das stimmt wohl
17:20:48 kevin: Also dann wohl doch pollen in job_complete() :-/
17:26:30 XanClic: ich kanns ja mal probieren
19:12:10 Paddy joined the channel