smb faster copy to local
ClosedPublic

Authored by sitter on Feb 19 2020, 2:41 PM.

Details

Summary

see https://bugs.kde.org/show_bug.cgi?id=291835#c57 for background

  • reading now happens inside a future. should be safe since we don't have any other threads doing anything while we wait.
  • the future feeds into a buffer from which the main thread will take file segments and write them to disk
  • buffer has 4 segments and synchronizes the threads via wait conditions
  • the size of a segment is determined somewhat dynamically between 64kb and 4mb. the larger a file is the more it benefits from larger read requests

under perfect conditions this yields approximately mount-level copy
performance, unfortunately those are hard to hit so on average it's usually
less (somewhere in the range of 10 to 20% depending on the actual file
size and server type).

for many tiny files performance is about where it was before. the larger
the files get the greater the gains from this diff though.

specifically here's some samples I've taken:

  • for downloads from windows10
    • 1G & 4G file
      • compared to 20.04 is ~77% faster
      • within 10% of windows
    • 8G file
      • compared to 20.04 is ~79% faster
      • within 5% of windows
  • uploads to windows10
    • all sizes
      • compared to 20.04 is ~50% faster
      • now comparable performance to windows
  • for remote-to-remote file copies from windows10 to smbd 4.11.6
    • 1000 x 5K files
      • no change, dreadfully slow, likely problem in KIO internals
    • 1G file
      • compared to 20.04 is ~45% faster
      • within 8% of windows
    • 4G file
      • compared to 20.04 is ~95% faster
      • and somehow 18% faster than windows (could be a fluke)

I've done transfers for 128M, 256M, 512M, 1G, 4G and partially 8G. Differences not mentioned are either unchanged or negligible.

Test Plan
  • fallocate -l 1G file
  • copy around
  • copy kio-extras around

Diff Detail

Repository
R320 KIO Extras
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sitter created this revision.Feb 19 2020, 2:41 PM
Restricted Application added projects: Dolphin, Frameworks. · View Herald TranscriptFeb 19 2020, 2:41 PM
Restricted Application added subscribers: kfm-devel, kde-frameworks-devel. · View Herald Transcript
sitter requested review of this revision.Feb 19 2020, 2:41 PM
sitter added inline comments.Feb 19 2020, 2:57 PM
smb/kio_smb_dir.cpp
85

This may actually be too optimistic. If you happen to use all 20 allowed instances of the slave (5 per server) and each copies a file that hits the segment limit, then we'd be looking at up to 400MiB. A bit steep. Then again, I am not sure how likely that is.

Yep I'm seeing pretty much double speed here. Amazing.

This code is above my pay grade, so I won't try to review it. :) But the patch works as described!

anthonyfieroni added inline comments.
smb/kio_smb_dir.cpp
46

Segment does not free its memory. Why not use QByteArray or similar with automatic storage allocation?

hallas added a subscriber: hallas.Feb 20 2020, 6:10 AM
hallas added inline comments.
smb/kio_smb_dir.cpp
46

Yes, you could (and should ;) ) use standard C++ for this, i.e.:

std::unique_ptr<char[]> buf;
buf = std::make_unique<char{}>(segmentSize);
165

Use standard C++ for this, i.e.:

std::array<std::unique_ptr<Segment>, m_capacity> m_buffer;

This makes m_buffer iterable and it also ensures that the segments are always correctly freed.

340

What is the reason to use QFuture over std::future?

sitter added inline comments.Feb 24 2020, 3:44 PM
smb/kio_smb_dir.cpp
46

we use c++11 so can't use make_unique and QBA seems excessive given the use. I am rather tempted to go for QVarLengthArray though, after all, technically the buf has a minimal size, so we could always put that on the stack and only alloc larger sizes on the heap. Thoughts?

340

No particular reason, haven't used std::future before is all.

sitter updated this revision to Diff 76865.Mar 3 2020, 4:13 PM
  • refine code a bit
  • extend to ::get works same as smbCopyGet. in fact I feel like the two functions should be merged. at their hearts they both could read into QIODevices (QFile/QBuffer(QBA))
  • use segment class for all transfer use to ultimately replace the xfer_buf define with smart calculation of transfer size
  • ::smbCopyPut didn't get any measurable improvement from threading, so I am leaving it out for now. having written that the lack of difference may have been because HDD IO of the server was saturated so perhaps this deserves another look
sitter updated this revision to Diff 77343.Mar 10 2020, 11:51 AM
  • rebase
  • move transfer impls to cpp
  • new unpop function to make the ring usage more consistent and make sure neither thread can overtake the other
  • new test for transfer tech. notably simulating imbalance in processing speed of the ring threads
  • segment now constructs from the file size instead of the segment size. this centralizes segment size code in the segment
  • make all transfers use a segment constructed with the relevant file size to get "smart" size calculation everywhere (ring buffer continues only for copyget and get)

remote-to-remote copy is blocked on smb_context not being thread safe nor reentrant. unlikely to change any time soon. it's also blocking file change notification support for both kio and gvfs :(

sitter retitled this revision from WIP: RFC: smb faster copy to local to smb faster copy to local.Mar 10 2020, 11:51 AM
sitter edited the summary of this revision. (Show Details)

Still +1 functionally.

meven added a subscriber: meven.Mar 20 2020, 10:06 AM

Looks good to me.

Any opinions on landing this for 20.04 still? It is technically a bugfix. It is also practically a whole lot of risky code, making me rather uneasy about putting it in past beta.

Yeah due to the quantity of new code, it feels more like 20.08 material, TBH.

mmustac added a subscriber: mmustac.Apr 1 2020, 7:53 PM
This comment was removed by mmustac.

@sitter Well, master is now 20.08. Shall we land this if nobody has any objections?

sitter edited the summary of this revision. (Show Details)Apr 3 2020, 4:59 PM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 3 2020, 5:03 PM
Closed by commit R320:46b5fb425c14: smb: fast copy (authored by sitter). · Explain Why
This revision was automatically updated to reflect the committed changes.