Fix parts file i/o errors (#3110)

Since i/o operations are not synchronized, it is possible that one thread closes file handle, opened in another thread. With addition of `hidden` file attribute, an old bug in `part_file::open_file()` revealed both problems, causing a lot of random i/o errors in parts file. Fixing `open_file` bug should reduce the number of such errors back to 'normal'.

ChangeLog

@@ -1,4 +1,6 @@
 
+	* fix race condition during part_file export
+	* fix part_file open mode compatibility test
 	* fixed race condition in random number generator
 	* fix race condition in stat_cache (disk storage)
 	* improve error handling of failing to change file priority

include/libtorrent/part_file.hpp

@@ -73,8 +73,11 @@ namespace libtorrent
 		void open_file(int mode, error_code& ec);
 		void flush_metadata_impl(error_code& ec);
 
+		boost::int64_t slot_offset(boost::int64_t const slot) const
+		{ return m_header_size + slot * m_piece_size; }
+
 		std::string m_path;
-		std::string m_name;
+		std::string const m_name;
 
 		// allocate a slot and return the slot index
 		int allocate_slot(int piece);
@@ -93,15 +96,15 @@ namespace libtorrent
 
 		// the max number of pieces in the torrent this part file is
 		// backing
-		int m_max_pieces;
+		int const m_max_pieces;
 
 		// number of bytes each piece contains
-		int m_piece_size;
+		int const m_piece_size;
 
 		// this is the size of the part_file header, it is added
 		// to offsets when calculating the offset to read and write
 		// payload data from
-		int m_header_size;
+		int const m_header_size;
 
 		// if this is true, the metadata in memory has changed since
 		// we last saved or read it from disk. It means that we

src/part_file.cpp

@@ -44,7 +44,7 @@ POSSIBILITY OF SUCH DAMAGE.
   // the size of the torrent (and can be used to calculate the size
   // of the file header)
   uint32_t num_pieces;
-  
+
   // the number of bytes in each piece. This determines the size of
   // each slot in the part file. This is typically an even power of 2,
   // but it is not guaranteed to be.
@@ -58,7 +58,7 @@ POSSIBILITY OF SUCH DAMAGE.
   // unused, n is defined as the number to align the size of this
   // header to an even multiple of 1024 bytes.
   uint8_t padding[n];
- 
+
 */
 
 #include "libtorrent/part_file.hpp"
@@ -183,17 +183,11 @@ namespace libtorrent
 		open_file(file::read_write | file::attribute_hidden, ec);
 		if (ec) return -1;
 
-		int slot = -1;
+		boost::unordered_map<int, int>::iterator const i = m_piece_map.find(piece);
-		boost::unordered_map<int, int>::iterator i = m_piece_map.find(piece);
+		int const slot = i == m_piece_map.end() ? allocate_slot(piece) : i->second;
-		if (i == m_piece_map.end())
-			slot = allocate_slot(piece);
-		else
-			slot = i->second;
 
 		l.unlock();
-
+		return m_file.writev(slot_offset(slot) + offset, bufs, num_bufs, ec);
-		boost::int64_t slot_offset = boost::int64_t(m_header_size) + boost::int64_t(slot) * m_piece_size;
-		return m_file.writev(slot_offset + offset, bufs, num_bufs, ec);
 	}
 
 	int part_file::readv(file::iovec_t const* bufs, int num_bufs
@@ -202,32 +196,29 @@ namespace libtorrent
 		TORRENT_ASSERT(offset >= 0);
 		mutex::scoped_lock l(m_mutex);
 
-		boost::unordered_map<int, int>::iterator i = m_piece_map.find(piece);
+		boost::unordered_map<int, int>::iterator const i = m_piece_map.find(piece);
 		if (i == m_piece_map.end())
 		{
-			ec = error_code(boost::system::errc::no_such_file_or_directory
+			ec = make_error_code(boost::system::errc::no_such_file_or_directory);
-				, boost::system::generic_category());
 			return -1;
 		}
 
-		int slot = i->second;
+		int const slot = i->second;
 
 		open_file(file::read_write | file::attribute_hidden, ec);
 		if (ec) return -1;
 
 		l.unlock();
-
+		return m_file.readv(slot_offset(slot) + offset, bufs, num_bufs, ec);
-		boost::int64_t slot_offset = boost::int64_t(m_header_size) + boost::int64_t(slot) * m_piece_size;
-		return m_file.readv(slot_offset + offset, bufs, num_bufs, ec);
 	}
 
 	void part_file::open_file(int mode, error_code& ec)
 	{
-		if (m_file.is_open()
+		if (m_file.is_open() && ((mode & file::rw_mask) == file::read_only
-			&& ((m_file.open_mode() & file::rw_mask) == mode
+			|| (m_file.open_mode() & file::rw_mask) == file::read_write))
-				|| mode == file::read_only)) return;
+			return;
 
-		std::string fn = combine_path(m_path, m_name);
+		std::string const fn = combine_path(m_path, m_name);
 		m_file.open(fn, mode, ec);
 		if (((mode & file::rw_mask) != file::read_only)
 			&& ec == boost::system::errc::no_such_file_or_directory)
@@ -323,14 +314,8 @@ namespace libtorrent
 
 				if (!buf) buf.reset(new char[m_piece_size]);
 
-				boost::int64_t const slot_offset = boost::int64_t(m_header_size)
-					+ boost::int64_t(slot) * m_piece_size;
-
-				// don't hold the lock during disk I/O
-				l.unlock();
-
 				file::iovec_t v = { buf.get(), size_t(block_to_copy) };
-				v.iov_len = m_file.readv(slot_offset + piece_offset, &v, 1, ec);
+				v.iov_len = m_file.readv(slot_offset(slot) + piece_offset, &v, 1, ec);
 				TORRENT_ASSERT(!ec);
 				if (ec || v.iov_len == 0) return;
 
@@ -338,10 +323,6 @@ namespace libtorrent
 				TORRENT_ASSERT(ec || ret == v.iov_len);
 				if (ec || ret != v.iov_len) return;
 
-				// we're done with the disk I/O, grab the lock again to update
-				// the slot map
-				l.lock();
-
 				if (block_to_copy == m_piece_size)
 				{
 					// since we released the lock, it's technically possible that
@@ -423,4 +404,3 @@ namespace libtorrent
 		if (ec) return;
 	}
 }
-