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

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

[Niels Dossche]

nvmet_ns_changed states via lockdep that the ns->subsys->lock must be
held. The only caller of nvmet_ns_changed which does not acquire that
lock is nvmet_ns_revalidate. 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. Add a parameter to
nvmet_ns_revalidate to indicate whether the lock was already taken or
not, and thus whether the function still needs to take a lock when
calling nvmet_ns_changed.

The alternative solution is to let nvmet_ns_revalidate return a bool
which indicates whether nvmet_ns_changed needs to be called and let the
callers handle the locking responsibility. This however places the
responsibility with its callers and causes more duplicate code and
potential to forget to check the return value.

Both of those identify functions are called from a common function
nvmet_execute_identify, which itself is called indirectly via the
req->execute function pointer.

This issue was found using a static type-based analyser and manually
verified.

Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
---

Changes in v3:
 - improve commit description
 - do the locking locally

Changes in v2:
 - added sentence about how the issue was found.
 - added missing &

 drivers/nvme/target/admin-cmd.c | 2 +-
 drivers/nvme/target/configfs.c  | 2 +-
 drivers/nvme/target/core.c      | 9 +++++++--
 drivers/nvme/target/nvmet.h     | 2 +-
 drivers/nvme/target/zns.c       | 3 ++-
 5 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 6fb24746de06..efa462374783 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -511,7 +511,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 		goto done;
 	}
 
-	nvmet_ns_revalidate(req->ns);
+	nvmet_ns_revalidate(req->ns, true);
 
 	/*
 	 * nuse = ncap = nsze isn't always true, but we have no way to find
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 091a0ca16361..a803cd66dc4b 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -586,7 +586,7 @@ static ssize_t nvmet_ns_revalidate_size_store(struct config_item *item,
 		mutex_unlock(&ns->subsys->lock);
 		return -EINVAL;
 	}
-	nvmet_ns_revalidate(ns);
+	nvmet_ns_revalidate(ns, false);
 	mutex_unlock(&ns->subsys->lock);
 	return count;
 }
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 5119c687de68..0ceef97e4093 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -531,7 +531,7 @@ static void nvmet_p2pmem_ns_add_p2p(struct nvmet_ctrl *ctrl,
 		ns->nsid);
 }
 
-void nvmet_ns_revalidate(struct nvmet_ns *ns)
+void nvmet_ns_revalidate(struct nvmet_ns *ns, bool should_acquire_lock)
 {
 	loff_t oldsize = ns->size;
 
@@ -540,8 +540,13 @@ void nvmet_ns_revalidate(struct nvmet_ns *ns)
 	else
 		nvmet_file_ns_revalidate(ns);
 
-	if (oldsize != ns->size)
+	if (oldsize != ns->size) {
+		if (should_acquire_lock)
+			mutex_lock(&ns->subsys->lock);
 		nvmet_ns_changed(ns->subsys, ns->nsid);
+		if (should_acquire_lock)
+			mutex_unlock(&ns->subsys->lock);
+	}
 }
 
 int nvmet_ns_enable(struct nvmet_ns *ns)
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index af193423c10b..e4f20fe95613 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -542,7 +542,7 @@ u16 nvmet_file_flush(struct nvmet_req *req);
 void nvmet_ns_changed(struct nvmet_subsys *subsys, u32 nsid);
 void nvmet_bdev_ns_revalidate(struct nvmet_ns *ns);
 int nvmet_file_ns_revalidate(struct nvmet_ns *ns);
-void nvmet_ns_revalidate(struct nvmet_ns *ns);
+void nvmet_ns_revalidate(struct nvmet_ns *ns, bool should_acquire_lock);
 u16 blk_to_nvme_status(struct nvmet_req *req, blk_status_t blk_sts);
 
 bool nvmet_bdev_zns_enable(struct nvmet_ns *ns);
diff --git a/drivers/nvme/target/zns.c b/drivers/nvme/target/zns.c
index 46bc30fe85d2..1987358bc855 100644
--- a/drivers/nvme/target/zns.c
+++ b/drivers/nvme/target/zns.c
@@ -123,7 +123,8 @@ void nvmet_execute_identify_cns_cs_ns(struct nvmet_req *req)
 		goto done;
 	}
 
-	nvmet_ns_revalidate(req->ns);
+	nvmet_ns_revalidate(req->ns, true);
+
 	zsze = (bdev_zone_sectors(req->ns->bdev) << 9) >>
 					req->ns->blksize_shift;
 	id_zns->lbafe[0].zsze = cpu_to_le64(zsze);
-- 
2.35.1

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

[Sagi Grimberg]

On 3/10/22 14:51, Niels Dossche wrote:
> nvmet_ns_changed states via lockdep that the ns->subsys->lock must be
> held. The only caller of nvmet_ns_changed which does not acquire that
> lock is nvmet_ns_revalidate. 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. Add a parameter to
> nvmet_ns_revalidate to indicate whether the lock was already taken or
> not, and thus whether the function still needs to take a lock when
> calling nvmet_ns_changed.
> 
> The alternative solution is to let nvmet_ns_revalidate return a bool
> which indicates whether nvmet_ns_changed needs to be called and let the
> callers handle the locking responsibility. This however places the
> responsibility with its callers and causes more duplicate code and
> potential to forget to check the return value.
> 
> Both of those identify functions are called from a common function
> nvmet_execute_identify, which itself is called indirectly via the
> req->execute function pointer.
> 
> This issue was found using a static type-based analyser and manually
> verified.
> 
> Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
> ---
> 
> Changes in v3:
>   - improve commit description
>   - do the locking locally
> 
> Changes in v2:
>   - added sentence about how the issue was found.
>   - added missing &
> 
>   drivers/nvme/target/admin-cmd.c | 2 +-
>   drivers/nvme/target/configfs.c  | 2 +-
>   drivers/nvme/target/core.c      | 9 +++++++--
>   drivers/nvme/target/nvmet.h     | 2 +-
>   drivers/nvme/target/zns.c       | 3 ++-
>   5 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 6fb24746de06..efa462374783 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -511,7 +511,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
>   		goto done;
>   	}
>   
> -	nvmet_ns_revalidate(req->ns);
> +	nvmet_ns_revalidate(req->ns, true);
>   
>   	/*
>   	 * nuse = ncap = nsze isn't always true, but we have no way to find
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 091a0ca16361..a803cd66dc4b 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -586,7 +586,7 @@ static ssize_t nvmet_ns_revalidate_size_store(struct config_item *item,
>   		mutex_unlock(&ns->subsys->lock);
>   		return -EINVAL;
>   	}
> -	nvmet_ns_revalidate(ns);
> +	nvmet_ns_revalidate(ns, false);
>   	mutex_unlock(&ns->subsys->lock);
>   	return count;
>   }
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 5119c687de68..0ceef97e4093 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -531,7 +531,7 @@ static void nvmet_p2pmem_ns_add_p2p(struct nvmet_ctrl *ctrl,
>   		ns->nsid);
>   }
>   
> -void nvmet_ns_revalidate(struct nvmet_ns *ns)
> +void nvmet_ns_revalidate(struct nvmet_ns *ns, bool should_acquire_lock)
>   {
>   	loff_t oldsize = ns->size;
>   
> @@ -540,8 +540,13 @@ void nvmet_ns_revalidate(struct nvmet_ns *ns)
>   	else
>   		nvmet_file_ns_revalidate(ns);
>   
> -	if (oldsize != ns->size)
> +	if (oldsize != ns->size) {
> +		if (should_acquire_lock)
> +			mutex_lock(&ns->subsys->lock);
>   		nvmet_ns_changed(ns->subsys, ns->nsid);
> +		if (should_acquire_lock)
> +			mutex_unlock(&ns->subsys->lock);
> +	}

What is the harm locking it always and avoid the conditional?

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

[Niels Dossche]

On 3/13/22 14:03, Sagi Grimberg wrote:
>
>
> On 3/10/22 14:51, Niels Dossche wrote:
>> nvmet_ns_changed states via lockdep that the ns->subsys->lock must be
>> held. The only caller of nvmet_ns_changed which does not acquire that
>> lock is nvmet_ns_revalidate. 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. Add a parameter to
>> nvmet_ns_revalidate to indicate whether the lock was already taken or
>> not, and thus whether the function still needs to take a lock when
>> calling nvmet_ns_changed.
>> 
>> The alternative solution is to let nvmet_ns_revalidate return a bool
>> which indicates whether nvmet_ns_changed needs to be called and let the
>> callers handle the locking responsibility. This however places the
>> responsibility with its callers and causes more duplicate code and
>> potential to forget to check the return value.
>> 
>> Both of those identify functions are called from a common function
>> nvmet_execute_identify, which itself is called indirectly via the
>> req->execute function pointer.
>> 
>> This issue was found using a static type-based analyser and manually
>> verified.
>> 
>> Signed-off-by: Niels Dossche <dossche.niels at gmail.com>
>> ---
>> 
>> Changes in v3:
>>   - improve commit description
>>   - do the locking locally
>> 
>> Changes in v2:
>>   - added sentence about how the issue was found.
>>   - added missing &
>> 
>>   drivers/nvme/target/admin-cmd.c | 2 +-
>>   drivers/nvme/target/configfs.c  | 2 +-
>>   drivers/nvme/target/core.c      | 9 +++++++--
>>   drivers/nvme/target/nvmet.h     | 2 +-
>>   drivers/nvme/target/zns.c       | 3 ++-
>>   5 files changed, 12 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
>> index 6fb24746de06..efa462374783 100644
>> --- a/drivers/nvme/target/admin-cmd.c
>> +++ b/drivers/nvme/target/admin-cmd.c
>> @@ -511,7 +511,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
>>   		goto done;
>>   	}
>>   
>> -	nvmet_ns_revalidate(req->ns);
>> +	nvmet_ns_revalidate(req->ns, true);
>>   
>>   	/*
>>   	 * nuse = ncap = nsze isn't always true, but we have no way to find
>> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
>> index 091a0ca16361..a803cd66dc4b 100644
>> --- a/drivers/nvme/target/configfs.c
>> +++ b/drivers/nvme/target/configfs.c
>> @@ -586,7 +586,7 @@ static ssize_t nvmet_ns_revalidate_size_store(struct config_item *item,
>>   		mutex_unlock(&ns->subsys->lock);
>>   		return -EINVAL;
>>   	}
>> -	nvmet_ns_revalidate(ns);
>> +	nvmet_ns_revalidate(ns, false);
>>   	mutex_unlock(&ns->subsys->lock);
>>   	return count;
>>   }
>> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
>> index 5119c687de68..0ceef97e4093 100644
>> --- a/drivers/nvme/target/core.c
>> +++ b/drivers/nvme/target/core.c
>> @@ -531,7 +531,7 @@ static void nvmet_p2pmem_ns_add_p2p(struct nvmet_ctrl *ctrl,
>>   		ns->nsid);
>>   }
>>   
>> -void nvmet_ns_revalidate(struct nvmet_ns *ns)
>> +void nvmet_ns_revalidate(struct nvmet_ns *ns, bool should_acquire_lock)
>>   {
>>   	loff_t oldsize = ns->size;
>>   
>> @@ -540,8 +540,13 @@ void nvmet_ns_revalidate(struct nvmet_ns *ns)
>>   	else
>>   		nvmet_file_ns_revalidate(ns);
>>   
>> -	if (oldsize != ns->size)
>> +	if (oldsize != ns->size) {
>> +		if (should_acquire_lock)
>> +			mutex_lock(&ns->subsys->lock);
>>   		nvmet_ns_changed(ns->subsys, ns->nsid);
>> +		if (should_acquire_lock)
>> +			mutex_unlock(&ns->subsys->lock);
>> +	}
> 
> What is the harm locking it always and avoid the conditional?

In my patch v2 submission I wrote the following text in my commit message:
> nvmet_ns_changed states via lockdep that the ns->subsys->lock must be
> held. The only caller of nvmet_ns_changed which does not acquire that
> lock is nvmet_ns_revalidate.
on which Christoph Hellwig replied:
> So acquire it in nvmet_ns_revalidate only when we actually call
> nvmet_ns_changed.  Otherwise we take a subsystem-wide lock for every
> Identify Namespace all.

Therefore, I changed it to a conditional lock in this patch submission.

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.

Thanks

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

[Sagi Grimberg]

On 3/13/22 15:14, Niels Dossche wrote:
> On 3/13/22 14:03, Sagi Grimberg wrote:
>>
>>
>> On 3/10/22 14:51, Niels Dossche wrote:
>>> nvmet_ns_changed states via lockdep that the ns->subsys->lock must be
>>> held. The only caller of nvmet_ns_changed which does not acquire that
>>> lock is nvmet_ns_revalidate. 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. Add a parameter to
>>> nvmet_ns_revalidate to indicate whether the lock was already taken or
>>> not, and thus whether the function still needs to take a lock when
>>> calling nvmet_ns_changed.
>>>
>>> The alternative solution is to let nvmet_ns_revalidate return a bool
>>> which indicates whether nvmet_ns_changed needs to be called and let the
>>> callers handle the locking responsibility. This however places the
>>> responsibility with its callers and causes more duplicate code and
>>> potential to forget to check the return value.
>>>
>>> Both of those identify functions are called from a common function
>>> nvmet_execute_identify, which itself is called indirectly via the
>>> req->execute function pointer.
>>>
>>> This issue was found using a static type-based analyser and manually
>>> verified.
>>>
>>> Signed-off-by: Niels Dossche <dossche.niels at gmail.com>
>>> ---
>>>
>>> Changes in v3:
>>>    - improve commit description
>>>    - do the locking locally
>>>
>>> Changes in v2:
>>>    - added sentence about how the issue was found.
>>>    - added missing &
>>>
>>>    drivers/nvme/target/admin-cmd.c | 2 +-
>>>    drivers/nvme/target/configfs.c  | 2 +-
>>>    drivers/nvme/target/core.c      | 9 +++++++--
>>>    drivers/nvme/target/nvmet.h     | 2 +-
>>>    drivers/nvme/target/zns.c       | 3 ++-
>>>    5 files changed, 12 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
>>> index 6fb24746de06..efa462374783 100644
>>> --- a/drivers/nvme/target/admin-cmd.c
>>> +++ b/drivers/nvme/target/admin-cmd.c
>>> @@ -511,7 +511,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
>>>    		goto done;
>>>    	}
>>>    
>>> -	nvmet_ns_revalidate(req->ns);
>>> +	nvmet_ns_revalidate(req->ns, true);
>>>    
>>>    	/*
>>>    	 * nuse = ncap = nsze isn't always true, but we have no way to find
>>> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
>>> index 091a0ca16361..a803cd66dc4b 100644
>>> --- a/drivers/nvme/target/configfs.c
>>> +++ b/drivers/nvme/target/configfs.c
>>> @@ -586,7 +586,7 @@ static ssize_t nvmet_ns_revalidate_size_store(struct config_item *item,
>>>    		mutex_unlock(&ns->subsys->lock);
>>>    		return -EINVAL;
>>>    	}
>>> -	nvmet_ns_revalidate(ns);
>>> +	nvmet_ns_revalidate(ns, false);
>>>    	mutex_unlock(&ns->subsys->lock);
>>>    	return count;
>>>    }
>>> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
>>> index 5119c687de68..0ceef97e4093 100644
>>> --- a/drivers/nvme/target/core.c
>>> +++ b/drivers/nvme/target/core.c
>>> @@ -531,7 +531,7 @@ static void nvmet_p2pmem_ns_add_p2p(struct nvmet_ctrl *ctrl,
>>>    		ns->nsid);
>>>    }
>>>    
>>> -void nvmet_ns_revalidate(struct nvmet_ns *ns)
>>> +void nvmet_ns_revalidate(struct nvmet_ns *ns, bool should_acquire_lock)
>>>    {
>>>    	loff_t oldsize = ns->size;
>>>    
>>> @@ -540,8 +540,13 @@ void nvmet_ns_revalidate(struct nvmet_ns *ns)
>>>    	else
>>>    		nvmet_file_ns_revalidate(ns);
>>>    
>>> -	if (oldsize != ns->size)
>>> +	if (oldsize != ns->size) {
>>> +		if (should_acquire_lock)
>>> +			mutex_lock(&ns->subsys->lock);
>>>    		nvmet_ns_changed(ns->subsys, ns->nsid);
>>> +		if (should_acquire_lock)
>>> +			mutex_unlock(&ns->subsys->lock);
>>> +	}
>>
>> What is the harm locking it always and avoid the conditional?
> 
> In my patch v2 submission I wrote the following text in my commit message:
>> nvmet_ns_changed states via lockdep that the ns->subsys->lock must be
>> held. The only caller of nvmet_ns_changed which does not acquire that
>> lock is nvmet_ns_revalidate.
> on which Christoph Hellwig replied:
>> So acquire it in nvmet_ns_revalidate only when we actually call
>> nvmet_ns_changed.  Otherwise we take a subsystem-wide lock for every
>> Identify Namespace all.

Yea, only wrap nvmet_ns_changed, but always.

> 
> Therefore, I changed it to a conditional lock in this patch submission.
> 
> 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.

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

[Niels Dossche]

On 3/13/22 14:31, Sagi Grimberg wrote:
> 
> 
> On 3/13/22 15:14, Niels Dossche wrote:
>> On 3/13/22 14:03, Sagi Grimberg wrote:
>>>
>>>
>>> On 3/10/22 14:51, Niels Dossche wrote:
>>>> nvmet_ns_changed states via lockdep that the ns->subsys->lock must be
>>>> held. The only caller of nvmet_ns_changed which does not acquire that
>>>> lock is nvmet_ns_revalidate. 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. Add a parameter to
>>>> nvmet_ns_revalidate to indicate whether the lock was already taken or
>>>> not, and thus whether the function still needs to take a lock when
>>>> calling nvmet_ns_changed.
>>>> 
>>>> The alternative solution is to let nvmet_ns_revalidate return a bool
>>>> which indicates whether nvmet_ns_changed needs to be called and let the
>>>> callers handle the locking responsibility. This however places the
>>>> responsibility with its callers and causes more duplicate code and
>>>> potential to forget to check the return value.
>>>> 
>>>> Both of those identify functions are called from a common function
>>>> nvmet_execute_identify, which itself is called indirectly via the
>>>> req->execute function pointer.
>>>> 
>>>> This issue was found using a static type-based analyser and manually
>>>> verified.
>>>> 
>>>> Signed-off-by: Niels Dossche <dossche.niels at gmail.com>
>>>> ---
>>>> 
>>>> Changes in v3:
>>>>   - improve commit description
>>>>   - do the locking locally
>>>> 
>>>> Changes in v2:
>>>>   - added sentence about how the issue was found.
>>>>   - added missing &
>>>> 
>>>>   drivers/nvme/target/admin-cmd.c | 2 +-
>>>>   drivers/nvme/target/configfs.c  | 2 +-
>>>>   drivers/nvme/target/core.c      | 9 +++++++--
>>>>   drivers/nvme/target/nvmet.h     | 2 +-
>>>>   drivers/nvme/target/zns.c       | 3 ++-
>>>>   5 files changed, 12 insertions(+), 6 deletions(-)
>>>> 
>>>> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
>>>> index 6fb24746de06..efa462374783 100644
>>>> --- a/drivers/nvme/target/admin-cmd.c
>>>> +++ b/drivers/nvme/target/admin-cmd.c
>>>> @@ -511,7 +511,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
>>>>   		goto done;
>>>>   	}
>>>>   
>>>> -	nvmet_ns_revalidate(req->ns);
>>>> +	nvmet_ns_revalidate(req->ns, true);
>>>>   
>>>>   	/*
>>>>   	 * nuse = ncap = nsze isn't always true, but we have no way to find
>>>> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
>>>> index 091a0ca16361..a803cd66dc4b 100644
>>>> --- a/drivers/nvme/target/configfs.c
>>>> +++ b/drivers/nvme/target/configfs.c
>>>> @@ -586,7 +586,7 @@ static ssize_t nvmet_ns_revalidate_size_store(struct config_item *item,
>>>>   		mutex_unlock(&ns->subsys->lock);
>>>>   		return -EINVAL;
>>>>   	}
>>>> -	nvmet_ns_revalidate(ns);
>>>> +	nvmet_ns_revalidate(ns, false);
>>>>   	mutex_unlock(&ns->subsys->lock);
>>>>   	return count;
>>>>   }
>>>> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
>>>> index 5119c687de68..0ceef97e4093 100644
>>>> --- a/drivers/nvme/target/core.c
>>>> +++ b/drivers/nvme/target/core.c
>>>> @@ -531,7 +531,7 @@ static void nvmet_p2pmem_ns_add_p2p(struct nvmet_ctrl *ctrl,
>>>>   		ns->nsid);
>>>>   }
>>>>   
>>>> -void nvmet_ns_revalidate(struct nvmet_ns *ns)
>>>> +void nvmet_ns_revalidate(struct nvmet_ns *ns, bool should_acquire_lock)
>>>>   {
>>>>   	loff_t oldsize = ns->size;
>>>>   
>>>> @@ -540,8 +540,13 @@ void nvmet_ns_revalidate(struct nvmet_ns *ns)
>>>>   	else
>>>>   		nvmet_file_ns_revalidate(ns);
>>>>   
>>>> -	if (oldsize != ns->size)
>>>> +	if (oldsize != ns->size) {
>>>> +		if (should_acquire_lock)
>>>> +			mutex_lock(&ns->subsys->lock);
>>>>   		nvmet_ns_changed(ns->subsys, ns->nsid);
>>>> +		if (should_acquire_lock)
>>>> +			mutex_unlock(&ns->subsys->lock);
>>>> +	}
>>>
>>> What is the harm locking it always and avoid the conditional?
>>
>> In my patch v2 submission I wrote the following text in my commit message:
>>> nvmet_ns_changed states via lockdep that the ns->subsys->lock must be
>>> held. The only caller of nvmet_ns_changed which does not acquire that
>>> lock is nvmet_ns_revalidate.
>> on which Christoph Hellwig replied:
>>> So acquire it in nvmet_ns_revalidate only when we actually call
>>> nvmet_ns_changed.  Otherwise we take a subsystem-wide lock for every
>>> Identify Namespace all.
> 
> Yea, only wrap nvmet_ns_changed, but always.
> 

Alright, I can send in a patch that does it like that.

>>
>> Therefore, I changed it to a conditional lock in this patch submission.
>>
>> 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?

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

[Sagi Grimberg]

>>> 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...

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

[Niels Dossche]

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.