Bug 223440 - world needs review and adjustment for the disabling of fallocate for zfs (-r325320), lld included
Summary: world needs review and adjustment for the disabling of fallocate for zfs (-r3...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: misc (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Ed Maste
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-11-05 00:05 UTC by Mark Millard
Modified: 2018-12-18 14:27 UTC (History)
1 user (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Millard 2017-11-05 00:05:00 UTC 
-r325320 breaks lld when referring zfs file systems:
lld needs to be updated. See:

https://lists.freebsd.org/pipermail/freebsd-current/2017-November/067432.html

for Ed Maste's patch.( Ed is probably acting
independent of this submittal. Systems that
use or require lld can be a problem to update
for a range on or after -r325320. aarch64
in particular could be problematical for
self hosting.)

Note that configure-testing of a specific live
file system is probably not a good idea since
a mix of file systems is allowed to be in use
at the same time overall. (lld has a run-time
test that is adjusted.)

It appears that sqlite3 might use fallocate.
And heimdal (via its sqlite3.c). I've not
reviewed the details for any of the below
that do not trace to the lld issue:

# grep -rl "fallocate" /usr/src/* | more
/usr/src/contrib/sqlite3/configure.ac
/usr/src/contrib/sqlite3/configure
/usr/src/contrib/sqlite3/sqlite3.c
/usr/src/contrib/netbsd-tests/lib/libc/sys/t_posix_fallocate.c
/usr/src/contrib/compiler-rt/lib/dfsan/libc_ubuntu1404_abilist.txt
/usr/src/contrib/compiler-rt/include/sanitizer/linux_syscall_hooks.h
/usr/src/contrib/llvm/lib/Support/Unix/Path.inc
/usr/src/contrib/pjdfstest/configure.ac
/usr/src/contrib/pjdfstest/tests/misc.sh
/usr/src/contrib/pjdfstest/pjdfstest.c
/usr/src/contrib/openbsm/NEWS
/usr/src/contrib/openbsm/etc/audit_event
/usr/src/crypto/heimdal/lib/sqlite/sqlite3.c
/usr/src/lib/libc/sys/posix_fallocate.2
/usr/src/lib/libc/sys/Symbol.map
/usr/src/lib/libc/sys/Makefile.inc
/usr/src/lib/libc/tests/sys/Makefile
/usr/src/lib/clang/include/llvm/Config/config.h
/usr/src/share/man/man9/VOP_ALLOCATE.9
/usr/src/sys/compat/freebsd32/syscalls.master
/usr/src/sys/compat/freebsd32/freebsd32_syscalls.c
/usr/src/sys/compat/freebsd32/freebsd32_sysent.c
/usr/src/sys/compat/freebsd32/capabilities.conf
/usr/src/sys/compat/freebsd32/freebsd32_misc.c
/usr/src/sys/compat/freebsd32/freebsd32_proto.h
/usr/src/sys/compat/freebsd32/freebsd32_syscall.h
/usr/src/sys/compat/freebsd32/freebsd32_systrace_args.c
/usr/src/sys/compat/cloudabi/cloudabi_file.c
/usr/src/sys/compat/linux/linux_file.c
/usr/src/sys/i386/linux/linux_sysent.c
/usr/src/sys/i386/linux/linux_proto.h
/usr/src/sys/i386/linux/linux_syscalls.c
/usr/src/sys/i386/linux/linux_systrace_args.c
/usr/src/sys/i386/linux/linux_syscall.h
/usr/src/sys/i386/linux/syscalls.master
/usr/src/sys/sys/fcntl.h
/usr/src/sys/sys/syscall.h
/usr/src/sys/sys/syscall.mk
/usr/src/sys/sys/syscallsubr.h
/usr/src/sys/sys/sysproto.h
/usr/src/sys/kern/init_sysent.c
/usr/src/sys/kern/syscalls.master
/usr/src/sys/kern/systrace_args.c
/usr/src/sys/kern/capabilities.conf
/usr/src/sys/kern/vfs_syscalls.c
/usr/src/sys/kern/syscalls.c
/usr/src/sys/amd64/linux32/linux32_syscall.h
/usr/src/sys/amd64/linux32/linux32_systrace_args.c
/usr/src/sys/amd64/linux32/syscalls.master
/usr/src/sys/amd64/linux32/linux32_syscalls.c
/usr/src/sys/amd64/linux32/linux32_sysent.c
/usr/src/sys/amd64/linux32/linux32_proto.h
/usr/src/sys/amd64/linux/linux_proto.h
/usr/src/sys/amd64/linux/syscalls.master
/usr/src/sys/amd64/linux/linux_syscalls.c
/usr/src/sys/amd64/linux/linux_systrace_args.c
/usr/src/sys/amd64/linux/linux_syscall.h
/usr/src/sys/amd64/linux/linux_sysent.c
/usr/src/tests/sys/pjdfstest/config.h
Comment 1 commit-hook freebsd_committer freebsd_triage 2017-11-05 00:52:27 UTC 
A commit references this bug:

Author: emaste
Date: Sun Nov  5 00:51:53 UTC 2017
New revision: 325420
URL: https://svnweb.freebsd.org/changeset/base/325420

Log:
  lld: accept EINVAL to indicate posix_fallocate is unsupported

  As of r325320 posix_fallocate on a ZFS filesystem returns EINVAL to
  indicate that the operation is not supported. (I think this is a strange
  choice of errno on the part of POSIX.)

  PR:		223383, 223440
  Reported by:	Mark Millard
  Tested by:	Mark Millard
  MFC after:	3 days
  Sponsored by:	The FreeBSD Foundation

Changes:
  head/contrib/llvm/lib/Support/Unix/Path.inc
Comment 2 Ed Maste freebsd_committer freebsd_triage 2017-11-05 19:45:47 UTC 
I have addressed one of the cases Mark identified (in lld), but the remainder still need to be reviewed.
Comment 3 Ed Maste freebsd_committer freebsd_triage 2017-11-05 21:23:37 UTC 
> contrib/sqlite3/sqlite3.c
> crypto/heimdal/lib/sqlite/sqlite3.c

AFAICT this is the only issue that needs more effort / investigation, as it appears that it does not correctly handle EINVAL or EOPNOTSUPP:

#if defined(HAVE_POSIX_FALLOCATE) && HAVE_POSIX_FALLOCATE
      /* The code below is handling the return value of osFallocate() 
      ** correctly. posix_fallocate() is defined to "returns zero on success, 
      ** or an error number on  failure". See the manpage for details. */
      int err;
      do{
        err = osFallocate(pFile->h, buf.st_size, nSize-buf.st_size);
      }while( err==EINTR );
      if( err ) return SQLITE_IOERR_WRITE;
#else

Other source files with posix_fallocate / fallocate do not have an issue:

> contrib/netbsd-tests/lib/libc/sys/t_posix_fallocate.c

It's only testing that posix_fallocate doesn't change errno, and that EBADF is returned for a bad fd (-1), so no issue here.

> contrib/llvm/lib/Support/Unix/Path.inc

Fixed by r325420

> contrib/pjdfstest/pjdfstest.c

Appears to be test harness only; I did not find a test invoking posix_fallocate or fallocate.
Comment 4 commit-hook freebsd_committer freebsd_triage 2017-11-08 00:43:09 UTC 
A commit references this bug:

Author: emaste
Date: Wed Nov  8 00:39:04 UTC 2017
New revision: 325523
URL: https://svnweb.freebsd.org/changeset/base/325523

Log:
  MFC r325420: lld: accept EINVAL to indicate posix_fallocate is unsupported

  As of r325320 posix_fallocate on a ZFS filesystem returns EINVAL to
  indicate that the operation is not supported. (I think this is a strange
  choice of errno on the part of POSIX.)

  PR:		223383, 223440
  Reported by:	Mark Millard
  Sponsored by:	The FreeBSD Foundation

Changes:
_U  stable/11/
  stable/11/contrib/llvm/lib/Support/Unix/Path.inc
Comment 5 commit-hook freebsd_committer freebsd_triage 2017-11-12 09:10:10 UTC 
A commit references this bug:

Author: brooks
Date: Sun Nov 12 09:09:35 UTC 2017
New revision: 454025
URL: https://svnweb.freebsd.org/changeset/ports/454025

Log:
  Merge from src:

  lld: accept EINVAL to indicate posix_fallocate is unsupported

  As of r325320 posix_fallocate on a ZFS filesystem returns EINVAL to
  indicate that the operation is not supported. (I think this is a strange
  choice of errno on the part of POSIX.)

  PR:		223383, 223440
  Reported by:	Mark Millard

Changes:
  head/devel/llvm50/Makefile
  head/devel/llvm50/files/patch-lib_Support_Unix_Path.inc
Comment 6 commit-hook freebsd_committer freebsd_triage 2017-11-13 01:45:54 UTC 
A commit references this bug:

Author: brooks
Date: Mon Nov 13 01:45:13 UTC 2017
New revision: 454093
URL: https://svnweb.freebsd.org/changeset/ports/454093

Log:
  Merge from src and upstream LLVM:

  lld: accept EINVAL to indicate posix_fallocate is unsupported

  As of r325320 posix_fallocate on a ZFS filesystem returns EINVAL to
  indicate that the operation is not supported. (I think this is a strange
  choice of errno on the part of POSIX.)

  PR:		223383, 223440
  Reported by:	Mark Millard

Changes:
  head/devel/llvm40/Makefile
  head/devel/llvm40/files/patch-lib_Support_Unix_Path.inc
Comment 7 commit-hook freebsd_committer freebsd_triage 2017-11-13 02:48:48 UTC 
A commit references this bug:

Author: brooks
Date: Mon Nov 13 02:47:52 UTC 2017
New revision: 454097
URL: https://svnweb.freebsd.org/changeset/ports/454097

Log:
  Update to a new snapshot.

  This includes the fix for posix_fallocate not being supported on ZFS.

  PR:		223383, 223440
  Reported by:	Mark Millard

Changes:
  head/devel/llvm-devel/Makefile
  head/devel/llvm-devel/Makefile.snapshot
  head/devel/llvm-devel/distinfo
  head/devel/llvm-devel/pkg-plist
Comment 8 commit-hook freebsd_committer freebsd_triage 2017-11-16 21:45:26 UTC 
A commit references this bug:

Author: brooks
Date: Thu Nov 16 21:44:19 UTC 2017
New revision: 454342
URL: https://svnweb.freebsd.org/changeset/ports/454342

Log:
  Update to a new snapshot and apply the patch for posix_fallocate()
  support being remove for ZFS

  PR:		223383, 223440
  Sponsored by:	DARPA, AFRL

Changes:
  head/devel/llvm-cheri/Makefile
  head/devel/llvm-cheri/Makefile.snapshot
  head/devel/llvm-cheri/distinfo
  head/devel/llvm-cheri/files/gen-Makefile.snapshot.sh
  head/devel/llvm-cheri/files/patch-lib_Support_Unix_Path.inc
  head/devel/llvm-cheri/pkg-plist
Comment 9 Eitan Adler freebsd_committer freebsd_triage 2018-05-20 23:54:14 UTC 
For bugs matching the following conditions:
- Status == In Progress
- Assignee == "bugs@FreeBSD.org"
- Last Modified Year <= 2017

Do
- Set Status to "Open"
Comment 10 Ed Maste freebsd_committer freebsd_triage 2018-06-01 15:45:02 UTC 
I believe the sqlite issue is a bug.  For cases where posix_fallocate is supported by the operating system but returns EINVAL indicating that the filesystem does not support the operation we should just treat the call as advisory, and carry on.

-      if( err ) return SQLITE_IOERR_WRITE;
+      if( err!=EINVAL ) return SQLITE_IOERR_WRITE;
Comment 11 Ed Maste freebsd_committer freebsd_triage 2018-06-01 15:50:12 UTC 
Change in my WIP branch: https://github.com/emaste/freebsd/commit/77572f574f8c4aa2d9f4eaee
Comment 12 Ed Maste freebsd_committer freebsd_triage 2018-07-24 14:36:18 UTC 
(In reply to Ed Maste from comment #11)
Twitter exchange with SQLite's author here: https://twitter.com/DRichardHipp/status/1002919969510821888
Comment 13 Ed Maste freebsd_committer freebsd_triage 2018-12-18 14:26:07 UTC 
cy@ addressed sqlite issue in r342183 (sqlite3 update)