Re: [PATCH v3] nvmet: add missing lock around nvmet_ns_changed in nvmet_ns_revalidate

[Niels Dossche <dossche.niels@gmail.com>]

On 3/13/22 21:41, Sagi Grimberg wrote:
> 
>>>> My commit message in v2 did not clearly state that nvmet_ns_revalidate has 3 callers, of which
>>>> 2 do not acquire that lock: nvmet_execute_identify_cns_cs_ns and nvmet_execute_identify_ns. The other caller
>>>> nvmet_ns_revalidate_size_store does acquire the lock. Maybe I caused some confusion because of the unclear wording.
>>>
>>> It is simpler to just move that call-site outside of the lock imo.
>>
>> The callsite that has the lock is nvmet_ns_revalidate_size_store. It checks for the enabled flag under that lock.
>> If nvmet_ns_revalidate_size_store calls nvmet_ns_revalidate without that lock taken, but with the lock acquired inside the nvmet_ns_revalidate_size_store function itself, is it not possible that the ns->enabled flag changes in between the ns->enabled check and the call to nvmet_ns_revalidate_size_store? I thought the locking in that function was to also make sure that the enabled flag does not change during the execution of nvmet_ns_revalidate?
> 
> The lock just protects subsys->ctrls traversal, can't see any
> harm with sending 2 aens out of order, twice...
> 

I understand, thanks. I'll send a patch.