Re: [PATCH 4/4] nvmet: make ver stable once connection established

[Max Gurtovoy <mgurtovoy@nvidia.com>]

On 4/20/2021 8:47 PM, Chaitanya Kulkarni wrote:
> On 4/20/21 02:09, Max Gurtovoy wrote:
>> From: Noam Gottlieb <ngottlieb@nvidia.com>
>>
>> Once some host has connected to the nvmf target, make sure that the
>> version number is stable and cannot be changed.
>>
>> Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>> Signed-off-by: Noam Gottlieb <ngottlieb@nvidia.com>
>> ---
>>   drivers/nvme/target/configfs.c | 36 +++++++++++++++++++++++++++++-----
>>   1 file changed, 31 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
>> index 61a9a52da30d..2c7aebd4d529 100644
>> --- a/drivers/nvme/target/configfs.c
>> +++ b/drivers/nvme/target/configfs.c
>> @@ -1007,13 +1007,26 @@ static ssize_t nvmet_subsys_attr_version_show(struct config_item *item,
>>   			NVME_MINOR(subsys->ver));
>>   }
>>   
>> -static ssize_t nvmet_subsys_attr_version_store(struct config_item *item,
>> -					       const char *page, size_t count)
>> +static ssize_t
>> +nvmet_subsys_attr_version_store_locked(struct nvmet_subsys *subsys,
>> +		const char *page, size_t count)
>>   {
> Seems like we lost the indentation in above line which was there previously.

We added a new line since the line was > 80 chars.

And we follow the conventions.


>
>> -	struct nvmet_subsys *subsys = to_subsys(item);
>>   	int major, minor, tertiary = 0;
>>   	int ret;
>>   
>> +	if (subsys->subsys_discovered) {
>> +		if (NVME_TERTIARY(subsys->ver))
>> +			pr_err("Can't set version number. %llu.%llu.%llu is already assigned\n",
>> +			       NVME_MAJOR(subsys->ver),
>> +			       NVME_MINOR(subsys->ver),
>> +			       NVME_TERTIARY(subsys->ver));
>> +		else
>> +			pr_err("Can't set version number. %llu.%llu is already assigned\n",
>> +			       NVME_MAJOR(subsys->ver),
>> +			       NVME_MINOR(subsys->ver));
>> +		return -EINVAL;
>> +	}
>> +
> Can you create a helper to for about error reporting so we can avoid
> extra long lines whenever itis possible something like :-
>
>          if (subsys->subsys_discovered)
>              return nvmet_subsys_discover_err(subsys);

We will create helper in the future in case this code will be re-used.

There is no need to create it now.

Long lines avoided by new lines.


>
>>   	/* passthru subsystems use the underlying controller's version */
>>   	if (nvmet_passthru_ctrl(subsys))
>>   		return -EINVAL;
>> @@ -1022,12 +1035,25 @@ static ssize_t nvmet_subsys_attr_version_store(struct config_item *item,
>>   	if (ret != 2 && ret != 3)
>>   		return -EINVAL;
>>   
>> -	down_write(&nvmet_config_sem);
>>   	subsys->ver = NVME_VS(major, minor, tertiary);
>> -	up_write(&nvmet_config_sem);
>>   
>>   	return count;
>>   }
>> +
>> +static ssize_t nvmet_subsys_attr_version_store(struct config_item *item,
>> +					       const char *page, size_t count)
>> +{
>> +	struct nvmet_subsys *subsys = to_subsys(item);
>> +	int ret;
>> +
> nit:- ret variable type should be ssize_t
> nvmet_subsys_attr_version_store_locke().

Ok. good catch.

Thanks.


>
>> +	down_write(&nvmet_config_sem);
>> +	mutex_lock(&subsys->lock);
>> +	ret = nvmet_subsys_attr_version_store_locked(subsys, page, count);
>> +	mutex_unlock(&subsys->lock);
>> +	up_write(&nvmet_config_sem);
>> +
>> +	return ret;
>> +}
>>   CONFIGFS_ATTR(nvmet_subsys_, attr_version);
>>   
>>   /* See Section 1.5 of NVMe 1.4 */

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme