common: posix_fallocate on ZFS returns EINVAL #20398

wjwithagen · 2018-02-11T21:09:14Z

But even still it would not work on any COW FS.
So reorganised the code to have a common routine
that in the end will allocate a file on disk if needed

FileStore would not build when there was no HAVE_POSIX_FALLOCATE
other than on Apple. With ceph_posix_fallocate FileStore will also
fallback to manually allocating the required file.

Signed-off-by: Willem Jan Withagen wjw@digiware.nl

tchaikov · 2018-02-12T07:01:06Z

src/common/ceph_posix_fallocate.c

+// On FreeBSD ZFS fallocate always fails since it is considered impossible to
+// reserve space on a COW filesystem. It returns EINVAL
+// Linux in this case already emulates it in glibc
+// In which case it is allocated manually, and still that is not a real guarantee


what is "it" in this line?

tchaikov · 2018-02-12T07:01:45Z

src/common/ceph_posix_fallocate.c

+#include <unistd.h>
+#include <errno.h>
+
+// On FreeBSD ZFS fallocate always fails since it is considered impossible to


add a comma after FreeBSD.

tchaikov · 2018-02-12T07:02:12Z

src/common/ceph_posix_fallocate.c

+
+// On FreeBSD ZFS fallocate always fails since it is considered impossible to
+// reserve space on a COW filesystem. It returns EINVAL
+// Linux in this case already emulates it in glibc


what is "it" in this line?

tchaikov · 2018-02-12T07:03:35Z

src/common/ceph_posix_fallocate.c

@@ -0,0 +1,65 @@
+


remove empty line.

tchaikov · 2018-02-12T07:06:12Z

src/common/ceph_posix_fallocate.c

+
+#ifdef HAVE_POSIX_FALLOCATE
+    ret = posix_fallocate(fd, offset, len);
+    ret = -ret;


Posix_fallocate returns errors in its return value. (0 is oke)
And as far as the original code goes these values were used negated.

tchaikov · 2018-02-12T07:06:23Z

src/common/ceph_posix_fallocate.c

+}
+
+int ceph_posix_fallocate(int fd, off_t offset, off_t len) {
+


remove empty line

tchaikov · 2018-02-12T07:06:56Z

src/common/ceph_posix_fallocate.c

+#if !defined(__FREEBSD__)
+    return ret;
+#else
+    if ( ret != !EINVAL ) {


i don't think this is what you intended to put.

tchaikov · 2018-02-12T07:09:09Z

src/CMakeLists.txt

@@ -492,6 +492,7 @@ set(libcommon_files
  common/HeartbeatMap.cc
  common/PluginRegistry.cc
  common/ceph_fs.cc
+  common/ceph_posix_fallocate.c


why not add this implementation to compat.cc, and declare the func in compat.h?

That would be another option, but reason to take this route was that this code was sort of replicated on 2 locations. Where in the second case the APPLE version was missing.
And since it is a sort of ceph-wrapper around posix_fallocate I expect it to be more obvious it is stands out on its own.
I'm open for both, please confim if you want to persist with your suggestion.

tchaikov · 2018-02-12T07:10:47Z

src/common/ceph_posix_fallocate.c

+      ret = -errno;
+    }
+#endif
+    if (ret < 0) {


this is dead code if HAVE_POSIX_FALLOCATE and not FreeBSD, also, the above __FREEBSD__ and __APPLE__ blocks are difficult to read. could you please consider restructuring them?

Right it is.
However trying to restructure this in the case that we do have HAVE_POSIX_FALLOCATE and __FreeBSD__ running on ZFS things start to get hairy because compile-time and run-time start to interact. But I'll do another attempt.

wjwithagen · 2018-02-20T17:31:44Z

@tchaikov
Perhaps this is now better too your liking?

wjwithagen · 2018-02-20T22:57:49Z

Jenkins retest please

tchaikov · 2018-02-22T04:30:59Z

@wjwithagen it does not build.

wjwithagen · 2018-02-22T10:28:18Z

@tchaikov
Some Linux <> FreeBSD include files issues
Looks like that is fixed now.

wjwithagen · 2018-02-23T14:10:23Z

@tchaikov
I set the backport label, since this will have to go into Luminous as well once we have agreed upon the finals of the PR. But I have no clue if I need to do anything else.

tchaikov · 2018-02-26T02:53:33Z

src/include/compat.h

+#include "fcntl.h"
+
+// The type-value for a ZFS FS in fstatfs.
+#define FS_ZFS_TYPE 0xde


please define this macro in compat.cc if it's not used elsewhere.

@wjwithagen this is not addressed. please remove this line.

tchaikov · 2018-02-26T02:56:23Z

src/include/compat.h

@@ -166,4 +170,16 @@
    0; })
 #endif

+
+#ifdef __cplusplus


why bother adding extern "C"? ceph_posix_fallocate() is not exposed as a part of public API, and it is not exposed to any C program.

tchaikov · 2018-02-26T03:38:33Z

src/common/compat.cc

+      ret = -errno;
+    }
+    return ret;
+#endif


we might want to keep the fallback code handling the platform where posix_fallocate() is not available? see 75b0f7d. also, please note that FileStore does not compile if posix_fallocate() is not available or it targeting platform is not MacOS. while BlueStore has a fallback to emulate fallocate. your PR changes the behavior. please note it down in the commit message.

@tchaikov
Yes I noticed that FileStore did not have a fallback, but just blew up building.
So I considered this a bonus.
But I will put this in the commit message

tchaikov · 2018-02-26T03:41:17Z

src/os/bluestore/BlueStore.cc

-	  r = ::posix_fallocate(fd, 0, size);
-	  if (r) {
+          r = ::ceph_posix_fallocate(fd, 0, size);
+          if (r) {


before your change r is the return code of posix_fallocate(), after your change, r is -(-errono). is this what you expect?

@tchaikov
Good catch, if fumbled that one.

@wjwithagen this is not addressed.

wjwithagen · 2018-03-09T14:29:10Z

@tchaikov
Can you take another look?

wjwithagen · 2018-04-09T21:39:12Z

@tchaikov
This needs to go in before the mimic freeze if possible.
Can you take another look?

tchaikov · 2018-04-10T11:08:51Z

src/include/compat.h

+#include "fcntl.h"
+
+// The type-value for a ZFS FS in fstatfs.
+#define FS_ZFS_TYPE 0xde


@wjwithagen this is not addressed. please remove this line.

tchaikov · 2018-04-10T11:09:38Z

src/os/bluestore/BlueStore.cc

-	  r = ::posix_fallocate(fd, 0, size);
-	  if (r) {
+          r = ::ceph_posix_fallocate(fd, 0, size);
+          if (r) {


@wjwithagen this is not addressed.

wjwithagen · 2018-04-10T14:32:20Z

@tchaikov
grmbl.... Sorry, I was sure that I did "fix" that.
Will fix and ping

tchaikov · 2018-04-10T15:08:48Z

src/common/compat.cc

+        return posix_fallocate(fd, offset, len);
+    }
+#elif defined(__APPLE__)
+    int ret;


nit, move definition of ret to where it is used.

tchaikov · 2018-04-10T15:09:37Z

src/common/compat.cc

+// To prevent this the written buffer needs to be loaded with random data.
+
+int manual_fallocate(int fd, off_t offset, off_t len) {
+    // Try to manually allocate the buffer


remove this comment.

tchaikov · 2018-04-10T15:12:11Z

src/common/compat.cc

+
+int manual_fallocate(int fd, off_t offset, off_t len) {
+    // Try to manually allocate the buffer
+    char data[1024*128];


move the definition of data to where it is used. also, might want pre-populate data with something. like

// populate data with random bits. memset(data, 0x42, sizeof(data));

otherwise the static analyzer will be complaining.

@tchaikov
I was also contemplation in filling it with random data.
In that sense it would really reserve space on a compressing ZFS partition. Otherwise 1024^2 baytes will be reduced to almost nothing, and it is nor really a reservation.
But I'll start with this

tchaikov · 2018-04-10T15:14:33Z

src/common/compat.cc

+	return errno;
+    for (off_t off = 0; off < len; off += sizeof(data)) {
+      if (off + sizeof(data) > len)
+        r = write(fd, data, len - off);


should use safe_write() instead.

tchaikov · 2018-04-10T15:14:42Z

src/common/compat.cc

+
+int on_zfs(int basedir_fd) {
+    struct statfs basefs;
+


nit, remove empty line.

tchaikov · 2018-04-10T15:15:14Z

src/common/compat.cc

+}
+
+int on_zfs(int basedir_fd) {
+    struct statfs basefs;


no need to add struct in C++.

Clang does not seem to agree:

/home/wjw/wip.patch/src/common/compat.cc:44:5: error: must use 'struct' tag to refer to type 'statfs' in this scope statfs basefs;

tchaikov · 2018-04-10T15:15:24Z

src/common/compat.cc

+
+#ifdef HAVE_POSIX_FALLOCATE
+    if (on_zfs(fd)) {
+        // Preallocate the required space manually


remove this comment.

tchaikov · 2018-04-10T15:16:53Z

src/include/compat.h

@@ -13,6 +13,7 @@
 #define CEPH_COMPAT_H

 #include "acconfig.h"
+#include "fcntl.h"


please move this #include to the .cc file.

@tchaikov
It is already there, so I guess that there are reason why I put it there first place
But I'll remove, and see what happens in FreeBSD/Jenkins.

@tchaikov
Perhaps you have a suggestion, since this is an error on the Linux/GCC side of things.

/home/jenkins-build/build/workspace/ceph-pull-requests/src/include/compat.h:169:34: error: 'off_t' has not been declared int ceph_posix_fallocate(int fd, off_t offset, off_t len); ^~~~~ /home/jenkins-build/build/workspace/ceph-pull-requests/src/include/compat.h:169:48: error: 'off_t' has not been declared int ceph_posix_fallocate(int fd, off_t offset, off_t len); ^~~~~ src/CMakeFiles/crush_objs.dir/build.make:230: recipe for target 'src/CMakeFiles/crush_objs.dir/crush/CrushLocation.cc.o' failed

please #include <sys/types.h> for off_t, see http://pubs.opengroup.org/onlinepubs/009696699/basedefs/sys/types.h.html .

wjwithagen · 2018-04-11T11:31:52Z

@tchaikov
Other than the struct statfs declaration, all issues should be addressed.

tchaikov · 2018-04-11T12:19:19Z

src/common/compat.cc

+// To prevent this the written buffer needs to be loaded with random data.
+
+int manual_fallocate(int fd, off_t offset, off_t len) {
+    int r = lseek(fd, offset, SEEK_SET);


wrong indent. please refer following settings:

// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*- // vim: ts=8 sw=2 smarttab

tchaikov · 2018-04-11T12:19:47Z

src/common/compat.cc

+// In which case it is allocated manually, and still that is not a real guarantee
+// that a full buffer is allocated on disk, since it could be compressed.
+// To prevent this the written buffer needs to be loaded with random data.
+


please drop this empty line.

tchaikov · 2018-04-11T14:44:51Z

src/common/compat.cc

+#endif
+} 
+
+// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*-


normally we put this editor variable settings at the top of a source file.

@tchaikov
Oke, but it is sort a mess than there at the top.
Vim/Vi will also find it at the bottom. But I'll fix

wjwithagen · 2018-04-11T15:27:49Z

'mmmm, why is the details output of the failing Jenkins run missing?
Something broke?

tchaikov · 2018-04-11T15:30:08Z

retest this please.

tchaikov · 2018-04-12T01:09:25Z

src/include/compat.h

@@ -15,6 +15,7 @@
 #include "acconfig.h"

 #if defined(__linux__)
+#include <fnctl.h>


@wjwithagen see #20398 (comment)

@tchaikov
missed that remark

tchaikov · 2018-04-12T13:07:13Z

src/include/compat.h

@@ -15,6 +15,7 @@
 #include "acconfig.h"

 #if defined(__linux__)
+#include <sys/types.h>


nit, i think we should move this out of #if defined(__linux__) as off_t is part of POSIX standard.

@tchaikov
Sure

But even still it would not work on any COW FS. So reorganised the code to have a common routine that in the end will allocate a file on disk if needed FileStore would not build when there was no HAVE_POSIX_FALLOCATE other than on Apple. With ceph_posix_fallocate FileStore will also fallback to manually allocating the required file. Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>

tchaikov · 2018-04-15T15:02:17Z

the failure is addressed by #21433

wjwithagen added core needs-qa common labels Feb 11, 2018

wjwithagen requested a review from tchaikov February 11, 2018 21:11

wjwithagen force-pushed the wip-posix_fallocate branch 3 times, most recently from 3d59ad1 to fd3a22c Compare February 11, 2018 23:22

tchaikov requested changes Feb 12, 2018

View reviewed changes

wjwithagen force-pushed the wip-posix_fallocate branch from fd3a22c to aca1799 Compare February 12, 2018 11:34

wjwithagen force-pushed the wip-posix_fallocate branch 3 times, most recently from c01672a to b38b65c Compare February 20, 2018 17:30

yuriw added wip-yuri-testing and removed wip-yuri-testing labels Feb 21, 2018

wjwithagen force-pushed the wip-posix_fallocate branch 2 times, most recently from 302e580 to 855a358 Compare February 21, 2018 23:02

tchaikov removed the needs-qa label Feb 22, 2018

wjwithagen force-pushed the wip-posix_fallocate branch 2 times, most recently from bb711cf to 4e4f6f9 Compare February 22, 2018 09:04

tchaikov self-requested a review February 22, 2018 10:50

wjwithagen added the backport label Feb 23, 2018

tchaikov requested changes Feb 26, 2018

View reviewed changes

wjwithagen force-pushed the wip-posix_fallocate branch from 4e4f6f9 to 22cd224 Compare February 27, 2018 10:11

tchaikov self-requested a review March 9, 2018 14:36

tchaikov requested changes Apr 10, 2018

View reviewed changes

wjwithagen force-pushed the wip-posix_fallocate branch from 2ef513f to 11f6df0 Compare April 10, 2018 14:48

tchaikov reviewed Apr 10, 2018

View reviewed changes

wjwithagen force-pushed the wip-posix_fallocate branch from 11f6df0 to 1b026bb Compare April 11, 2018 11:32

tchaikov requested changes Apr 11, 2018

View reviewed changes

tchaikov reviewed Apr 11, 2018

View reviewed changes

wjwithagen force-pushed the wip-posix_fallocate branch 2 times, most recently from 48f77f7 to 459e519 Compare April 11, 2018 12:58

tchaikov reviewed Apr 11, 2018

View reviewed changes

wjwithagen force-pushed the wip-posix_fallocate branch from 459e519 to 8436735 Compare April 11, 2018 15:29

tchaikov added the wip-kefu-testing label Apr 11, 2018

tchaikov reviewed Apr 12, 2018

View reviewed changes

tchaikov removed the wip-kefu-testing label Apr 12, 2018

wjwithagen force-pushed the wip-posix_fallocate branch from 8436735 to 5354d54 Compare April 12, 2018 10:40

tchaikov added the needs-qa label Apr 12, 2018

tchaikov reviewed Apr 12, 2018

View reviewed changes

wjwithagen force-pushed the wip-posix_fallocate branch from 5354d54 to 1230e69 Compare April 13, 2018 07:27

tchaikov added the wip-kefu-testing label Apr 13, 2018

tchaikov approved these changes Apr 15, 2018

View reviewed changes

tchaikov merged commit ccb44e6 into ceph:master Apr 15, 2018

smithfarm mentioned this pull request Apr 19, 2018

luminous: common: posix_fallocate on ZFS returns EINVAL #21514

Merged

		}

		int ceph_posix_fallocate(int fd, off_t offset, off_t len) {

common: posix_fallocate on ZFS returns EINVAL #20398

common: posix_fallocate on ZFS returns EINVAL #20398

Conversation

wjwithagen commented Feb 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov Feb 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwithagen commented Feb 20, 2018

wjwithagen commented Feb 20, 2018

tchaikov commented Feb 22, 2018

wjwithagen commented Feb 22, 2018

wjwithagen commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwithagen commented Mar 9, 2018

wjwithagen commented Apr 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwithagen commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwithagen commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwithagen commented Apr 11, 2018

tchaikov commented Apr 11, 2018

tchaikov Apr 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Apr 15, 2018

wjwithagen commented Feb 11, 2018 •

edited

tchaikov Feb 12, 2018 •

edited

tchaikov Apr 12, 2018 •

edited