aio_set_event_notifier(is_external=true) in Xen code?

aio_set_event_notifier(is_external=true) in Xen code?

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 490 bytes --]

Hi,
I'm removing the aio_disable_external() API from QEMU and noticed that
Xen code calls aio_set_fd_handler(is_external=true) in hw/xen/xen-bus.c
and hw/i386/kvm/xen_xenstore.c.

It wasn't clear to me whether is_external=true is necessary here.
is_external=true is mainly used to temporarily pause I/O submission in
the QEMU block layer. Maybe is_external=true was chosen out of caution
but actually has no effect in this code.

Does the Xen code rely on is_external=true?

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: aio_set_event_notifier(is_external=true) in Xen code?

[Durrant, Paul]

> -----Original Message-----
> From: Stefan Hajnoczi <stefanha@redhat.com>
> Sent: 28 March 2023 16:51
> To: Woodhouse, David <dwmw@amazon.co.uk>; Durrant, Paul
> <pdurrant@amazon.co.uk>
> Cc: qemu-devel@nongnu.org; qemu-block@nongnu.org
> Subject: [EXTERNAL] aio_set_event_notifier(is_external=true) in Xen code?
> 
> Hi,
> I'm removing the aio_disable_external() API from QEMU and noticed that
> Xen code calls aio_set_fd_handler(is_external=true) in hw/xen/xen-bus.c
> and hw/i386/kvm/xen_xenstore.c.
> 
> It wasn't clear to me whether is_external=true is necessary here.
> is_external=true is mainly used to temporarily pause I/O submission in
> the QEMU block layer. Maybe is_external=true was chosen out of caution
> but actually has no effect in this code.
> 
> Does the Xen code rely on is_external=true?


That's a good question. The call in xen-bus.c has been there since commit 83361a8a1f932, which was when it substituted the old call to qemu_set_fd_handler(). I suspect this was out of caution (or possibly misunderstanding) at the time, although setting the call to xen_device_set_event_channel_context() in xen_block_dataplane_stop() does suggest it may be happening while I/O could be in progress so it could have been in response to problems caught in testing.
I suspect the code in xen_xenstore.c just copied what xen-bus.c did.

Sorry I can't give you a definitive answer... it's all rather a long time ago.

  Cheers,

    Paul

Re: aio_set_event_notifier(is_external=true) in Xen code?

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1761 bytes --]

On Tue, Mar 28, 2023 at 04:36:29PM +0000, Durrant, Paul wrote:
> > -----Original Message-----
> > From: Stefan Hajnoczi <stefanha@redhat.com>
> > Sent: 28 March 2023 16:51
> > To: Woodhouse, David <dwmw@amazon.co.uk>; Durrant, Paul
> > <pdurrant@amazon.co.uk>
> > Cc: qemu-devel@nongnu.org; qemu-block@nongnu.org
> > Subject: [EXTERNAL] aio_set_event_notifier(is_external=true) in Xen code?
> > 
> > Hi,
> > I'm removing the aio_disable_external() API from QEMU and noticed that
> > Xen code calls aio_set_fd_handler(is_external=true) in hw/xen/xen-bus.c
> > and hw/i386/kvm/xen_xenstore.c.
> > 
> > It wasn't clear to me whether is_external=true is necessary here.
> > is_external=true is mainly used to temporarily pause I/O submission in
> > the QEMU block layer. Maybe is_external=true was chosen out of caution
> > but actually has no effect in this code.
> > 
> > Does the Xen code rely on is_external=true?
> 
> 
> That's a good question. The call in xen-bus.c has been there since commit 83361a8a1f932, which was when it substituted the old call to qemu_set_fd_handler(). I suspect this was out of caution (or possibly misunderstanding) at the time, although setting the call to xen_device_set_event_channel_context() in xen_block_dataplane_stop() does suggest it may be happening while I/O could be in progress so it could have been in response to problems caught in testing.
> I suspect the code in xen_xenstore.c just copied what xen-bus.c did.
> 
> Sorry I can't give you a definitive answer... it's all rather a long time ago.

Thanks, that helped! I'll study hw/block/dataplane/xen-block.c and make
sure it has a safe alternative to is_external=true.

I think is_external=false is safe for xen_xenstore.c.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]