Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reflink(Copy On Write)支持 #39

Merged
merged 9 commits into from May 10, 2023
Merged

Conversation

tksmly
Copy link
Contributor

@tksmly tksmly commented Mar 23, 2023

#38
未经过充分测试,基本功能正常

在btrfs下备份时间由30.1s缩短至2.2s
xfs,zfs同理
其他文件系统理论上无影响,未测试

@Fallen-Breath
Copy link
Collaborator

  • 请完成充分的测试
  • 请使用英文进行注释
  • 请主动释放 open 的文件对象

@Fallen-Breath Fallen-Breath marked this pull request as draft March 26, 2023 04:31
@tksmly tksmly marked this pull request as ready for review March 26, 2023 05:41
@tksmly tksmly marked this pull request as draft March 26, 2023 06:24
@tksmly
Copy link
Contributor Author

tksmly commented Mar 26, 2023

我怀疑我是***但是我没有证据
为什么没有人告诉我这玩意只有linux才有

@tksmly tksmly marked this pull request as ready for review March 27, 2023 02:36
@Fallen-Breath
Copy link
Collaborator

在 review 前,请完成充分的测试,包括但不限于在不同系统平台下,开启/关闭所述功能的拷贝准确性,以及速度对比

@tksmly
Copy link
Contributor Author

tksmly commented Mar 27, 2023

在 review 前,请完成充分的测试,包括但不限于在不同系统平台下,开启/关闭所述功能的拷贝准确性,以及速度对比

拷贝准确性已完成测试,速度对比进行

@tksmly
Copy link
Contributor Author

tksmly commented Mar 27, 2023

为保证准确性删除session.lock后进行压缩对比md5
将windows11iso复制进world目录进行大文件测试
将含有18000+文件的.config目录复制进world目录进行小文件测试

@tksmly
Copy link
Contributor Author

tksmly commented Mar 27, 2023

arch linux+ntfs:37.6s-48.1s
arch linux+btrfs:0.6s-2.2s

windows11iso md5相同
zip记录打包时间,就算是一样的文件md5也不同.......
随便拉几个出来都是没问题的

@tksmly
Copy link
Contributor Author

tksmly commented Mar 27, 2023

windows+ntfs:60.9-61.5

@tksmly
Copy link
Contributor Author

tksmly commented Mar 27, 2023

原版qbm:
windows+ntfs:60.3-61.7
arch linux+ntfs:43.0-50.7
arch linux+btrfs:15.2-19.4

除了btrfs其他的都是误差范围

@Fallen-Breath
Copy link
Collaborator

请在进行完所有的测试后,进行统一的结果汇总展示,而不是把这里当做测试实况播报

@tksmly
Copy link
Contributor Author

tksmly commented Mar 27, 2023

环境 Reflink支持 原版qbm
linux+ntfs 37.6s-48.1s 43.0s-50.7s
linux+btrfs 0.6s-2.2s 15.2s-19.4s
windows+ntfs 60.9s-61.5s 60.3s-61.7s

误差很大,但是除了btrfs其他的都是误差范围

拷贝准确性没有任何问题

@Fallen-Breath
Copy link
Collaborator

环境 Reflink支持 原版qbm
linux+ntfs 37.6s-48.1s 43.0s-50.7s
linux+btrfs 0.6s-2.2s 15.2s-19.4s
windows+ntfs 60.9s-61.5s 60.3s-61.7s

误差很大,但是除了btrfs其他的都是误差范围

拷贝准确性没有任何问题

测试方式是什么,包括速度的测试方式,以及准确性的测试方式

测试的环境是什么,包括具体系统版本,硬盘硬件信息

是否有测试多world_names的情况,world_names含单独文件的情况,world_name含文件路径分隔符的各种特殊情况

shutil.copy(src_path, dst_path)
else:

_cpcow(src_path, dst_path)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

可以考虑使用 shutil.copytree 函数的 copy_function 参数来实现自定义的拷贝,而非自己造一套递归拼路径的轮子(更容易出错,维护更麻烦)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果当前环境不支持 os.copy_file_range,那就没必要走新的这堆逻辑了

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

可以考虑使用 shutil.copytree 函数的 copy_function 参数来实现自定义的拷贝,而非自己造一套递归拼路径的轮子(更容易出错,维护更麻烦)

已修改

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果当前环境不支持 os.copy_file_range,那就没必要走新的这堆逻辑了

如果不支持,和shutil.copy2一样

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果不支持,和shutil.copy2一样

什么叫“一样”

shutil.copy(src_path, dst_path)
else:
shutil.copy(src_path, dst_path)
elif os.path.isdir(src_path):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

对于 elif 仍不匹配的情况,建议输出一条警告日志。现在这样什么都不做,在出问题时不好定位

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

对于 elif 仍不匹配的情况,建议输出一条警告日志。现在这样什么都不做,在出问题时不好定位

134-135

if not os.path.isdir(src_path) and not os.path.isfile(src_path):
    server_inst.logger.warning('{} does not exist while copying ({} -> {})'.format(src_path, src_path, dst_path))

确实移到拷贝函数里好一点

@tksmly
Copy link
Contributor Author

tksmly commented Mar 28, 2023

应该是没问题了

@Fallen-Breath
Copy link
Collaborator

请不遗漏地阅读我提出的所有测试/实现的修改点,并做出完善/回应

@tksmly
Copy link
Contributor Author

tksmly commented Mar 29, 2023

环境
Reflink支持
原版qbm

linux+ntfs
37.6s-48.1s
43.0s-50.7s

linux+btrfs
0.6s-2.2s
15.2s-19.4s

windows+ntfs
60.9s-61.5s
60.3s-61.7s

误差很大,但是除了btrfs其他的都是误差范围
拷贝准确性没有任何问题

测试方式是什么,包括速度的测试方式,以及准确性的测试方式

测试的环境是什么,包括具体系统版本,硬盘硬件信息

是否有测试多world_names的情况,world_names含单独文件的情况,world_name含文件路径分隔符的各种特殊情况

备份三次时间,md5
btrfs和ntfs不是同一块硬盘,只有横向有可比性

只改了复制函数,world_names的判定还是之前的

@tksmly
Copy link
Contributor Author

tksmly commented Apr 9, 2023

#39 (comment)
也许旧的可读性更高

with open(src_path,'rb') as f11, open(dst_path,'wb+') as f21:
	f1 = f11.fileno()
	f2 = f21.fileno()
	size = os.path.getsize(src_path)
		if size > COW_COPY_LIMIT:
			for i in range(0, size, COW_COPY_LIMIT):
				os.copy_file_range(f1, f2, COW_COPY_LIMIT, i) 
		else:
			os.copy_file_range(f1, f2, size)

@Fallen-Breath
Copy link
Collaborator

注意 os.copy_file_range 的文档,可以不传递 offset_srcoffset_dst 这两个变量。因此这部分代码可以简化为:

with open(src_file, 'rb') as src, open(dst_file, 'wb') as dst:
    while True:
        if os.copy_file_range(src.fileno(), dst.fileno(), BLOCK_SIZE) <= 0:
            break

同时,正如文档中所述

Additionally, some filesystems could implement extra optimizations, such as the use of reflinks (i.e., two or more inodes that share pointers to the same copy-on-write disk blocks; supported file systems include btrfs and XFS) and server-side copy (in the case of NFS).

os.copy_file_range 并不能保证可以使用 COW 优化。在 Ubuntu20,内核 5.13.0,python 3.8.10,ext4 文件系统 的环境下测试,os.copy_file_rangeshutil.copy 慢了 7%。我建议将 copy_file_range 作为一个可配置的特性开关,并且保持默认关闭。

@tksmly
Copy link
Contributor Author

tksmly commented May 2, 2023

作为一个可配置的特性开关,并且保持默认关闭。

那样的话也许应该试试通用的增量备份?
未更改的文件只保留一份最旧的

@Fallen-Breath
Copy link
Collaborator

作为一个可配置的特性开关,并且保持默认关闭。

那样的话也许应该试试通用的增量备份? 未更改的文件只保留一份最旧的

这是另一个 feature request,与本 PR 无关

@tksmly
Copy link
Contributor Author

tksmly commented May 2, 2023

因此这部分代码可以简化为:

with open(src_file, 'rb') as src, open(dst_file, 'wb') as dst:
    while True:
        if os.copy_file_range(src.fileno(), dst.fileno(), BLOCK_SIZE) <= 0:
            break

也许应该更进一步成这样?

with open(src_file, 'rb') as src, open(dst_file, 'wb') as dst:
    while os.copy_file_range(src.fileno(), dst.fileno(), BLOCK_SIZE) :
        pass

@Fallen-Breath
Copy link
Collaborator

Fallen-Breath commented May 2, 2023

当然可以。#39 (comment) 的代码仅仅是一个例子,并非必须实现

不过,修改后的实现进行了从 intbool 的隐式类型转换,对我个人而言我是不推崇这种做法的。此外这里还作出了 copy_file_range 返回值不会小于 0 的期望(虽然问题应该不大)。具体究竟怎么实现,你自己决定

@@ -76,6 +76,30 @@ def get_backup_file_name(backup_format: BackupFormat):
raise ValueError('unknown backup mode {}'.format(backup_format))


copy_file_range_supported=hasattr(os, "copy_file_range")
COW_COPY_LIMIT = 2**30 # 1GB / need int, may overflow, so cannot copy files larger than 2GB in a single pass
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

现在这个变量的含义已经不再是个“limit”,更像是个“buffer/block size”。改成 COW_COPY_BUFFER_SIZE 什么的会不会更加准确?对应的,其注释也有些过时了。

除此之外,是否有测试过这个值取 1MiB、10MiB、1GiB 等情况下的性能表现。可以测一下取最优的

@Fallen-Breath
Copy link
Collaborator

基本没什么问题了,然后就是得整理下代码格式,包括运算符前后的空格、logger.warning 里混用 str.format+、无用的空行、行内注释的 2 空格等等,参考 pep8

最后就是给 readme 加上新配置项的文档了

@Fallen-Breath
Copy link
Collaborator

#39 (comment) 这里提的 COW_COPY_BUFFER_SIZE 的取值测试做一下,然后就 ok 了。之后合并了我再整理下细节吧

@tksmly
Copy link
Contributor Author

tksmly commented May 10, 2023

#39 (comment) 这里提的 COW_COPY_BUFFER_SIZE 的取值测试做一下,然后就 ok 了。之后合并了我再整理下细节吧

更大的值会带来提升,但边际效应明显,精准测试不好做,因为提升还没有误差大,
现在的取值比较理想,毕竟这个不是真正的buffer,只是逻辑上的

@Fallen-Breath
Copy link
Collaborator

#39 (comment) 这里提的 COW_COPY_BUFFER_SIZE 的取值测试做一下,然后就 ok 了。之后合并了我再整理下细节吧

更大的值会带来提升,但边际效应明显,精准测试不好做,因为提升还没有误差大, 现在的取值比较理想,毕竟这个不是真正的buffer,只是逻辑上的

对于 #39 (comment) 中建议的测试,使用的 buffer 取值是更小的值,而非更大的值。毕竟这只是个 buffer,调整成 buffer 常见的大小可能会有不一样的性能表现

@tksmly
Copy link
Contributor Author

tksmly commented May 10, 2023

毕竟这只是个 buffer,调整成 buffer 常见的大小可能会有不一样的性能表现

这不是buffer,这是减少python层循环次数的buffer-like
因为c循环不会构成瓶颈,但py循环会


使用大量杂乱文件与其压缩包进行测试,

4k约为5s
16k约为2s-3s
1g约为1s-2s

边际效应非常明显,但是增加这个值没有任何坏处,在不出错的情况下应当尽可能提升这个值

@Fallen-Breath
Copy link
Collaborator

这不是buffer,这是减少python层循环次数的buffer-like

在 cpython 的实现下,这个参数是会直接传递给系统调用的,因此这可能会对实际性能产生影响。虽然正如 #39 (comment) 所说,glibc 的实现是没问题的

因为c循环不会构成瓶颈,但py循环会

我不认为 1E3 以下数量级的 python 循环会产生任何瓶颈,除非有相关的性能分析证明

使用大量杂乱文件与其压缩包进行测试

能详细说明下吗,是分别测试,还是同时测试。没啥没问题的话,这样就 ok 了

@tksmly
Copy link
Contributor Author

tksmly commented May 10, 2023

能详细说明下吗,是分别测试,还是同时测试。

同时

我不认为 1E3 以下数量级的 python 循环会产生任何瓶颈

我的测试文件大概1E4, 1s确实不算瓶颈,但是总共也就几s欸

@Fallen-Breath
Copy link
Collaborator

Fallen-Breath commented May 10, 2023

“我不认为 1E3 以下数量级”,这里的 1E3 指的是,使用 #39 (comment) 中建议取值最小的 10MiB 的情况,循环拷贝可能较大文件 10GiB,需要 10GiB / 10MiB = 约 1E3 次 python 循环调用 os.copy_file_range。不知你说的 1E4 是什么情况

@tksmly
Copy link
Contributor Author

tksmly commented May 10, 2023

“我不认为 1E3 以下数量级”,这里的 1E3 指的是,使用 #39 (comment) 中建议取值最小的 10MiB 的情况,循环拷贝可能较大文件 10GiB,需要 10GiB / 10MiB = 约 1E3 次 python 循环调用 os.copy_file_range。不知你说的 1E4 是什么情况

文件数量

不要低估日用linux平台的config目录

@Fallen-Breath
Copy link
Collaborator

“我不认为 1E3 以下数量级”,这里的 1E3 指的是,使用 #39 (comment) 中建议取值最小的 10MiB 的情况,循环拷贝可能较大文件 10GiB,需要 10GiB / 10MiB = 约 1E3 次 python 循环调用 os.copy_file_range。不知你说的 1E4 是什么情况

文件数量

不要低估日用linux平台的config目录

这跟你在 #39 (comment) 说的是不是不太匹配?

“这不是buffer,这是减少python层循环次数的buffer-like。因为c循环不会构成瓶颈,但py循环会”

在谈论 buffer 作为上下文的时,循环次数应该指的是循环调用 os.copy_file_range 的次数。而“文件数量”,指的是调用 _cpcow 的次数,这与 os.copy_file_range 的循环次数导致的性能影响不太相关啊

@Fallen-Breath
Copy link
Collaborator

Fallen-Breath commented May 10, 2023

对于 #39 (comment) 中的 COW_COPY_BUFFER_SIZE 测试,正确的测试方式应该是使用不同的 buffer size 值,来拷贝文件大小不同的文件多次,文件大小应当覆盖常见的量级

对于测试结果,我已经反复多次提出该怎么客观严谨表述一份测试结果。#39 (comment) 中对结果的描述是非常含糊不清缺少信息的。


算了,估计你也不会好好测,就不浪费时间了,buffer 大小这部分影响也不大,就这样吧

@Fallen-Breath Fallen-Breath merged commit 8effa9c into TISUnion:master May 10, 2023
1 check passed
@tksmly
Copy link
Contributor Author

tksmly commented May 10, 2023

_cpcow 的次数,这与 os.copy_file_range 的循环次数导致的性能影响不太相关啊

不仅仅是大量小文件,还有整个打包的zip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants