Repository: rclone/rclone
Pull request #7043 (2023-06-05 11:17:29)

fs: allow setting a write buffer for multithread
#### What is the purpose of this change?

This change adds an option to buffer multithread download parts before writing to disk.

In our testing, we saw that multithreaded downloads from S3 to CIFS (as a mounted filesystem) were much slower with rclone than with the official aws cli. Interestingly, single-threaded downloads were faster than multi-threaded, albeit still far away from what our bandwidth should offer. Downloads to local disk were fine.

The end result looked like this for us:

rclone standard: ~50 MiB/s
rclone with --multi-thread-buffer-size=1M: 400MiB/s+ 

this is close to our max theoretical speed based on bandwidth (5Gib/s)

I did consider just using the already existing buffer-size option, as it does seem to to exactly the same, only for multiple transfers. However I wasn't 100% sure, and with the extra option there should be *no changes* in behaviour unless the user uses the flag, which would not be the case with the buffer-size as it does have a non-zero default AFAIK. Let me know if I should change.

#### Was the change discussed in an issue or in the forum before?

no

#### Checklist

- [x] I have read the [contribution guidelines](https://github.com/rclone/rclone/blob/master/CONTRIBUTING.md#submitting-a-new-feature-or-bug-fix).
- [x] I have added tests for all changes in this PR if appropriate.
- [x] I have added documentation for the changes if appropriate.
- [x] All commit messages are in [house style](https://github.com/rclone/rclone/blob/master/CONTRIBUTING.md#commit-messages).
- [x] I'm done, this Pull Request is ready for review :-)


jorjao81: I just noticed the 1.19 and 1.18 builds failed because of my usage of the OffsetWriter :(

How big of an issue is that? The OffsetWriter was quite nice, as it also allowed me to use the BufferedWriter, so I don't need to do byte arithmetic myself. But if making it go 18 compatible is required for getting the code merged, I can look at it.
ncw: Your new code looks good.

How did you get on with perf testing with different buffer sizes? Want me to help here?
jorjao81: Additional test, same configs as the previous:

| Command | Mean [s] | Min [s] | Max [s] | Relative |
|:---|---:|---:|---:|---:|
| `rclone-smb-4-threads-16K` | 44.116 ± 16.421 | 31.497 | 62.682 | 1.38 ± 0.56 |
| `rclone-smb-4-threads-1M` | 37.972 ± 1.404 | 36.351 | 38.808 | 1.19 ± 0.19 |
| `rclone-smb-4-threads-10M` | 34.916 ± 4.469 | 30.027 | 38.792 | 1.09 ± 0.22 |
| `rclone-smb-4-threads-50M` | 38.654 ± 0.909 | 37.873 | 39.651 | 1.21 ± 0.19 |
| `rclone-smb-8-threads-16K` | 37.537 ± 2.010 | 35.223 | 38.850 | 1.18 ± 0.19 |
| `rclone-smb-8-threads-1M` | 37.757 ± 0.177 | 37.606 | 37.952 | 1.18 ± 0.18 |
| `rclone-smb-8-threads-10M` | 37.737 ± 0.953 | 37.074 | 38.829 | 1.18 ± 0.18 |
| `rclone-smb-8-threads-50M` | 39.745 ± 0.599 | 39.079 | 40.238 | 1.25 ± 0.19 |
| `rclone-smb-20-threads-16K` | 31.898 ± 4.844 | 28.963 | 37.490 | 1.00 |
| `rclone-smb-20-threads-1M` | 33.098 ± 4.187 | 28.287 | 35.913 | 1.04 ± 0.21 |
| `rclone-smb-20-threads-10M` | 37.702 ± 0.098 | 37.592 | 37.780 | 1.18 ± 0.18 |
| `rclone-smb-20-threads-50M` | 37.100 ± 1.278 | 35.960 | 38.481 | 1.16 ± 0.18 |
| `rclone-smb-50-threads-16K` | 33.467 ± 4.258 | 28.558 | 36.151 | 1.05 ± 0.21 |
| `rclone-smb-50-threads-1M` | 33.114 ± 4.513 | 29.340 | 38.113 | 1.04 ± 0.21 |
| `rclone-smb-50-threads-10M` | 34.151 ± 4.283 | 29.217 | 36.915 | 1.07 ± 0.21 |
| `rclone-smb-50-threads-50M` | 37.928 ± 2.900 | 34.665 | 40.210 | 1.19 ± 0.20 |
jorjao81: ### Server 
AWS m6in.2xlarge with Windows
gp3 EBS volume
5000 IOPS
500 throughput
(same as before)

### Client (Windoes)
m6in.xlarge

### Test command
    hyperfine -r 3 --show-output -n "rclone-smb-50-thread-{buffer}" --parameter-list buffer 4k,16K,32k,64k,128k,256K,512k,1M,2M,5M,10M 'rclone -vv copyto s3:MYBUCKET/test-10-GiB test --multi-thread-streams 8 --progress --multi-thread-write-buffer-size={buffer} && del /F /Q test ' --export-markdown results.md

### Results

| Command | Mean [s] | Min [s] | Max [s] | Relative |
|:---|---:|---:|---:|---:|
| `rclone-smb-50-thread-4k` | 159.705 ± 4.533 | 154.474 | 162.474 | 6.45 ± 0.44 |
| `rclone-smb-50-thread-16K` | 100.276 ± 0.459 | 99.828 | 100.745 | 4.05 ± 0.25 |
| `rclone-smb-50-thread-32k` | 64.814 ± 2.513 | 62.850 | 67.645 | 2.62 ± 0.19 |
| `rclone-smb-50-thread-64k` | 40.437 ± 1.068 | 39.206 | 41.114 | 1.63 ± 0.11 |
| `rclone-smb-50-thread-128k` | 28.261 ± 1.093 | 27.375 | 29.482 | 1.14 ± 0.08 |
| `rclone-smb-50-thread-256K` | 26.881 ± 4.588 | 21.833 | 30.795 | 1.08 ± 0.20 |
| `rclone-smb-50-thread-512k` | 26.736 ± 1.400 | 25.856 | 28.350 | 1.08 ± 0.09 |
| `rclone-smb-50-thread-1M` | 25.996 ± 1.367 | 24.426 | 26.925 | 1.05 ± 0.09 |
| `rclone-smb-50-thread-2M` | 25.000 ± 1.454 | 23.976 | 26.664 | 1.01 ± 0.09 |
| `rclone-smb-50-thread-5M` | 26.120 ± 1.493 | 24.408 | 27.158 | 1.05 ± 0.09 |
| `rclone-smb-50-thread-10M` | 24.780 ± 1.539 | 23.807 | 26.554 | 1.00 |


jorjao81: Hi, see the windows tests above, they also seem to benefit quite a bit from a bigger buffer. That said, I think that the most common case by far will be writing to SSDs, am I right?

That being the case, I would go for 16k as the default, and explain that for HDDs and mounted remote filesystems it can pay to test higher numbers.
However, I sorta did change my mind and would to do it per thread though, as I think that will usually be the relevant factor. Do you agree? And in that case, it cannot really use --buffer-size but need our own flag...


ncw: > Hi, see the windows tests above, they also seem to benefit quite a bit from a bigger buffer.

Yes they do. 128k would seem to be a better default for those.

> That said, I think that the most common case by far will be writing to SSDs, am I right?

Most likely.

> That being the case, I would go for 16k as the default

I think that the feature makes very little difference on Linux - the changes are lost in the noise I think so 128k would be fine there too .

As far as memory use - no-one is going to notice an extra 4*128k = 0.5MB per multi-thread copy.

>  and explain that for HDDs and mounted remote filesystems it can pay to test higher numbers. However, I sorta did change my mind and would to do it per thread though, as I think that will usually be the relevant factor. Do you agree? And in that case, it cannot really use `--buffer-size` but need our own flag...

Yes I think the flag as you have it now is probably exactly what we need.


Here are the results from my Windows VM running on SSD which qualitatively speaking aren't much different from yours though my machine has lots of variance in the runs (its a busy dev machine!).

| Command | Mean [s] | Min [s] | Max [s] | Relative |
|:---|---:|---:|---:|---:|
| `rclone-win-ssd-4-threads-0` | 3.311 ± 0.408 | 2.862 | 3.894 | 1.59 ± 0.20 |
| `rclone-win-ssd-4-threads-16k` | 3.687 ± 0.794 | 2.777 | 4.744 | 1.77 ± 0.38 |
| `rclone-win-ssd-4-threads-32k` | 2.960 ± 0.207 | 2.830 | 3.328 | 1.42 ± 0.10 |
| `rclone-win-ssd-4-threads-64k` | 2.728 ± 0.464 | 2.441 | 3.540 | 1.31 ± 0.22 |
| `rclone-win-ssd-4-threads-128k` | 2.352 ± 0.049 | 2.302 | 2.418 | 1.13 ± 0.03 |
| `rclone-win-ssd-4-threads-256k` | 2.203 ± 0.074 | 2.133 | 2.314 | 1.06 ± 0.04 |
| `rclone-win-ssd-4-threads-512k` | 2.149 ± 0.080 | 2.074 | 2.262 | 1.03 ± 0.04 |
| `rclone-win-ssd-4-threads-1M` | 2.130 ± 0.044 | 2.077 | 2.184 | 1.02 ± 0.03 |
| `rclone-win-ssd-4-threads-2M` | 2.083 ± 0.034 | 2.047 | 2.126 | 1.00 |
| `rclone-win-ssd-4-threads-4M` | 2.116 ± 0.063 | 2.051 | 2.196 | 1.02 ± 0.03 |
| `rclone-win-ssd-4-threads-8M` | 2.565 ± 0.489 | 2.179 | 3.318 | 1.23 ± 0.24 |

So in conclusion, I think this feature is great and the flag you've made already is what we should use.

I'd like to set the default to something > 0. 16k maybe or 128k maybe - I'd be happy with either or something in between - I leave the choice to you :-)

I think that plus a little extra in the help about HDD and/or network file systems and this will be ready to merge :-)
jorjao81: I went with 128k, as the benefits for Windows seem significant and I don't think there is any real drawback for other cases as you mentioned.

Also update the docs, hope it's clear enough.

Let me know if there is anything else I should do.

Thanks!

Patch:
diff --git a/backend/azureblob/azureblob.go b/backend/azureblob/azureblob.go
index ec9756bb7..404b143c0 100644
--- a/backend/azureblob/azureblob.go
+++ b/backend/azureblob/azureblob.go
@@ -1063,11 +1063,7 @@ func (f *Fs) list(ctx context.Context, containerName, directory, prefix string,
 				fs.Debugf(f, "Odd name received %q", remote)
 				continue
 			}
-			remote = remote[len(prefix):]
 			isDirectory := isDirectoryMarker(*file.Properties.ContentLength, file.Metadata, remote)
-			if addContainer {
-				remote = path.Join(containerName, remote)
-			}
 			if isDirectory {
 				// Don't insert the root directory
 				if remote == directory {
@@ -1076,6 +1072,10 @@ func (f *Fs) list(ctx context.Context, containerName, directory, prefix string,
 				// process directory markers as directories
 				remote = strings.TrimRight(remote, "/")
 			}
+			remote = remote[len(prefix):]
+			if addContainer {
+				remote = path.Join(containerName, remote)
+			}
 			// Send object
 			err = fn(remote, file, isDirectory)
 			if err != nil {
diff --git a/backend/combine/combine.go b/backend/combine/combine.go
index f83a3c486..d0e747376 100644
--- a/backend/combine/combine.go
+++ b/backend/combine/combine.go
@@ -233,6 +233,7 @@ func NewFs(ctx context.Context, name, root string, m configmap.Mapper) (outFs fs
 		ReadMetadata:            true,
 		WriteMetadata:           true,
 		UserMetadata:            true,
+		PartialUploads:          true,
 	}).Fill(ctx, f)
 	canMove := true
 	for _, u := range f.upstreams {
diff --git a/backend/compress/compress.go b/backend/compress/compress.go
index 21ab99c03..6d99604b8 100644
--- a/backend/compress/compress.go
+++ b/backend/compress/compress.go
@@ -186,6 +186,7 @@ func NewFs(ctx context.Context, name, rpath string, m configmap.Mapper) (fs.Fs,
 		ReadMetadata:            true,
 		WriteMetadata:           true,
 		UserMetadata:            true,
+		PartialUploads:          true,
 	}).Fill(ctx, f).Mask(ctx, wrappedFs).WrapsFs(f, wrappedFs)
 	// We support reading MIME types no matter the wrapped fs
 	f.features.ReadMimeType = true
diff --git a/backend/crypt/crypt.go b/backend/crypt/crypt.go
index 0fec3f013..5b8ff2c8a 100644
--- a/backend/crypt/crypt.go
+++ b/backend/crypt/crypt.go
@@ -268,6 +268,7 @@ func NewFs(ctx context.Context, name, rpath string, m configmap.Mapper) (fs.Fs,
 		ReadMetadata:            true,
 		WriteMetadata:           true,
 		UserMetadata:            true,
+		PartialUploads:          true,
 	}).Fill(ctx, f).Mask(ctx, wrappedFs).WrapsFs(f, wrappedFs)
 
 	return f, err
diff --git a/backend/googlecloudstorage/googlecloudstorage.go b/backend/googlecloudstorage/googlecloudstorage.go
index c8537372d..4bd7a41e4 100644
--- a/backend/googlecloudstorage/googlecloudstorage.go
+++ b/backend/googlecloudstorage/googlecloudstorage.go
@@ -690,11 +690,7 @@ func (f *Fs) list(ctx context.Context, bucket, directory, prefix string, addBuck
 				fs.Logf(f, "Odd name received %q", object.Name)
 				continue
 			}
-			remote = remote[len(prefix):]
 			isDirectory := remote == "" || strings.HasSuffix(remote, "/")
-			if addBucket {
-				remote = path.Join(bucket, remote)
-			}
 			// is this a directory marker?
 			if isDirectory {
 				// Don't insert the root directory
@@ -704,6 +700,10 @@ func (f *Fs) list(ctx context.Context, bucket, directory, prefix string, addBuck
 				// process directory markers as directories
 				remote = strings.TrimRight(remote, "/")
 			}
+			remote = remote[len(prefix):]
+			if addBucket {
+				remote = path.Join(bucket, remote)
+			}
 
 			err = fn(remote, object, isDirectory)
 			if err != nil {
diff --git a/backend/hasher/hasher.go b/backend/hasher/hasher.go
index e80daedee..3428b4dc2 100644
--- a/backend/hasher/hasher.go
+++ b/backend/hasher/hasher.go
@@ -166,6 +166,7 @@ func NewFs(ctx context.Context, fsname, rpath string, cmap configmap.Mapper) (fs
 		ReadMetadata:            true,
 		WriteMetadata:           true,
 		UserMetadata:            true,
+		PartialUploads:          true,
 	}
 	f.features = stubFeatures.Fill(ctx, f).Mask(ctx, f.Fs).WrapsFs(f, f.Fs)
 
diff --git a/backend/s3/s3.go b/backend/s3/s3.go
index f5a87aaed..6a93af10a 100644
--- a/backend/s3/s3.go
+++ b/backend/s3/s3.go
@@ -3051,6 +3051,7 @@ func NewFs(ctx context.Context, name, root string, m configmap.Mapper) (fs.Fs, e
 	if err != nil {
 		return nil, err
 	}
+	fs.Debugf(nil, "name = %q, root = %q, opt = %#v", name, root, opt)
 	err = checkUploadChunkSize(opt.ChunkSize)
 	if err != nil {
 		return nil, fmt.Errorf("s3: chunk size: %w", err)
@@ -3726,11 +3727,7 @@ func (f *Fs) list(ctx context.Context, opt listOpt, fn listFn) error {
 				fs.Logf(f, "Odd name received %q", remote)
 				continue
 			}
-			remote = remote[len(opt.prefix):]
 			isDirectory := (remote == "" || strings.HasSuffix(remote, "/")) && object.Size != nil && *object.Size == 0
-			if opt.addBucket {
-				remote = bucket.Join(opt.bucket, remote)
-			}
 			// is this a directory marker?
 			if isDirectory {
 				if opt.noSkipMarkers {
@@ -3745,6 +3742,10 @@ func (f *Fs) list(ctx context.Context, opt listOpt, fn listFn) error {
 					remote = strings.TrimRight(remote, "/")
 				}
 			}
+			remote = remote[len(opt.prefix):]
+			if opt.addBucket {
+				remote = bucket.Join(opt.bucket, remote)
+			}
 			if versionIDs != nil {
 				err = fn(remote, object, versionIDs[i], isDirectory)
 			} else {
diff --git a/backend/union/union.go b/backend/union/union.go
index 268860621..c87b6e2d7 100644
--- a/backend/union/union.go
+++ b/backend/union/union.go
@@ -902,6 +902,7 @@ func NewFs(ctx context.Context, name, root string, m configmap.Mapper) (fs.Fs, e
 		ReadMetadata:            true,
 		WriteMetadata:           true,
 		UserMetadata:            true,
+		PartialUploads:          true,
 	}).Fill(ctx, f)
 	canMove, slowHash := true, false
 	for _, f := range upstreams {
diff --git a/backend/zoho/zoho.go b/backend/zoho/zoho.go
index 25b5d24af..d1172fe98 100644
--- a/backend/zoho/zoho.go
+++ b/backend/zoho/zoho.go
@@ -1206,7 +1206,7 @@ func (o *Object) Open(ctx context.Context, options ...fs.OpenOption) (in io.Read
 	if err != nil {
 		return nil, err
 	}
-	if partialContent && resp.StatusCode == 200 {
+	if partialContent && resp.StatusCode == 200 && resp.Header.Get("Content-Range") == "" {
 		if start > 0 {
 			// We need to read and discard the beginning of the data...
 			_, err = io.CopyN(io.Discard, resp.Body, start)
diff --git a/cmd/mountlib/help.go b/cmd/mountlib/help.go
index 03ead5b7e..7e65d16d2 100644
--- a/cmd/mountlib/help.go
+++ b/cmd/mountlib/help.go
@@ -408,20 +408,19 @@ or create systemd mount units:
 |||
 # /etc/systemd/system/mnt-data.mount
 [Unit]
-After=network-online.target
+Description=Mount for /mnt/data
 [Mount]
 Type=rclone
 What=sftp1:subdir
 Where=/mnt/data
-Options=rw,allow_other,args2env,vfs-cache-mode=writes,config=/etc/rclone.conf,cache-dir=/var/rclone
+Options=rw,_netdev,allow_other,args2env,vfs-cache-mode=writes,config=/etc/rclone.conf,cache-dir=/var/rclone
 |||
 
 optionally accompanied by systemd automount unit
 |||
 # /etc/systemd/system/mnt-data.automount
 [Unit]
-After=network-online.target
-Before=remote-fs.target
+Description=AutoMount for /mnt/data
 [Automount]
 Where=/mnt/data
 TimeoutIdleSec=600
diff --git a/docs/content/authors.md b/docs/content/authors.md
index 2360f3cf2..4c0ad378e 100644
--- a/docs/content/authors.md
+++ b/docs/content/authors.md
@@ -729,3 +729,6 @@ put them back in again.` >}}
   * Tareq Sharafy <tareq.sha@gmail.com>
   * kapitainsky <dariuszb@me.com>
   * douchen <playgoobug@gmail.com>
+  * Sam Lai <70988+slai@users.noreply.github.com>
+  * URenko <18209292+URenko@users.noreply.github.com>
+  * Stanislav Gromov <kullfar@gmail.com>
diff --git a/docs/content/commands/rclone_mount.md b/docs/content/commands/rclone_mount.md
index 322a3426c..37583f110 100644
--- a/docs/content/commands/rclone_mount.md
+++ b/docs/content/commands/rclone_mount.md
@@ -407,8 +407,6 @@ mount sftp1:subdir /mnt/data -t rclone -o vfs_cache_mode=writes,sftp_key_file=/p
 or create systemd mount units:
 ```
 # /etc/systemd/system/mnt-data.mount
-[Unit]
-After=network-online.target
 [Mount]
 Type=rclone
 What=sftp1:subdir
@@ -420,7 +418,6 @@ optionally accompanied by systemd automount unit
 ```
 # /etc/systemd/system/mnt-data.automount
 [Unit]
-After=network-online.target
 Before=remote-fs.target
 [Automount]
 Where=/mnt/data
diff --git a/docs/content/docs.md b/docs/content/docs.md
index fe68b65b7..8dc82d96e 100644
--- a/docs/content/docs.md
+++ b/docs/content/docs.md
@@ -1511,6 +1511,25 @@ if you are reading and writing to an OS X filing system this will be
 
 This command line flag allows you to override that computed default.
 
+### --multi-thread-write-buffer-size=SIZE ###
+
+When downloading with multiple threads, rclone will buffer SIZE bytes in
+memory before writing to disk for each thread.
+
+This can improve performance if the underlying filesystem does not deal
+well with a lot of small writes in different positions of the file, so
+if you see downloads being limited by disk write speed, you might want
+to experiment with different values. Specially for magnetic drives and
+remote file systems a higher value can be useful.
+
+Nevertheless, the default of `128k` should be fine for almost all use
+cases, so before changing it ensure that network is not really your
+bottleneck.
+
+As a final hint, size is not the only factor: block size (or similar
+concept) can have an impact. In one case, we observed that exact
+multiples of 16k performed much better than other values.
+
 ### --multi-thread-cutoff=SIZE ###
 
 When downloading files to the local backend above this size, rclone
diff --git a/docs/content/drive.md b/docs/content/drive.md
index c687315e9..12d9b0545 100644
--- a/docs/content/drive.md
+++ b/docs/content/drive.md
@@ -403,7 +403,7 @@ like a symlink in unix, except they point to the underlying file data
 (e.g. the inode in unix terms) so they don't break if the source is
 renamed or moved about.
 
-Be default rclone treats these as follows.
+By default rclone treats these as follows.
 
 For shortcuts pointing to files:
 
diff --git a/fs/accounting/accounting_unix.go b/fs/accounting/accounting_unix.go
index ec79ce544..761466a55 100644
--- a/fs/accounting/accounting_unix.go
+++ b/fs/accounting/accounting_unix.go
@@ -23,15 +23,26 @@ func (tb *tokenBucket) startSignalHandler() {
 		// This runs forever, but blocks until the signal is received.
 		for {
 			<-signals
-			tb.mu.Lock()
-			tb.toggledOff = !tb.toggledOff
-			tb.curr, tb.prev = tb.prev, tb.curr
-			s := "disabled"
-			if !tb.curr._isOff() {
-				s = "enabled"
-			}
-			tb.mu.Unlock()
-			fs.Logf(nil, "Bandwidth limit %s by user", s)
+
+			func() {
+				tb.mu.Lock()
+				defer tb.mu.Unlock()
+
+				// if there's no bandwidth limit configured now, do nothing
+				if !tb.currLimit.Bandwidth.IsSet() {
+					fs.Debugf(nil, "SIGUSR2 received but no bandwidth limit configured right now, ignoring")
+					return
+				}
+
+				tb.toggledOff = !tb.toggledOff
+				tb.curr, tb.prev = tb.prev, tb.curr
+				s := "disabled"
+				if !tb.curr._isOff() {
+					s = "enabled"
+				}
+
+				fs.Logf(nil, "Bandwidth limit %s by user", s)
+			}()
 		}
 	}()
 }
diff --git a/fs/accounting/token_bucket.go b/fs/accounting/token_bucket.go
index c875c568f..29204b5eb 100644
--- a/fs/accounting/token_bucket.go
+++ b/fs/accounting/token_bucket.go
@@ -30,12 +30,11 @@ type buckets [TokenBucketSlots]*rate.Limiter
 
 // tokenBucket holds info about the rate limiters in use
 type tokenBucket struct {
-	mu          sync.RWMutex // protects the token bucket variables
-	curr        buckets
-	prev        buckets
-	toggledOff  bool
-	currLimitMu sync.Mutex // protects changes to the timeslot
-	currLimit   fs.BwTimeSlot
+	mu         sync.RWMutex // protects the token bucket variables
+	curr       buckets
+	prev       buckets
+	toggledOff bool
+	currLimit  fs.BwTimeSlot
 }
 
 // Return true if limit is disabled
@@ -106,11 +105,11 @@ func (tb *tokenBucket) StartTokenBucket(ctx context.Context) {
 	if tb.currLimit.Bandwidth.IsSet() {
 		tb.curr = newTokenBucket(tb.currLimit.Bandwidth)
 		fs.Infof(nil, "Starting bandwidth limiter at %v Byte/s", &tb.currLimit.Bandwidth)
-
-		// Start the SIGUSR2 signal handler to toggle bandwidth.
-		// This function does nothing in windows systems.
-		tb.startSignalHandler()
 	}
+
+	// Start the SIGUSR2 signal handler to toggle bandwidth.
+	// This function does nothing in windows systems.
+	tb.startSignalHandler()
 }
 
 // StartTokenTicker creates a ticker to update the bandwidth limiter every minute.
@@ -126,11 +125,9 @@ func (tb *tokenBucket) StartTokenTicker(ctx context.Context) {
 	go func() {
 		for range ticker.C {
 			limitNow := ci.BwLimit.LimitAt(time.Now())
-			tb.currLimitMu.Lock()
+			tb.mu.Lock()
 
 			if tb.currLimit.Bandwidth != limitNow.Bandwidth {
-				tb.mu.Lock()
-
 				// If bwlimit is toggled off, the change should only
 				// become active on the next toggle, which causes
 				// an exchange of tb.curr <-> tb.prev
@@ -156,9 +153,9 @@ func (tb *tokenBucket) StartTokenTicker(ctx context.Context) {
 				}
 
 				tb.currLimit = limitNow
-				tb.mu.Unlock()
 			}
-			tb.currLimitMu.Unlock()
+
+			tb.mu.Unlock()
 		}
 	}()
 }
diff --git a/fs/config.go b/fs/config.go
index a6de70b85..060567447 100644
--- a/fs/config.go
+++ b/fs/config.go
@@ -51,101 +51,102 @@ var (
 
 // ConfigInfo is filesystem config options
 type ConfigInfo struct {
-	LogLevel                LogLevel
-	StatsLogLevel           LogLevel
-	UseJSONLog              bool
-	DryRun                  bool
-	Interactive             bool
-	CheckSum                bool
-	SizeOnly                bool
-	IgnoreTimes             bool
-	IgnoreExisting          bool
-	IgnoreErrors            bool
-	ModifyWindow            time.Duration
-	Checkers                int
-	Transfers               int
-	ConnectTimeout          time.Duration // Connect timeout
-	Timeout                 time.Duration // Data channel timeout
-	ExpectContinueTimeout   time.Duration
-	Dump                    DumpFlags
-	InsecureSkipVerify      bool // Skip server certificate verification
-	DeleteMode              DeleteMode
-	MaxDelete               int64
-	MaxDeleteSize           SizeSuffix
-	TrackRenames            bool   // Track file renames.
-	TrackRenamesStrategy    string // Comma separated list of strategies used to track renames
-	LowLevelRetries         int
-	UpdateOlder             bool // Skip files that are newer on the destination
-	NoGzip                  bool // Disable compression
-	MaxDepth                int
-	IgnoreSize              bool
-	IgnoreChecksum          bool
-	IgnoreCaseSync          bool
-	NoTraverse              bool
-	CheckFirst              bool
-	NoCheckDest             bool
-	NoUnicodeNormalization  bool
-	NoUpdateModTime         bool
-	DataRateUnit            string
-	CompareDest             []string
-	CopyDest                []string
-	BackupDir               string
-	Suffix                  string
-	SuffixKeepExtension     bool
-	UseListR                bool
-	BufferSize              SizeSuffix
-	BwLimit                 BwTimetable
-	BwLimitFile             BwTimetable
-	TPSLimit                float64
-	TPSLimitBurst           int
-	BindAddr                net.IP
-	DisableFeatures         []string
-	UserAgent               string
-	Immutable               bool
-	AutoConfirm             bool
-	StreamingUploadCutoff   SizeSuffix
-	StatsFileNameLength     int
-	AskPassword             bool
-	PasswordCommand         SpaceSepList
-	UseServerModTime        bool
-	MaxTransfer             SizeSuffix
-	MaxDuration             time.Duration
-	CutoffMode              CutoffMode
-	MaxBacklog              int
-	MaxStatsGroups          int
-	StatsOneLine            bool
-	StatsOneLineDate        bool   // If we want a date prefix at all
-	StatsOneLineDateFormat  string // If we want to customize the prefix
-	ErrorOnNoTransfer       bool   // Set appropriate exit code if no files transferred
-	Progress                bool
-	ProgressTerminalTitle   bool
-	Cookie                  bool
-	UseMmap                 bool
-	CaCert                  []string // Client Side CA
-	ClientCert              string   // Client Side Cert
-	ClientKey               string   // Client Side Key
-	MultiThreadCutoff       SizeSuffix
-	MultiThreadStreams      int
-	MultiThreadSet          bool   // whether MultiThreadStreams was set (set in fs/config/configflags)
-	OrderBy                 string // instructions on how to order the transfer
-	UploadHeaders           []*HTTPOption
-	DownloadHeaders         []*HTTPOption
-	Headers                 []*HTTPOption
-	MetadataSet             Metadata // extra metadata to write when uploading
-	RefreshTimes            bool
-	NoConsole               bool
-	TrafficClass            uint8
-	FsCacheExpireDuration   time.Duration
-	FsCacheExpireInterval   time.Duration
-	DisableHTTP2            bool
-	HumanReadable           bool
-	KvLockTime              time.Duration // maximum time to keep key-value database locked by process
-	DisableHTTPKeepAlives   bool
-	Metadata                bool
-	ServerSideAcrossConfigs bool
-	TerminalColorMode       TerminalColorMode
-	DefaultTime             Time // time that directories with no time should display
-	Inplace                 bool // Download directly to destination file instead of atomic download to temp/rename
+	LogLevel                   LogLevel
+	StatsLogLevel              LogLevel
+	UseJSONLog                 bool
+	DryRun                     bool
+	Interactive                bool
+	CheckSum                   bool
+	SizeOnly                   bool
+	IgnoreTimes                bool
+	IgnoreExisting             bool
+	IgnoreErrors               bool
+	ModifyWindow               time.Duration
+	Checkers                   int
+	Transfers                  int
+	ConnectTimeout             time.Duration // Connect timeout
+	Timeout                    time.Duration // Data channel timeout
+	ExpectContinueTimeout      time.Duration
+	Dump                       DumpFlags
+	InsecureSkipVerify         bool // Skip server certificate verification
+	DeleteMode                 DeleteMode
+	MaxDelete                  int64
+	MaxDeleteSize              SizeSuffix
+	TrackRenames               bool   // Track file renames.
+	TrackRenamesStrategy       string // Comma separated list of strategies used to track renames
+	LowLevelRetries            int
+	UpdateOlder                bool // Skip files that are newer on the destination
+	NoGzip                     bool // Disable compression
+	MaxDepth                   int
+	IgnoreSize                 bool
+	IgnoreChecksum             bool
+	IgnoreCaseSync             bool
+	NoTraverse                 bool
+	CheckFirst                 bool
+	NoCheckDest                bool
+	NoUnicodeNormalization     bool
+	NoUpdateModTime            bool
+	DataRateUnit               string
+	CompareDest                []string
+	CopyDest                   []string
+	BackupDir                  string
+	Suffix                     string
+	SuffixKeepExtension        bool
+	UseListR                   bool
+	BufferSize                 SizeSuffix
+	MultiThreadWriteBufferSize SizeSuffix
+	BwLimit                    BwTimetable
+	BwLimitFile                BwTimetable
+	TPSLimit                   float64
+	TPSLimitBurst              int
+	BindAddr                   net.IP
+	DisableFeatures            []string
+	UserAgent                  string
+	Immutable                  bool
+	AutoConfirm                bool
+	StreamingUploadCutoff      SizeSuffix
+	StatsFileNameLength        int
+	AskPassword                bool
+	PasswordCommand            SpaceSepList
+	UseServerModTime           bool
+	MaxTransfer                SizeSuffix
+	MaxDuration                time.Duration
+	CutoffMode                 CutoffMode
+	MaxBacklog                 int
+	MaxStatsGroups             int
+	StatsOneLine               bool
+	StatsOneLineDate           bool   // If we want a date prefix at all
+	StatsOneLineDateFormat     string // If we want to customize the prefix
+	ErrorOnNoTransfer          bool   // Set appropriate exit code if no files transferred
+	Progress                   bool
+	ProgressTerminalTitle      bool
+	Cookie                     bool
+	UseMmap                    bool
+	CaCert                     []string // Client Side CA
+	ClientCert                 string   // Client Side Cert
+	ClientKey                  string   // Client Side Key
+	MultiThreadCutoff          SizeSuffix
+	MultiThreadStreams         int
+	MultiThreadSet             bool   // whether MultiThreadStreams was set (set in fs/config/configflags)
+	OrderBy                    string // instructions on how to order the transfer
+	UploadHeaders              []*HTTPOption
+	DownloadHeaders            []*HTTPOption
+	Headers                    []*HTTPOption
+	MetadataSet                Metadata // extra metadata to write when uploading
+	RefreshTimes               bool
+	NoConsole                  bool
+	TrafficClass               uint8
+	FsCacheExpireDuration      time.Duration
+	FsCacheExpireInterval      time.Duration
+	DisableHTTP2               bool
+	HumanReadable              bool
+	KvLockTime                 time.Duration // maximum time to keep key-value database locked by process
+	DisableHTTPKeepAlives      bool
+	Metadata                   bool
+	ServerSideAcrossConfigs    bool
+	TerminalColorMode          TerminalColorMode
+	DefaultTime                Time // time that directories with no time should display
+	Inplace                    bool // Download directly to destination file instead of atomic download to temp/rename
 }
 
 // NewConfig creates a new config with everything set to the default
@@ -170,6 +171,7 @@ func NewConfig() *ConfigInfo {
 	c.MaxDepth = -1
 	c.DataRateUnit = "bytes"
 	c.BufferSize = SizeSuffix(16 << 20)
+	c.MultiThreadWriteBufferSize = SizeSuffix(128 * 1024)
 	c.UserAgent = "rclone/" + Version
 	c.StreamingUploadCutoff = SizeSuffix(100 * 1024)
 	c.MaxStatsGroups = 1000
diff --git a/fs/config/configflags/configflags.go b/fs/config/configflags/configflags.go
index 60047227e..a3eeedf70 100644
--- a/fs/config/configflags/configflags.go
+++ b/fs/config/configflags/configflags.go
@@ -126,6 +126,7 @@ func AddFlags(ci *fs.ConfigInfo, flagSet *pflag.FlagSet) {
 	flags.StringVarP(flagSet, &ci.ClientKey, "client-key", "", ci.ClientKey, "Client SSL private key (PEM) for mutual TLS auth")
 	flags.FVarP(flagSet, &ci.MultiThreadCutoff, "multi-thread-cutoff", "", "Use multi-thread downloads for files above this size")
 	flags.IntVarP(flagSet, &ci.MultiThreadStreams, "multi-thread-streams", "", ci.MultiThreadStreams, "Max number of streams to use for multi-thread downloads")
+	flags.FVarP(flagSet, &ci.MultiThreadWriteBufferSize, "multi-thread-write-buffer-size", "", "In memory buffer size for writing when in multi-thread mode")
 	flags.BoolVarP(flagSet, &ci.UseJSONLog, "use-json-log", "", ci.UseJSONLog, "Use json log format")
 	flags.StringVarP(flagSet, &ci.OrderBy, "order-by", "", ci.OrderBy, "Instructions on how to order the transfers, e.g. 'size,descending'")
 	flags.StringArrayVarP(flagSet, &uploadHeaders, "header-upload", "", nil, "Set HTTP header for upload transactions")
diff --git a/fs/newfs.go b/fs/newfs.go
index 52375e86e..9be890ec7 100644
--- a/fs/newfs.go
+++ b/fs/newfs.go
@@ -115,11 +115,11 @@ func ParseRemote(path string) (fsInfo *RegInfo, configName, fsPath string, conne
 	return fsInfo, configName, fsPath, parsed.Config, err
 }
 
-// ConfigString returns a canonical version of the config string used
+// configString returns a canonical version of the config string used
 // to configure the Fs as passed to fs.NewFs
-func ConfigString(f Fs) string {
+func configString(f Fs, full bool) string {
 	name := f.Name()
-	if open := strings.IndexRune(name, '{'); open >= 0 && strings.HasSuffix(name, "}") {
+	if open := strings.IndexRune(name, '{'); full && open >= 0 && strings.HasSuffix(name, "}") {
 		suffix := name[open:]
 		overriddenConfigMu.Lock()
 		config, ok := overriddenConfig[suffix]
@@ -137,6 +137,21 @@ func ConfigString(f Fs) string {
 	return name + ":" + root
 }
 
+// ConfigString returns a canonical version of the config string used
+// to configure the Fs as passed to fs.NewFs. For Fs with extra
+// parameters this will include a canonical {hexstring} suffix.
+func ConfigString(f Fs) string {
+	return configString(f, false)
+}
+
+// ConfigStringFull returns a canonical version of the config string
+// used to configure the Fs as passed to fs.NewFs. This string can be
+// used to re-instantiate the Fs exactly so includes all the extra
+// parameters passed in.
+func ConfigStringFull(f Fs) string {
+	return configString(f, true)
+}
+
 // TemporaryLocalFs creates a local FS in the OS's temporary directory.
 //
 // No cleanup is performed, the caller must call Purge on the Fs themselves.
diff --git a/fs/operations/multithread.go b/fs/operations/multithread.go
index b4b79a5a6..5ab41b9a2 100644
--- a/fs/operations/multithread.go
+++ b/fs/operations/multithread.go
@@ -1,6 +1,7 @@
 package operations
 
 import (
+	"bufio"
 	"context"
 	"errors"
 	"fmt"
@@ -12,11 +13,32 @@ import (
 )
 
 const (
-	multithreadChunkSize     = 64 << 10
-	multithreadChunkSizeMask = multithreadChunkSize - 1
-	multithreadBufferSize    = 32 * 1024
+	multithreadChunkSize      = 64 << 10
+	multithreadChunkSizeMask  = multithreadChunkSize - 1
+	multithreadReadBufferSize = 32 * 1024
 )
 
+// An offsetWriter maps writes at offset base to offset base+off in the underlying writer.
+//
+// Modified from the go source code. Can be replaced with
+// io.OffsetWriter when we no longer need to support go1.19
+type offsetWriter struct {
+	w   io.WriterAt
+	off int64 // the current offset
+}
+
+// newOffsetWriter returns an offsetWriter that writes to w
+// starting at offset off.
+func newOffsetWriter(w io.WriterAt, off int64) *offsetWriter {
+	return &offsetWriter{w, off}
+}
+
+func (o *offsetWriter) Write(p []byte) (n int, err error) {
+	n, err = o.w.WriteAt(p, o.off)
+	o.off += int64(n)
+	return
+}
+
 // Return a boolean as to whether we should use multi thread copy for
 // this transfer
 func doMultiThreadCopy(ctx context.Context, f fs.Fs, src fs.Object) bool {
@@ -62,6 +84,7 @@ type multiThreadCopyState struct {
 
 // Copy a single stream into place
 func (mc *multiThreadCopyState) copyStream(ctx context.Context, stream int) (err error) {
+	ci := fs.GetConfig(ctx)
 	defer func() {
 		if err != nil {
 			fs.Debugf(mc.src, "multi-thread copy: stream %d/%d failed: %v", stream+1, mc.streams, err)
@@ -84,8 +107,13 @@ func (mc *multiThreadCopyState) copyStream(ctx context.Context, stream int) (err
 	}
 	defer fs.CheckClose(rc, &err)
 
+	var writer io.Writer = newOffsetWriter(mc.wc, start)
+	if ci.MultiThreadWriteBufferSize > 0 {
+		writer = bufio.NewWriterSize(writer, int(ci.MultiThreadWriteBufferSize))
+		fs.Debugf(mc.src, "multi-thread copy: write buffer set to %v", ci.MultiThreadWriteBufferSize)
+	}
 	// Copy the data
-	buf := make([]byte, multithreadBufferSize)
+	buf := make([]byte, multithreadReadBufferSize)
 	offset := start
 	for {
 		// Check if context cancelled and exit if so
@@ -98,7 +126,7 @@ func (mc *multiThreadCopyState) copyStream(ctx context.Context, stream int) (err
 			if err != nil {
 				return fmt.Errorf("multipart copy: accounting failed: %w", err)
 			}
-			nw, ew := mc.wc.WriteAt(buf[0:nr], offset)
+			nw, ew := writer.Write(buf[0:nr])
 			if nw > 0 {
 				offset += int64(nw)
 			}
@@ -113,6 +141,16 @@ func (mc *multiThreadCopyState) copyStream(ctx context.Context, stream int) (err
 			if er != io.EOF {
 				return fmt.Errorf("multipart copy: read failed: %w", er)
 			}
+
+			// if we were buffering, flush do disk
+			switch w := writer.(type) {
+			case *bufio.Writer:
+				er2 := w.Flush()
+				if er2 != nil {
+					return fmt.Errorf("multipart copy: flush failed: %w", er2)
+				}
+			}
+
 			break
 		}
 	}
diff --git a/fs/operations/operations.go b/fs/operations/operations.go
index 4308df7fa..8c20fbe40 100644
--- a/fs/operations/operations.go
+++ b/fs/operations/operations.go
@@ -529,6 +529,9 @@ func Copy(ctx context.Context, f fs.Fs, dst fs.Object, remote string, src fs.Obj
 	if err != nil {
 		err = fs.CountError(err)
 		fs.Errorf(src, "Failed to copy: %v", err)
+		if !inplace {
+			removeFailedPartialCopy(ctx, f, remotePartial)
+		}
 		return newDst, err
 	}
 


Test patch:
diff --git a/backend/s3/s3_internal_test.go b/backend/s3/s3_internal_test.go
index e23c68bc3..f48045072 100644
--- a/backend/s3/s3_internal_test.go
+++ b/backend/s3/s3_internal_test.go
@@ -18,6 +18,7 @@ import (
 	"github.com/rclone/rclone/fs/hash"
 	"github.com/rclone/rclone/fstest"
 	"github.com/rclone/rclone/fstest/fstests"
+	"github.com/rclone/rclone/lib/bucket"
 	"github.com/rclone/rclone/lib/random"
 	"github.com/rclone/rclone/lib/version"
 	"github.com/stretchr/testify/assert"
@@ -317,15 +318,19 @@ func (f *Fs) InternalTestVersions(t *testing.T) {
 
 		// Check we can make a NewFs from that object with a version suffix
 		t.Run("NewFs", func(t *testing.T) {
-			newPath := path.Join(fs.ConfigString(f), fileNameVersion)
+			newPath := bucket.Join(fs.ConfigStringFull(f), fileNameVersion)
 			// Make sure --s3-versions is set in the config of the new remote
-			confPath := strings.Replace(newPath, ":", ",versions:", 1)
-			fNew, err := cache.Get(ctx, confPath)
+			fs.Debugf(nil, "oldPath = %q", newPath)
+			lastColon := strings.LastIndex(newPath, ":")
+			require.True(t, lastColon >= 0)
+			newPath = newPath[:lastColon] + ",versions" + newPath[lastColon:]
+			fs.Debugf(nil, "newPath = %q", newPath)
+			fNew, err := cache.Get(ctx, newPath)
 			// This should return pointing to a file
 			require.Equal(t, fs.ErrorIsFile, err)
 			require.NotNil(t, fNew)
 			// With the directory the directory above
-			assert.Equal(t, dirName, path.Base(fs.ConfigString(fNew)))
+			assert.Equal(t, dirName, path.Base(fs.ConfigStringFull(fNew)))
 		})
 	})
 
diff --git a/fs/newfs_test.go b/fs/newfs_test.go
index 3f86cd2d5..4d5ef8520 100644
--- a/fs/newfs_test.go
+++ b/fs/newfs_test.go
@@ -32,12 +32,14 @@ func TestNewFs(t *testing.T) {
 	assert.Equal(t, ":mockfs{S_NHG}", f2.Name())
 	assert.Equal(t, "/tmp", f2.Root())
 
-	assert.Equal(t, ":mockfs,potato='true':/tmp", fs.ConfigString(f2))
+	assert.Equal(t, ":mockfs{S_NHG}:/tmp", fs.ConfigString(f2))
+	assert.Equal(t, ":mockfs,potato='true':/tmp", fs.ConfigStringFull(f2))
 
 	f3, err := fs.NewFs(ctx, ":mockfs,potato='true':/tmp")
 	require.NoError(t, err)
 	assert.Equal(t, ":mockfs{S_NHG}", f3.Name())
 	assert.Equal(t, "/tmp", f3.Root())
 
-	assert.Equal(t, ":mockfs,potato='true':/tmp", fs.ConfigString(f3))
+	assert.Equal(t, ":mockfs{S_NHG}:/tmp", fs.ConfigString(f3))
+	assert.Equal(t, ":mockfs,potato='true':/tmp", fs.ConfigStringFull(f3))
 }