Page MenuHomeFreeBSD

Fix blksize selection in vn_generic_copy_file_range() for ZFS
ClosedPublic

Authored by rmacklem on Oct 21 2022, 12:58 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Jan 8, 4:24 PM
Unknown Object (File)
Sat, Jan 7, 4:18 PM
Unknown Object (File)
Dec 18 2022, 7:56 AM
Unknown Object (File)
Dec 16 2022, 5:34 PM
Subscribers

Details

Summary

Allan Jude proposed D36194 to fix vn_generic_copy_file_range(),
which only uses a 512byte blksize. Although his patch
fixes the blksize problem, I realized that the calculation
would appear to be "magic math" to code readers.

During simple benchmarking, using f_iosize appears to
perform as well or slightly better than 1Mbyte, so this
patch uses f_iosize for ZFS and usually for other file
systems.

The main difference with this patch is that it includes
rationale for the two cases, so hopefully the calculation
will make sense to source code readers/maintainers.

I did review D36194 and I do not care which patch gets committed.

Test Plan

Tested by doing "cp" of a fairly large sparse file between
ZFS, UFS and msdosfs file systems, while printing out the
selected blksize.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

sys/kern/vfs_vnops.c.new
3341 ↗(On Diff #112067)

Do you really need this special case? I would think that the "else" clause's value of MAX(in f_iosize, out f_iosize) would still be appropriate here.

3352 ↗(On Diff #112067)

s/512/f_iosize/

sys/kern/vfs_vnops.c.new
3341 ↗(On Diff #112067)

Well, yes and no. If there was a filesystem that reported
f_iosize == 131072 and hole_size == 32768 being used
as the output file (and it really did not allocate a block
for an unwritten blob of 32Kbytes that is 32Kbytes aligned)
the, yes, this special case is needed so that 32Kbytes is used.

Are there any such file systems right now?
I don't think so, but I'd like to leave the special case in
here, if only to retain the comment explaining why it is
a special case.

ZFS does this now, except the hole_size == 512 is bogus,
at least in the sense that it does allocate a block for an
unallocated blob of 512 bytes that is 512 byte aligned.

3352 ↗(On Diff #112067)

Thanks, I will fix this typo.

Ok, this LGTM if you fix the comment typo. And I think I like the reasoning here better than in the other PR.

This revision is now accepted and ready to land.Oct 25 2022, 2:57 PM

Reviewed By: allanjude

Just remember to fix the 512 -> f_iosize comment asomers pointed out.

This revision was automatically updated to reflect the committed changes.