[PATCH v2 2/4] nvmet: make sn stable once connection was established

[Max Gurtovoy]

On 6/7/2021 11:37 PM, Chaitanya Kulkarni wrote:
> Max,
>
> On 6/7/21 02:23, Max Gurtovoy wrote:
>> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
>> index 0ae809ca428c..bd0a0b91d843 100644
>> --- a/drivers/nvme/target/nvmet.h
>> +++ b/drivers/nvme/target/nvmet.h
>> @@ -231,6 +231,7 @@ struct nvmet_subsys {
>>   
>>   	u64			ver;
>>   	char			serial[NVMET_SN_MAX_SIZE];
>> +	bool			subsys_discovered;
> When subsystem is discovered (connected to the host) we will have non empty
> controller list, instead of adding a new bool variable why not use
> something like following tested patch on the top of this one ?

No it's not the same. The controller list can be empty if initiator_A 
performed connect + disconnect.

In your solution, it is allowed to change the sn after the disconnection 
but in the original solution we don't allow this.

Once *any* initiator connected to this subsystem, the serial nmber will 
not be changed anymore.


> (that also fixes the indentation of
> nvmet_subsys_attr_serial_store_locked()).
>
> diff --git a/drivers/nvme/target/admin-cmd.c
> b/drivers/nvme/target/admin-cmd.c
> index bec84c231d37..897f419be618 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -366,12 +366,6 @@ static void nvmet_execute_identify_ctrl(struct
> nvmet_req *req)
>          u32 cmd_capsule_size;
>          u16 status = 0;
>   
> -       if (!subsys->subsys_discovered) {
> -               mutex_lock(&subsys->lock);
> -               subsys->subsys_discovered = true;
> -               mutex_unlock(&subsys->lock);
> -       }
> -
>          /*
>           * If there is no model number yet, set it now.  It will then remain
>           * stable for the life time of the subsystem.
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index a13da86fb374..94e23dca763f 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -1046,11 +1046,11 @@ static ssize_t
> nvmet_subsys_attr_serial_show(struct config_item *item,
>   
>   static ssize_t
>   nvmet_subsys_attr_serial_store_locked(struct nvmet_subsys *subsys,
> -               const char *page, size_t count)
> +                                     const char *page, size_t count)
>   {
>          int pos, len = strcspn(page, "\n");
>   
> -       if (subsys->subsys_discovered) {
> +       if (!list_empty(&subsys->ctrls)) {
>                  pr_err("Can't set serial number. %s is already assigned\n",
>                         subsys->serial);
>                  return -EINVAL;
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index f7fbf9690ce8..73c6c4dc8997 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -232,7 +232,6 @@ struct nvmet_subsys {
>   
>          u64                     ver;
>          char                    serial[NVMET_SN_MAX_SIZE];
> -       bool                    subsys_discovered;
>          char                    *subsysnqn;
>          bool                    pi_support;
>   
>
> # nvme list
> Node                  SN
> Model                                    Namespace
> Usage                      Format           FW Rev
> --------------------- --------------------
> ---------------------------------------- ---------
> -------------------------- ---------------- --------
> /dev/nvme0n1          foo                  QEMU NVMe
> Ctrl                           1           1.07  GB /   1.07  GB
> 512   B +  0 B   1.0
> /dev/nvme1n1          bf854da611a264d448c2
> Linux                                    1           2.15  GB /   2.15
> GB      4 KiB +  0 B   5.13.0-r
> # echo 1 > /sys/kernel/config/nvmet/subsystems/nqn/
> allowed_hosts/       attr_cntlid_min      attr_serial          passthru/
> attr_allow_any_host  attr_model           attr_version
> attr_cntlid_max      attr_pi_enable       namespaces/
>   # echo "1234567890" > /sys/kernel/config/nvmet/subsystems/nqn/attr_serial
> bash: echo: write error: Invalid argument
> # dmesg  -c
> [311081.155148] nvmet: Can't set serial number. bf854da611a264d448c2 is
> already assigned
>
>