From 85cf5ac1957e7341dcfe61c50390a1f379864b87 Mon Sep 17 00:00:00 2001 From: arvidn Date: Sat, 19 Mar 2016 23:32:50 -0400 Subject: [PATCH 1/5] remove unused aligned_holder class. fix logging issue of out-of-bound piece requests --- include/libtorrent/allocator.hpp | 23 ----------------------- src/peer_connection.cpp | 12 +++++++----- 2 files changed, 7 insertions(+), 28 deletions(-) diff --git a/include/libtorrent/allocator.hpp b/include/libtorrent/allocator.hpp index bee417229..3c04aaea6 100644 --- a/include/libtorrent/allocator.hpp +++ b/include/libtorrent/allocator.hpp @@ -53,29 +53,6 @@ namespace libtorrent #endif }; - struct TORRENT_EXTRA_EXPORT aligned_holder - { - aligned_holder(): m_buf(0) {} - aligned_holder(int size): m_buf(page_aligned_allocator::malloc(size)) {} - ~aligned_holder() { if (m_buf) page_aligned_allocator::free(m_buf); } - char* get() const { return m_buf; } - void reset(char* buf = 0) - { - if (m_buf) page_aligned_allocator::free(m_buf); - m_buf = buf; - } - void swap(aligned_holder& h) - { - char* tmp = m_buf; - m_buf = h.m_buf; - h.m_buf = tmp; - } - private: - aligned_holder(aligned_holder const&); - aligned_holder& operator=(aligned_holder const&); - char* m_buf; - }; - } #endif diff --git a/src/peer_connection.cpp b/src/peer_connection.cpp index ec8b0431b..5241744d6 100644 --- a/src/peer_connection.cpp +++ b/src/peer_connection.cpp @@ -2212,6 +2212,9 @@ namespace libtorrent m_counters.inc_stats_counter(counters::piece_requests); #ifndef TORRENT_DISABLE_LOGGING + const bool valid_piece_index + = r.piece >= 0 && r.piece < int(t->torrent_file().num_pieces()); + peer_log(peer_log_alert::incoming_message, "REQUEST" , "piece: %d s: %x l: %x", r.piece, r.start, r.length); #endif @@ -2222,9 +2225,6 @@ namespace libtorrent m_counters.inc_stats_counter(counters::invalid_piece_requests); ++m_num_invalid_requests; #ifndef TORRENT_DISABLE_LOGGING - const bool valid_piece_index - = r.piece >= 0 && r.piece < int(t->torrent_file().num_pieces()); - peer_log(peer_log_alert::info, "INVALID_REQUEST", "piece not superseeded " "i: %d t: %d n: %d h: %d ss1: %d ss2: %d" , m_peer_interested @@ -2306,7 +2306,8 @@ namespace libtorrent #ifndef TORRENT_DISABLE_LOGGING peer_log(peer_log_alert::info, "INVALID_REQUEST", "peer is not interested " " t: %d n: %d block_limit: %d" - , int(t->torrent_file().piece_size(r.piece)) + , valid_piece_index + ? int(t->torrent_file().piece_size(r.piece)) : -1 , t->torrent_file().num_pieces() , t->block_size()); peer_log(peer_log_alert::info, "INTERESTED", "artificial incoming INTERESTED message"); @@ -2346,7 +2347,8 @@ namespace libtorrent peer_log(peer_log_alert::info, "INVALID_REQUEST" , "i: %d t: %d n: %d h: %d block_limit: %d" , m_peer_interested - , int(t->torrent_file().piece_size(r.piece)) + , valid_piece_index + ? int(t->torrent_file().piece_size(r.piece)) : -1 , t->torrent_file().num_pieces() , t->has_piece_passed(r.piece) , t->block_size()); From 139fb29647ad8f2a8146285c753c165e5364b02b Mon Sep 17 00:00:00 2001 From: arvidn Date: Sun, 20 Mar 2016 00:55:31 -0400 Subject: [PATCH 2/5] remove unused O_DIRECT mode, and relax/simplify alignment requirements for coalesce_buffers --- include/libtorrent/file.hpp | 4 ---- src/file.cpp | 35 ++++++++++++++--------------------- 2 files changed, 14 insertions(+), 25 deletions(-) diff --git a/include/libtorrent/file.hpp b/include/libtorrent/file.hpp index 93b3a294b..736e94752 100644 --- a/include/libtorrent/file.hpp +++ b/include/libtorrent/file.hpp @@ -271,10 +271,6 @@ namespace libtorrent // leaving running applications in the page cache no_cache = 0x40, - // this corresponds to Linux' O_DIRECT flag - // and may impose alignment restrictions - direct_io = 0x80, - // this is only used for readv/writev flags coalesce_buffers = 0x100, diff --git a/src/file.cpp b/src/file.cpp index 9f42c1bfe..74c126580 100644 --- a/src/file.cpp +++ b/src/file.cpp @@ -1408,7 +1408,6 @@ namespace libtorrent DWORD flags = ((mode & random_access) ? 0 : FILE_FLAG_SEQUENTIAL_SCAN) | (a ? a : FILE_ATTRIBUTE_NORMAL) | FILE_FLAG_OVERLAPPED - | ((mode & direct_io) ? FILE_FLAG_NO_BUFFERING : 0) | ((mode & no_cache) ? FILE_FLAG_WRITE_THROUGH : 0); handle_type handle = CreateFile_(file_path.c_str(), m.rw_mode @@ -1456,16 +1455,13 @@ namespace libtorrent #ifdef O_NOATIME | ((mode & no_atime) ? O_NOATIME : 0) #endif -#ifdef O_DIRECT - | ((mode & direct_io) ? O_DIRECT : 0) -#endif #ifdef O_SYNC | ((mode & no_cache) ? O_SYNC: 0) #endif ; - handle_type handle = ::open(convert_to_native(path).c_str() - , mode_array[mode & rw_mask] | open_mode + handle_type handle = ::open(convert_to_native(path).c_str() + , mode_array[mode & rw_mask] | open_mode , permissions); #ifdef O_NOATIME @@ -1668,12 +1664,11 @@ typedef struct _FILE_ALLOCATED_RANGE_BUFFER { } #if !TORRENT_USE_PREADV - bool coalesce_read_buffers(file::iovec_t const*& bufs, int& num_bufs, file::iovec_t* tmp) + bool coalesce_read_buffers(file::iovec_t const*& bufs, int& num_bufs + , file::iovec_t* tmp) { - int buf_size = bufs_size(bufs, num_bufs); - // this is page aligned since it's used in APIs which - // are likely to require that (depending on OS) - char* buf = static_cast(page_aligned_allocator::malloc(buf_size)); + int const buf_size = bufs_size(bufs, num_bufs); + char* buf = static_cast(malloc(buf_size)); if (!buf) return false; tmp->iov_base = buf; tmp->iov_len = buf_size; @@ -1682,20 +1677,18 @@ typedef struct _FILE_ALLOCATED_RANGE_BUFFER { return true; } - void coalesce_read_buffers_end(file::iovec_t const* bufs, int num_bufs, char* buf, bool copy) + void coalesce_read_buffers_end(file::iovec_t const* bufs, int const num_bufs + , char* const buf, bool const copy) { if (copy) scatter_copy(bufs, num_bufs, buf); - page_aligned_allocator::free(buf); + free(buf); } - bool coalesce_write_buffers(file::iovec_t const*& bufs, int& num_bufs, file::iovec_t* tmp) + bool coalesce_write_buffers(file::iovec_t const*& bufs, int& num_bufs + , file::iovec_t* tmp) { - // coalesce buffers means allocate a temporary buffer and - // issue a single write operation instead of using a vector - // operation - int buf_size = 0; - for (int i = 0; i < num_bufs; ++i) buf_size += bufs[i].iov_len; - char* buf = static_cast(page_aligned_allocator::malloc(buf_size)); + int const buf_size = bufs_size(bufs, num_bufs); + char* buf = static_cast(malloc(buf_size)); if (!buf) return false; gather_copy(bufs, num_bufs, buf); tmp->iov_base = buf; @@ -2243,7 +2236,7 @@ typedef struct _FILE_ALLOCATED_RANGE_BUFFER { // return the offset to the next allocated region return buffer.FileOffset.QuadPart; - + #elif defined SEEK_DATA // this is supported on solaris boost::int64_t ret = lseek(native_handle(), start, SEEK_DATA); From a9d12766d47272ea401063c56319f1f002e599e6 Mon Sep 17 00:00:00 2001 From: arvidn Date: Sun, 20 Mar 2016 16:14:29 -0400 Subject: [PATCH 3/5] fix typo in hard_link() --- src/file.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/file.cpp b/src/file.cpp index 74c126580..6202c2301 100644 --- a/src/file.cpp +++ b/src/file.cpp @@ -538,7 +538,7 @@ namespace libtorrent // TODO: 3 find out what error code is reported when the filesystem // does not support hard links. DWORD error = GetLastError(); - if (error != ERROR_NOT_SUPPORTED || error != ERROR_ACCESS_DENIED) + if (error != ERROR_NOT_SUPPORTED && error != ERROR_ACCESS_DENIED) { // it's possible CreateHardLink will copy the file internally too, // if the filesystem does not support it. From 0095aa084f4c774b1ec87485306aa512ae66fd1f Mon Sep 17 00:00:00 2001 From: arvidn Date: Sun, 20 Mar 2016 11:38:55 -0400 Subject: [PATCH 4/5] make coalesce reads and coalesce writes actually work --- include/libtorrent/disk_io_job.hpp | 5 +---- src/disk_io_thread.cpp | 33 +++++++++++++++++++----------- src/file.cpp | 20 +++++++++++++++--- src/storage.cpp | 12 +++++++---- test/test_file.cpp | 32 +++++++++++++++++++++++++++++ test/test_transfer.cpp | 26 ++++++++++++++++++++++- 6 files changed, 104 insertions(+), 24 deletions(-) diff --git a/include/libtorrent/disk_io_job.hpp b/include/libtorrent/disk_io_job.hpp index 25bbdc485..8baa89301 100644 --- a/include/libtorrent/disk_io_job.hpp +++ b/include/libtorrent/disk_io_job.hpp @@ -133,10 +133,7 @@ namespace libtorrent // this job is currently being performed, or it's hanging // on a cache piece that may be flushed soon - in_progress = 0x20, - - // turns into file::coalesce_buffers in the file operation - coalesce_buffers = 0x40 + in_progress = 0x20 }; // for write jobs, returns true if its block diff --git a/src/disk_io_thread.cpp b/src/disk_io_thread.cpp index 10ae2c7fd..0499608a0 100644 --- a/src/disk_io_thread.cpp +++ b/src/disk_io_thread.cpp @@ -131,12 +131,12 @@ namespace libtorrent #endif } - int file_flags_for_job(disk_io_job* j) + int file_flags_for_job(disk_io_job* j + , bool const coalesce_buffers) { int ret = 0; - if (!(j->flags & disk_io_job::sequential_access)) ret |= file::random_access; - if (j->flags & disk_io_job::coalesce_buffers) ret |= file::coalesce_buffers; + if (coalesce_buffers) ret |= file::coalesce_buffers; return ret; } @@ -639,6 +639,9 @@ namespace libtorrent DLOG("]\n"); #endif + int const file_flags = m_settings.get_bool(settings_pack::coalesce_writes) + ? file::coalesce_buffers : 0; + // issue the actual write operation file::iovec_t const* iov_start = iov; int flushing_start = 0; @@ -652,7 +655,7 @@ namespace libtorrent , i - flushing_start , piece + flushing[flushing_start] / blocks_in_piece , (flushing[flushing_start] % blocks_in_piece) * block_size - , 0, error); + , file_flags, error); if (ret < 0 || error) failed = true; iov_start = &iov[i]; flushing_start = i; @@ -1188,7 +1191,8 @@ namespace libtorrent time_point start_time = clock_type::now(); - int file_flags = file_flags_for_job(j); + int const file_flags = file_flags_for_job(j + , m_settings.get_bool(settings_pack::coalesce_reads)); file::iovec_t b = { j->buffer.disk_block, size_t(j->d.io.buffer_size) }; int ret = j->storage->get_storage_impl()->readv(&b, 1 @@ -1262,7 +1266,8 @@ namespace libtorrent // can remove them. We can now release the cache mutex and dive into the // disk operations. - int file_flags = file_flags_for_job(j); + int const file_flags = file_flags_for_job(j + , m_settings.get_bool(settings_pack::coalesce_reads)); time_point start_time = clock_type::now(); ret = j->storage->get_storage_impl()->readv(iov, iov_len @@ -1270,7 +1275,7 @@ namespace libtorrent if (!j->error.ec) { - boost::uint32_t read_time = total_microseconds(clock_type::now() - start_time); + boost::uint32_t const read_time = total_microseconds(clock_type::now() - start_time); m_read_time.add_sample(read_time / iov_len); m_stats_counters.inc_stats_counter(counters::num_blocks_read, iov_len); @@ -1419,8 +1424,9 @@ namespace libtorrent { time_point start_time = clock_type::now(); - file::iovec_t b = { j->buffer.disk_block, size_t(j->d.io.buffer_size) }; - int file_flags = file_flags_for_job(j); + file::iovec_t const b = { j->buffer.disk_block, size_t(j->d.io.buffer_size) }; + int const file_flags = file_flags_for_job(j + , m_settings.get_bool(settings_pack::coalesce_writes)); m_stats_counters.inc_stats_counter(counters::num_writing_threads, 1); @@ -2232,7 +2238,8 @@ namespace libtorrent int const piece_size = j->storage->files()->piece_size(j->piece); int const block_size = m_disk_cache.block_size(); int const blocks_in_piece = (piece_size + block_size - 1) / block_size; - int const file_flags = file_flags_for_job(j); + int const file_flags = file_flags_for_job(j + , m_settings.get_bool(settings_pack::coalesce_reads)); file::iovec_t iov; iov.iov_base = m_disk_cache.allocate_buffer("hashing"); @@ -2278,7 +2285,8 @@ namespace libtorrent INVARIANT_CHECK; int const piece_size = j->storage->files()->piece_size(j->piece); - int const file_flags = file_flags_for_job(j); + int const file_flags = file_flags_for_job(j + , m_settings.get_bool(settings_pack::coalesce_reads)); mutex::scoped_lock l(m_cache_mutex); @@ -2628,7 +2636,8 @@ namespace libtorrent || m_settings.get_bool(settings_pack::use_read_cache) == false) return 0; - int file_flags = file_flags_for_job(j); + int const file_flags = file_flags_for_job(j + , m_settings.get_bool(settings_pack::coalesce_reads)); mutex::scoped_lock l(m_cache_mutex); diff --git a/src/file.cpp b/src/file.cpp index 6202c2301..93f1e93f2 100644 --- a/src/file.cpp +++ b/src/file.cpp @@ -1827,8 +1827,16 @@ typedef struct _FILE_ALLOCATED_RANGE_BUFFER { int ret = iov(&::preadv, native_handle(), file_offset, bufs, num_bufs, ec); #else + // there's no point in coalescing single buffer writes + if (num_bufs == 1) + { + flags &= ~file::coalesce_buffers; + } + file::iovec_t tmp; - if (flags & file::coalesce_buffers) + file::iovec_t const* const orig_bufs = bufs; + int const orig_num_bufs = num_bufs; + if ((flags & file::coalesce_buffers)) { if (!coalesce_read_buffers(bufs, num_bufs, &tmp)) // ok, that failed, don't coalesce this read @@ -1841,8 +1849,8 @@ typedef struct _FILE_ALLOCATED_RANGE_BUFFER { int ret = iov(&::read, native_handle(), file_offset, bufs, num_bufs, ec); #endif - if (flags & file::coalesce_buffers) - coalesce_read_buffers_end(bufs, num_bufs + if ((flags & file::coalesce_buffers)) + coalesce_read_buffers_end(orig_bufs, orig_num_bufs , static_cast(tmp.iov_base), !ec); #endif @@ -1877,6 +1885,12 @@ typedef struct _FILE_ALLOCATED_RANGE_BUFFER { int ret = iov(&::pwritev, native_handle(), file_offset, bufs, num_bufs, ec); #else + // there's no point in coalescing single buffer writes + if (num_bufs == 1) + { + flags &= ~file::coalesce_buffers; + } + file::iovec_t tmp; if (flags & file::coalesce_buffers) { diff --git a/src/storage.cpp b/src/storage.cpp index db8613870..1ee74249d 100644 --- a/src/storage.cpp +++ b/src/storage.cpp @@ -230,7 +230,9 @@ namespace libtorrent , m_flags(flags) {} - int file_op(int file_index, boost::int64_t file_offset, int size + int file_op(int const file_index + , boost::int64_t const file_offset + , int const size , file::iovec_t const* bufs, storage_error& ec) TORRENT_OVERRIDE TORRENT_FINAL { @@ -314,12 +316,14 @@ namespace libtorrent struct read_fileop : fileop { - read_fileop(default_storage& st, int flags) + read_fileop(default_storage& st, int const flags) : m_storage(st) , m_flags(flags) {} - int file_op(int file_index, boost::int64_t file_offset, int size + int file_op(int const file_index + , boost::int64_t const file_offset + , int const size , file::iovec_t const* bufs, storage_error& ec) TORRENT_OVERRIDE TORRENT_FINAL { @@ -396,7 +400,7 @@ namespace libtorrent private: default_storage& m_storage; - int m_flags; + int const m_flags; }; default_storage::default_storage(storage_params const& params) diff --git a/test/test_file.cpp b/test/test_file.cpp index f5f9fa88f..349e7302b 100644 --- a/test/test_file.cpp +++ b/test/test_file.cpp @@ -360,3 +360,35 @@ TORRENT_TEST(hard_link) fprintf(stderr, "remove failed: [%s] %s\n", ec.category().name(), ec.message().c_str()); } +TORRENT_TEST(coalesce_buffer) +{ + error_code ec; + file f; + TEST_CHECK(f.open("test_file", file::read_write, ec)); + if (ec) + fprintf(stderr, "open failed: [%s] %s\n", ec.category().name(), ec.message().c_str()); + TEST_EQUAL(ec, error_code()); + if (ec) fprintf(stderr, "%s\n", ec.message().c_str()); + file::iovec_t b[2] = {{(void*)"test", 4}, {(void*)"foobar", 6}}; + TEST_EQUAL(f.writev(0, b, 2, ec, file::coalesce_buffers), 4 + 6); + if (ec) + fprintf(stderr, "writev failed: [%s] %s\n", ec.category().name(), ec.message().c_str()); + TEST_CHECK(!ec); + char test_buf1[5] = {0}; + char test_buf2[7] = {0}; + b[0].iov_base = test_buf1; + b[0].iov_len = 4; + b[1].iov_base = test_buf2; + b[1].iov_len = 6; + TEST_EQUAL(f.readv(0, b, 2, ec), 4 + 6); + if (ec) + { + fprintf(stderr, "readv failed: [%s] %s\n" + , ec.category().name(), ec.message().c_str()); + } + TEST_EQUAL(ec, error_code()); + TEST_CHECK(strcmp(test_buf1, "test") == 0); + TEST_CHECK(strcmp(test_buf2, "foobar") == 0); + f.close(); +} + diff --git a/test/test_transfer.cpp b/test/test_transfer.cpp index cb00755da..bb144c0a6 100644 --- a/test/test_transfer.cpp +++ b/test/test_transfer.cpp @@ -228,7 +228,7 @@ void test_transfer(int proxy_type, settings_pack const& sett create_directory("tmp1_transfer", ec); std::ofstream file("tmp1_transfer/temporary"); - boost::shared_ptr t = ::create_torrent(&file, "temporary", 16 * 1024, 13, false); + boost::shared_ptr t = ::create_torrent(&file, "temporary", 32 * 1024, 13, false); file.close(); TEST_CHECK(exists(combine_path("tmp1_transfer", "temporary"))); @@ -421,6 +421,30 @@ TORRENT_TEST(allow_fast) cleanup(); } +TORRENT_TEST(coalesce_reads) +{ + using namespace libtorrent; + // test allowed fast + settings_pack p; + p.set_int(settings_pack::read_cache_line_size, 16); + p.set_bool(settings_pack::coalesce_reads, true); + test_transfer(0, p, false); + + cleanup(); +} + +TORRENT_TEST(coalesce_writes) +{ + using namespace libtorrent; + // test allowed fast + settings_pack p; + p.set_bool(settings_pack::coalesce_writes, true); + test_transfer(0, p, false); + + cleanup(); +} + + TORRENT_TEST(allocate) { using namespace libtorrent; From 2dd28c1708560be54cafc8c78f9482ed2e1437bf Mon Sep 17 00:00:00 2001 From: arvidn Date: Sun, 20 Mar 2016 17:00:49 -0400 Subject: [PATCH 5/5] just use malloc/free in test_storage (instead of page_aligned_allocator). file no longer requires alignment --- test/test_storage.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/test_storage.cpp b/test/test_storage.cpp index a032f0b54..d4c2b255a 100644 --- a/test/test_storage.cpp +++ b/test/test_storage.cpp @@ -169,7 +169,7 @@ typedef boost::shared_array buf_ptr; buf_ptr new_piece(int size) { - buf_ptr ret(page_aligned_allocator::malloc(size), &page_aligned_allocator::free); + buf_ptr ret(static_cast(malloc(size)), &free); std::generate(ret.get(), ret.get() + size, random_byte); return ret; } @@ -209,7 +209,7 @@ void run_storage_tests(boost::shared_ptr info , unbuffered ? settings_pack::disable_os_cache : settings_pack::enable_os_cache); - char* piece = page_aligned_allocator::malloc(piece_size); + char* piece = static_cast(malloc(piece_size)); { // avoid having two storages use the same files file_pool fp; @@ -290,7 +290,7 @@ void run_storage_tests(boost::shared_ptr info s->release_files(ec); } - page_aligned_allocator::free(piece); + free(piece); } void test_remove(std::string const& test_path, bool unbuffered)