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

Current fallocate usage is not compatible with NFS #624

Open
voidpointertonull opened this issue Jun 28, 2024 · 1 comment
Open

Current fallocate usage is not compatible with NFS #624

voidpointertonull opened this issue Jun 28, 2024 · 1 comment

Comments

@voidpointertonull
Copy link

Chasing some fragmentation issues on a setup with HDD, I was surprised to see rsync having an opt-in --preallocate option without trying to opportunistically use fallocate by default.

Couldn't enjoy the expected upside though as I was just getting "Operation not supported (95)" messages.
Looked at the requirements of fallocate, and it seemed to be all right with using NFS 4.2 exposing a filesystem supporting fallocate, even the "fallocate" utility works, so went deeper to see what's wrong.

strace rsync:
fallocate(1, FALLOC_FL_KEEP_SIZE, 0, 8388607) = -1 EOPNOTSUPP (Operation not supported)

strace fallocate:
fallocate(3, 0, 0, 8388608) = 0

Apparently rsync uses FALLOC_FL_KEEP_SIZE, but NFS imposes some restrictions on the mode, not allowing that on its own:

	if ((mode != 0) && (mode != (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE)))
		return -EOPNOTSUPP;

The tricky part I can think of is the case of dealing with differential changes where rsync would surely not want to just punch a huge hole, destroying already existing data. However currently with freshly created files, there's a huge missed opportunity for reducing fragmentation.

Also, just mildly related, but I noticed the strace outputs differing in size, caused by this piece of code:

	if (length & 1) /* make the length not match the desired length */
		length++;
	else
		length--;

What kind of magic ritual is this? Wouldn't be surprised if this made sense on some exotic setup, but neither the comment, nor the almost 8 years old commit message explains the significance.
Ideally this should have been made conditional to be done only where it helps, but at least I believe that it should be swept away from the optimal path of using Linux with a well-behaving fallocate.

@HanabishiRecca
Copy link

Yeah, it's unclear why FALLOC_FL_KEEP_SIZE flag is set for do_fallocate.
It seems to work totally fine without it.

--- a/syscall.c
+++ b/syscall.c
@@ -592,7 +592,7 @@ int do_utime(const char *path, STRUCT_STAT *stp)
 
 #ifdef SUPPORT_PREALLOCATION
 #ifdef FALLOC_FL_KEEP_SIZE
-#define DO_FALLOC_OPTIONS FALLOC_FL_KEEP_SIZE
+#define DO_FALLOC_OPTIONS 0
 #else
 #define DO_FALLOC_OPTIONS 0
 #endif

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

No branches or pull requests

2 participants