Re: [PATCH] nvme-fabrics: open code __nvmf_host_find()

[Max Gurtovoy <mgurtovoy@nvidia.com>]

On 05/06/2023 12:13, Chaitanya Kulkarni wrote:
> On 6/3/23 17:35, Max Gurtovoy wrote:
>> Hi,
>>
>> On 02/06/2023 9:47, Chaitanya Kulkarni wrote:
>>> There is no point in maintaining a separate funciton __nvmf_host_find()
>>
>> typo *function
>>
>>> that has only one caller nvmf_host_add() especially when caller and
>>> callee both are small enough to merge.
>>>
>>> Due to this we are actually repeating the error handling code in both
>>> callee and caller for no reason that can be avoided, but instead we have
>>> to read both function to establish the correctness along with additional
>>> lockdep warning check due to involved locking.
>>>
>>> Just open code __nvmf_host_find() in nvme_host_alloc() with appropriate
>>> comment that removes repeated error checks in the callee/caller and
>>> lockdep check that is needed for the nvmf_hosts_mutex involvement,
>>> diffstats :-
>>
>> The above 2 sentences are redundant IMO.
>> There is no error handling in the callee so it can't be repeated. We
>> just return error in the callee.
> 
> and that is error handling, without error conditions we would
> only returning either valid host (that is non NULL) or NULL in
> absence of host in the list. That -EINVAL return is the reason
> we need separate check with IS_ERR in the caller ...
> 
> -    if (IS_ERR(host)) {
> -        goto out_unlock;
> -    } else if (host) {
> -        kref_get(&host->ref);
> -        goto out_unlock;

this check happens in the caller. No duplication.

> 
>> The first sentence is good enough to justify this patch.
>>
>> Lets have instead:
>>
>> "Merge its code with the only caller nvmf_host_add() since both are
>> small enough.
>> The lockdep check in __nvmf_host_find() after the merge becomes
>> redundant so we can remove it too."
>>
>>
> 
> I don't understand what you are saying, provide a complete commit
> log that can be applied verbatim to this patch.

just say something like:

"
Open code __nvmf_host_find() since there is only one caller for it and 
both are small enough to merge.
As a result, the lockdep check in __nvmf_host_find() after the merge 
becomes redundant so we can remove it too.
"


> 
> -ck
> 
>