D29634: sftp: break large writes into multiple requests

[Harald Sitter]

This revision was automatically updated to reflect the committed changes.
Closed by commit R320:1df6174834bb: sftp: break large writes into multiple 
requests (authored by sitter).

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29634?vs=82723&id=82834

REVISION DETAIL
  https://phabricator.kde.org/D29634

AFFECTED FILES
  sftp/kio_sftp.cpp
  sftp/kio_sftp.h

To: sitter, ngraham, meven, feverfew
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Méven Car]

meven added a comment.


  In D29634#670485 , @feverfew wrote:
  
  > In D29634#670419 , @meven wrote:
  >
  > > In D29634#670377 , @ngraham 
wrote:
  > >
  > > > Nice work.
  > > >
  > > > In D29634#670159 , @feverfew 
wrote:
  > > >
  > > > > I imagine something similar should be done for FileJob::write?
  > > >
  > > >
  > > > Yeah.
  > >
  > >
  > > I guess you meant `FileProtocol::write`.
  > >  There is no need there, it uses `QIODevice::write` directly.
  >
  >
  > Sorry, I meant `kio_sftp`'s implementation of this: 
https://api.kde.org/frameworks/kio/html/classKIO_1_1FileJob.html#a481871536fb9471ccb64929792f31165
  
  
  I believe this is taken care of this patch in `SFTPInternal::write`

REPOSITORY
  R320 KIO Extras

BRANCH
  release/20.04

REVISION DETAIL
  https://phabricator.kde.org/D29634

To: sitter, ngraham, meven, feverfew
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Alexander Saoutkin]

feverfew added a comment.


  In D29634#670419 , @meven wrote:
  
  > In D29634#670377 , @ngraham 
wrote:
  >
  > > Nice work.
  > >
  > > In D29634#670159 , @feverfew 
wrote:
  > >
  > > > I imagine something similar should be done for FileJob::write?
  > >
  > >
  > > Yeah.
  >
  >
  > I guess you meant `FileProtocol::write`.
  >  There is no need there, it uses `QIODevice::write` directly.
  
  
  Sorry, I meant `kio_sftp`'s implementation of this: 
https://api.kde.org/frameworks/kio/html/classKIO_1_1FileJob.html#a481871536fb9471ccb64929792f31165

REPOSITORY
  R320 KIO Extras

BRANCH
  release/20.04

REVISION DETAIL
  https://phabricator.kde.org/D29634

To: sitter, ngraham, meven, feverfew
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Méven Car]

meven added a comment.


  In D29634#670377 , @ngraham wrote:
  
  > Nice work.
  >
  > In D29634#670159 , @feverfew 
wrote:
  >
  > > I imagine something similar should be done for FileJob::write?
  >
  >
  > Yeah.
  
  
  I guess you meant `FileProtocol::write`.
  There is no need there, it uses `QIODevice::write` directly.

REPOSITORY
  R320 KIO Extras

BRANCH
  release/20.04

REVISION DETAIL
  https://phabricator.kde.org/D29634

To: sitter, ngraham, meven, feverfew
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  Nice work.
  
  In D29634#670159 , @feverfew wrote:
  
  > I imagine something similar should be done for FileJob::write?
  
  
  Yeah.

REPOSITORY
  R320 KIO Extras

BRANCH
  release/20.04

REVISION DETAIL
  https://phabricator.kde.org/D29634

To: sitter, ngraham, meven, feverfew
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Alexander Saoutkin]

feverfew accepted this revision.
feverfew added a comment.


  I imagine something similar should be done for FileJob::write?

REPOSITORY
  R320 KIO Extras

BRANCH
  release/20.04

REVISION DETAIL
  https://phabricator.kde.org/D29634

To: sitter, ngraham, meven, feverfew
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Méven Car]

meven accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R320 KIO Extras

BRANCH
  release/20.04

REVISION DETAIL
  https://phabricator.kde.org/D29634

To: sitter, ngraham, meven
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Harald Sitter]

sitter edited the test plan for this revision.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D29634

To: sitter, ngraham, meven
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Harald Sitter]

sitter updated this revision to Diff 82723.
sitter added a comment.


  - rejigger write segmentation into new sftpWrite function used by both the 
put() and write()
  - fix length calculation
  - refine buf size comment

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29634?vs=82534&id=82723

BRANCH
  release/20.04

REVISION DETAIL
  https://phabricator.kde.org/D29634

AFFECTED FILES
  sftp/kio_sftp.cpp
  sftp/kio_sftp.h

To: sitter, ngraham, meven
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Méven Car]

meven added a comment.


  Well apart from the length of data needed to be reduced as we progress, all 
this makes sense.
  
  Btw gvfs uses 32768 as MAX_BUFFER_SIZE for sftp 
https://gitlab.gnome.org/GNOME/gvfs/-/blob/master/daemon/gvfsbackendsftp.c#L94

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D29634

To: sitter, ngraham, meven
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> meven wrote in kio_sftp.cpp:58
> Why not change it now to 32 * 1024 then ?
> I guess you tested this value works at least  with openssh.
> 
> I guess the best solution would be to ask/figure out the server buffer size.
> 
> What does gvfs, or other libs ?

The reason I am not changing it is because it is out of scope for the bug fix 
and requires research. I'm also kinda leaning towards keeping it until there's 
a bug report for a server that doesn't support it. The way the RFC is written 
reads incredibly open ended to the point where there may be no maximum or it 
may be even smaller than 32kb.

> feverfew wrote in kio_sftp.cpp:1831-1832
> I'm not sure if that would help. `MAX_XFER_BUF_SIZE` would be the upper bound 
> of this approach, and if the server buffer size is less I imagine we'll crash 
> anyway (as detailed in the bug report). If we simply write less then yes, we 
> can use this to "calibrate" but seeming as it crashes instead then 
> unfortunately I don't think this will work.

What Alex said.

I guess we could try to infer if a write failed because of buffer size but it 
seems a waste of time in the grand scheme of things (and also has lots of pits 
to fall into since we'd have to hook onto a callback and then carry out a 
mid-flight session reset).

In the long run we aren't loosing much by going with a static size and request 
queuing like we do for read() already. From what I have seen in the past the 
small requests of sftp become largely irrelevant with queuing and efficient 
(threaded) encryption.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D29634

To: sitter, ngraham, meven
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Alexander Saoutkin]

feverfew added inline comments.

INLINE COMMENTS

> meven wrote in kio_sftp.cpp:1831-1832
> Maybe we can deduce the server buffer size based on the `bytesWritten` value 
> : at init `serv_buffer_size =MAX_XFER_BUF_SIZE; ` and then ` if (length > 
> bytesWritten) { serv_buffer_size = bytesWritten }` and use `serv_buffer_size` 
> instead of MAX_XFER_BUF_SIZE in the loop.

I'm not sure if that would help. `MAX_XFER_BUF_SIZE` would be the upper bound 
of this approach, and if the server buffer size is less I imagine we'll crash 
anyway (as detailed in the bug report). If we simply write less then yes, we 
can use this to "calibrate" but seeming as it crashes instead then 
unfortunately I don't think this will work.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D29634

To: sitter, ngraham, meven
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> feverfew wrote in kio_sftp.cpp:1831-1832
> AFAICT the size of the buffer never changes so this will easily cause a 
> buffer overrun if I'm not mistaken?
> 
> Say for example you have a buffer with `buffer.size() == MAX_XFER_BUF_SIZE + 
> 1`. Then on the second iteration of the while loop (assuming `bytesWritten == 
> MAX_XFER_BUF_SIZE`) you'll do a `sftp_write()` pointing to a `char` buffer of 
> size 1, but which incorrectly states that the size is `MAX_XFER_BUF_SIZE`.

Maybe we can deduce the server buffer size based on the `bytesWritten` value : 
at init `serv_buffer_size =MAX_XFER_BUF_SIZE; ` and then ` if (length > 
bytesWritten) { serv_buffer_size = bytesWritten }` and use `serv_buffer_size` 
instead of MAX_XFER_BUF_SIZE in the loop.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D29634

To: sitter, ngraham, meven
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Méven Car]

meven requested changes to this revision.
meven added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kio_sftp.cpp:58
>  // you will overflow the 2 byte size variable in a sftp packet.
> +// FIXME: this seems too large
> +// from the RFC:
> +//   The maximum size of a packet is in practise determined by the client
> +//   (the maximum size of read or write requests that it sends, plus a few
> +//   bytes of packet overhead).  All servers SHOULD support packets of at
> +//   least 34000 bytes (where the packet size refers to the full length,
> +//   including the header above).  This should allow for reads and writes of
> +//   at most 32768 bytes.
> +// In practise that means we can assume that the server supports 32kb,
> +// it may be more or it could be less. Since there's not really a system in 
> place to
> +// figure out the maximum (and at least openssh arbitrarily resets the entire
> +// session if it finds a packet that is too large
> +// [https://bugs.kde.org/show_bug.cgi?id=404890]) we ought to be more 
> conservative!
>  #define MAX_XFER_BUF_SIZE (60 * 1024)
>  

Why not change it now to 32 * 1024 then ?
I guess you tested this value works at least  with openssh.

I guess the best solution would be to ask/figure out the server buffer size.

What does gvfs, or other libs ?

> kio_sftp.cpp:1831
> +while (offset < buffer.size()) {
> +const auto length = qMin(MAX_XFER_BUF_SIZE, 
> buffer.size());
> +ssize_t bytesWritten = sftp_write(file, buffer.data() + 
> offset, length);

`const auto length = qMin(MAX_XFER_BUF_SIZE, buffer.size() - offset);`

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D29634

To: sitter, ngraham, meven
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Alexander Saoutkin]

feverfew added a comment.


  Seems like something similar should also occur in `FileJob::write`?

INLINE COMMENTS

> kio_sftp.cpp:1831-1832
> +while (offset < buffer.size()) {
> +const auto length = qMin(MAX_XFER_BUF_SIZE, 
> buffer.size());
> +ssize_t bytesWritten = sftp_write(file, buffer.data() + 
> offset, length);
> +if (bytesWritten < 0) {

AFAICT the size of the buffer never changes so this will easily cause a buffer 
overrun if I'm not mistaken?

Say for example you have a buffer with `buffer.size() == MAX_XFER_BUF_SIZE + 
1`. Then on the second iteration of the while loop (assuming `bytesWritten == 
MAX_XFER_BUF_SIZE`) you'll do a `sftp_write()` pointing to a `char` buffer of 
size 1, but which incorrectly states that the size is `MAX_XFER_BUF_SIZE`.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D29634

To: sitter, ngraham
Cc: feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Harald Sitter]

sitter created this revision.
sitter added a reviewer: ngraham.
Herald added projects: Dolphin, Frameworks.
Herald added subscribers: kfm-devel, kde-frameworks-devel.
sitter requested review of this revision.

REVISION SUMMARY
  servers have arbitrary limits that we should stay below. to ensure this
  happens use MAX_XFER_BUF_SIZE as maximum size per request. if we read
  data large than that, break it apart into multiple requests
  
  (I am mostly guessing here, the rfc doesn't seem to impose any size
   constraint on the write requests themselves, so probably packet
   constraints apply by default)
  
  BUG: 404890
  FIXED-IN: 20.04.2

TEST PLAN
  - fallocate -l 128M file
  - sftp to localhost
  - copy file from one dir to another

REPOSITORY
  R320 KIO Extras

BRANCH
  release/20.04

REVISION DETAIL
  https://phabricator.kde.org/D29634

AFFECTED FILES
  sftp/kio_sftp.cpp

To: sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, pberestov, 
iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov