List:Commits« Previous MessageNext Message »
From:Chris Powers Date:April 7 2008 5:32pm
Subject:Re: bk commit into 6.0 tree (vvaintroub:1.2627) WL#3998
View as plain text  
Wlad,

Comments below.
vvaintroub@stripped wrote:
[...]
> ChangeSet@stripped, 2008-04-07 15:49:58+02:00, vvaintroub@wva. +2 -0
>   WL#3998 - Falcon positioned IO.
>   Use the native windows functionality to do positioned IO 
>   (pass the offset to ReadFile/WriteFile in the OVERLAPPED parameter)

The file is not opened with FILE_FLAG_OVERLAPPED, we should therefore assume write
operations will completed upon return, i.e. a synchronous write, correct?

>   This allows to eliminate SetFilePointerEx/SyncObject locks around IOs.
> 
>   storage/falcon/IO.cpp@stripped, 2008-04-07 15:49:54+02:00, vvaintroub@wva. +25 -0
>     WL#3998 - Falcon positioned IO.
>     Use the native windows functionality to do positioned IO 
>     (pass the offset to ReadFile/WriteFile in the OVERLAPPED parameter)
>     
>     This allows to eliminate SetFilePointerEx/SyncObject locks around IOs.
> 
>   storage/falcon/SerialLogFile.cpp@stripped, 2008-04-07 15:49:55+02:00, vvaintroub@wva.
> +11 -19
>     WL#3998 - Falcon positioned IO.
>     Use the native windows functionality to do positioned IO 
>     (pass the offset to ReadFile/WriteFile in the OVERLAPPED parameter)
>     
>     This allows to eliminate SetFilePointerEx/SyncObject locks around IOs.
> 
> diff -Nrup a/storage/falcon/IO.cpp b/storage/falcon/IO.cpp
> --- a/storage/falcon/IO.cpp	2008-03-27 07:09:04 +01:00
> +++ b/storage/falcon/IO.cpp	2008-04-07 15:49:54 +02:00
> @@ -500,6 +500,17 @@ int IO::pread(int64 offset, int length, 
>  
>  #if defined(HAVE_PREAD) && !defined(HAVE_BROKEN_PREAD)
>  	ret = ::pread (fileId, buffer, length, offset);
> +#elif defined(_WIN32)
> +	HANDLE hFile = (HANDLE)_get_osfhandle(fileId);
> +	OVERLAPPED overlapped = {0};
> +	LARGE_INTEGER pos;
> +
> +	pos.QuadPart = offset;
> +	overlapped.Offset = pos.LowPart;
> +	overlapped.OffsetHigh = pos.HighPart;
> +
> +	if (!ReadFile(hFile, buffer, length, (DWORD *) &ret, &overlapped))
> +		ret = -1;
>  #else
>  	Sync sync (&syncObject, "IO::pread");
>  	sync.lock (Exclusive);
> @@ -532,6 +543,20 @@ int IO::pwrite(int64 offset, int length,
>  	
>  #if defined(HAVE_PREAD) && !defined(HAVE_BROKEN_PREAD)
>  	ret = ::pwrite (fileId, buffer, length, offset);
> +#elif defined(_WIN32)
> +
> +	ASSERT(length > 0);
> +
> +	HANDLE hFile = (HANDLE)_get_osfhandle(fileId);
> +	OVERLAPPED overlapped = {0};
> +	LARGE_INTEGER pos;
> +
> +	pos.QuadPart = offset;
> +	overlapped.Offset = pos.LowPart;
> +	overlapped.OffsetHigh = pos.HighPart;
> +
> +	if (!WriteFile(hFile, buffer, length, (DWORD *)&ret, &overlapped))
> +		ret = -1;
>  #else
>  	Sync sync (&syncObject, "IO::pwrite");
>  	sync.lock (Exclusive);
> diff -Nrup a/storage/falcon/SerialLogFile.cpp b/storage/falcon/SerialLogFile.cpp
> --- a/storage/falcon/SerialLogFile.cpp	2008-03-11 16:16:31 +01:00
> +++ b/storage/falcon/SerialLogFile.cpp	2008-04-07 15:49:55 +02:00
> @@ -179,22 +179,15 @@ void SerialLogFile::write(int64 position
>  		priority.schedule(PRIORITY_HIGH);
>  		
>  #ifdef _WIN32
> -	
> -	Sync sync(&syncObject, "SerialLogFile::write");
> -	sync.lock(Exclusive);
> -	

Before this change, SerialLogFile::write() was serialized by an exclusive lock on
SerialLogFile::syncObject. Now that this lock is removed, are we assured that
SerialLogFile::write() is still protected?

I see that access to the serial log block being written is controlled by the mutex lock in
SerialLog::flush() and overflowFlush(), so we won't have multiple instances of
SerialLogFile::write() called on top of each other.

However, SeriaLog::copyClone() and SerialLogFile::zap() also call ::write(). Is this safe?

> -	if (position != offset)
> -		{
> -		LARGE_INTEGER pos;
> -		pos.QuadPart = position;
> -
> -		if (!SetFilePointerEx(handle, pos, NULL, FILE_BEGIN))
> -			throw SQLError(IO_ERROR, "serial log SetFilePointerEx failed with %d",
> GetLastError());
> -		}
> +	LARGE_INTEGER pos;
> +	pos.QuadPart = position;
> +	OVERLAPPED overlapped = {0};
> +	overlapped.Offset = pos.LowPart;
> +	overlapped.OffsetHigh = pos.HighPart;
>  
>  	DWORD ret;
>  	
> -	if (!WriteFile(handle, data, effectiveLength, &ret, NULL))
> +	if (!WriteFile(handle, data, effectiveLength, &ret, &overlapped))
>  		{
>  		int lastError = GetLastError();
>  		
> @@ -259,18 +252,17 @@ uint32 SerialLogFile::read(int64 positio
>  		priority.schedule(PRIORITY_HIGH);
>  
>  #ifdef _WIN32
> -	Sync sync(&syncObject, "SerialLogFile::read");
> -	sync.lock(Exclusive);
> +

Similarly, SerialLogFile::read() was serialized by an exclusive lock on
SerialLogFile::syncObject. Now that this lock is removed, are we assured there won't be
concurrent calls to SerialLogFile::read()?

>  	ASSERT(position < writePoint || writePoint == 0);
>  	LARGE_INTEGER pos;
>  	pos.QuadPart = position;
> -	
> -	if (!SetFilePointerEx(handle, pos, NULL, FILE_BEGIN))
> -		throw SQLError(IO_ERROR, "serial log SetFilePointer failed with %d",
> GetLastError());
> +	OVERLAPPED overlapped = {0};
> +	overlapped.Offset = pos.LowPart;
> +	overlapped.OffsetHigh = pos.HighPart;
>  
>  	DWORD ret;
>  
> -	if (!ReadFile(handle, data, effectiveLength, &ret, NULL))
> +	if (!ReadFile(handle, data, effectiveLength, &ret, &overlapped))
>  		throw SQLError(IO_ERROR, "serial log ReadFile failed with %d", GetLastError());
>  
>  	offset = position + effectiveLength;
> 
Thread
bk commit into 6.0 tree (vvaintroub:1.2627) WL#3998vvaintroub7 Apr
  • Re: bk commit into 6.0 tree (vvaintroub:1.2627) WL#3998Chris Powers7 Apr
    • RE: bk commit into 6.0 tree (vvaintroub:1.2627) WL#3998Vladislav Vaintroub7 Apr
      • RE: bk commit into 6.0 tree (vvaintroub:1.2627) WL#3998Vladislav Vaintroub7 Apr