From 08bac479befdc9b1dadb2989837b1bb10b650be5 Mon Sep 17 00:00:00 2001 From: arvidn Date: Tue, 15 Mar 2016 01:55:36 -0400 Subject: [PATCH 1/2] allow each peer have at least 2 allocated disk blocks at any given time, to avoid stalling when cache_size setting is small. also deprecate use_write_cache --- include/libtorrent/disk_io_job.hpp | 6 +---- include/libtorrent/disk_io_thread.hpp | 2 +- include/libtorrent/settings_pack.hpp | 14 +++++++----- src/disk_io_thread.cpp | 33 +++------------------------ src/peer_connection.cpp | 12 ++++++++-- src/settings_pack.cpp | 8 +++---- 6 files changed, 27 insertions(+), 48 deletions(-) diff --git a/include/libtorrent/disk_io_job.hpp b/include/libtorrent/disk_io_job.hpp index e0a6c28e5..e64c5d889 100644 --- a/include/libtorrent/disk_io_job.hpp +++ b/include/libtorrent/disk_io_job.hpp @@ -136,11 +136,7 @@ namespace libtorrent in_progress = 0x20, // turns into file::coalesce_buffers in the file operation - coalesce_buffers = 0x40, - - // the disk cache was enabled when this job was issued, it should use - // the disk cache once it's handled by a disk thread - use_disk_cache = 0x80 + coalesce_buffers = 0x40 }; // for write jobs, returns true if its block diff --git a/include/libtorrent/disk_io_thread.hpp b/include/libtorrent/disk_io_thread.hpp index ce9709905..0c55e2726 100644 --- a/include/libtorrent/disk_io_thread.hpp +++ b/include/libtorrent/disk_io_thread.hpp @@ -1,6 +1,6 @@ /* -Copyright (c) 2007-2016, Arvid Norberg +Copyright (c) 2007-2016, Arvid Norberg, Steven Siloti All rights reserved. Redistribution and use in source and binary forms, with or without diff --git a/include/libtorrent/settings_pack.hpp b/include/libtorrent/settings_pack.hpp index 746ad80c2..aecc3fa81 100644 --- a/include/libtorrent/settings_pack.hpp +++ b/include/libtorrent/settings_pack.hpp @@ -259,14 +259,16 @@ namespace libtorrent // passes the hash check, it is taken out of parole mode. use_parole_mode, - // enable and disable caching of read blocks and blocks to be written - // to disk respsectively. the purpose of the read cache is partly - // read-ahead of requests but also to avoid reading blocks back from - // the disk multiple times for popular pieces. the write cache purpose - // is to hold off writing blocks to disk until they have been hashed, - // to avoid having to read them back in again. + // enable and disable caching of blocks read from disk. the purpose of + // the read cache is partly read-ahead of requests but also to avoid + // reading blocks back from the disk multiple times for popular + // pieces. use_read_cache, +#ifndef TORRENT_NO_DEPRECATED use_write_cache, +#else + deprecated7, +#endif // this will make the disk cache never flush a write piece if it would // cause is to have to re-read it once we want to calculate the piece diff --git a/src/disk_io_thread.cpp b/src/disk_io_thread.cpp index e36b1e2b3..d2606cf72 100644 --- a/src/disk_io_thread.cpp +++ b/src/disk_io_thread.cpp @@ -1,6 +1,6 @@ /* -Copyright (c) 2007-2016, Arvid Norberg +Copyright (c) 2007-2016, Arvid Norberg, Steven Siloti All rights reserved. Redistribution and use in source and binary forms, with or without @@ -1208,14 +1208,6 @@ namespace libtorrent int disk_io_thread::do_read(disk_io_job* j, jobqueue_t& completed_jobs) { - if ((j->flags & disk_io_job::use_disk_cache) == 0) - { - // we're not using a cache. This is the simple path - // just read straight from the file - int ret = do_uncached_read(j); - return ret; - } - int block_size = m_disk_cache.block_size(); int piece_size = j->storage->files()->piece_size(j->piece); int blocks_in_piece = (piece_size + block_size - 1) / block_size; @@ -1232,26 +1224,8 @@ namespace libtorrent cached_piece_entry* pe = m_disk_cache.find_piece(j); if (pe == NULL) { - // this isn't supposed to happen. The piece is supposed - // to be allocated when the read job is posted to the - // queue, and have 'outstanding_read' set to 1 - TORRENT_ASSERT(false); - - int cache_state = (j->flags & disk_io_job::volatile_read) - ? cached_piece_entry::volatile_read_lru - : cached_piece_entry::read_lru1; - pe = m_disk_cache.allocate_piece(j, cache_state); - if (pe == NULL) - { - j->error.ec = error::no_memory; - j->error.operation = storage_error::alloc_cache_piece; - m_disk_cache.free_iovec(iov, iov_len); - return -1; - } -#if TORRENT_USE_ASSERTS - pe->piece_log.push_back(piece_log_t(piece_log_t::set_outstanding_jobs)); -#endif - pe->outstanding_read = 1; + l.unlock(); + return do_uncached_read(j); } TORRENT_PIECE_ASSERT(pe->outstanding_read == 1, pe); @@ -1633,7 +1607,6 @@ namespace libtorrent j->error.operation = storage_error::read; return 0; } - j->flags |= disk_io_job::use_disk_cache; if (pe->outstanding_read) { TORRENT_PIECE_ASSERT(j->piece == pe->piece, pe); diff --git a/src/peer_connection.cpp b/src/peer_connection.cpp index 088c191f5..0ba41cba5 100644 --- a/src/peer_connection.cpp +++ b/src/peer_connection.cpp @@ -2611,7 +2611,13 @@ namespace libtorrent return; } - if (exceeded) + // every peer is entitled to have two disk blocks allocated at any given + // time, regardless of whether the cache size is exceeded or not. If this + // was not the case, when the cache size setting is very small, most peers + // would be blocked most of the time, because the disk cache would + // continously be in exceeded state. Only rarely would it actually drop + // down to 0 and unblock all peers. + if (exceeded && m_outstanding_writing_bytes > 0) { if ((m_channel_state[download_channel] & peer_info::bw_disk) == 0) m_counters.inc_stats_counter(counters::num_peers_down_disk); @@ -4541,7 +4547,9 @@ namespace libtorrent return false; } - if (exceeded) + // to understand why m_outstanding_writing_bytes is here, see comment by + // the other call to allocate_disk_buffer() + if (exceeded && m_outstanding_writing_bytes > 0) { #ifndef TORRENT_DISABLE_LOGGING peer_log(peer_log_alert::info, "DISK", "exceeded disk buffer watermark"); diff --git a/src/settings_pack.cpp b/src/settings_pack.cpp index 5d85d2ae8..c743e9833 100644 --- a/src/settings_pack.cpp +++ b/src/settings_pack.cpp @@ -150,7 +150,7 @@ namespace libtorrent SET(upnp_ignore_nonrouters, false, 0), SET(use_parole_mode, true, 0), SET(use_read_cache, true, 0), - SET(use_write_cache, true, 0), + DEPRECATED_SET(use_write_cache, true, 0), SET(dont_flush_write_cache, false, 0), SET(explicit_read_cache, false, 0), SET(coalesce_reads, false, 0), @@ -603,14 +603,14 @@ namespace libtorrent && std::find(callbacks.begin(), callbacks.end(), sa.fun) == callbacks.end()) callbacks.push_back(sa.fun); } - + for (std::vector >::const_iterator i = pack->m_ints.begin() , end(pack->m_ints.end()); i != end; ++i) { // disregard setting indices that are not int types if ((i->first & settings_pack::type_mask) != settings_pack::int_type_base) continue; - + // ignore settings that are out of bounds int index = i->first & settings_pack::index_mask; if (index < 0 || index >= settings_pack::num_int_settings) @@ -629,7 +629,7 @@ namespace libtorrent // disregard setting indices that are not bool types if ((i->first & settings_pack::type_mask) != settings_pack::bool_type_base) continue; - + // ignore settings that are out of bounds int index = i->first & settings_pack::index_mask; if (index < 0 || index >= settings_pack::num_bool_settings) From 36d0cfe40de823d4f0c13ef3e9f87809aa66c23b Mon Sep 17 00:00:00 2001 From: arvidn Date: Tue, 15 Mar 2016 21:10:58 -0400 Subject: [PATCH 2/2] fix shutdown issue caused by peer connections being kept alive by disk buffer pool callbacks --- include/libtorrent/disk_buffer_pool.hpp | 2 +- src/disk_buffer_pool.cpp | 21 ++++++++++++++------- src/session_impl.cpp | 20 +++++++++----------- 3 files changed, 24 insertions(+), 19 deletions(-) diff --git a/include/libtorrent/disk_buffer_pool.hpp b/include/libtorrent/disk_buffer_pool.hpp index ba62d695b..1f5545d11 100644 --- a/include/libtorrent/disk_buffer_pool.hpp +++ b/include/libtorrent/disk_buffer_pool.hpp @@ -135,7 +135,7 @@ namespace libtorrent // of buffers in use drops below the low watermark, // we start calling these functions back // TODO: try to remove the observers, only using the async_allocate handlers - std::vector > m_observers; + std::vector > m_observers; // these handlers are executed when a new buffer is available std::vector m_handlers; diff --git a/src/disk_buffer_pool.cpp b/src/disk_buffer_pool.cpp index d74dd6410..1bfeefd26 100644 --- a/src/disk_buffer_pool.cpp +++ b/src/disk_buffer_pool.cpp @@ -64,8 +64,10 @@ POSSIBILITY OF SUCH DAMAGE. namespace libtorrent { + namespace { + // this is posted to the network thread - static void watermark_callback(std::vector >* cbs + void watermark_callback(std::vector >* cbs , std::vector* handlers) { if (handlers) @@ -78,13 +80,18 @@ namespace libtorrent if (cbs != NULL) { - for (std::vector >::iterator i = cbs->begin() + for (std::vector >::iterator i = cbs->begin() , end(cbs->end()); i != end; ++i) - (*i)->on_disk(); + { + boost::shared_ptr o = i->lock(); + if (o) o->on_disk(); + } delete cbs; } } + } // anonymous namespace + disk_buffer_pool::disk_buffer_pool(int block_size, io_service& ios , boost::function const& trigger_trim) : m_block_size(block_size) @@ -183,7 +190,7 @@ namespace libtorrent { l.unlock(); m_ios.post(boost::bind(&watermark_callback - , static_cast >*>(NULL) + , static_cast >*>(NULL) , slice)); return; } @@ -195,13 +202,13 @@ namespace libtorrent { l.unlock(); m_ios.post(boost::bind(&watermark_callback - , static_cast >*>(NULL) + , static_cast >*>(NULL) , handlers)); return; } - std::vector >* cbs - = new std::vector >(); + std::vector >* cbs + = new std::vector >(); m_observers.swap(*cbs); l.unlock(); m_ios.post(boost::bind(&watermark_callback, cbs, handlers)); diff --git a/src/session_impl.cpp b/src/session_impl.cpp index e3ca42823..9ae687d63 100644 --- a/src/session_impl.cpp +++ b/src/session_impl.cpp @@ -1153,17 +1153,6 @@ namespace aux { TORRENT_ASSERT_VAL(conn == int(m_connections.size()) + 1, conn); } - m_download_rate.close(); - m_upload_rate.close(); - - // #error closing the udp socket here means that - // the uTP connections cannot be closed gracefully - m_udp_socket.close(); - m_external_udp_port = 0; -#ifdef TORRENT_USE_OPENSSL - m_ssl_udp_socket.close(); -#endif - // we need to give all the sockets an opportunity to actually have their handlers // called and cancelled before we continue the shutdown. This is a bit // complicated, if there are no "undead" peers, it's safe tor resume the @@ -1179,6 +1168,15 @@ namespace aux { void session_impl::abort_stage2() { + m_download_rate.close(); + m_upload_rate.close(); + + m_udp_socket.close(); + m_external_udp_port = 0; +#ifdef TORRENT_USE_OPENSSL + m_ssl_udp_socket.close(); +#endif + // it's OK to detach the threads here. The disk_io_thread // has an internal counter and won't release the network // thread until they're all dead (via m_work).