Fixes hang in DeviceIoControl

[WorkingRobot]

Fixes #10

[WorkingRobot]

bump

[extratype]

If you can't wait you may build winspd[-x64].dll and put it in the same directory containing your binary, for example, C:\Program Files (x86)\WinSpd\bin.

[extratype]

Just to note: the public API SpdStorageUnitSendResponse always creates an event object. The user can't reuse it.

[FlorianFreudiger]

@billziss-gh Sorry for the bump but could this be merged? Or is there anything we can do to help?

While including a winspd dll built from this pull request like @extratype suggested works perfectly fine it would be easier to have an official version fixing #10, especially for new developers building upon this project.

[billziss-gh]

@FlorianFreudiger there are a few different reasons why I have not accepted this patch:

• I have never seen this problem myself, despite considerable effort to reproduce at the time. For this reason I do not understand what the problem is or why the solution works. In general I avoid accepting solutions that I do not understand why they work.

• From my understanding of how the Windows I/O Manager works, this patch should be unnecessary. The reasoning is explained in the comment above the call to DeviceIoControl:

  Our DeviceHandle is opened with FILE_FLAG_OVERLAPPED, but we call
  DeviceIoControl with a NULL Overlapped parameter, which the MSDN
  explicitly warns against. Why do we do this and why does it work?

  The reason that this works despite MSDN warnings is that we know
  that our kernel driver handles IOCTL_MINIPORT_PROCESS_SERVICE_IRP
  in a synchronous manner and never returns STATUS_PENDING for it.
  This means that the OVERLAPPED structure special processing never
  comes into play for our DeviceIoControl calls and it is safe to
  call DeviceIoControl without an Overlapped structure.

  The next obvious question: what is the beneft of FILE_FLAG_OVERLAPPED
  then? To answer this consider that Windows serializes all calls for
  handles that are not open with FILE_FLAG_OVERLAPPED by acquiring an
  internal file lock. This effectively means that without the
  FILE_FLAG_OVERLAPPED flag, there can only be one DeviceIoControl active
  at a time, which is not something we want. (Our kernel driver I/O queue
  can very efficiently manage waiting threads and the above mentioned
  file lock serialization limits our performance.) By specifying the
  FILE_FLAG_OVERLAPPED flag we ensure that the unproductive serialization
  does not happen.

  Now the above rationale assumes that the IOCTL_MINIPORT_PROCESS_SERVICE_IRP control code is always handled in a synchronous manner and that the driver handling it never returns STATUS_PENDING for it. This is certainly true of the winspd driver, but is it true of the storport driver that sits in between the kernel and winspd? I don't know for sure and perhaps this is where the problem that causes the original bug lies.

• This patch changes the (low-level) API of WinSpd. (Note that the prototype of SpdIoctlTransact changes in the patch.) Perhaps this the right thing to do here as this project is still in Beta.

• This patch only addresses the DeviceIoControl calls done by SpdIoctlTransact. It does not address other DeviceIoControl calls done on the same HANDLE (e.g. SpdIoctlProvision, SpdIoctlUnprovision, etc.)

[FlorianFreudiger]

@billziss-gh Thank you for the detailed response!
Unfortunately I am not familiar with the Windows I/O systems or the low-level API of WinSpd so I can not explain why or how this pull request fixes the issue for me.

As for reproducing the issue I have posted a comment in #10 stating my affected code and some benchmarks which may help trying to reproduce the problem

[Johno-ACSLive]

@billziss-gh I am also having this issue. I've not tested the patch however there is an easy way to reproduce. Create an image and format it as NTFS. Load up a bunch of files into the virtual disk. Mount the image and use R-Studio data recovery in Demo mode (www.r-tt.com). Select the WinSpd disk in R-Studio and Scan it. This is how I've run into the problem.

[Johno-ACSLive]

I've tested the patch now and can confirm using my testing method above the issue is now resolved.