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.