explicitly FlushViewOfFile() on windows when closing a file

[arvidn]

This is to address the issue of windows not pre-emptively flushing dirty pages to disk in memory mapped files.

[sledgehammer999]

I corrected the typo and build it. I can't say I saw a difference.
Apparently you only flush on closing the file (when complete), right?

[arvidn]

yeah, there needs to be some periodic flushing too

[sledgehammer999]

@arvidn any news on this? It is kinda a blocker item for qbittorrent.
IMO, we can't do an official release based on RC_2_0 when this "bug" is happening.

[arvidn]

@sledgehammer999 I don't have a very good windows environment to test this on. I think this might work, but I can't say for sure. Would you mind giving it a try?

[sledgehammer999]

Do I need to define TORRENT_HAVE_MAP_VIEW_OF_FILE somewhere? Your diff doesn't seem to define it.
Moreover, from a fast check I did, RAM usage keeps climbing up. I stopped when it reached 13.5GB out of the 16GB available.

But now the file size on disk seems to be updating. This is strange.

[arvidn]

Do I need to define TORRENT_HAVE_MAP_VIEW_OF_FILE somewhere?

TORRENT_HAVE_MAP_VIEW_OF_FILE is defined in config.hpp.

from a fast check I did, RAM usage keeps climbing up. I stopped when it reached 13.5GB out of the 16GB available.

But now the file size on disk seems to be updating. This is strange.

That suggests that the flushing is working. I suppose the non-dirty pages are kept in RAM but would be the first to be evicted under memory pressure.

[sledgehammer999]

I suppose the non-dirty pages are kept in RAM but would be the first to be evicted under memory pressure.

I suppose I can try to let it exhaust the whole RAM and see what happens...

Does the view map the whole file or parts of it(based on the down/up requests)?

[sledgehammer999]

I suppose I can try to let it exhaust the whole RAM and see what happens...

I let it download a ~20GB file. Eventually the RAM was exhausted hovering between 15.8 and 15.9GB usage in the Task Manager tab.
However, regular PC usage didn't seem to be impacted. Along with the download I was browsing, opening files, watching youtube, listening to music. During the RAM exhaustion I didn't see any slowdown to the above. Only twice at random the streaming music from youtube did a hiccup/freeze.
I lack the imagination on how to benchmark this under more load of the PC.

Lastly, I am a bit skeptical of the regular user reaction when they see full RAM usage even though in actuality it won't impact them. In the future, you might want to periodically unmap the view so the OS reclaims instantly the memory. The criteria could be huge files and long running operations on them.

[arvidn]

There is a feature to periodically close the least recently used file in the file pool. For an actively seeded large file for though, the large file won't likely be the least recently used.

Libtorrent does map the entire files. I was considering mapping smaller views (to support 32 bit machines). But the complexity of that is quite high, only to gain 32 bit support.

[arvidn]

There's also this classic :)

https://www.linuxatemyram.com

[sledgehammer999]

I was considering mapping smaller views (to support 32 bit machines). But the complexity of that is quite high, only to gain 32 bit support.

I understand. I want to ask this: Does this mean that RC_2_0 isn't actually suitable for 32bit binaries? Will it fail if it tries to map a big file? Or does it have a fallback mechanism?
This is an important detail to know if I should provide 32bit binaries or not.

There's also this classic :)

At least I hope that on linux/macos the file maps don't need manual flushing like on Windows to write data to disk. I haven't tested this situation on those platforms.

[arvidn]

There is a fallback mechanism for 32 bit systems, and systems that don't support memory mapped files. However, it's single-threaded. Also, people are reporting that the fopen-family of functions also fail intermittently on windows.

[arvidn]

this is the option to close files periodically. It's supposed to be set to 120 seconds on windows. http://libtorrent.org/reference-Settings.html#close_file_interval

[sledgehammer999]

this is the option to close files periodically. It's supposed to be set to 120 seconds on windows. http://libtorrent.org/reference-Settings.html#close_file_interval

Eureka!
I think you have a bug in your sources. I grepped them and you set that setting to zero unconditionally. See:

libtorrent/src/settings_pack.cpp

Line 359 in 139131e

SET(close_file_interval, 0, nullptr),

I set it explicitly to 120 seconds in qbt. Now the RAM usage doesn't take all available RAM.
Moreover, I used it with vanilla RC_2_0 and it does flush to disk after the file is closed. This PR is kinda unneeded. However, I am not positive that a 2 minute* window is good for data to reside in RAM.

*plus the time it takes the system manager to lazily commit it. Supposedly it commits 1/8 of cached data every second.

[sledgehammer999]

This PR is kinda unneeded. However, I am not positive that a 2 minute* window is good for data to reside in RAM.

Another sideeffect is that it tanks the download speed when closing the file. Does this mean it is a synchronous call?
For example I might be downloading a popular torrent at 9MB/sec which after 2 minutes has about 1GB cached. During the close these data committing to HDD (slow). And for 2-3 seconds the download speed is tanked to 1-3MB/sec.
I don't know if this PR suffers from the same thing because the 30seconds gap doesn't let it accumulate much data.

EDIT: I observe similar speed drops every 30 seconds with this patch too.

[arvidn]

interesting. the FlushViewOfFile() is documented to be asynchronous. So the back-pressure to the network thread is probably happening in the threads faulting in new pages of the memory mapped file. Presumably the speed drops every 30 seconds are shorter though, right?

[arvidn]

could you try increasing the number of disk threads, to see if that makes a difference?
If it's some lock contention in the memory manager that causes the slow-downs, it probably won't.

[arvidn]

I still think this PR serves a purpose. The flushing of file views in this patch round-robins the files, whereas the file close interval closes the least recently used file. so, if you have one file that's more commonly accessed, it may never be closed, but in this patch it will at least be flushed periodically.

[arvidn]

in fact, I wonder if the 30 seconds flush interval that's hard coded here should be proportional to the size of the file view pool. The large the pool, the more rare each individual file will be flushed.

[sledgehammer999]

could you try increasing the number of disk threads, to see if that makes a difference?

I've set aio_threads to 16 but I still see speed dropdowns during flush. It is random but is is always at least 2MB/sec dropdown from whatever speed I am at.

Presumably the speed drops every 30 seconds are shorter though, right?

Personally, I don't observe a difference. I am not measuring this via code. Rough estimation of seeing the values fluctuate in the GUI.

I still think this PR serves a purpose. The flushing of file views in this patch round-robins the files, whereas the file close interval closes the least recently used file. so, if you have one file that's more commonly accessed, it may never be closed, but in this patch it will at least be flushed periodically.

👍

The large the pool, the more rare each individual file will be flushed.

Do you mean the opposite or did I misunderstand you?

in fact, I wonder if the 30 seconds flush interval that's hard coded here should be proportional

Maybe a combination of "data received since last flush" and a hard time limit since last flush?

[arvidn]

The way this patch works is that it flushes the next file in the file_view_pool every 30 seconds. If there are a lot of files in the pool, one specific file will be flushed less often than every 30 seconds. The larger the file pool is, the longer it will take to rotate back to the same file again.

[glassez]

The way this patch works is that it flushes the next file in the file_view_pool every 30 seconds. If there are a lot of files in the pool, one specific file will be flushed less often than every 30 seconds. The larger the file pool is, the longer it will take to rotate back to the same file again.

I'm sorry, I haven't followed this topic in great detail, so my question may be a bit silly... What about downloading one large file, the size of which exceeds the amount of RAM? As far as I understand, the main problem is with such torrents, which contain large files, and not just have a total large size. Will such a file be flushed to disk regularly so as not to clog RAM with dirty pages, as is happening now?

[arvidn]

Yes. Fundamentally it's windows responsibility to flush dirty pages to disk and evict clean pages from the disk cache. If you leave all this to the OS, I don't think windows will completely lock up or break in some way, but it seems it's not very good at it, hence the "manual" periodic flushing of the file cache.

So, to answer your question; yes it will work and this patch will also ask windows to flush dirty pages periodically, to prevent it from just accruing GBs of them.

[arvidn]

That said, I only have windows in a VM, so I don't have a good sense of how the system actually performs. But above is my understanding at least.

[glassez]

The way this patch works is that it flushes the next file in the file_view_pool every 30 seconds.

I just want to make sure that any file will be flushed periodically, even if there are only one file in the pool.

[arvidn]

I just want to make sure that any file will be flushed periodically, even if there are only one file in the pool.

yes. in that scenario the file will be flushed every 30 seconds. If there are two files in the pool, one specific file will be flushed every 60 seconds. If the file isn't in the pool anymore, it has been closed, which also implies a flush.

[arvidn]

the worst-case scenario I can think of is having a very large file pool size, say several thousand file handles. A torrent with one very large file and hundreds of tiny files. Most disk activity will happen in the large file, but the small files wil "dilute" the round-robin flushing, possibly making the large file only be flushed very rarely. And the "flushing" of the tiny files ending up being no-ops, because essentially no activity is happening on those.

[sledgehammer999]

Would it be costly to just flush every open file every 30secs?

[arvidn]

I don't know. in that case it would probably make sense to make this interval configurable.

The way I would implement that would be to make the interval be 30 / file_pool.size()

[glassez]

Would it be costly to just flush every open file every 30secs?

I don't think any time interval is an appropriate measure for this. It seems to me that it would be better to perform FlushViewOfFile() for file after writing a certain amount of data to it.

[AllSeeingEyeTolledEweSew]

Has anyone experimented to see if large reads on a file mapping cause the same kind of memory management problems as large writes? I wouldn't expect problems, but it seems you never know with windows.

IIUC from the docs, FlushViewOfFile should just flush dirty pages. It looks like the code currently flushes all files. Could it instead only flush known-dirty ones?

FWIW, this article claims FlushViewOfFile is O(N) in the size of the range being referred to, and can be optimized by flushing only known-dirty ranges.

[arvidn]

IIUC from the docs, FlushViewOfFile should just flush dirty pages. It looks like the code currently flushes all files. Could it instead only flush known-dirty ones?

I consider this a future improvement. It's not obvious that it would be more efficient to track every 16 kiB block that has been written since the last flush and flush just those.

[arvidn]

but maybe it is a hint that FlushViewOfFile() shouldn't be called this frequently. every 2 seconds.

[AllSeeingEyeTolledEweSew]

IIUC from the docs, FlushViewOfFile should just flush dirty pages. It looks like the code currently flushes all files. Could it instead only flush known-dirty ones?

I consider this a future improvement. It's not obvious that it would be more efficient to track every 16 kiB block that has been written since the last flush and flush just those.

sorry; I meant: could we flush only known-dirty files? We could still flush the whole range of a file; I agree tracking ranges more granularly is complex

[glassez]

Has anyone experimented to see if large reads on a file mapping cause the same kind of memory management problems as large writes?

What exactly is bothering you about reading? Can it cause the same problems as writing? Isn't the main problem of writing that we are discussing is the accumulation of a large number of dirty pages in RAM, which leads either to performance problems when they all start being written to disk when the file is closed, or to data loss in the event of a sudden system crash (or power loss)?

[AllSeeingEyeTolledEweSew]

It itches my brain that Windows appears to implement file mappings as a second layer above their page cache, and that some believe this is done because their page cache has bad performance characteristics.

If those things are true, then maybe they also thought it would be "helpful" to cache reads as well as writes in that second layer.

[arvidn]

I'm having second thoughts about this patch. Considering that FlushViewOfFile() might traverse all pages of a mapping, to find the dirty ones, it might be extremely inefficient to make these calls unconditionally once a torrent is seeding. We know there are no dirty pages, yet we ask windows to traverse them all.

The reason I've been hesitant to flush only files with dirty pages, or just the dirty parts of those files is the additional complexity of tracking those things. Right now, the concurrency considerations are quite simple. The main file handle container is protected by one mutex and (in this patch) encodes the round-robin order of flushing the files.

In order to track files that have been written to, one has to take into account that the writing happens in separate threads, with no other synchronization. There would have to be a separate index of references to these files, protected by its own mutex.

[arvidn]

this patch will, every 30 seconds, flush the file with the most bytes written to it since the last flush.
The benefit compared to the previous version is that a client that is only seeding will never invoke the (redundant) flush calls.
A possible drawback is that if a single file is the most actively written to, other files with dirty pages won't be considered (until the active files is completely downloaded and writes cease).

[glassez]

this patch will, every 30 seconds, flush the file with the most bytes written to it since the last flush. The benefit compared to the previous version is that a client that is only seeding will never invoke the (redundant) flush calls. A possible drawback is that if a single file is the most actively written to, other files with dirty pages won't be considered (until the active files is completely downloaded and writes cease).

Along with this, you could also provide an API method so that application developers can apply their own flushing optimization algorithms. Then you also need to provide the ability to disable the built-in flushing mechanism (or even better, configure it - maybe it will be enough for someone to change its timeout).

[arvidn]

@sledgehammer999 could you give this new patch a try?
my testing suggests that it works as intended

[sledgehammer999]

@arvidn give me ~5-6hours when I'll be home to test it.

[sledgehammer999]

Test case: Torrent with a 20GB file downloading
Result:

1. Filesize on disk seems to be updated regularly. Probably every 30secs.
2. System RAM usage seems to be under control. It gets "released" after ~2GBs are used every few minutes.
3. I didn't observe any speed slowdown during flushing. But don't take my word for it. It was a simple test. It may have been just luck.
4. Not important: File timestamps for accessed/modified time don't get updated after flushing.

[arvidn]

thanks for testing!