Marc-André Lureau <[email protected]> writes: > Hi > > On Tue, Oct 27, 2020 at 12:41 PM Markus Armbruster <[email protected]> wrote: >> >> [email protected] writes: >> >> > From: Marc-André Lureau <[email protected]> >> > >> > Thanks to the monitors coroutine support, the screendump handler can >> >> monitors' >> >> Suggest to add (merge commit b7092cda1b3) right before the comma. >> >> > trigger a graphic_hw_update(), yield and let the main loop run until >> > update is done. Then the handler is resumed, and ppm_save() will write >> > the screen image to disk in the coroutine context (thus non-blocking). >> > >> > Potentially, during non-blocking write, some new graphic update could >> > happen, and thus the image may have some glitches. Whether that >> > behaviour is acceptable is discutable. Allocating new memory may not be >> >> s/discutable/debatable/ >> >> > a good idea, as framebuffers can be quite large. Even then, QEMU may >> > become less responsive as it requires paging in etc. >> >> Tradeoff. I'm okay with "simple & efficient, but might glitch". It >> should be documented, though. Followup patch is fine. >> >> > Related to: >> > https://bugzilla.redhat.com/show_bug.cgi?id=1230527 >> > >> > Signed-off-by: Marc-André Lureau <[email protected]> [...] >> Let's revisit the experiment I did for v1: "observe the main loop keeps >> running while the screendump does its work". >> >> Message-ID: <[email protected]> >> https://lists.nongnu.org/archive/html/qemu-devel/2020-03/msg01235.html >> >> I repeated the first experiment "does the main loop continue to run when >> writing out the screendump blocks / would block?" Same result: main >> loop remains blocked. >> >> Back then, you replied >> >> Right, the goal was rather originally to fix rhbz#1230527. We got >> coroutine IO by accident, and I was too optimistic about default >> behaviour change ;) I will update the patch. >> >> I'm unsure what exactly the update is. Is it the change from >> >> Fixes: >> https://bugzilla.redhat.com/show_bug.cgi?id=1230527 >> >> to >> >> Related to: >> https://bugzilla.redhat.com/show_bug.cgi?id=1230527 >> >> ? > > Right, qmp_screendump() calls qemu_open_old(filename, O_WRONLY | > O_CREAT | O_TRUNC | O_BINARY, 0666), so opened in blocking mode. > > So simply opening a FS file with O_NONBLOCK or calling > qemu_try_set_nonblock(fd) with the resulting fd doesn't really help to > check it's async (unless I am missing a trick to slow down disk IO > somehow...?) > > It's annoying that it also does O_TRUNC: even if you pass a > socket/pipe to add-fd, it will fail to ftruncate() (via the > "/dev/fdset" path). Furthermore, access mode check seems kinda > incomplete. Afaict, in monitor_fdset_dup_fd_add(), F_GETFL may return > O_RDWR which is different than O_RDONLY or O_WRONLY, yet should be > considered compatible for the caller I think.. > > With some little hacks though, I could check passing a pipe does > indeed make PPM save async, and the main loop is reentered. I don't > know if it's enough to convince you it's not really the problem of > this code change though. We get coroutine IO by accident here, I think > we already said that.
I'm okay with this patch "only" getting us closer to the goal of having screendump not block the main loop. I just want the commit message to be clear on how close. >> The commit message says "ppm_save() will write the screen image to disk >> in the coroutine context (thus non-blocking)." Sure reads like a claim >> the main loop is no longer blocked. It is blocked, at least for me. >> Please clarify. > > Let's clarify it by saying it's still blocking then, and tackle that > in a future change. Works for me! >> Back then, I proposed a second experiment: "does the main loop continue >> to run while we wait for graphic_hw_update_done()?" I don't know the >> result. Do you? >> >> The commit message claims "the screendump handler can trigger a >> graphic_hw_update(), yield and let the main loop run until update is >> done." > > Isn't it clearly the case with the BH being triggered after main loop? Yes, but have you *tested* the main loop keeps running?
