bluestore: Allow bluestore to build and test on FreeBSD. #25576

wjwithagen · 2018-12-16T17:15:51Z

Free after the branch originally made by Alan Somers.

Introduce HAVE_POSIXAIO to allow FreeBSD aio to be used

From:
https://github.com/asomers/ceph/tree/bluestore_fbsd

Submited-by: Willem Jan Withagen wjw@digiware.nl
Signed-off-by: Alan Somers asomers@gmail.com

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

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

tchaikov · 2018-12-17T03:42:35Z

src/os/bluestore/aio.cc

@@ -56,20 +73,52 @@ int aio_queue_t::submit_batch(aio_iter begin, aio_iter end,

 int aio_queue_t::get_next_completed(int timeout_ms, aio_t **paio, int max)
 {
-  io_event event[max];
+#if defined(HAVE_LIBAIO)
+  io_event events[max];


please avoid mixing refactoring changes along with the ones introducing features. in this case, please keep event as it is. and create another PR commit for s/event/events/.

tchaikov · 2018-12-17T03:51:32Z

src/os/bluestore/BlueStore.cc

@@ -5926,7 +5926,7 @@ int BlueStore::add_new_bluefs_device(int id, const string& dev_path)

  r = _mount_for_bluefs();

-  int reserved;
+  int reserved = 0;


i'd suggest move this change into a separated commit. and explain the reason what it fixes in the commit message.

@wjwithagen could you share with us the error message regarding to "undefined value" ?

tchaikov · 2018-12-17T03:55:51Z

CMakeLists.txt

+    set(HAVE_LIBAIO OFF)
+    set(HAVE_POSIXAIO ON)
+  else()
+    set(HAVE_LIBAIO OFF)


why shall we set them to OFF explicitly?

@tchaikov
Because in this case neither of the two versions are available. And lets make that explicit.

@wjwithagen the inquiry is not addressed. and i am inclined to remove them.

tchaikov · 2018-12-17T04:10:12Z

CMakeLists.txt

+    set(HAVE_LIBAIO ${AIO_FOUND})
+    set(HAVE_POSIXAIO OFF)
+  elseif(FREEBSD)
+    # the AIO library is native in libc


IMHO, it would be easier to digest if put this way:

POSIX AIO is integrated into FreeBSD kernel, and exposed by libc.

FreeBSD does not have libaio as GNU/Linux does. see also
#18357 (comment)

tchaikov · 2018-12-17T04:13:26Z

src/os/bluestore/ceph-aio.h

  struct iocb iocb{};  // must be first element; see shenanigans in aio_queue_t
+#elif defined(HAVE_POSIXAIO)
+  //  static long aio_listio_max = -1;


what is this comment for? can we remove it?

@tchaikov
Copied it from Alan's woork, and I think it is working up to keeping track of the max items that can go into io_submit. And that needs to be read from sysctl, but not implemented yet.

@wjwithagen could you please file a tracker or add a // TODO instead? i can hardly deduce the intention of the author by reading this comment.

src/os/bluestore/ceph-aio.h

tchaikov · 2018-12-17T04:19:39Z

src/include/config-h.in.cmake

@@ -93,6 +93,9 @@
 /* Defined if you have libaio */
 #cmakedefine HAVE_LIBAIO

+/* Defined if you have posix aio */


s/posix aio/POSIX AIO/

tchaikov · 2018-12-17T04:20:54Z

src/os/bluestore/ceph-aio.h

@@ -2,7 +2,15 @@
 // vim: ts=8 sw=2 smarttab

 #pragma once
-# include <libaio.h>
+


might want to revise the topic of this commit to s/build/os/

@wjwithagen this comment is not addressed.

and apparently this change is not merely renaming this file. please split this change into two commit:

rename this file

add the POSIX AIO bits to ceph-aio.h.

and squash the POSIX AIO change into the bluestore enabling commit.

tchaikov · 2018-12-17T04:22:59Z

CMakeLists.txt

@@ -258,8 +257,18 @@ endif()

 option(WITH_BLUESTORE "Bluestore OSD backend" ON)
 if(WITH_BLUESTORE)
-  find_package(aio)
-  set(HAVE_LIBAIO ${AIO_FOUND})
+  if(LINUX)


the commit message is misleading, should s/HAVE_POSIX/HAVE_POSIXAIO/ .

tchaikov · 2018-12-17T04:28:49Z

src/os/bluestore/BlockDevice.cc

@@ -19,7 +19,7 @@

 #include "BlockDevice.h"

-#if defined(HAVE_LIBAIO)
+#if defined(HAVE_LIBAIO) || defined(HAVE_POSIXAIO)


this commit and 544b4f7 come from Alan Somers's branch at https://github.com/asomers/ceph/tree/bluestore_fbsd . i'd suggest

squash 544b4f7 into this commit

keep Alan's "Signed-off-by: Alan Somers asomers@gmail.com" in this commit and remove yours. because i don't think there is enough creative work from you after reading through this commit and asomers@3bf3cbc .

@tchaikov
I don't claim originality for this work. Which is why I attribute the work to Alan's PR/branch. No problem with changing Signed-off.

tchaikov · 2018-12-17T04:36:07Z

Free after the PR originally made by Alan Somers.

i don't think @asomers has ever posted a PR for this.

Introduce HAVE_POSIXAIO to allow FreeBSD aio to be used From: https://github.com/asomers/ceph/tree/bluestore_fbsd Ssubmited-by: Willem Jan Withagen <wjw@digiware.nl> Signed-off-by: Alan Somers <asomers@gmail.com>

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

tchaikov

@wjwithagen

the commit message and title of b3d992b is not accurate, also, i'd suggest not add "Submitted-by" line in the commit message, as every commit in Git has its own meta data attached to it. in this case, it reads

commit b3d992b11f2c02f058bd0ed7c69a7952b8c17b4d
Author: Willem Jan Withagen <wjw@digiware.nl>
Date:   Sun Dec 16 18:08:31 2018 +0100

    bluestore: Set build environment to build BLUESTORE

    Introduce HAVE_POSIXAIO to allow FreeBSD aio to be used

    From:
        https://github.com/asomers/ceph/tree/bluestore_fbsd

    Ssubmited-by: Willem Jan Withagen <wjw@digiware.nl>
    Signed-off-by: Alan Somers <asomers@gmail.com>

actually, i am inclined to just cherry-pick asomers@3bf3cbc to preserve that commit's meta data.

tchaikov · 2018-12-18T10:24:32Z

CMakeLists.txt

+    set(HAVE_LIBAIO OFF)
+    set(HAVE_POSIXAIO ON)
+  else()
+    set(HAVE_LIBAIO OFF)


@wjwithagen the inquiry is not addressed. and i am inclined to remove them.

tchaikov · 2018-12-18T10:27:54Z

src/os/bluestore/BlueStore.cc

@@ -5926,7 +5926,7 @@ int BlueStore::add_new_bluefs_device(int id, const string& dev_path)

  r = _mount_for_bluefs();

-  int reserved;
+  int reserved = 0;


@wjwithagen could you share with us the error message regarding to "undefined value" ?

tchaikov · 2018-12-18T10:31:23Z

src/os/bluestore/ceph-aio.h

@@ -2,7 +2,15 @@
 // vim: ts=8 sw=2 smarttab

 #pragma once
-# include <libaio.h>
+


@wjwithagen this comment is not addressed.

and apparently this change is not merely renaming this file. please split this change into two commit:

rename this file

add the POSIX AIO bits to ceph-aio.h.

and squash the POSIX AIO change into the bluestore enabling commit.

wjwithagen · 2018-12-18T11:11:48Z

@tchaikov
No problem
I'm also fine with you cherry picking asomers/ceph@3bf3cbc first. It might require a bit of tweaking, 'cause I think it does not merge 100%.
So if you do that first, I'll submit new PR's on top of that to get the other stuff in..

tchaikov · 2018-12-18T11:50:55Z

@wjwithagen #25608

build: rename aio.h to prevent conflict with OS aio.h

3921dcc

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

wjwithagen requested review from tchaikov and liewegas December 16, 2018 17:16

wjwithagen added build/ops bluestore FreeBSD Modifications specific for (only) FreeBSD labels Dec 16, 2018

wjwithagen force-pushed the wjw-wip-bluestore branch 2 times, most recently from 9111078 to c59bdd8 Compare December 16, 2018 23:16

tchaikov reviewed Dec 17, 2018

View reviewed changes

wjwithagen added 2 commits December 17, 2018 22:54

bluestore: Set build environment to build BLUESTORE

b3d992b

Introduce HAVE_POSIXAIO to allow FreeBSD aio to be used From: https://github.com/asomers/ceph/tree/bluestore_fbsd Ssubmited-by: Willem Jan Withagen <wjw@digiware.nl> Signed-off-by: Alan Somers <asomers@gmail.com>

os: fix compiler warning about undefined value

70a32e0

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

wjwithagen force-pushed the wjw-wip-bluestore branch from c59bdd8 to 70a32e0 Compare December 17, 2018 21:57

tchaikov reviewed Dec 18, 2018

View reviewed changes

wjwithagen closed this Dec 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bluestore: Allow bluestore to build and test on FreeBSD. #25576

bluestore: Allow bluestore to build and test on FreeBSD. #25576

wjwithagen commented Dec 16, 2018 •

edited

Loading

tchaikov Dec 17, 2018

tchaikov Dec 17, 2018

tchaikov Dec 18, 2018

tchaikov Dec 17, 2018

wjwithagen Dec 17, 2018

tchaikov Dec 18, 2018

tchaikov Dec 17, 2018 •

edited

Loading

tchaikov Dec 17, 2018

wjwithagen Dec 17, 2018

tchaikov Dec 18, 2018

tchaikov Dec 17, 2018

tchaikov Dec 17, 2018

tchaikov Dec 18, 2018

tchaikov Dec 17, 2018

tchaikov Dec 17, 2018

wjwithagen Dec 17, 2018

tchaikov commented Dec 17, 2018

tchaikov left a comment •

edited

Loading

tchaikov Dec 18, 2018

tchaikov Dec 18, 2018

tchaikov Dec 18, 2018

wjwithagen commented Dec 18, 2018

tchaikov commented Dec 18, 2018

bluestore: Allow bluestore to build and test on FreeBSD. #25576

bluestore: Allow bluestore to build and test on FreeBSD. #25576

Conversation

wjwithagen commented Dec 16, 2018 • edited Loading

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 Dec 17, 2018 • edited Loading

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 commented Dec 17, 2018

tchaikov left a comment • edited Loading

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 Dec 18, 2018

tchaikov commented Dec 18, 2018

wjwithagen commented Dec 16, 2018 •

edited

Loading

tchaikov Dec 17, 2018 •

edited

Loading

tchaikov left a comment •

edited

Loading