linuxdcpp-team team mailing list archive

[cologic <1909861@xxxxxxxxxxxxxxxxxx>]

Apropos "So possibly the FileReader methods should rather be called
readAsync (overlapped/AIO) and readUnbuffered
(FILE_FLAG_NO_BUFFERING/O_DIRECT/F_NOCACHE). Is buffered reading even
needed there? When would that work if the unbuffered read fails?":

An unbuffered read might depend on buffer alignment, e.g., in NetBSD:
"To meet the alignment requirements for direct I/O, file offset, the length of the I/O and the address of the buffer in memory must all be multiples of DEV_BSIZE (512 bytes)." A buffered read doesn't have that requirement, so might succeed where an unbuffered read fails. There'd still be some value in a buffered fallback, then.

A file reading framework for hashing or uploading might ideally:
- be asynchronous, e.g., OVERLAPPED on Windows, io_uring on Linux, and kqueue() on BSDs/macOS;
- and avoid pointlessly filling disk cache with file contents that will likely not be read again within a relevant timeframe, while still allowing the OS to pre-fetch/readahead cache for blocking calls:
  (1) FILE_FLAG_SEQUENTIAL_SCAN for non-OVERLAPPED, because readahead caching's valuable for blocking operations, and FILE_FLAG_NO_BUFFERING for OVERLAPPED on Windows, per https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew, which states "When FILE_FLAG_NO_BUFFERING is combined with FILE_FLAG_OVERLAPPED, the flags give maximum asynchronous performance, because the I/O does not rely on the synchronous operations of the memory manager. However, some I/O operations take more time, because data is not being held in the cache."
  (2a) POSIX_FADV_SEQUENTIAL potentially combined with POSIX_FADV_DONTNEED for already-read files/sections of file and O_DIRECT/F_NOCACHE when using io_uring/aio/kqueue for similar reasons as FILE_FLAG_NO_BUFFERING and FILE_FLAG_OVERLAPPED work together in Windows; or
  (2b) with a different speed/memory usage tradeoff than (2a), using FILE_FLAG_NO_BUFFERING/O_DIRECT/F_NOCACHE regardless of whether the reads are asynchronous. This is slightly slower in your benchmarks, likely being hurt by disabling readahead/pre-fetch.  Still, it's possible that FILE_FLAG_SEQUENTIAL_SCAN clobbers the OS disk caches when one pushes 20GB of hashed files through it.

If it's fast enough, and explicitly in async cases,
FILE_FLAG_NO_BUFFERING/O_DIRECT/F_NOCACHE might probably on balance be
better, but does create buffer alignment requirements on various
platforms, which might require more testing, and/or a buffered fallback.
As you suggest, I'd be curious too about how well
FILE_FLAG_NO_BUFFERING/O_DIRECT/F_NOCACHE works with high-latency
devices, such as HDDs and slower networks; fast networks with SSDs on
the other end should have lower end-to-end latency than typical HDDs
(~10ms).

https://github.com/facebook/rocksdb/issues/1032#issuecomment-196874121
suggests that rocksdb does (2a), going through the file sequentially and
then explicitly dropping cache behind itself, or did in 2016 at least. I
couldn't verify from their current code that they still do that reading,
though they definitely do it when writing files they're not going to
read back: they write the file, then call with POSIX_FADV_DONTNEED
afterwards.

Asynchronous file I/O is useful here partly because it allows for
avoiding file caching with fewer other tradeoffs, especially in AirDC++,
where one might have multiple hashing threads.

On Windows, DC++, at least, has long used:
size_t FileReader::readDirect(const string& file, const DataCallback& callback) {
	...

	auto tmp = ::CreateFile(tfile.c_str(), GENERIC_READ, FILE_SHARE_READ, nullptr, OPEN_EXISTING,
		FILE_FLAG_NO_BUFFERING | FILE_FLAG_OVERLAPPED | FILE_FLAG_POSIX_SEMANTICS, nullptr);

Capturing the asynchronous case already. As far as I understand, neither
DC++ nor AirDC++ read files asynchronously on any non-Windows platform,
to capture the better case (1). This is likely out of scope of near-term
DC++ development, but since AirDC++ does support non-Windows platforms,
might prove worthwhile.

So, it looks reasonable to have DC++ and AirDC++ on each supported
platform prefer (1), and use one or both of (2a) and (2b), depending on
the speed/fragility hit from (2a). If it's possible to align and size
buffers reliably to match (2a) requirements, then that's probably more
elegant, and if it also doesn't cause too much trouble with higher-
latency I/O devices, it's probably overall more elegant.

** Bug watch added: github.com/facebook/rocksdb/issues #1032
   https://github.com/facebook/rocksdb/issues/1032

-- 
You received this bug notification because you are a member of
Dcplusplus-team, which is subscribed to DC++.
https://bugs.launchpad.net/bugs/1909861

Title:
  FileReader is not thread safe on Linux

Status in DC++:
  New

Bug description:
  FileReader::readMapped currently modifies the global SIGBUS handler in
  order to catch read errors:

  https://sourceforge.net/p/dcplusplus/code/ci/default/tree/dcpp/FileReader.cpp#l289

  Since the function can be called concurrently from different threads
  (currently hashing/queue recheck/sfv check in DC++) and each of them
  sets and resets the SIGBUS handler, there's a high risk that the
  application will crash in case of read errors as they aren't being
  handler properly.

  More information about the caveats:
  https://www.sublimetext.com/blog/articles/use-mmap-with-care

  These issues are much more likely to happen with AirDC++ as it uses
  multiple threads for hashing. Read errors caused rather nasty crashes
  with corrupted stack traces for one user, but luckily he was able to
  catch the SIGBUS signal with gdb.

  I didn't even spend time in trying to figure out how to make the
  mapped reads work properly, as based on my testing the basic
  FileReader::readCached function is noticeably faster:

  readMapped: 671 files (21.70 GiB) in 9 directories have been hashed in 4 minutes 21 seconds (84.87 MiB/s)
  readCached: 671 files (21.70 GiB) in 9 directories have been hashed in 3 minutes 58 seconds (93.08 MiB/s)

  FileReader::readMapped is now disabled in AirDC++, as I can't see any
  benefits from using it. The included setjmp.h header is even causing
  issues when using clang for compiling on Linux:
  https://bugs.gentoo.org/731676

To manage notifications about this bug go to:
https://bugs.launchpad.net/dcplusplus/+bug/1909861/+subscriptions