Skip to content

util: remove WSL 1 workaround in fs #25898

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 16, 2023
Merged

util: remove WSL 1 workaround in fs #25898

merged 1 commit into from
Feb 16, 2023

Conversation

philmb3487
Copy link
Contributor

@philmb3487 philmb3487 commented Aug 22, 2022 •
edited by fanquake
Loading

Following discussion, the WSL1 patch will be removed, as WSL1 is no longer being developed by Microsoft. Instead, please upgrade to a mainstream WSL2 version. More information can be found on the official website.

@philmb3487 philmb3487 changed the title clarify that the patch targets WSL1. clarify that fs IsWSL patch targets WSL1. Aug 22, 2022
@NorrinRadd
Copy link

LGTM

@hebasto
Copy link
Member

hebasto commented Aug 22, 2022

Are there any limitations why a Windows user cannot upgrade their WSL1 to WSL2?

@philmb3487
Copy link
Contributor Author

philmb3487 commented Aug 22, 2022 •
edited
Loading

Are there any limitations why a Windows user cannot upgrade their WSL1 to WSL2?

hi @hebasto. As far as I know WSL1 is no longer being developed. Documentation indicates that WSL1 is slightly advantageous in some cases where access to the native filesystem is required, and WSL2 might interact with other virtualization solutions.
I'm not particularly attached to this hack, however it is safe to assume that it will never be fixed on MS's side.

@hebasto
Copy link
Member

hebasto commented Aug 22, 2022

Why not just drop support for WSL1 then?

@ghost
Copy link

ghost commented Aug 23, 2022

I think this is reasonable to specify, as I myself spent a few minutes looking why the call IsWSL() was failing on my WSL environment.

@sinetek how can I test this?

Why not just drop support for WSL1 then?

@hebasto

Everyone would not have upgraded to WSL2 yet and there are some trade-offs according to the docs: https://docs.microsoft.com/en-us/windows/wsl/compare-versions#exceptions-for-using-wsl-1-rather-than-wsl-2

@philmb3487
Copy link
Contributor Author

I think this is reasonable to specify, as I myself spent a few minutes looking why the call IsWSL() was failing on my WSL environment.

@sinetek how can I test this?

Run the app under GDB and set a breakpoint on this function.
Alternatively you can see the result of utsname.version with the command uname -v.
The result on my box is #1 SMP Wed Mar 2 00:30:59 UTC 2022.

Why not just drop support for WSL1 then?

@hebasto

Everyone would not have upgraded to WSL2 yet and there are some trade-offs according to the docs: https://docs.microsoft.com/en-us/windows/wsl/compare-versions#exceptions-for-using-wsl-1-rather-than-wsl-2

I tend to agree. On the other hand I can understand why the removal is warranted, there might be some other unseen bugs in WSL1 and building a complete syscall emulation of Linux is beyond the scope of this project.

@fanquake
Copy link
Member

I agree that we could probably just remove support for WSL1 here.

@philmb3487
Copy link
Contributor Author

updated.

ghost
ghost approved these changes Aug 26, 2022
View reviewed changes
Copy link

@ghost ghost left a comment •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 7566cd6

nit: can fix lint error (whitespace)

@fanquake
Copy link
Member

nit: can fix lint error (whitespace)

That isn't a nit. It needs fixing before merge.

The PR title and description need to be updated to reflect the new changes.

@philmb3487 philmb3487 changed the title clarify that fs IsWSL patch targets WSL1. Remove old fs IsWSL patch that targets WSL1. (Please use WSL2) Aug 26, 2022
@philmb3487
Copy link
Contributor Author

nit: can fix lint error (whitespace)

That isn't a nit. It needs fixing before merge.

The PR title and description need to be updated to reflect the new changes.

That's a cute way to enforce spaces. Updated.

@luke-jr
Copy link
Member

luke-jr commented Aug 26, 2022

Considering IBD bottleneck is often I/O, and Microsoft's documentation says WSL1 is better for that, I think someone should do some benchmarks before we drop WSL1 support.

ghost
ghost approved these changes Aug 26, 2022
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reACK 2e80345

@ghost
Copy link

ghost commented Aug 27, 2022

Considering IBD bottleneck is often I/O, and Microsoft's documentation says WSL1 is better for that, I think someone should do some benchmarks before we drop WSL1 support.

WSL2 is better if files (data directory) is on WSL linux and WSL1 is better if files (data directory) is on Windows host.

I tried syncing bitcoind (signet) for 5 minutes with default dbcache and txindex enabled. WSL2 was better and synced 20% compared to 14% in WSL1.

Memory usage is a concern in WSL2 however there is a workaround suggested in this issue: microsoft/WSL#4166 (comment)

@maflcko
Copy link
Member

maflcko commented Sep 9, 2022

Please revert to the previous commit, we usually do not allow merge commits in the commit history unless there is a reason to use them.

@philmb3487
Copy link
Contributor Author

Please revert to the previous commit, we usually do not allow merge commits in the commit history unless there is a reason to use them.

Thanks for alerting me. That was not intentional and fixed.

ghost
ghost approved these changes Sep 9, 2022
View reviewed changes
Copy link

@ghost ghost left a comment •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 5669afb

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 23, 2022 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK 1440000bytes, fanquake

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

@hebasto
Copy link
Member

hebasto commented Nov 30, 2022

Is reverting of the original e8fa0a3 commit cleaner?

Maybe it's worth updating doc/build-windows.md:

bitcoin/doc/build-windows.md

Line 9 in e2bfd41

* On Windows, using [Windows Subsystem for Linux (WSL)](https://docs.microsoft.com/windows/wsl/about) and Mingw-w64.
as well, I mean, s/WSL/WSL 2/?

fanquake
fanquake approved these changes Feb 6, 2023
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 5669afb - seems ok as-is.

@fanquake fanquake changed the title Remove old fs IsWSL patch that targets WSL1. (Please use WSL2) fs: remove WSL 1 workaround Feb 6, 2023
@maflcko maflcko changed the title fs: remove WSL 1 workaround util: remove WSL 1 workaround in fs Feb 6, 2023
@fanquake fanquake merged commit 3995c88 into bitcoin:master Feb 16, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 16, 2023
@fanquake
Merge bitcoin#25898: util: remove WSL 1 workaround in fs
14ce44f 
5669afb fs: drop old WSL1 hack. (sinetek)

Pull request description:

  Following discussion, the WSL1 patch will be removed, as WSL1 is no longer being developed by Microsoft. Instead, please upgrade to a mainstream WSL2 version. More information can be found on [the official website](https://docs.microsoft.com/en-us/windows/wsl/).

ACKs for top commit:
  1440000bytes:
    ACK bitcoin@5669afb
  fanquake:
    ACK 5669afb - seems ok as-is.

Tree-SHA512: 256c13985f6dd3453caf39c7ef1c951dbdfa8457a18cd05e4624db36d8ed8a4f809bb78a7b3c82c72997e9ed3823d5566a5c2d0812d2501aba2e54bc5e6eec79
@bitcoin bitcoin locked and limited conversation to collaborators Feb 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fanquake fanquake fanquake approved these changes

+1 more reviewer
Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@philmb3487 @NorrinRadd @hebasto @fanquake @luke-jr @maflcko @DrahtBot