[v1] selftests: ublk: kublk: fix feature list

[PATCH 3/3] selftests: ublk: add test to verify that feat_map is complete

Posted by Uday Shankar 2 weeks, 1 day ago

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@purestorage.com/t/#u
[2] https://lore.kernel.org/linux-block/2a370ab1-d85b-409d-b762-f9f3f6bdf705@nvidia.com/t/#m1c520a058448d594fd877f07804e69b28908533f

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

-- 
2.34.1

Re: [PATCH 3/3] selftests: ublk: add test to verify that feat_map is complete

Posted by Ming Lei 2 weeks, 1 day ago

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@purestorage.com/t/#u
> [2] https://lore.kernel.org/linux-block/2a370ab1-d85b-409d-b762-f9f3f6bdf705@nvidia.com/t/#m1c520a058448d594fd877f07804e69b28908533f
> 
> 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

Re: [PATCH 3/3] selftests: ublk: add test to verify that feat_map is complete

Posted by Uday Shankar 2 weeks ago

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@purestorage.com/t/#u
> > [2] https://lore.kernel.org/linux-block/2a370ab1-d85b-409d-b762-f9f3f6bdf705@nvidia.com/t/#m1c520a058448d594fd877f07804e69b28908533f
> > 
> > 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?

Re: [PATCH 3/3] selftests: ublk: add test to verify that feat_map is complete

Posted by Ming Lei 2 weeks ago

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@purestorage.com/t/#u
> > > [2] https://lore.kernel.org/linux-block/2a370ab1-d85b-409d-b762-f9f3f6bdf705@nvidia.com/t/#m1c520a058448d594fd877f07804e69b28908533f
> > >
> > > 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,