cmake: detect availability of posix_fallocate(3) by pks-t · Pull Request #5087 · libgit2/libgit2

pks-t · 2019-05-24T11:53:00Z

Instead of having fragile checks for platform definitions to
guess whether posix_fallocate(3) is available or not, let's just
use CMake's CHECK_FUNCTION_EXISTS to detect that. This is both
more accurate and simpler.

tiennou

LGTM, and is clearly a better solution. Thanks for fixing this @pks-t.

(I do hope there's no such problem as the qsort/_r/_s conundrum, but I don't recall inconsistent signatures)

pks-t · 2019-05-24T16:36:08Z

On Fri, May 24, 2019 at 07:01:26AM -0700, Etienne Samson wrote: I do hope there's no such problem as the `qsort/_r/_s` conundrum, but I don't recall inconsistent signatures)

Well, I'd definitely hope that if a function already has a `posix_` prefix, that implementers will in fact implement what POSIX specifies. But you never know ;)

ethomson · 2019-05-25T08:29:07Z

Well, I'd definitely hope that if a function already has a
posix_ prefix, that implementers will in fact implement what
POSIX specifies. But you never know ;)

Indeed. The GCC builds are complaining about something, though I don't know what that is.

pks-t · 2019-05-25T10:45:45Z

On Sat, May 25, 2019 at 01:29:16AM -0700, Edward Thomson wrote: Indeed. The GCC builds are complaining about something, though I don't know what that is.

Huh. For whatever reason, those systems do not detect `posix_fallocate`, even though they should definitely have it available: 2019-05-24T11:54:32.6862203Z -- Looking for posix_fallocate 2019-05-24T11:54:32.7131435Z -- Looking for posix_fallocate - not found Will need to investigate why that is.

tiennou · 2019-05-25T13:59:50Z

I've pushed a fixup here. I've ended up doing a check for the posix_fallocate symbol, and if that fails, it'll check if we're on some kind of BSD.

This test is "looser" that the original NetBSD preprocessor checks provided by @jacquesg, but if I understood correctly, NetBSD gained posix_fallocate in v7, so doing the generic check first should handle all BSD (modulo Apple-specific modifications).

(edit I've also made more explicit that it's a BSD-specific implementation)

jacquesg · 2019-05-25T14:46:14Z

FreeBSD has had posix_fallocate for many years, wouldn't worry about it.

jacquesg · 2019-05-25T14:47:22Z

I would also check for it on Solaris, as I believe it is only available from 11 onwards. Version 10 is still in wide-spread use.

tiennou · 2019-05-25T16:42:20Z

Thanks for the insight. I haven't been able to find a reference about what was supported on Solaris then. If it's the fcntl stuff as the BSDs need it's okay, but if it's not then (I'd really like to not have to implement a "stupid" write zeroes fallback)…

jacquesg · 2019-05-26T08:19:38Z

Here is some guidance on how to implement it for Solaris: https://fossies.org/linux/dpkg/lib/dpkg/fdio.c

jacquesg · 2019-05-26T08:26:07Z

Ok, so it doesn't seem like any version of NetBSD or OpenBSD has F_PREALLOCATE. So in summary:

FreeBSD: Has posix_fallocate since version 7 (added about 11 years ago)
OpenBSD: Doesn't have posix_fallocate or F_PREALLOCATE
NetBSD: Has posix_fallocate from version 7, doesn't have F_PREALLOCATE
macOS: Has F_PREALLOCATE
Solaris: Has F_ALLOCSP64.

krytarowski · 2019-05-26T13:00:56Z

This code is still not fully correct.

On NetBSD posix_fallocate(2) will report EOPNOTSUPP for frequently used filesystems (we need kernel support for each filesystem) and must have a fallback for the ftruncate(2) function.

krytarowski · 2019-05-26T13:02:38Z

Example: https://github.com/llvm-mirror/llvm/blob/90a7bd7e82703a62ec7b54a817ca6f4fde080e8d/lib/Support/Unix/Path.inc#L505

tiennou · 2019-05-27T23:14:04Z

Okay, I've updated my fixup commit to CMake checks all of those, choose the one available, and fallback to ftruncate on ENOTSUP/nothing being available.

ethomson · 2019-06-11T12:57:47Z

/rebuild

libgit2-azure-pipelines · 2019-06-11T12:58:02Z

Sorry @ethomson, I was not able to find any builds to requeue.

ethomson · 2019-06-11T13:35:02Z

Gah, the build expired. I'll have to figure out how to remove the build expirations. In the meantime, I queued a new build against this branch manually, and it's still failing, unfortunately.

tiennou · 2019-06-12T11:33:18Z

@pks-t Do you mind if I take over this one ? I'm responsible for the half-working platform support, have a tentative fix handy which I forgot to point out here (there should be a lone commit I've added to my fork). As you wish, but since it impacts platforms not quite under CI, it feel best to fix it "fast" (maybe) ?

pks-t · 2019-06-12T20:14:35Z

On Wed, Jun 12, 2019 at 04:33:28AM -0700, Etienne Samson wrote: @pks-t Do you mind if I take over this one ? I'm responsible for the half-working platform support, have a tentative fix handy which I forgot to point out here (there should be a [lone commit](master...tiennou:pks/cmake-posix-fallocate) I've added to my fork). As you wish, but since it impacts platforms not quite under CI, it feel best to fix it "fast" (maybe) ?

Nope, I'm fine with that. Sorry for not investing more time here, and thanks for taking over!

pks-t · 2019-06-13T14:44:51Z

Updated with @tiennou's changes

tiennou · 2019-06-13T15:50:55Z

Okay, so the Linux test failed because I have inconsistent semantics, I believe because of the ftruncate fallback I've added without thinking.

My expectation of whatever POSIX describes as fallocate, is that a call to p_fallocate(fd, 0, 1024); cannot be assumed to mean that I want the file backed by fd to be truncated at 1024, only that I expect that part of the file to exist (else, what would happen with multiple calls, in case I wanted to zero parts of a file for some subsequent mmaps). So I've reimplemented the fallback as a dumb write loop (@pks-t if you can pull that, that'd be 🙏). Does anyone sees something wrong with that understanding ?

ethomson · 2019-06-13T16:00:08Z

Okay, so the Linux test failed because I have inconsistent semantics, I believe because of the ftruncate fallback I've added without thinking.

Hmm, what version of Linux are we building on that's missing posix_fallocate and is thus falling into the ftruncate fallback?

ethomson · 2019-06-13T16:04:08Z

In any case, yes, I agree that we need to avoid shrinking a file with ftruncate, but it should be much faster to just do arithmetic to determine if we would be shrinking or not, and only calling ftruncate opportunistically (or doing nothing). Is there something wrong with the code that was there from bd13204#diff-af200ec3b10b961c568b86610775effe ?

tiennou · 2019-06-13T18:33:50Z

Hmm, what version of Linux are we building on that's missing posix_fallocate and is thus falling into the ftruncate fallback?

Dunno, but I'd wager it's also filesystem-specific, and we're in crazy vm shenanigans territory, so that could have been the fallback.

In any case, yes, I agree that we need to avoid shrinking a file with ftruncate, but it should be much faster to just do arithmetic to determine if we would be shrinking or not, and only calling ftruncate opportunistically (or doing nothing). Is there something wrong with the code that was there from bd13204#diff-af200ec3b10b961c568b86610775effe ?

Not per se, I could have just added a check to make sure we'd not cause the file to shrink. Would that work better ? It's just that I'm expecting I'll have to juggle offsets, lengths and seek positions to convert "fallocate hole" into "ftruncate complete file but don't make it smaller", and the dumb loop seemed safer…

Instead of having fragile checks for platform definitions to guess whether posix_fallocate(3) is available or not, let's just use CMake's `CHECK_FUNCTION_EXISTS` to detect that. This is both more accurate and simpler.

tiennou · 2019-06-14T07:46:18Z

@pks-t branch updated again (Xcode's renamer doesn't look inside #ifdefs). Do you have an opinion on the dump-write-loop vs. ftruncate-unless-it-would-shrink approach ?

pks-t

So... this seems mighty complex. And the funny thing is: we don't even use p_fallocate anywhere except for in our tests for the bigfile emulation. Is it really worth the effort? Shouldn't we just remove p_fallocate altogether and create a binary blob that explodes to a huge size upon uncompressing it?

pks-t · 2019-06-14T08:14:09Z

+	goto fallback;
 #endif
+
+fallback:


I don't quite get it. Aren't we now always executing the fallback, regardless of whether the platform-specific implementation succeeded or not?

Heh, true, I seem to have forgot a few returns when whatever platform-specific version succeeded. IOW, only failures of the (potential) platform fallocate would cause the fallback to run.

pks-t · 2019-06-14T08:15:56Z

+#elif defined(GIT_USE_FALLOCATE)
+	do {
+		error = fallocate(fd, 0, offset, len);
+	} while (error < 0 && errno == EINTR);


Can fallocate set EAGAIN?

That's copy-pasted from #5087 (comment), actually. And isn't EAGAIN usually async network stuff ? I'm scared now…

pks-t · 2019-06-14T08:16:45Z

+		goto fallback;
 #else
-	return posix_fallocate(fd, offset, len);
+	goto fallback;


I'd just remove the else block, but that's a matter of taste

It's protecting against an "unused label" warning on macOS case, actually.

tiennou

So... this seems mighty complex.

God yes. Removing the fallback uncovered another "issue": macOS needs the fallback, because somehow after the call, its fstat size is still 0 (I've added a comment). So if there's any chance to drop that madness, I'm all for it now 😭.

tiennou · 2019-06-14T09:24:39Z

+	goto fallback;
 #endif
+
+fallback:


Heh, true, I seem to have forgot a few returns when whatever platform-specific version succeeded. IOW, only failures of the (potential) platform fallocate would cause the fallback to run.

tiennou · 2019-06-14T09:25:30Z

+		goto fallback;
 #else
-	return posix_fallocate(fd, offset, len);
+	goto fallback;


It's protecting against an "unused label" warning on macOS case, actually.

tiennou · 2019-06-14T09:34:39Z

+#elif defined(GIT_USE_FALLOCATE)
+	do {
+		error = fallocate(fd, 0, offset, len);
+	} while (error < 0 && errno == EINTR);


That's copy-pasted from #5087 (comment), actually. And isn't EAGAIN usually async network stuff ? I'm scared now…

pks-t · 2019-06-14T09:56:59Z

So if there's any chance to drop that madness, I'm all for it now

Yeah, seeing how this escalated quickly I'm all for dropping p_fallocate completely and using some other sorcery for the bigfile test.

ethomson · 2019-06-23T19:33:03Z

Yeah, seeing how this escalated quickly I'm all for dropping p_fallocate completely and using some other sorcery for the bigfile test.

+1

pks-t · 2019-06-24T12:35:04Z

Closing, this has been fixed via #5114 by removing p_fallocate altogether.

tiennou approved these changes May 24, 2019

View reviewed changes

pks-t force-pushed the pks/cmake-posix-fallocate branch from 464d830 to 908e11f Compare June 13, 2019 14:44

pks-t force-pushed the pks/cmake-posix-fallocate branch from 908e11f to 4f717d6 Compare June 13, 2019 15:53

cmake: detect availability of posix_fallocate(3)

efb2733

Instead of having fragile checks for platform definitions to guess whether posix_fallocate(3) is available or not, let's just use CMake's `CHECK_FUNCTION_EXISTS` to detect that. This is both more accurate and simpler.

pks-t force-pushed the pks/cmake-posix-fallocate branch from 4f717d6 to efb2733 Compare June 14, 2019 08:09

pks-t commented Jun 14, 2019

View reviewed changes

tiennou reviewed Jun 14, 2019

View reviewed changes

pks-t mentioned this pull request Jun 14, 2019

Removal of p_fallocate #5114

Merged

pks-t closed this Jun 24, 2019

pks-t deleted the pks/cmake-posix-fallocate branch June 24, 2019 12:35

Uh oh!

Conversation

pks-t commented May 24, 2019

Uh oh!

tiennou left a comment

Choose a reason for hiding this comment

Uh oh!

pks-t commented May 24, 2019 via email

Uh oh!

ethomson commented May 25, 2019

Uh oh!

pks-t commented May 25, 2019 via email

Uh oh!

tiennou commented May 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacquesg commented May 25, 2019

Uh oh!

jacquesg commented May 25, 2019

Uh oh!

tiennou commented May 25, 2019

Uh oh!

jacquesg commented May 26, 2019

Uh oh!

jacquesg commented May 26, 2019

Uh oh!

krytarowski commented May 26, 2019

Uh oh!

krytarowski commented May 26, 2019

Uh oh!

tiennou commented May 27, 2019

Uh oh!

ethomson commented Jun 11, 2019

Uh oh!

libgit2-azure-pipelines Bot commented Jun 11, 2019

Uh oh!

ethomson commented Jun 11, 2019

Uh oh!

tiennou commented Jun 12, 2019

Uh oh!

pks-t commented Jun 12, 2019 via email

Uh oh!

pks-t commented Jun 13, 2019

Uh oh!

tiennou commented Jun 13, 2019

Uh oh!

ethomson commented Jun 13, 2019

Uh oh!

ethomson commented Jun 13, 2019

Uh oh!

tiennou commented Jun 13, 2019

Uh oh!

tiennou commented Jun 14, 2019

Uh oh!

pks-t left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiennou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pks-t commented Jun 14, 2019

tiennou commented May 25, 2019 •

edited

Loading

pks-t left a comment •

edited

Loading