| Message ID | 20240109104453.3764096-3-shinichiro.kawasaki@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/031: allow to run with built-in null_blk driver | expand |
On 1/9/24 02:44, Shin'ichiro Kawasaki wrote: > The test case block/031 sets null_blk parameter shared_tag_bitmap=1 for > testing. The parameter has been set as a module parameter then null_blk > driver must be loadable. However, null_blk allows to set > shared_tag_bitmap as a configfs parameter since the kernel commit > 7012eef520cb ("null_blk: add configfs variables for 2 options"). Then > the test case now can be run with built-in null_blk driver by specifying > shared_tag_bitmap through configfs. > > Modify the test case for that purpose. Refer null_blk feature list and > check if shared_tag_bitmap can be specified through configfs. If so, > specify the parameter as an option of _configure_null_blk and set it > through configfs. If not, check in requires() that shared_tag_bitmap can > be specified as a module parameter. Then call _init_null_blk() in test() > and specify shared_tag_bitmap=1 at null_blk module load. > > Also change null_blk device name from nullb0 to nullb1 since the default > null_blk device name nullb0 is not usable with built-in null_blk driver. > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > --- > Looks good. Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com> -ck
On 1/9/24 02:44, Shin'ichiro Kawasaki wrote: > The test case block/031 sets null_blk parameter shared_tag_bitmap=1 for > testing. > The parameter has been set as a module parameter then null_blk > driver must be loadable. It seems like the word "If" is missing from the start of the above sentence? > - if ! _init_null_blk nr_devices=0 shared_tag_bitmap=1; then > - echo "Loading null_blk failed" > - return 1 > + if _have_null_blk_feature shared_tag_bitmap; then > + opts+=(shared_tag_bitmap=1) > + else > + # Old kernel requires shared_tag_bitmap as a module parameter > + # instead of configfs parameter. > + if ! _init_null_blk shared_tag_bitmap=1; then > + echo "Loading null_blk failed" > + return 1 > + fi > fi _have_null_blk_feature loads the null_blk kernel module as a side effect. The above code relies on that side effect. I think that _have_null_blk_feature either should unload the null_blk kernel module or that a comment should be added above the above if-statement that explains this side effect. Otherwise readers of this code will wonder why there is an _init_null_blk call in one branch of the if-statement and not in the other branch. Thanks, Bart.
On Jan 09, 2024 / 12:19, Bart Van Assche wrote: > On 1/9/24 02:44, Shin'ichiro Kawasaki wrote: > > The test case block/031 sets null_blk parameter shared_tag_bitmap=1 for > > testing. > > The parameter has been set as a module parameter then null_blk > > driver must be loadable. > > It seems like the word "If" is missing from the start of the above sentence? With this sentence, I wanted to describe current implmenetation fact. Before applying this patch, shared_tag_bitmap=1 was set as a module parameter, not through a configfs file. So I don't think "If" is missing. > > > - if ! _init_null_blk nr_devices=0 shared_tag_bitmap=1; then > > - echo "Loading null_blk failed" > > - return 1 > > + if _have_null_blk_feature shared_tag_bitmap; then > > + opts+=(shared_tag_bitmap=1) > > + else > > + # Old kernel requires shared_tag_bitmap as a module parameter > > + # instead of configfs parameter. > > + if ! _init_null_blk shared_tag_bitmap=1; then > > + echo "Loading null_blk failed" > > + return 1 > > + fi > > fi > > _have_null_blk_feature loads the null_blk kernel module as a side effect. The > above code relies on that side effect. No. Regardless of the null_blk module load status, _init_null_blk() should work in same manner. Actually _init_null_blk() unload null_blk (modprobe -r null_blk) then load null_blk (modprobe null_blk). > I think that _have_null_blk_feature either > should unload the null_blk kernel module or that a comment should be added above > the above if-statement that explains this side effect. Otherwise readers of this > code will wonder why there is an _init_null_blk call in one branch of the > if-statement and not in the other branch. I think the inline comment above was not clear enough and caused the confusion. How about to improve the comment as follows? I hope it explains why _init_null_blk is called in the if-statement. # _configure_null_blk() sets null_blk parameters via configfs, while # _init_null_blk() sets null_blk parameters as module parameters. # Old kernel requires shared_tag_bitmap as a module parameter. In that # case, call _init_null_blk() for shared_tag_bitmap.
On 1/9/24 18:19, Shinichiro Kawasaki wrote: > On Jan 09, 2024 / 12:19, Bart Van Assche wrote: >> On 1/9/24 02:44, Shin'ichiro Kawasaki wrote: >>> The parameter has been set as a module parameter then null_blk >>> driver must be loadable. >> >> It seems like the word "If" is missing from the start of the above sentence? > > With this sentence, I wanted to describe current implmenetation fact. Before > applying this patch, shared_tag_bitmap=1 was set as a module parameter, not > through a configfs file. So I don't think "If" is missing. If I upload the sentence that I quoted to a free online grammar checker (https://quillbot.com/grammar-check) then it tells me that there is a grammatical issue with that sentence. That's why I asked whether a word is perhaps missing. I'm not trying to be pedantic - I raised my question because the above sentence is incomprehensible to me. > I think the inline comment above was not clear enough and caused the confusion. > How about to improve the comment as follows? I hope it explains why > _init_null_blk is called in the if-statement. > > # _configure_null_blk() sets null_blk parameters via configfs, while > # _init_null_blk() sets null_blk parameters as module parameters. > # Old kernel requires shared_tag_bitmap as a module parameter. In that > # case, call _init_null_blk() for shared_tag_bitmap. That sounds better to me. Thanks, Bart.
On Jan 10, 2024 / 09:50, Bart Van Assche wrote: > On 1/9/24 18:19, Shinichiro Kawasaki wrote: > > On Jan 09, 2024 / 12:19, Bart Van Assche wrote: > > > On 1/9/24 02:44, Shin'ichiro Kawasaki wrote: > > > > The parameter has been set as a module parameter then null_blk > > > > driver must be loadable. > > > > > > It seems like the word "If" is missing from the start of the above sentence? > > > > With this sentence, I wanted to describe current implmenetation fact. Before > > applying this patch, shared_tag_bitmap=1 was set as a module parameter, not > > through a configfs file. So I don't think "If" is missing. > > If I upload the sentence that I quoted to a free online grammar checker > (https://quillbot.com/grammar-check) then it tells me that there is a > grammatical issue with that sentence. That's why I asked whether a word is > perhaps missing. I'm not trying to be pedantic - I raised my question because > the above sentence is incomprehensible to me. Thanks for pointing out the incomprehensible sentence. The grammar checker looks useful for improving my writing. I will use it to revise the commit message as well as the code block comments. > > > I think the inline comment above was not clear enough and caused the confusion. > > How about to improve the comment as follows? I hope it explains why > > _init_null_blk is called in the if-statement. > > > > # _configure_null_blk() sets null_blk parameters via configfs, while > > # _init_null_blk() sets null_blk parameters as module parameters. > > # Old kernel requires shared_tag_bitmap as a module parameter. In that > > # case, call _init_null_blk() for shared_tag_bitmap. > > That sounds better to me. > > Thanks, > > Bart. > >
diff --git a/tests/block/031 b/tests/block/031 index d253af8..61a3502 100755 --- a/tests/block/031 +++ b/tests/block/031 @@ -11,30 +11,41 @@ DESCRIPTION="do IO on null-blk with a host tag set" TIMED=1 requires() { - _have_fio && _have_null_blk && _have_module_param null_blk shared_tag_bitmap + _have_fio + _have_null_blk + if ! _have_null_blk_feature shared_tag_bitmap; then + _have_module_param null_blk shared_tag_bitmap + fi } test() { local fio_status bs=512 + local -a opts=(nullb1 completion_nsec=0 blocksize="$bs" size=1 \ + submit_queues="$(nproc)" memory_backed=1) : "${TIMEOUT:=30}" - if ! _init_null_blk nr_devices=0 shared_tag_bitmap=1; then - echo "Loading null_blk failed" - return 1 + if _have_null_blk_feature shared_tag_bitmap; then + opts+=(shared_tag_bitmap=1) + else + # Old kernel requires shared_tag_bitmap as a module parameter + # instead of configfs parameter. + if ! _init_null_blk shared_tag_bitmap=1; then + echo "Loading null_blk failed" + return 1 + fi fi - if ! _configure_null_blk nullb0 completion_nsec=0 blocksize=$bs size=1\ - submit_queues="$(nproc)" memory_backed=1 power=1; then + if ! _configure_null_blk "${opts[@]}" power=1; then echo "Configuring null_blk failed" return 1 fi fio --verify=md5 --rw=randwrite --bs=$bs --loops=$((10**6)) \ --iodepth=64 --group_reporting --sync=1 --direct=1 \ --ioengine=libaio --runtime="${TIMEOUT}" --thread \ - --name=block-031 --filename=/dev/nullb0 \ + --name=block-031 --filename=/dev/nullb1 \ --output="${RESULTS_DIR}/block/fio-output-031.txt" \ >>"$FULL" fio_status=$? - rmdir /sys/kernel/config/nullb/nullb0 + rmdir /sys/kernel/config/nullb/nullb1 _exit_null_blk case $fio_status in 0) echo "Passed";;
The test case block/031 sets null_blk parameter shared_tag_bitmap=1 for testing. The parameter has been set as a module parameter then null_blk driver must be loadable. However, null_blk allows to set shared_tag_bitmap as a configfs parameter since the kernel commit 7012eef520cb ("null_blk: add configfs variables for 2 options"). Then the test case now can be run with built-in null_blk driver by specifying shared_tag_bitmap through configfs. Modify the test case for that purpose. Refer null_blk feature list and check if shared_tag_bitmap can be specified through configfs. If so, specify the parameter as an option of _configure_null_blk and set it through configfs. If not, check in requires() that shared_tag_bitmap can be specified as a module parameter. Then call _init_null_blk() in test() and specify shared_tag_bitmap=1 at null_blk module load. Also change null_blk device name from nullb0 to nullb1 since the default null_blk device name nullb0 is not usable with built-in null_blk driver. Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> --- tests/block/031 | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-)