Add disk IO write mode write-through to advanced settings #17005

ghost · 2022-05-08T09:11:49Z

and separate the disk IO read mode settings.
This also removes the OS cache settings since it's broken into separate configs now.

ghost · 2022-05-08T09:14:52Z

This basically allows this patch to work arvidn/libtorrent#6703 ~~and sets it by default for all OS.~~
That patch is supposed to fix arvidn/libtorrent#6561 and some other RC2_0 issues related to disk cache flushing such as corrupted files on macOS(presumably?)

Side note: Anyone claiming that the libtorrent patch had no effect is because the settings was being overridden by the OS cache settings so basically it was using os cache instead of write through.

xavier2k6 · 2022-05-08T10:20:32Z

Once applying that patch, you also need to enable the option disk_io_write_mode = settings_pack::write_through.

Main point to note!

Reference: arvidn/libtorrent#6561 (comment) & arvidn/libtorrent#6561 (comment)

xavier2k6 · 2022-05-08T10:51:19Z

@Chocobo1 @glassez
May be a good enough reason to now raise libtorrent requirement to 2.0.6 in CMakeLists etc.??

src/base/bittorrent/session.cpp

src/base/bittorrent/session.h

src/base/bittorrent/session.cpp

src/base/bittorrent/session.h

Chocobo1

Although not strictly required, the webui part is absent. Maybe try adding it? (a non-trivial portion of users are using it)

src/base/bittorrent/session.cpp

src/base/bittorrent/session.h

src/gui/advancedsettings.cpp

ghost · 2022-05-10T05:58:23Z

Although not strictly required, the webui part is absent. Maybe try adding it? (a non-trivial portion of users are using it)

Added!

glassez · 2022-05-10T19:20:17Z

@Chocobo1
How about #17005 (comment)?

src/webui/api/appcontroller.cpp

Chocobo1 · 2022-05-11T02:34:13Z

How about #17005 (comment)?

I have no opposing reason to say no.

src/base/bittorrent/session.cpp

src/webui/api/appcontroller.cpp

src/webui/www/private/views/preferences.html

src/gui/advancedsettings.cpp

Rootax · 2022-05-15T10:40:13Z

(for the record, in my case it doesn't solve arvidn/libtorrent#6561 at all : / )

ghost · 2022-05-15T16:00:16Z

(for the record, in my case it doesn't solve arvidn/libtorrent#6561 at all : / )

I’d assume you mean that the write through mode set with the build from github actions doesn’t work for you on a windows system?

Can you provide some more info as to you system configuration, working set size set in qBittorrent and the process memory priority?

Rootax · 2022-05-15T16:11:55Z

(for the record, in my case it doesn't solve arvidn/libtorrent#6561 at all : / )

I’d assume you mean that the write through mode set with the build from github actions doesn’t work for you on a windows system?

Can you provide some more info as to you system configuration, working set size set in qBittorrent and the process memory priority?

Hello,

I mean that the performances (constant download speed in my case) doesn't improve. It's still acting like a yoyo (full speed => very slow while it's writing the piece => full speed => very slow while writing, etc). Of course all is ok with lib 1.2.x.

My system is Windows 10 x64, 32gb of ram, i3-8100i cpu, nvme ssd drive (mp510). I try with process memory low, normal, same stuff. Working size set a 2GB, but the ram usage is always under anyway. Now I guess this PR is very usefull from some people, but for the people affected by inconsistent speed problem, it's not helping a lot. Still, good job doing this : )

The5kull · 2022-05-15T23:00:55Z

I am not experiencing any difference compared to the latest Master-CL build.

The problems I notice are the same as @Rootax mentioned here, here and here. The HDD is reading a lot of MB/s for an unknown reason which slow the writing to HDD down enormously and eventually the download speeds goes down.

Chocobo1 · 2022-05-16T04:12:52Z

@Rootax @The5kull
Just to be sure we are not missing anything: you have to change the new setting "Disk IO write mode" to "Write Through", otherwise it would be the same as before.
Also you might be affected by another issue which is recently resolved in PR #16951.

Rootax · 2022-05-16T04:44:34Z

@Rootax @The5kull Just to be sure we are not missing anything: you have to change the new setting "Disk IO write mode" to "Write Through", otherwise it would be the same as before. Also you might be affected by another issue which is recently resolved in PR #16951.

Yes I changed this setting.

For the other issue, from what I read it's impacting mechanical drives, but ssd, not so much. But even with only one hashing thread, same behaviour.

glassez · 2022-05-16T05:03:46Z

If this is actually relevant then it is real disadvantage of MMap based I/O subsystem.

The5kull · 2022-05-16T13:00:31Z

@Rootax @The5kull Just to be sure we are not missing anything: you have to change the new setting "Disk IO write mode" to "Write Through", otherwise it would be the same as before. Also you might be affected by another issue which is recently resolved in PR #16951.

Yes I changed this setting.

For the other issue, from what I read it's impacting mechanical drives, but ssd, not so much. But even with only one hashing thread, same behaviour.

@Chocobo1 I had the same experience as @Rootax on 1 hashing thread. Also I did set "Disk IO Write Through", even tried it with POSIX but then qB became frequently less responding ie stuck so that setting went back to default (memory mapped).

If this is actually relevant then it is real disadvantage of MMap based I/O subsystem.

From what I have read @arvidn is aware of this since it is not windows-only.

arvidn · 2022-05-16T13:06:00Z

this is related: arvidn/libtorrent#6861

glassez · 2022-06-11T05:04:16Z

What is current status of this PR?

ghost · 2022-06-12T08:21:19Z

What is current status of this PR?

Some previous comments suggest that the slow download is due to unnecessary disk reads and this PR doesn't help their situation. So I'm not confident in continuing with this.

Chocobo1 · 2022-06-14T05:25:48Z

So I'm not confident in continuing with this.

The idea of exposing libtorrent settings is still valuable IMO, it gives users a chance to try something out.

ghost · 2022-06-14T14:33:06Z

So I'm not confident in continuing with this.

The idea of exposing libtorrent settings is still valuable IMO, it gives users a chance to try something out.

I think libtorrent should add back RC1_2 style disk I/O and make mmap I/O optional. The former being more predictable and much less buggy. At this point introducing new things in qBit like working set limit and PRs like this one which are half baked solutions, might end up with unexpected pitfalls.

Rootax · 2022-06-14T14:51:09Z

So I'm not confident in continuing with this.

The idea of exposing libtorrent settings is still valuable IMO, it gives users a chance to try something out.

I think libtorrent should add back RC1_2 style disk I/O and make mmap I/O optional. The former being more predictable and much less buggy. At this point introducing new things in qBit like working set limit and PRs like this one which are half baked solutions, might end up with unexpected pitfalls.

I don't think it will happen, the main "improvement" was
supposed to be the new i/o system, no more cache to manage,etc...

glassez · 2022-06-14T14:55:44Z

I think libtorrent should add back RC1_2 style disk I/O and make mmap I/O optional. The former being more predictable and much less buggy.

This is a huge drawback of libtorrent 2.0, which apparently will not allow it to replace libtorrent 1.2, even despite the support of BitTorrent v2, which no one needs at the cost of such an unsuccessful I/O subsystem. After all, memory problems are far from its only drawback. This can include problems with network drives, problems with the latest macOS, etc.

glassez · 2022-06-14T14:58:11Z

I don't think it will happen, the main "improvement" was
supposed to be the new i/o system, no more cache to manage,etc...

Yes, but it seems to have failed.

arvidn · 2022-06-14T20:36:23Z

I don't think it will happen, the main "improvement" was
supposed to be the new i/o system, no more cache to manage,etc...

Yes, but it seems to have failed.

Yes, at least for some people in some scenarios. The new disk I/O subsystem does not preclude using a threaded pwritev() and preadv() subsystem. It just hasn't been written yet.

src/gui/advancedsettings.cpp

src/base/bittorrent/session.h

Chocobo1 · 2022-07-08T07:08:19Z

src/gui/advancedsettings.cpp

+    // Disk IO write mode
+    m_comboBoxDiskIOWriteMode.addItem(tr("Disable OS Cache"), QVariant::fromValue(BitTorrent::DiskIOWriteMode::DisableOSCache));
+    m_comboBoxDiskIOWriteMode.addItem(tr("Enable OS Cache"), QVariant::fromValue(BitTorrent::DiskIOWriteMode::EnableOSCache));
+    m_comboBoxDiskIOWriteMode.addItem(tr("Write Through (requires libtorrent >= 2.0.6)"), QVariant::fromValue(BitTorrent::DiskIOWriteMode::WriteThrough));


IMO (for GUI only) it isn't good to use a such wording, better use #if to enable/disable it at compile time. Otherwise linux user might complain that they upgraded the libtorrent version from 2.0.5 to 2.0.6 but the option is not working.

then what'll be selected incase someone downgrades?
#17005 (comment)

I changed the wording a bit. See if that helps.

Otherwise linux user might complain that they upgraded the libtorrent version from 2.0.5 to 2.0.6 but the option is not working.

Why not increase requirements up to libtorrent 2.0.6 (for 2.0 branch)?

better use #if to enable/disable it at compile time.

IMO, it's preferred way for GUI.
@summerqb, don't you still understand why we can't do the same in WebUI?

then what'll be selected incase someone downgrades?
#17005 (comment)

The mentioned comment is about differences between the same qBittorrent built with different branches of libtorrent.
I mean if option in config is write-through but libtorrent is 1.2 (e.g. libtorrent 2.0 based build was used previously) you should explicitly replace it with one of the supported options.

then what'll be selected incase someone downgrades?

From previous discussion with other developers, we reached an conclusion: It is often near impossible to provide reliable behavior for downgrades so we usually won't talk about it. We would only consider upgrade scenario.

As for this specific code: qbt will detect incompatible setting (on start up) and use the default value (which is EnableOSCache). So the work around you added in advancedsettings.cpp is unnecessary.

It is often near impossible to provide reliable behavior for downgrades so we usually won't talk about it. We would only consider upgrade scenario.

IMO, we shouldn't still consider switching between libtorrent 1.2 and 2.0 within the same qBittorrent as a clear upgrade/downgrade. Although we cannot consider it as something ordinary and reliable.

src/base/bittorrent/session.cpp

src/gui/advancedsettings.cpp

ghost · 2022-07-13T04:17:01Z

I've updated the PR again. Now when user downgrades libtorrent, nothing is selected. So it most likely falls back to default case.

Chocobo1

One last comment and then it is ready

src/base/bittorrent/session.cpp

Chocobo1 · 2022-07-13T06:28:07Z

Appveyor is failing, so either its libtorrent needs to be updated or qbt needs to bump its supported libtorrent version number.

Splits the OS cache settings into Disk IO read/write modes

glassez · 2022-07-13T07:15:46Z

Now when user downgrades libtorrent, nothing is selected. So it most likely falls back to default case.

And what will be saved to the settings file if the user applies some changes?

I would still prefer to see the option that is effectively used.

It seems that we are confused about what we want to get in the end.
The cornerstone of the problem is the Web API, which should be stable within its version, regardless of the libtorrent version used, etc., i.e. it should have a single set of methods/endpoints and supported parameters.
But at the application core level, support for certain functions may differ. So, for example, Write-through is not supported by the libtorrent < 2.0.6 based builds, so it is an invalid value. If it is contained in the configuration file or in the Web API request, then it should be treated as any other invalid value (replaced by the default value). The WebAPI can simply return the runtime error code in this case (for example, HTTP Conflict status). As for the UI, the GUI simply should not display invalid options. The WebUI is more difficult. It can either display all options, but have warnings that some of them are not supported by some version of libtorrent, or you can try to change it so that it behaves like GUI, i.e. it receives the current version of libtorrent and displays only those options that are available.

Chocobo1 · 2022-07-13T09:26:44Z

As for the UI, the GUI simply should not display invalid options.

It is missing a conditional at the enum definition which will obstruct the invalid value detection, and I'll handle it.

If it is contained in the configuration file or in the Web API request, then it should be treated as any other invalid value (replaced by the default value).

It seems it already do that.

The WebAPI can simply return the runtime error code in this case (for example, HTTP Conflict status).

It is cumbersome to do that in the current code, so IMO lets just let it silently fall back to default value for now.

Other comments should be already handled correctly.
This PR is near to finish, I plan to took over and handle it myself.

ghost added GUI GUI-related issues/changes Core labels May 8, 2022

ghost requested review from Chocobo1 and a team May 8, 2022 09:16

This comment was marked as resolved.

Sign in to view

glassez reviewed May 8, 2022

View reviewed changes

src/base/bittorrent/session.cpp Outdated Show resolved Hide resolved

src/base/bittorrent/session.cpp Outdated Show resolved Hide resolved

src/base/bittorrent/session.h Outdated Show resolved Hide resolved

src/base/bittorrent/session.h Outdated Show resolved Hide resolved

glassez requested changes May 8, 2022

View reviewed changes

src/base/bittorrent/session.cpp Outdated Show resolved Hide resolved

src/base/bittorrent/session.cpp Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

thalieht reviewed May 8, 2022

View reviewed changes

src/base/bittorrent/session.h Outdated Show resolved Hide resolved

Chocobo1 reviewed May 9, 2022

View reviewed changes

src/base/bittorrent/session.cpp Outdated Show resolved Hide resolved

src/base/bittorrent/session.cpp Outdated Show resolved Hide resolved

src/base/bittorrent/session.h Outdated Show resolved Hide resolved

src/gui/advancedsettings.cpp Outdated Show resolved Hide resolved

ghost added the WebUI WebUI-related issues/changes label May 10, 2022

glassez reviewed May 10, 2022

View reviewed changes

src/webui/api/appcontroller.cpp Outdated Show resolved Hide resolved

This comment was marked as off-topic.

Sign in to view

glassez reviewed May 12, 2022

View reviewed changes

src/base/bittorrent/session.cpp Outdated Show resolved Hide resolved

Chocobo1 reviewed May 15, 2022

View reviewed changes

src/base/bittorrent/session.cpp Outdated Show resolved Hide resolved

src/webui/api/appcontroller.cpp Outdated Show resolved Hide resolved

src/webui/api/appcontroller.cpp Outdated Show resolved Hide resolved

src/webui/www/private/views/preferences.html Outdated Show resolved Hide resolved

Chocobo1 added this to the 4.5.0 milestone May 15, 2022

glassez reviewed May 15, 2022

View reviewed changes

src/gui/advancedsettings.cpp Outdated Show resolved Hide resolved

This comment was marked as off-topic.

Sign in to view

ghost marked this pull request as draft July 5, 2022 13:01

ghost marked this pull request as ready for review July 7, 2022 05:57

ghost requested review from glassez and Chocobo1 July 7, 2022 06:05

Chocobo1 reviewed Jul 8, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

Chocobo1 reviewed Jul 8, 2022

View reviewed changes

src/gui/advancedsettings.cpp Outdated Show resolved Hide resolved

ghost changed the title ~~Add disk IO write mode "write through" to advanced settings~~ Add disk IO write mode write-through to advanced settings Jul 12, 2022

ghost requested a review from Chocobo1 July 13, 2022 03:50

Chocobo1 reviewed Jul 13, 2022

View reviewed changes

src/base/bittorrent/session.cpp Outdated Show resolved Hide resolved

Add disk IO write mode "write through" to advanced settings

5e8707d

Splits the OS cache settings into Disk IO read/write modes

ghost closed this Jul 13, 2022

ghost deleted the disk-io-write branch July 13, 2022 08:10

ghost removed this from the 4.5.0 milestone Jul 18, 2022

This pull request was closed.

Add disk IO write mode write-through to advanced settings #17005

Add disk IO write mode write-through to advanced settings #17005

Conversation

ghost commented May 8, 2022 • edited by ghost Loading

ghost commented May 8, 2022 • edited by ghost Loading

xavier2k6 commented May 8, 2022

This comment was marked as resolved.

xavier2k6 commented May 8, 2022

This comment was marked as resolved.

Chocobo1 left a comment • edited Loading

Choose a reason for hiding this comment

ghost commented May 10, 2022

glassez commented May 10, 2022

Chocobo1 commented May 11, 2022

This comment was marked as off-topic.

Rootax commented May 15, 2022

ghost commented May 15, 2022

Rootax commented May 15, 2022

The5kull commented May 15, 2022

Chocobo1 commented May 16, 2022 • edited Loading

Rootax commented May 16, 2022

glassez commented May 16, 2022

The5kull commented May 16, 2022

arvidn commented May 16, 2022

glassez commented Jun 11, 2022

ghost commented Jun 12, 2022

Chocobo1 commented Jun 14, 2022

ghost commented Jun 14, 2022

Rootax commented Jun 14, 2022

glassez commented Jun 14, 2022

glassez commented Jun 14, 2022

arvidn commented Jun 14, 2022

This comment was marked as off-topic.

Chocobo1 Jul 8, 2022

Choose a reason for hiding this comment

ghost Jul 8, 2022

Choose a reason for hiding this comment

ghost Jul 8, 2022

Choose a reason for hiding this comment

glassez Jul 8, 2022

Choose a reason for hiding this comment

Chocobo1 Jul 8, 2022 • edited Loading

Choose a reason for hiding this comment

glassez Jul 8, 2022

Choose a reason for hiding this comment

This comment was marked as outdated.

ghost commented Jul 13, 2022

Chocobo1 left a comment

Choose a reason for hiding this comment

Chocobo1 commented Jul 13, 2022

glassez commented Jul 13, 2022

Chocobo1 commented Jul 13, 2022 • edited Loading

ghost commented May 8, 2022 •

edited by ghost

Loading

ghost commented May 8, 2022 •

edited by ghost

Loading

Chocobo1 left a comment •

edited

Loading

Chocobo1 commented May 16, 2022 •

edited

Loading

Chocobo1 Jul 8, 2022 •

edited

Loading

Chocobo1 commented Jul 13, 2022 •

edited

Loading