Nvmet discovery implementation. Questions and a patch / proposal.

[Mark Ruijter]

>> See inline,

    But: the current implementation only has a _single_ discovery subsystem 
    (nvmet_disc_subsys), which is shared across all configured subsystems.

>> The patch that I provided (slightly improved patch attached) doesn't change this?
>> It merely removes the the attr_discovery_nqn the subsystems and adds it to the ports.
>> Instead of having an attr_discovery_nqn in every subsystem, this changes it to having the attribute in the port section.
>> Maybe the discovery nqn should be moved to /sys/kernel/config/nvmet/discovery/attr_discovery_nqn.
>> This would make it clear that it is a global setting.

    And there's a filter when generating the discovery log page such that 
    only the information for the current subsystem is returned (cf 
    nvmet_execute_disc_get_log_page()).
    So in effect each subsystem has its own discovery subsystem.

>> Good to know. As far as I can tell this still works with the change?

    Moving the discovery nqn to the port will only confuse matters, as then 
    it's quite unclear what will happen if you change the discovery 
    subsystem nqn on a per-port basis; with the current design you would
    change it for _all_ discovery subsystem ...

>> Nothing changes in this respect when you use the attached patch?
>> When changing the param_discovery_nqn attribute of the port, it still changes _all_discovery subsystems for all ports & subsystems.

>> Note that the current code tries to prevent the discovery nqn to be identical to the subsystem nqn.
>> However, the check is flawed since when you change it from another subsystem you can still end up with collisions.

Correct behavior:         
root at saturn:/sys/kernel/config/nvmet/subsystems# echo "nqn.2020-11.com.somesys:uuid:ubuntu-vg--ubuntu-lv1" > nqn.2020-11.com.somesys\:uuid\:ubuntu-vg--ubuntu-lv1/attr_discovery_nqn 
-bash: echo: write error: Device or resource busy

Creating a collision by using another subsystem:
root at saturn:/sys/kernel/config/nvmet/subsystems# echo "nqn.2020-11.com.somesys:uuid:ubuntu-vg--ubuntu-lv1" > nqn.2020-11.com.somesys\:uuid\:ubuntu-vg-ubuntu-lv2/attr_discovery_nqn 
root at saturn:/sys/kernel/config/nvmet/subsystems#

>> The change I made does not solve this problem. But it should be fairly easy to check all existing subsystems nqns for a conflicting name.

>> This example illustrates that with the attached patch changing the discovery_nqn in one port also changes it for every subsystem and other port.

root at saturn:/# nvme discover -t tcp -a 172.16.50.151 -s 4420 

Discovery Log Number of Records 2, Generation counter 6
=====Discovery Log Entry 0======
trtype:  tcp
adrfam:  ipv4
subtype: unrecognized
treq:    not specified, sq flow control disable supported
portid:  10030
trsvcid: 4420
subnqn:  nqn.2014-08.org.nvmexpress.discovery
traddr:  172.16.50.151
sectype: none
=====Discovery Log Entry 1======
trtype:  tcp
adrfam:  ipv4
subtype: nvme subsystem
treq:    not specified, sq flow control disable supported
portid:  10030
trsvcid: 4420
subnqn:  nqn.2020-11.com.somesys:uuid:ubuntu-vg--ubuntu-lv
traddr:  172.16.50.151
sectype: none
root at saturn:/# nvme discover -t tcp -a 10.100.10.1 -s 4420 

Discovery Log Number of Records 2, Generation counter 6
=====Discovery Log Entry 0======
trtype:  tcp
adrfam:  ipv4
subtype: unrecognized
treq:    not specified, sq flow control disable supported
portid:  10120
trsvcid: 4420
subnqn:  nqn.2014-08.org.nvmexpress.discovery
traddr:  10.100.10.1
sectype: none
=====Discovery Log Entry 1======
trtype:  tcp
adrfam:  ipv4
subtype: nvme subsystem
treq:    not specified, sq flow control disable supported
portid:  10120
trsvcid: 4420
subnqn:  nqn.2020-11.com.somesys:uuid:ubuntu-vg--ubuntu-lv
traddr:  10.100.10.1
sectype: none
root at saturn:/# echo nqn.2014-08.org.test.discovery >/sys/kernel/config/nvmet/ports/10030/
addr_adrfam             addr_treq               addr_trtype             param_discovery_nqn     param_inline_data_size  referrals/
addr_traddr             addr_trsvcid            ana_groups/             param_enable_target     param_pi_enable         subsystems/
root at saturn:/# echo nqn.2014-08.org.test.discovery >/sys/kernel/config/nvmet/ports/10030/
addr_adrfam             addr_treq               addr_trtype             param_discovery_nqn     param_inline_data_size  referrals/
addr_traddr             addr_trsvcid            ana_groups/             param_enable_target     param_pi_enable         subsystems/
root at saturn:/# echo nqn.2014-08.org.test.discovery >/sys/kernel/config/nvmet/ports/10030/param_discovery_nqn 
root at saturn:/# nvme discover -t tcp -a 10.100.10.1 -s 4420 

Discovery Log Number of Records 2, Generation counter 6
=====Discovery Log Entry 0======
trtype:  tcp
adrfam:  ipv4
subtype: unrecognized
treq:    not specified, sq flow control disable supported
portid:  10120
trsvcid: 4420
subnqn:  nqn.2014-08.org.test.discovery
traddr:  10.100.10.1
sectype: none
=====Discovery Log Entry 1======
trtype:  tcp
adrfam:  ipv4
subtype: nvme subsystem
treq:    not specified, sq flow control disable supported
portid:  10120
trsvcid: 4420
subnqn:  nqn.2020-11.com.somesys:uuid:ubuntu-vg--ubuntu-lv
traddr:  10.100.10.1
sectype: none
root at saturn:/# nvme discover -t tcp -a 172.16.50.151 -s 4420 

Discovery Log Number of Records 2, Generation counter 6
=====Discovery Log Entry 0======
trtype:  tcp
adrfam:  ipv4
subtype: unrecognized
treq:    not specified, sq flow control disable supported
portid:  10030
trsvcid: 4420
subnqn:  nqn.2014-08.org.test.discovery
traddr:  172.16.50.151
sectype: none
=====Discovery Log Entry 1======
trtype:  tcp
adrfam:  ipv4
subtype: nvme subsystem
treq:    not specified, sq flow control disable supported
portid:  10030
trsvcid: 4420
subnqn:  nqn.2020-11.com.somesys:uuid:ubuntu-vg--ubuntu-lv
traddr:  172.16.50.151
sectype: none
root at saturn:/#


    So if you want to do something like this, you'd need to change the 
    implementation to have a discovery subsystem per port, and also change 
    the logic in drivers/nvme/target/discovery.c such that the discovery
    subsystems will return informations for _all_ configured subsystems.

>> I wonder if we even need a separate discovery subsystem per port. 
>> It adds code complexity for a feature that may not be very useful?

Thanks,

Mark


-------------- next part --------------
A non-text attachment was scrubbed...
Name: discover_example_v2.patch
Type: application/octet-stream
Size: 4136 bytes
Desc: discover_example_v2.patch
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20220301/728f6afb/attachment.obj>