obj: control POOL_FEAT_CHECK_BAD_BLOCKS via sds.at_create CTL

[osalyk]

https://github.com/daos-stack/pmdk/actions/runs/20714683550

This change is 

[janekmi]

IMHO we miss this bit in order to take into account the feature bit you turn off.

diff --git a/src/common/set.c b/src/common/set.c
index f82e9fd51..24b68ae45 100644
--- a/src/common/set.c
+++ b/src/common/set.c
@@ -3042,6 +3042,9 @@ util_pool_open(struct pool_set **setp, const char *path, size_t minpartsize,
 		goto err_poolset_free;
 	}
 
+	/* filter out unsupported or turned off features */
+	compat_features &= attr->features.compat;
+
 	if (compat_features & POOL_FEAT_CHECK_BAD_BLOCKS) {
 		/* check if any bad block recovery file exists */
 		int bfe = badblocks_recovery_file_exists(set);

@janekmi reviewed 1 file and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72 and @osalyk).

---

src/libpmemobj/obj.c line 1062 at r1 (raw file):

	} else {
		attr->features.incompat &= ~POOL_FEAT_SDS; /* off */
		attr->features.compat &= ~POOL_FEAT_CHECK_BAD_BLOCKS;

Suggestion:

attr->features.compat &= ~POOL_FEAT_CHECK_BAD_BLOCKS; /* off */

[osalyk]

@osalyk made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @grom72 and @janekmi).

---

src/libpmemobj/obj.c line 1062 at r1 (raw file):

	} else {
		attr->features.incompat &= ~POOL_FEAT_SDS; /* off */
		attr->features.compat &= ~POOL_FEAT_CHECK_BAD_BLOCKS;

Done.

[grom72]

@grom72 reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi and @osalyk).

---

a discussion (no related file):
Can we have at least one test checking that the ndctl API isn't used when sds_at_create is set to 0?

[janekmi]

@janekmi made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk).

---

a discussion (no related file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Can we have at least one test checking that the ndctl API isn't used when sds_at_create is set to 1?

It seems we can. Using a static build I managed to mock the ndctl_bus_get_first() function. So, whenever it is called we can just crash a test. Names and all the rest of what a proper test should have are TBD.

The question is whether we would like to do it right now. Or at the later stage. @grom72 as we discussed previously I think it still makes sense to clean up this space after the release. Please let me remind you we have to fix the issue before the DAOS 2.8 release so we have very little time.

@osalyk what do you think? Are you up for a challenge? or would prefer to keep it reasonable? 😉

diff --git a/src/test/obj_ndctl/Makefile b/src/test/obj_ndctl/Makefile
new file mode 100644
index 000000000..232ec15b6
--- /dev/null
+++ b/src/test/obj_ndctl/Makefile
@@ -0,0 +1,12 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright 2025, Hewlett Packard Enterprise Development LP
+
+TARGET = obj_ndctl
+OBJS = obj_ndctl.o
+
+BUILD_STATIC_NONDEBUG=n
+
+LIBPMEMOBJ=y
+
+include ../Makefile.inc
+LDFLAGS += $(call extract_funcs, obj_ndctl.c)
diff --git a/src/test/obj_ndctl/obj_ndctl.c b/src/test/obj_ndctl/obj_ndctl.c
new file mode 100644
index 000000000..adfdb3cc0
--- /dev/null
+++ b/src/test/obj_ndctl/obj_ndctl.c
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: BSD-3-Clause
+/* Copyright 2025, Hewlett Packard Enterprise Development LP */
+
+/*
+ * obj_ndctl.c -- XXX
+ */
+
+#include <libpmemobj.h>
+#include <ndctl/libndctl.h>
+
+#include "unittest.h"
+
+FUNC_MOCK(ndctl_bus_get_first, struct ndctl_bus *, struct ndctl_ctx *ctx)
+	FUNC_MOCK_RUN_DEFAULT {
+		UT_ASSERT(0);
+	}
+FUNC_MOCK_END
+
+int
+main(int argc, char *argv[])
+{
+	START(argc, argv, "obj_ndctl");
+	if (argc < 2)
+		exit(1);
+
+	const char *path = argv[1];
+
+	PMEMobjpool *pop = pmemobj_open(path, NULL);
+	UT_ASSERTne(pop, NULL);
+
+	pmemobj_close(pop);
+
+	DONE(NULL);
+}

[janekmi]

@janekmi reviewed 22 files and all commit messages, made 7 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @osalyk).

---

src/test/pmempool_sync/TEST32 line 18 at r3 (raw file):
It is just me or something smells fishy in here? 🤣

I think here is buried the secret: 93d454d

test: test fixing bad blocks saved in recovery files on a non-pmem FS

Fixing bad blocks saved in recovery files should be tested on a non-pmem FS (because on a pmem FS it requires superuser privileges).

It was a long time ago. It may be no longer the truth. I think we should check it. If it is still not possible to run these tests as we run them in our CI right now without superuser privileges on real PMem I think these tests had to be turned off entirely. Since we cannot met both conditions at the same time require_real_pmem and require_fs_type non-pmem.

Code quote:

require_real_pmem
require_test_type medium
require_fs_type non-pmem

---

src/test/pmempool_sync/TEST32 line 40 at r3 (raw file):

expect_normal_exit "$OBJ_VERIFY$EXESUFFIX $POOLSET pmempool$SUFFIX c v &>> $LOG"

turn_on_checking_bad_blocks $POOLSET

This won't be necessary any more when we move to require_fs_type pmem, right?

---

src/test/obj_pool/TEST32 line 14 at r3 (raw file):

require_test_type medium
require_real_pmem

Suggestion:

# For non-pmem POOL_FEAT_CHECK_BAD_BLOCKS is turned off via PMEMOBJ_CONF="sds.at_create=0"
require_real_pmem

---

src/test/obj_pool/TEST33 line 14 at r3 (raw file):

require_test_type medium
require_real_pmem

Suggestion:

# For non-pmem POOL_FEAT_CHECK_BAD_BLOCKS is turned off via PMEMOBJ_CONF="sds.at_create=0"
require_real_pmem

---

src/test/unittest/unittest.sh line 2622 at r3 (raw file):

		path="$DIR"
	fi
	if [ "$PMEM_IS_PMEM_FORCE" != "" ]; then

I think this is the only value we fear. In theory it could be set to 0 as well. It has its own special meaning.
Anyhow I believe the condition written like below conveys more meaning.

Suggestion:

"$PMEM_IS_PMEM_FORCE" == "1"

---

src/test/unittest/unittest.sh line 2623 at r3 (raw file):

	fi
	if [ "$PMEM_IS_PMEM_FORCE" != "" ]; then
		echo "require_real_pmem: PMEM is forced (PMEM_IS_PMEM_FORCE=$PMEM_IS_PMEM_FORCE)"

We ought to have a function doing it so all skip messages look uniformly across the code base. Just copied from require_fs_type().

Suggestion:

verbose_msg "$UNITTEST_NAME: SKIP ($TEST/$REAL_FS/$BUILD$MCSTR$PROV$PM) PMEM is forced (PMEM_IS_PMEM_FORCE=$PMEM_IS_PMEM_FORCE), real PMEM required"

---

src/test/unittest/unittest.sh line 2630 at r3 (raw file):

			;;
		*)
			echo "require_real_pmem: REAL_FS=$REAL_FS, this is not real PMEM"

As above + even the message copied directly from require_fs_type() for uniformity.

Suggestion:

verbose_msg "$UNITTEST_NAME: SKIP ($TEST/$REAL_FS/$BUILD$MCSTR$PROV$PM) REAL_FS $REAL_FS (pmem required)"

[osalyk]

@osalyk made 7 comments.
Reviewable status: 18 of 22 files reviewed, 8 unresolved discussions (waiting on @janekmi).

---

src/test/pmempool_sync/TEST32 line 18 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It is just me or something smells fishy in here? 🤣

I think here is buried the secret: 93d454d

test: test fixing bad blocks saved in recovery files on a non-pmem FS

Fixing bad blocks saved in recovery files should be tested on a non-pmem FS (because on a pmem FS it requires superuser privileges).

It was a long time ago. It may be no longer the truth. I think we should check it. If it is still not possible to run these tests as we run them in our CI right now without superuser privileges on real PMem I think these tests had to be turned off entirely. Since we cannot met both conditions at the same time require_real_pmem and require_fs_type non-pmem.

Done.

---

src/test/pmempool_sync/TEST32 line 40 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

This won't be necessary any more when we move to require_fs_type pmem, right?

• require_real_pmem — required
• turn_on_checking_bad_blocks — still required
• require_fs_type non-pmem — obsolete and removed

---

src/test/unittest/unittest.sh line 2622 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think this is the only value we fear. In theory it could be set to 0 as well. It has its own special meaning.
Anyhow I believe the condition written like below conveys more meaning.

Done.

---

src/test/unittest/unittest.sh line 2623 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

We ought to have a function doing it so all skip messages look uniformly across the code base. Just copied from require_fs_type().

Done.

---

src/test/unittest/unittest.sh line 2630 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

As above + even the message copied directly from require_fs_type() for uniformity.

Done.

---

src/test/obj_pool/TEST33 line 14 at r3 (raw file):

require_test_type medium
require_real_pmem

Done.

---

src/test/obj_pool/TEST32 line 14 at r3 (raw file):

require_test_type medium
require_real_pmem

Done.

[janekmi]

@janekmi reviewed 3 files and all commit messages, and resolved 5 discussions.
Reviewable status: 21 of 22 files reviewed, 3 unresolved discussions (waiting on @osalyk).

[janekmi]

@janekmi reviewed 16 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk).

---

src/test/obj_pool/TEST32 line 16 at r5 (raw file):

# For non-pmem POOL_FEAT_CHECK_BAD_BLOCKS is turned off via PMEMOBJ_CONF="sds.at_create=0"
require_real_pmem
require_badblock_tests_enabled $DIR/testfile

Can you please give me a reason why this is necessary? Possibly a link to the failed test result. Thanks!

[osalyk]

@osalyk made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi).

---

src/test/obj_pool/TEST32 line 16 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Can you please give me a reason why this is necessary? Possibly a link to the failed test result. Thanks!

I wanted to limit running this test with ndctl disabled.

[janekmi]

@janekmi made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk).

---

src/test/obj_pool/TEST32 line 16 at r5 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

I wanted to limit running this test with ndctl disabled.

Why you chose to limit it differently compared to other tests? e.g. using require_sds?

[osalyk]

@osalyk made 1 comment.
Reviewable status: 28 of 30 files reviewed, 2 unresolved discussions (waiting on @janekmi).

---

src/test/obj_pool/TEST32 line 16 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Why you chose to limit it differently compared to other tests? e.g. using require_sds?

I think you're right ;)
Done.

[janekmi]

@janekmi reviewed 2 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @osalyk).

[osalyk]

@osalyk made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72).

---

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

It seems we can. Using a static build I managed to mock the ndctl_bus_get_first() function. So, whenever it is called we can just crash a test. Names and all the rest of what a proper test should have are TBD.

The question is whether we would like to do it right now. Or at the later stage. @grom72 as we discussed previously I think it still makes sense to clean up this space after the release. Please let me remind you we have to fix the issue before the DAOS 2.8 release so we have very little time.

@osalyk what do you think? Are you up for a challenge? or would prefer to keep it reasonable? 😉

diff --git a/src/test/obj_ndctl/Makefile b/src/test/obj_ndctl/Makefile
new file mode 100644
index 000000000..232ec15b6
--- /dev/null
+++ b/src/test/obj_ndctl/Makefile
@@ -0,0 +1,12 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright 2025, Hewlett Packard Enterprise Development LP
+
+TARGET = obj_ndctl
+OBJS = obj_ndctl.o
+
+BUILD_STATIC_NONDEBUG=n
+
+LIBPMEMOBJ=y
+
+include ../Makefile.inc
+LDFLAGS += $(call extract_funcs, obj_ndctl.c)
diff --git a/src/test/obj_ndctl/obj_ndctl.c b/src/test/obj_ndctl/obj_ndctl.c
new file mode 100644
index 000000000..adfdb3cc0
--- /dev/null
+++ b/src/test/obj_ndctl/obj_ndctl.c
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: BSD-3-Clause
+/* Copyright 2025, Hewlett Packard Enterprise Development LP */
+
+/*
+ * obj_ndctl.c -- XXX
+ */
+
+#include <libpmemobj.h>
+#include <ndctl/libndctl.h>
+
+#include "unittest.h"
+
+FUNC_MOCK(ndctl_bus_get_first, struct ndctl_bus *, struct ndctl_ctx *ctx)
+	FUNC_MOCK_RUN_DEFAULT {
+		UT_ASSERT(0);
+	}
+FUNC_MOCK_END
+
+int
+main(int argc, char *argv[])
+{
+	START(argc, argv, "obj_ndctl");
+	if (argc < 2)
+		exit(1);
+
+	const char *path = argv[1];
+
+	PMEMobjpool *pop = pmemobj_open(path, NULL);
+	UT_ASSERTne(pop, NULL);
+
+	pmemobj_close(pop);
+
+	DONE(NULL);
+}

We agreed offline to do it in a separate PR.

[janekmi]

@janekmi reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72).