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

Allow tmpfile compression to be disabled #110

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

charles-dyfis-net
Copy link

For kernels without #107 applied

Copy link
Contributor

@kakra kakra left a 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' },
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation isn't correct

Comment on lines +518 to +524
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);
}
Copy link
Contributor

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"
Copy link
Contributor

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.

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