-
Notifications
You must be signed in to change notification settings - Fork 56
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
Allow tmpfile compression to be disabled #110
base: master
Are you sure you want to change the base?
Allow tmpfile compression to be disabled #110
Conversation
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'm not sure if this should be upstreamed at all. Is there any reason why you'd not want compression of data processed by bees besides working around a kernel bug that should be in the distribution in the first place? The only reasons I can think of would make you force compression off globally anyways.
@@ -779,6 +783,7 @@ bees_main(int argc, char *argv[]) | |||
{ "strip-paths", no_argument, NULL, 'P' }, | |||
{ "no-timestamps", no_argument, NULL, 'T' }, | |||
{ "workaround-btrfs-send", no_argument, NULL, 'a' }, | |||
{ "no-tmpfile-compression", no_argument, NULL, 'N' }, |
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.
Indentation isn't correct
if(!bees_tmpfile_compression_disabled) { | ||
BEESTRACE("Getting FS_COMPR_FL on m_fd " << name_fd(m_fd)); | ||
int flags = ioctl_iflags_get(m_fd); | ||
flags |= FS_COMPR_FL; | ||
BEESTRACE("Setting FS_COMPR_FL on m_fd " << name_fd(m_fd) << " flags " << to_hex(flags)); | ||
ioctl_iflags_set(m_fd, flags); | ||
} |
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.
This logic is wrong as it won't prevent compression if the parent directory of the tmpfile has the compression attribute set. This would leave you with compression still active even when you said no-tmpfile-compression
on the cmdline. That would be quite a surprising effect. Actually, I think it's better to get the updated kernel or force compression off for the whole filesystem if this kernel issue is a concern.
@@ -56,6 +57,7 @@ do_cmd_help(char *argv[]) | |||
"\n" | |||
"Workarounds:\n" | |||
" -a, --workaround-btrfs-send Workaround for btrfs send\n" | |||
" -N, --no-tmpfile-compression Disable compression for temporary files\n" |
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 the naming could be better: What it actually wants to do is decompressing shared extents. Nobody cares if this is done through tmpfiles or other means. Options should try not exposing too many details of the internal workings so they can still be true after code redesigns.
For kernels without #107 applied