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.
