Skip to content
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

pmemobj: introduce fuses against ill-considered use of NDCTL_ENABLE=n #5946

Merged
merged 2 commits into from
Mar 19, 2024

Conversation

janekmi
Copy link
Contributor

@janekmi janekmi commented Nov 29, 2023

Requires:

  • release 2.0.1
  • changelog entry

This change is Reviewable

@janekmi janekmi added libpmemobj src/libpmemobj sprint goal This pull request is part of the ongoing sprint no changelog Add to skip the changelog check on your pull request labels Nov 29, 2023
@janekmi
Copy link
Contributor Author

janekmi commented Nov 29, 2023

@janekmi janekmi force-pushed the NDCTL_ENABLE-sealing branch 2 times, most recently from 75d2a9a to 7cace0d Compare November 30, 2023 08:33
Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

🐐

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @janekmi and @osalyk)


src/libpmemobj/Makefile line 39 at r3 (raw file):

include ../Makefile.inc

# https://www.intel.com/content/www/us/en/developer/articles/technical/build-pmem-apps-with-ras.html

Shall we add this link to the error message?

Code quote:

https://www.intel.com/content/www/us/en/developer/articles/technical/build-pmem-apps-with-ras.html

@janekmi janekmi added this to the 2.0.2 milestone Nov 30, 2023
Copy link
Contributor Author

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @osalyk)

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi and @osalyk)


src/libpmemobj/Makefile line 43 at r4 (raw file):

        PMEMOBJ_IGNORE_DIRTY_SHUTDOWN ?= n
        ifneq ($(PMEMOBJ_IGNORE_DIRTY_SHUTDOWN),y)
                $(error Libpmemobj without NDCTL cannot detect dirty shutdowns. This may result in silent data corruption. Continuing the build without NDCTL is highly NOT recommended for production quality systems. If you understand the consequences for consistency of your data please set PMEMOBJ_IGNORE_DIRTY_SHUTDOWN=y to silence this error. $(RAS_SUFFIX))

Let's increase readability.

Suggestion:

$(info Libpmemobj without NDCTL cannot detect dirty shutdowns. This may result in silent data corruption. Continuing the build without NDCTL is highly NOT recommended for production quality systems.)
$(info $(RAS_SUFFIX))
$(info If you understand the consequences for consistency of your data, please set PMEMOBJ_IGNORE_DIRTY_SHUTDOWN=y to silence this error.)
$(error )

src/libpmemobj/Makefile line 48 at r4 (raw file):

        PMEMOBJ_IGNORE_BAD_BLOCKS ?= n
        ifneq ($(PMEMOBJ_IGNORE_BAD_BLOCKS),y)
                $(error Libpmemobj without NDCTL cannot detect bad blocks up front. This may result in SIGBUS at runtime. Continuing the build without NDCTL is highly NOT recommended for production quality systems. If you understand the consequences for the behaviour of your application at runtime please set PMEMOBJ_IGNORE_BAD_BLOCKS=y to silence this error. $(RAS_SUFFIX))

Suggestion:

                $(info Libpmemobj without NDCTL cannot detect bad blocks up front. This may result in SIGBUS at runtime. Continuing the build without NDCTL is highly NOT recommended for production quality systems.)
				$(info $(RAS_SUFFIX))
                $(info If you understand the consequences for the behaviour of your application at runtime, please set PMEMOBJ_IGNORE_BAD_BLOCKS=y to silence this error.)
				$(error )
                

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi and @osalyk)

Copy link
Contributor Author

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @grom72 and @osalyk)


src/libpmemobj/Makefile line 43 at r4 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Let's increase readability.

$(info...) doesn't stop the build. The rest has been applied.


src/libpmemobj/Makefile line 48 at r4 (raw file):

        PMEMOBJ_IGNORE_BAD_BLOCKS ?= n
        ifneq ($(PMEMOBJ_IGNORE_BAD_BLOCKS),y)
                $(error Libpmemobj without NDCTL cannot detect bad blocks up front. This may result in SIGBUS at runtime. Continuing the build without NDCTL is highly NOT recommended for production quality systems. If you understand the consequences for the behaviour of your application at runtime please set PMEMOBJ_IGNORE_BAD_BLOCKS=y to silence this error. $(RAS_SUFFIX))

.

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r6.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @janekmi and @osalyk)


src/libpmemobj/Makefile line 43 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

$(info...) doesn't stop the build. The rest has been applied.

I know, but there is an $(error ) at the end which does the work and stops the build.


src/libpmemobj/Makefile line 48 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

.


src/libpmemobj/Makefile line 39 at r7 (raw file):

include ../Makefile.inc

# Newline character define. Note: Both empty lines are required for it to work.

Do we need such a tricky solution?

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @janekmi and @osalyk)


src/libpmemobj/Makefile line 43 at r4 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

I know, but there is an $(error ) at the end which does the work and stops the build.

The idea was also to put the link in the middle of the text

Copy link
Contributor Author

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @grom72 and @osalyk)


src/libpmemobj/Makefile line 43 at r4 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

The idea was also to put the link in the middle of the text

Done.


src/libpmemobj/Makefile line 48 at r4 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

Done.


src/libpmemobj/Makefile line 39 at r7 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Do we need such a tricky solution?

AFAIK it is the most clean and widely accepted solution. But I think I like yours better.

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 2 files at r8.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @janekmi and @osalyk)


src/libpmemobj/Makefile line 39 at r7 (raw file):

Previously, janekmi (Jan Michalski) wrote…

AFAIK it is the most clean and widely accepted solution. But I think I like yours better.

:)

Copy link
Contributor

@osalyk osalyk left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 2 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

@janekmi janekmi removed the sprint goal This pull request is part of the ongoing sprint label Jan 29, 2024
@janekmi janekmi added sprint goal This pull request is part of the ongoing sprint and removed no changelog Add to skip the changelog check on your pull request labels Mar 14, 2024
Copy link
Contributor Author

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

@janekmi janekmi requested review from grom72 and osalyk March 15, 2024 13:04
Copy link
Contributor

@osalyk osalyk left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @grom72)

Signed-off-by: Jan Michalski <jan.michalski@intel.com>
Signed-off-by: Jan Michalski <jan.michalski@intel.com>
Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

@janekmi janekmi merged commit eb8c8ab into pmem:master Mar 19, 2024
9 checks passed
janekmi added a commit that referenced this pull request Mar 20, 2024
…(fix)

Ref: #5946

Signed-off-by: Jan Michalski <jan.michalski@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libpmemobj src/libpmemobj sprint goal This pull request is part of the ongoing sprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants