cmd/sync: allocate memory from sync.Pool instead of Chunk.NewOffPage to avoid stall problem

[YinhaoHu]

close #5492

All unit tests related to this update are adjusted and passed.

The case mentioned in the issue can also run now.

[CLAassistant]

All committers have signed the CLA.

[zhijian-pro]

@YinhaoHu Thank you very much for your contribution, but we think the following approach would be more appropriate.
The issue lies in our sync download logic where we shouldn't be using chunk.NewOffPage to allocate memory.

juicefs/pkg/sync/download.go

Line 76 in b51019d

p := chunk.NewOffPage(int(size))

In this line of code

juicefs/pkg/sync/sync.go

Line 362 in b51019d

downer := newParallelDownloader(src, key, size, 10<<20, concurrent)

we have hard-coded a block size of 10M, so we can directly use a fixed-size sync.pool to allocate memory.

[YinhaoHu]

@zhijian-pro Good idea. I have updated this PR based on the more appropriate approach.

[zhijian-pro]

It is best to adjust the TestDownload unit test so that it passes

[YinhaoHu]

It is best to adjust the TestDownload unit test so that it passes

All test cases in the TestDownload unit test are modified in an equivalent approach as before. And they all pass.

[zhijian-pro]

@YinhaoHu If the size of the last block is smaller than the default block size, it should not be put back into the pool, otherwise it will cause panic when you get it.

[YinhaoHu]

@YinhaoHu If the size of the last block is smaller than the default block size, it should not be put back into the pool, otherwise it will cause panic when you get it.

The capacity of each buf is always downloadBufSize even if the length is variable, maybe smaller than the default block size, and there will be no block requires capacity more than downloadBufSize. I don't think panic will occur. Or could you show me a case where the panic will occur?

[zhijian-pro]

The last block returned when file A is downloaded may be removed from the pool when file B is downloaded.
But this block may be less than the length of downloadBufSize, and the B file wants it to be downloadBufSize.

[YinhaoHu]

The last block returned when file A is downloaded may be removed from the pool when file B is downloaded. But this block may be less than the length of downloadBufSize, and the B file wants it to be downloadBufSize.

The New function of downloadBufPool is:

New: func() interface{} {
		buf := make([]byte, 0, downloadBufSize)
		return &buf
	},

And the capacity is always downloadBufSize. In your case, the buf might be sliced to length downloadBufSize/2 and returned to the pool after file A is downloaded. The buf would be got again for downloading file B. It is OK to slice it to length downloadBufSize which is not greater than its capacity. If I have got your point, you're meaning the code case below which does not panic:

const downloadBufSize = 10 << 20

var downloadBufPool = sync.Pool{
	New: func() interface{} {
		buf := make([]byte, 0, downloadBufSize)
		return &buf
	},
}

func Main() {
	downloadFileA()
	downloadFileB()
}

func downloadFileA() {
	buf := downloadBufPool.Get().(*[]byte)
	*buf = (*buf)[:downloadBufSize/2]
	// read file
	printBuf("A", buf)
	downloadBufPool.Put(buf)
}

func downloadFileB() {
	buf := downloadBufPool.Get().(*[]byte)
	*buf = (*buf)[:downloadBufSize]
	// read file
	printBuf("B", buf)
	downloadBufPool.Put(buf)
}

func printBuf(name string, buf *[]byte) {
	fmt.Printf("%v: len = %v, cap = %v\n", name, len(*buf), cap(*buf))
}

Output:

A: len = 5242880, cap = 10485760
B: len = 10485760, cap = 10485760

[zhijian-pro]

OK，I missed *p = (*p)[:size].