Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error handling2 #39

Closed
wants to merge 42 commits into from
Closed

Error handling2 #39

wants to merge 42 commits into from

Conversation

kspangsege
Copy link
Contributor

There are two main things introduced:

  1. New File and File::Map classes that serve as abstraction layers over systems particulars

  2. Scoped transaction. All units tests have been changed to use them.

Also, SharedGroup::is_valid() has been removed.

@ghost ghost assigned astigsen Jan 24, 2013

#else // POSIX version

if (TIGHTDB_LIKELY(::posix_fallocate(m_fd, offset, size) == 0)) return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be an idea to use fallocate directly when supported (on linux). In samba they are quite explicit about which fallocate they use. See http://lists.samba.org/archive/rsync/2008-February/020103.html

@rrrlasse
Copy link
Contributor

SetFileValidData requires SE_MANAGE_VOLUME_NAME privilege to be enabled in Windows. It's disabled by default in all windows distributions and can only be enabled in the Server editions of Windows and Ultimate. The cheaper editions doesn't allow you to enable it.

///
/// Calling this method with a size that is greater than the size
/// of the file has undefined behavior.
void* map(AccessMode, std::size_t size) const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If might be an idea to add a flag parameter as well. When mapping files for transient data, we would like the ability to tell it that it that syncing is not needed if the os supports it (MAP_NOSYNC on freebsd).

// FIXME: This downgrading of the lock is not guaranteed to be atomic
flock(m_fd, LOCK_SH);
m_file.unlock();
m_file.lock_shared();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to unlock before downgrading? Is it to keep windows semantics?

@astigsen
Copy link
Contributor

Looks fine to me (and passes all test in xcode).

@rrrlasse
Copy link
Contributor

Issuing more than ~64 MB to WriteFile()/write() will make them do only a partial write both on Win/Linux, so the test with (n == static_cast(size) will fail.

Also, issuing too much data at a time will block the file system (at least I've seen this on Windows 7) and gives bad latency for other sevices. Around 32 KB still gives nearly max throughput.

Missing test of close() return value so double-close could go undetected.

If the customer wants unicode characters in file names - but also paths! - we would need to use the Ex versions of file API which use unicode w_char and wstring (*nix kernels use UTF8 so no problem here).

Just a memo: we need tests for hidden shares on Windows and UNC paths (common on servers).

Will review more later...

@kspangsege
Copy link
Contributor Author

Lasse - According to Microsofts documentation for FileWrite(), partial writes are only possible in asynchronous mode or when writing to a pipe. Also, I have not been able to find any other people claiming that partial write are possible in our case. If you are sure that partial writes are possible, let me know.

The WriteFile function returns when one of the following conditions occur:

  • The number of bytes requested is written.
  • A read operation releases buffer space on the read end of the pipe (if the write was blocked). For more information, see the Pipes section.
  • An asynchronous handle is being used and the write is occurring asynchronously.
  • An error occurs.

http://msdn.microsoft.com/en-us/library/windows/desktop/aa365747%28v=vs.85%29.aspx

On Linux and indeed on any POSIX compliant system, a write() that reports success can never be partial unless non-blocking I/O is explicitly enabled.

With respect to your second point, that large writes should be submitted from the application to the kernel in smaller chunks, I think that should be up to the user of the File API.

@rrrlasse
Copy link
Contributor

I think partial writes are probably covered by the quoted.

But another issue is failed writes that could have been avoided. I once tried to tune eXdupe by issuing 256 MB blocks and got heaps of bug reports. Like timeouts when writing to slow network drives mapped by Terminal Server (worked fine on SMB), and tape devices that would make write() return an error code, etc, etc, etc. Also weird behaviour caused by being so long in kernel space.

I'm of course thinking of the case where you would dump 10+ GB somewhere.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants