This patch simplifies kublk's implementation of the feature list command, fixes a bug where a feature was missing, and adds a test to ensure that similar bugs do not happen in the future.
Signed-off-by: Uday Shankar ushankar@purestorage.com --- Uday Shankar (3): selftests: ublk: kublk: simplify feat_map definition selftests: ublk: kublk: add UBLK_F_BUF_REG_OFF_DAEMON to feat_map selftests: ublk: add test to verify that feat_map is complete
tools/testing/selftests/ublk/Makefile | 1 + tools/testing/selftests/ublk/kublk.c | 32 +++++++++++++------------ tools/testing/selftests/ublk/test_generic_13.sh | 16 +++++++++++++ 3 files changed, 34 insertions(+), 15 deletions(-) --- base-commit: da7b97ba0d219a14a83e9cc93f98b53939f12944 change-id: 20250916-ublk_features-07af4e321e5a
Best regards,
Simplify the definition of feat_map by introducing a helper macro FEAT_NAME to avoid having to type the feature name twice. As a side effect, this changes the names in the feature list to be the full macro name instead of the abbreviated names that were used before, but this is a good change for clarity.
Using the full feature macro names ruins the alignment of the output, so change the output format to put each feature's hex value before its name, as this is easier to align nicely. The output now looks as follows:
# ./kublk features ublk_drv features: 0x7fff 0x1 : UBLK_F_SUPPORT_ZERO_COPY 0x2 : UBLK_F_URING_CMD_COMP_IN_TASK 0x4 : UBLK_F_NEED_GET_DATA 0x8 : UBLK_F_USER_RECOVERY 0x10 : UBLK_F_USER_RECOVERY_REISSUE 0x20 : UBLK_F_UNPRIVILEGED_DEV 0x40 : UBLK_F_CMD_IOCTL_ENCODE 0x80 : UBLK_F_USER_COPY 0x100 : UBLK_F_ZONED 0x200 : UBLK_F_USER_RECOVERY_FAIL_IO 0x400 : UBLK_F_UPDATE_SIZE 0x800 : UBLK_F_AUTO_BUF_REG 0x1000 : UBLK_F_QUIESCE 0x2000 : UBLK_F_PER_IO_DAEMON 0x4000 : unknown
Signed-off-by: Uday Shankar ushankar@purestorage.com --- tools/testing/selftests/ublk/kublk.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/ublk/kublk.c b/tools/testing/selftests/ublk/kublk.c index 6512dfbdbce3a82f1202de17319ea593337427e6..4e5d82f2a14a01d9e56d31126eae2e26ec718b6c 100644 --- a/tools/testing/selftests/ublk/kublk.c +++ b/tools/testing/selftests/ublk/kublk.c @@ -1363,21 +1363,22 @@ static int cmd_dev_list(struct dev_ctx *ctx) static int cmd_dev_get_features(void) { #define const_ilog2(x) (63 - __builtin_clzll(x)) +#define FEAT_NAME(f) [const_ilog2(f)] = #f static const char *feat_map[] = { - [const_ilog2(UBLK_F_SUPPORT_ZERO_COPY)] = "ZERO_COPY", - [const_ilog2(UBLK_F_URING_CMD_COMP_IN_TASK)] = "COMP_IN_TASK", - [const_ilog2(UBLK_F_NEED_GET_DATA)] = "GET_DATA", - [const_ilog2(UBLK_F_USER_RECOVERY)] = "USER_RECOVERY", - [const_ilog2(UBLK_F_USER_RECOVERY_REISSUE)] = "RECOVERY_REISSUE", - [const_ilog2(UBLK_F_UNPRIVILEGED_DEV)] = "UNPRIVILEGED_DEV", - [const_ilog2(UBLK_F_CMD_IOCTL_ENCODE)] = "CMD_IOCTL_ENCODE", - [const_ilog2(UBLK_F_USER_COPY)] = "USER_COPY", - [const_ilog2(UBLK_F_ZONED)] = "ZONED", - [const_ilog2(UBLK_F_USER_RECOVERY_FAIL_IO)] = "RECOVERY_FAIL_IO", - [const_ilog2(UBLK_F_UPDATE_SIZE)] = "UPDATE_SIZE", - [const_ilog2(UBLK_F_AUTO_BUF_REG)] = "AUTO_BUF_REG", - [const_ilog2(UBLK_F_QUIESCE)] = "QUIESCE", - [const_ilog2(UBLK_F_PER_IO_DAEMON)] = "PER_IO_DAEMON", + FEAT_NAME(UBLK_F_SUPPORT_ZERO_COPY), + FEAT_NAME(UBLK_F_URING_CMD_COMP_IN_TASK), + FEAT_NAME(UBLK_F_NEED_GET_DATA), + FEAT_NAME(UBLK_F_USER_RECOVERY), + FEAT_NAME(UBLK_F_USER_RECOVERY_REISSUE), + FEAT_NAME(UBLK_F_UNPRIVILEGED_DEV), + FEAT_NAME(UBLK_F_CMD_IOCTL_ENCODE), + FEAT_NAME(UBLK_F_USER_COPY), + FEAT_NAME(UBLK_F_ZONED), + FEAT_NAME(UBLK_F_USER_RECOVERY_FAIL_IO), + FEAT_NAME(UBLK_F_UPDATE_SIZE), + FEAT_NAME(UBLK_F_AUTO_BUF_REG), + FEAT_NAME(UBLK_F_QUIESCE), + FEAT_NAME(UBLK_F_PER_IO_DAEMON), }; struct ublk_dev *dev; __u64 features = 0; @@ -1404,7 +1405,7 @@ static int cmd_dev_get_features(void) feat = feat_map[i]; else feat = "unknown"; - printf("\t%-20s: 0x%llx\n", feat, 1ULL << i); + printf("0x%-16llx: %s\n", 1ULL << i, feat); } }
On Tue, Sep 16, 2025 at 04:05:55PM -0600, Uday Shankar wrote:
Simplify the definition of feat_map by introducing a helper macro FEAT_NAME to avoid having to type the feature name twice. As a side effect, this changes the names in the feature list to be the full macro name instead of the abbreviated names that were used before, but this is a good change for clarity.
Using the full feature macro names ruins the alignment of the output, so change the output format to put each feature's hex value before its name, as this is easier to align nicely. The output now looks as follows:
# ./kublk features ublk_drv features: 0x7fff 0x1 : UBLK_F_SUPPORT_ZERO_COPY 0x2 : UBLK_F_URING_CMD_COMP_IN_TASK 0x4 : UBLK_F_NEED_GET_DATA 0x8 : UBLK_F_USER_RECOVERY 0x10 : UBLK_F_USER_RECOVERY_REISSUE 0x20 : UBLK_F_UNPRIVILEGED_DEV 0x40 : UBLK_F_CMD_IOCTL_ENCODE 0x80 : UBLK_F_USER_COPY 0x100 : UBLK_F_ZONED 0x200 : UBLK_F_USER_RECOVERY_FAIL_IO 0x400 : UBLK_F_UPDATE_SIZE 0x800 : UBLK_F_AUTO_BUF_REG 0x1000 : UBLK_F_QUIESCE 0x2000 : UBLK_F_PER_IO_DAEMON 0x4000 : unknown
Signed-off-by: Uday Shankar ushankar@purestorage.com
Reviewed-by: Ming Lei ming.lei@redhat.com
Thanks, Ming
When UBLK_F_BUF_REG_OFF_DAEMON was added, we missed updating kublk's feat_map, which results in the feature being reported as "unknown." Add UBLK_F_BUF_REG_OFF_DAEMON to feat_map to fix this.
Signed-off-by: Uday Shankar ushankar@purestorage.com --- tools/testing/selftests/ublk/kublk.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/ublk/kublk.c b/tools/testing/selftests/ublk/kublk.c index 4e5d82f2a14a01d9e56d31126eae2e26ec718b6c..b636d40b4889d88f7d64d0e71c6f09eca17e3989 100644 --- a/tools/testing/selftests/ublk/kublk.c +++ b/tools/testing/selftests/ublk/kublk.c @@ -1379,6 +1379,7 @@ static int cmd_dev_get_features(void) FEAT_NAME(UBLK_F_AUTO_BUF_REG), FEAT_NAME(UBLK_F_QUIESCE), FEAT_NAME(UBLK_F_PER_IO_DAEMON), + FEAT_NAME(UBLK_F_BUF_REG_OFF_DAEMON), }; struct ublk_dev *dev; __u64 features = 0;
On Tue, Sep 16, 2025 at 04:05:56PM -0600, Uday Shankar wrote:
When UBLK_F_BUF_REG_OFF_DAEMON was added, we missed updating kublk's feat_map, which results in the feature being reported as "unknown." Add UBLK_F_BUF_REG_OFF_DAEMON to feat_map to fix this.
Signed-off-by: Uday Shankar ushankar@purestorage.com
tools/testing/selftests/ublk/kublk.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/ublk/kublk.c b/tools/testing/selftests/ublk/kublk.c index 4e5d82f2a14a01d9e56d31126eae2e26ec718b6c..b636d40b4889d88f7d64d0e71c6f09eca17e3989 100644 --- a/tools/testing/selftests/ublk/kublk.c +++ b/tools/testing/selftests/ublk/kublk.c @@ -1379,6 +1379,7 @@ static int cmd_dev_get_features(void) FEAT_NAME(UBLK_F_AUTO_BUF_REG), FEAT_NAME(UBLK_F_QUIESCE), FEAT_NAME(UBLK_F_PER_IO_DAEMON),
FEAT_NAME(UBLK_F_BUF_REG_OFF_DAEMON),
Reviewed-by: Ming Lei ming.lei@redhat.com
Thanks, Ming
Add a test that verifies that the currently running kernel does not report support for any features that are unrecognized by kublk. This should catch cases where features are added without updating kublk's feat_map accordingly, which has happened multiple times in the past (see [1], [2]).
Note that this new test may fail if the test suite is older than the kernel, and the newer kernel contains a newly introduced feature. I believe this is not a use case we currently care about - we only care about newer test suites passing on older kernels.
[1] https://lore.kernel.org/linux-block/20250606214011.2576398-1-csander@puresto... [2] https://lore.kernel.org/linux-block/2a370ab1-d85b-409d-b762-f9f3f6bdf705@nvi...
Signed-off-by: Uday Shankar ushankar@purestorage.com --- tools/testing/selftests/ublk/Makefile | 1 + tools/testing/selftests/ublk/test_generic_13.sh | 16 ++++++++++++++++ 2 files changed, 17 insertions(+)
diff --git a/tools/testing/selftests/ublk/Makefile b/tools/testing/selftests/ublk/Makefile index 5d7f4ecfb81612f919a89eb442f948d6bfafe225..770269efe42ab460366485ccc80abfa145a0c57b 100644 --- a/tools/testing/selftests/ublk/Makefile +++ b/tools/testing/selftests/ublk/Makefile @@ -20,6 +20,7 @@ TEST_PROGS += test_generic_09.sh TEST_PROGS += test_generic_10.sh TEST_PROGS += test_generic_11.sh TEST_PROGS += test_generic_12.sh +TEST_PROGS += test_generic_13.sh
TEST_PROGS += test_null_01.sh TEST_PROGS += test_null_02.sh diff --git a/tools/testing/selftests/ublk/test_generic_13.sh b/tools/testing/selftests/ublk/test_generic_13.sh new file mode 100755 index 0000000000000000000000000000000000000000..ff5f22b078ddd08bc19f82aa66da6a44fa073f6f --- /dev/null +++ b/tools/testing/selftests/ublk/test_generic_13.sh @@ -0,0 +1,16 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 + +. "$(cd "$(dirname "$0")" && pwd)"/test_common.sh + +TID="generic_13" +ERR_CODE=0 + +_prep_test "null" "check that feature list is complete" + +if ${UBLK_PROG} features | grep -q unknown; then + ERR_CODE=255 +fi + +_cleanup_test "null" +_show_result $TID $ERR_CODE
On Tue, Sep 16, 2025 at 04:05:57PM -0600, Uday Shankar wrote:
Add a test that verifies that the currently running kernel does not report support for any features that are unrecognized by kublk. This should catch cases where features are added without updating kublk's feat_map accordingly, which has happened multiple times in the past (see [1], [2]).
Note that this new test may fail if the test suite is older than the kernel, and the newer kernel contains a newly introduced feature. I believe this is not a use case we currently care about - we only care about newer test suites passing on older kernels.
[1] https://lore.kernel.org/linux-block/20250606214011.2576398-1-csander@puresto... [2] https://lore.kernel.org/linux-block/2a370ab1-d85b-409d-b762-f9f3f6bdf705@nvi...
Signed-off-by: Uday Shankar ushankar@purestorage.com
tools/testing/selftests/ublk/Makefile | 1 + tools/testing/selftests/ublk/test_generic_13.sh | 16 ++++++++++++++++ 2 files changed, 17 insertions(+)
diff --git a/tools/testing/selftests/ublk/Makefile b/tools/testing/selftests/ublk/Makefile index 5d7f4ecfb81612f919a89eb442f948d6bfafe225..770269efe42ab460366485ccc80abfa145a0c57b 100644 --- a/tools/testing/selftests/ublk/Makefile +++ b/tools/testing/selftests/ublk/Makefile @@ -20,6 +20,7 @@ TEST_PROGS += test_generic_09.sh TEST_PROGS += test_generic_10.sh TEST_PROGS += test_generic_11.sh TEST_PROGS += test_generic_12.sh +TEST_PROGS += test_generic_13.sh TEST_PROGS += test_null_01.sh TEST_PROGS += test_null_02.sh diff --git a/tools/testing/selftests/ublk/test_generic_13.sh b/tools/testing/selftests/ublk/test_generic_13.sh new file mode 100755 index 0000000000000000000000000000000000000000..ff5f22b078ddd08bc19f82aa66da6a44fa073f6f --- /dev/null +++ b/tools/testing/selftests/ublk/test_generic_13.sh @@ -0,0 +1,16 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0
+. "$(cd "$(dirname "$0")" && pwd)"/test_common.sh
+TID="generic_13" +ERR_CODE=0
+_prep_test "null" "check that feature list is complete"
+if ${UBLK_PROG} features | grep -q unknown; then
ERR_CODE=255+fi
+_cleanup_test "null" +_show_result $TID $ERR_CODE
What if the ublk selftest is run on downstream kernel?
Maybe the output can changed to "unsupported" to show that ublk selftest code doesn't cover or use this feature.
Thanks, Ming
On Wed, Sep 17, 2025 at 11:52:38AM +0800, Ming Lei wrote:
On Tue, Sep 16, 2025 at 04:05:57PM -0600, Uday Shankar wrote:
Add a test that verifies that the currently running kernel does not report support for any features that are unrecognized by kublk. This should catch cases where features are added without updating kublk's feat_map accordingly, which has happened multiple times in the past (see [1], [2]).
Note that this new test may fail if the test suite is older than the kernel, and the newer kernel contains a newly introduced feature. I believe this is not a use case we currently care about - we only care about newer test suites passing on older kernels.
[1] https://lore.kernel.org/linux-block/20250606214011.2576398-1-csander@puresto... [2] https://lore.kernel.org/linux-block/2a370ab1-d85b-409d-b762-f9f3f6bdf705@nvi...
Signed-off-by: Uday Shankar ushankar@purestorage.com
tools/testing/selftests/ublk/Makefile | 1 + tools/testing/selftests/ublk/test_generic_13.sh | 16 ++++++++++++++++ 2 files changed, 17 insertions(+)
diff --git a/tools/testing/selftests/ublk/Makefile b/tools/testing/selftests/ublk/Makefile index 5d7f4ecfb81612f919a89eb442f948d6bfafe225..770269efe42ab460366485ccc80abfa145a0c57b 100644 --- a/tools/testing/selftests/ublk/Makefile +++ b/tools/testing/selftests/ublk/Makefile @@ -20,6 +20,7 @@ TEST_PROGS += test_generic_09.sh TEST_PROGS += test_generic_10.sh TEST_PROGS += test_generic_11.sh TEST_PROGS += test_generic_12.sh +TEST_PROGS += test_generic_13.sh TEST_PROGS += test_null_01.sh TEST_PROGS += test_null_02.sh diff --git a/tools/testing/selftests/ublk/test_generic_13.sh b/tools/testing/selftests/ublk/test_generic_13.sh new file mode 100755 index 0000000000000000000000000000000000000000..ff5f22b078ddd08bc19f82aa66da6a44fa073f6f --- /dev/null +++ b/tools/testing/selftests/ublk/test_generic_13.sh @@ -0,0 +1,16 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0
+. "$(cd "$(dirname "$0")" && pwd)"/test_common.sh
+TID="generic_13" +ERR_CODE=0
+_prep_test "null" "check that feature list is complete"
+if ${UBLK_PROG} features | grep -q unknown; then
ERR_CODE=255+fi
+_cleanup_test "null" +_show_result $TID $ERR_CODE
What if the ublk selftest is run on downstream kernel?
Maybe the output can changed to "unsupported" to show that ublk selftest code doesn't cover or use this feature.
Yes I pointed out this issue in the commit message too. But I am unsure what you're asking for.
I think we need a failure here if this test is to fulfill its intended purposes (catching cases where a feature is added without updating the feat_map in kublk). This does also cause failures in cases where an old test suite is run against a newer kernel. Since I think this is an unusual case, perhaps I can add a log line when this test fails saying that the failure is expected if running an old test suite against a new kernel?
On Thu, Sep 18, 2025 at 3:13 AM Uday Shankar ushankar@purestorage.com wrote:
On Wed, Sep 17, 2025 at 11:52:38AM +0800, Ming Lei wrote:
On Tue, Sep 16, 2025 at 04:05:57PM -0600, Uday Shankar wrote:
Add a test that verifies that the currently running kernel does not report support for any features that are unrecognized by kublk. This should catch cases where features are added without updating kublk's feat_map accordingly, which has happened multiple times in the past (see [1], [2]).
Note that this new test may fail if the test suite is older than the kernel, and the newer kernel contains a newly introduced feature. I believe this is not a use case we currently care about - we only care about newer test suites passing on older kernels.
[1] https://lore.kernel.org/linux-block/20250606214011.2576398-1-csander@puresto... [2] https://lore.kernel.org/linux-block/2a370ab1-d85b-409d-b762-f9f3f6bdf705@nvi...
Signed-off-by: Uday Shankar ushankar@purestorage.com
tools/testing/selftests/ublk/Makefile | 1 + tools/testing/selftests/ublk/test_generic_13.sh | 16 ++++++++++++++++ 2 files changed, 17 insertions(+)
diff --git a/tools/testing/selftests/ublk/Makefile b/tools/testing/selftests/ublk/Makefile index 5d7f4ecfb81612f919a89eb442f948d6bfafe225..770269efe42ab460366485ccc80abfa145a0c57b 100644 --- a/tools/testing/selftests/ublk/Makefile +++ b/tools/testing/selftests/ublk/Makefile @@ -20,6 +20,7 @@ TEST_PROGS += test_generic_09.sh TEST_PROGS += test_generic_10.sh TEST_PROGS += test_generic_11.sh TEST_PROGS += test_generic_12.sh +TEST_PROGS += test_generic_13.sh
TEST_PROGS += test_null_01.sh TEST_PROGS += test_null_02.sh diff --git a/tools/testing/selftests/ublk/test_generic_13.sh b/tools/testing/selftests/ublk/test_generic_13.sh new file mode 100755 index 0000000000000000000000000000000000000000..ff5f22b078ddd08bc19f82aa66da6a44fa073f6f --- /dev/null +++ b/tools/testing/selftests/ublk/test_generic_13.sh @@ -0,0 +1,16 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0
+. "$(cd "$(dirname "$0")" && pwd)"/test_common.sh
+TID="generic_13" +ERR_CODE=0
+_prep_test "null" "check that feature list is complete"
+if ${UBLK_PROG} features | grep -q unknown; then
ERR_CODE=255+fi
+_cleanup_test "null" +_show_result $TID $ERR_CODE
What if the ublk selftest is run on downstream kernel?
Maybe the output can changed to "unsupported" to show that ublk selftest code doesn't cover or use this feature.
Yes I pointed out this issue in the commit message too. But I am unsure what you're asking for.
I think we need a failure here if this test is to fulfill its intended purposes (catching cases where a feature is added without updating the feat_map in kublk). This does also cause failures in cases where an old test suite is run against a newer kernel. Since I think this is an unusual case, perhaps I can add a log line when this test fails saying
That also requires any new feature to have a selftest to cover, but sounds good, and should be the motivation of this patch.
that the failure is expected if running an old test suite against a new kernel?
Fair enough, then this patch looks fine for me:
Reviewed-by: Ming Lei ming.lei@redhat.com
Thanks,
linux-kselftest-mirror@lists.linaro.org
-
Ming Lei -
Uday Shankar