Re: [PATCHv3] nvme: correctly account for namespace head reference counter

[Nilay Shroff <nilay@linux.ibm.com>]

On 6/26/25 1:18 PM, Daniel Wagner wrote:
> On Thu, Jun 26, 2025 at 10:49:19AM +0530, Nilay Shroff wrote:
>> This change prevents stale kobject entries from lingering in sysfs and
>> eliminates the module reload failures observed just after running
>> nvme/058.
>>
>> [1] https://lore.kernel.org/all/CAHj4cs8fOBS-eSjsd5LUBzy7faKXJtgLkCN+mDy_-ezCLLLq+Q@mail.gmail.com/
>>
>> Reported-by: yi.zhang@redhat.com
>> Closes: https://lore.kernel.org/all/CAHj4cs8fOBS-eSjsd5LUBzy7faKXJtgLkCN+mDy_-ezCLLLq+Q@mail.gmail.com/
>> Fixes: 62188639ec16 ("nvme-multipath: introduce delayed removal of the multipath head node")
>> Tested-by: yi.zhang@redhat.com
>> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
> 
> Reviewed-by: Daniel Wagner <dwagner@suse.de>
> 
>>  		list_del_init(&ns->head->entry);
>> +		/*
>> +		 * If multipath is not configured, we still create a namespace
>> +		 * head (nshead), but head->disk is not initialized in that case.
>> +		 * As a result, only a single reference to nshead is held (via
>> +		 * kref_init()) when it is created. Therefore, ensure that we
>> +		 * do not release the reference to nshead twice if head->disk
>> +		 * is not present.
>> +		 */
>> +		if (ns->head->disk)
>> +			last_path = true;
> 
> just nitpicking: what about renaming the variable to something like
> 'multipath' which matches more with the comment.

The variable @last_path is meant to indicate that we're releasing/removing the last
remaining path to the namespace, which is relevant only when NVMe multipath is enabled
(i.e., ns->head->disk is set). So if we rename @last_path to @multipath, it could
become less intuitive, as the name wouldn't reflect the actual logic — that we're
conditionally releasing the nshead reference only when the last path to the namespace 
is being removed. Hence, in this context, IMO, @last_path is more semantically accurate
and self-explanatory.

Moreover, in nvme_ns_remove() as well we use @last_path in the same context as
mentioned above. 

Thanks,
--Nilay