Simplify the fallback logic for shared_fs

[ollie-etl]

This PR is a light refactoring, which moves the special case handling for SharedFd which falls back to synchronous file close if the runtime or driver is unavailable. It moves it from being implemented in Op, and available to all Ops, to within SharedFd, to make its purpose more clear.

Relates to #150 (doesn't close)

[ollie-etl]

@Noah-Kennedy @FrankReh This is ready for review

[FrankReh]

Does this fix a panic? Is there a reproducer for that panic or is this help for the discussion in #150? Just trying to understand the context when looking at the diffs.

[FrankReh]

Totally unrelated to this PR. Is anyone else getting confused by the src/driver directory structure?

There is an driver/op subdirectory but its not used for defining driver operations. The driver operations get their own files under src/driver but there's also bind, pool, shared_fd and util and possibly more, I can't look at the list of files in that directory and see which are ops and which aren't. When I saw the driver/close.rs mentioned in this PR, my mind when to "oh, the close of the driver". Actually it's the "close" operation the driver supports, having nothing to do with closing the driver.

And besides a driver/op/ directory, there's a driver/op.rs file.

So, I don't mean to make a big point about here. It's completely unrelated to this PR. But if either of you agree with some of this, maybe we can open an issue and discuss moving files, either to new names or even new directories.

[FrankReh]

Time to throw my hat into the ring about file descriptors.

I don't have a clear picture yet of where it will affect the model that tokio-uring creates, but soon, we should be supporting the uring direct descriptors, available since the 5.15 kernel, that can be created with the openat, openat2, or accept operations and closed with the close operation. I suspect anywhere we are using a fd that we think of as a normal file descriptor, it could also be a direct descriptor that was created by one of the ops mentioned above or that was created with the register_files uring register variant.

The liburing manpage for io_uring_register_files gives a short explanation why an app would want to use direct descriptors. Of course, it boils down to getting better performance.

[oliverbunting]

@FrankReh this PR is purely cosmetic, it gives no new functionality, and changes no behaviour. I was trying to understand the special case logic on Close, and realised the logic for it was contained in try_submit_with, which is only called in Close.

I just moved the logic into Close to make it clearer what's going on

[oliverbunting]

@FrankReh on your point if structure, yes, I think there is an argument for moving some files around. I also think driver/ops.rs is overdue splitting

[FrankReh]

this PR is purely cosmetic, it gives no new functionality, and changes no behaviour. I was trying to understand the special case logic on Close, and realised the logic for it was contained in try_submit_with, which is only called in Close.

I just moved the logic into Close to make it clearer what's going on

Thanks. That context helps. I'm for it but let's wait some for @Noah-Kennedy to free up and have a look too.

And regarding the file layout, if you also think something could be done, let's try to convince @Noah-Kennedy. My suggestion would be that we come up with a proposal ahead of time for file movement, but we get all the old PRs taken care of before we make those changes and we warn any fresh PRs coming in that files are about to change - I don't trust git to merge changes well when file names change and content changes. I mean Noah might feel its not worth changing and everything is clear in his head and over time, they will be in ours too, and the churn would be too much work for PRs we didn't resolve ahead of time. Let's see. Thanks for thinking about it.

[Noah-Kennedy]

I've been wanting to restructure the file layout for everything, but am waiting for a break in PRs to quickly move things around.

[Noah-Kennedy]

What I'm wondering is if we even want to do this, or if we just want to do a synchronous close on drop. I really don't like the whole deferred cleanup semantics this drop behavior has created.

[oliverbunting]

What I'm wondering is if we even want to do this, or if we just want to do a synchronous close on drop. I really don't like the whole deferred cleanup semantics this drop behavior has created.

I think the alternative is to tie the lifetime of the runtime to that of all Ops, or at least, ops you want to have shutdown semantics

[oliverbunting]

Because it's not just Close, it's also multi poll ops

[ollie-etl]

@Noah-Kennedy How do you feel about merging this until we get round to working out a better scheme for Close/cancellation semantics?

[Noah-Kennedy]

I'd be down with merging while we work out a better scheme.