From 0232dc35cdd3695f608364ad271b83ba42776072 Mon Sep 17 00:00:00 2001 From: Steven Siloti Date: Sun, 17 Sep 2017 01:43:27 -0700 Subject: [PATCH] deprecate lock_files (#2344) File locking interacts poorly with the path to replace a read only file handle with a read/write one. For performance reasons the old handle is not closed until the new one is put in place. With file locking this obviously cannot work because the first handle is holding a lock on the file. As a result, file_pool::open_file fails in this case. Even if we dropped the reference to the read only file handle before attempting to re-open it, the open could still fail if another thread is holding a reference to the read only handle. There doesn't seem to be a good way around this. The simple solution would be to always open files in read/write mode, but that has obvious safety downsides. Any other solution would significantly complicate the file pool code. Deprecating file locking seems to be the least bad option. --- bindings/python/src/torrent_handle.cpp | 4 +++- examples/client_test.cpp | 1 - include/libtorrent/disk_interface.hpp | 4 +++- include/libtorrent/file.hpp | 12 ++++-------- include/libtorrent/settings_pack.hpp | 4 ++++ src/file.cpp | 8 +------- src/file_pool.cpp | 1 - src/settings_pack.cpp | 2 +- src/storage.cpp | 13 ------------- 9 files changed, 16 insertions(+), 33 deletions(-) diff --git a/bindings/python/src/torrent_handle.cpp b/bindings/python/src/torrent_handle.cpp index 28f4575b3..1e599ddbc 100644 --- a/bindings/python/src/torrent_handle.cpp +++ b/bindings/python/src/torrent_handle.cpp @@ -596,7 +596,9 @@ void bind_torrent_handle() s.attr("sparse") = file_open_mode::sparse; s.attr("no_atime") = file_open_mode::no_atime; s.attr("random_access") = file_open_mode::random_access; - s.attr("locked") = file_open_mode::locked; +#ifndef TORRENT_NO_DEPRECATE + s.attr("locked") = 0; +#endif } enum_("file_progress_flags") diff --git a/examples/client_test.cpp b/examples/client_test.cpp index 62825752e..b7fd097e0 100644 --- a/examples/client_test.cpp +++ b/examples/client_test.cpp @@ -1843,7 +1843,6 @@ COLUMN OPTIONS else if ((f->open_mode & lt::file_open_mode::rw_mask) == lt::file_open_mode::read_only) title += "read "; else if ((f->open_mode & lt::file_open_mode::rw_mask) == lt::file_open_mode::write_only) title += "write "; if (f->open_mode & lt::file_open_mode::random_access) title += "random_access "; - if (f->open_mode & lt::file_open_mode::locked) title += "locked "; if (f->open_mode & lt::file_open_mode::sparse) title += "sparse "; title += "]"; ++f; diff --git a/include/libtorrent/disk_interface.hpp b/include/libtorrent/disk_interface.hpp index bd6b55fdb..043ec2ad5 100644 --- a/include/libtorrent/disk_interface.hpp +++ b/include/libtorrent/disk_interface.hpp @@ -94,9 +94,11 @@ namespace libtorrent { // logic constexpr file_open_mode_t random_access = 5_bit; +#ifndef TORRENT_NO_DEPRECATE // prevent the file from being opened by another process // while it's still being held open by this handle - constexpr file_open_mode_t locked = 6_bit; + constexpr file_open_mode_t TORRENT_DEPRECATED locked = 6_bit; +#endif } // this contains information about a file that's currently open by the diff --git a/include/libtorrent/file.hpp b/include/libtorrent/file.hpp index e6bc80311..89297841d 100644 --- a/include/libtorrent/file.hpp +++ b/include/libtorrent/file.hpp @@ -159,26 +159,22 @@ namespace libtorrent { // logic constexpr open_mode_t random_access = 4_bit; - // prevent the file from being opened by another process - // while it's still being held open by this handle - constexpr open_mode_t lock_file = 5_bit; - // don't put any pressure on the OS disk cache // because of access to this file. We expect our // files to be fairly large, and there is already // a cache at the bittorrent block level. This // may improve overall system performance by // leaving running applications in the page cache - constexpr open_mode_t no_cache = 6_bit; + constexpr open_mode_t no_cache = 5_bit; // this is only used for readv/writev flags - constexpr open_mode_t coalesce_buffers = 7_bit; + constexpr open_mode_t coalesce_buffers = 6_bit; // when creating a file, set the hidden attribute (windows only) - constexpr open_mode_t attribute_hidden = 8_bit; + constexpr open_mode_t attribute_hidden = 7_bit; // when creating a file, set the executable attribute - constexpr open_mode_t attribute_executable = 9_bit; + constexpr open_mode_t attribute_executable = 8_bit; // the mask of all attribute bits constexpr open_mode_t attribute_mask = attribute_hidden | attribute_executable; diff --git a/include/libtorrent/settings_pack.hpp b/include/libtorrent/settings_pack.hpp index 8a1bfcf57..c7e398cbd 100644 --- a/include/libtorrent/settings_pack.hpp +++ b/include/libtorrent/settings_pack.hpp @@ -582,6 +582,7 @@ namespace libtorrent { deprecated19, #endif +#ifndef TORRENT_NO_DEPRECATE // ``lock_files`` determines whether or not to lock files which // libtorrent is downloading to or seeding from. This is implemented // using ``fcntl(F_SETLK)`` on unix systems and by not passing in @@ -589,6 +590,9 @@ namespace libtorrent { // 3rd party processes from corrupting the files under libtorrent's // feet. lock_files, +#else + deprecated26, +#endif #ifndef TORRENT_NO_DEPRECATE // ``contiguous_recv_buffer`` determines whether or not libtorrent diff --git a/src/file.cpp b/src/file.cpp index a6ef862ed..4935c4007 100644 --- a/src/file.cpp +++ b/src/file.cpp @@ -542,7 +542,7 @@ static_assert(!(open_mode::sparse & open_mode::attribute_mask), "internal flags | ((mode & open_mode::no_cache) ? FILE_FLAG_WRITE_THROUGH : 0); handle_type handle = CreateFileW(file_path.c_str(), m.rw_mode - , (mode & open_mode::lock_file) ? FILE_SHARE_READ : FILE_SHARE_READ | FILE_SHARE_WRITE + , FILE_SHARE_READ | FILE_SHARE_WRITE , 0, m.create_mode, flags, 0); if (handle == INVALID_HANDLE_VALUE) @@ -618,12 +618,6 @@ static_assert(!(open_mode::sparse & open_mode::attribute_mask), "internal flags m_file_handle = handle; - // The purpose of the lock_file flag is primarily to prevent other - // processes from corrupting files that are being used by libtorrent. - // the posix file locking mechanism does not prevent others from - // accessing files, unless they also attempt to lock the file. That's - // why the SETLK mechanism is not used here. - #ifdef DIRECTIO_ON // for solaris if ((mode & open_mode::no_cache)) diff --git a/src/file_pool.cpp b/src/file_pool.cpp index 465595c2b..3e422851f 100644 --- a/src/file_pool.cpp +++ b/src/file_pool.cpp @@ -198,7 +198,6 @@ namespace libtorrent { if (mode & open_mode::sparse) ret |= file_open_mode::sparse; if (mode & open_mode::no_atime) ret |= file_open_mode::no_atime; if (mode & open_mode::random_access) ret |= file_open_mode::random_access; - if (mode & open_mode::lock_file) ret |= file_open_mode::locked; return ret; } diff --git a/src/settings_pack.cpp b/src/settings_pack.cpp index 70e1a1b67..3b35fa82b 100644 --- a/src/settings_pack.cpp +++ b/src/settings_pack.cpp @@ -179,7 +179,7 @@ constexpr int CLOSE_FILE_INTERVAL = 0; SET(always_send_user_agent, false, nullptr), SET(apply_ip_filter_to_trackers, true, nullptr), DEPRECATED_SET(use_disk_read_ahead, true, nullptr), - SET(lock_files, false, nullptr), + DEPRECATED_SET(lock_files, false, nullptr), DEPRECATED_SET(contiguous_recv_buffer, true, nullptr), SET(ban_web_seeds, true, nullptr), SET(allow_partial_disk_writes, true, nullptr), diff --git a/src/storage.cpp b/src/storage.cpp index 11916ced1..1b287b7ed 100644 --- a/src/storage.cpp +++ b/src/storage.cpp @@ -637,9 +637,6 @@ namespace libtorrent { file_handle default_storage::open_file_impl(file_index_t file, open_mode_t mode , error_code& ec) const { - bool const lock_files = m_settings ? settings().get_bool(settings_pack::lock_files) : false; - if (lock_files) mode |= open_mode::lock_file; - if (!m_allocate_files) mode |= open_mode::sparse; // files with priority 0 should always be sparse @@ -658,16 +655,6 @@ namespace libtorrent { file_handle ret = m_pool.open_file(storage_index(), m_save_path, file , files(), mode, ec); - if (ec && (mode & open_mode::lock_file)) - { - // we failed to open the file and we're trying to lock it. It's - // possible we're failing because we have another handle to this - // file in use (but waiting to be closed). Just retry to open it - // without locking. - mode &= ~open_mode::lock_file; - ret = m_pool.open_file(storage_index(), m_save_path, file, files() - , mode, ec); - } return ret; }