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

Implemented asynchronous IO #210 #307

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

Corillian
Copy link

@Corillian Corillian commented Aug 3, 2016

The work on this PR continues in the Corillian-asyncio-branch in this repository and will eventually become Dokan 2.0.

This PR was a tremendous amount of work and more or less consists of a full rewrite of dokan.dll. It will need a significant amount of testing before it's ready to be merged into master. We really need to add a large number of unit tests. Notable issues:

  1. Significant changes to the Dokan API
  2. FILE_OPEN_FOR_BACKUP_INTENT in create.c has been removed for now so that it can be more thoroughly investigated
  3. Initialization is been moved outside of DLL_PROCESS_ATTACH and DLL_PROCESS_DETACH to DokanInit() and DokanShutdown(). This solves the problem of missing DLL's in DLL_PROCESS_DETACH and more importantly allows the user to hook into Dokan memory allocations.

Lastly I still need to profile the driver for an optimization pass.

Keith Newton added 10 commits May 9, 2016 14:07
More or less rewrote all of dokan.dll to support async IO.
Rewrote the Mirror example to support the new Dokan API. Still need to add async IO support.
User-mode drivers can reeturn STATUS_PENDING to indicate they are performing an async operation at which point they are responsible for calling EndDispatch*() to notify the driver that the operation has completed.
Still some known issues that need to be fixed.
Close/Cleanup no longer gets called if Create fails and no user context is supplied
Renamed EndDispatch*() functions to DokanEndDispatch*() since they're public
The DOKAN_VECTOR API is now exported from dokan.dll
Made critical sections a bit easier to identify
Added missing exports
Added DokanInit() and DokanShutdown()
FIxed a bunch of memory leaks caused by the Close handler
@Corillian
Copy link
Author

Also FUSE hasn't been tested at all.

@Liryna
Copy link
Member

Liryna commented Aug 3, 2016

Thank you @Corillian for the pull request!

About unit test there is already some running on appveyor. A big par of the test seems to fail, can you take a look at them?

@Corillian
Copy link
Author

Compiling FUSE under certain circumstances seems to have failed because the Windows thread pool headers couldn't be located. I don't see any unit tests listed anywhere in the appveyor - only building.

@Liryna
Copy link
Member

Liryna commented Aug 3, 2016

@Corillian
Copy link
Author

Ah ok, cool, thanks! I'll take a look on Monday.

Keith Newton added 10 commits August 8, 2016 14:55
Added a unit tets project so that we can port winfstest to it instead of having a separate unit tests repo.
This is a better fix for the previous Mirror fix
Access tokens now get passed to the user mode driver for SetFileSecurity and CreateFile
Fixed an issue in Mirror with readonly files not returning access denied in CanDelete
Added unit tests for file streams
Fixed a bunch of x86 warnings
@Corillian
Copy link
Author

Corillian commented Aug 10, 2016

Ok nearly all of the unit tests are passing. In WinFSTest I get a couple of failures for the regular Windows filesystem however it looks with Mirror I have 2 extra failures. FSX seems to be intermittently failing on appveyor, I can only assume it has something to do with async IO however I haven't been able to reproduce the failures on my system yet.

I'll hopefully finish up these last couple of issues next week Monday.

@Liryna
Copy link
Member

Liryna commented Aug 10, 2016

👍 This begin to looks very good !
Can't wait to review it ^^ !

@Liryna
Copy link
Member

Liryna commented Aug 18, 2016

@Corillian Just wanted to say that I seen you are creating a project based on some WinFSTest .
I think this is a good idea and it will be more easy to add test from issue reported by users like this !

I will try to give you a hand on it and move all test to the project you created (if you think it is also a good idea).
This is in the long time TODO !

@Daniel-Abrecht
Copy link
Contributor

I've compiled fuse-nfs with the v2.0.0-BETA1 version, just to see if it works or not. It turned out the -s option to disable mutithreading in fuse does no longer work in dokanys fuse wrapper. Single threading is often prefered in programs which use fuse, because it prevents any race conditions, keeps things simpler, doesn't require all used libraries to be thread safe and isn't really slower for a lot of use cases. Fuse-nfs and probably a lot of other programs which use fuse relay on its single thread mode and can't be used without it.

@Corillian
Copy link
Author

Corillian commented Oct 17, 2017

Are you suggesting that every single file system event should be serialized to a single thread?

@Daniel-Abrecht
Copy link
Contributor

Yes, but only if the program which uses fuse requests the single thread mode. The current fuse wrapper tries to do this here:

dokanOptions->ThreadCount = mt ? FUSE_THREAD_COUNT : 1;

Alternatively, it would probably be sufficent to just use a mutex lock to prevent two fuse callbacks to be called at the same time. Or if the single thread mode won't be supported anymore, there should at least be a warning if a program attemps to use it.

@Corillian
Copy link
Author

Corillian commented Oct 18, 2017

Excluding debugging I can't think of any reason why you would want to do that regardless of whether or not you're using FUSE. It would completely destroy the performance of your filesystem. That being said, and as you suggested, there's nothing preventing you from forcing your FUSE driver from being single threaded by taking a lock on a mutex or serializing filesystem events to a queue.

@Daniel-Abrecht
Copy link
Contributor

I already gave a lot of reasons way a lot of fuse file systems prefer single thread mode over multithreaded mode. There are also a lot of cases where not using multithreading can actually increase the overall system performance. Let me give you an example: Let's assume we have a network file system which uses a single socket to transfer datas. In that case, the network would be the bottle-neck, and having multiple threads which try to send data and/or synchronize some actions just wastes cpu time other processes could have used.

However, I'm not interested in the single thread mode because of it's possible bennefits and drawbacks.
The interesting thing about fuse is that it's an api that allows a program to provide a file system and that program will work on any system which has an implementation of the fuse api. The single user mode of fuse is a basic feature every implementation of fuse provides and many programs rely on. Reimplementing this feature in an existing fuse file system which previously relayed on the fuse implementation to provide it is somthing I would never do, it wouldn't make any sense.

Just to make this clear, this isn't about just adding a feature I think may be neat, this is about maintaining compatiblity with existing programs and the fuse api itself.

@Corillian
Copy link
Author

The examples you have provided would be much less performant on a single thread instead of using multiple threads - particularly for a networked filesystem. From a performance perspective there are no benefits to using a single thread which is why Windows uses APC's to talk to the kernel driver in the first place. I'm not familiar with the gritty details of building a filesystem for Linux however, from what I remember of working on the FUSE API for this PR, it appears to be poorly designed because each call to the FUSE API must block a thread until the requested operation completes (unless you implement coroutines). If what you are saying was true Windows wouldn't support APC's, overlapped IO, or IO completion ports and Linux wouldn't support polling.

ThreadCount is no longer used because Dokan's user mode driver now uses the system thread pool which creates and destroys threads on the fly based on the number of CPU's in the system along with other heuristics. I probably should have just removed it. If FUSE supports forcing everything to run through a single thread and a flag was added to enforce such behavior in Dokan for compatibility purposes I certainly wouldn't complain - though you'd be crazy to use it in production.

@Liryna
Copy link
Member

Liryna commented Oct 19, 2017

Well point @Daniel-Abrecht , I forgot about the single thread option of FUSE.
@Corillian crazy as it is, I already see a couple of FUSE fs using the single thread option because of shared resources... 😢 Will need to add a way to limit the thread pool to 1 for compatibility.
People already having their FUSE FS are not always willing to change it for windows even if it is a simple mutex to add.

@ghost
Copy link

ghost commented Oct 19, 2017

I don't currently use dokany, but am keeping a close eye on it, particularly on asynchronous IO and on the hopeful implementation of the FUSE lowlevel API as this would make the eventual porting of my application a whole lot easier.

My application currently uses a single thread and is centered around the libuv event loop. Since it is a networked filesystem, and the socket is the bottleneck, additional threads don't add any extra performance. Rather, they'd take more resources and slow things down due to the necessity of working with mutexes.

The asynchronous IO feature is really neat, but forcing people to use multiple threads is - IMHO - silly.

@lostmsu
Copy link
Contributor

lostmsu commented Dec 1, 2017

Just came here for the first time. Is this pull request still up to date? Or was it reworked in some way? E.g. where can I find the diff.

@Liryna
Copy link
Member

Liryna commented Dec 1, 2017

@lostmsu The PR has been updated here:
https://github.com/dokan-dev/dokany/tree/Corillian-asyncio

Only 11 commit missing compared to master. Will do the update during the weekend 👍

@Liryna
Copy link
Member

Liryna commented Feb 2, 2018

@timofonic This PR is keep as a reference of Corillian since he is lacking of time to continue.

However, dokany has his own branch updated with last changes.
Also i keep the pr open for the history and discussion.

But you are perhaps right, i should rename the branch for v2 and create separate issue for the v2 and roadmap! :)

@Liryna Liryna mentioned this pull request Apr 14, 2018
5 tasks
Liryna added a commit that referenced this pull request Dec 29, 2021
… events

This is highly based on #307 without the async logic. The reason is to avoid using the kernel NotificationLoop that was dispatching requests to workers (userland thread) sequentially. It has a high cost to do a thread switch each time to wake up the workers.
The current logic is nice when you want workers to be async but as we have threads (and now a thread poll) dedicated to pull and process events, we can make them synchronously wait for new events and take them from the request list themself.
This commit introduces the logic of a single main thread that polls events by batch and dispatches them to other threads and keeps the last one for itself to process and send the response to the kernel. Each thread waken will do the same and pull new events at the same time. If none is returned, the thread goes back to sleep and otherwise does the same as the main thread (dispatch and process).
The difference is that the main thread waits indefinitely for new events while others wait 100ms in the kernel before returning back to userland.This and the memory poll also added, offer a great flexibility of resources especially on heavy load.

This CL is huge but the perf that comes with it are about 10-35% on sequential requests but in the real world with the thread poll the perf are way above.
@Corillian full rewrite of FindFiles improved the perf between 100-250%...
Liryna added a commit that referenced this pull request Dec 30, 2021
… events

This is highly based on #307 without the async logic. The reason is to avoid using the kernel NotificationLoop that was dispatching requests to workers (userland thread) sequentially. It has a high cost to do a thread switch each time to wake up the workers.
The current logic is nice when you want workers to be async but as we have threads (and now a thread poll) dedicated to pull and process events, we can make them synchronously wait for new events and take them from the request list themself.
This commit introduces the logic of a single main thread that polls events by batch and dispatches them to other threads and keeps the last one for itself to process and send the response to the kernel. Each thread waken will do the same and pull new events at the same time. If none is returned, the thread goes back to sleep and otherwise does the same as the main thread (dispatch and process).
The difference is that the main thread waits indefinitely for new events while others wait 100ms in the kernel before returning back to userland.This and the memory poll also added, offer a great flexibility of resources especially on heavy load.

This CL is huge but the perf that comes with it are about 10-35% on sequential requests but in the real world with the thread poll the perf are way above.
@Corillian full rewrite of FindFiles improved the perf between 100-250%...
@Liryna
Copy link
Member

Liryna commented Jan 3, 2022

This is now partially merged in 2.0.0. the only remaining part I see that could be interesting to merge is the possibility for userland to return status pending and call the dokan API when the operation can be completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants