Commit ab0c79da authored by Paal Kvamme's avatar Paal Kvamme
Browse files

Make xx_eof() and xx_write() thread safe for local file access. Technically...

Make xx_eof() and xx_write() thread safe for local file access. Technically they don't need to be. But it adds robustness.
parent fe9d723e
......@@ -271,12 +271,7 @@ protected:
/**
* Keep track of EOF: Initially set on file open; later kept
* up to date if we write to the file.
* Thread safety:
* TODO-Medium: This makes the low level xx_write() not thread safe.
* OpenZGY is in general not thread safe when writing, but in this
* case it doesn't cost much to synchronize using a lock.
* Make _set_local_eof() / get_local_eof accessors and make the
* variable itself private.
* Thread safety: Synchronized by the per-file mutex.
*/
std::int64_t _eof;
std::shared_ptr<SummaryTimer> _rtimer; // Access is thread safe
......
......@@ -32,6 +32,7 @@
#include <fcntl.h>
#include <errno.h>
#include <unistd.h>
#include <mutex>
using OpenZGY::IOContext;
namespace InternalZGY {
......@@ -63,8 +64,10 @@ namespace {
* to do appropriate locking.
*
* Thread safety when used for writing:
* TODO-Low: Not thread safe. Might be fixed, but the high level design
* states that writes are never thread safe anyway.
* Yes, but caller won't mak use of this because the high level design
* states that writes (i.e. calls from ZgyWriter) are not thread safe.
* Besides, other file backends might not be thread safe and those might
* not be so easy to change.
*
* Thread safety when closing a file:
* Not thread safe.
......@@ -90,6 +93,7 @@ public:
virtual std::int64_t _real_eof() const;
private:
int _fd;
mutable std::mutex _mutex;
};
/////////////////////////////////////////////////////////////////////////////
......@@ -99,6 +103,7 @@ private:
LocalFileLinux::LocalFileLinux(const std::string& filename, OpenMode mode, const IOContext*)
: FileCommon(filename, mode)
, _fd(-1)
, _mutex()
{
switch (mode) {
case OpenMode::ReadOnly:
......@@ -192,11 +197,12 @@ LocalFileLinux::xx_close()
}
/**
* \details: Thread safety: No. Might be fixed. See xx_write().
* \details: Thread safety: Yes, by locking.
*/
std::int64_t
LocalFileLinux::xx_eof() const
{
std::lock_guard<std::mutex> lk(_mutex); // protect _eof
return this->_eof;
}
......@@ -239,7 +245,7 @@ LocalFileLinux::xx_readv(const ReadList& requests, bool parallel_ok, bool immuta
}
/**
* \details: Thread safety: No. TODO-Medium, make thread safe.
* \details: Thread safety: Yes.
* OpenZGY is in general not thread safe when writing, but in this
* low level function it doesn't cost much to synchronize _eof
* both here and the places it is read.
......@@ -256,14 +262,14 @@ LocalFileLinux::xx_write(const void* data, std::int64_t offset, std::int64_t siz
ssize_t nbytes = ::pwrite(_fd, data, size, offset);
if (nbytes < 0)
throw OpenZGY::Errors::ZgyIoError(_name, errno);
// NOT THREADSAFE
std::lock_guard<std::mutex> lk(_mutex); // protect _eof
_eof = std::max(_eof, offset + nbytes);
if (nbytes != size)
throw OpenZGY::Errors::ZgyInternalError(_name + ": Short write");
}
/**
* \details: Thread safety: Not for writes. Uses xx_eof().
* \details: Thread safety: Yes. Uses xx_eof().
* And should in any case be changed to call ::stat().
*/
std::int64_t
......
......@@ -67,8 +67,10 @@ namespace {
* to do appropriate locking.
*
* Thread safety when used for writing:
* TODO-Low: Not thread safe. Might be fixed, but the high level design
* states that writes are never thread safe anyway.
* Yes, but caller won't mak use of this because the high level design
* states that writes (i.e. calls from ZgyWriter) are not thread safe.
* Besides, other file backends might not be thread safe and those might
* not be so easy to change.
*
* Thread safety when closing a file:
* Not thread safe.
......@@ -94,7 +96,7 @@ public:
virtual std::int64_t _real_eof() const;
private:
int _fd;
std::mutex _mutex;
mutable std::mutex _mutex;
};
/////////////////////////////////////////////////////////////////////////////
......@@ -198,11 +200,12 @@ LocalFileWindows::xx_close()
}
/**
* \details: Thread safety: No. Might be fixed. See xx_write().
* \details: Thread safety: Yes, by locking.
*/
std::int64_t
LocalFileWindows::xx_eof() const
{
std::lock_guard<std::mutex> lk(_mutex); // protect _eof
return this->_eof;
}
......@@ -251,7 +254,7 @@ LocalFileWindows::xx_readv(const ReadList& requests, bool parallel_ok, bool immu
}
/**
* \details: Thread safety: No. TODO-Medium, make thread safe.
* \details: Thread safety: Yes.
* OpenZGY is in general not thread safe when writing, but in this
* low level function it doesn't cost much to synchronize _eof
* both here and the places it is read.
......@@ -269,7 +272,7 @@ LocalFileWindows::xx_write(const void* data, std::int64_t offset, std::int64_t s
std::cout << "xx_write(*, " << std::hex
<< offset << ", " << size << ", hint=" << (int)usagehint
<< std::dec << ")\n";
std::lock_guard<std::mutex> lk(_mutex); // make seek + write atomic
std::lock_guard<std::mutex> lk(_mutex); // make seek + write + _eof atomic
_lseeki64(_fd, offset, SEEK_SET);
std::int64_t nbytes = static_cast<std::int64_t>(_write(_fd, data, static_cast<int>(size)));
if (nbytes < 0)
......@@ -280,7 +283,7 @@ LocalFileWindows::xx_write(const void* data, std::int64_t offset, std::int64_t s
}
/**
* \details: Thread safety: Not for writes. Uses xx_eof().
* \details: Thread safety: Yes. Uses xx_eof().
* And should in any case be changed to call ::stat().
* Or the Windows equivalent.
*/
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment