4

My company is developing a "fancy" USB Mass Storage Device running under Windows 7. The Mass Storage Client Driver that handles the reading and writing to the actual storage media on the client side is being written in C++. The problem we are having is very, very slow write speeds. About 30 times slower than expected. We are using calls to WriteFile() to write blocks of data to the storage media (specifically the physical drive 'PhysicalDrive2') as they are received from the Host device. I have read in many other forums that people have experience very slow write speeds using WriteFile() especially on Windows 7. So I am trying to figure out if I am using the best method and function calls for this particular task.

Below are two blocks of code. One for LockVolume() function that gets called one time by the program during initialization and actually just unmounts the volume. The other block of code is WriteSector() which is used to write the actual data to the physical drive when its received by the USB Client controller driver. I am hoping that someone can shed some light on what I might be doing wrong or provided suggestions on a better way to implement this.

    short WriteSector
       (LPCWSTR _dsk,    // disk to access
       char *&_buff,         // buffer containing data to be stored
       unsigned int _nsect,   // sector number, starting with 0
       ULONG    Blocks
       )
{
    DWORD bytesWritten;   
    wchar_t errMsg[256];

    //attempt to get a handle to the specified volume or physical drive
    HANDLE hDisk = CreateFile(_dsk, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, 0, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, 0);

    //make sure we have a handle to the specified volume or physical drive
    if(hDisk==INVALID_HANDLE_VALUE) 
    {  
        FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, GetLastError(),
                      MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), errMsg, 255, NULL);    
        OutputDebugString(errMsg);
        printf("Error attempting to get a handle to the device! (%s)\n", errMsg);
        goto exit;
    }

    // set pointer to the sector on the disk that we want to write to
    SetFilePointer(hDisk, (_nsect * SIZE_OF_BLOCK), 0, FILE_BEGIN); 

    //write the data
    if (!WriteFile(hDisk, _buff, (Blocks * SIZE_OF_BLOCK), &bytesWritten, NULL))
    {
        FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, GetLastError(),
                        MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), errMsg, 255, NULL);  
        printf("WriteFile failed! (%s)\n", errMsg);
        goto exit;
    }

exit:
    CloseHandle(hDisk);

    writeMutex.unlock();

    return 0;
}

    UINT Disk_LockVolume(LPCWSTR _dsk)
{       
    HANDLE hVol;
    LPWSTR errMsg;
    DWORD status;
    bool success = false;

    //now try to get a handle to the specified volume so we can write to it
    hVol = CreateFile(_dsk, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, 0, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, 0);

    //check to see if we were able to obtain a handle to the volume
    if( hVol == INVALID_HANDLE_VALUE )
    {
        FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, GetLastError(),
                      MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), errMsg, 255, NULL);    

        printf("Disk_LockVolume() - CreateFile failed (%s)\n", errMsg);
        goto exit;
    }

    // now lock volume
    if (!DeviceIoControl(hVol, FSCTL_LOCK_VOLUME, NULL, 0, NULL, 0, &status, NULL))
    {
        FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, GetLastError(),
                      MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), errMsg, 255, NULL);    
        printf("Disk_LockVolume() - Error attempting to lock device!  (%s)\n", errMsg);
        goto exit;
    }

    //dismount the device 
    if (!DeviceIoControl(hVol, FSCTL_DISMOUNT_VOLUME, NULL, 0, NULL, 0, &status, NULL))
    {
        FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, GetLastError(),
                      MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), errMsg, 255, NULL);    
        printf("Disk_LockVolume() - Error attempting to dismount volume.  (%s)\n", errMsg);
        goto exit;
    }


exit:
    CloseHandle(hVol);

    return 1;
}

EDIT #1 (2/10/2015)

So I incorporated the suggestions made by Ben Voigt and found that calling CreateFile and CloseHandle only once (instead of every time we want to write data to the drive) significantly improved write speed. 80% increase. Even with that increase, the write speed was still much slower than expected. Around 6 times slower. So I then incorporated his other suggested change which included eliminating the original call to SetFilePointer() and replace it with and OVERLAPPED structure that now gets passed to WriteFile. After I made that change, I now get and error that states "stack around the variable 'MyOverLappedStructure' was corrupted". Below is the updated version of my SectorWrite function along with the new Disk_GetHandle() function which gets the initial handle to the Physical Drive. Also, I am still calling Disk_LockVolume(), one time, after I call Disk_GetHandle(). However, I have modified the Disk_LockVolume() function so that the handle to the volume (in this case) does not get closed at the end of the function. Ultimately that would be closed at the end of the program, prior to closing the handle on the Physical Drive. Any thoughts on this new error would be greatly appreciated. Oh, the FILE_FLAG_NO_BUFFERING had no impact on performance that I could see.

UINT WriteSector(HANDLE hWriteDisk, PBYTE   Buf, ULONG   Lba, ULONG Blocks)
{       
    DWORD bytesWritten;
    LPTSTR errMsg = "";

    //setup overlapped structure to tell WriteFile function where to write the data
    OVERLAPPED overlapped_structure;
    memset(&overlapped_structure, 0, (Blocks * SIZE_OF_BLOCK));
    overlapped_structure.Offset = (Lba * SIZE_OF_BLOCK);
    overlapped_structure.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);


    //write the data
    if (!WriteFile(hWriteDisk, Buf, (Blocks * SIZE_OF_BLOCK), &bytesWritten, &overlapped_structure))
    {
        FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, GetLastError(),
                      MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), errMsg, 255, NULL);    
        printf("WriteSector() - WriteFile failed (%s)\n", errMsg);
    }

    if (bytesWritten != (Blocks * SIZE_OF_BLOCK))
    {
        printf("WriteSector() - Bytes written did not equal the number of bytes to be written\n");
        return 0;
    }
    else
    {
        return Blocks;
    }
}

HANDLE Disk_GetHandle(UINT Lun)
{       
    HANDLE hVol;
    LPTSTR errMsg = "";
    bool success = false;

    //now try to get a handle to the specified volume so we can write to it
    hVol = CreateFile(MassStorageDisk[Lun].PhysicalDisk, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, 0, OPEN_EXISTING, FILE_FLAG_OVERLAPPED|FILE_FLAG_NO_BUFFERING, 0);

    //check to see if we were able to obtain a handle to the volume
    if( hVol == INVALID_HANDLE_VALUE )
    {
        FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, GetLastError(),
                      MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), errMsg, 255, NULL);
        printf("Disk_WriteData() - CreateFile failed (%s)\n", errMsg);
    }

    return hVol;
}

EDIT #2 (2/10/2015)

So I eliminated the FILE_FLAG_OVERLAPPED from CreateFile() call per Ben's comment. I also modified part of the WriteSector() function to include a check to see if IO is pending after call to WriteFile(). If so, I call WaitForSingleObject() which waits indefinitely until the IO operation is complete. Lastly, I call CloseHandle() on the OVERLAPPED structure hEvent. Even with these changes I still get the error "stack around the variable 'osWrite' was corrupted", where osWrite is the OVERLAPPED structure. Below is a code snippet illustrating the changes.

OVERLAPPED osWrite;
memset(&osWrite, 0, (Blocks * SIZE_OF_BLOCK));
osWrite.Offset = (Lba * SIZE_OF_BLOCK);
osWrite.hEvent = 0;


//write the data
if (!WriteFile(hWriteDisk, Buf, (Blocks * SIZE_OF_BLOCK), &bytesWritten, &osWrite))
{
    DWORD Errorcode = GetLastError();
    if (Errorcode == ERROR_IO_PENDING)
    {
        WaitForSingleObject(osWrite.hEvent, INFINITE);
    }
    else
    {
        FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, GetLastError(),
                      MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), errMsg, 255, NULL);    
        printf("WriteSector() - WriteFile failed (%s)\n", errMsg);
        goto exit;
    }
}

EDIT #3 (2/10/2015)

So the code is now working with Ben's inputs. The code above has been modified to reflect those changes. I need to mention that up until this afternoon, all of my testing was done where the storage media on the client side was a USB flash drive. I have since changed that so the client now writes to an attached SSD. With the USB flash drive setup, the speed at which I can write data to the client over the USB connection is virtually identical now to the speed at which the client SBC can transfer the same file directly from itself to the storage media (without the host connected). However, with the SSD now being used, this is not the case. The test file I am using which is 34MB takes 2.5 seconds when transferred directly from the client SBC to the SSD. It takes 2.5 MINUTES from host to client over the USB. Other than changing the volume letter and physical drive number, no other changes were made to the code

Pungo120
  • 105
  • 1
  • 13
  • The biggest problem I see is that you're potentially writing non-sequentially to a newly created file without using `SetFileValidData()`. That will cause the OS to zero the entire file up to the point you are writing to. – Mysticial Feb 06 '15 at 22:49
  • Secondly, if you're gonna be doing this low level stuff, you might want to consider completely bypassing the Windows disk buffering by using `FILE_NO_BUFFERING`. All of the Windows cache policies have a tendency to suck under a lot of situations. – Mysticial Feb 06 '15 at 22:52
  • @Mysticial: What newly created file? I agree with disabling buffering, though. – Ben Voigt Feb 06 '15 at 23:22
  • @BenVoigt I got misled by the call to `CreateFile`. :) – Mysticial Feb 07 '15 at 00:18
  • FILE_FLAG_NO_BUFFERING had no impact on write speed – Pungo120 Feb 10 '15 at 14:00
  • 1
    I didn't actually intend for you to start using FILE_FLAG_OVERLAPPED, which makes the operation complete asynchronously. That's why you have memory corruption. If you remove that flag, then the OVERLAPPED structure is needed only during the WriteFile call. With the flag, you must arrange for the OVERLAPPED structure to exist until the write completes. – Ben Voigt Feb 10 '15 at 14:32
  • @Ben - See Edit #2. Still getting error message. – Pungo120 Feb 10 '15 at 15:01
  • 1
    Umm, that memset is totally wrong. Use either `memset(&osWrite, 0, sizeof MyOverlapped);` or just `OVERLAPPED osWrite = { 0 };` – Ben Voigt Feb 10 '15 at 15:03
  • @Ben - Thanks! So I changed the memset to `memset(&osWrite, 0, sizeof(osWrite));` and now the program runs without errors. However, write speed is actually a little slower than it was after Edit #1. Any other thoughts or suggestions are appreciated. Thanks for all your help thus far! – Pungo120 Feb 10 '15 at 15:22
  • 1
    Now if you are not using FILE_FLAG_OVERLAPPED you don't need the event either. – Ben Voigt Feb 10 '15 at 15:24
  • @Ben - Done! Set hEvent to 0 and eliminated call to CloseHandle() for hEvent. No change in write speed however. – Pungo120 Feb 10 '15 at 15:43
  • OK what speed are you seeing, what speed are you expecting (and why), how big are the transfers? Are you measuring time per transaction or average throughput? From the device code or the usb host? – Ben Voigt Feb 10 '15 at 16:51
  • @BenVoigt - See Edit #3 – Pungo120 Feb 10 '15 at 20:47

1 Answers1

5

You should not call CreateFile and CloseHandle for each sector overwritten. CreateFile is a very expensive operation that has to do security checks (evaluate group membership, walk SIDs, etc).

Open the handle once, pass it to WriteFile many times, and close it once. This means changing your _dsk parameter from a volume path to a HANDLE.

You probably also want to lose the call to SetFilePointer, and use an OVERLAPPED structure instead, which lets you supply the position to write to as part of the write call. (The operation won't be overlapped unless you use FILE_FLAG_OVERLAPPED, but non-overlapped I/O respects the position information in the OVERLAPPED structure).

Ben Voigt
  • 277,958
  • 43
  • 419
  • 720
  • Given that this code is being used in a USB Mass Storage Client application, I am wondering what the best trigger would be for closing the handle to the physical disk. What if the user just unplugs the USB device without doing a safe removal (i.e. eject device). – Pungo120 Feb 09 '15 at 14:50
  • @Pungo are you bus powered? Anyway, users should know that safe removal is there for a reason and failure to use it can result in data loss. – Ben Voigt Feb 09 '15 at 14:57
  • A few things... It is not bus powered, but I think I have a solution for that concern. I have edited my original post with some comments regarding implementing your suggestions. Any additional thoughts or inputs would be appreciated. – Pungo120 Feb 10 '15 at 13:57