3:11 p.m.
I managed to get a build of the async Rust handle compiling on FreeBSD
(although the cirrus CI appears to not actually run 'make check' on
non-Linux machines, at least when run on my fork):
https://gitlab.com/ebblake/libnbd/-/jobs/4985192286
However, I'd really like Tage's review on patch 2 to see if my Rust
makes sense.
Eric Blake (2):
maint: Favor 4-space indent in .rs files
rust: Use mio::poll instead of requiring epoll
rust/Cargo.toml | 2 +-
rust/src/async_handle.rs | 46 +++++++++++++++++++++++++---------------
.editorconfig | 4 ++++
3 files changed, 34 insertions(+), 18 deletions(-)
--
2.41.0
3:11 p.m.
New subject: [libnbd PATCH 1/2] maint: Favor 4-space indent in .rs files
Since rustfmt favors 4-space indent on .rs files, we should do
likewise to reduce the churn when running rustfmt after editing in
an emacs session set to honor editorconfig.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
.editorconfig | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/.editorconfig b/.editorconfig
index 86ef47d0..f5bc5604 100644
--- a/.editorconfig
+++ b/.editorconfig
@@ -21,6 +21,10 @@ indent_size = 4
indent_style = tab
indent_size = 4
+[*.rs]
+# Match rustfmt style
+indent_size = 4
+
[{Makefile,Makefile.in,Makefile.am,*.mk}]
# Make requires tabs.
indent_style = tab
--
2.41.0
4:29 p.m.
New subject: [libnbd PATCH 1/2] maint: Favor 4-space indent in .rs files
On Wed, Aug 30, 2023 at 03:11:23PM -0500, Eric Blake wrote:
Since rustfmt favors 4-space indent on .rs files, we should do
likewise to reduce the churn when running rustfmt after editing in
an emacs session set to honor editorconfig.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
.editorconfig | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/.editorconfig b/.editorconfig
index 86ef47d0..f5bc5604 100644
--- a/.editorconfig
+++ b/.editorconfig
@@ -21,6 +21,10 @@ indent_size = 4
indent_style = tab
indent_size = 4
+[*.rs]
+# Match rustfmt style
+indent_size = 4
+
[{Makefile,Makefile.in,Makefile.am,*.mk}]
# Make requires tabs.
indent_style = tab
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
3:11 p.m.
New subject: [libnbd PATCH 2/2] rust: Use mio::poll instead of requiring epoll
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(a)posteo.net>
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
rust/Cargo.toml | 2 +-
rust/src/async_handle.rs | 46 +++++++++++++++++++++++++---------------
2 files changed, 30 insertions(+), 18 deletions(-)
diff --git a/rust/Cargo.toml b/rust/Cargo.toml
index 0879b34c..391c80e9 100644
--- a/rust/Cargo.toml
+++ b/rust/Cargo.toml
@@ -49,7 +49,7 @@ thiserror = "1.0.40"
log = { version = "0.4.19", optional = true }
libc = "0.2.147"
tokio = { optional = true, version = "1.29.1", default-features = false,
features = ["rt", "sync", "net"] }
-epoll = "4.3.3"
+mio = "0.8.0"
[features]
default = ["log", "tokio"]
diff --git a/rust/src/async_handle.rs b/rust/src/async_handle.rs
index ab28b910..35b1c0d2 100644
--- a/rust/src/async_handle.rs
+++ b/rust/src/async_handle.rs
@@ -35,9 +35,11 @@ use crate::sys;
use crate::Handle;
use crate::{Error, FatalErrorKind, Result};
use crate::{AIO_DIRECTION_BOTH, AIO_DIRECTION_READ, AIO_DIRECTION_WRITE};
-use epoll::Events;
+use mio::unix::SourceFd;
+use mio::{Events, Interest as MioInterest, Poll, Token};
use std::sync::Arc;
use std::sync::Mutex;
+use std::time::Duration;
use tokio::io::{unix::AsyncFd, Interest, Ready as IoReady};
use tokio::sync::Notify;
use tokio::task;
@@ -176,12 +178,12 @@ async fn polling_task(handle_data: &HandleData) ->
Result<(), FatalErrorKind> {
} = handle_data;
let fd = handle.aio_get_fd().map_err(Error::to_fatal)?;
let tokio_fd = AsyncFd::new(fd)?;
- let epfd = epoll::create(false)?;
- epoll::ctl(
- epfd,
- epoll::ControlOptions::EPOLL_CTL_ADD,
- fd,
- epoll::Event::new(Events::EPOLLIN | Events::EPOLLOUT, 42),
+ let mut events = Events::with_capacity(1);
+ let mut poll = Poll::new()?;
+ poll.registry().register(
+ &mut SourceFd(&fd),
+ Token(0),
+ MioInterest::READABLE | MioInterest::WRITABLE,
)?;
// The following loop does approximately the following things:
@@ -248,19 +250,29 @@ async fn polling_task(handle_data: &HandleData) ->
Result<(), FatalErrorKind> {
}
drop(pending_cmds_lock);
- // 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.
+ match poll.poll(&mut events, Some(Duration::ZERO)) {
+ Ok(_) => {
+ 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);
+ }
+ }
+ }
+ Err(_) => {
+ ready_guard.clear_ready_matching(IoReady::READABLE);
+ ready_guard.clear_ready_matching(IoReady::WRITABLE);
+ }
+ };
ready_guard.retain_ready();
}
}
--
2.41.0
3:35 a.m.
New subject: [libnbd PATCH 2/2] rust: Use mio::poll instead of requiring epoll
On 8/30/2023 10:11 PM, Eric Blake wrote:
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).
The problem with mio is that it uses edge triggered notifications.
According to this page <https://docs.rs/mio/latest/mio/struct.Poll.html>;:
Draining readiness
Once a readiness event is received, the corresponding operation must
be performed repeatedly until it returns variant
std::io::error::ErrorKind::WouldBlock. Unless this is done, there is
no guarantee that another readiness event will be delivered, even if
further data is received for the event source.
The motivation behind using epoll in addition to tokio in the first
place was to check if fd is readable/writable without necessarily
knowing if the last read/write operation returned
[ErrorKind::WouldBlock]. And it doesn't seems that mio full fills that
requirenment.
I tried using the polling <https://docs.rs/polling/latest/polling/>
crate, which uses epoll and kqueue under the hood. but for some reason
the disconnect call failes, at least on my Linux machine. I have know
idea what's the problem.
Another idea would be that we create our own abstraction upon epoll and
kqueue. But I am unable to test any kqueue implementation, I think that
I can not even compile it on my machine.
It would of course also be possible to disable the Rust bindings (or at
least the asynchronous API) on BSD and MacOS, but that would not be a
good solution I think.
--
Best regards,
Tage
Fixes: 223a9965 ("rust: async: Create an async friendly handle
type")
CC: Tage Johansson<tage.j.lists(a)posteo.net>
Signed-off-by: Eric Blake<eblake(a)redhat.com>
---
rust/Cargo.toml | 2 +-
rust/src/async_handle.rs | 46 +++++++++++++++++++++++++---------------
2 files changed, 30 insertions(+), 18 deletions(-)
diff --git a/rust/Cargo.toml b/rust/Cargo.toml
index 0879b34c..391c80e9 100644
--- a/rust/Cargo.toml
+++ b/rust/Cargo.toml
@@ -49,7 +49,7 @@ thiserror = "1.0.40"
log = { version = "0.4.19", optional = true }
libc = "0.2.147"
tokio = { optional = true, version = "1.29.1", default-features = false,
features = ["rt", "sync", "net"] }
-epoll = "4.3.3"
+mio = "0.8.0"
[features]
default = ["log", "tokio"]
diff --git a/rust/src/async_handle.rs b/rust/src/async_handle.rs
index ab28b910..35b1c0d2 100644
--- a/rust/src/async_handle.rs
+++ b/rust/src/async_handle.rs
@@ -35,9 +35,11 @@ use crate::sys;
use crate::Handle;
use crate::{Error, FatalErrorKind, Result};
use crate::{AIO_DIRECTION_BOTH, AIO_DIRECTION_READ, AIO_DIRECTION_WRITE};
-use epoll::Events;
+use mio::unix::SourceFd;
+use mio::{Events, Interest as MioInterest, Poll, Token};
use std::sync::Arc;
use std::sync::Mutex;
+use std::time::Duration;
use tokio::io::{unix::AsyncFd, Interest, Ready as IoReady};
use tokio::sync::Notify;
use tokio::task;
@@ -176,12 +178,12 @@ async fn polling_task(handle_data: &HandleData) ->
Result<(), FatalErrorKind> {
} = handle_data;
let fd = handle.aio_get_fd().map_err(Error::to_fatal)?;
let tokio_fd = AsyncFd::new(fd)?;
- let epfd = epoll::create(false)?;
- epoll::ctl(
- epfd,
- epoll::ControlOptions::EPOLL_CTL_ADD,
- fd,
- epoll::Event::new(Events::EPOLLIN | Events::EPOLLOUT, 42),
+ let mut events = Events::with_capacity(1);
+ let mut poll = Poll::new()?;
+ poll.registry().register(
+ &mut SourceFd(&fd),
+ Token(0),
+ MioInterest::READABLE | MioInterest::WRITABLE,
)?;
// The following loop does approximately the following things:
@@ -248,19 +250,29 @@ async fn polling_task(handle_data: &HandleData) ->
Result<(), FatalErrorKind> {
}
drop(pending_cmds_lock);
- // 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.
+ match poll.poll(&mut events, Some(Duration::ZERO)) {
+ Ok(_) => {
+ 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);
+ }
+ }
+ }
+ Err(_) => {
+ ready_guard.clear_ready_matching(IoReady::READABLE);
+ ready_guard.clear_ready_matching(IoReady::WRITABLE);
+ }
+ };
ready_guard.retain_ready();
}
}
8:41 a.m.
New subject: [libnbd PATCH 2/2] rust: Use mio::poll instead of requiring epoll
On Fri, Sep 01, 2023 at 08:35:58AM +0000, Tage Johansson wrote:
On 8/30/2023 10:11 PM, Eric Blake wrote:
> 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).
The problem with mio is that it uses edge triggered notifications. According
to this page <https://docs.rs/mio/latest/mio/struct.Poll.html>;:
> Draining readiness
> Once a readiness event is received, the corresponding operation must be
> performed repeatedly until it returns variant
> std::io::error::ErrorKind::WouldBlock. Unless this is done, there is no
> guarantee that another readiness event will be delivered, even if
> further data is received for the event source.
Would that still be true even if we deregister the interest after our
one-shot poll, and reregister it immediately before-hand? In other
words, even if registration sets up edge triggers for future
notifications, the fact that we are only doing a one-shot query seems
like it would be sufficient to use our one-shot as a level probe
(assuming that the registration starts by learning the current state
of the fd before waiting for any future edges).
I agree that once we get a notification after a .poll(), the normal
assumption is that we must loop and consume all data before calling
.poll again (because the .poll would block until the next edge - but
if we didn't consume to the blocking point, there is no next edge).
But since we are NOT looping, nor doing any consumption work, our
timeout of 0 is trying to behave as an instant level check, and if
adding a reregister/deregister wrapper around the .poll makes it more
reliable (by wiping out all prior events and re-priming the
registration to start with the current level), that may be all the
more we need.
The motivation behind using epoll in addition to tokio in the first place
was to check if fd is readable/writable without necessarily knowing if the
last read/write operation returned [ErrorKind::WouldBlock]. And it doesn't
seems that mio full fills that requirenment.
Do we need to expand the libnbd API itself to make it more obvious
whether our state machine completed because it hit a EWOULDBLOCK
scenario? In general, once you kick the state machine (by sending a
new command, or calling one of the two aio_notify calls), the state
machine tries to then process as much data as possible until it blocks
again, rather than stopping after just processing a single
transaction. That is, if libnbd isn't already designed to work with
edge triggers, how much harder would it be to modify it (perhaps by
adding a new API) so that it DOES work nicely with edge triggers?
If we can solve _that_ problem, then the Rust async handle wouldn't
need to use epoll, mio, or anything else; tokio's edge trigger
behavior would already be sufficient on its own.
I tried using the polling <https://docs.rs/polling/latest/polling/> crate,
which uses epoll and kqueue under the hood. but for some reason the
disconnect call failes, at least on my Linux machine. I have know idea
what's the problem.
Another idea would be that we create our own abstraction upon epoll and
kqueue. But I am unable to test any kqueue implementation, I think that I
can not even compile it on my machine.
If there is already a solution out there that works, let's favor that
instead of reimplementing a portability wrapper ourselves.
It would of course also be possible to disable the Rust bindings (or at
least the asynchronous API) on BSD and MacOS, but that would not be a good
solution I think.
Yeah, crippling the BSD build is less desirable, even if doing it in
the short term to get green CI is tempting.
You mentioned that the disconnect API was hanging. Is it because we
have not set up tokio to inform us when it detects that a socket has
closed? Admittedly, I haven't reproduced the hang you mentioned yet;
is it something that was easy to reproduce (for example, just remove
all the epoll code without replacement, then run 'make check')? If I
can reproduce the hang, maybe I can learn from strace where things are
getting stuck?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
12:45 p.m.
New subject: [libnbd PATCH 2/2] rust: Use mio::poll instead of requiring epoll
On 9/1/2023 3:41 PM, Eric Blake wrote:
On Fri, Sep 01, 2023 at 08:35:58AM +0000, Tage Johansson wrote:
> On 8/30/2023 10:11 PM, Eric Blake wrote:
>> 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).
>
> The problem with mio is that it uses edge triggered notifications. According
> to this page <https://docs.rs/mio/latest/mio/struct.Poll.html>;:
>
>> Draining readiness
>> Once a readiness event is received, the corresponding operation must be
>> performed repeatedly until it returns variant
>> std::io::error::ErrorKind::WouldBlock. Unless this is done, there is no
>> guarantee that another readiness event will be delivered, even if
>> further data is received for the event source.
Would that still be true even if we deregister the interest after our
one-shot poll, and reregister it immediately before-hand? In other
words, even if registration sets up edge triggers for future
notifications, the fact that we are only doing a one-shot query seems
like it would be sufficient to use our one-shot as a level probe
(assuming that the registration starts by learning the current state
of the fd before waiting for any future edges).
I agree that once we get a notification after a .poll(), the normal
assumption is that we must loop and consume all data before calling
.poll again (because the .poll would block until the next edge - but
if we didn't consume to the blocking point, there is no next edge).
But since we are NOT looping, nor doing any consumption work, our
timeout of 0 is trying to behave as an instant level check, and if
adding a reregister/deregister wrapper around the .poll makes it more
reliable (by wiping out all prior events and re-priming the
registration to start with the current level), that may be all the
more we need.
It should work. I tried to do that and it works on my machine at least.
Although it might be a bit uggly, it may be a short term solution. I
will send your patch with these modifications soon as reply to this message.
> The motivation behind using epoll in addition to tokio in the
first place
> was to check if fd is readable/writable without necessarily knowing if the
> last read/write operation returned [ErrorKind::WouldBlock]. And it doesn't
> seems that mio full fills that requirenment.
Do we need to expand the libnbd API itself to make it more obvious
whether our state machine completed because it hit a EWOULDBLOCK
scenario? In general, once you kick the state machine (by sending a
new command, or calling one of the two aio_notify calls), the state
machine tries to then process as much data as possible until it blocks
again, rather than stopping after just processing a single
transaction. That is, if libnbd isn't already designed to work with
edge triggers, how much harder would it be to modify it (perhaps by
adding a new API) so that it DOES work nicely with edge triggers?
This would be the cleanest and best solution I think. Adding two methods
to the handle: aio_read_blocked() and aio_write_blocked() which return
true iff the last read/write operation blocked. It could be two bool
fields on the handle struct which are set to false by default and reset
to false whenever a read/write operation is performed without blocking.
It is very important that aio_read_blocked() will return true only if
the last read operation blocked, (and the same thing for
aio_write_blocked()).
If we can solve _that_ problem, then the Rust async handle
wouldn't
need to use epoll, mio, or anything else; tokio's edge trigger
behavior would already be sufficient on its own.
Yes, this is true. It would be more efficient, easier to understand, and
require less dependencies.
--
Best regards,
Tage
11:07 a.m.
New subject: [libnbd PATCH] rust: Use mio::poll instead of requiring epoll
From: Eric Blake <eblake(a)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(a)posteo.net>
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
rust/Cargo.toml | 3 ++-
rust/src/async_handle.rs | 40 +++++++++++++++++++++++-----------------
2 files changed, 25 insertions(+), 18 deletions(-)
diff --git a/rust/Cargo.toml b/rust/Cargo.toml
index 0879b34..678848a 100644
--- a/rust/Cargo.toml
+++ b/rust/Cargo.toml
@@ -49,9 +49,10 @@ thiserror = "1.0.40"
log = { version = "0.4.19", optional = true }
libc = "0.2.147"
tokio = { optional = true, version = "1.29.1", default-features = false,
features = ["rt", "sync", "net"] }
-epoll = "4.3.3"
+mio = { optional = true, version = "0.8.0" }
[features]
+tokio = ["dep:tokio", "dep:mio"]
default = ["log", "tokio"]
[dev-dependencies]
diff --git a/rust/src/async_handle.rs b/rust/src/async_handle.rs
index 6793ce9..8f3e8df 100644
--- a/rust/src/async_handle.rs
+++ b/rust/src/async_handle.rs
@@ -35,9 +35,11 @@ use crate::sys;
use crate::Handle;
use crate::{Error, FatalErrorKind, Result};
use crate::{AIO_DIRECTION_BOTH, AIO_DIRECTION_READ, AIO_DIRECTION_WRITE};
-use epoll::Events;
+use mio::unix::SourceFd;
+use mio::{Events, Interest as MioInterest, Poll, Token};
use std::sync::Arc;
use std::sync::Mutex;
+use std::time::Duration;
use tokio::io::{unix::AsyncFd, Interest, Ready as IoReady};
use tokio::sync::Notify;
use tokio::task;
@@ -176,13 +178,8 @@ async fn polling_task(handle_data: &HandleData) ->
Result<(), FatalErrorKind> {
} = handle_data;
let fd = handle.aio_get_fd().map_err(Error::to_fatal)?;
let tokio_fd = AsyncFd::new(fd)?;
- let epfd = epoll::create(false)?;
- epoll::ctl(
- epfd,
- epoll::ControlOptions::EPOLL_CTL_ADD,
- fd,
- epoll::Event::new(Events::EPOLLIN | Events::EPOLLOUT, 42),
- )?;
+ let mut events = Events::with_capacity(1);
+ let mut poll = Poll::new()?;
// The following loop does approximately the following things:
//
@@ -248,19 +245,28 @@ async fn polling_task(handle_data: &HandleData) ->
Result<(), FatalErrorKind> {
}
drop(pending_cmds_lock);
- // 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))?;
+ 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))?;
}
}
base-commit: 9afb980d05d6144129c899285e44779757a380e8
prerequisite-patch-id: 8d1779610795021ed5a3d0973ddf9ef854cd6a24
prerequisite-patch-id: 1f0f333311f11ac9b1ff0be08f5aa0a9904ba4de
prerequisite-patch-id: dc4af7343c57a4f99dc82918c07470030e542747
prerequisite-patch-id: 8e8f7a043c80d6c24e883967f5bd952a64db1228
prerequisite-patch-id: ba7b3482e2e16f76b5f285daeeda30b31a841912
prerequisite-patch-id: 219a9595e550a8caf43d466dcb2b044114e1b7bf
prerequisite-patch-id: 3de46c9673221bff1d897970aa983b3f8e6cab74
prerequisite-patch-id: 4235d5e174fce05b9a947b3b838bebf968f0fa6a
prerequisite-patch-id: 07773355d5718e0593c4030a8f035fc11fea3715
prerequisite-patch-id: f023deea8b706706e3c2980403064d90a254af3c
prerequisite-patch-id: dd415a31fa127d90bcc7d993a2659e39d7d4cae8
prerequisite-patch-id: 618cc5f574207f36818ef803f2e586314c2458f5
prerequisite-patch-id: 8639c6cc4fec58f4761771c5d8a9476d538c6251
prerequisite-patch-id: 9bc660aed54a6266b014993ff0f388a26ac2982a
prerequisite-patch-id: a334eeb471214d13ea2f47604b0a5b6d86b869b6
--
2.42.0
1:23 p.m.
New subject: [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(a)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(a)posteo.net>
Signed-off-by: Eric Blake <eblake(a)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
1:59 p.m.
New subject: [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(a)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(a)posteo.net>
> Signed-off-by: Eric Blake <eblake(a)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).
9:45 a.m.
New subject: [libnbd PATCH] rust: Use mio::poll instead of requiring epoll
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
5:02 p.m.
New subject: [libnbd PATCH] rust: Use mio::poll instead of requiring epoll
On Tue, Sep 05, 2023 at 09:45:44AM -0500, Eric Blake wrote:
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.
Now in as 75979812. Still not a green CI run; but the BSD builds now
work. The only remaining holdout is OpenSUSE Leap 15.5, where OCaml is
stuck at 4.06 and therefore lacks List.to_seq |> Map.of_seq. I'm not
sure if it is worth trying to copy the 4.07 implementations just so we
can keep the two sorted name maps (one of the reasons you WANT to use
a standard library implementation of Map is that writing a correct
self-balancing binary tree implementation is not trivial). But do we
need the resulting sets of APIs to be sorted by name? If we just
stick to constructs in 4.06, we may be hit with slower O(n^2) list
filtering when comparing which APIs go in which of the two lists (sync
vs async), but is our list of APIs so large that it actually makes a
noticeable difference to compile time?
Rich, do you have any more ideas beyond Erik's recommendation [1] of
just dropping OpenSUSE 15.5 and focusing only on Tumbleweed?
[1] https://gitlab.com/nbdkit/libnbd/-/merge_requests/5#note_1535510343
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
8:09 a.m.
New subject: [libnbd PATCH] rust: Use mio::poll instead of requiring epoll
On Wed, Sep 06, 2023 at 05:02:06PM -0500, Eric Blake wrote:
> 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.
Now in as 75979812. Still not a green CI run; but the BSD builds now
work. The only remaining holdout is OpenSUSE Leap 15.5, where OCaml is
stuck at 4.06 and therefore lacks List.to_seq |> Map.of_seq. I'm not
sure if it is worth trying to copy the 4.07 implementations just so we
can keep the two sorted name maps (one of the reasons you WANT to use
a standard library implementation of Map is that writing a correct
self-balancing binary tree implementation is not trivial). But do we
need the resulting sets of APIs to be sorted by name? If we just
stick to constructs in 4.06, we may be hit with slower O(n^2) list
filtering when comparing which APIs go in which of the two lists (sync
vs async), but is our list of APIs so large that it actually makes a
noticeable difference to compile time?
Rich, do you have any more ideas beyond Erik's recommendation [1] of
just dropping OpenSUSE 15.5 and focusing only on Tumbleweed?
[1] https://gitlab.com/nbdkit/libnbd/-/merge_requests/5#note_1535510343
Thanks to stackoverflow [2], I came up with an alternative (using
List.fold_left to build up from NameMap.empty through repeated
NameMap.add, without any intermediate representation in Seq). As of
commit db9f6bf6, we finally have green CI in libnbd again!
[2] https://stackoverflow.com/questions/61220655/how-to-initialize-map-from-l...
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
11:05 a.m.
New subject: [libnbd PATCH] rust: Use mio::poll instead of requiring epoll
On Thu, Sep 07, 2023 at 08:09:33AM -0500, Eric Blake wrote:
On Wed, Sep 06, 2023 at 05:02:06PM -0500, Eric Blake wrote:
> > 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.
>
> Now in as 75979812. Still not a green CI run; but the BSD builds now
> work. The only remaining holdout is OpenSUSE Leap 15.5, where OCaml is
> stuck at 4.06 and therefore lacks List.to_seq |> Map.of_seq. I'm not
> sure if it is worth trying to copy the 4.07 implementations just so we
> can keep the two sorted name maps (one of the reasons you WANT to use
> a standard library implementation of Map is that writing a correct
> self-balancing binary tree implementation is not trivial). But do we
> need the resulting sets of APIs to be sorted by name? If we just
> stick to constructs in 4.06, we may be hit with slower O(n^2) list
> filtering when comparing which APIs go in which of the two lists (sync
> vs async), but is our list of APIs so large that it actually makes a
> noticeable difference to compile time?
>
> Rich, do you have any more ideas beyond Erik's recommendation [1] of
> just dropping OpenSUSE 15.5 and focusing only on Tumbleweed?
>
> [1] https://gitlab.com/nbdkit/libnbd/-/merge_requests/5#note_1535510343
Thanks to stackoverflow [2], I came up with an alternative (using
List.fold_left to build up from NameMap.empty through repeated
NameMap.add, without any intermediate representation in Seq). As of
commit db9f6bf6, we finally have green CI in libnbd again!
[2] https://stackoverflow.com/questions/61220655/how-to-initialize-map-from-l...
Excellent news, thanks!
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
256
days inactive
264
days old
13 comments
3 participants
participants (3)
-
Eric Blake -
Richard W.M. Jones -
Tage Johansson