steps towards removing disk_io_job from disk_interface

include/libtorrent/torrent.hpp

@@ -449,7 +449,11 @@ namespace libtorrent
 
 		std::string resolve_filename(int file) const;
 		void handle_exception();
-		void handle_disk_error(disk_io_job const* j, peer_connection* c = 0);
+
+		enum class disk_class { none, write };
+		void handle_disk_error(string_view job_name
+			, storage_error const& error, peer_connection* c = nullptr
+			, disk_class rw = disk_class::none);
 		void clear_error();
 
 		void set_error(error_code const& ec, int file);

src/disk_io_thread.cpp

@@ -773,7 +773,7 @@ namespace libtorrent
 		{
 			disk_io_job* j = src.pop_front();
 			TORRENT_ASSERT((j->flags & disk_io_job::in_progress) || !j->storage);
-			j->ret = -1;
+			j->ret = disk_interface::fatal_disk_error;
 			j->error = e;
 			dst.push_back(j);
 		}
@@ -1101,25 +1101,26 @@ namespace libtorrent
 		}
 		catch (boost::system::system_error const& err)
 		{
-			ret = -1;
+			ret = disk_interface::fatal_disk_error;
 			j->error.ec = err.code();
 			j->error.operation = storage_error::exception;
 		}
 		catch (std::bad_alloc const&)
 		{
-			ret = -1;
+			ret = disk_interface::fatal_disk_error;
 			j->error.ec = errors::no_memory;
 			j->error.operation = storage_error::exception;
 		}
 		catch (std::exception const&)
 		{
-			ret = -1;
+			ret = disk_interface::fatal_disk_error;
 			j->error.ec = boost::asio::error::fault;
 			j->error.operation = storage_error::exception;
 		}
 
 		// note that -2 errors are OK
-		TORRENT_ASSERT(ret != -1 || (j->error.ec && j->error.operation != 0));
+		TORRENT_ASSERT(ret != disk_interface::fatal_disk_error
+			|| (j->error.ec && j->error.operation != 0));
 
 		m_stats_counters.inc_stats_counter(counters::num_running_disk_jobs, -1);
 
@@ -1178,7 +1179,7 @@ namespace libtorrent
 		{
 			j->error.ec = error::no_memory;
 			j->error.operation = storage_error::alloc_cache_piece;
-			return -1;
+			return disk_interface::fatal_disk_error;
 		}
 
 		time_point start_time = clock_type::now();
@@ -1464,7 +1465,7 @@ namespace libtorrent
 			TORRENT_ASSERT(pe->blocks[j->d.io.offset / 16 / 1024].buf != nullptr);
 			j->error.ec = error::operation_aborted;
 			j->error.operation = storage_error::write;
-			return -1;
+			return disk_interface::fatal_disk_error;
 		}
 
 		pe = m_disk_cache.add_dirty_block(j);
@@ -1598,7 +1599,7 @@ namespace libtorrent
 
 		if (pe == nullptr)
 		{
-			j->ret = -1;
+			j->ret = disk_interface::fatal_disk_error;
 			j->error.ec = error::no_memory;
 			j->error.operation = storage_error::read;
 			return 0;
@@ -2126,7 +2127,7 @@ namespace libtorrent
 
 		sha1_hash piece_hash = h.final();
 		std::memcpy(j->d.piece_hash, piece_hash.data(), 20);
-		return ret >= 0 ? 0 : -1;
+		return ret >= 0 ? 0 : disk_interface::fatal_disk_error;
 	}
 
 	int disk_io_thread::do_hash(disk_io_job* j, jobqueue_t& /* completed_jobs */ )
@@ -2187,7 +2188,7 @@ namespace libtorrent
 		{
 			j->error.ec = error::no_memory;
 			j->error.operation = storage_error::alloc_cache_piece;
-			return -1;
+			return disk_interface::fatal_disk_error;
 		}
 
 		if (pe->hashing)
@@ -2286,7 +2287,7 @@ namespace libtorrent
 
 					j->error.ec = errors::no_memory;
 					j->error.operation = storage_error::alloc_cache_piece;
-					return -1;
+					return disk_interface::fatal_disk_error;
 				}
 
 				DLOG("do_hash: reading (piece: %d block: %d)\n", int(pe->piece), i);
@@ -2309,7 +2310,7 @@ namespace libtorrent
 				// of this file
 				if (ret != iov.iov_len)
 				{
-					ret = -1;
+					ret = disk_interface::fatal_disk_error;
 					j->error.ec = boost::asio::error::eof;
 					j->error.operation = storage_error::read;
 					m_disk_cache.free_buffer(static_cast<char*>(iov.iov_base));
@@ -2392,7 +2393,7 @@ namespace libtorrent
 		l.unlock();
 
 		j->storage->release_files(j->error);
-		return j->error ? -1 : 0;
+		return j->error ? disk_interface::fatal_disk_error : 0;
 	}
 
 	int disk_io_thread::do_delete_files(disk_io_job* j, jobqueue_t& completed_jobs)
@@ -2409,7 +2410,7 @@ namespace libtorrent
 		l.unlock();
 
 		j->storage->delete_files(j->buffer.delete_options, j->error);
-		return j->error ? -1 : 0;
+		return j->error ? disk_interface::fatal_disk_error : 0;
 	}
 
 	int disk_io_thread::do_check_fastresume(disk_io_job* j, jobqueue_t& /* completed_jobs */ )
@@ -2485,7 +2486,7 @@ namespace libtorrent
 		// if files need to be closed, that's the storage's responsibility
 		j->storage->rename_file(j->piece, j->buffer.string
 			, j->error);
-		return j->error ? -1 : 0;
+		return j->error ? disk_interface::fatal_disk_error : disk_interface::no_error;
 	}
 
 	int disk_io_thread::do_stop_torrent(disk_io_job* j, jobqueue_t& completed_jobs)
@@ -2503,7 +2504,7 @@ namespace libtorrent
 		m_disk_cache.release_memory();
 
 		j->storage->release_files(j->error);
-		return j->error ? -1 : 0;
+		return j->error ? disk_interface::fatal_disk_error : disk_interface::no_error;
 	}
 
 	namespace {

src/peer_connection.cpp

@@ -2998,12 +2998,47 @@ namespace libtorrent
 		// to allow to receive more data
 		setup_receive();
 
-		piece_block block_finished(p.piece, p.start / t->block_size());
+		piece_block const block_finished(p.piece, p.start / t->block_size());
 
 		if (j->ret < 0)
 		{
+			// we failed to write j->piece to disk tell the piece picker
+			// this will block any other peer from issuing requests
+			// to this piece, until we've cleared it.
+			if (j->error.ec == boost::asio::error::operation_aborted)
+			{
+				if (t->has_picker())
+					t->picker().mark_as_canceled(block_finished, nullptr);
+			}
+			else
+			{
+				// if any other peer has a busy request to this block, we need
+				// to cancel it too
+				t->cancel_block(block_finished);
+				if (t->has_picker())
+					t->picker().write_failed(block_finished);
+
+				if (t->has_storage())
+				{
+					// when this returns, all outstanding jobs to the
+					// piece are done, and we can restore it, allowing
+					// new requests to it
+					m_disk_thread.async_clear_piece(&t->storage(), j->piece
+						, std::bind(&torrent::on_piece_fail_sync, t, _1, block_finished));
+				}
+				else
+				{
+					// is m_abort true? if so, we should probably just
+					// exit this function early, no need to keep the picker
+					// state up-to-date, right?
+					disk_io_job sj;
+					sj.piece = j->piece;
+					t->on_piece_fail_sync(&sj, block_finished);
+				}
+			}
+			t->update_gauge();
 			// handle_disk_error may disconnect us
-			t->handle_disk_error(j, this);
+			t->handle_disk_error("write", j->error, this, torrent::disk_class::write);
 			return;
 		}
 
@@ -5146,7 +5181,7 @@ namespace libtorrent
 
 		if (j->error)
 		{
-			t->handle_disk_error(j, this);
+			t->handle_disk_error("hash", j->error, this);
 			t->leave_seed_mode(false);
 			return;
 		}
@@ -5261,7 +5296,7 @@ namespace libtorrent
 		if (j->ret != r.length)
 		{
 			// handle_disk_error may disconnect us
-			t->handle_disk_error(j, this);
+			t->handle_disk_error("read", j->error, this);
 			return;
 		}
 

src/torrent.cpp

@@ -1032,89 +1032,51 @@ namespace libtorrent
 		}
 	}
 
-	void torrent::handle_disk_error(disk_io_job const* j, peer_connection* c)
+	void torrent::handle_disk_error(string_view job_name
+		, storage_error const& error
+		, peer_connection* c
+		, disk_class rw)
 	{
 		TORRENT_ASSERT(is_single_thread());
-		if (!j->error) return;
+		TORRENT_ASSERT(error);
 
 #ifndef TORRENT_DISABLE_LOGGING
 		if (should_log())
 		{
-			debug_log("disk error: (%d) %s [%s : %s] in file: %s"
+			debug_log("disk error: (%d) %s [%*s : %s] in file: %s"
-				, j->error.ec.value(), j->error.ec.message().c_str()
+				, error.ec.value(), error.ec.message().c_str()
-				, job_name(j->action), j->error.operation_str()
+				, int(job_name.size()), job_name.data(), error.operation_str()
-				, resolve_filename(j->error.file).c_str());
+				, resolve_filename(error.file).c_str());
 		}
 #endif
 
-		if (j->action == disk_io_job::write)
+		if (error.ec == boost::system::errc::not_enough_memory)
-		{
-			piece_block block_finished(j->piece, j->d.io.offset / block_size());
-
-			// we failed to write j->piece to disk tell the piece picker
-			// this will block any other peer from issuing requests
-			// to this piece, until we've cleared it.
-			if (j->error.ec == boost::asio::error::operation_aborted)
-			{
-				if (has_picker())
-					picker().mark_as_canceled(block_finished, nullptr);
-			}
-			else
-			{
-				// if any other peer has a busy request to this block, we need
-				// to cancel it too
-				cancel_block(block_finished);
-				if (has_picker())
-					picker().write_failed(block_finished);
-
-				if (m_storage)
-				{
-					// when this returns, all outstanding jobs to the
-					// piece are done, and we can restore it, allowing
-					// new requests to it
-					m_ses.disk_thread().async_clear_piece(m_storage.get(), j->piece
-						, std::bind(&torrent::on_piece_fail_sync, shared_from_this(), _1, block_finished));
-				}
-				else
-				{
-					// is m_abort true? if so, we should probably just
-					// exit this function early, no need to keep the picker
-					// state up-to-date, right?
-					disk_io_job sj;
-					sj.piece = j->piece;
-					on_piece_fail_sync(&sj, block_finished);
-				}
-			}
-			update_gauge();
-		}
-
-		if (j->error.ec == boost::system::errc::not_enough_memory)
 		{
 			if (alerts().should_post<file_error_alert>())
-				alerts().emplace_alert<file_error_alert>(j->error.ec
+				alerts().emplace_alert<file_error_alert>(error.ec
-					, resolve_filename(j->error.file), j->error.operation_str(), get_handle());
+					, resolve_filename(error.file), error.operation_str(), get_handle());
 			if (c) c->disconnect(errors::no_memory, op_file);
 			return;
 		}
 
-		if (j->error.ec == boost::asio::error::operation_aborted) return;
+		if (error.ec == boost::asio::error::operation_aborted) return;
 
 		// notify the user of the error
 		if (alerts().should_post<file_error_alert>())
-			alerts().emplace_alert<file_error_alert>(j->error.ec
+			alerts().emplace_alert<file_error_alert>(error.ec
-				, resolve_filename(j->error.file), j->error.operation_str(), get_handle());
+				, resolve_filename(error.file), error.operation_str(), get_handle());
 
 		// if a write operation failed, and future writes are likely to
 		// fail, while reads may succeed, just set the torrent to upload mode
 		// if we make an incorrect assumption here, it's not the end of the
 		// world, if we ever issue a read request and it fails as well, we
 		// won't get in here and we'll actually end up pausing the torrent
-		if (j->action == disk_io_job::write
+		if (rw == disk_class::write
-			&& (j->error.ec == boost::system::errc::read_only_file_system
+			&& (error.ec == boost::system::errc::read_only_file_system
-			|| j->error.ec == boost::system::errc::permission_denied
+			|| error.ec == boost::system::errc::permission_denied
-			|| j->error.ec == boost::system::errc::operation_not_permitted
+			|| error.ec == boost::system::errc::operation_not_permitted
-			|| j->error.ec == boost::system::errc::no_space_on_device
+			|| error.ec == boost::system::errc::no_space_on_device
-			|| j->error.ec == boost::system::errc::file_too_large))
+			|| error.ec == boost::system::errc::file_too_large))
 		{
 			// if we failed to write, stop downloading and just
 			// keep seeding.
@@ -1128,7 +1090,7 @@ namespace libtorrent
 		}
 
 		// put the torrent in an error-state
-		set_error(j->error.ec, j->error.file);
+		set_error(error.ec, error.file);
 
 		// if the error appears to be more serious than a full disk, just pause the torrent
 		pause();
@@ -1178,7 +1140,7 @@ namespace libtorrent
 		{
 			rp->fail = true;
 			rp->error = j->error.ec;
-			handle_disk_error(j);
+			handle_disk_error("read", j->error);
 		}
 		else
 		{
@@ -1334,7 +1296,7 @@ namespace libtorrent
 
 		if (j->ret == -1)
 		{
-			handle_disk_error(j);
+			handle_disk_error("write", j->error);
 			return;
 		}
 
@@ -1988,7 +1950,7 @@ namespace libtorrent
 		{
 			TORRENT_ASSERT(m_outstanding_check_files == false);
 			m_add_torrent_params.reset();
-			handle_disk_error(j);
+			handle_disk_error("check_resume_data", j->error);
 			auto_managed(false);
 			pause();
 			set_state(torrent_status::checking_files);
@@ -2240,7 +2202,7 @@ namespace libtorrent
 
 		if (j->ret == disk_interface::fatal_disk_error)
 		{
-			handle_disk_error(j);
+			handle_disk_error("force_recheck", j->error);
 			return;
 		}
 		if (j->ret == 0)
@@ -3708,7 +3670,7 @@ namespace libtorrent
 		}
 		else if (ret == -1)
 		{
-			handle_disk_error(j);
+			handle_disk_error("piece_verified", j->error);
 		}
 		else
 		{