This is an archive of the discontinued LLVM Phabricator instance.

[Support] Revert posix_fallocate in resize_file
ClosedPublic

Authored by MaskRay on Dec 17 2021, 10:56 AM.

Details

Summary

This reverts 3816c53f040cc6aa06425978dd504b0bd5b7899c and removes follow-up
fixups.

The original intention was to show error earlier (posix_fallocate time) than
later for ld.lld but it appears to cause some problems which make it not free.

  • FreeBSD ZFS: EINVAL, not too bad.
  • FreeBSD UFS: according to khng "devastatingly slow on freebsd because UFS on freebsd does not have preallocation support like illumos. It zero-fills."
  • NetBSD: maybe EOPNOTSUPP
  • Linux tmpfs: unless tmpfs is set up to use huge pages (requires CONFIG_TRANSPARENT_HUGE_PAGECACHE=y), I can consistently demonstrate ~300ms delay for a 1.4GiB output.
  • Linux ext4: I don't measure any benefit, either backed by a hard disk or by a file in tmpfs.
  • The current code organization of defined(HAVE_POSIX_FALLOCATE) costs us a macro dispatch for AIX.

I think we should just remove it. I think if posix_fallocate ever finds demonstrable benefit,
it is likely Linux specific and will not need HAVE_POSIX_FALLOCATE, and possibly opt-in by some specific programs.

In a filesystem with CoW and compression, the ENOSPC benefit may be lost as well.

Diff Detail

Event Timeline

MaskRay created this revision.Dec 17 2021, 10:56 AM
MaskRay requested review of this revision.Dec 17 2021, 10:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2021, 10:56 AM
MaskRay edited the summary of this revision. (Show Details)Dec 17 2021, 10:57 AM
MaskRay edited the summary of this revision. (Show Details)Dec 17 2021, 11:35 AM
MaskRay added reviewers: khng300, joerg, mgorny.

it is likely Linux specific

Using it to prevent disk-full would equally apply to FreeBSD and other operating systems as Linux, on non-CoW filesystems.

Anyhow, the original motivation was early detection of disk-full not performance. I wouldn't really expect a performance benefit. I don't object to removing use of posix_fallocate() but as it wasn't intended as a performance improvement IMO removing it is not justified by lack of performance benefit.

it is likely Linux specific

Using it to prevent disk-full would equally apply to FreeBSD and other operating systems as Linux, on non-CoW filesystems.

Anyhow, the original motivation was early detection of disk-full not performance. I wouldn't really expect a performance benefit. I don't object to removing use of posix_fallocate() but as it wasn't intended as a performance improvement IMO removing it is not justified by lack of performance benefit.

It greatly harms performance on Linux tmpfs, 0.3s in my 5.2s link.

joerg added a comment.Dec 17 2021, 3:46 PM

I agree that I don't think it generally helps all that much. Under memory pressure it can even result in additional writes.

khng300 accepted this revision.Dec 19 2021, 9:33 PM
This revision is now accepted and ready to land.Dec 19 2021, 9:33 PM

My only suggestion is in future it could be platform-specific, like on macOS with fcntl(F_PREALLOCATE).

Thanks! I'll wait few days and push. If posix_fallocate is to be restored, I believe it should be passed explicitly as a flag, and there should be higher justification for lld/ELF.
I've noted this thing in https://maskray.me/blog/2021-12-19-why-isnt-ld.lld-faster#open-output-file

It greatly harms performance on Linux tmpfs, 0.3s in my 5.2s link.

Indeed, and I'm fine with it being removed.

It greatly harms performance on Linux tmpfs, 0.3s in my 5.2s link.

Indeed, and I'm fine with it being removed.

OK. Thanks! Then I will land this soon.

MaskRay edited the summary of this revision. (Show Details)Dec 20 2021, 11:07 AM

Update description that posix_fallocate is more harmful on FreeBSD UFS: "devastatingly slow on freebsd because UFS on freebsd does not have preallocation support like illumos. It zero-fills."

MaskRay edited the summary of this revision. (Show Details)Dec 20 2021, 11:11 AM
MaskRay edited the summary of this revision. (Show Details)Dec 20 2021, 11:15 AM
This revision was landed with ongoing or failed builds.Dec 20 2021, 11:16 AM
This revision was automatically updated to reflect the committed changes.