Bug 2927

Summary: scp -l can't limit the bandwidth at the first several seconds
Product: Portable OpenSSH Reporter: Hao Lee <haolee.swjtu>
Component: scpAssignee: Assigned to nobody <unassigned-bugs>
Status: CLOSED FIXED    
Severity: major CC: djm, dtucker
Priority: P5    
Version: 7.7p1   
Hardware: amd64   
OS: Linux   
Bug Depends on:    
Bug Blocks: 2915    
Attachments:
Description Flags
Fix scp bwlimit calculation. djm: ok+

Description Hao Lee 2018-11-09 00:45:22 AEDT
When I use `-l` to limit the bandwidth to `10240Kbits`, the actual bandwidth at the first several seconds is `10MB/s`. After several seconds, the bandwidth will be limitted to `1MB/s` as expetced. I don't know if this is a bug.

[root@danban-1 ~]# scp -l 10240 CentOS-7-x86_64-Everything-1804.iso root@xxx.xxx.xxx.xxx:/

Authorized users only. All activities may be monitored and reported.
CentOS-7-x86_64-Everything-1804.iso                                                                                                                                     0%   10MB  10.0MB/s   14:53 ETA
Comment 1 Hao Lee 2018-11-11 00:33:55 AEDT
In the first 8 seconds, the actual bandwidth is 8 times of expected bandwidth limit.

If the bandwidth limit is 10240Kbit/s, the actual bandwidth is 10MB/s. After 8 seconds, the actual bandwidth limit will reduce to the expected limit which is 1.25MB/s.
Comment 2 Darren Tucker 2018-12-07 15:02:02 AEDT
Created attachment 3212 [details]
Fix scp bwlimit calculation.

Looking at the misc.c:bandwidth* functions I think there's two problems.

> bandwidth_limit(struct bwlimit *bw, size_t read_len)
> { [...]
>        if (!timerisset(&bw->bwstart)) {
>                monotime_tv(&bw->bwstart);
>                return;
>        }
>        bw->lamt += read_len;

The first (minor) problem: the first buffer written (16k, by default) is not accounted for at all because it returns before it gets to the code that accounts for it.

>        if (bw->lamt < bw->thresh)
>                return;

This is the main problem: bw->thresh is set to the bandwidth limit (in kbit/s), which means that for any non-trivial bandwidth limit it'll receive many writes before it even considers inserting a delay, so you get an initial burst followed by pause until the average drops back to the intended rate.

This patch moves accounting so the first block is accounted for, and sets the initial threshold such that it'll start computing the rate on the second and subsequent blocks.

Without patch:
$ ./scp -l 1000 /tmp/1MB  localhost:/dev/null
1MB                    0%    0     0.0KB/s   --:-- ETA
1MB                  100% 1024KB   1.0MB/s   00:01    
1MB                  100% 1024KB 511.8KB/s   00:02    
1MB                  100% 1024KB 341.2KB/s   00:03    
1MB                  100% 1024KB 255.9KB/s   00:04    
1MB                  100% 1024KB 204.7KB/s   00:05    
1MB                  100% 1024KB 170.6KB/s   00:06    
1MB                  100% 1024KB 146.2KB/s   00:07    
1MB                  100% 1024KB 127.9KB/s   00:08 

With patch:
$ ./scp -l 1000 /tmp/1MB  localhost:/dev/null
1MB                    0%    0     0.0KB/s   --:-- ETA
1MB                   12%  128KB 127.9KB/s   00:07 ETA
1MB                   25%  256KB 127.9KB/s   00:06 ETA
1MB                   37%  384KB 127.9KB/s   00:05 ETA
1MB                   50%  512KB 127.9KB/s   00:04 ETA
1MB                   60%  624KB 126.3KB/s   00:03 ETA
1MB                   73%  752KB 126.5KB/s   00:02 ETA
1MB                   85%  880KB 126.6KB/s   00:01 ETA
1MB                   98% 1008KB 126.8KB/s   00:00 ETA
1MB                  100% 1024KB 124.6KB/s   00:08
Comment 3 Darren Tucker 2018-12-07 16:13:28 AEDT
Thanks for the report, the patch has been applied and will be in the OpenSSH 8.0 release.
Comment 4 Hao Lee 2018-12-07 18:15:17 AEDT
(In reply to Darren Tucker from comment #3)
> Thanks for the report, the patch has been applied and will be in the
> OpenSSH 8.0 release.

Hi, I also submitted my patch to resolve this problem last month. However, I may misunderstand the algorithm for limiting the bandwidth, so I think my patch is wrong after reading you explanation. :)
Comment 6 Damien Miller 2019-05-03 14:42:37 AEST
Move resolved bugs -> CLOSED after 8.0 release