| Message ID | 20240613123019.3983399-1-ofir.gal@volumez.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [blktests] md: add regression test for "md/md-bitmap: fix writing non bitmap pages" | expand |
On Jun 13, 2024 / 15:30, Ofir Gal wrote: > A bug in md-bitmap has been discovered by setting up a md raid on top of > nvme-tcp devices that has optimal io size larger than the allocated > bitmap. > > The following test reproduce the bug by setting up a md-raid on top of > nvme-tcp device over ram device that sets the optimal io size by using > dm-stripe. > > Signed-off-by: Ofir Gal <ofir.gal@volumez.com> > --- > This is my first contribution to blktests. The tests hangs when being > ran on a kernel without the bugfix. Should we wait for the bugfix to be > merged to upstream before merging the test? Thank you for the contribution :) If the bug does not cause any hang or failure of other following test case runs, I think it's ok to add this test case to the blktests before the fix gets merged to the upstream. Please find my in-line comments below. Later on, I will do trial runs. > > common/brd | 28 +++++++++++++++++ > tests/md/001 | 80 ++++++++++++++++++++++++++++++++++++++++++++++++ > tests/md/001.out | 2 ++ > tests/md/rc | 12 ++++++++ > 4 files changed, 122 insertions(+) > create mode 100644 common/brd > create mode 100755 tests/md/001 > create mode 100644 tests/md/001.out > create mode 100644 tests/md/rc > > diff --git a/common/brd b/common/brd > new file mode 100644 > index 0000000..b8cd4e3 > --- /dev/null > +++ b/common/brd > @@ -0,0 +1,28 @@ > +#!/bin/bash > +# SPDX-License-Identifier: GPL-3.0+ > +# Copyright (C) 2024 Ofir Gal > +# > +# brd helper functions > + > +. common/shellcheck > + > +_have_brd() { > + _have_driver brd This _have_driver() call checks brd driver is available, but it does not check brd driver is loadable. I think _init_brd() and _cleanup_brd() assume that brd driver is loadable. For that check, please do "_have_module brd" instead. One left work is to improve this case work with built-in brd driver, because some blktests users prefer running tests with built-in drivers. At this point, I think it's okay to go with loadable brd driver. > +} > + > +_init_brd() { > + # _have_brd loads brd, we need to wait a bit for brd to be not in use in > + # order to reload it > + sleep 0.2 > + > + if ! modprobe -r brd || ! modprobe brd "$@" ; then > + echo "failed to reload brd with args: $*" > + return 1 > + fi > + > + return 0 > +} > + > +_cleanup_brd() { > + modprobe -r brd > +} > diff --git a/tests/md/001 b/tests/md/001 > new file mode 100755 > index 0000000..d5fb755 > --- /dev/null > +++ b/tests/md/001 > @@ -0,0 +1,80 @@ > +#!/bin/bash > +# SPDX-License-Identifier: GPL-3.0+ > +# Copyright (C) 2024 Ofir Gal > +# > +# Regression test for patch "md/md-bitmap: fix writing non bitmap pages" and > +# for patch "nvme-tcp: use sendpages_ok() instead of sendpage_ok()" > + > +. tests/md/rc > +. tests/nvme/rc I want to avoid cross references acoss test groups. So far, all test groups do not have any cross reference to keep them independent. How about to add this test case to the nvme test group? > +. common/brd > + > +DESCRIPTION="Create a raid with bitmap on top of nvme device with > +optimal-io-size over bitmap size" This descrption is printed as blktests runs. All other blktests have single line description then the two lines description looks strange. Can we make it shorter to fit in one line? > +QUICK=1 > + > +#restrict test to nvme-tcp only > +nvme_trtype=tcp > +nvmet_blkdev_type="device" > + > +requires() { > + # Require dm-stripe > + _have_program dmsetup > + _have_driver dm-mod > + > + _require_nvme_trtype tcp > + _have_brd > +} > + > +# Sets up a brd device of 1G with optimal-io-size of 256K > +setup_underlying_device() { > + if ! _init_brd rd_size=1048576 rd_nr=1; then > + return 1 > + fi > + > + dmsetup create ram0_big_optio --table \ > + "0 $(blockdev --getsz /dev/ram0) striped 1 512 /dev/ram0 0" > +} > + > +cleanup_underlying_device() { > + dmsetup remove ram0_big_optio > + _cleanup_brd > +} > + > +# Sets up a local host nvme over tcp > +setup_nvme_over_tcp() { > + _setup_nvmet > + > + local port > + port="$(_create_nvmet_port "${nvme_trtype}")" > + > + _create_nvmet_subsystem "blktests-subsystem-0" "/dev/mapper/ram0_big_optio" > + _add_nvmet_subsys_to_port "${port}" "blktests-subsystem-0" > + > + _create_nvmet_host "blktests-subsystem-0" "${def_hostnqn}" > + > + _nvme_connect_subsys --subsysnqn "blktests-subsystem-0" > + nvmedev=$(_find_nvme_dev "blktests-subsystem-0") > +} > + > +cleanup_nvme_over_tcp() { > + _nvmet_target_cleanup --subsysnqn "blktests-subsystem-0" > +} > + > +test() { > + echo "Running ${TEST_NAME}" > + > + setup_underlying_device > + setup_nvme_over_tcp > + > + # Hangs here without the fix > + mdadm -q --create /dev/md/blktests_md --level=1 --bitmap=internal \ In the past, short options caused some trouble. I suggest to use the long option "--quiet" in place of the short option "-q". > + --bitmap-chunk=1024K --assume-clean --run --raid-devices=2 \ > + /dev/"${nvmedev}"n1 missing > + > + mdadm -q --stop /dev/md/blktests_md Ditto. > + cleanup_nvme_over_tcp > + cleanup_underlying_device > + > + echo "Test complete" > +} > diff --git a/tests/md/001.out b/tests/md/001.out > new file mode 100644 > index 0000000..edd2ced > --- /dev/null > +++ b/tests/md/001.out > @@ -0,0 +1,2 @@ > +Running md/001 > +Test complete > diff --git a/tests/md/rc b/tests/md/rc > new file mode 100644 > index 0000000..d492579 > --- /dev/null > +++ b/tests/md/rc > @@ -0,0 +1,12 @@ > +#!/bin/bash > +# SPDX-License-Identifier: GPL-3.0+ > +# Copyright (C) 2024 Ofir Gal > +# > +# Tests for md raid > + > +. common/rc > + > +group_requires() { > + _have_root > + _have_program mdadm > +} > -- > 2.45.1 >
On 17/06/2024 14:32, Shinichiro Kawasaki wrote: > On Jun 13, 2024 / 15:30, Ofir Gal wrote: >> A bug in md-bitmap has been discovered by setting up a md raid on top of >> nvme-tcp devices that has optimal io size larger than the allocated >> bitmap. >> >> The following test reproduce the bug by setting up a md-raid on top of >> nvme-tcp device over ram device that sets the optimal io size by using >> dm-stripe. >> >> Signed-off-by: Ofir Gal <ofir.gal@volumez.com> >> --- >> This is my first contribution to blktests. The tests hangs when being >> ran on a kernel without the bugfix. Should we wait for the bugfix to be >> merged to upstream before merging the test? > Thank you for the contribution :) If the bug does not cause any hang or failure > of other following test case runs, I think it's ok to add this test case to the > blktests before the fix gets merged to the upstream. > > Please find my in-line comments below. Later on, I will do trial runs. Great! >> common/brd | 28 +++++++++++++++++ >> tests/md/001 | 80 ++++++++++++++++++++++++++++++++++++++++++++++++ >> tests/md/001.out | 2 ++ >> tests/md/rc | 12 ++++++++ >> 4 files changed, 122 insertions(+) >> create mode 100644 common/brd >> create mode 100755 tests/md/001 >> create mode 100644 tests/md/001.out >> create mode 100644 tests/md/rc >> >> diff --git a/common/brd b/common/brd >> new file mode 100644 >> index 0000000..b8cd4e3 >> --- /dev/null >> +++ b/common/brd >> @@ -0,0 +1,28 @@ >> +#!/bin/bash >> +# SPDX-License-Identifier: GPL-3.0+ >> +# Copyright (C) 2024 Ofir Gal >> +# >> +# brd helper functions >> + >> +. common/shellcheck >> + >> +_have_brd() { >> + _have_driver brd > This _have_driver() call checks brd driver is available, but it does not check > brd driver is loadable. I think _init_brd() and _cleanup_brd() assume that brd > driver is loadable. For that check, please do "_have_module brd" instead. > > One left work is to improve this case work with built-in brd driver, because > some blktests users prefer running tests with built-in drivers. At this point, I > think it's okay to go with loadable brd driver. Ok, applied to v2 >> diff --git a/tests/md/001 b/tests/md/001 >> new file mode 100755 >> index 0000000..d5fb755 >> --- /dev/null >> +++ b/tests/md/001 >> @@ -0,0 +1,80 @@ >> +#!/bin/bash >> +# SPDX-License-Identifier: GPL-3.0+ >> +# Copyright (C) 2024 Ofir Gal >> +# >> +# Regression test for patch "md/md-bitmap: fix writing non bitmap pages" and >> +# for patch "nvme-tcp: use sendpages_ok() instead of sendpage_ok()" >> + >> +. tests/md/rc >> +. tests/nvme/rc > I want to avoid cross references acoss test groups. So far, all test groups do > not have any cross reference to keep them independent. How about to add this > test case to the nvme test group? I don't mind to add it to the nvme test group, just to clarify the test checks a bug in md. The bug is "visible" only when the underlying device of the raid is a network block device that utilize MSG_SPLICE_PAGES. nvme-tcp is used as the network device, I'm not sure it's related to the nvme test group. What do you think? >> +. common/brd >> + >> +DESCRIPTION="Create a raid with bitmap on top of nvme device with >> +optimal-io-size over bitmap size" > This descrption is printed as blktests runs. All other blktests have single line > description then the two lines description looks strange. Can we make it shorter > to fit in one line? Yes, does "Raid with bitmap on nvme device with opt-io-size over bitmap size" sounds good? >> +test() { >> + echo "Running ${TEST_NAME}" >> + >> + setup_underlying_device >> + setup_nvme_over_tcp >> + >> + # Hangs here without the fix >> + mdadm -q --create /dev/md/blktests_md --level=1 --bitmap=internal \ > In the past, short options caused some trouble. I suggest to use the long option > "--quiet" in place of the short option "-q". Applied to v2.
CC+: linux-nvme, Daniel, Chaitanya, On Jun 17, 2024 / 19:05, Ofir Gal wrote: [...] > > >> diff --git a/tests/md/001 b/tests/md/001 > >> new file mode 100755 > >> index 0000000..d5fb755 > >> --- /dev/null > >> +++ b/tests/md/001 > >> @@ -0,0 +1,80 @@ > >> +#!/bin/bash > >> +# SPDX-License-Identifier: GPL-3.0+ > >> +# Copyright (C) 2024 Ofir Gal > >> +# > >> +# Regression test for patch "md/md-bitmap: fix writing non bitmap pages" and > >> +# for patch "nvme-tcp: use sendpages_ok() instead of sendpage_ok()" > >> + > >> +. tests/md/rc > >> +. tests/nvme/rc > > I want to avoid cross references acoss test groups. So far, all test groups do > > not have any cross reference to keep them independent. How about to add this > > test case to the nvme test group? > I don't mind to add it to the nvme test group, just to clarify the test > checks a bug in md. The bug is "visible" only when the underlying device > of the raid is a network block device that utilize MSG_SPLICE_PAGES. Good to know this background. I suggest to add the last sentence above to the test case script header comment. > > nvme-tcp is used as the network device, I'm not sure it's related to > the nvme test group. What do you think? I see... The bug is in md sub-system, then it's the better to have the new test case in the new md test group. To avoid the cross reference, the nvmet related helper functions should move from tests/nvme/rc to common/nvmet, so that this test/md/001 can refer them. This will be another separated, preparation patch. To nvme experts: If you have comments on this, please chime in. > > >> +. common/brd > >> + > >> +DESCRIPTION="Create a raid with bitmap on top of nvme device with > >> +optimal-io-size over bitmap size" > > This descrption is printed as blktests runs. All other blktests have single line > > description then the two lines description looks strange. Can we make it shorter > > to fit in one line? > Yes, does "Raid with bitmap on nvme device with opt-io-size over bitmap > size" sounds good? The word "tcp" sounds important. And the word "nvmet" sounds better than "nvme". So how about: "Raid with bitmap on tcp nvmet with opt-io-size over bitmap size"? > > >> +test() { > >> + echo "Running ${TEST_NAME}" > >> + > >> + setup_underlying_device > >> + setup_nvme_over_tcp > >> + > >> + # Hangs here without the fix > >> + mdadm -q --create /dev/md/blktests_md --level=1 --bitmap=internal \ > > In the past, short options caused some trouble. I suggest to use the long option > > "--quiet" in place of the short option "-q". > Applied to v2. >
On 6/17/2024 6:24 PM, Shinichiro Kawasaki wrote: >> nvme-tcp is used as the network device, I'm not sure it's related to >> the nvme test group. What do you think? > I see... The bug is in md sub-system, then it's the better to have the new test > case in the new md test group. To avoid the cross reference, the nvmet related > helper functions should move from tests/nvme/rc to common/nvmet, so that this > test/md/001 can refer them. This will be another separated, preparation patch. > > To nvme experts: If you have comments on this, please chime in. no cross references please, above suggestion seems reasonable, however I'd like to see actual prep patch before I comment further .. -ck
On Tue, Jun 18, 2024 at 04:41:22AM GMT, Chaitanya Kulkarni wrote: > On 6/17/2024 6:24 PM, Shinichiro Kawasaki wrote: > >> nvme-tcp is used as the network device, I'm not sure it's related to > >> the nvme test group. What do you think? > > I see... The bug is in md sub-system, then it's the better to have the new test > > case in the new md test group. To avoid the cross reference, the nvmet related > > helper functions should move from tests/nvme/rc to common/nvmet, so that this > > test/md/001 can refer them. This will be another separated, preparation patch. > > > > To nvme experts: If you have comments on this, please chime in. > > no cross references please, above suggestion seems reasonable, however > I'd like to see actual prep patch before I comment further .. Splitting the host from the target code in rc kind makes sense to me. I am fine with moving the nvmet code to common.
On 18/06/2024 4:24, Shinichiro Kawasaki wrote: > CC+: linux-nvme, Daniel, Chaitanya, > > On Jun 17, 2024 / 19:05, Ofir Gal wrote: > [...] >>>> diff --git a/tests/md/001 b/tests/md/001 >>>> new file mode 100755 >>>> index 0000000..d5fb755 >>>> --- /dev/null >>>> +++ b/tests/md/001 >>>> @@ -0,0 +1,80 @@ >>>> +#!/bin/bash >>>> +# SPDX-License-Identifier: GPL-3.0+ >>>> +# Copyright (C) 2024 Ofir Gal >>>> +# >>>> +# Regression test for patch "md/md-bitmap: fix writing non bitmap pages" and >>>> +# for patch "nvme-tcp: use sendpages_ok() instead of sendpage_ok()" >>>> + >>>> +. tests/md/rc >>>> +. tests/nvme/rc >>> I want to avoid cross references acoss test groups. So far, all test groups do >>> not have any cross reference to keep them independent. How about to add this >>> test case to the nvme test group? >> I don't mind to add it to the nvme test group, just to clarify the test >> checks a bug in md. The bug is "visible" only when the underlying device >> of the raid is a network block device that utilize MSG_SPLICE_PAGES. > Good to know this background. I suggest to add the last sentence above to the > test case script header comment. Will do. >> nvme-tcp is used as the network device, I'm not sure it's related to >> the nvme test group. What do you think? > I see... The bug is in md sub-system, then it's the better to have the new test > case in the new md test group. To avoid the cross reference, the nvmet related > helper functions should move from tests/nvme/rc to common/nvmet, so that this > test/md/001 can refer them. This will be another separated, preparation patch. Ok, should it be a patch set or two completely separated patches? >>>> +. common/brd >>>> + >>>> +DESCRIPTION="Create a raid with bitmap on top of nvme device with >>>> +optimal-io-size over bitmap size" >>> This descrption is printed as blktests runs. All other blktests have single line >>> description then the two lines description looks strange. Can we make it shorter >>> to fit in one line? >> Yes, does "Raid with bitmap on nvme device with opt-io-size over bitmap >> size" sounds good? > The word "tcp" sounds important. And the word "nvmet" sounds better than "nvme". > So how about: "Raid with bitmap on tcp nvmet with opt-io-size over bitmap size"? Sounds good to me.
On Jun 18, 2024 / 10:45, Ofir Gal wrote: > > > On 18/06/2024 4:24, Shinichiro Kawasaki wrote: > > CC+: linux-nvme, Daniel, Chaitanya, > > > > On Jun 17, 2024 / 19:05, Ofir Gal wrote: [...] > >> nvme-tcp is used as the network device, I'm not sure it's related to > >> the nvme test group. What do you think? > > I see... The bug is in md sub-system, then it's the better to have the new test > > case in the new md test group. To avoid the cross reference, the nvmet related > > helper functions should move from tests/nvme/rc to common/nvmet, so that this > > test/md/001 can refer them. This will be another separated, preparation patch. > Ok, should it be a patch set or two completely separated patches? I suggest to make them a patch set: the 1st patch moves the nvmet functions from tests/nvme/rc to common/nvmet, and the 2nd patch adds tests/md/001.
On 18/06/2024 11:38, Shinichiro Kawasaki wrote: > On Jun 18, 2024 / 10:45, Ofir Gal wrote: >> >> On 18/06/2024 4:24, Shinichiro Kawasaki wrote: >>> CC+: linux-nvme, Daniel, Chaitanya, >>> >>> On Jun 17, 2024 / 19:05, Ofir Gal wrote: > [...] >>>> nvme-tcp is used as the network device, I'm not sure it's related to >>>> the nvme test group. What do you think? >>> I see... The bug is in md sub-system, then it's the better to have the new test >>> case in the new md test group. To avoid the cross reference, the nvmet related >>> helper functions should move from tests/nvme/rc to common/nvmet, so that this >>> test/md/001 can refer them. This will be another separated, preparation patch. >> Ok, should it be a patch set or two completely separated patches? > I suggest to make them a patch set: the 1st patch moves the nvmet functions from > tests/nvme/rc to common/nvmet, and the 2nd patch adds tests/md/001. Will do, thanks!
diff --git a/common/brd b/common/brd new file mode 100644 index 0000000..b8cd4e3 --- /dev/null +++ b/common/brd @@ -0,0 +1,28 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-3.0+ +# Copyright (C) 2024 Ofir Gal +# +# brd helper functions + +. common/shellcheck + +_have_brd() { + _have_driver brd +} + +_init_brd() { + # _have_brd loads brd, we need to wait a bit for brd to be not in use in + # order to reload it + sleep 0.2 + + if ! modprobe -r brd || ! modprobe brd "$@" ; then + echo "failed to reload brd with args: $*" + return 1 + fi + + return 0 +} + +_cleanup_brd() { + modprobe -r brd +} diff --git a/tests/md/001 b/tests/md/001 new file mode 100755 index 0000000..d5fb755 --- /dev/null +++ b/tests/md/001 @@ -0,0 +1,80 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-3.0+ +# Copyright (C) 2024 Ofir Gal +# +# Regression test for patch "md/md-bitmap: fix writing non bitmap pages" and +# for patch "nvme-tcp: use sendpages_ok() instead of sendpage_ok()" + +. tests/md/rc +. tests/nvme/rc +. common/brd + +DESCRIPTION="Create a raid with bitmap on top of nvme device with +optimal-io-size over bitmap size" +QUICK=1 + +#restrict test to nvme-tcp only +nvme_trtype=tcp +nvmet_blkdev_type="device" + +requires() { + # Require dm-stripe + _have_program dmsetup + _have_driver dm-mod + + _require_nvme_trtype tcp + _have_brd +} + +# Sets up a brd device of 1G with optimal-io-size of 256K +setup_underlying_device() { + if ! _init_brd rd_size=1048576 rd_nr=1; then + return 1 + fi + + dmsetup create ram0_big_optio --table \ + "0 $(blockdev --getsz /dev/ram0) striped 1 512 /dev/ram0 0" +} + +cleanup_underlying_device() { + dmsetup remove ram0_big_optio + _cleanup_brd +} + +# Sets up a local host nvme over tcp +setup_nvme_over_tcp() { + _setup_nvmet + + local port + port="$(_create_nvmet_port "${nvme_trtype}")" + + _create_nvmet_subsystem "blktests-subsystem-0" "/dev/mapper/ram0_big_optio" + _add_nvmet_subsys_to_port "${port}" "blktests-subsystem-0" + + _create_nvmet_host "blktests-subsystem-0" "${def_hostnqn}" + + _nvme_connect_subsys --subsysnqn "blktests-subsystem-0" + nvmedev=$(_find_nvme_dev "blktests-subsystem-0") +} + +cleanup_nvme_over_tcp() { + _nvmet_target_cleanup --subsysnqn "blktests-subsystem-0" +} + +test() { + echo "Running ${TEST_NAME}" + + setup_underlying_device + setup_nvme_over_tcp + + # Hangs here without the fix + mdadm -q --create /dev/md/blktests_md --level=1 --bitmap=internal \ + --bitmap-chunk=1024K --assume-clean --run --raid-devices=2 \ + /dev/"${nvmedev}"n1 missing + + mdadm -q --stop /dev/md/blktests_md + cleanup_nvme_over_tcp + cleanup_underlying_device + + echo "Test complete" +} diff --git a/tests/md/001.out b/tests/md/001.out new file mode 100644 index 0000000..edd2ced --- /dev/null +++ b/tests/md/001.out @@ -0,0 +1,2 @@ +Running md/001 +Test complete diff --git a/tests/md/rc b/tests/md/rc new file mode 100644 index 0000000..d492579 --- /dev/null +++ b/tests/md/rc @@ -0,0 +1,12 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-3.0+ +# Copyright (C) 2024 Ofir Gal +# +# Tests for md raid + +. common/rc + +group_requires() { + _have_root + _have_program mdadm +}
A bug in md-bitmap has been discovered by setting up a md raid on top of nvme-tcp devices that has optimal io size larger than the allocated bitmap. The following test reproduce the bug by setting up a md-raid on top of nvme-tcp device over ram device that sets the optimal io size by using dm-stripe. Signed-off-by: Ofir Gal <ofir.gal@volumez.com> --- This is my first contribution to blktests. The tests hangs when being ran on a kernel without the bugfix. Should we wait for the bugfix to be merged to upstream before merging the test? common/brd | 28 +++++++++++++++++ tests/md/001 | 80 ++++++++++++++++++++++++++++++++++++++++++++++++ tests/md/001.out | 2 ++ tests/md/rc | 12 ++++++++ 4 files changed, 122 insertions(+) create mode 100644 common/brd create mode 100755 tests/md/001 create mode 100644 tests/md/001.out create mode 100644 tests/md/rc