Re: [RFC PATCH 01/21] block: add and use init tagset helper

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

On 10/5/22 10:22, Chaitanya Kulkarni wrote:
> On 10/5/22 09:54, Bart Van Assche wrote:
>> On 10/5/22 02:47, Ulf Hansson wrote:
>>> On Wed, 5 Oct 2022 at 07:11, Damien Le Moal
>>> <damien.lemoal@opensource.wdc.com> wrote:
>>>> On 10/5/22 12:22, Chaitanya Kulkarni wrote:
>>>>> +void blk_mq_init_tag_set(struct blk_mq_tag_set *set,
>>>>> +             const struct blk_mq_ops *ops, unsigned int nr_hw_queues,
>>>>> +             unsigned int queue_depth, unsigned int cmd_size, int
>>>>> numa_node,
>>>>> +             unsigned int timeout, unsigned int flags, void
>>>>> *driver_data)
>>>>
>>>> That is an awful lot of arguments... I would be tempted to say pack all
>>>> these into a struct but then that would kind of negate this patchset
>>>> goal.
>>>> Using a function with that many arguments will be error prone, and
>>>> hard to
>>>> review... Not a fan.
>>>
>>> I completely agree.
>>>
>>> But there is also another problem going down this route. If/when we
>>> realize that there is another parameter needed in the blk_mq_tag_set.
>>> Today that's quite easy to add (assuming the parameter can be
>>> optional), without changing the blk_mq_init_tag_set() interface.
>>
>> Hi Chaitanya,
>>
>> Please consider to drop the entire patch series. In addition to the
>> disadvantages mentioned above I'd like to mention the following
>> disadvantages:
>> * Replacing named member assignments with positional arguments in a
>>     function call makes code harder to read and harder to verify.
>> * This patch series makes tree-wide changes without improving the code
>>     in a substantial way.
>>
>> Thanks,
>>
>> Bart.
>>
> 
> Thanks for the feedback, will drop it.
> 
> -ck
> 

Actually I take that back.

The problems listed here are:-
* Long argument list, hard to verify (Damien,Bart):-
   solution is to make smaller like other function bio_alloc_bioset()
   blk_next_bio() present in the tree.

* Not future proof if tag_set gets new member that needs to be
   initialized increasing the argument list of the new API. (Ulf):-
   solution is to only use common and mandatory members which are
   necessary as new API args, so if new members gets added it will not
   affect the API, that also trims down the argument list.

I will trim down the argument list with the most common arguments
and keep it to max 4-5 mandatory arguments identical to what we
have done this for blk_next_bio() and bio_alloc_bioset() [1]
where mandatory arguments are part of the initialization API
than repeating the code all the in the tree, that creates
maintenance work of treewide patches.

Also, instead of doing tree wide change in series I'll start small
and gradually add more patches over time.

This definitely adds a more value to the code where code is not
repeated for mandatory arguments, which are way less than 9.

-ck

[1]

8c16567d867ed bio_alloc_bioset() 5 arguments.
0a3140ea0fae3 blk_next_bio() 5 arguments.