Skip to content

ceph.spec.in, debian/rules: enable rbd-rwl-cache by default only on x86_64#41998

Merged
tchaikov merged 2 commits intoceph:masterfrom
kevinzs2048:arm64-rwl-cache-optional
Jun 27, 2021
Merged

ceph.spec.in, debian/rules: enable rbd-rwl-cache by default only on x86_64#41998
tchaikov merged 2 commits intoceph:masterfrom
kevinzs2048:arm64-rwl-cache-optional

Conversation

@kevinzs2048
Copy link
Copy Markdown
Contributor

@kevinzs2048 kevinzs2048 commented Jun 24, 2021

set rwl cache option on arm64 as PMDK is not well supported

Currently, PMDK is not well supported on Arm64. The PMDK make check can not pass, and librpmem(remote persist memory lib) is not enabled on Arm64.
So in Ceph we can make it optional and wait for PMDK changes on Arm64

Tracker Link: https://tracker.ceph.com/issues/51339


Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@kevinzs2048 kevinzs2048 force-pushed the arm64-rwl-cache-optional branch from c3ff46b to c298dfb Compare June 24, 2021 01:31
@tchaikov tchaikov changed the title pacific: ceph.spec.in: Set rbd-rwl-cache optional on arm64 ceph.spec.in: disable rbd-rwl-cache by default on arm64 Jun 24, 2021
@tchaikov
Copy link
Copy Markdown
Contributor

@kevinzs2048 could you remove "pacific: " from the title of the commit message. and add

Fixes: https://tracker.ceph.com/issues/51339

right before your Signed-off-by line. please see the output of git log for more examples. also, would be be great to accompany some references explaining why

Currently, PMDK is not well supported on Arm64.

in the commit message.

@kevinzs2048 kevinzs2048 force-pushed the arm64-rwl-cache-optional branch from c298dfb to 417e97c Compare June 24, 2021 02:05
@kevinzs2048
Copy link
Copy Markdown
Contributor Author

@tchaikov Thanks for the comments. Done

@tchaikov
Copy link
Copy Markdown
Contributor

jenkins test make check

@kevinzs2048 kevinzs2048 force-pushed the arm64-rwl-cache-optional branch 2 times, most recently from 5161eac to a1c3e81 Compare June 24, 2021 07:15
@idryomov idryomov added the rbd label Jun 24, 2021
ceph.spec.in Outdated
%elifarch aarch64
%bcond_without lttng
%bcond_with rbd_rwl_cache
%bcond_without rbd_ssd_cache
Copy link
Copy Markdown
Contributor

@idryomov idryomov Jun 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem with disabling the rwl mode by default on aarch64, but I don't understand why the ssd mode gets special treatment in the spec file. IIRC it doesn't depend on PMDK or any other libraries, so I think should be enabled by default regardless of the architecture and distribution.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@idryomov #42006 is created to address this issue.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i will rebase this PR once #42006 gets merged (if it is approved).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approved #42006. For this PR, I think we should move rbd_rwl_cache manipulation to the top level because the architectures that it is supported on (x86_64 ppc64le) are the same for Fedora/RHEL and SUSE.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@idryomov agreed.

@kevinzs2048 could you rebase on top of my change and address Ilya's comment?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. @idryomov @tchaikov
Done.
Disable ppc64 as well as PMDK is only official support on x86

@idryomov
Copy link
Copy Markdown
Contributor

@tchaikov Do you see a problem with a single %bcond_without rbd_ssd_cache line at the top level replacing all existing instances?

unlike rbd_rwl_cache, rbd_ssd_cache does not depend on pmdk (libpmem),
so let's enable it on all supported architecture and rpm based distros.

Signed-off-by: Kefu Chai <kchai@redhat.com>
tchaikov
tchaikov previously approved these changes Jun 24, 2021
@tchaikov tchaikov dismissed their stale review June 24, 2021 12:14

dismissed.

@idryomov
Copy link
Copy Markdown
Contributor

idryomov commented Jun 24, 2021

@tchaikov What about deb packages? I see unconditional

extraopts += -DWITH_RBD_RWL=ON

in debian/rules, am I missing something or do we only care about amd64 there?

@tchaikov
Copy link
Copy Markdown
Contributor

@tchaikov What about deb packages? I see unconditional

extraopts += -DWITH_RBD_RWL=ON

in debian/rules, am I missing something or do we only care about amd64 there?

ahh, right! we should conditionalize WITH_RBD_RWL in debian/rules as well.

@kevinzs2048 kevinzs2048 force-pushed the arm64-rwl-cache-optional branch from a1c3e81 to 2bcb3bd Compare June 25, 2021 02:57
…pc64le

set rwl cache option on arm64 and ppc64le as PMDK is not well supported.
Currently, only 64-bit Linux* and Windows* on x86 are supported PMDK

Reference:
1. Experimental support on Arm64, but lacking of librpmem:
See: https://github.com/pmem/pmdk#experimental-support-for-64-bit-arm
2. No RPM for PMDK on Arm64:
See: https://bugzilla.redhat.com/show_bug.cgi?id=1340635
3. > Does PMDK support ARM64*?
   > Currently only 64-bit Linux* and Windows* on x86 are supported.
See: https://software.intel.com/content/www/us/en/develop/articles/persistent-memory-faq.html
4. Make check fail on Arm64
See: pmem/pmdk#5255

Fixes: https://tracker.ceph.com/issues/51339
Signed-off-by: Kevin Zhao <kevin.zhao@linaro.org>
@kevinzs2048 kevinzs2048 force-pushed the arm64-rwl-cache-optional branch from 16c0843 to 8c7729a Compare June 25, 2021 03:57
@kevinzs2048 kevinzs2048 changed the title ceph.spec.in: disable rbd-rwl-cache by default on arm64 ceph.spec.in, debian/rules: disable rbd-rwl-cache by default on arm64 Jun 25, 2021
@tchaikov tchaikov added the arm64 label Jun 25, 2021
@tchaikov
Copy link
Copy Markdown
Contributor

this changeset includes #42006

@tchaikov
Copy link
Copy Markdown
Contributor

@idryomov idryomov changed the title ceph.spec.in, debian/rules: disable rbd-rwl-cache by default on arm64 ceph.spec.in, debian/rules: enable rbd-rwl-cache by default only on x86_64 Jun 25, 2021
@idryomov
Copy link
Copy Markdown
Contributor

Disable ppc64 as well as PMDK is only official support on x86

I see the note about AArch64 support being highly experimental (https://github.com/pmem/pmdk/blob/master/README.md#experimental-packages) but nothing about POWER. Can you share a link?

@tchaikov
Copy link
Copy Markdown
Contributor

@idryomov please see the commit message, in which https://software.intel.com/content/www/us/en/develop/articles/persistent-memory-faq.html is referenced. if you search "Does PMDK support ARM64" in that web page, you could find following answer

Currently only 64-bit Linux* and Windows* on x86 are supported.

which is also quoted in the commit message.

@idryomov
Copy link
Copy Markdown
Contributor

Ah, I thought there might be some sort of architecture support status table in the repo going into more detail and didn't realize that that sentence was all there is. I see that a number of test cases are blocklisted on ppc64 though, so it is probably right.

@idryomov
Copy link
Copy Markdown
Contributor

I assume the plan is to merge #42006 and then rebase this PR so that there is only a single commit?

@tchaikov
Copy link
Copy Markdown
Contributor

I assume the plan is to merge #42006 and then rebase this PR so that there is only a single commit?

either way, probably it's simpler to just merge this one so we save a merge commit.

@tchaikov tchaikov merged commit d3d9677 into ceph:master Jun 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants