| Message ID | 20221230065424.19998-1-yukuai1@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [blktests] dm: add a regression test | expand |
Hi Yu,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.2-rc1 next-20221226]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Yu-Kuai/dm-add-a-regression-test/20221230-143530
patch link: https://lore.kernel.org/r/20221230065424.19998-1-yukuai1%40huaweicloud.com
patch subject: [PATCH blktests] dm: add a regression test
reproduce:
scripts/spdxcheck.py
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
spdxcheck warnings: (new ones prefixed by >>)
>> tests/dm/001: 2:27 Invalid License ID: GPL-3.0+
>> tests/dm/rc: 2:27 Invalid License ID: GPL-3.0+
Hello Yu, thanks for the patch. I think it is good to have this test case to avoid repeating the same regression. Please find my comments in line. CC+: Mike, dm-devel, Mike, could you take a look in this new test case? It adds "dm" test group to blktests. If you have thoughts on how to add device-mapper related test cases to blktests, please share (Or we may be able to discuss later at LSF 2023). On Dec 30, 2022 / 14:54, Yu Kuai wrote: > From: Yu Kuai <yukuai3@huawei.com> > > Verify that reload a dm with itself won't crash the kernel. With this test case, I observed the system crash on Fedora default kernel 6.0.18-300.fc37. Will the kernel fix be delivered to stable kernels? If not, it would be the better to require kernel version 6.2 for this test case not to surprise the blktests users. Regarding the wording, "reload a dm with itself" is a bit confusing for me. How about "reload a dm with maps to itself"? > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > --- > tests/dm/001 | 27 +++++++++++++++++++++++++++ > tests/dm/001.out | 4 ++++ > tests/dm/rc | 11 +++++++++++ > 3 files changed, 42 insertions(+) > create mode 100755 tests/dm/001 > create mode 100644 tests/dm/001.out > create mode 100644 tests/dm/rc > > diff --git a/tests/dm/001 b/tests/dm/001 > new file mode 100755 > index 0000000..55e07f3 > --- /dev/null > +++ b/tests/dm/001 > @@ -0,0 +1,27 @@ > +#!/bin/bash > +# SPDX-License-Identifier: GPL-3.0+ > +# Copyright (C) 2022 Yu Kuai > +# > +# Regression test for commit 077a4033541f ("block: don't allow a disk link > +# holder to itself") > + > +. tests/dm/rc > + > +DESCRIPTION="reload a dm with itself" Same comment on wording as the commit message. > +QUICK=1 > + > +requires() { > + _have_program dmsetup && _have_driver dm-mod I suggest to move these checks to group_requires in dm/rc, since future new test cases in dm group will require them also. Nit: '&&' is no longer required to check multiple requirements. Just, _have_program dmsetup _have_driver dm-mod is fine and a bit better. > +} > + > +test_device() { > + echo "Running ${TEST_NAME}" > + > + dmsetup create test --table "0 8192 linear ${TEST_DEV} 0" > + dmsetup suspend test > + dmsetup reload test --table "0 8192 linear /dev/mapper/test 0" > + dmsetup resume test > + dmsetup remove test > + > + echo "Test complete" > +} > diff --git a/tests/dm/001.out b/tests/dm/001.out > new file mode 100644 > index 0000000..23cf1f0 > --- /dev/null > +++ b/tests/dm/001.out > @@ -0,0 +1,4 @@ > +Running dm/001 > +device-mapper: reload ioctl on test failed: Invalid argument > +Command failed > +Test complete With my test system and kernel v6.2-rc3, the messages above were slightly diffrent. To make the test case pass, I needed changes below: diff --git a/tests/dm/001.out b/tests/dm/001.out index 23cf1f0..543b1db 100644 --- a/tests/dm/001.out +++ b/tests/dm/001.out @@ -1,4 +1,4 @@ Running dm/001 -device-mapper: reload ioctl on test failed: Invalid argument -Command failed +device-mapper: reload ioctl on test failed: Invalid argument +Command failed. Test complete > diff --git a/tests/dm/rc b/tests/dm/rc > new file mode 100644 > index 0000000..e1ad6c6 > --- /dev/null > +++ b/tests/dm/rc > @@ -0,0 +1,11 @@ > +#!/bin/bash > +# SPDX-License-Identifier: GPL-3.0+ > +# Copyright (C) 2022 Yu Kuai > +# > +# TODO: provide a brief description of the group here. Let's not leave this TODO. How about "# Tests for device-mapper."? > + > +. common/rc > + > +group_requires() { > + _have_root > +} > -- > 2.31.1 >
Hi, 在 2023/01/12 9:05, Shinichiro Kawasaki 写道: > Hello Yu, thanks for the patch. I think it is good to have this test case to > avoid repeating the same regression. Please find my comments in line. > > CC+: Mike, dm-devel, > > Mike, could you take a look in this new test case? It adds "dm" test group to > blktests. If you have thoughts on how to add device-mapper related test cases > to blktests, please share (Or we may be able to discuss later at LSF 2023). Can we add "dm" test group to blktests? I'll send a new version if we can. Thanks, Kuai > > On Dec 30, 2022 / 14:54, Yu Kuai wrote: >> From: Yu Kuai <yukuai3@huawei.com> >> >> Verify that reload a dm with itself won't crash the kernel. > > With this test case, I observed the system crash on Fedora default kernel > 6.0.18-300.fc37. Will the kernel fix be delivered to stable kernels? If not, > it would be the better to require kernel version 6.2 for this test case not > to surprise the blktests users. > > Regarding the wording, "reload a dm with itself" is a bit confusing for me. > How about "reload a dm with maps to itself"? > >> >> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >> --- >> tests/dm/001 | 27 +++++++++++++++++++++++++++ >> tests/dm/001.out | 4 ++++ >> tests/dm/rc | 11 +++++++++++ >> 3 files changed, 42 insertions(+) >> create mode 100755 tests/dm/001 >> create mode 100644 tests/dm/001.out >> create mode 100644 tests/dm/rc >> >> diff --git a/tests/dm/001 b/tests/dm/001 >> new file mode 100755 >> index 0000000..55e07f3 >> --- /dev/null >> +++ b/tests/dm/001 >> @@ -0,0 +1,27 @@ >> +#!/bin/bash >> +# SPDX-License-Identifier: GPL-3.0+ >> +# Copyright (C) 2022 Yu Kuai >> +# >> +# Regression test for commit 077a4033541f ("block: don't allow a disk link >> +# holder to itself") >> + >> +. tests/dm/rc >> + >> +DESCRIPTION="reload a dm with itself" > > Same comment on wording as the commit message. > >> +QUICK=1 >> + >> +requires() { >> + _have_program dmsetup && _have_driver dm-mod > > I suggest to move these checks to group_requires in dm/rc, since future new test > cases in dm group will require them also. > > Nit: '&&' is no longer required to check multiple requirements. Just, > > _have_program dmsetup > _have_driver dm-mod > > is fine and a bit better. > >> +} >> + >> +test_device() { >> + echo "Running ${TEST_NAME}" >> + >> + dmsetup create test --table "0 8192 linear ${TEST_DEV} 0" >> + dmsetup suspend test >> + dmsetup reload test --table "0 8192 linear /dev/mapper/test 0" >> + dmsetup resume test >> + dmsetup remove test >> + >> + echo "Test complete" >> +} >> diff --git a/tests/dm/001.out b/tests/dm/001.out >> new file mode 100644 >> index 0000000..23cf1f0 >> --- /dev/null >> +++ b/tests/dm/001.out >> @@ -0,0 +1,4 @@ >> +Running dm/001 >> +device-mapper: reload ioctl on test failed: Invalid argument >> +Command failed >> +Test complete > > With my test system and kernel v6.2-rc3, the messages above were slightly > diffrent. To make the test case pass, I needed changes below: > > diff --git a/tests/dm/001.out b/tests/dm/001.out > index 23cf1f0..543b1db 100644 > --- a/tests/dm/001.out > +++ b/tests/dm/001.out > @@ -1,4 +1,4 @@ > Running dm/001 > -device-mapper: reload ioctl on test failed: Invalid argument > -Command failed > +device-mapper: reload ioctl on test failed: Invalid argument > +Command failed. > Test complete > > >> diff --git a/tests/dm/rc b/tests/dm/rc >> new file mode 100644 >> index 0000000..e1ad6c6 >> --- /dev/null >> +++ b/tests/dm/rc >> @@ -0,0 +1,11 @@ >> +#!/bin/bash >> +# SPDX-License-Identifier: GPL-3.0+ >> +# Copyright (C) 2022 Yu Kuai >> +# >> +# TODO: provide a brief description of the group here. > > Let's not leave this TODO. How about "# Tests for device-mapper."? > >> + >> +. common/rc >> + >> +group_requires() { >> + _have_root >> +} >> -- >> 2.31.1 >> >
On Apr 25, 2023 / 16:22, Yu Kuai wrote: > Hi, > > 在 2023/01/12 9:05, Shinichiro Kawasaki 写道: > > Hello Yu, thanks for the patch. I think it is good to have this test case to > > avoid repeating the same regression. Please find my comments in line. > > > > CC+: Mike, dm-devel, > > > > Mike, could you take a look in this new test case? It adds "dm" test group to > > blktests. If you have thoughts on how to add device-mapper related test cases > > to blktests, please share (Or we may be able to discuss later at LSF 2023). > > Can we add "dm" test group to blktests? I'll send a new version if we > can. I suggest to wait for LSF discussion in May, which could be a good chance to hear opinions of dm experts. I think your new test case is valuable, so IMO it should be added to the new "dm" group, or at least to the existing "block" group. Let's decide the target group after LSF.
On Tue, Apr 25 2023 at 8:15P -0400, Shin'ichiro Kawasaki <shinichiro@fastmail.com> wrote: > On Apr 25, 2023 / 16:22, Yu Kuai wrote: > > Hi, > > > > 在 2023/01/12 9:05, Shinichiro Kawasaki 写道: > > > Hello Yu, thanks for the patch. I think it is good to have this test case to > > > avoid repeating the same regression. Please find my comments in line. > > > > > > CC+: Mike, dm-devel, > > > > > > Mike, could you take a look in this new test case? It adds "dm" test group to > > > blktests. If you have thoughts on how to add device-mapper related test cases > > > to blktests, please share (Or we may be able to discuss later at LSF 2023). > > > > Can we add "dm" test group to blktests? I'll send a new version if we > > can. > > I suggest to wait for LSF discussion in May, which could be a good chance to > hear opinions of dm experts. I think your new test case is valuable, so IMO it > should be added to the new "dm" group, or at least to the existing "block" > group. Let's decide the target group after LSF. > It's obviously fine to add a new "dm" test group to blktests. But just so others are aware: more elaborate dm testing is currently spread across multiple testsuites (e.g. lvm2, cryptsetup, mptest, device-mapper-test-suite, etc). There is new effort to port device-mapper-test-suite tests (which use ruby) to a new python harness currently named "dmtest-python", Joe Thornber is leading this effort (with the assistance of ChatGPT.. apparently it has been wonderful in helping Joe glue python code together to accomplish anything he's needed): https://github.com/jthornber/dmtest-python (we've discussed renaming "dmtest-python" to "dmtests") I've also discussed with Joe the plan to wrap the other disparate testsuites with DM coverage in terms of the new dmtest-python. blktests can be made to be one of the testsuites we add support for (so that all blktests run on various types of DM targets). Really all we need is a means to: 1) list all tests in a testsuite 2) initiate running each test individually Mike
On 4/25/23 10:24, Mike Snitzer wrote: > On Tue, Apr 25 2023 at 8:15P -0400, > Shin'ichiro Kawasaki <shinichiro@fastmail.com> wrote: > >> On Apr 25, 2023 / 16:22, Yu Kuai wrote: >>> Hi, >>> >>> 在 2023/01/12 9:05, Shinichiro Kawasaki 写道: >>>> Hello Yu, thanks for the patch. I think it is good to have this test case to >>>> avoid repeating the same regression. Please find my comments in line. >>>> >>>> CC+: Mike, dm-devel, >>>> >>>> Mike, could you take a look in this new test case? It adds "dm" test group to >>>> blktests. If you have thoughts on how to add device-mapper related test cases >>>> to blktests, please share (Or we may be able to discuss later at LSF 2023). >>> Can we add "dm" test group to blktests? I'll send a new version if we >>> can. >> I suggest to wait for LSF discussion in May, which could be a good chance to >> hear opinions of dm experts. I think your new test case is valuable, so IMO it >> should be added to the new "dm" group, or at least to the existing "block" >> group. Let's decide the target group after LSF. >> > It's obviously fine to add a new "dm" test group to blktests. > > But just so others are aware: more elaborate dm testing is currently > spread across multiple testsuites (e.g. lvm2, cryptsetup, mptest, > device-mapper-test-suite, etc). > > There is new effort to port device-mapper-test-suite tests (which use > ruby) to a new python harness currently named "dmtest-python", Joe > Thornber is leading this effort (with the assistance of > ChatGPT.. apparently it has been wonderful in helping Joe glue python > code together to accomplish anything he's needed): > https://github.com/jthornber/dmtest-python > > (we've discussed renaming "dmtest-python" to "dmtests") > > I've also discussed with Joe the plan to wrap the other disparate > testsuites with DM coverage in terms of the new dmtest-python. > blktests can be made to be one of the testsuites we add support for > (so that all blktests run on various types of DM targets). > > Really all we need is a means to: > 1) list all tests in a testsuite > 2) initiate running each test individually > > Mike Thanks Mike for the detailed information, we did talk about DM testcases in last LSFMM, this is really important piece of blktest that is missing and need to be discussed this year's LSFMM so we can integrate above work in blktests as right now we are not able to establish complete stability due to lack of of the dm tests as we are doing it for block layer code or nvme for example. -ck
On Apr 26, 2023 / 08:42, Chaitanya Kulkarni wrote: > On 4/25/23 10:24, Mike Snitzer wrote: > > On Tue, Apr 25 2023 at 8:15P -0400, > > Shin'ichiro Kawasaki <shinichiro@fastmail.com> wrote: > > > >> On Apr 25, 2023 / 16:22, Yu Kuai wrote: > >>> Hi, > >>> > >>> 在 2023/01/12 9:05, Shinichiro Kawasaki 写道: > >>>> Hello Yu, thanks for the patch. I think it is good to have this test case to > >>>> avoid repeating the same regression. Please find my comments in line. > >>>> > >>>> CC+: Mike, dm-devel, > >>>> > >>>> Mike, could you take a look in this new test case? It adds "dm" test group to > >>>> blktests. If you have thoughts on how to add device-mapper related test cases > >>>> to blktests, please share (Or we may be able to discuss later at LSF 2023). > >>> Can we add "dm" test group to blktests? I'll send a new version if we > >>> can. > >> I suggest to wait for LSF discussion in May, which could be a good chance to > >> hear opinions of dm experts. I think your new test case is valuable, so IMO it > >> should be added to the new "dm" group, or at least to the existing "block" > >> group. Let's decide the target group after LSF. > >> > > It's obviously fine to add a new "dm" test group to blktests. Mike, thanks for the positive comment. Now I know that there are various testsuites related to dm as you noted below. I think the new dm test group in blktests can have different coverage from other dm related testsuites: tests to confirm fixes for failures related to block layer and device-mapper. The new test Yu Kuai suggests will fit into it. Yu, could you post the new version of your patch? Let's add the dm group and the first test case. > > > > But just so others are aware: more elaborate dm testing is currently > > spread across multiple testsuites (e.g. lvm2, cryptsetup, mptest, > > device-mapper-test-suite, etc). Thanks. Good to know that dm test has wide varieties. I found the testsuites on the net. Will try to look into them. [1] lvm2: https://github.com/lvmteam/lvm2/tree/master/test [2] cryptsetup: https://gitlab.com/cryptsetup/cryptsetup/-/tree/main/tests [3] mptest: https://github.com/snitm/mptest [4] device-mapper-test-suite: https://github.com/jthornber/device-mapper-test-suite > > > > There is new effort to port device-mapper-test-suite tests (which use > > ruby) to a new python harness currently named "dmtest-python", Joe > > Thornber is leading this effort (with the assistance of > > ChatGPT.. apparently it has been wonderful in helping Joe glue python > > code together to accomplish anything he's needed): > > https://github.com/jthornber/dmtest-python > > > > (we've discussed renaming "dmtest-python" to "dmtests") > > > > I've also discussed with Joe the plan to wrap the other disparate > > testsuites with DM coverage in terms of the new dmtest-python. It sounds a valuable action to gather the various testsuites so that the dm maintainer or dm developers can run them all easily. I also think that the list of testsuites will help developers to tell what kind of test goes to which testsuite. When an developer adds a test case related to dm, the developer needs to choose which testsuite to add it. As an example, I wonder about dm-zoned. In case we would add test cases to exercise dm-zoned, is there any good testsuite to add them? (Can we set up dm-zoned with dmtest- python?) > > blktests can be made to be one of the testsuites we add support for > > (so that all blktests run on various types of DM targets). > > > > Really all we need is a means to: > > 1) list all tests in a testsuite > > 2) initiate running each test individually IIUC, the dmtest-python takes the role to prepare the various DM targets for test, and each testsuite runs test cases on the targets, right? From blktests point of view, the "block" group of blktests can be run with the various DM targets. Also the "zbd" group of blktests can be run with DM targets with DM_TARGET_ZONED_HM feature. Other test groups such as "nvme" or "scsi" won't run with DM targets. I think it is not difficult for dmtest-python to prepare blktests config files to specify DM targets and the block/zbd test group. If blktests side change is desired, we can discuss. > > > > Mike > > Thanks Mike for the detailed information, we did talk about DM testcases > in last LSFMM, this is really important piece of blktest that is missing > and need to be discussed this year's LSFMM so we can integrate above > work in blktests as right now we are not able to establish complete > stability due to lack of of the dm tests as we are doing it for block > layer code or nvme for example. > > -ck > >
diff --git a/tests/dm/001 b/tests/dm/001 new file mode 100755 index 0000000..55e07f3 --- /dev/null +++ b/tests/dm/001 @@ -0,0 +1,27 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-3.0+ +# Copyright (C) 2022 Yu Kuai +# +# Regression test for commit 077a4033541f ("block: don't allow a disk link +# holder to itself") + +. tests/dm/rc + +DESCRIPTION="reload a dm with itself" +QUICK=1 + +requires() { + _have_program dmsetup && _have_driver dm-mod +} + +test_device() { + echo "Running ${TEST_NAME}" + + dmsetup create test --table "0 8192 linear ${TEST_DEV} 0" + dmsetup suspend test + dmsetup reload test --table "0 8192 linear /dev/mapper/test 0" + dmsetup resume test + dmsetup remove test + + echo "Test complete" +} diff --git a/tests/dm/001.out b/tests/dm/001.out new file mode 100644 index 0000000..23cf1f0 --- /dev/null +++ b/tests/dm/001.out @@ -0,0 +1,4 @@ +Running dm/001 +device-mapper: reload ioctl on test failed: Invalid argument +Command failed +Test complete diff --git a/tests/dm/rc b/tests/dm/rc new file mode 100644 index 0000000..e1ad6c6 --- /dev/null +++ b/tests/dm/rc @@ -0,0 +1,11 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-3.0+ +# Copyright (C) 2022 Yu Kuai +# +# TODO: provide a brief description of the group here. + +. common/rc + +group_requires() { + _have_root +}