sync: reduce memory allocation when write into files

[201341]

Signed-off-by: swj [email protected]

[codecov-commenter]

Codecov Report

Merging #1644 (5c095fd) into main (5fc885c) will increase coverage by 0.06%.
The diff coverage is 78.26%.

❗ Current head 5c095fd differs from pull request most recent head 93c6042. Consider uploading reports for the commit 93c6042 to get more accurate results

@@            Coverage Diff             @@
##             main    #1644      +/-   ##
==========================================
+ Coverage   61.49%   61.55%   +0.06%     
==========================================
  Files         133      133              
  Lines       22475    22565      +90     
==========================================
+ Hits        13820    13889      +69     
- Misses       7088     7116      +28     
+ Partials     1567     1560       -7     

Impacted Files	Coverage Δ
pkg/meta/config.go	54.66% <ø> (ø)
pkg/meta/base.go	72.41% <25.00%> (+0.17%)	⬆️
pkg/chunk/cached_store.go	78.70% <77.77%> (+2.07%)	⬆️
cmd/fsck.go	68.36% <100.00%> (ø)
cmd/gc.go	78.92% <100.00%> (ø)
cmd/mount.go	70.08% <100.00%> (ø)
pkg/object/file.go	65.51% <100.00%> (+4.76%)	⬆️
pkg/object/sharding.go	69.52% <100.00%> (-1.35%)	⬇️
pkg/object/interface.go	80.00% <0.00%> (-20.00%)	⬇️
pkg/object/prefix.go	69.76% <0.00%> (-7.16%)	⬇️
... and 16 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b79d6e...93c6042. Read the comment docs.

[SandyXSD]

@201341 Could you provide some detailed explanation?

[201341]

@201341 Could you provide some detailed explanation?

For file2file, io.CopyBuffer -> readfrom->io.Copy, it doesn't use the bufpool.

[SandyXSD]

*os.File has the method ReadFrom, why do you want to convert its type to WriteCloser?

[201341]

*os.File has the method ReadFrom, why do you want to convert its type to WriteCloser?

In order to use the buffer, not to alloc the memory, we need to convert to WriteCloser.

[davies]

@201341 io.CopyBuffer() will use copy_file_range (zero copy or in kernel) in linux, and fallback to io.Copy() in other OS, where do you saw the allocation?

[201341]

@201341 io.CopyBuffer() will use copy_file_range (zero copy or in kernel) in linux, and fallback to io.Copy() in other OS, where do you saw the allocation?

From pprof, io.CopyBuffer->io.Copy() takes up most of the memory, And I see the code io.Copy allocate the new memory!

[davies]

What's the OS and version you are using?

[201341]

What's the OS and version you are using?

The version is Centos7(kernel 3.x), And it seems not support the copy_file_range. Is there any way to optimize the put interface?

[SandyXSD]

This change makes sense since ReadFrom of File requires the src to be a *File as well to use copy_file_range, while in JuiceFS usually the src is bytes.Reader (except for sync/backup).

src, ok := r.(*File)
if !ok {
    return 0, false, nil
}

[davies]

I see, the pool is not used when the in is not a File, or copy_file_range is not available.

We should check the difference when src and dst are disk and copy_file_range is available.

[SandyXSD]

@201341 It looks like this commit won't change the behavior at all, cuz io.WriteCloser(f) still contains the ReadFrom method. Could you double check to confirm if it fixes the issue of io.Copy?

[201341]

@SandyXSD Yes, it doesn't change the behavior. I have changed the code. Now it can use the buffer, By the way, copy_file_range has the dataloss risk (syncthing/syncthing#4271)

[SandyXSD]

Cool, and we need a method to check kernel version. After a quick look at the issue you linked, it looks that the bug is only NFS related?

[davies]

@201341 Thanks! This patch disable the copy_file_range when both source and destination are same file system, we will find a way to enable it when it's safe.

[penglaixy99]

From golang official webpage: https://pkg.go.dev/io#CopyBuffer
CopyBuffer is identical to Copy except that it stages through the provided buffer (if one is required) rather than allocating a temporary one. If buf is nil, one is allocated; otherwise if it has zero length, CopyBuffer panics.

If either src implements WriterTo or dst implements ReaderFrom, buf will not be used to perform the copy.