-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
only FALLOC_FL_PUNCH_HOLE when ftruncate is buggy #2102
Conversation
env/io_posix.cc
Outdated
// After ftruncate, we check whether ftruncate has the correct behavior. | ||
// If not, we should hack it with FALLOC_FL_PUNCH_HOLE | ||
if ((file_stats.st_size == 0 && file_stats.st_blocks != 0) || | ||
(file_stats.st_size - 1) / file_stats.st_blksize + 1 != |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about:
(file_stats.st_size + file_stats.st_blksize - 1) / file_stats.st_blksize
env/io_posix.cc
Outdated
(file_stats.st_size - 1) / file_stats.st_blksize + 1 != | ||
file_stats.st_blocks / (file_stats.st_blksize / 512)) { | ||
fprintf(stderr, | ||
"Your kernel is buggy and does not free preallocated" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think mentioning which kernel version could be buggy in the message will be useful.
@lightmark updated the pull request - view changes |
@lightmark updated the pull request - view changes |
@lightmark has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
file_stats.st_blksize != | ||
file_stats.st_blocks / (file_stats.st_blksize / 512)) { | ||
fprintf(stderr, | ||
"Your kernel is buggy (<= 4.0.x) and does not free preallocated" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We won't be 100% sure it is <= 4.0.x, so I suggest we avoid this assertion. I also think you need to mention this is Linux kernel version, although ROCKSDB_FALLOCATE_PRESENT is only likely to set under Linux.
Summary: In RocksDB, we sometimes preallocate the estimated space for a file to have better perf with fallocate (if supported). Usually it is a little bit bigger than the real resulting file size. At this time, we have to let the Filesystem reclaim the space not used. Ideally, calling ftruncate to truncate the file to its real size should be enough. HOWEVER, it isn't on tmpfs, which we witness in our case, with some buggy kernel version. ftruncate a file with preallocated space doesn't change number of the blocks used by the file, which means the space not used by the file is not returned to the filesystems. So in this case we need fallocate with FALLOC_FL_PUNCH_HOLE to explicitly reclaim the used blocks. It is a hack to cope with the kernel bug and usually we should not need it. Closes facebook#2102 Differential Revision: D4848934 Pulled By: lightmark fbshipit-source-id: f1b40b5
In RocksDB, we sometimes preallocate the estimated space for a file to have better perf with fallocate (if supported). Usually it is a little bit bigger than the real resulting file size. At this time, we have to let the Filesystem reclaim the space not used.
Ideally, calling ftruncate to truncate the file to its real size should be enough. HOWEVER, it isn't on tmpfs, which we witness in our case, with some buggy kernel version. ftruncate a file with preallocated space doesn't change number of the blocks used by the file, which means the space not used by the file is not returned to the filesystems. So in this case we need fallocate with FALLOC_FL_PUNCH_HOLE to explicitly reclaim the used blocks. It is a hack to cope with the kernel bug and usually we should not need it.