Add posixaio_waitcomplete engine. #1266
Conversation
engines/posixaio.c
Outdated
| static struct io_u * | ||
| io_u_from_aiocb(struct aiocb *aiocb) | ||
| { | ||
| return (struct io_u *)((char *)aiocb - (offsetof(struct io_u, aiocb))); |
There was a problem hiding this comment.
container_of()? That'd be way more readable ;-)
|
Overall looks fine, just a few notes:
|
|
Actually, the style issue I was going to mention goes away when you kill io_u_from_aiocb(). |
macdice
force-pushed
the
aio_waitcomplete
branch
2 times, most recently
from
bce7dd2
to
b24866d
Compare
|
(Sorry for the force pushes, not too used to github workflow and didn't know that'd show up like that. :-)) Thanks for the feedback! I was writing up the docs and realised there is a better way to spin this: it's kinda sorta arguably a bug that posixaio doesn't respect iodepth_batch_complete_max. Let's just make posixaio_waitcomplete behave "normally", and add an option for posixaio that opts into a behaviour change to make it more like other engines. Hence, split into two commits. What do you think? |
|
I’m used to it as the person pulling, I don’t use gh much as a contributor. I just wait until the person tells me it’s now good to go :-)
Splitting into two commits seems sane, I generally always prefer splitting if it makes any sense to do so.
… On Aug 21, 2021, at 10:41 PM, Thomas Munro ***@***.***> wrote:
(Sorry for the force pushes, not too used to github workflow and didn't know that'd show up like that. :-))
Thanks for the feedback! I was writing up the docs and realised there is a better way to spin this: it's kinda sorta arguably a bug that posixaio doesn't respect iodepth_batch_complete_max. Let's just make posixaio_waitcomplete behave "normally", and add an option for posixaio that opts into a behaviour change to make it more like other engines. Hence, split into two commits. What do you think?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Now on laptop and looking at it, I think the best approach is to just have this be an option for the posixaio engine, if the platform supports the waitcomplete mode. Don't think a an extra engine makes a lot of sense, even if they end up having two separate getevents implementations. Just have a parent one that picks the right one. Provide an option for the posixaio engine that selects which mode to use, and just error if waitcomplete is specified and it isn't available. It'd be nice to include some performance numbers from the commit enabling waitcomplete in the commit message. |
|
@macdice any follow up on this one? |
|
Sorry for the delay: I will post a new patch this weekend coming, with an option as requested. |
The posixaio engine prevously ignored iodepth_batch_complete_max and polled the whole set of in flight IOs. This might be a little confusing when comparing with other engines. Therefore, provide a new option --posixaio_respect_iodepth_batch_complete_max. Not enabled by default, so as not to change any results unexpectedly. Signed-off-by: Thomas Munro <[email protected]>
Provide an option to use FreeBSD's aio_waitcomplete() function to wait for completions, instead of aio_suspend(). Not enabled by default. Signed-off-by: Thomas Munro <[email protected]>
macdice
force-pushed
the
aio_waitcomplete
branch
from
b24866d
to
99eea02
Compare
|
Done. Seems quite nice this way. BTW I plan to propose a couple more values for posixaio_wait, in later PRs (sigwaitinfo and kevent). |
sitsofe
removed
the
needreporterinfo
Provide a variant of the posixaio engine that uses FreeBSD's
aio_waitcomplete() function to consume completions, instead of running
around polling all IOs with aio_error().
To preserve the traditional posixaio behaviour and for fair comparison
with it, it drains as many extra completions as it can without waiting.
To disable that and drain just the minimum number as some other engines
do, there's a new option --posixaio-drain-min; this might keep the IO
queue depth more stable/full.