Libguestfs - Sep 2023 - [libnbd PATCH] rust: Use mio::poll instead of requiring epoll

On Mon, Sep 04, 2023 at 04:07:54PM +0000, Tage Johansson
wrote:
> From: Eric Blake <eblake at redhat.com>
> 
> CI shows our async handle fails to build on FreeBSD and MacOS (where
> epoll() is not available as a syscall, and therefore not available as
> a Rust crate).  We can instead accomplish the same level-probing
> effects by doing a zero-timeout poll with mio (which defers under the
> hood to epoll on Linux, and kqueue on BSD).
> 
> Fixes: 223a9965 ("rust: async: Create an async friendly handle
type")
> CC: Tage Johansson <tage.j.lists at posteo.net>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  rust/Cargo.toml          |  3 ++-
>  rust/src/async_handle.rs | 40 +++++++++++++++++++++++-----------------
>  2 files changed, 25 insertions(+), 18 deletions(-)
> 
> -        // Use epoll to check the current read/write availability on the
fd.
> +        // Use mio poll to check the current read/write availability on
the fd.
>          // This is needed because Tokio supports only edge-triggered
>          // notifications but Libnbd requires level-triggered
notifications.
> -        let mut revent = epoll::Event { data: 0, events: 0 };
>          // Setting timeout to 0 means that it will return immediately.
> -        epoll::wait(epfd, 0, std::slice::from_mut(&mut revent))?;
> -        let revents = Events::from_bits(revent.events).unwrap();
> -        if !revents.contains(Events::EPOLLIN) {
> -            ready_guard.clear_ready_matching(IoReady::READABLE);
> -        }
> -        if !revents.contains(Events::EPOLLOUT) {
> -            ready_guard.clear_ready_matching(IoReady::WRITABLE);
> +        // mio states that it is OS-dependent on whether a single event
> +        // can be both readable and writable, but we survive just fine
> +        // if we only see one direction even when both are available.
> +        poll.registry().register(
> +            &mut SourceFd(&fd),
> +            Token(0),
> +            MioInterest::READABLE | MioInterest::WRITABLE,
> +        )?;
> +        poll.poll(&mut events, Some(Duration::ZERO))?;
Why do we want 'poll.poll()?;', that is, to fail this function if the
poll returns an error?  We _expect_ poll to sometimes return an error
(namely, the fact that it timed out) if there is nothing pending on
the fd, at which point we WANT to successfully clear the ready_guard
for both read and write, rather than to error out of this function.
> +        for event in &events {
> +            if !event.is_readable() {
> +                ready_guard.clear_ready_matching(IoReady::READABLE);
> +            }
> +            if !event.is_writable() {
> +                ready_guard.clear_ready_matching(IoReady::WRITABLE);
> +            }
>          }
>          ready_guard.retain_ready();
> +        poll.registry().deregister(&mut SourceFd(&fd))?;
Furthermore, if the regsiter()/deregister() should always occur in
pairs, the early poll()? exit breaks that assumption (I don't know if
Rust has enough smarts to automatically clean up the unpaired
registration).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

On 9/4/2023 8:23 PM, Eric Blake wrote:
> On Mon, Sep 04, 2023 at 04:07:54PM +0000, Tage Johansson wrote:
>> From: Eric Blake <eblake at redhat.com>
>>
>> CI shows our async handle fails to build on FreeBSD and MacOS (where
>> epoll() is not available as a syscall, and therefore not available as
>> a Rust crate).  We can instead accomplish the same level-probing
>> effects by doing a zero-timeout poll with mio (which defers under the
>> hood to epoll on Linux, and kqueue on BSD).
>>
>> Fixes: 223a9965 ("rust: async: Create an async friendly handle
type")
>> CC: Tage Johansson <tage.j.lists at posteo.net>
>> Signed-off-by: Eric Blake <eblake at redhat.com>
>> ---
>>   rust/Cargo.toml          |  3 ++-
>>   rust/src/async_handle.rs | 40
+++++++++++++++++++++++-----------------
>>   2 files changed, 25 insertions(+), 18 deletions(-)
>>
>> -        // Use epoll to check the current read/write availability on
the fd.
>> +        // Use mio poll to check the current read/write availability
on the fd.
>>           // This is needed because Tokio supports only edge-triggered
>>           // notifications but Libnbd requires level-triggered
notifications.
>> -        let mut revent = epoll::Event { data: 0, events: 0 };
>>           // Setting timeout to 0 means that it will return
immediately.
>> -        epoll::wait(epfd, 0, std::slice::from_mut(&mut revent))?;
>> -        let revents = Events::from_bits(revent.events).unwrap();
>> -        if !revents.contains(Events::EPOLLIN) {
>> -            ready_guard.clear_ready_matching(IoReady::READABLE);
>> -        }
>> -        if !revents.contains(Events::EPOLLOUT) {
>> -            ready_guard.clear_ready_matching(IoReady::WRITABLE);
>> +        // mio states that it is OS-dependent on whether a single
event
>> +        // can be both readable and writable, but we survive just fine
>> +        // if we only see one direction even when both are available.
>> +        poll.registry().register(
>> +            &mut SourceFd(&fd),
>> +            Token(0),
>> +            MioInterest::READABLE | MioInterest::WRITABLE,
>> +        )?;
>> +        poll.poll(&mut events, Some(Duration::ZERO))?;
> Why do we want 'poll.poll()?;', that is, to fail this function if
the
> poll returns an error?  We _expect_ poll to sometimes return an error
> (namely, the fact that it timed out) if there is nothing pending on
> the fd, at which point we WANT to successfully clear the ready_guard
> for both read and write, rather than to error out of this function.

You are right. I thought that the poll() call would return Ok(()) upon 
timeout, but according to the documentation:
> Currently if the timeout elapses without any readiness events 
> triggering this will return Ok(()). However we?re not guaranteeing 
> this behaviour as this depends on the OS.
So I guess it is best to ignore any errors from the poll call as in your 
patch.


--

Best regards,

Tage

>> +        for event in &events {
>> +            if !event.is_readable() {
>> +                ready_guard.clear_ready_matching(IoReady::READABLE);
>> +            }
>> +            if !event.is_writable() {
>> +                ready_guard.clear_ready_matching(IoReady::WRITABLE);
>> +            }
>>           }
>>           ready_guard.retain_ready();
>> +        poll.registry().deregister(&mut SourceFd(&fd))?;
> Furthermore, if the regsiter()/deregister() should always occur in
> pairs, the early poll()? exit breaks that assumption (I don't know if
> Rust has enough smarts to automatically clean up the unpaired
> registration).
>