Skip to content
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.

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

If tmp2 and final files are in same filesystem, rename() not copy() #314

Merged
merged 5 commits into from Jan 29, 2022
Merged

Conversation

cross
Copy link
Contributor

@cross cross commented Oct 20, 2021

This avoids potentially painful copies of large files within the same filesystem.
To note:
a) I'm a UNIX person, so I'm not sure if this will compile on Windows as coded. Directions welcome.
b) I didn't write a test for this, it appears all of the existing test only use files in the same directory. Is the copy codepath in CreatePlotDisk() tested somewhere?

@cross
Copy link
Contributor Author

cross commented Oct 21, 2021

This fixes #313 , waiting on workflows to test windows applicability.

@cross
Copy link
Contributor Author

cross commented Oct 22, 2021

Live-testing this change yields the following result (successful):

Starting plotting progress into temporary dirs: /diskc/tmp and /diska/tmp
ID: IDSTR
Plot size is: 33
[...]
Final Directory is: /diska/chia/plots/k33
[...][...]
Final File size: 208.858 GiB
Total time = 63537.734 seconds. CPU (152.580%) Thu Oct 21 18:52:28 2021
Renamed final file from "/diska/tmp/plot-k33-2021-10-21-01-13-IDSTR.plot.2.tmp" to "/diska/chia/plots/k33/plot-k33-2021-10-21-01-13-IDSTR.plot"

@cross
Copy link
Contributor Author

cross commented Oct 25, 2021

Anything else needed here? Who can provide feedback or approval?

@cross
Copy link
Contributor Author

cross commented Oct 29, 2021

Requesting review - @hoffmang9 @wjblanke

Copy link
Contributor

@rostislav rostislav left a comment

Choose a reason for hiding this comment

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

Looks good to me. I've also verified that this works on Windows.

@cross
Copy link
Contributor Author

cross commented Nov 3, 2021

Can someone merge this? Is anything else needed?

@hoffmang9
Copy link
Member

This is likely to take a bit broader testing and that's the only hesitation. Right this moment there are other things at a higher priority. Should it test well on BSD/MacOS both flavors, three FSs etc.

@cross
Copy link
Contributor Author

cross commented Nov 3, 2021

This is likely to take a bit broader testing and that's the only hesitation. Right this moment there are other things at a higher priority. Should it test well on BSD/MacOS both flavors, three FSs etc.

Okay. It's very old libc calls, so I'm sure all UNIX/Linux variants and Mac OS (>9) will work the same, but I do not criticize at all your wanting to test it fully. Let me know if I can help.

@cross
Copy link
Contributor Author

cross commented Nov 24, 2021

If I could get this in before the next chia-blockchain release, so its in there, that would be ideal. Is there anything I can do to help testing this? Just checking since it's been a few weeks here, happy to help if I can.

@cross
Copy link
Contributor Author

cross commented Dec 3, 2021

Ping. Sorry to be a noodge, but just wanted to make sure there's a plan to get the required testing done and get this merged. Again, let me know if I can help.

@cross
Copy link
Contributor Author

cross commented Dec 20, 2021

@rostislav @hoffmang9 Any status on this PR?

@wjblanke wjblanke merged commit 0502f34 into Chia-Network:main Jan 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants