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

Sequentially issue write requests, process results concurrently #482

Merged
merged 1 commit into from
Dec 15, 2021

Conversation

puellanivis
Copy link
Collaborator

@puellanivis puellanivis commented Dec 5, 2021

Supersedes #480

Originally, it was assumed that we could issue write requests in any arbitrary order, and while this holds and is correct, it can cause locality issues for servers. Issuing write requests sequentially means the server can efficiently handle the writes in order without needing to seek around the file so much.

So, this takes care to issue requests sequentially, and then handles the results concurrently as they come back.

Also, this changes writeAtWithConcurrency to issue sequential writes as well, not just ReadFromWithConcurrency

@drakkan
Copy link
Collaborator

drakkan commented Dec 5, 2021

Thanks @puellanivis ! Is the note about concurrent writes still valid after this PR? Do we still need to truncate the file after an error?

@puellanivis
Copy link
Collaborator Author

It’s definitely much less likely, but I think the standard defines that while things can potentially be handled by the server out-of-order, the responses returned have to be essentially identical to doing them in order. So, it’s probably unlikely that this concern would remain.

@drakkan
Copy link
Collaborator

drakkan commented Dec 9, 2021

I did a quick test and the patch seems ok for me, @ncw can you please provide your feedback? Thanks

@puellanivis
Copy link
Collaborator Author

puellanivis commented Dec 15, 2021

I’m going to go ahead and merge this. I would like to also address #468 before we cut any sort of release, though.

Edit: I had linked to the wrong issue.

@puellanivis puellanivis merged commit e0c1059 into master Dec 15, 2021
@puellanivis puellanivis deleted the patch/sequential-concurrent-write-requests branch December 15, 2021 11:22
ncw added a commit to rclone/rclone that referenced this pull request Jan 14, 2022
This stops the SFTP library issuing out of order writes which fixes
the problems uploading to `serve sftp` from the `sftp` backend.

This was fixes upstream in this pull request: pkg/sftp#482

Fixes #5806
ncw added a commit to rclone/rclone that referenced this pull request Mar 3, 2022
This stops the SFTP library issuing out of order writes which fixes
the problems uploading to `serve sftp` from the `sftp` backend.

This was fixes upstream in this pull request: pkg/sftp#482

Fixes #5806
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.

2 participants