search

pkg / sftp

SFTP support for the go.crypto/ssh package
BSD 2-Clause "Simplified" License
1.5k stars 380 forks source link

Stop ReadFromWithConcurrency submitting out of order writes #480

Closed ncw closed 2 years ago

ncw commented 2 years ago

This is for discussion. This is causing a real problem in rclone https://github.com/rclone/rclone/issues/5806 but there may be a better way of fixing.

Before this change, ReadFromWithConcurrency would very often submit writes to the server out of order due to small timing differences when lots of blocks were queued and ready to send.

This causes more work for the server as seeking about the file isn't free. Writing chunks of a file out of order can also cause fragmentation and consequently loss of performance on that file.

This patch causes the write to the channel to be handled synchronously but the acks are received asynchronously allowing for many write packets to be in flight as before.

ncw commented 2 years ago

I speed tested this over a long wide pipe and I couldn't see any difference in speed before and after the patch, both about 40 MiB/s falling all the way back to 900KiB/s with concurrent writes disabled as you'd expect.

ncw commented 2 years ago

Thinking about this some more

drakkan commented 2 years ago

Hi,

thanks for the contribution, it seems fine to me, however this code was written by @puellanivis, at the moment she has some personal issues, please wait some time for her to have a chance to have a look to the code, thanks for your patience

puellanivis commented 2 years ago

Indeed, sequential ordering of requests is probably a good idea, and I think I was writing it into the client updates.

There are some things I think that should be addressed, but as @drakkan mentioned, I'm currently out. (In hospital) and haven't been able to really do anything yet. This also happened as I was moving into a new apartment, and now missed the appointment for connecting my internet. 😫

ncw commented 2 years ago

Thanks for taking a look and happy to make changes when you are up to it. No hurry though and look after yourself.

puellanivis commented 2 years ago

Oh, I did want to mention, I did similar work already in PR 443, it might help to review that PR, and compare to what you're doing here. That'll end up being one of the first things I do myself.

ncw commented 2 years ago

Any thoughts on this PR?

puellanivis commented 2 years ago

Functionality was merged in with #482