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

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).
>

On Mon, Sep 04, 2023 at 06:59:15PM +0000, Tage Johansson
wrote:
> 
> > > +        // 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.
Okay, I'll merge in that aspect of my original along with your other
improvements, and push something soon.  Fingers crossed that we'll
finally get a green CI run today.

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