Re: [PATCH v3 2/2] nvme: fix cdev lifetime

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

On 7/16/26 10:03 PM, John Garry wrote:
> On 16/07/2026 16:03, Nilay Shroff wrote:
>> On 7/13/26 4: 12 PM, John Garry wrote: > Fix the lifetime for the cdev by making
>> adding the cdev add take a > reference to the NS head and drop that reference in
>> the > nvme_ns_head. cdev_device release function. > > The same problem
>>
>>
>> On 7/13/26 4:12 PM, John Garry wrote:
>>> Fix the lifetime for the cdev by making adding the cdev add take a
>>> reference to the NS head and drop that reference in the
>>> nvme_ns_head.cdev_device release function.
>>>
>>> The same problem exists for the NS cdev lifetime, so resolve that issue
>>> through a similar method by taking a reference to the NS for the lifetime
>>> of the cdev. Note that nvme_ns_chr_open() -> nvme_ns_open() also takes a
>>> reference to the NS. Now that should not be needed, but that code is
>>> common to bdev ioctl, so keep as is.
>>
>> The bdev ioctl uses nvme_ns_open() and nvme_ns_release(). So, IMO, you may also
>> remove nvme_ns_chr_open() and nvme_ns_chr_release() methods.
> 
> nvme_ns_chr_open() and nvme_ns_chr_release() call nvme_ns_open() and nvme_ns_relase(), respectively, and they do more than get and put a ref to the NS - specifically they also check that they are not called for multipath mode (as the NS bdev/cdev should be hidden) and take/put a reference to the controller ops module. Why are those addition actions not required for the cdev? Or should it be done when we add/del the cdev (like in this patch)?
> 

I think the first check in nvme_ns_open() that verifies we have not entered
while multipath is enabled is essentially a sanity/paranoia check.

As for the module reference, I think that is redundant for the cdev path.
chrdev_open() already takes a reference to module before invoking
nvme_ns_chr_open(), so IMO, nvme_ns_open() taking another reference to
module appears unnecessary for this path.

Unless I'm missing something, it seems nvme_ns_chr_open()/nvme_ns_chr_release()
don't provide any additional lifetime guarantees for the cdev beyond what is
already handled elsewhere.

Thanks,
--Nilay