Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add blockio-uring dependency #103

Merged
merged 1 commit into from
Mar 19, 2024
Merged

Add blockio-uring dependency #103

merged 1 commit into from
Mar 19, 2024

Conversation

jorisdral
Copy link
Collaborator

@jorisdral jorisdral commented Feb 21, 2024

Now that we depend on blockio-uring, we have to drop GHA for Windows and MacOS for now, until we have an alternative for those platforms EDIT: no longer the case, we are implementing an alternative API for Windows and MacOS

Copy link
Collaborator

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could provide an emulation and use a conditional dependency. We'll need to keep things working on Windows and OSX, but without high perf support. So it seems better to keep the CI on those platforms continuously, rather than dropping and later trying to restore support.

@jorisdral jorisdral force-pushed the jdral/blockio-uring-dep branch from cfd9039 to fa4d934 Compare February 23, 2024 11:04
@jorisdral jorisdral marked this pull request as draft February 23, 2024 14:26
@jorisdral
Copy link
Collaborator Author

We could provide an emulation and use a conditional dependency. We'll need to keep things working on Windows and OSX, but without high perf support. So it seems better to keep the CI on those platforms continuously, rather than dropping and later trying to restore support.

Okay, I'm marking the PR as draft again for now until I have this in place

@jorisdral jorisdral force-pushed the jdral/blockio-uring-dep branch 11 times, most recently from 51edeee to cec31fa Compare February 23, 2024 16:46
Copy link
Collaborator

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking plausible.

Do we need to do separate posix and windows serial implementations? Can't we do a generic one in terms of the fs-api?

Another thing I'm wondering, perhaps the HasBlockIO API could have just the submit and close operations, and the initIOCtx could be moved to ioHasBlockIO. So the idea is that given an existing HasFS we can open a new block I/O context, and then use that. The advantage of this is that we'd have fewer things to pass around when using it. We'd just need to pass the HasFs and HasBlockIO and not also need to pass the ioctx.

I'm not sure how to make the params work generically. It's all very well to make it a type param and have a default, but if one wants to use a non-default value then it's not possible to write that generically. Hmm.

fs-api-blockio/src/System/FS/BlockIO/IO.hs Outdated Show resolved Hide resolved
@jorisdral
Copy link
Collaborator Author

jorisdral commented Feb 26, 2024

Looking plausible.

Do we need to do separate posix and windows serial implementations? Can't we do a generic one in terms of the fs-api?

We can have a single implementation, and I was planning on unifying the two this once I was sure everything was compiling on each of the OS's. And, fs-api doesn't provide buffer versions of reading/writing, so we can't implement it generically in terms of fs-api (yet). This PR includes the necessary preadBuf and pwriteBuf functions that I'll upstream to fs-api

Another thing I'm wondering, perhaps the HasBlockIO API could have just the submit and close operations, and the initIOCtx could be moved to ioHasBlockIO. So the idea is that given an existing HasFS we can open a new block I/O context, and then use that. The advantage of this is that we'd have fewer things to pass around when using it. We'd just need to pass the HasFs and HasBlockIO and not also need to pass the ioctx.

Sounds like a good idea!

I'm not sure how to make the params work generically. It's all very well to make it a type param and have a default, but if one wants to use a non-default value then it's not possible to write that generically. Hmm.

Maybe it's fine to just use the one from blockio-uring, and ignore it in the serial case

@dcoutts
Copy link
Collaborator

dcoutts commented Feb 26, 2024

And, fs-api doesn't provide buffer versions of reading/writing, so we can't implement it generically in terms of fs-api (yet).

Ahh yes, good point. Yes upstreaming that makes sense. And indeed if we add that, it'll make some of our I/O conversion issues easier, e.g. where we want to read into a (Mutable)ByteArray directly.

Maybe it's fine to just use the one from blockio-uring, and ignore it in the serial case

Seems ok to me.

@jorisdral jorisdral force-pushed the jdral/blockio-uring-dep branch 4 times, most recently from 083b2f3 to 8430025 Compare March 15, 2024 14:16
@jorisdral jorisdral marked this pull request as ready for review March 15, 2024 14:16
@jorisdral
Copy link
Collaborator Author

Okay, opened for review again. It's not completely final, but I'd like your input before I continue. In particular, I wonder if the serial implementation makes sense.

Copy link
Collaborator

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great.

You asked about the serial impl. I think we can actually simplify that. See below.

Other than that I think this is good to go.

fs-api-blockio/src-linux/System/FS/BlockIO/Async.hs Outdated Show resolved Hide resolved
fs-api-blockio/src/System/FS/BlockIO/Serial.hs Outdated Show resolved Hide resolved

data IOCtx m = IOCtx { ctxFS :: SomeHasFS m, openVar :: MVar m CtxState }

data CtxState = Open Word | Closing Word | Closed
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think I see what is going on here.

So we're trying to implement the behaviour specified in the API:

Using 'submitIO' after 'close' should thrown an 'FsError' exception.

And we're doing that here by counting to track exactly when a IOCtx finally gets closed, so we can throw exceptions.

I think we can simplify. We're currently tracking this closing state, and then having submitIO fail if the ctx is closed by the time submitIO is finishing up. This is a bit odd. Firstly note that it involves submitIO and close being called concurrently. The API comments don't say what happens in this case, but in particular doesn't require that it cause a failure. Consider also what the blockio-uring one does: it make close prevent new submissions, and blocks until existing submissions have completed. So existing submissions are not made to fail.

We can simplify if we follow the latter behaviour: not requiring existing submissions fail, just failing new submissions after close.

In the serial case there's no actual cleanup that needs to happen since there are no extra resources. The only thing we want to do is catch incorrect uses of close vs submitIO. For that I think we only need an MVar Bool to distinguish the closed / not-closed state. We would check that on submitIO and set it in close.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And we're doing that here by counting to track exactly when a IOCtx finally gets closed, so we can throw exceptions.

New submitIO will throw an error on a Closing or Closed context. Existing ones don't throw an error, but we do record when all of them are done by switching from Closing to Closed.

I do agree we only need a Bool, and that the CtxState is keeping track of more info then we need

Copy link
Collaborator

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM.

@dcoutts
Copy link
Collaborator

dcoutts commented Mar 19, 2024

Squash and merge!

Changes include:
* Add blockio-uring dependency
* Add an abstract API that captures the file
operations from `blockio-uring`.
* Implementations of this for three different
operating systems: Linux, MacOS, or Windows. The
Linux implementation uses `blockio-uring` and
benefits from async IO. MacOS and Windows use a
simple implementation that performs file I/O
sequentially instead of in asynchronous batches.
* Implement some basic tests for the API.
@jorisdral jorisdral force-pushed the jdral/blockio-uring-dep branch from 7934c04 to 57bb4f0 Compare March 19, 2024 18:22
@jorisdral jorisdral enabled auto-merge March 19, 2024 18:22
@jorisdral jorisdral added this pull request to the merge queue Mar 19, 2024
Merged via the queue into main with commit 2e6b874 Mar 19, 2024
10 of 11 checks passed
@jorisdral jorisdral deleted the jdral/blockio-uring-dep branch March 19, 2024 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants