Re: [PATCH] nvmet: move percpu handling into nvmet_ns_{enable,disable}

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

On 1/24/25 4:39 PM, Hannes Reinecke wrote:
> On 1/24/25 11:50, Sagi Grimberg wrote:
>>
>>
>>
>> On 24/01/2025 10:25, hare@kernel.org wrote:
>>> From: Hannes Reinecke <hare@kernel.org>
>>>
>>> The namespace percpu counter protects pending I/O, and we can
>>> only safely diable the namespace once the counter drop to zero.
>>> So we need to init the percpu counter in nvmet_ns_enable(), and
>>> wait for it to drop to zero in nvmet_ns_disable() to avoid having
>>> I/O pending after the namespace has been disabled.
>>>
>>> Fixes: 74d16965d7ac ("nvmet-loop: avoid using mutex in IO hotpath")
>>
>> Can you please describe the bug and scenario which you are hitting this bug?
>> It is also unclear how the above patch is causing this.
>>
>>  From quick look patch itself looks reasonable.
> 
> Should've said: this is the fix to the reported blktest failure
> on nvme/058...
> 
Thanks Hannes for addressing this issue!

I think while implementing 74d16965d7ac ("nvmet-loop: avoid using mutex in 
IO hotpath"), I moved initialization of ns->ref from nvmet_ns_enable() to 
nvmet_ns_alloc() as well moved killing of ns->ref logic from nvmet_ns_disable() 
to nvmet_ns_free() and that probably caused the observed symptom. I thought we 
need to init ns->ref while we insert ns in Xarray and kill ns->ref while we erase
it from Xarray, but that was not correct. My bad!

Though I was never able to recreate this issue on my setup even after running
nvme/058 hundreds of times, still I think the issue reported by Shinichiro 
appears plausible. So we need this fix and your changes look good to me.

Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>