On Mon, Nov 28, 2022 at 09:21:47AM +0100, Jan Beulich wrote: > On 26.11.2022 23:19, Julien Grall wrote: > > On 25/11/2022 14:15, Per Bilse wrote: > >> A change to XAPI varstored causes > > > > For those unfamiliar with XAPI (like me), can you explain what was the > > change made?
One ioreq client used by XenServer doesn't handle buffered ioreqs, so the broadcasted TIMEOFFSET always triggers an error due to that ioreq not handling it. While not harmful, it's still annoying to get the messages on the hypervisor console for something that's not really an error. The description can indeed be reworded to not mention XenServer specific components, something like: "Avoid printing an error message when a broadcast buffered ioreq is not handled by all registered clients, as long as the failure is strictly because the client doesn't handle buffered ioreqs." > >> an unnecessary error message > >> to be logged in hypervisor.log whenever an RTC timeoffset update > >> is broadcast. > >> In extreme cases this could flood the log file. > > > > Which should be ratelimited as this is using guest error loglevel. But I > > think this is irrelevant here. It would be more relevant to explain why > > it is OK to allow a partial broadcast. > > > >> This patch modifies ioreq_broadcast() to allow partial success. > > > > The commit message is quite vague, so it is hard to know what you are > > trying to solve exactly. AFAIU, there are two reasons for > > ioreq_broadcast to fails: > > 1) The IOREQ server didn't register the bufioreq > > 2) The IOREQ buffer page is full > > > > While I would agree that the error message is not necessary for 1) (the > > IOREQ server doesn't care about the event), I would disagree for 2) > > because it would indicate something went horribly wrong in the IOREQ > > server and there is a chance your domain may misbehave afterwards. > > In addition I think ignoring failure (and, as said by Julien, only because > of no bufioreq being registered) is (kind of implicitly) valid only for > buffered requests. Hence I'm unconvinced of the need of a new boolean > function parameter. Instead I think we need a new IOREQ_STATUS_... value > representing the "not registered" case. And that could then be used by > ioreq_broadcast() to skip incrementing of "failed". So introduce an IOREQ_STATUS_UNREGISTERED return code and don't increase failed if buffered == true and UNREGISTERED is returned, would that be acceptable? Thanks, Roger.
