RE: bk commit into 6.0 tree (vvaintroub:1.2627) WL#3998

[Vladislav Vaintroub]

> 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?

Yes Chris, this is correct. I use synchronous positional write.

Wlad

> -----Original Message-----
> From: Vladislav Vaintroub [mailto:vaintroub@stripped]
> Sent: Monday, April 07, 2008 8:08 PM
> To: 'Chris Powers'; vvaintroub@stripped
> Cc: commits@stripped
> Subject: RE: bk commit into 6.0 tree (vvaintroub:1.2627) WL#3998
> 
> Hi Chris,
> 
> >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?
> 
> There was no exclusive lock in HAVE_PREAD case however. What I've done here is
> essentially port of pread/pwrite without "seek" (SetFilePointerEx) and thus
> without the need to hold the file position after SetFilePointerEx before
> WriteFile.
> So SetFilePointerEx was eliminated and the SyncObject became obsolete.
> 
> Wlad
> 
> 
> > -----Original Message-----
> > From: Chris Powers [mailto:cpowers@stripped]
> > Sent: Monday, April 07, 2008 7:32 PM
> > To: vvaintroub@stripped
> > Cc: commits@stripped
> > Subject: Re: bk commit into 6.0 tree (vvaintroub:1.2627) WL#3998
> >
> > 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;
> > >
> 
> 
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:    http://lists.mysql.com/commits?unsub=1