New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rbd: support rbd on FreeBSD (without KRBD) #12798

Merged
merged 8 commits into from Jan 16, 2017

Conversation

Projects
None yet
3 participants
@wjwithagen
Contributor

wjwithagen commented Jan 5, 2017

Cmake changes to exclude KRBD and keyutils from building because FreeBSD does not support that.
Code changes to:

  • Stub krbd kernel calls
  • small incompatabilities, missing includes.
  • Fix Clang casts errors

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

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jan 5, 2017

@tchaikov @liewegas

Hi I'm getting this casting error in Clang that doesn't want to go away, with any of the casting version, other than static_cast.
Depressing is that GNU really doesn't like that, so either we fix the code, or I if-else-endif it.

I hope you have some suggestions.

/usr/srcs/Ceph/work/ceph/src/test/librbd/test_mock_fixture.h:28:27: error: reinterpret_cast from 'nullptr_t' to 'ContextWQ *' is not allowed
  ContextWQ *context_wq = reinterpret_cast<ContextWQ *>(wq);
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/srcs/Ceph/work/ceph/src/googletest/googlemock/include/gmock/gmock-generated-actions.h:523:27: note: in instantiation of function template specialization 'CompleteContextActionP2<int, nullptr_t>::gmock_Impl<void (Context *)>::gmock_PerformImpl<Context *, testing::internal::ExcessiveArg, testing::internal::ExcessiveArg, testing::internal::ExcessiveArg, testing::internal::ExcessiveArg, testing::internal::ExcessiveArg, testing::internal::ExcessiveArg, testing::internal::ExcessiveArg, testing::internal::ExcessiveArg, testing::internal::ExcessiveArg>' requested here
    return impl->template gmock_PerformImpl<A0>(args, get<0>(args),
                          ^

@tchaikov tchaikov self-assigned this Jan 5, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jan 5, 2017

@wjwithagen i will take a look at it tomorrow.

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jan 5, 2017

@tchaikov

Probably it is due to NULL usuage on:
/usr/srcs/Ceph/work/ceph/src/test/librbd/test_mock_Journal.cc:268:29

But as usual I'm not able to fix it. nullptr/NULL is not quite my thing.

rbd_types
rbd_replay_types)
if(WITH_KRBD)

This comment has been minimized.

@tchaikov

tchaikov Jan 6, 2017

Contributor

could you squash this commit into 6022d01 ?

${CMAKE_DL_LIBS}
${EXTRALIBS})
if(HAVE_UDEV)

This comment has been minimized.

@tchaikov

tchaikov Jan 6, 2017

Contributor

might want to remove udev from the linked libraries of librbd. as krbd links against it already.

This comment has been minimized.

@wjwithagen

wjwithagen Jan 6, 2017

Contributor

@tchaikov
To start it should use the define for that:
./cmake/modules/Findudev.cmake:23:# UDEV_LIBRARIES - List of libraries when using udev.
But I feel uncomfortable removing it, since it was already there, and I try to change as little functionality as possible when adding FreeBSD/Clang specifics.
Old behaviour should be kept as much as possible for all others.

That said, you are right: librbd add udev.
I'll make a separate PR for that, and rebase this one.

This comment has been minimized.

@tchaikov

tchaikov Jan 6, 2017

Contributor

I try to change as little functionality as possible when adding FreeBSD/Clang specifics.

i agree, but behaviorwise, removing libudev here does not change anything.

I'll make a separate PR for that, and rebase this one.

thanks.

endif()
if(HAVE_UDEV)
target_link_libraries(ceph_test_cls_rbd
udev)

This comment has been minimized.

@tchaikov

tchaikov Jan 6, 2017

Contributor

could just remove blkid and udev from ceph_test_cls_rbd's linked libraries.

This comment has been minimized.

@wjwithagen

wjwithagen Jan 6, 2017

Contributor

@tchaikov
Yup, like above

@@ -233,6 +233,9 @@ option(WITH_KVS "Key value store is here" ON)
# remote block storage
option(WITH_RBD "Remote block storage is here" ON)
# KERNEL remote block storage
option(WITH_KRBD "Kernel Remote block storage is here" ON)

This comment has been minimized.

@tchaikov

tchaikov Jan 6, 2017

Contributor

should send out a FATAL_ERROR message if WITH_KRBD=ON and WITH_RBD=OFF.

This comment has been minimized.

@wjwithagen

wjwithagen Jan 6, 2017

Contributor

@tchaikov
I'll add some diganostics

This comment has been minimized.

@tchaikov

tchaikov Jan 6, 2017

Contributor

this is not addressed.

@@ -38,6 +38,9 @@
#undef ENODATA
#define ENODATA ENOATTR
#endif
#ifndef EREMOTEIO

This comment has been minimized.

@tchaikov

tchaikov Jan 6, 2017

Contributor

could you consolidate this #define with the ones at line:79. so there is only one place that we define EREMOTEIO and HOST_NAME_MAX. also instead of HOST_NAME_MAX , FreeBSD has MAXHOSTNAMELEN. maybe we can define HOST_NAME_MAX like

#ifndef HOST_NAME_MAX
# ifdef MAXHOSTNAMELEN)
#  define HOST_NAME_MAX MAXHOSTNAMELEN
# else
#  define HOST_NAME_MAX 64
# endif
#endif

BTW, 64 is a very conservative number. probably we can make it 255.

@@ -163,6 +163,7 @@ static int parse_unmap_options(char *options)
static int do_kernel_showmapped(Formatter *f)
{
#if defined(__linux__)

This comment has been minimized.

@tchaikov

tchaikov Jan 6, 2017

Contributor

probably you can just ignore Kernel.cc and Nbd.cc in rbd_srcs in src/tools/rbd/CMakeLists.txt if !WITH_KRBD .

This comment has been minimized.

@wjwithagen

wjwithagen Jan 6, 2017

Contributor

@tchaikov
I'm leaving this in for the moment, because I would like to not change the run-cli tests.
And that expects showmapped map unmap in the help output.
So for met that would make this the most transparent fix.

This comment has been minimized.

@tchaikov

tchaikov Jan 6, 2017

Contributor

makes sense. it's non-trivial to conditionalize the run-cli tests, i guess.

This comment has been minimized.

@dillaman

dillaman Jan 6, 2017

Contributor

Should it be based on WITH_KRBD?

This comment has been minimized.

@tchaikov

tchaikov Jan 8, 2017

Contributor

@wjwithagen could you add WITH_KRBD to config-h.in.cmake and conditionalize on it instead?

@@ -25,7 +25,11 @@ ACTION_P(CopyInBufferlist, str) {
}
ACTION_P2(CompleteContext, r, wq) {
#if defined(__clang__)
ContextWQ *context_wq = static_cast<ContextWQ *>(wq);

This comment has been minimized.

@tchaikov

tchaikov Jan 6, 2017

Contributor

although this is a little bit awkward, but this is the best we can have with minimal change, otherwise we need to change some of the callers of CompleteContext to cast the NULL parameter to the ContextWQ *.

@dillaman what do you think?

This comment has been minimized.

@wjwithagen

wjwithagen Jan 6, 2017

Contributor

@tchaikov @dillaman
I'm not a real fan of this sollution either,
But it is the least invasive, and still very obvious as to why this is done.

This comment has been minimized.

@dillaman

dillaman Jan 6, 2017

Contributor

Hmm --- it's pretty ugly so I might vote for just using a C-style cast if Clang doesn't like it.

This comment has been minimized.

@dillaman

dillaman Jan 6, 2017

Contributor

... do you have any details on the compile failure? The only way I can see this failing is if somehow wq auto-resolved to nullptr_t, but nothing that I can find is passing in nullptr (unless Clang is somehow substituting nullptr for NULL -- which is wrong since they are not the same)

This comment has been minimized.

@wjwithagen

wjwithagen Jan 8, 2017

Contributor

@dillaman @tchaikov
This is the full output of the build error when I use the original cast:

 Building CXX object src/test/librbd/CMakeFiles/unittest_librbd.dir/test_mock_Journal.cc.o
cd /usr/srcs/Ceph/work/ceph/build/src/test/librbd && ccache /usr/bin/CC   -DCEPH_LIBDIR=\"/usr/local/lib\" -DCEPH_PKGLIBDIR=\"/usr/local/lib/ceph\" -DTEST_LIBRBD_INTERNALS -I/usr/srcs/Ceph/work/ceph/build/src/include -I/usr/srcs/Ceph/work/ceph/src -isystem /usr/local/include -isystem /usr/srcs/Ceph/work/ceph/build/include -I/usr/srcs/Ceph/work/ceph/src/xxHash -I/usr/srcs/Ceph/work/ceph/src/googletest/googlemock/include -I/usr/srcs/Ceph/work/ceph/src/googletest/googletest/include  -Wall -Wtype-limits -Wignored-qualifiers -Winit-self -Wpointer-arith -Werror=format-security -fno-strict-aliasing -fsigned-char -Wno-unused-function -Wno-unused-local-typedef -Wno-varargs -Wno-gnu-designator -Wno-missing-braces -Wno-parentheses -Wno-deprecated-register  -ftemplate-depth-1024 -Wno-invalid-offsetof -Wnon-virtual-dtor -Wno-inconsistent-missing-override -Wno-mismatched-tags -Wno-unused-private-field -fdiagnostics-color=auto -I/usr/local/include/nss/nss -I/usr/local/include/nspr -fno-builtin-malloc -fno-builtin-calloc -fno-builtin-realloc -fno-builtin-free -Wno-null-dereference -O0 -g -fPIE   -DHAVE_CONFIG_H -D__CEPH__ -D_REENTRANT -D_THREAD_SAFE -D__STDC_FORMAT_MACROS -I/usr/srcs/Ceph/work/ceph/src/googletest/googlemock/include -I/usr/srcs/Ceph/work/ceph/build/src/googletest/googlemock/include -I/usr/srcs/Ceph/work/ceph/src/googletest/googletest/include -I/usr/srcs/Ceph/work/ceph/build/src/googletest/googletest/include -fno-strict-aliasing -std=c++11 -o CMakeFiles/unittest_librbd.dir/test_mock_Journal.cc.o -c /usr/srcs/Ceph/work/ceph/src/test/librbd/test_mock_Journal.cc
In file included from /usr/srcs/Ceph/work/ceph/src/test/librbd/test_mock_Journal.cc:4:
/usr/srcs/Ceph/work/ceph/src/test/librbd/test_mock_fixture.h:31:27: error: reinterpret_cast from 'nullptr_t' to 'ContextWQ *' is not allowed
  ContextWQ *context_wq = reinterpret_cast<ContextWQ *>(wq);
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/srcs/Ceph/work/ceph/src/googletest/googlemock/include/gmock/gmock-generated-actions.h:523:27: note: in instantiation of function template specialization 'CompleteContextActionP2<int, nullptr_t>::gmock_Impl<void (Context *)>::gmock_PerformImpl<Context *, testing::internal::ExcessiveArg, testing::internal::ExcessiveArg, testing::internal::ExcessiveArg, testing::internal::ExcessiveArg, testing::internal::ExcessiveArg, testing::internal::ExcessiveArg, testing::internal::ExcessiveArg, testing::internal::ExcessiveArg, testing::internal::ExcessiveArg>' requested here
    return impl->template gmock_PerformImpl<A0>(args, get<0>(args),
                          ^
/usr/srcs/Ceph/work/ceph/src/test/librbd/test_mock_fixture.h:27:1: note: in instantiation of function template specialization 'testing::internal::ActionHelper<void, CompleteContextActionP2<int, nullptr_t>::gmock_Impl<void (Context *)> >::Perform<Context *>' requested here
ACTION_P2(CompleteContext, r, wq) {
^
/usr/srcs/Ceph/work/ceph/src/googletest/googlemock/include/gmock/gmock-generated-actions.h:1420:13: note: expanded from macro 'ACTION_P2'
            Perform(this, args);\
            ^
/usr/srcs/Ceph/work/ceph/src/test/librbd/test_mock_fixture.h:27:1: note: in instantiation of member function 'CompleteContextActionP2<int, nullptr_t>::gmock_Impl<void (Context *)>::Perform' requested here
/usr/srcs/Ceph/work/ceph/src/googletest/googlemock/include/gmock/gmock-generated-actions.h:1416:7: note: expanded from macro 'ACTION_P2'
      gmock_Impl(p0##_type gmock_p0, p1##_type gmock_p1) : p0(gmock_p0), \
      ^
/usr/srcs/Ceph/work/ceph/src/test/librbd/test_mock_fixture.h:27:1: note: in instantiation of member function 'CompleteContextActionP2<int, nullptr_t>::gmock_Impl<void (Context *)>::gmock_Impl' requested here
/usr/srcs/Ceph/work/ceph/src/googletest/googlemock/include/gmock/gmock-generated-actions.h:1436:39: note: expanded from macro 'ACTION_P2'
      return ::testing::Action<F>(new gmock_Impl<F>(p0, p1));\
                                      ^
/usr/srcs/Ceph/work/ceph/src/test/librbd/test_mock_Journal.cc:268:29: note: in instantiation of function template specialization 'CompleteContextActionP2<int, nullptr_t>::operator Action<void (Context *)>' requested here
                  .WillOnce(CompleteContext(0, NULL));
                            ^
1 error generated.

This comment has been minimized.

@wjwithagen

wjwithagen Jan 10, 2017

Contributor

@dillaman
Not quite.....
A nicer sollution might be to actually fix the typecaset for NULL:
CompleteContext(r, static_cast<ContextWQ *>(NULL))

That works for both FreeBSD/Clang and linux/GCC.
Need to fix that in about 8 locations.
Could do that in a separate PR and drop this commit from this PR?

This comment has been minimized.

@dillaman

dillaman Jan 10, 2017

Contributor

That's fine as well -- long term would still need a Clang compiler in the Jenkins build set to ensure future PRs don't break things

This comment has been minimized.

@wjwithagen

wjwithagen Jan 11, 2017

Contributor

@dillaman
And it needs to be running with a environment where NULL is defined to be nullptr !!
Will submit a PR tomorrow. It is past midnight here.

This comment has been minimized.

@tchaikov

tchaikov Jan 11, 2017

Contributor

yeah, that's also what i suggested in the beginning. as NULL could also be defined as 0 where static_cast<>() does not work.

This comment has been minimized.

@wjwithagen

wjwithagen Jan 11, 2017

Contributor

@tchaikov @dillaman
Gentleman,
If you could checkout #12880.
Once that is accepted, I'll rebase this PR to macht it.

@tchaikov tchaikov removed their assignment Jan 6, 2017

@tchaikov tchaikov requested a review from dillaman Jan 6, 2017

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jan 6, 2017

@tchaikov
Thanx for the review... I'll go over them and fix.

#define HOST_NAME_MAX 255
#endif

This comment has been minimized.

@tchaikov

tchaikov Jan 6, 2017

Contributor

see #12798 (comment), could use MAXHOSTNAMELEN instead hardwire HOST_NAME_MAX to 255.

This comment has been minimized.

@tchaikov

tchaikov Jan 6, 2017

Contributor

and could move the #endif /* __APPLE__ */ before #ifndef EREMOTEIO and change it to #endif.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jan 6, 2017

@wjwithagen needs rebase.

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jan 6, 2017

@tchaikov
Yup, due to the UDEV commit. rebased.
I'm in and out of the datacenter with screwdriver and more, So this needs to go inbetween real work. :)

option(WITH_KRBD "Kernel Remote block storage is here" ON)
if(WITH_KRBD AND !WITH_RBD)
message(FATAL_ERROR "Cannot have WITH_KRBD with WITH_RBD.")

This comment has been minimized.

@tchaikov

tchaikov Jan 8, 2017

Contributor

s/with/without/

This comment has been minimized.

@wjwithagen

wjwithagen Jan 8, 2017

Contributor

@tchaikov
Every day something new to learn.... ;-)

@@ -163,6 +163,7 @@ static int parse_unmap_options(char *options)
static int do_kernel_showmapped(Formatter *f)
{
#if defined(__linux__)

This comment has been minimized.

@tchaikov

tchaikov Jan 8, 2017

Contributor

@wjwithagen could you add WITH_KRBD to config-h.in.cmake and conditionalize on it instead?

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jan 8, 2017

@tchaikov
I think I addressed your suggestions...

wjwithagen added some commits Jan 5, 2017

CMakeLists.txt: Introduce WITH_KBRD for kernel RBD
Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
./do_freebsd.sh: Do not use KRBD
Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
common/event_socket.h: include errno.h to use errno
Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
include/compat.h: define EREMOTEIO for FreeBSD
 - and combined some FreeBSD defines with Apple's

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
include/rbd_types.h: include/types.h for __le*
Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
tools/rbd/action/Kernel.cc: create stubs for FreeBSD
Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
test/librbd/Clang: Clang complains about reinterpret_cast where stati…
…c_cast is sufficient

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
test/run-cli-tests: enable RBD testing
Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jan 12, 2017

@tchaikov @dillaman
Rebased for the NULL-cast if-def PR.
So this PR is now more or less how it should be as well.

@dillaman

lgtm

@tchaikov tchaikov self-assigned this Jan 12, 2017

@dillaman dillaman changed the title from RBD: enable rbd on FreeBSD (without KRBD) to rbd: support rbd on FreeBSD (without KRBD) Jan 16, 2017

@dillaman dillaman merged commit 7c55be9 into ceph:master Jan 16, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@wjwithagen wjwithagen deleted the wjwithagen:wip-wjw-krbd-optional branch Apr 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment